fix(openapi): order path parameters to match the url in generated schema#4933
fix(openapi): order path parameters to match the url in generated schema#4933Bishwas-py wants to merge 9 commits into
Conversation
Path parameters were ordered by how the handler happened to consume them (direct argument, dependency, or unconsumed), not by their position in the url. Consuming a path parameter through a dependency instead of a direct argument silently reordered the generated OpenAPI schema, which reshuffles any client code generated from it. Sort only the path-type parameters to match the url's real order; query and header parameters keep their existing position. Fixes litestar-org#3644
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4933 +/- ##
==========================================
- Coverage 67.30% 67.30% -0.01%
==========================================
Files 293 293
Lines 15230 15239 +9
Branches 1728 1729 +1
==========================================
+ Hits 10251 10256 +5
- Misses 4832 4836 +4
Partials 147 147 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| ordered_path_parameters = sorted( | ||
| (parameters[i] for i in path_slots), key=lambda parameter: path_order[parameter.name] | ||
| ) | ||
| for slot, parameter in zip(path_slots, ordered_path_parameters, strict=True): |
There was a problem hiding this comment.
question: can we order all parameters? it would be a bigger change, but I guess it makes sense to have a strong and consistent order based on: type and internal logic for each type.
There was a problem hiding this comment.
scoped this to path parameters, matching #3644. ordering all parameter types is a bigger change, it would need a canonical cross-type order and would break test_unwrap_new_type's current assumption that a query param can precede a path param. am happy to open a follow-up for that if you're interested.
There was a problem hiding this comment.
I don't think that this is a good idea to split ordering in PRs. Let's focus on this problem in a single PR :)
There was a problem hiding this comment.
I agree with @sobolevn. Let's try to find a general solution for this problem that provides a canonical ordering for parameters of all types.
There was a problem hiding this comment.
pushed a general solution in 7f7273d. parameters are now ordered by type: path, query, cookie, header (matching ParamType's own order), then by an internal rule per type as url position for path and alphabetical for the rest.
Per review feedback, generalize the path-only ordering fix to cover query, cookie, and header parameters too. Parameters are now ordered by type first (path, query, cookie, header, matching ParamType's own declaration order), then by an internal rule per type: path parameters by their position in the url, and query/cookie/header parameters alphabetically by name, since those types have no positional signal to order by otherwise. This keeps parameter order stable and predictable regardless of how a handler happens to consume its parameters.
| """ | ||
| path_order = {name: index for index, name in enumerate(self.path_parameters)} | ||
|
|
||
| def sort_key(parameter: Parameter) -> tuple[int, int | str]: |
There was a problem hiding this comment.
why do we put this function inside the method?
There was a problem hiding this comment.
it closed over path_order, which is local there. inlined it as a lambda in 9f6737b, no nested def needed.
| path_order = {name: index for index, name in enumerate(self.path_parameters)} | ||
| return sorted( | ||
| parameters, | ||
| key=lambda parameter: ( |
There was a problem hiding this comment.
Let's make this a module level protected named function please :)
There's not need to compile it in-place
There was a problem hiding this comment.
moved it to a module level protected function in 7bcd4ea, bound path_order with functools.partial.
| _PARAM_TYPE_ORDER = {"path": 0, "query": 1, "cookie": 2, "header": 3} | ||
|
|
||
|
|
||
| def _parameter_sort_key(parameter: Parameter, path_order: Mapping[str, int]) -> tuple[int, int | str]: |
There was a problem hiding this comment.
let's put it in very bottom :)
Co-authored-by: Janek Nouvertné <provinzkraut@posteo.de>
Description
The generated OpenAPI schema orders path parameters by how the handler happens to consume them, not by their position in the url. A path parameter consumed through a dependency, or not consumed at all, gets pushed out of its natural url position, so refactoring code that doesn't touch the url can still reorder the public schema (and any client code generated from it).
Fixes #3644
Root cause
ParameterFactory.create_parameters_for_handlerbuilds the parameter list by iterating the handler's function signature, recursing into dependency providers when a field is a dependency, and appending unconsumed path parameters at the very end. None of these three insertion paths follow the url's actual left-to-right order, even though that order is already available viaroute.path_parameters.Fix
Added
ParameterFactory._order_path_parameters, which reorders only the path-type parameters to match their position inself.path_parameters(the url's real order) before the list is returned. Query and header parameters keep their existing position; this is scoped to fixing path-parameter ordering specifically.How was this tested
Added three tests to
tests/unit/test_openapi/test_parameters.py:Before the fix, for
/users/{user_id}/posts/{post_id}/comments/{comment_id}withpost_idconsumed via a dependency, the generated schema ordered parameters asuser_id, comment_id, post_id. After the fix, it orders them asuser_id, post_id, comment_id, matching the url.📚 Documentation preview 📚: https://litestar-org.github.io/litestar-docs-preview/4933