-
Notifications
You must be signed in to change notification settings - Fork 63
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
remove user and password from DSN record #305
Conversation
Codecov Report
@@ Coverage Diff @@
## master #305 +/- ##
==========================================
+ Coverage 78.24% 78.42% +0.17%
==========================================
Files 6 6
Lines 754 760 +6
==========================================
+ Hits 590 596 +6
Misses 164 164
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm in favor of these changes. It seems you're probably still working on stuff right now, right? For example, I don't see the user/password fields removed from the Connection
struct yet?
The extraauth
stuff seems fine to me as well. I get a little nervous changing some of that stuff just because (as you may know) differences between drivers can be pretty finicky. But in general, the changes you have there look fine.
Just ping me here once you think this is ready to review/merge!
src/API.jl
Outdated
# however there seems no standard way to escape PWD=mypass}hasacurly. (SQL server wants double }}, but other drivers differ). | ||
function getextraauth(usr::Union{AbstractString, Nothing}, pwd::Union{AbstractString, Nothing}, extraauth::Union{AbstractString, Nothing}) :: String | ||
return join([ | ||
usr === nothing ? "" : "UID={$usr}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is more escaping that we had before! So I say we run w/ this for now; the nice thing is that people can always just pass the whole connection string themselves if they want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
woop. I did have one thought on this - my change is technically a breaking change for any user who is currently passing their password manually like connect(dsn, "root", "{im escaping my pwd myself}")
. Do you have any thoughts on that? it's a bit of an edge case and I can't see any issues discussing it so maybe no-one has hit it yet.
thanks for the response, yep I had finished at that point but will fix up your review points in a little bit. I am a bit confused by
Have I missed/misunderstood something? I preserved Connection(dsn::AbstractString, usr, pwd) = Connection(dsn; user=usr, password=pwd) to not break your API but as far as i can tell |
f696c7f
to
7f339f9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good; thanks!
Hey,
This PR removes auth stuff from the DSN record of the Connection struct. Instead it is passed in at connection time them forgotten. Two rationales:
Additionally, it allows me to add an extraauth parameter which is appended as-is and likewise treated as sensitive info. This will be database specific, and could be used for e.g. auth tokens, Snowflake warehouse specification, etc etc, basically things for where my code's connection string needs to be the same per-user, but users may still need to put account-specific config to get a DB connection. (This model actually follows how PowerBI models ODBC connections, and I figure that they have done a bit more solution design across different DBs than I have :) )
Added public API:
Tests added and checked against MariaDB locally (we'll see what Travis says).. I will do further testing if Travis passes.
Any and all feedback welcome, this is my first public work in Julia so there are probably style issues at the very least.