fix(openapi): use exact-match dedup in default_operation_id_creator - #4923
Open
chuenchen309 wants to merge 1 commit into
Open
fix(openapi): use exact-match dedup in default_operation_id_creator#4923chuenchen309 wants to merge 1 commit into
chuenchen309 wants to merge 1 commit into
Conversation
`default_operation_id_creator` builds an operationId by concatenating
title-cased path components, skipping a component if it's already
present via `component.title() not in components_namespace` — a
substring check against the concatenated string, not an exact-match
check against individual components.
This causes false collisions: for path components `["users", "user"]`,
"User" is a substring of the already-accumulated "Users", so the
"user" component is silently dropped, producing the same operationId
("UsersGetHandler") as a route whose only component is `["users"]`.
Two structurally different routes can end up with identical
operationIds, which OpenAPI requires to be unique
(test_routes_with_different_paths_should_generate_unique_operation_ids
already encodes this invariant, just not for this substring case).
Fix: track seen component titles in a set and check membership there
instead of doing a substring check against the concatenated namespace
string. This preserves the original intent (skip an exact-duplicate
component, e.g. when a router prefix repeats a literal path segment)
without falsely dropping components that merely share a substring.
How verified: reproduced the collision directly against
default_operation_id_creator with ["users"] vs ["users", "user"])
producing the same id; added a regression test
(test_default_operation_id_creator_does_not_collide_on_component_substring)
confirmed failing before the fix (AssertionError: 'UsersGetHandler' !=
'UsersGetHandler'); applied the fix; confirmed all 23 tests in
test_path_item.py and the full 262-test tests/unit/test_openapi/ suite
pass. mypy clean, pre-commit clean (ruff, unasyncd, typos).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4923 +/- ##
=======================================
Coverage 67.29% 67.30%
=======================================
Files 293 293
Lines 15228 15231 +3
Branches 1728 1728
=======================================
+ Hits 10248 10251 +3
Misses 4833 4833
Partials 147 147 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
default_operation_id_creatorcan generate identicaloperationIds for two structurally different routes, violating the uniqueness OpenAPI requires ofoperationId(and which this codebase's owntest_routes_with_different_paths_should_generate_unique_operation_idsalready asserts for the cases it covers).Root cause
The dedup check is a substring check against the concatenated
components_namespacestring, not an exact-match check against the individual components seen so far. So if a later component's title happens to be a substring of the already-built string, it gets silently dropped — even though it's a distinct path component.Example: path components
["users"]vs["users", "user"].["users"]→components_namespace = "Users".["users", "user"]→"Users"added, then"User"is checked:"User" in "Users"→True(substring!) → the"user"component is dropped. Result stays"Users".Both routes now produce the operationId
UsersGetHandler, even though they represent different paths.Fix
Track the set of components already seen (by exact value) instead of doing substring containment against the concatenated string:
This preserves the original intent — skip an exact-duplicate component (e.g. a router prefix literally repeating a path segment name) — without dropping components that merely share a substring with something already accumulated.
How I verified this
default_operation_id_creator:["users"]and["users", "user"]both produced"UsersGetHandler".test_default_operation_id_creator_does_not_collide_on_component_substring, intests/unit/test_openapi/test_path_item.py; confirmed it fails on the current code (AssertionError: 'UsersGetHandler' != 'UsersGetHandler').test_path_item.pypass.tests/unit/test_openapi/suite: 262 passed, no regressions.mypy litestar/_openapi/utils.py— clean.pre-commit runon changed files — clean (ruff check/format, unasyncd, typos).Disclosure
I used AI assistance (Claude) to help investigate and draft this fix, but I personally reproduced the collision, wrote/reviewed the regression test, ran the full suite myself, and reviewed the final diff before submitting. Happy to answer any questions.
📚 Documentation preview 📚: https://litestar-org.github.io/litestar-docs-preview/4923