Skip to content

Dynamic routes with percent-encodable fixed segments (space, non-ASCII) are unreachable and url_for( - #13498

Open
loonister1 wants to merge 3 commits into
aio-libs:masterfrom
loonister1:fix/13433-dynamic-routes-with-percent-encodable-fi
Open

Dynamic routes with percent-encodable fixed segments (space, non-ASCII) are unreachable and url_for(#13498
loonister1 wants to merge 3 commits into
aio-libs:masterfrom
loonister1:fix/13433-dynamic-routes-with-percent-encodable-fi

Conversation

@loonister1

@loonister1 loonister1 commented Aug 19, 2026

Copy link
Copy Markdown

What do these changes do?

DynamicResource and PrefixResource percent-encoded their fixed path segment at
registration and used that encoded form for both the match pattern and canonical,
while UrlDispatcher.resolve walks the decoded path_safe and derives index keys
from canonical — so those resources were never probed and always returned 404.

The fix keeps the encoded form only for URL construction (_formatter, and a new
_quoted_prefix on PrefixResource) and uses the as-written form for matching and
canonical, which is the representation PlainResource already used and the router
already matches on. _get_resource_index_key and resolve are unchanged.

Are there changes in behavior for the user?

Yes, two.

1. Routes that were silently unreachable now resolve. Before this change a dynamic
route /hello world/{name}, a static prefix /static files, and a sub-application
mounted at /sub app returned 404 for every request. They now match, and the URL
built by url_for() routes back to its own resource.

2. Route metadata reports the registered form instead of the percent-encoded one:

before after
DynamicResource.canonical /hello%20world/{name} /hello world/{name}
PrefixResource.canonical (static, sub-app) /static%20files /static files
get_info()['prefix'] /static%20files /static files
DynamicResource.get_info()['formatter'] /hello%20world/{name} unchanged

formatter deliberately stays percent-encoded — it is what url_for() builds from.
Third-party instrumentation that labels routes by resource.canonical (OpenTelemetry,
Prometheus) will observe the new representation. This is documented under each affected
attribute in docs/web_reference.rst and called out in the change note.

Is it a substantial burden for the maintainers to support this?

No new public API and no new configuration. It removes an inconsistency rather than
adding one: matching and canonical now consistently use the as-written representation
while URL building consistently uses the percent-encoded one, instead of mixing the two.
The single new attribute, PrefixResource._quoted_prefix, is private.

Related issue number

Fixes #13433

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes
  • If you provide code modification, please add yourself to CONTRIBUTORS.txt
  • Add a new news fragment into the CHANGES/ folder

Notes for reviewers

  1. versionchanged directive version is a guess. docs/web_reference.rst uses
    .. versionchanged:: 3.14.4 on the assumption that this .bugfix gets backported to
    the 3.14 line, making 3.14.4 the earliest release containing it. If you label it
    minor-only, it should read 3.15 — one sed either way, just say which.

  2. Known residual gap, matching existing behaviour. A fixed part containing a literal
    % (e.g. /100%/{x}) is still unmatchable, because path_safe leaves %25 encoded.
    This matches PlainResource's existing limitation and preserves _requote_path's
    "existing %-sequences are pre-encodings" semantics, so /a%2Fb/{x} keeps working.

  3. Pre-existing and deliberately untouched: PlainResource.url_for() returns a raw
    space for a path registered as /hello world (it builds with encoded=True from the
    as-written path). Verified identical on master and on this branch, so it is not a
    regression from this PR and is left out of scope.

  4. The Backport label added check is red and stays red until a maintainer applies a
    backport-* label — nothing in this diff can turn it green.

Verification run

Run against the rebased branch with the C extensions built (http_parser.NO_EXTENSIONS
is False), CPython 3.14.4 on macOS.

$ PYTHONPATH=. python -m pytest -q --numprocesses=auto \
    tests/test_urldispatch.py tests/test_web_urldispatcher.py tests/test_route_def.py \
    tests/test_web_middleware.py tests/test_web_app.py tests/test_web_functional.py
8 workers [547 items]
........................................................................ [ 13%]
........................................................................ [ 26%]
........................................................................ [ 39%]
........................................................................ [ 52%]
........................................................................ [ 65%]
........................................................................ [ 78%]
........s.s..........s.................................................. [ 92%]
..............s......s.s...................                              [100%]

The two new tests in tests/test_web_urldispatcher.py were confirmed to fail on
master
and pass on this branch:

$ git worktree add /tmp/master-check origin/master
$ cp tests/test_web_urldispatcher.py /tmp/master-check/tests/
$ cd /tmp/master-check && pytest -q tests/test_web_urldispatcher.py -k encodable_prefix
tests/test_web_urldispatcher.py FF                                       [100%]

Sub-application resolution, branch vs master:

                              BRANCH                    MASTER
/sub%20app/plain          ->  UrlMappingMatchInfo {}     MatchInfoError (404)
/sub%20app/dyn/x          ->  UrlMappingMatchInfo {var}  MatchInfoError (404)
/sub%20app/static/a.txt   ->  UrlMappingMatchInfo {file} MatchInfoError (404)

Lint: black, isort, pyupgrade, codespell, flake8 (7.3.0 with the pinned
plugins) and the changelog hooks all pass on the changed files. mypy reports only
pre-existing errors unrelated to this diff (aiohttp/worker.py, and unused
type: ignores at web_urldispatcher.py:161,169 outside every hunk here).

Drafted with Claude Code (Claude Opus 5); pending review by @loonister1.

@greptile-apps

greptile-apps Bot commented Aug 19, 2026

Copy link
Copy Markdown

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains; the previously requested API documentation and changelog attribution are now present and accurately cover the changed metadata contracts.

Reviews (4): Last reviewed commit: "Document the route metadata change" | Re-trigger Greptile

@property
def canonical(self) -> str:
return self._formatter
return self._canonical

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Document raw route metadata

DynamicResource.canonical, PrefixResource.canonical, and StaticResource.get_info()["prefix"] now expose raw spaces and non-ASCII characters instead of percent-encoded values. This user-visible representation change needs the repository-required API documentation, and its changelog fragment needs the required -- by :user: attribution.

Context Used: CLAUDE.md (source)

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

@codecov

codecov Bot commented Aug 19, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.00%. Comparing base (8c8906a) to head (c331f57).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #13498   +/-   ##
=======================================
  Coverage   99.00%   99.00%           
=======================================
  Files         132      132           
  Lines       49635    49696   +61     
  Branches     2575     2575           
=======================================
+ Hits        49141    49202   +61     
  Misses        370      370           
  Partials      124      124           
Flag Coverage Δ
Autobahn 22.02% <7.46%> (-0.02%) ⬇️
CI-GHA 98.91% <100.00%> (+<0.01%) ⬆️
OS-Linux 98.69% <100.00%> (+<0.01%) ⬆️
OS-Windows 97.01% <100.00%> (+<0.01%) ⬆️
OS-macOS 97.93% <100.00%> (+<0.01%) ⬆️
Py-3.10 98.13% <100.00%> (+<0.01%) ⬆️
Py-3.11 98.37% <100.00%> (+<0.01%) ⬆️
Py-3.12 98.45% <100.00%> (-0.01%) ⬇️
Py-3.13 98.44% <100.00%> (-0.01%) ⬇️
Py-3.14 98.47% <100.00%> (+<0.01%) ⬆️
Py-3.14t 97.55% <100.00%> (-0.01%) ⬇️
Py-pypy-3.11 97.41% <100.00%> (+<0.01%) ⬆️
VM-macos 97.93% <100.00%> (+<0.01%) ⬆️
VM-ubuntu 98.69% <100.00%> (+<0.01%) ⬆️
VM-windows 97.01% <100.00%> (+<0.01%) ⬆️
cython-coverage 82.26% <92.85%> (+<0.01%) ⬆️

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

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

@codspeed-hq

codspeed-hq Bot commented Aug 19, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 84 untouched benchmarks
⏩ 83 skipped benchmarks1


Comparing loonister1:fix/13433-dynamic-routes-with-percent-encodable-fi (c331f57) with master (8c8906a)

Open in CodSpeed

Footnotes

  1. 83 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@loonister1
loonister1 force-pushed the fix/13433-dynamic-routes-with-percent-encodable-fi branch from bfcccb4 to c331f57 Compare August 19, 2026 13:45
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.

Dynamic routes with percent-encodable fixed segments (space, non-ASCII) are unreachable and url_for() URLs 404

1 participant