Populate headers before making Authentication request #5
Conversation
Moving old commits with history --------- Co-authored-by: Paul Nienaber <[email protected]>
| FlightClient::Connect(location, client_options, &flight_client)); | ||
|
|
||
| PopulateMetadataSettings(properties); | ||
| PopulateCallOptions(properties); |
There was a problem hiding this comment.
This makes sense. The properties object never changes.
However there is a subtle behavioral change around the call options.
In the original implementation, call options added by the authentication method are added to the front of the headers list, then extra headers are added after. Now, the extra headers are first, then auth-generated headers are added after.
This likely matters for headers that conflict, which shouldn't really happen.
There was a problem hiding this comment.
IIRC in JDBC we separated out headers returned by authentication from headers supplied by the user, and when making requests, always combined them with auth-generated headers, user-headers second.
There was a problem hiding this comment.
ahhh yeah didn't think about ordering or headers returned by the server. That probably means we still also need to make the PopulateCallOptions call AFTER the auth call. Alternatively, do something smarter to ensure the correct ordering if you think that's an issue.
There was a problem hiding this comment.
Yeah I think what you might want to do is headers into "connection-string-supplied" vs. "auth-generated". Supply the connection-string-supplied ones to the auth functions.
Then when building the final set of headers, start with the auth-generated ones then the connection-string ones.
Wondering if there's a scenario where the auth-process can mutate the connection-string-supplied headers, but that can be solved later if we run into it.
There was a problem hiding this comment.
That makes sense! Any chance you can make those changes? I've never really worked with C++ and it's not immediately obvious how to make those changes.
There was a problem hiding this comment.
Hey guys, is there plan of shipping this PR? Also I am interested if you will publish a new ODBC Driver with those changes? We are stuck right now because of headers issue
Thank you
|
|
5 participants
We need the authentication request to include all of the headers that get set as connection properties as those can affect the authentication flow.
I'm not sure if moving this call has any unintended consequences.