Fix/Invalid uri created when extras contains non string elements #57969
Conversation
|
@Lee-W @jason810496 Could you please take a look at this PR when you have a moment? |
| uri += ("?" if self.schema else "/?") + query | ||
| else: | ||
| extras_dict = self.extra_dejson | ||
| has_unserializable = any(v in (None, "") for v in extras_dict.values()) |
There was a problem hiding this comment.
I'm not sure I understand this line. Could you please elaborate a bit?
There was a problem hiding this comment.
thanks for the review
I added that check to basically catch values that contain none query parameters from the URI
i can add comments explaining that or just remove that if we solely want to rely on stringified data for safety
| if query and self.extra_dejson == dict(parse_qsl(query, keep_blank_values=True)): | ||
| uri += ("?" if self.schema else "/?") + query | ||
| else: | ||
| extras_dict = self.extra_dejson |
There was a problem hiding this comment.
This assignment is not needed
| extras_dict = self.extra_dejson |
| json.dumps(v) | ||
| if isinstance(v, (dict, list)) | ||
| else str(v).lower() | ||
| if isinstance(v, bool) | ||
| else "null" | ||
| if v is None | ||
| else str(v) |
There was a problem hiding this comment.
Let's turn this into a function instead. it's a bit hard to understand
jason810496
left a comment
jason810496
left a comment
There was a problem hiding this comment.
It seems this PR only change the unit test instead of fixing the source.
Do we push the wrong branch for this PR?
|
Thanks for the review — I’m still working on it locally. I’ve made some progress and should be able to push the updated changes tomorrow. |
Pyasma
force-pushed
the
Fix/ConnectiongetURI
branch
2 times, most recently
from
00ae537 to
3932c62
Compare
|
Will this fix be released soon? |
Depends when it will get green, approved, merged and relesed. In open-source "soon" might mean many things and generally the answer "when" is "when ready". People contribute to open source when they can - and sometimes it's faster, sometimes slower. But if you would like to speed it up - doing reviews or even asking to take over the PR and get it merged (i.e. green and passing review) quickly is a better way that asking "when". Highly recommended. |
There was a problem hiding this comment.
Thanks for the fix!
So It seem _parse_from_uri might not be right place to fix. get_extra_dejson might be a better place to fix, as the self.extra_dejson property will call get_extra_dejson always with nested = False.
Additionally, we could implement the stringify logic inside the get_extra_dejson (along with the existed if isinstance(value, str): condition for list, dict, bool , etc.
| self.extra = json.dumps(query) | ||
| decoded = {k: self._maybe_json_load(v) for k, v in query.items()} | ||
| self.extra = json.dumps(decoded) |
There was a problem hiding this comment.
_parse_from_uri will only be called if we provide uri (line 170), but the problem described in the issue is only related to extra and the uri it unset.
There was a problem hiding this comment.
Thanks for the Review sorry for the Delay I was busy with something forgot to push changes I will Push the new changes Today
I agree with you. I found info on chat that some changes are created internally and I am only asked for timeline - not to speed up. |
Pyasma
force-pushed
the
Fix/ConnectiongetURI
branch
from
a0fed51 to
3966d0a
Compare
5 participants
This PR fixes how Connection.get_uri() handles extras with non-string values (floats, booleans, nested dicts).
Extras are now stringified before encoding to produce valid, SQLAlchemy-compatible URIs.
Added a unit test test_get_uri_with_non_string_extras to verify the change.
All pre-commit checks passed.
Issue :#55893