Skip to content

Fix false RouterPathParamsModifiedError when renaming path-versioned endpoints#358

Merged
zmievsa merged 4 commits intomainfrom
fix/path-versioning-endpoint-rename
Mar 22, 2026
Merged

Fix false RouterPathParamsModifiedError when renaming path-versioned endpoints#358
zmievsa merged 4 commits intomainfrom
fix/path-versioning-endpoint-rename

Conversation

@zmievsa
Copy link
Copy Markdown
Owner

@zmievsa zmievsa commented Mar 21, 2026

When api_version_location="path", the {api_version} placeholder appears
in route paths but not in dependant.path_params — FastAPI doesn't treat it
as an endpoint param. This caused a false positive in the path param
consistency check when using .had(path=...) to rename an endpoint.

Instead of inferring the version-managed param via regex diff against the
original route path, thread api_version_parameter_name from Cadwyn
through generate_versioned_routers_EndpointTransformer
_apply_endpoint_had_instruction, where it is discarded from the candidate
param set before comparison.

Fixes #357

…ersion} prefix

When api_version_location="path", the api_version param appears in route
paths but not in dependant.path_params, causing a false positive when
validating path param consistency on endpoint renames.

Thread api_version_parameter_name through generate_versioned_routers and
_EndpointTransformer down to _apply_endpoint_had_instruction, which now
discards the version param directly instead of inferring it via regex.

Fixes #357

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 21, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.84%. Comparing base (5a7eea2) to head (35ef027).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #358   +/-   ##
=======================================
  Coverage   99.84%   99.84%           
=======================================
  Files          72       72           
  Lines        5645     5663   +18     
  Branches      358      359    +1     
=======================================
+ Hits         5636     5654   +18     
  Misses          9        9           

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@zmievsa-semasoftware
Copy link
Copy Markdown

Code review

Found 1 issue:

  1. The new test defines an inline class RenameEndpoint(VersionChange) (line 266), but the established pattern in this file is to use the version_change() helper (used ~40 times vs ~9 inline classes). Per the code review on PR Tolerate FastAPI 0.128+ utils removal, restrict fastapi >= 0.128.6 #327: "We also (almost) never use classes for tests." This case is a simple single-instruction rename that can be expressed with version_change(endpoint("/{api_version}/new-name", ["GET"]).had(path="/{api_version}/old-name")).

app_router.include_router(sub_router, prefix="/new-name")
class RenameEndpoint(VersionChange):
description = "Rename /old-name to /new-name"
instructions_to_migrate_to_previous_version = (
endpoint("/{api_version}/new-name", ["GET"]).had(path="/{api_version}/old-name"),
)
versions = VersionBundle(
Version("v2", RenameEndpoint),
Version("v1"),
)

🤖 Generated with Claude Code

- If this code review was useful, please react with 👍. Otherwise, react with 👎.

zmievsa and others added 3 commits March 22, 2026 14:03
…nferring it

Replace the indirect `_api_version_path_param_name` approach with explicitly
passing `api_version_location` to `_EndpointTransformer` and
`_apply_endpoint_attributes_to_route`. This makes the path param filtering
logic clearer by checking `api_version_location == "path"` directly.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@zmievsa zmievsa merged commit a346768 into main Mar 22, 2026
13 checks passed
@zmievsa zmievsa deleted the fix/path-versioning-endpoint-rename branch March 22, 2026 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

When using {api_version} on the routes, renaming an endpoint breaks

2 participants