Fix AnyUrl.build doesn't do percent encoding (#3061)

[faresbakhit]

Change Summary

• Add percent_encode function to pydantic.utils
• Add tests for percent_encode
• Fix AnyUrl.build
• Add new tests for AnyUrl.build

Related issue number

fix #3061

Checklist

• Unit tests for the changes exist
• Tests pass on CI and coverage remains at 100%
• Documentation reflects the changes where applicable, not applicable
• changes/<pull request or issue id>-<github username>.md file added describing change
• My PR is ready to review

[faresbakhit]

please review

[samuelcolvin]

Looks good in principle, can we get some benchmarks to see how much this slows down validation

[samuelcolvin]

please update.

[faresbakhit]

Looks good in principle, can we get some benchmarks to see how much this slows down validation

I'm not sure how this affects validation or how to benchmark pydantic's validation, can you assist me on this?

[samuelcolvin]

Thanks so much for the benchmarks, I think we should go with from urllib.parse import quote_plus for the following reasons:

• keep it simple, minimise our own logic
• I would guess most URLs will be short enough that's quote_plus will be marginally quicker

I'm not sure how this affects validation or how to benchmark pydantic's validation, can you assist me on this?

I think we can skip this, we have to get this right, so there's no option of not quote encoding.

The real question long terms is whether we should move this logic to rust, but provided the API doesn't change we could do that in a minor release after V2.

[faresbakhit]

Okay, I removed pydantic.utils.percent_encode in the latest commit.

The real question long terms is whether we should move this logic to rust, but provided the API doesn't change we could do that in a minor release after V2.

I think this should be moved to rust since urllib.parse is just pure python.

I will drop some benchmarks later for urllib.prase.quote against percent-encoding and urlencoding

Edit:

Here's the benchmark, code:

This is a 12%ish increase

[samuelcolvin]

Looking good, my main concerns are:

• this could in theory be considered a breaking change, I think it's fine because it's pretty clear than without this the built URLs are "wrong", @PrettyWood do you agree?
• there's no way to set quote_plus on a URL field, other than stricturl - maybe that's fine?

please can you add to the docs:

• An example of this in action
• an example with stricturl of using quote_plus=False (or whatever the non-default is)
• a "note" admonition saying "percent encoding was added in V1.10 ..."

Please update.

[samuelcolvin]

Suggested change

	plus: bool = False,
	quote_plus: bool = False,

might be a clearer name.

[samuelcolvin]

Also, is there a good reason not to have quote_plus = True as the default?

[samuelcolvin]

Please add quote_plus to stricturl

[faresbakhit]

Also, is there a good reason not to have quote_plus = True as the default?

Just conforming to the standard (RFC 3986)

[samuelcolvin]

Also, @FaresAhmedb please remember to use the magic comment "please review" (with one space) to request a review, otherwise we might miss updates on the PR.

[faresbakhit]

please review

[samuelcolvin]

Looking good, just a few small things here, but you need to fix linting in the docs so build succeeds and we can check the docs look right.

Please update.

[faresbakhit]

please review

[samuelcolvin]

Thanks so much.