Skip to content

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