Skip to content

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
@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 (Святослав Сидоренко) <[email protected]>
Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) <[email protected]>
Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) <[email protected]>
Co-authored-by: J. Nick Koston <[email protected]>
Co-authored-by: J. Nick Koston <[email protected]>
@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

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

It was not easy, but a pleasure.

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