SNOW-892284: Fix boolean parameter parsing from URL query #439
Conversation
|
@sfc-gh-stan have you had a chance to look at the PR already? |
|
@sfc-gh-stan The CI run found a small bug, fixed it now and added a note to the changelog. |
|
@sfc-gh-stan Another fix, messed up the default case |
| @@ -226,10 +226,27 @@ def create_connect_args(self, url): | |||
| opts["host"] = opts["host"] + ".snowflakecomputing.com" | |||
| opts["port"] = "443" | |||
| opts["autocommit"] = False # autocommit is disabled by default | |||
| opts.update(url.query) | |||
|
|
|||
| cache_column_metadata = opts.pop("cache_column_metadata", None) | |||
There was a problem hiding this comment.
What about setting using "False" as the default value and set self._cache_column_metadata = parse_url_boolean(opts.pop("cache_column_metadata", "False"))?
There was a problem hiding this comment.
It's a minor difference, I just prefer the code to not do parsing on constants if possible.
|
@sfc-gh-stan Another fix - the |
|
@sfc-gh-stan hey, could we get this through? I feel like this should be / is very close to working. |
|
These lines is now whats causing issues (as they are unknown parameters). I guess passing arbitrary arguments is not prohibited, I think raising a warning makes the most sense it that case. |
|
Adding warnings fails the SQLAlchemy test suite (it's set to error out on warnings) so for now I added a fallthrough without warnings. That can be a subject of a different PR as more investigation on how to set that up is needed. |
|
Hi @saulbein, I resolved a rebase conflict and fixed some linting to kick off the merge gates in #446. The merge gates passed. I tried pushing the changes to saulbein:main but that seemed to have closed this PR. Sorry for the inconveniences. Would you mind merging the changes from #446 instead or creating a new PR? |
Please answer these questions before submitting your pull requests. Thanks!
What GitHub issue is this PR addressing? Make sure that there is an accompanying issue to your PR.
Fixes SNOW-892284: Boolean URL connection parameters are passed down to the driver as strings #438
Fill out the following pre-review checklist:
Please describe how your code solves the related issue.
Re-parses the booleans that become stringified by SQLAlchemy using the DEFAULT_PARAMETERS map from the connector that lists which parameters are boolean-typed. Not sure what to do with other types, such as integers, dicts or callables - I'm also not sure those work right now either way.