sql_mode can be set in session, therefore we should look for ANSI_QUOTES in session variable instead of global variable

[SoledaD208]

…TES in session variable instead of global variable

SUMMARY

ISSUE TYPE

• Bugfix Pull Request
• Docs Pull Request
• Feature Pull Request
• New Module Pull Request

COMPONENT NAME

ADDITIONAL INFORMATION

[laurent-indermuehle]

Hi @SoledaD208 and thank you for taking the time to open a PR.

Before we can merge, it misses:

• A description that explains why the change is made.
• A change log fragment with the same explanation.
• Integration tests.

About tests : I'm not sure what will happen when you change the scope of the sql_mode from global to session for the mysql_role and mysql_user modules. A rapid grep showed that the line you modified is the only time we use sql_mode in the entire collection.

Also, while writing tests, you could demonstrate how you intent to set a session with set sql_mode='ANSI_QUOTES'". As I don't think community.mysql allow for this. But tests are a great way to experiment that.

If I'm right, we could modify your PR to add such a feature. Feel free to ask me for help executing tests. I'm also available on matrix : #mysql:ansible.com

[SoledaD208]

Hi Laurent, Thanks for your answer. I'm a bit busy these days so can't reply to you promptly. Before we can merge, it misses: Sure, I will spend some time doing those. A rapid grep showed that the line you modified is the only time we use sql_mode in the entire collection. Agreed with you. That's also what I can see from the code. you could demonstrate how you intent to set a session with set sql_mode='ANSI_QUOTES'". As I don't think community.mysql allow for this Oh, I don't know about that, let me try if I can. In a 'real life' situation, I can set sql_mode by making use of parameter session_vars or setting sql_mode in a client config file, and point the module to that file. By the way, not related to sql_mode, I think mysql_user cannot parse the role grants or partial revokes (Please correct if I'm wrong). E.g: - partial revoke: mysql> SHOW GRANTS FOR u1; +------------------------------------------+ | Grants for u1@% | +------------------------------------------+ | GRANT SELECT, INSERT ON *.* TO `u1`@`%` | | REVOKE INSERT ON `world`.* FROM `u1`@`%` | +------------------------------------------+ - role grants: mysql> SHOW GRANTS FOR 'dev1'@'localhost'; +-------------------------------- -----------------+ | Grants for ***@***.*** | +-------------------------- -----------------------+ | GRANT USAGE ON *.* TO `dev1`@`localhost` | | GRANT `app_developer`@`%` TO `dev1`@`localhost` | +------------------------- ------------------------+ If those types of grants are not able to be parsed yet. Would they be supported in newer versions of the module? Thanks, Tuan ( aka soledad208 :) )

…

------------------------------ *From:* Laurent Indermühle ***@***.***> *Sent:* Wednesday, September 11, 2024 2:29:06 PM *To:* ansible-collections/community.mysql < ***@***.***> *Cc:* Soledad208 ***@***.***>; Mention < ***@***.***> *Subject:* Re: [ansible-collections/community.mysql] sql_mode can be set in session, therefore we should look for ANSI_QUOTES in session variable instead of global variable (PR #677) Hi @SoledaD208 <https://github.com/SoledaD208> and thank you for taking the time to open a PR. Before we can merge, it misses: - A description that explains why the change is made. - A change log fragment with the same explanation. - Integration tests. About tests : I'm not sure what will happen when you change the scope of the sql_mode from global to session for the mysql_role and mysql_user modules. A rapid grep showed that the line you modified is the only time we use sql_mode in the entire collection. Also, while writing tests, you could demonstrate how you intent to set a session with set sql_mode='ANSI_QUOTES'". As I don't think community.mysql allow for this. But tests are a great way to experiment that. If I'm right, we could modify your PR to add such a feature. Feel free to ask me for help executing tests. I'm also available on matrix : #mysql:ansible.com <https://matrix.to/#/%23mysql:ansible.com> — Reply to this email directly, view it on GitHub <#677 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACCTXOXYG6L7TUPBQ7ZUN43ZV7WMFAVCNFSM6AAAAABN4CGCSSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNBSHA3TANJUGI> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[Andersson007]

folks any thoughts on ^?

[laurent-indermuehle]

Hi @SoledaD208,
Could you provide examples of when sql_mode='ANSI_QUOTES' is useful please? Then I can help you with the integration tests. You already talked about table names in the priv fields of mysql_user. But are there other places where this would be useful? Like in a query, for instance?

For your question about the role/revoke/read of privileges, we should keep discussions separate. Maybe in a new issue. But before doing so, please use the mysql_info module with the users and users_info filters. Maybe they can do what you want. Also, keep in mind that MySQL and MariaDB treat roles differently. So please provide the engine and version you're using when you file a bug. This helps to better understand the issue.

[SoledaD208]

Could you provide examples of when sql_mode='ANSI_QUOTES' is useful please?

if you ask about the bad effects to community.mysql module. Incorrect quotation in store procedure and function (not tables) will make the module always see the privs on function/SP are the new ones. As consequence, it will try to revoke the existing privs and re-grant them. Not only mysql_user, mysql_role also import the function privileges_unpack: , thus it will also affect by ANSI_QUOTES.

About the general purpose of ANSI_QUOTES. Afaik, ANSI_QUOTES is the standard SQL (which Mysql, by default, doesn't follow). So without enabling that mode, if you dump some data from Postresql or other RDBMS to a sql file, then run it in Mysql, the sql dump files will fail.

sql_mode='ANSI_QUOTE' or any session's variable can be easily set by making use of community.mysql's session_vars argument. The ansible module allows users to freely set or unset ANSI_QUOTES per session. Due to Bug #115953 , that feature is really needed for mysql_user and mysql_role to work properly, but for now it's not working correctly. The reason is it's extracting ANSI_QUOTES from global sql_mode instead of session

[SoledaD208]

please provide the engine and version you're using

I'm using Mysql 8. Mysql 5.7 doesn't have partial revokes or RBAC, so there's no issue with the old version.
It looks like MariaDB does not have such feature, so we may not have issue with MariaDB

For your question about the role/revoke/read of privileges, we should keep discussions separate

Agreed with you, I will open another issue. mysql_user, mysql_role and also mysql_info will break if partial revoke is used.

[laurent-indermuehle]

Very good tests @SoledaD208.
I'm wondering if the failed tests are because of your change. It is related to roles so it could be.
I'll need to study how the "params + persist" works. I never used it before.
I'm busy on something else, but remember you can asks for help if the tests/github action or else bother you.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.96%. Comparing base (28bf709) to head (8ef32e5).
Report is 1 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff             @@
##             main     #677       +/-   ##
===========================================
+ Coverage   25.74%   76.96%   +51.21%     
===========================================
  Files          32       20       -12     
  Lines        2808     2657      -151     
  Branches      704      679       -25     
===========================================
+ Hits          723     2045     +1322     
+ Misses       2044      413     -1631     
- Partials       41      199      +158     

Flag	Coverage Δ
integration	76.32% <100.00%> (?)
sanity	?
units	?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[SoledaD208]

thanks @laurent-indermuehle indeed my "params + persist" messed up the tests.
Reason is when persist is used, the sql_mode's value will be written down to the container's .my.conf therefore it affects the other tests. I added Ansible tasks to reconfigure sql_mode back to the original value after the test succeeds. Let's see if all the tests get passed...

[SoledaD208]

The tests went well for Mysql, but failed for Mariadb. Apparently, it's because in Mariadb, users can't persist sql_mode to config file like what they can do in Mysql 8. I had changed the testing playbook a bit: mode: "{% if db_engine == 'mariadb' %}global{% else %}persist{% endif %}" , then thanks god, all good now.

[laurent-indermuehle]

@SoledaD208 great job! Are you ready for a review or you want to add more tests?

Also, please add a changelogs / framents file to describe what you're changing and why.

[Andersson007]

@SoledaD208 thanks for the persistent work on the improvement!

Yep, as @laurent-indermuehle spotted we need a fragment here https://docs.ansible.com/ansible/latest/community/development_process.html#creating-a-changelog-fragment

[Andersson007]

Suggested change

	register: sql_mode_orig
	register: sql_mode_orig

would you like to assert: this one too? I see you assert the second one called result

[SoledaD208]

Thanks, let me check and add the assert. by the way, due to some reasons, I will need to close this PR and open another via other account.