Skip to content

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

Merged
samuelcolvin merged 10 commits intopydantic:masterfrom
faresbakhit:master
Aug 19, 2022
Merged

Fix AnyUrl.build doesn't do percent encoding (#3061)#4224
samuelcolvin merged 10 commits intopydantic:masterfrom
faresbakhit:master

Conversation

@faresbakhit
Copy link
Copy Markdown
Contributor

@faresbakhit faresbakhit commented Jul 10, 2022 •
edited
Loading

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
Copy link
Copy Markdown
Contributor Author

faresbakhit commented Jul 11, 2022

please review

samuelcolvin
samuelcolvin reviewed Aug 8, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread pydantic/utils.py Outdated
Comment thread pydantic/utils.py Outdated
@samuelcolvin
Copy link
Copy Markdown
Member

samuelcolvin commented Aug 8, 2022

please update.

@github-actions github-actions Bot added awaiting author revision awaiting changes from the PR author and removed ready for review labels Aug 8, 2022
@faresbakhit
Copy link
Copy Markdown
Contributor Author

faresbakhit commented Aug 10, 2022

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
Copy link
Copy Markdown
Member

samuelcolvin commented Aug 11, 2022

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
Copy link
Copy Markdown
Contributor Author

faresbakhit commented Aug 13, 2022 •
edited
Loading

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:

Python vs Rust percent encoding benchmark

This is a 12%ish increase

samuelcolvin
samuelcolvin reviewed Aug 14, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread pydantic/networks.py Outdated
path: Optional[str] = None,
query: Optional[str] = None,
fragment: Optional[str] = None,
plus: bool = False,
Copy link
Copy Markdown
Member

@samuelcolvin samuelcolvin Aug 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
plus: bool = False,
quote_plus: bool = False,

might be a clearer name.

Copy link
Copy Markdown
Member

@samuelcolvin samuelcolvin Aug 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

@samuelcolvin samuelcolvin Aug 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add quote_plus to stricturl

Copy link
Copy Markdown
Contributor Author

@faresbakhit faresbakhit Aug 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Just conforming to the standard (RFC 3986)

Comment thread changes/3061-FaresAhmedb.md Outdated
Comment thread pydantic/networks.py Outdated
@samuelcolvin
Copy link
Copy Markdown
Member

samuelcolvin commented Aug 14, 2022

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.

Fares Ahmed and others added 3 commits August 15, 2022 12:35
@faresbakhit
Copy link
Copy Markdown
Contributor Author

faresbakhit commented Aug 15, 2022

please review

@pydantic-hooky pydantic-hooky Bot added ready for review and removed awaiting author revision awaiting changes from the PR author labels Aug 15, 2022
samuelcolvin
samuelcolvin reviewed Aug 16, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread docs/usage/types.md Outdated
Comment thread docs/usage/types.md Outdated
@pydantic-hooky pydantic-hooky Bot added awaiting author revision awaiting changes from the PR author and removed ready for review labels Aug 16, 2022
faresbakhit and others added 2 commits August 17, 2022 04:13
@faresbakhit @samuelcolvin
Fix precent encoding version note to V1.10
31f81bd 
Co-authored-by: Samuel Colvin <samcolvin@gmail.com>
@faresbakhit @samuelcolvin
Fix Building URLs snippit markdown
f733f30 
Co-authored-by: Samuel Colvin <samcolvin@gmail.com>
@faresbakhit
Copy link
Copy Markdown
Contributor Author

faresbakhit commented Aug 17, 2022

please review

@pydantic-hooky pydantic-hooky Bot added ready for review and removed awaiting author revision awaiting changes from the PR author labels Aug 17, 2022
@samuelcolvin samuelcolvin enabled auto-merge (squash) August 19, 2022 09:02
@samuelcolvin
Copy link
Copy Markdown
Member

samuelcolvin commented Aug 19, 2022

Thanks so much.

@samuelcolvin samuelcolvin merged commit e34ff92 into pydantic:master Aug 19, 2022
@JPBergsma JPBergsma mentioned this pull request Sep 2, 2022
Closed
samuelcolvin added a commit that referenced this pull request Sep 2, 2022
@samuelcolvin
Revert "Fix AnyUrl.build doesn't do percent encoding (#3061) (#4224)"
5d6b36b 
This reverts commit e34ff92.
This was referenced Sep 2, 2022
Merged
Closed
Closed
samuelcolvin added a commit that referenced this pull request Sep 5, 2022
@samuelcolvin
Revert percent encoding in URLs. (#4470)
a4367c1 
* Revert "Fix `AnyUrl.build` doesn't do percent encoding (#3061) (#4224)"

This reverts commit e34ff92.

* change and warning
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@samuelcolvin samuelcolvin samuelcolvin left review comments

Assignees

@hramezani hramezani

@samuelcolvin samuelcolvin

@PrettyWood PrettyWood

Labels

ready for review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

*Url.build functions doesn't percent-encode arguments

4 participants

@faresbakhit @samuelcolvin @hramezani @PrettyWood