Codecov Report

❌ Patch coverage is 0% with 14 lines in your changes missing coverage. Please review.

📢 Thoughts on this report? Let us know!

CodSpeed Performance Report

Merging #1829 will not alter performance

_{Comparing willswire:main (0e120ce) with main (3043aba)}

Summary

✅ 163 untouched

please review

Thanks, the approach seems good to me, with a suggestion on efficiency.

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.

Is it normal that underscores are as well encoded?

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?
The context of this is that when upgrading Pydantic the url builder broke, because previously we were encoding the special characters before calling build(), and this encodes those already encoded characters again.
Just my 2 cents, it was pretty confusing as we couldn't find any references in Pydantic.

Also, this closes #1467 too.

Should a change like this be mentioned in release notes of Pydantic?

It is mentioned in https://github.com/pydantic/pydantic/releases/tag/v2.12.1.

The context of this is that when upgrading Pydantic the url builder broke, because previously we were encoding the special characters before calling build()

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?

It is mentioned in https://github.com/pydantic/pydantic/releases/tag/v2.12.1.

Ah I see, didn't put 2 and 2 together, was looking for a subclass😅

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?

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 encode_credentials optional keyword argument would allow us to keep the logic and allow users who want the encoding to benefit?

I do not think we need to revert fully, instead an encode_credentials optional keyword argument would allow us to keep the logic and allow users who want the encoding to benefit?

Should we default to True or False? I think we can default to False until V3, and keep it after V3 even if the default would the be True, as some users may have already percent-encoded components.