dbcli
more accurate parsing of mysql configuration
Although my pull request does resolve issues with my particular configuration, it is not a general solution. My original intent was to provide a general solution but I acted in haste; sincerest apologies. At least it's a very localized change that's easy to undo. I would take no offense whatsoever if my pull request was reverted and my name thus removed from the contributor list.
To demonstrate:
from configobj import ConfigObj
value = "a, b, c"
lines = ["[client]", f"key = {value}"]
parsed = ",".join(ConfigObj(lines, list_values=True)["client"]["key"])
print(f" value = {value}")
print(f"parsed = {parsed}")
print(f" same? = {parsed == value}")
i.e. Intermediate blanks are stripped, so the mapping between parsed value (a list) and raw value (a string) is NOT invertible. I should have been more careful and perhaps pursued further my original intuition about initializing ConfigObj with list_values=False. I will try to do so when time permits.
In case someone else beats me to that, I'd like to offer a few tips:
- Be aware of this documented issue with
list_values=False:
ConfigObj doesn’t quote and unquote values if list_values=False. This means that leading or > trailing whitespace in values will be lost when writing. (Unless you manually quote).
With list_values=False, note that several values from myclirc currently are quoted, so those quotes would have to be removed somehow; e.g. colors section. More tricky would be something like the prompt key from main section which quotes a trailing blank.
-
For configuration files written by official mysql client, the goal should be to implement strictly compatible parsing behavior. Assuming configobj must remain, then one must test how the official mysql client parses at least the following:
- Values enclosed in matching quotes (double and single)
- Unquoted values surrounded by blanks
-
It might be better to have two different config parser implementations (or perhaps one implementation with two distinct configurations); i.e. one (read-only) for mysql configuration, and another (read/write) for mycli configuration (toml might be worth considering).
@dhx-mike-palandra Thanks for the detailed follow-up to your pull request. I think your pull request did just fine :thumbsup: and shouldn't have a negative side effect on the writing of configuration files.
As far as I remember, the code in your pull request was in a method that would only have affected reading. We don't use the method read_my_cnf_files() to read from the myclirc (which is the file we write to). We only use it to read from the following files:
-
/etc/my.cnf -
/etc/mysql/my.cnf -
/usr/local/etc/my.cnf -
~/.my.cnf -
~/.mylogin.cnf - possibly a different location based on environment variables and command-line arguments
The only data we are reading from those files that your solution would impact is:
- database
- host
- user
- password
- port
- socket
- default-character-set
- ssl-ca
- ssl-cert
- ssl-key
- ssl-cipher
- ssl-mode
- local-infile
- pager (the command itself)
- skip-pager
- local-infile
- loose-local-infile
As far as I can tell, none of them will be negatively impacted by your code change since we are only reading from those config files.
So, I think your pull request will serve as a fine bug fix. What do you think?
My solution is a partial one at best. For example, it permits users having passwords containing commas (but not spaces) to connect instead of receiving a cryptic error message. OTOH, users that have passwords containing a comma followed by a space might be in a worse position: they would be refused login by a MySQL server even if they entered their password correctly, and without any feedback from the actual problem source: mycli.
Of course, the full story is more complex. For example, I suspect that my patch can handle correctly passwords containing both commas and spaces but only if they are not adjacent, and only if the spaces are neither leading nor trailing. I'm not even certain if MySQL allows passwords containing spaces, and of course some other fields you've enumerated may allow values that require some extra care to handle with configobj (e.g. file path values).
I would like to contribute a better fix but it may take a while before I can do so. I use mycli only at work, and I completed my first attempt within a reasonable time frame during those hours. Currently, I must focus on other priorities at work, so I may have to save this for personal time which is fine. Most important thing is that I documented the problem thoroughly, so I can pick it up again when feasible.
@tsroten: I trust your judgment about handling the patch; i.e. keep or revert. I can see good reason in either choice. Thanks for being a friendly community member who offers constructive feedback. Take good care.
@dhx-mike-palandra I had some time today to give this some more thought. Thanks again for your attention to detail on this.
I think we are going to want to disable list_values. It will give us the most flexibility and allow us to respect what the user has typed. It should only require stripping matching quotation marks from values.
We never need to interpret the MySQL config files as having a list in each line. We also never need to write the data we read from these files.
If their password has leading or trailing quotation mark, then they can wrap their password in additional quotation marks. If someone has leading or trailing spaces in their password, they simply wrap their password in quotes (single or double). If they don't have leading/trailing spaces or quotation marks, then the password can be written with or without surrounding quotation marks.
As far as the other config keys in the list above, they'll be subject to the same rules, which should be a non-issue for nearly every user as leading/trailing spaces in those fields would be unusual (same with quotation marks).
I'll get a pull request submitted here shortly for this. Thanks again for your help in thinking this through 👍
Thank you very much for picking this up. Feel free to revert my original patch. IIRC, ConfigObj is initialized the same way both for MySQL client configuration and for MyCLI configuration, so if list_values will be disabled for all cases, the default MyCLI configuration file may need some adjustment to account for this change.
Good luck. Keep up the good work!
-----Original Message----- From: Thomas Roten [email protected] Reply-To: dbcli/mycli < [email protected]> To: dbcli/mycli [email protected] Cc: dhx-mike-palandra [email protected], Mention < [email protected]> Subject: Re: [dbcli/mycli] more accurate parsing of mysql configuration (#744) Date: Sun, 04 Aug 2019 16:07:00 -0700
@dhx-mike-palandra I had some time today to give this some more thought. Thanks again for your attention to detail on this.
I think we are going to want to disable list_values. It will give us the most flexibility and allow us to respect what the user has typed. It should only require stripping matching quotation marks from values.
We never need to interpret the MySQL config files as having a list in each line. We also never need to write the data we read from these files.
If their password has leading or trailing quotation mark, then they can wrap their password in additional quotation marks. If someone has leading or trailing spaces in their password, they simply wrap their password in quotes (single or double). If they don't have leading/trailing spaces or quotation marks, then the password can be written with or without surrounding quotation marks.
As far as the other config keys in the list above, they'll be subject to the same rules, which should be a non-issue for nearly every user as leading/trailing spaces in those fields would be unusual (same with quotation marks).
I'll get a pull request submitted here shortly for this. Thanks again for your help in thinking this through 👍
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.
Mike Palandra Systems Administrator | DHX Media [email protected] T: +1 416-977-7758 | M: +1 416-460-1033 Queen's Quay Terminal, 207 Queens Quay W., Suite 550 Toronto, ON, CANADA M5J 1A7
