Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #167 #168

Merged
merged 4 commits into from
Aug 15, 2020
Merged

Fix #167 #168

merged 4 commits into from
Aug 15, 2020

Conversation

alex-lew
Copy link
Contributor

@alex-lew alex-lew commented Aug 15, 2020

Thanks for maintaining this package!

This pull request does two things one thing:

  1. (Commit 1) Fix MySQL.setoptions sets at most one option #167 by changing the if ... elseif ... end block in MySQL.setoptions! to multiple independent if ... end blocks.

2. (Commits 2-3) Add a keyword argument to setoptions! for the enable_cleartext_plugin option (already supported behind the scenes via the existing API.MYSQL_ENABLE_CLEARTEXT_PLUGIN enum value). Let me know if you'd prefer this be a separate PR -- it was something I need for my use case and while I was inside the setoptions! function I thought I'd add it. (Also added a line documenting this to the DBInterface.connect docstring.)

-Alex

@codecov
Copy link

codecov bot commented Aug 15, 2020

Codecov Report

Merging #168 into master will decrease coverage by 0.06%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #168      +/-   ##
==========================================
- Coverage   58.40%   58.34%   -0.07%     
==========================================
  Files          10       10              
  Lines         952      953       +1     
==========================================
  Hits          556      556              
- Misses        396      397       +1     
Impacted Files Coverage Δ
src/MySQL.jl 63.41% <100.00%> (ø)
src/api/ccalls.jl 3.61% <0.00%> (-0.03%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 85bdb29...57eaef3. Read the comment docs.

src/MySQL.jl Outdated
@@ -249,6 +284,7 @@ Connect to a MySQL database with provided `host`, `user`, and `passwd` positiona
* `option_file::String`: the argument is interpreted as a path to a custom option file, and only that option file is read.
* `read_default_group::Bool`: only the default option groups are read from specified option file(s)
* `option_group::String`: it is interpreted as a custom option group, and that custom option group is read in addition to the default option groups.
* `enable_cleartext_plugin::Bool`: This option is supported to be compatible with MySQL client libraries. MySQL client libraries use this option to determine whether the mysql_clear_password authentication plugin can be used.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's the mariadb docs as to why I didn't originally include this option:

For MySQL compatibility, MariaDB Connector/C also allows applications to set the MYSQL_ENABLE_CLEARTEXT_PLUGIN option with the mysql_optionsv function. However, this option does not actually do anything in MariaDB Connector/C, because the mysql_clear_password client authentication plugin is always enabled for MariaDB clients and client libraries.

So yeah, I don't really mind including it here, but it sounds like it doesn't really do anything w/ mariadb client (which MySQL.jl uses under the hood)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@quinnj Got it, thank you! I'd thought adding it was what made my use case work, but it must have been something else -- I just tried without the option and it did work. Removing the extra code here.

@alex-lew alex-lew changed the title Fix #167 (+ new keyword arg. for enable_cleartext_plugin) Fix #167 Aug 15, 2020
Copy link
Member

@quinnj quinnj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much!

@quinnj quinnj merged commit 5a64b34 into JuliaDatabases:master Aug 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MySQL.setoptions sets at most one option
2 participants