Bugfix/ignore schema mappings and import mappings for forced generate schemas - #24771
Conversation
…as as isolated shadow models
…mas as isolated shadow models
There was a problem hiding this comment.
All reported issues were addressed across 39 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…o handle isolated shadow model generation
…ependents and suppress mapping leaks
…s for improved readability
There was a problem hiding this comment.
7 issues found across 36 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/test/java/org/openapitools/codegen/csharpnetcore/ForcedGenerateSchemasCSharpTest.java">
<violation number="1" location="modules/openapi-generator/src/test/java/org/openapitools/codegen/csharpnetcore/ForcedGenerateSchemasCSharpTest.java:93">
P2: The wildcard test omits ApiShape even though Shape is a mapping-suppressed discriminator/oneOf schema that the wildcard must also force. The first test proves ApiShape is generated when forced, and the mirrored Kotlin wildcard test asserts it. Not asserting it here lets a regression where the wildcard fails to force the oneOf/discriminator schema silently pass for the C# family. Add ApiShape to the existence loop (and optionally assert its file does not leak the FQN).</violation>
<violation number="2" location="modules/openapi-generator/src/test/java/org/openapitools/codegen/csharpnetcore/ForcedGenerateSchemasCSharpTest.java:93">
P3: The C# wildcard test omits ApiShape from its generated-file assertions even though Shape is mapping-suppressed and must be force-generated by the wildcard. The sibling forcedFqn test asserts ApiShape.cs exists and the Spring wildcard test includes ApiShape, so add it here too, otherwise a regression where the wildcard path drops the forced oneOf/discriminator Shape model goes unnoticed.</violation>
</file>
<file name="modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/AbstractCSharpCodegen.java">
<violation number="1" location="modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/AbstractCSharpCodegen.java:130">
P1: When a forced model inherits from a non-forced model that references a mapped schema, the cached parent `CodegenModel` from the mapped pass is reused in the forced pass. Clear `codegenModelNameAndSchemaKeyToCodegenModelCache` here so inherited properties and enum reconciliation are rebuilt with the current mappings.</violation>
</file>
<file name="modules/openapi-generator/src/test/java/org/openapitools/codegen/java/spring/ForcedGenerateSchemasSpringTest.java">
<violation number="1" location="modules/openapi-generator/src/test/java/org/openapitools/codegen/java/spring/ForcedGenerateSchemasSpringTest.java:44">
P2: MODEL_DIR starts with "/", so `new File(output, MODEL_DIR)` treats it as absolute and discards the temp `output` parent directory. Every model-file assertion in this class then checks against the filesystem/repo root instead of the generated temp output, so the tests don't validate what they claim (and the existence asserts will fail). Drop the leading slash so it is resolved relative to `output`.</violation>
</file>
<file name="modules/openapi-generator/src/test/java/org/openapitools/codegen/ForcedGenerateSchemasSupportedFamiliesTest.java">
<violation number="1" location="modules/openapi-generator/src/test/java/org/openapitools/codegen/ForcedGenerateSchemasSupportedFamiliesTest.java:84">
P2: The test only verifies the forced-to-forced path (Widget/Group both forced) and never asserts the other half of the isolation contract: non-forced models and APIs must keep referencing the mapped class. The spec already declares Container (non-mapped, non-forced) referencing Widget and a /container API returning it, but APIS is disabled and Container's generated file is never read. A regression that emits the mapped name into forced models or the stock name into non-forced/API artifacts would pass this test. Load Container's generated file and assert it contains mappedName (and not the stock filename), and keep APIS enabled to assert API artifacts reference the mapped class.</violation>
</file>
<file name="modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultGenerator.java">
<violation number="1" location="modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultGenerator.java:516">
P1: When a forced schema uses a type mapping backed by an import mapping and references that mapped type, this removal leaves the mapped type without its required import, producing uncompilable shadow source. Keep the import mapping during shadow emission and bypass only the model-suppression check for the emitted model.</violation>
<violation number="2" location="modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultGenerator.java:652">
P2: During the forced-schema shadow pass, recursive dependents that lie outside the top-level `modelKeys()` set are re-emitted even when they are not forced. In the shadow pass the suppressing mappings for forced schemas have been removed, so a dependent that was already generated in Phase 1 with mapped class references is regenerated/overwritten here with stock-name references, which contradicts the PR's stated goal that ordinary models keep referencing the mapped classes. Restrict recursive emission during the shadow pass to dependents that are themselves in `modelsToEmit` (i.e. forced), rather than emitting every pulled-in dependent.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| String widgetContents = Files.readString(Path.of(widgetFile.toURI())); | ||
| assertTrue(widgetContents.contains(stockRelatedModelName), | ||
| codegen.getName() + " must use the stock name for a reference between shadow models"); | ||
| assertFalse(widgetContents.contains(mappedRelatedName), |
There was a problem hiding this comment.
P2: The test only verifies the forced-to-forced path (Widget/Group both forced) and never asserts the other half of the isolation contract: non-forced models and APIs must keep referencing the mapped class. The spec already declares Container (non-mapped, non-forced) referencing Widget and a /container API returning it, but APIS is disabled and Container's generated file is never read. A regression that emits the mapped name into forced models or the stock name into non-forced/API artifacts would pass this test. Load Container's generated file and assert it contains mappedName (and not the stock filename), and keep APIS enabled to assert API artifacts reference the mapped class.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At modules/openapi-generator/src/test/java/org/openapitools/codegen/ForcedGenerateSchemasSupportedFamiliesTest.java, line 84:
<comment>The test only verifies the forced-to-forced path (Widget/Group both forced) and never asserts the other half of the isolation contract: non-forced models and APIs must keep referencing the mapped class. The spec already declares Container (non-mapped, non-forced) referencing Widget and a /container API returning it, but APIS is disabled and Container's generated file is never read. A regression that emits the mapped name into forced models or the stock name into non-forced/API artifacts would pass this test. Load Container's generated file and assert it contains mappedName (and not the stock filename), and keep APIS enabled to assert API artifacts reference the mapped class.</comment>
<file context>
@@ -0,0 +1,91 @@
+ String widgetContents = Files.readString(Path.of(widgetFile.toURI()));
+ assertTrue(widgetContents.contains(stockRelatedModelName),
+ codegen.getName() + " must use the stock name for a reference between shadow models");
+ assertFalse(widgetContents.contains(mappedRelatedName),
+ codegen.getName() + " must not leak mapped names into shadow models");
+ assertEquals(codegen.schemaMapping().get("Widget"), mappedName,
</file context>
…sure correct model reference handling
…y API isolation Enabling APIS in ForcedGenerateSchemasSupportedFamiliesTest exposed that the forced shadow pass reused a CodegenProperty cached during api generation (with the schemaMapping intact), leaking the mapped name into the stock shadow model (observed in the R generator's widget.R). Clear schemaCodegenPropertyCache in DefaultCodegen.clearModelNameCache() so properties are rebuilt with the current mappings. The test now keeps APIS enabled and asserts non-forced API artifacts still reference the mapped class. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
PR checklist
Commit all changed files.
This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
These must match the expectations made by your contribution.
You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example
./bin/generate-samples.sh bin/configs/java*.IMPORTANT: Do NOT purge/delete any folders/files (e.g. tests) when regenerating the samples as manually written tests may be removed.
Summary by cubic
Forced schema generation now ignores schema/import mappings and emits isolated shadow models under their stock names. Normal generation keeps mapped classes, and unsupported generators fail fast before writing files; we also clear mapping-sensitive caches so shadow models don’t leak mapped names while APIs keep mapped references.
Bug Fixes
typeMappingstill applies;*includes all mapping‑suppressed schemas, including type+import‑mapped.generateRecursiveDependentModelsis enabled and the model set is constrained.CodegenPropertyduring the shadow pass to rebuild properties with current mappings and avoid mapped-name leakage into shadow models; tests keep APIs enabled and verify isolation.ForcedSchemaSupport.clearModelNameCache()and implements it for Java, Groovy, Kotlin, C#, Python (incl. Pydantic v1), PHP, Go client, Perl, PowerShell, R, and Ruby.forcedGenerateSchemas; unsupported generators are rejected early.schemaMappingsforCategoryandforcedGenerateSchemasinspring-boot-3.yamlandkotlin-spring-boot-3.yaml; samples importcom.example.mapped.Category; add a JSON equivalence test.Migration
forcedGenerateSchemas.Category.java), while APIs and non‑forced models still reference the mapped class (e.g.,com.example.mapped.Category).forcedGenerateSchemasor switch to a supported generator.Written for commit f351bd8. Summary will update on new commits.