Skip to content

Commit 3d02910

Browse files
zmievsaclaude
andcommitted
Fix RouterPathParamsModifiedError when renaming endpoints with {api_version} 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>
1 parent 42240f6 commit 3d02910

File tree

3 files changed

+56
-3
lines changed

3 files changed

+56
-3
lines changed

cadwyn/applications.py

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -215,9 +215,11 @@ def __init__(
215215
if api_version_location == "custom_header":
216216
self._api_version_manager = HeaderVersionManager(api_version_parameter_name=api_version_parameter_name)
217217
self._api_version_fastapi_depends_class = fastapi.Header
218+
self._api_version_path_param_name: Union[str, None] = None
218219
elif api_version_location == "path":
219220
self._api_version_manager = URLVersionManager(possible_version_values=self.versions._version_values_set)
220221
self._api_version_fastapi_depends_class = fastapi.Path
222+
self._api_version_path_param_name = api_version_parameter_name
221223
else:
222224
assert_never(api_version_location)
223225
# TODO: Add a test validating the error message when there are no versions
@@ -265,6 +267,7 @@ def _cadwyn_initialize(self) -> None:
265267
self._latest_version_router,
266268
webhooks=self.webhooks,
267269
versions=self.versions,
270+
api_version_parameter_name=self._api_version_path_param_name,
268271
)
269272
except RecursionError as e:
270273
raise ImportError(

cadwyn/route_generation.py

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -76,10 +76,11 @@ def generate_versioned_routers(
7676
versions: VersionBundle,
7777
*,
7878
webhooks: Union[_WR, None] = None,
79+
api_version_parameter_name: Union[str, None] = None,
7980
) -> GeneratedRouters[_R, _WR]:
8081
if webhooks is None:
8182
webhooks = cast("_WR", APIRouter())
82-
return _EndpointTransformer(router, versions, webhooks).transform()
83+
return _EndpointTransformer(router, versions, webhooks, api_version_parameter_name).transform()
8384

8485

8586
class VersionedAPIRouter(fastapi.routing.APIRouter):
@@ -128,11 +129,18 @@ def copy_route(route: _RouteT) -> _RouteT:
128129

129130

130131
class _EndpointTransformer(Generic[_R, _WR]):
131-
def __init__(self, parent_router: _R, versions: VersionBundle, webhooks: _WR) -> None:
132+
def __init__(
133+
self,
134+
parent_router: _R,
135+
versions: VersionBundle,
136+
webhooks: _WR,
137+
api_version_parameter_name: Union[str, None] = None,
138+
) -> None:
132139
super().__init__()
133140
self.parent_router = parent_router
134141
self.versions = versions
135142
self.parent_webhooks_router = webhooks
143+
self.api_version_parameter_name = api_version_parameter_name
136144
self.schema_generators = generate_versioned_models(versions)
137145

138146
self.routes_that_never_existed = [
@@ -425,7 +433,12 @@ def _apply_endpoint_changes_to_router( # noqa: C901
425433
elif isinstance(instruction, EndpointHadInstruction):
426434
for original_route in original_routes:
427435
methods_to_which_we_applied_changes |= original_route.methods
428-
_apply_endpoint_had_instruction(version_change.__name__, instruction, original_route)
436+
_apply_endpoint_had_instruction(
437+
version_change.__name__,
438+
instruction,
439+
original_route,
440+
self.api_version_parameter_name,
441+
)
429442
err = (
430443
'Endpoint "{endpoint_methods} {endpoint_path}" you tried to change in'
431444
' "{version_change_name}" doesn\'t exist'
@@ -484,6 +497,7 @@ def _apply_endpoint_had_instruction(
484497
version_change_name: str,
485498
instruction: EndpointHadInstruction,
486499
original_route: APIRoute,
500+
api_version_parameter_name: Union[str, None] = None,
487501
):
488502
for attr_name in instruction.attributes.__dataclass_fields__:
489503
attr = getattr(instruction.attributes, attr_name)
@@ -499,6 +513,8 @@ def _apply_endpoint_had_instruction(
499513
if attr_name == "path":
500514
original_path_params = {p.alias for p in original_route.dependant.path_params}
501515
new_path_params = set(re.findall("{(.*?)}", attr))
516+
if api_version_parameter_name is not None:
517+
new_path_params.discard(api_version_parameter_name)
502518
if new_path_params != original_path_params:
503519
raise RouterPathParamsModifiedError(
504520
f'When altering the path of "{list(original_route.methods)} {original_route.path}" '

tests/test_router_generation.py

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -253,6 +253,40 @@ def test__endpoint_had_another_path_variable(
253253
)
254254

255255

256+
def test__endpoint_had__path_rename_with_api_version_in_prefix__should_not_raise():
257+
app_router = VersionedAPIRouter(prefix="/{api_version}")
258+
sub_router = VersionedAPIRouter()
259+
260+
@sub_router.get("")
261+
async def list_items():
262+
return 83
263+
264+
app_router.include_router(sub_router, prefix="/new-name")
265+
266+
class RenameEndpoint(VersionChange):
267+
description = "Rename /old-name to /new-name"
268+
instructions_to_migrate_to_previous_version = (
269+
endpoint("/{api_version}/new-name", ["GET"]).had(path="/{api_version}/old-name"),
270+
)
271+
272+
versions = VersionBundle(
273+
Version("v2", RenameEndpoint),
274+
Version("v1"),
275+
)
276+
277+
cadwyn_app = Cadwyn(
278+
versions=versions,
279+
api_version_location="path",
280+
api_version_parameter_name="api_version",
281+
api_version_format="string",
282+
)
283+
cadwyn_app.generate_and_include_versioned_routers(app_router)
284+
285+
client_v2 = TestClient(cadwyn_app)
286+
assert client_v2.get("/v2/new-name").json() == 83
287+
assert client_v2.get("/v1/old-name").json() == 83
288+
289+
256290
def test__endpoint_had__another_path_with_the_other_migration_at_the_same_time__should_require_old_name(
257291
create_versioned_app: CreateVersionedApp,
258292
):

0 commit comments

Comments
 (0)