fix: stop inline schemas from silently overwriting spec-defined models - #24549
fix: stop inline schemas from silently overwriting spec-defined models#24549donald wants to merge 3 commits into
Conversation
An inline request-body schema could silently replace a schema declared in
components.schemas, deleting its properties from the generated code. No
error, no warning -- the generated client just has the wrong contract for
an endpoint that never used an inline schema:
paths:
/members/import:
post:
operationId: importMembers # inline body -> importMembers_request
components:
schemas:
ImportMembersRequest: # mangles to the same model name
properties: { declaredFieldA: ..., declaredFieldB: ... }
Only one model was emitted, carrying the inline schema's properties;
declaredFieldA/declaredFieldB disappeared, and the endpoint that $refs
ImportMembersRequest silently serialised the wrong shape.
InlineModelResolver.uniqueName() compared candidate names by raw string
only. Generators mangle schema names before emitting types, so
"importMembers_request" and "ImportMembersRequest" both become the model
"ImportMembersRequest" while the raw keys look distinct -- no collision is
detected and one model overwrites the other.
Make the collision check normalization-aware: compare names ignoring
separators and case, i.e. the way they will look after mangling. The
existing name + "_" + count fallback then keeps both models, emitting
ImportMembersRequest and ImportMembersRequest1.
Regenerating all 787 sample configs produces no diff.
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="modules/openapi-generator/src/main/java/org/openapitools/codegen/InlineModelResolver.java">
<violation number="1" location="modules/openapi-generator/src/main/java/org/openapitools/codegen/InlineModelResolver.java:967">
P2: The normalization in `normalizeNameForCollision()` only strips underscores (`_`) before lowercasing. Spec-defined schema names stored in `openAPI.getComponents().getSchemas()` can contain hyphens (`-`), dots (`.`), or other non-alphanumeric characters that `sanitizeName` would eventually convert to `_`. For example, a spec-defined schema named `Import-Members-Request` normalizes to `import-members-request` (hyphen not removed), while an inline schema `importMembers_request` normalizes to `importmembersrequest` — the collision is missed, and the silent overwrite issue could still occur for schemas using other separator characters. Since the Javadoc says the method "ignores separators and case," stripping only `_` is narrower than described.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| private String normalizeNameForCollision(final String name) { | ||
| return name.replace("_", "").toLowerCase(Locale.ROOT); |
There was a problem hiding this comment.
P2: The normalization in normalizeNameForCollision() only strips underscores (_) before lowercasing. Spec-defined schema names stored in openAPI.getComponents().getSchemas() can contain hyphens (-), dots (.), or other non-alphanumeric characters that sanitizeName would eventually convert to _. For example, a spec-defined schema named Import-Members-Request normalizes to import-members-request (hyphen not removed), while an inline schema importMembers_request normalizes to importmembersrequest — the collision is missed, and the silent overwrite issue could still occur for schemas using other separator characters. Since the Javadoc says the method "ignores separators and case," stripping only _ is narrower than described.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At modules/openapi-generator/src/main/java/org/openapitools/codegen/InlineModelResolver.java, line 967:
<comment>The normalization in `normalizeNameForCollision()` only strips underscores (`_`) before lowercasing. Spec-defined schema names stored in `openAPI.getComponents().getSchemas()` can contain hyphens (`-`), dots (`.`), or other non-alphanumeric characters that `sanitizeName` would eventually convert to `_`. For example, a spec-defined schema named `Import-Members-Request` normalizes to `import-members-request` (hyphen not removed), while an inline schema `importMembers_request` normalizes to `importmembersrequest` — the collision is missed, and the silent overwrite issue could still occur for schemas using other separator characters. Since the Javadoc says the method "ignores separators and case," stripping only `_` is narrower than described.</comment>
<file context>
@@ -924,13 +924,50 @@ private String uniqueName(final String name) {
+ return false;
+ }
+
+ private String normalizeNameForCollision(final String name) {
+ return name.replace("_", "").toLowerCase(Locale.ROOT);
+ }
</file context>
| private String normalizeNameForCollision(final String name) { | |
| return name.replace("_", "").toLowerCase(Locale.ROOT); | |
| private String normalizeNameForCollision(final String name) { | |
| return name.replaceAll("[^A-Za-z0-9]", "").toLowerCase(Locale.ROOT); | |
| } |
…ting collisions Addresses review feedback on normalizeNameForCollision. sanitizeName turns '-', '.', ' ' and friends into '_' before the name is camelized, so a spec-defined `Import-Members-Request` mangles onto the same model as the inline `importMembers_request`. Stripping only '_' missed those, and was also narrower than the javadoc's "ignores separators and case". Strip every non-alphanumeric character instead. New test inlineSchemaDoesNotOverwriteSpecDefinedSchemaNamedWithOtherSeparators covers a hyphenated spec-defined name; it fails on the previous version. Regenerating all 787 sample configs still produces no diff.
|
Good catch — fixed in 17162a9. You are right, and the javadoc claim ("ignores separators and case") was wider than the implementation. Now every non-alphanumeric character is stripped. Added Full test suite and all 787 sample configs re-run — still no diff. |
There was a problem hiding this comment.
2 issues found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="modules/openapi-generator/src/main/java/org/openapitools/codegen/InlineModelResolver.java">
<violation number="1" location="modules/openapi-generator/src/main/java/org/openapitools/codegen/InlineModelResolver.java:930">
P1: An inline schema name mapping can still overwrite an existing component schema, so the new guard does not cover the final name written to `components.schemas`. Collision checking should include the mapped target and choose a non-colliding mapped name before insertion.</violation>
<violation number="2" location="modules/openapi-generator/src/main/java/org/openapitools/codegen/InlineModelResolver.java:974">
P2: Valid schema names that differ only by letter case are unnecessarily renamed even though the default model mangler keeps them distinct. Collision normalization should mirror the target mangler instead of lowercasing every letter.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| int count = 0; | ||
| while (true) { | ||
| if (!openAPI.getComponents().getSchemas().containsKey(uniqueName) && !uniqueNames.contains(uniqueName)) { | ||
| if (!isNameTaken(uniqueName)) { |
There was a problem hiding this comment.
P1: An inline schema name mapping can still overwrite an existing component schema, so the new guard does not cover the final name written to components.schemas. Collision checking should include the mapped target and choose a non-colliding mapped name before insertion.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At modules/openapi-generator/src/main/java/org/openapitools/codegen/InlineModelResolver.java, line 930:
<comment>An inline schema name mapping can still overwrite an existing component schema, so the new guard does not cover the final name written to `components.schemas`. Collision checking should include the mapped target and choose a non-colliding mapped name before insertion.</comment>
<file context>
@@ -924,13 +927,53 @@ private String uniqueName(final String name) {
int count = 0;
while (true) {
- if (!openAPI.getComponents().getSchemas().containsKey(uniqueName) && !uniqueNames.contains(uniqueName)) {
+ if (!isNameTaken(uniqueName)) {
return uniqueName;
}
</file context>
| // Every non-alphanumeric character is a separator here, not just '_': sanitizeName turns | ||
| // '-', '.', ' ' and friends into '_' before the name is camelized, so `Import-Members-Request` | ||
| // and `importMembers_request` end up as the same model too. | ||
| return NON_ALPHANUMERIC.matcher(name).replaceAll("").toLowerCase(Locale.ROOT); |
There was a problem hiding this comment.
P2: Valid schema names that differ only by letter case are unnecessarily renamed even though the default model mangler keeps them distinct. Collision normalization should mirror the target mangler instead of lowercasing every letter.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At modules/openapi-generator/src/main/java/org/openapitools/codegen/InlineModelResolver.java, line 974:
<comment>Valid schema names that differ only by letter case are unnecessarily renamed even though the default model mangler keeps them distinct. Collision normalization should mirror the target mangler instead of lowercasing every letter.</comment>
<file context>
@@ -924,13 +927,53 @@ private String uniqueName(final String name) {
+ // Every non-alphanumeric character is a separator here, not just '_': sanitizeName turns
+ // '-', '.', ' ' and friends into '_' before the name is camelized, so `Import-Members-Request`
+ // and `importMembers_request` end up as the same model too.
+ return NON_ALPHANUMERIC.matcher(name).replaceAll("").toLowerCase(Locale.ROOT);
+ }
+
</file context>
…sing
Addresses review feedback that lowercasing over-matches.
Names that differ only by letter case can stay distinct after mangling, so
folding case flagged collisions that do not exist:
importMembers_request -> ImportMembersRequest
Import-Members-Request -> ImportMembersRequest (real collision)
ImportMembersRequest -> ImportMembersRequest (real collision)
Importmembersrequest -> Importmembersrequest (distinct!)
IMPORTMEMBERSREQUEST -> IMPORTMEMBERSREQUEST (distinct!)
The last two would have been pushed to _1 for no reason.
Rather than hand-rolling a closer approximation, just run the names through
the mangler itself -- camelize(sanitizeName(name)). That catches every real
collision, cannot over-match by construction, and makes the javadoc's claim
verifiable instead of aspirational.
New test inlineSchemaKeepsItsNameWhenTheManglerKeepsNamesDistinct covers the
false-positive direction; it fails on the previous version with "no real
collision, the inline schema should keep its own name".
Regenerating all 787 sample configs still produces no diff.
|
Thanks — one adopted, one I would like to push back on. P2 (case folding over-matches) — valid, fixed in dfff3c4You are right. Checking against the real mangler: The last two would have been pushed to Rather than hand-rolling a closer approximation, I now run the names through the mangler itself: private String normalizeNameForCollision(final String name) {
return camelize(sanitizeName(name));
}That catches every real collision, cannot over-match by construction, and makes the javadoc claim verifiable instead of aspirational. It also subsumes the separator fix from the previous round. Added P1 (inline schema name mapping) — I do not think this belongs in this PRTwo reasons: It is pre-existing and unrelated to this change. Silently renaming would defeat the option. Happy to open a separate issue for it if maintainers agree it should be addressed. Full test suite and all 787 sample configs re-run — still no diff. |
Fixes #24548
What
An inline request-body schema could silently replace a schema declared in
components.schemas,deleting its properties from the generated code. No error, no warning — the generated client just has
the wrong contract for an endpoint that never used an inline schema.
Why
InlineModelResolver.uniqueName()compares candidate names by raw string:
Generators mangle schema names before emitting types, so
importMembers_requestandImportMembersRequestboth become the modelImportMembersRequest. The raw keys differ, no collision isdetected, the inline schema is registered under its own key, and later one model overwrites the other.
How
Make the collision check normalization-aware: compare names ignoring separators and case — i.e. the way
they will look after mangling. The existing
name + "_" + countfallback then does the rest, so theinline schema lands on
importMembers_request_1and both models survive.This only ever makes
uniqueName()pick a different (numbered) name where it previously returned onethat would collide after mangling; it cannot merge or rename anything that was already distinct.
Testing
InlineModelResolverTest.inlineSchemaDoesNotOverwriteSpecDefinedSchemaWithMangledSameName—asserts the spec-defined schema keeps its own properties and the inline schema is registered under a
non-colliding key. Verified that this test fails without the fix.
InlineModelResolverTest(60 tests) passes../bin/generate-samples.sh ./bin/configs/*.yaml) and./bin/utils/export_docs_generators.sh— no diff. No existing sample hits the collision, so thechange is inert on the entire sample corpus.
PR checklist
(Both scripts were run; neither produced any change, so there is nothing to commit beyond the fix and its test.)
masterThis is in the language-agnostic core (
InlineModelResolver), so it is not owned by one language committee.CC @wing328
🤖 Generated with Claude Code
Summary by cubic
Prevents inline request-body schemas from overwriting spec-defined models after name mangling. Collision checks now use the same normalization as the model mangler so real collisions are numbered and distinct names stay distinct.
InlineModelResolver.uniqueName()to compare names viacamelize(sanitizeName(name)); fallback toname_1when needed.inlineSchemaDoesNotOverwriteSpecDefinedSchemaWithMangledSameName,inlineSchemaDoesNotOverwriteSpecDefinedSchemaNamedWithOtherSeparators,inlineSchemaKeepsItsNameWhenTheManglerKeepsNamesDistinct.Written for commit dfff3c4. Summary will update on new commits.