Fix invalid route path used in by path migrations when the path was renamed (#271)

Whenever a route was migrated by path, we used the path and methods of the request. So if the request wanted "/v1/webhook_settings" and we renamed them to "/v1/webhook_subscriptions" -- all migrations for "/v1/webhook_subscriptions" would not get applied to any requests that wanted "/v1/webhook_settings". This effectively means that previously route renamings were incompatible with by path converters. Now we use the head route id instead of the path and methods of the request for matching so we always know which migrations to apply.

Author: zmievsa

CHANGELOG.md

@@ -5,6 +5,12 @@ Please follow [the Keep a Changelog standard](https://keepachangelog.com/en/1.0.
 
 ## [Unreleased]
 
+## [5.2.2]
+
+### Fixed
+
+* Whenever a route was migrated by path, we used the path and methods of the request. So if the request wanted "/v1/webhook_settings" and we renamed them to "/v1/webhook_subscriptions" -- all migrations for "/v1/webhook_subscriptions" would not get applied to any requests that wanted "/v1/webhook_settings". This effectively means that previously route renamings were incompatible with by path converters. Now we use the head route id instead of the path and methods of the request for matching so we always know which migrations to apply.
+
 ## [5.2.1]
 
 ### Fixed

cadwyn/route_generation.py

@@ -37,11 +37,13 @@
 )
 from cadwyn.structure import Version, VersionBundle
 from cadwyn.structure.common import Endpoint, VersionType
+from cadwyn.structure.data import _AlterRequestByPathInstruction, _AlterResponseByPathInstruction
 from cadwyn.structure.endpoints import (
     EndpointDidntExistInstruction,
     EndpointExistedInstruction,
     EndpointHadInstruction,
 )
+from cadwyn.structure.versions import VersionChange
 
 if TYPE_CHECKING:
     from fastapi.dependencies.models import Dependant
@@ -52,6 +54,9 @@
 _RouteT = TypeVar("_RouteT", bound=BaseRoute)
 # This is a hack we do because we can't guarantee how the user will use the router.
 _DELETED_ROUTE_TAG = "_CADWYN_DELETED_ROUTE"
+_RoutePath = str
+_RouteMethod = str
+_RouteId = int
 
 
 @dataclass(**DATACLASS_SLOTS, frozen=True, eq=True)
@@ -123,6 +128,7 @@ def __init__(self, parent_router: _R, versions: VersionBundle, webhooks: _WR) ->
         ]
 
     def transform(self) -> GeneratedRouters[_R, _WR]:
+        # Copy MUST keep the order and number of routes. Otherwise, a ton of code below will break.
         router = copy_router(self.parent_router)
         webhook_router = copy_router(self.parent_webhooks_router)
         routers: dict[VersionType, _R] = {}
@@ -132,7 +138,7 @@ def transform(self) -> GeneratedRouters[_R, _WR]:
             self.schema_generators[str(version.value)].annotation_transformer.migrate_router_to_version(router)
             self.schema_generators[str(version.value)].annotation_transformer.migrate_router_to_version(webhook_router)
 
-            self._validate_all_data_converters_are_applied(router, version)
+            self._attach_routes_to_data_converters(router, self.parent_router, version)
 
             routers[version.value] = router
             webhook_routers[version.value] = webhook_router
@@ -193,9 +199,11 @@ def transform(self) -> GeneratedRouters[_R, _WR]:
             ]
         return GeneratedRouters(routers, webhook_routers)
 
-    def _validate_all_data_converters_are_applied(self, router: APIRouter, version: Version):
-        path_to_route_methods_mapping, head_response_models, head_request_bodies = self._extract_all_routes_identifiers(
-            router
+    def _attach_routes_to_data_converters(self, router: APIRouter, head_router: APIRouter, version: Version):
+        # This method is way out of its league in terms of complexity. We gotta refactor it.
+
+        path_to_route_methods_mapping, head_response_models, head_request_bodies = (
+            self._extract_all_routes_identifiers_for_route_to_converter_matching(router)
         )
 
         for version_change in version.changes:
@@ -204,21 +212,10 @@ def _validate_all_data_converters_are_applied(self, router: APIRouter, version:
                 *version_change.alter_request_by_path_instructions.values(),
             ]:
                 for by_path_converter in by_path_converters:
-                    missing_methods = by_path_converter.methods.difference(
-                        path_to_route_methods_mapping[by_path_converter.path]
+                    self._attach_routes_by_path_converter(
+                        head_router, path_to_route_methods_mapping, version_change, by_path_converter
                     )
 
-                    if missing_methods:
-                        raise RouteByPathConverterDoesNotApplyToAnythingError(
-                            f"{by_path_converter.repr_name} "
-                            f'"{version_change.__name__}.{by_path_converter.transformer.__name__}" '
-                            f"failed to find routes with the following methods: {list(missing_methods)}. "
-                            f"This means that you are trying to apply this converter to non-existing endpoint(s). "
-                            "Please, check whether the path and methods are correct. (hint: path must include "
-                            "all path variables and have a name that was used in the version that this "
-                            "VersionChange resides in)"
-                        )
-
             for by_schema_converters in version_change.alter_request_by_schema_instructions.values():
                 for by_schema_converter in by_schema_converters:
                     if not by_schema_converter.check_usage:  # pragma: no cover
@@ -249,14 +246,50 @@ def _validate_all_data_converters_are_applied(self, router: APIRouter, version:
                             f"{version_change.__name__}.{by_schema_converter.transformer.__name__}"
                         )
 
-    def _extract_all_routes_identifiers(
-        self, router: APIRouter
-    ) -> tuple[defaultdict[str, set[str]], set[Any], set[Any]]:
-        response_models: set[Any] = set()
-        request_bodies: set[Any] = set()
-        path_to_route_methods_mapping: dict[str, set[str]] = defaultdict(set)
+    def _attach_routes_by_path_converter(
+        self,
+        head_router: APIRouter,
+        path_to_route_methods_mapping: dict[_RoutePath, dict[_RouteMethod, set[_RouteId]]],
+        version_change: type[VersionChange],
+        by_path_converter: Union[_AlterResponseByPathInstruction, _AlterRequestByPathInstruction],
+    ):
+        missing_methods = set()
+        for method in by_path_converter.methods:
+            if method in path_to_route_methods_mapping[by_path_converter.path]:
+                for route_index in path_to_route_methods_mapping[by_path_converter.path][method]:
+                    route = head_router.routes[route_index]
+                    if isinstance(by_path_converter, _AlterResponseByPathInstruction):
+                        version_change._route_to_response_migration_mapping[id(route)].append(by_path_converter)
+                    else:
+                        version_change._route_to_request_migration_mapping[id(route)].append(by_path_converter)
+            else:
+                missing_methods.add(method)
+
+        if missing_methods:
+            raise RouteByPathConverterDoesNotApplyToAnythingError(
+                f"{by_path_converter.repr_name} "
+                f'"{version_change.__name__}.{by_path_converter.transformer.__name__}" '
+                f"failed to find routes with the following methods: {list(missing_methods)}. "
+                f"This means that you are trying to apply this converter to non-existing endpoint(s). "
+                "Please, check whether the path and methods are correct. (hint: path must include "
+                "all path variables and have a name that was used in the version that this "
+                "VersionChange resides in)"
+            )
 
-        for route in router.routes:
+    def _extract_all_routes_identifiers_for_route_to_converter_matching(
+        self, router: APIRouter
+    ) -> tuple[dict[_RoutePath, dict[_RouteMethod, set[_RouteId]]], set[Any], set[Any]]:
+        # int is the index of the route in the router.routes list.
+        # So we essentially keep track of which routes have which response models and request bodies.
+        # and their indices in the router.routes list. The indices will allow us to match them to the same
+        # routes in the head version. This gives us the ability to later apply changes to these routes
+        # without thinking about any renamings or response model changes.
+
+        response_models = set()
+        request_bodies = set()
+        path_to_route_methods_mapping: dict[str, dict[str, set[int]]] = defaultdict(lambda: defaultdict(set))
+
+        for index, route in enumerate(router.routes):
             if isinstance(route, APIRoute):
                 if route.response_model is not None and lenient_issubclass(route.response_model, BaseModel):
                     response_models.add(route.response_model)
@@ -265,7 +298,8 @@ def _extract_all_routes_identifiers(
                     annotation = route.body_field.field_info.annotation
                     if annotation is not None and lenient_issubclass(annotation, BaseModel):
                         request_bodies.add(annotation)
-                path_to_route_methods_mapping[route.path] |= route.methods
+                for method in route.methods:
+                    path_to_route_methods_mapping[route.path][method].add(index)
 
         head_response_models = {model.__cadwyn_original_model__ for model in response_models}
         head_request_bodies = {getattr(body, "__cadwyn_original_model__", body) for body in request_bodies}

cadwyn/schema_generation.py

@@ -196,8 +196,7 @@ def migrate_response_body(
         response,
         current_version=version,
         head_response_model=latest_response_model,
-        path="\0\0\0",
-        method="GET",
+        head_route=None,
     )
 
     versioned_response_model: type[pydantic.BaseModel] = generate_versioned_models(versions)[str(version)][

cadwyn/structure/versions.py

@@ -53,6 +53,7 @@
 _CADWYN_RESPONSE_PARAM_NAME = "cadwyn_response_param"
 _P = ParamSpec("_P")
 _R = TypeVar("_R")
+_RouteId = int
 
 PossibleInstructions: TypeAlias = Union[
     AlterSchemaSubInstruction, AlterEndpointSubInstruction, AlterEnumSubInstruction, SchemaHadInstruction, staticmethod
@@ -85,6 +86,8 @@ class VersionChange:
     alter_response_by_schema_instructions: ClassVar[dict[type, list[_AlterResponseBySchemaInstruction]]] = Sentinel
     alter_response_by_path_instructions: ClassVar[dict[str, list[_AlterResponseByPathInstruction]]] = Sentinel
     _bound_version_bundle: "Union[VersionBundle, None]"
+    _route_to_request_migration_mapping: ClassVar[dict[_RouteId, list[_AlterRequestByPathInstruction]]] = Sentinel
+    _route_to_response_migration_mapping: ClassVar[dict[_RouteId, list[_AlterResponseByPathInstruction]]] = Sentinel
 
     def __init_subclass__(cls, _abstract: bool = False) -> None:
         super().__init_subclass__()
@@ -96,6 +99,8 @@ def __init_subclass__(cls, _abstract: bool = False) -> None:
         cls._extract_body_instructions_into_correct_containers()
         cls._check_no_subclassing()
         cls._bound_version_bundle = None
+        cls._route_to_request_migration_mapping = defaultdict(list)
+        cls._route_to_response_migration_mapping = defaultdict(list)
 
     @classmethod
     def _extract_body_instructions_into_correct_containers(cls):
@@ -358,7 +363,6 @@ async def _migrate_request(
         self,
         body_type: Union[type[BaseModel], None],
         head_dependant: Dependant,
-        path: str,
         request: FastapiRequest,
         response: FastapiResponse,
         request_info: RequestInfo,
@@ -369,18 +373,17 @@ async def _migrate_request(
         embed_body_fields: bool,
         background_tasks: Union[BackgroundTasks, None],
     ) -> dict[str, Any]:
-        method = request.method
-
         start = self.reversed_version_values.index(current_version)
+        head_route_id = id(head_route)
         for v in self.reversed_versions[start + 1 :]:
             for version_change in v.changes:
                 if body_type is not None and body_type in version_change.alter_request_by_schema_instructions:
                     for instruction in version_change.alter_request_by_schema_instructions[body_type]:
                         instruction(request_info)
-                if path in version_change.alter_request_by_path_instructions:
-                    for instruction in version_change.alter_request_by_path_instructions[path]:
-                        if method in instruction.methods:  # pragma: no branch # safe branch to skip
-                            instruction(request_info)
+                if head_route_id in version_change._route_to_request_migration_mapping:
+                    for instruction in version_change._route_to_request_migration_mapping[head_route_id]:
+                        instruction(request_info)
+
         request.scope["headers"] = tuple((key.encode(), value.encode()) for key, value in request_info.headers.items())
         del request._headers
         # This gives us the ability to tell the user whether cadwyn is running its dependencies or FastAPI
@@ -406,10 +409,10 @@ def _migrate_response(
         self,
         response_info: ResponseInfo,
         current_version: VersionType,
-        head_response_model: type[BaseModel],
-        path: str,
-        method: str,
+        head_response_model: Union[type[BaseModel], None],
+        head_route: Union[APIRoute, None],
     ) -> ResponseInfo:
+        head_route_id = id(head_route)
         end = self.version_values.index(current_version)
         for v in self.versions[:end]:
             for version_change in v.changes:
@@ -420,10 +423,8 @@ def _migrate_response(
                         version_change.alter_response_by_schema_instructions[head_response_model]
                     )
 
-                if path in version_change.alter_response_by_path_instructions:
-                    for instruction in version_change.alter_response_by_path_instructions[path]:
-                        if method in instruction.methods:  # pragma: no branch # Safe branch to skip
-                            migrations_to_apply.append(instruction)  # noqa: PERF401
+                if head_route_id in version_change._route_to_response_migration_mapping:
+                    migrations_to_apply.extend(version_change._route_to_response_migration_mapping[head_route_id])
 
                 for migration in migrations_to_apply:
                     if response_info.status_code < 300 or migration.migrate_http_errors:
@@ -576,8 +577,7 @@ async def _convert_endpoint_response_to_version(  # noqa: C901
             response_info,
             api_version,
             head_route.response_model,
-            route.path,
-            method,
+            head_route,
         )
         if isinstance(response_or_response_body, FastapiResponse):
             # a webserver (uvicorn for instance) calculates the body at the endpoint level.
@@ -671,7 +671,6 @@ async def _convert_endpoint_kwargs_to_version(
         new_kwargs = await self._migrate_request(
             head_body_field,
             head_dependant,
-            route.path,
             request,
             response,
             request_info,

pyproject.toml

@@ -1,6 +1,6 @@
 [project]
 name = "cadwyn"
-version = "5.2.1"
+version = "5.2.2"
 description = "Production-ready community-driven modern Stripe-like API versioning in FastAPI"
 authors = [{ name = "Stanislav Zmiev", email = "zmievsa@gmail.com" }]
 license = "MIT"

ruff.toml

@@ -135,7 +135,7 @@ ignore = [
 ]
 
 [lint.mccabe]
-max-complexity = 14
+max-complexity = 15
 
 [format]
 quote-style = "double"

tests/test_router_generation.py

@@ -20,11 +20,13 @@
 from typing_extensions import Any, NewType, TypeAlias, TypeAliasType, get_args
 
 from cadwyn import VersionBundle, VersionedAPIRouter
+from cadwyn.applications import Cadwyn
 from cadwyn.dependencies import current_dependency_solver
 from cadwyn.exceptions import CadwynError, RouterGenerationError, RouterPathParamsModifiedError
 from cadwyn.route_generation import generate_versioned_routers
 from cadwyn.schema_generation import generate_versioned_models
 from cadwyn.structure import Version, convert_request_to_next_version_for, endpoint, schema
+from cadwyn.structure.data import RequestInfo, ResponseInfo, convert_response_to_previous_version_for
 from cadwyn.structure.enums import enum
 from cadwyn.structure.versions import VersionChange
 from tests._data.unversioned_schema_dir import UnversionedSchema2
@@ -252,6 +254,50 @@ def test__endpoint_had_another_path_variable(
         )
 
 
+def test__endpoint_had__another_path_with_the_other_migration_at_the_same_time__should_require_old_name(
+    create_versioned_app: CreateVersionedApp,
+):
+    router = VersionedAPIRouter()
+
+    @router.post("/A")
+    async def test_endpoint_post(body: list[str]):
+        return body
+
+    @router.put("/A")
+    async def test_endpoint_put(body: list[str]):
+        return body
+
+    @convert_response_to_previous_version_for("/A", ["POST"])
+    def response_migration(response: ResponseInfo):
+        response.body.append("response")
+
+    @convert_request_to_next_version_for("/A", ["POST"])
+    def request_migration(request: RequestInfo):
+        request.body.append("request")
+
+    app = Cadwyn(
+        versions=VersionBundle(
+            Version(
+                "2001-01-01",
+                version_change(
+                    endpoint("/A", ["POST"]).had(path="/B"),
+                    request_migration=request_migration,
+                    response_migration=response_migration,
+                ),
+            ),
+            Version("2000-01-01"),
+        )
+    )
+    app.generate_and_include_versioned_routers(router)
+
+    with TestClient(app) as client:
+        assert client.post("/B", headers={"X-API-VERSION": "2000-01-01"}, json=[]).json() == ["request", "response"]
+        assert client.post("/A", headers={"X-API-VERSION": "2001-01-01"}, json=[]).json() == []
+
+        assert client.put("/A", headers={"X-API-VERSION": "2000-01-01"}, json=[]).json() == []
+        assert client.put("/A", headers={"X-API-VERSION": "2001-01-01"}, json=[]).json() == []
+
+
 def test__endpoint_had_dependencies(
     test_endpoint: Endpoint,
     test_path: str,