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

RFC3986 compatible URL.join honoring empty segments again #1082

Merged
merged 89 commits into from
Sep 4, 2024

Conversation

commonism
Copy link
Contributor

This is a revised version of #1039 using encoded arguments to work around issues with + in query segments.
It uses encoded=True for the parts when building the URL and normalizes the path itself.

What do these changes do?

The changes re-implement URL.join within yarl.URL instead of using urllib.parse.urljoin to avoid known issues when joining paths with empty segments (python/cpython#84774)

Are there changes in behavior for the user?

Due to the unit tests for URL.join it was required to align the query parameter rendering to match the unit tests, empty parameters are rendered without value therefore instead of an empty value.

Related issue number

#1066

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes

@psf-chronographer psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Sep 2, 2024
yarl/_url.py Outdated Show resolved Hide resolved
@bdraco
Copy link
Member

bdraco commented Sep 2, 2024

Thanks for following up 👍

commonism and others added 27 commits September 2, 2024 07:53
urllib.parse.urljoin does not honor empty segments,
python/cpython#84774

implement using joinpath

change query string generation to not emit empty values as ""
always encode parameters with an assignment
adjust rfc3986 tests to include an assignment in the expectation
Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) <wk.cvs.github@sydorenko.org.ua>
Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) <wk.cvs.github@sydorenko.org.ua>
Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) <wk.cvs.github@sydorenko.org.ua>
Co-authored-by: J. Nick Koston <nick@koston.org>
Co-authored-by: J. Nick Koston <nick@koston.org>
@bdraco
Copy link
Member

bdraco commented Sep 4, 2024

Pushed a fix for the new test cases.

If there are any other cases to test, that you can think of that might break it, it would be great to add them. I think we are a bit light on tests for joining encoded paths.

I've got to get some sleep now.

@commonism
Copy link
Contributor Author

Pushed a fix for the new test cases.

possible without make_child

@bdraco
Copy link
Member

bdraco commented Sep 4, 2024

Pushed a fix for the new test cases.

possible without make_child

Nice!

Now I really have to get some sleep. Cheers

tests/test_url.py Outdated Show resolved Hide resolved
@bdraco
Copy link
Member

bdraco commented Sep 4, 2024

I think this is ready to go. Some small nits above, and maybe there’s more coverage to add?

@bdraco
Copy link
Member

bdraco commented Sep 4, 2024

Screenshot 2024-09-04 at 11 46 00 AM

@bdraco
Copy link
Member

bdraco commented Sep 4, 2024

I added some more coverage and cleaned up the message a bit

@bdraco
Copy link
Member

bdraco commented Sep 4, 2024

I should have time to do another release for this fix later today

@bdraco
Copy link
Member

bdraco commented Sep 4, 2024

Still doing testing with this, but I think its good to go if the CI passes and the performance testing looks ok

@bdraco
Copy link
Member

bdraco commented Sep 4, 2024

Performance testing looks good.

@bdraco
Copy link
Member

bdraco commented Sep 4, 2024

Thanks for sticking with this one @commonism

@bdraco bdraco enabled auto-merge (squash) September 4, 2024 23:04
@bdraco bdraco merged commit af585b2 into aio-libs:master Sep 4, 2024
47 of 49 checks passed
@commonism
Copy link
Contributor Author

It was not easy, but a pleasure.

bdraco added a commit that referenced this pull request Sep 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot:chronographer:provided There is a change note present in this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants