RFC3986 compatible URL.join honoring empty segments #1039

commonism · 2024-07-18T18:26:31Z

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

Checklist

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

urllib.parse.urljoin does not honor empty segments, python/cpython#84774 implement using joinpath change query string generation to not emit empty values as ""

codecov · 2024-07-18T18:36:27Z

Codecov Report

Attention: Patch coverage is 73.49398% with 22 lines in your changes missing coverage. Please review.

Project coverage is 62.60%. Comparing base (84e1c7d) to head (f94589c).
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
tests/test_url.py	59.52%	17 Missing ⚠️
yarl/_url.py	87.80%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1039      +/-   ##
==========================================
+ Coverage   62.37%   62.60%   +0.23%     
==========================================
  Files          38       38              
  Lines        6626     6721      +95     
  Branches      356      368      +12     
==========================================
+ Hits         4133     4208      +75     
- Misses       2466     2486      +20     
  Partials       27       27

Flag	Coverage Δ
CI-GHA	`62.57% <73.49%> (+0.23%)`	⬆️
MyPy	`25.91% <39.02%> (+0.30%)`	⬆️
OS-Linux	`99.26% <100.00%> (+0.01%)`	⬆️
OS-Windows	`99.59% <100.00%> (+<0.01%)`	⬆️
OS-macOS	`99.03% <100.00%> (+0.01%)`	⬆️
Py-3.10.11	`98.94% <100.00%> (+0.04%)`	⬆️
Py-3.10.14	`99.15% <100.00%> (+0.04%)`	⬆️
Py-3.11.9	`99.15% <100.00%> (+0.04%)`	⬆️
Py-3.12.5	`99.15% <100.00%> (+0.04%)`	⬆️
Py-3.13.0-rc.1	`99.15% <100.00%> (+0.04%)`	⬆️
Py-3.8.10	`98.88% <100.00%> (+0.01%)`	⬆️
Py-3.8.18	`99.09% <100.00%> (+0.01%)`	⬆️
Py-3.9.13	`98.88% <100.00%> (+0.01%)`	⬆️
Py-3.9.19	`99.09% <100.00%> (+0.01%)`	⬆️
Py-pypy7.3.11	`99.40% <100.00%> (+<0.01%)`	⬆️
Py-pypy7.3.16	`99.43% <100.00%> (+<0.01%)`	⬆️
VM-macos-latest	`99.03% <100.00%> (+0.01%)`	⬆️
VM-ubuntu-latest	`99.26% <100.00%> (+0.01%)`	⬆️
VM-windows-latest	`99.59% <100.00%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

commonism · 2024-07-19T07:02:55Z

Towncrier ain't happy with my intersphinx link to python docs:

/home/runner/work/yarl/yarl/docs/[towncrier-fragments]:45: WARNING: py:meth reference target not found: python.urllib.parse.urljoin

commonism · 2024-07-19T19:41:44Z

As

:py:function:`urllib.parse.urljoin`

does not work either

/home/runner/work/yarl/yarl/docs/changes.rst:45: ERROR: Unknown interpreted text role "py:function".

But according to

python3 -m sphinx.ext.intersphinx https://docs.python.org/3/objects.inv | grep -E "(^py:.*|urllib.parse.urljoin)"
…
py:function
	urllib.parse.urljoin                     library/urllib.parse.html#urllib.parse.urljoin
…

this should work and the intersphinx is set up in the job

loading intersphinx inventory 'python' from https://docs.python.org/3/objects.inv...
loading intersphinx inventory 'multidict' from https://multidict.aio-libs.org/en/stable/objects.inv...

@Dreamsorcerer @webknjaz any ideas?

webknjaz · 2024-07-19T20:03:40Z

@commonism use :func:. That table shows object types with differs from the role names sometimes.

In my helper, I tried replacing them with proper reference hints: https://webknjaz.github.io/intersphinx-untangled

webknjaz · 2024-07-19T20:13:41Z

Towncrier ain't happy with my intersphinx link to python docs:

Technically, it's not Towncrier that's unhappy. Towncrier is just a fancy template renderer. But when that's injected into Sphinx (through my sphinxcontrib-towncrier extension), it can emit warnings and we turn them into errors.

commonism · 2024-07-19T21:51:21Z

The python domain maps function to func, therefore it is :py:func:
https://github.com/sphinx-doc/sphinx/blob/685e3fdb49c42b464e09ec955e1033e2a8729fff/sphinx/domains/python.py#L845-L881

To many docs for intersphinx show :py:function: as example.

That said, mastered this as well.

yarl/_url.py

tests/test_update_query.py

always encode parameters with an assignment adjust rfc3986 tests to include an assignment in the expectation

Dreamsorcerer

I think this looks right to me. @webknjaz Can you check it over too?

CHANGES/1039.bugfix.rst

yarl/_url.py

CHANGES/1039.bugfix.rst

tests/test_url.py

Co-authored-by: Sam Bull <aa6bs0@sambull.org>

bdraco · 2024-08-30T22:18:27Z

Ran a profile. Only thing I noticed is schema comes up a lot in yarl. Probably would make sense to put that one in the dict to and avoid the property call.. but thats another issue

Noticed this one comes up a lot while profiling #1039

bdraco · 2024-08-30T22:24:41Z

Tested one production. No adverse effect observed

bdraco · 2024-08-30T22:25:14Z

Thanks @commonism

#1039 introduced a regression in decoding query string params

bdraco · 2024-08-31T04:09:45Z

This PR introduced a regression in decoding query string params in joined urls.

I attempted to fix it in #1066 but the fix looks more involved so I'm going to revert this PR and do another release

I think this needs to be reworked to use URL(self._val._replace(.....), encoded=True) instead like some of the other methods but more analysis is needed as there is some complex re-quoting logic for query strings already.

I would like to encourage another attempt at solving this problem.

This reverts commit e3dd736.

…1067)

…1039)" (#1067)" This reverts commit e175d74.

commonism added 2 commits July 18, 2024 08:47

rfc3986 - URL.join

778b5ac

urllib.parse.urljoin does not honor empty segments, python/cpython#84774 implement using joinpath change query string generation to not emit empty values as ""

URL.join - align tests with empty parameters

aa570ff

commonism added 3 commits July 19, 2024 07:37

tests - URL.join with empty segments

eeacc09

CHANGES

efea232

CHANGES - links

14a0b18

commonism added 3 commits July 19, 2024 20:27

CHANGES - intersphinx ref

0fcb819

CHANGES - intersphinx

d9acbe2

CHANGES - intersphinx

b33a959

commonism added 2 commits July 19, 2024 23:32

CHANGES - no intersphinx

66b7f7f

CHANGES - intersphinx

d379d8f

commonism added 2 commits July 20, 2024 08:57

URL.join - uncrustify joining other.path

4979bd6

tests - adding cpython urljoin cases not covered yet

d4b689a

Dreamsorcerer reviewed Jul 22, 2024

View reviewed changes

yarl/_url.py Outdated Show resolved Hide resolved

Dreamsorcerer reviewed Jul 22, 2024

View reviewed changes

yarl/_url.py Outdated Show resolved Hide resolved

Dreamsorcerer reviewed Jul 22, 2024

View reviewed changes

tests/test_update_query.py Outdated Show resolved Hide resolved

commonism added 3 commits July 23, 2024 16:31

import at the top

68f2e55

query parameters - the other way around

f4c52dd

always encode parameters with an assignment adjust rfc3986 tests to include an assignment in the expectation

query - using query_string instead …

c1b8a54

Dreamsorcerer approved these changes Jul 24, 2024

View reviewed changes

CHANGES - no changes to parameters

e650ccf

webknjaz reviewed Jul 24, 2024

View reviewed changes

CHANGES/1039.bugfix.rst Outdated Show resolved Hide resolved

webknjaz reviewed Jul 24, 2024

View reviewed changes

CHANGES/1039.bugfix.rst Show resolved Hide resolved

webknjaz reviewed Jul 24, 2024

View reviewed changes

CHANGES/1039.bugfix.rst Outdated Show resolved Hide resolved

webknjaz reviewed Jul 24, 2024

View reviewed changes

CHANGES/1039.bugfix.rst Outdated Show resolved Hide resolved

Dreamsorcerer reviewed Aug 15, 2024

View reviewed changes

yarl/_url.py Show resolved Hide resolved

webknjaz reviewed Aug 15, 2024

View reviewed changes

CHANGES/1039.bugfix.rst Outdated Show resolved Hide resolved

webknjaz reviewed Aug 15, 2024

View reviewed changes

tests/test_url.py Outdated Show resolved Hide resolved

webknjaz reviewed Aug 15, 2024

View reviewed changes

tests/test_url.py Outdated Show resolved Hide resolved

webknjaz reviewed Aug 15, 2024

View reviewed changes

tests/test_url.py Outdated Show resolved Hide resolved

commonism added 4 commits August 15, 2024 14:53

lost change

4729518

as suggested

a93abd8

pycon test

531ff90

pycon - expected result

46bc760

Dreamsorcerer reviewed Aug 30, 2024

View reviewed changes

tests/test_url.py Outdated Show resolved Hide resolved

bdraco and others added 3 commits August 30, 2024 11:59

Update tests/test_url.py

5ef4396

Co-authored-by: Sam Bull <aa6bs0@sambull.org>

Merge branch 'master' into rfc3986-urljoin

defbc90

lint

f94589c

bdraco added a commit that referenced this pull request Aug 30, 2024

Make scheme a cached property as well

b64e96a

Noticed this one comes up a lot while profiling #1039

bdraco mentioned this pull request Aug 30, 2024

Make scheme a cached property as well #1058

Merged

bdraco approved these changes Aug 30, 2024

View reviewed changes

bdraco merged commit e3dd736 into aio-libs:master Aug 30, 2024
45 of 48 checks passed

bdraco added a commit that referenced this pull request Aug 31, 2024

Fix regression decoding query string params in join urls

578d94f

#1039 introduced a regression in decoding query string params

bdraco mentioned this pull request Aug 31, 2024

Add additional coverage for joining urls #1066

Merged

bdraco added a commit that referenced this pull request Aug 31, 2024

Revert "RFC3986 compatible URL.join honoring empty segments (#1039)"

0dc2211

This reverts commit e3dd736.

bdraco mentioned this pull request Aug 31, 2024

Revert "RFC3986 compatible URL.join honoring empty segments (#1039)" #1067

Merged

bdraco added a commit that referenced this pull request Aug 31, 2024

Revert "RFC3986 compatible URL.join honoring empty segments (#1039)" (#…

e175d74

…1067)

bdraco added a commit that referenced this pull request Aug 31, 2024

Revert "Revert "RFC3986 compatible URL.join honoring empty segments (#…

a858693

…1039)" (#1067)" This reverts commit e175d74.

bdraco mentioned this pull request Aug 31, 2024

Release 1.9.6 #1068

Merged

commonism deleted the rfc3986-urljoin branch September 2, 2024 05:23

commonism mentioned this pull request Sep 2, 2024

RFC3986 compatible URL.join honoring empty segments again #1082

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFC3986 compatible URL.join honoring empty segments #1039

RFC3986 compatible URL.join honoring empty segments #1039

commonism commented Jul 18, 2024 •

edited

Loading

codecov bot commented Jul 18, 2024 •

edited

Loading

commonism commented Jul 19, 2024

commonism commented Jul 19, 2024

webknjaz commented Jul 19, 2024

webknjaz commented Jul 19, 2024

commonism commented Jul 19, 2024

Dreamsorcerer left a comment

bdraco commented Aug 30, 2024

bdraco commented Aug 30, 2024

bdraco commented Aug 30, 2024

bdraco commented Aug 31, 2024 •

edited

Loading

RFC3986 compatible URL.join honoring empty segments #1039

RFC3986 compatible URL.join honoring empty segments #1039

Conversation

commonism commented Jul 18, 2024 • edited Loading

What do these changes do?

Are there changes in behavior for the user?

Related issue number

Checklist

codecov bot commented Jul 18, 2024 • edited Loading

Codecov Report

commonism commented Jul 19, 2024

commonism commented Jul 19, 2024

webknjaz commented Jul 19, 2024

webknjaz commented Jul 19, 2024

commonism commented Jul 19, 2024

Dreamsorcerer left a comment

Choose a reason for hiding this comment

bdraco commented Aug 30, 2024

bdraco commented Aug 30, 2024

bdraco commented Aug 30, 2024

bdraco commented Aug 31, 2024 • edited Loading

commonism commented Jul 18, 2024 •

edited

Loading

codecov bot commented Jul 18, 2024 •

edited

Loading

bdraco commented Aug 31, 2024 •

edited

Loading