-
Notifications
You must be signed in to change notification settings - Fork 312
fix: encode credentials in MultiHostUrl builder #1829
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
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
CodSpeed Performance ReportMerging #1829 will not alter performanceComparing Summary
|
|
please review |
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.
Thanks, the approach seems good to me, with a suggestion on efficiency.
Co-authored-by: David Hewitt <mail@davidhewitt.dev>
Reverted the suggestion due to failing tests |
|
Thanks for the contribution @willswire. @davidhewitt if there's no risk of regressions with this one, we can ship it as a 2.12 backport as well. |
…SQLAlchemy URL as interpolation markers. Appeared becaused Pydantic started to percent-encode user credentials in MultiHostUrl: pydantic/pydantic-core#1829
…SQLAlchemy URL as interpolation markers. Appeared becaused Pydantic started to percent-encode user credentials in MultiHostUrl: pydantic/pydantic-core#1829
…SQLAlchemy URL as interpolation markers. Appeared becaused Pydantic started to percent-encode user credentials in MultiHostUrl: pydantic/pydantic-core#1829
|
Is it normal that underscores are as well encoded? |
…re#1829) Co-authored-by: David Hewitt <mail@davidhewitt.dev> Original-commit-link: pydantic/pydantic-core@1e1c962
|
Possibly needs tuning back a bit, see https://docs.rs/percent-encoding/latest/percent_encoding/constant.NON_ALPHANUMERIC.html - says it probably encodes too much. |
|
Should a change like this be mentioned in release notes of Pydantic? Also, this closes #1467 too. |
It is mentioned in https://github.com/pydantic/pydantic/releases/tag/v2.12.1.
Thinking about it, this is indeed inconvenient for users already URL-encoding the components. @davidhewitt thoughts? Should we actually revert the change altogether and reconsider for v3? |
Ah I see, didn't put 2 and 2 together, was looking for a subclass😅 |
I think it's correct to see that this should have been a feature rather than a bugfix. This was a mistake on us in assessing the original report, I think. I do not think we need to revert fully, instead an |
Should we default to |
Change Summary
even with reserved characters.
userinfoencoding logic.Related issue number
Checklist
reviewers
Selected Reviewer: @Viicos