Skip to content

Bugfix/ignore schema mappings and import mappings for forced generate schemas - #24771

Draft
Picazsoo wants to merge 11 commits into
OpenAPITools:masterfrom
Picazsoo:bugfix/ignore-schemaMappings-and-importMappings-for-forcedGenerateSchemas
Draft

Bugfix/ignore schema mappings and import mappings for forced generate schemas#24771
Picazsoo wants to merge 11 commits into
OpenAPITools:masterfrom
Picazsoo:bugfix/ignore-schemaMappings-and-importMappings-for-forcedGenerateSchemas

Conversation

@Picazsoo

@Picazsoo Picazsoo commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

PR checklist

  • Read the contribution guidelines.
  • Run the following to build the project and update samples:
    ./mvnw clean package || exit
    ./bin/generate-samples.sh ./bin/configs/*.yaml || exit
    ./bin/utils/export_docs_generators.sh || exit
    
    (For Windows users, please run the script in WSL)
    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.
  • If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

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

    • Adds a second pass that emits only forced schemas as stock models; forced-to-forced references use stock names; typeMapping still applies; * includes all mapping‑suppressed schemas, including type+import‑mapped.
    • Pulls in recursive dependents during the shadow pass when generateRecursiveDependentModels is enabled and the model set is constrained.
    • Prevents leaks into production outputs: APIs, non‑forced models, and supporting-file metadata continue to use mapped classes; mapping‑sensitive caches and mutable state are restored between passes.
    • Clears cached CodegenProperty during the shadow pass to rebuild properties with current mappings and avoid mapped-name leakage into shadow models; tests keep APIs enabled and verify isolation.
    • Introduces ForcedSchemaSupport.clearModelNameCache() and implements it for Java, Groovy, Kotlin, C#, Python (incl. Pydantic v1), PHP, Go client, Perl, PowerShell, R, and Ruby.
    • Updates CLI/Gradle/Maven help for forcedGenerateSchemas; unsupported generators are rejected early.
    • Samples/configs: add schemaMappings for Category and forcedGenerateSchemas in spring-boot-3.yaml and kotlin-spring-boot-3.yaml; samples import com.example.mapped.Category; add a JSON equivalence test.
  • Migration

    • No action if you don’t use forcedGenerateSchemas.
    • If you relied on forced models using mapped names, update references: forced schemas now generate as stock names (e.g., Category.java), while APIs and non‑forced models still reference the mapped class (e.g., com.example.mapped.Category).
    • If your generator isn’t supported, remove forcedGenerateSchemas or switch to a supported generator.

Written for commit f351bd8. Summary will update on new commits.

Review in cubic

@Picazsoo
Picazsoo marked this pull request as ready for review August 24, 2026 21:31
@Picazsoo
Picazsoo marked this pull request as draft August 24, 2026 21:32

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 39 files

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

@Picazsoo
Picazsoo marked this pull request as ready for review August 25, 2026 12:04
@Picazsoo
Picazsoo marked this pull request as draft August 25, 2026 12:05

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>

Picazsoo and others added 2 commits August 25, 2026 15:51
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant