Skip to content
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

Revert percent encoding in URLs. #4470

Merged
merged 2 commits into from Sep 5, 2022
Merged

Revert percent encoding in URLs. #4470

merged 2 commits into from Sep 5, 2022

Conversation

samuelcolvin
Copy link
Member

This reverts #4224.

fix #4458, fix #4468. Replaces #4469 and #4461.

There are a number of problems with #4224:

  1. It re-encodes existing percent encoded URLs, this was reported in AnyUrl escapes percent sign in URLs without TLD that are already escaped. #4458 and could be fixed by Fix existing percent encoding #4469
  2. It wrongly encoded /, this could also be fixed by Fix existing percent encoding #4469
  3. It wrongly encoded +, this was reported in AnyUrl percent encoding (#3061) introduced v1.10.0 breaks database connection string #4468 and could also be fixed by Fix existing percent encoding #4469
  4. It would require us to rebuild virtually all URLs, currently we only rebuild URLs with "international" domains, if I enable rebuild for all URL validation, lots of tests fail, fixing those tests involves significant changes and we're well into the "this is not really suitable for a patch release" territory.

Therefore after some consideration and discussion with other pydantic maintainers (and with a heavy heart), I've decided the best approach is to remove percentage encoding from URL logic until V2.

Long term (e.g. in V2) I think we should use an external library (either in python or rust) to perform URL parsing and validation.

@samuelcolvin
Copy link
Member Author

please review.

Copy link
Member

@hramezani hramezani left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@samuelcolvin samuelcolvin mentioned this pull request Sep 2, 2022
6 tasks
@samuelcolvin samuelcolvin merged commit a4367c1 into main Sep 5, 2022
@samuelcolvin samuelcolvin deleted the revert-4224 branch September 5, 2022 10:02
@gmanny
Copy link

gmanny commented Sep 5, 2022

Sorry if this isn't the place to write this, but the now-reverted change was also what caused our DB connectivity problems because we used PostgresDsn to create a connection string for SQLAlchemy and the name of unix socket contained : characters.

Was very hard to debug because on Google Cloud Run you only get File not found and a vague error log about the URL-encoded name of the DB not being reachable. I think, we may not be alone with this, so it's worth releasing this fix as fast as possible.

@samuelcolvin
Copy link
Member Author

agreed, it'll be out today, I'm wait for one PR, but if it's not in soon, I'll release.

@samuelcolvin
Copy link
Member Author

For anyone interested, I've just implemented URL parsing as part of pydantic-core for V2, see pydantic/pydantic-core#317.

Feedback welcome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants