Fix: Respect parent properties in case of merged schemas by allOf by musaprg · Pull Request #1415 · oapi-codegen/oapi-codegen

musaprg · 2024-01-02T15:08:47Z

fixes #697

Currently, oapi-codegen ignores original (parent) fields if allOf is defined. OpenAPI schema doesn't seem to refuse a combination of general fields (properties) and allOf.

For example, the following schema

    PersonWithMorePropertiesOutsideOfAllOf:
      type: object
      description: |
        This is a person record as returned from a Create endpoint. It contains
        all the fields of a Person, with an additional property outside of allOf directives.
      properties:
        additionalProperty:
          type: string
      required: [ additionalProperty ]
      allOf:
        - $ref: "#/components/schemas/Person"

will be converted into the following type definition, which seems to be wrong result.

type PersonWithMorePropertiesOutsideOfAllOf = Person

The expected result would be like:

type PersonWithMorePropertiesOutsideOfAllOf struct {
	// Embedded struct due to allOf(#/components/schemas/Person)
	Person `yaml:",inline"`
	// Embedded fields due to inline allOf schema
	AdditionalProperty string `json:"additionalProperty"`
}

The known approach is normalization as proposed in other similar project like REFACTOR_ALLOF_WITH_PROPERTIES_ONLY in opneapi-generator. This PR is inspired by the idea.

REFACTOR_ALLOF_WITH_PROPERTIES_ONLY: When set to true, refactor schema with allOf and properties in the same level to a schema with allOf only and, the allOf contains a new schema containing the properties in the top level.
https://github.com/OpenAPITools/openapi-generator/blob/8b5b5a74c333b809c5a651366656257ec8a6fef3/docs/customization.md?plain=1#L567

@oapi-codegen/maintainers edit to PR message:
Closes #697
Closes #931
Closes #1710
Closes #1960
Closes #2102
Partially improves ##1139
Supersedes #717

musaprg · 2024-01-27T00:53:08Z

@jamietanna Would you take a look at this PR? Thanks.

tymofij · 2025-04-17T23:19:52Z

Thank you for this, I hope it gets reviewed soon

Signed-off-by: Kotaro Inoue <k.musaino@gmail.com>

… schema Signed-off-by: Kotaro Inoue <k.musaino@gmail.com>

Signed-off-by: Kotaro Inoue <k.musaino@gmail.com>

# Conflicts: # internal/test/all_of/v1/openapi.gen.go # internal/test/all_of/v2/openapi.gen.go

mromaszewicz · 2026-05-03T07:36:12Z

Thanks for this PR. I'm patching it up, and it actually closes quite a few related bugs. I will push a couple of fix-up commits to your branch.

…tural The unconditional injection of the parent (with AllOf cleared) as a final allOf member correctly merges structural siblings like properties/required, but it also forces n>=2 in mergeSchemas for pure wrappers like `{allOf: [$ref: X]}`, which previously took the n==1 fast-path and returned the referenced type. Without the fast-path those wrappers regenerate as inlined anonymous structs and lose their named-type identity (regression visible in examples/output-options/preferskipoptionalpointer). Add hasStructuralSiblings() so injection runs only when the parent has Properties, Required, or AdditionalProperties. Pure wrappers keep the fast-path. Also copy the parent's Description into the merged result — the merge does not carry Description, so it was being silently dropped. Addresses oapi-codegen#697, oapi-codegen#931, oapi-codegen#1710, oapi-codegen#1960, oapi-codegen#2102. Partial fix for oapi-codegen#1139 (anonymous-schema hoisting requires further work).

mromaszewicz · 2026-05-03T07:41:13Z

@greptileai

greptile-apps · 2026-05-03T07:44:29Z

Greptile Summary

This PR fixes the long-standing issue (#697) where allOf schemas with sibling properties, required, or additionalProperties at the same level silently dropped those siblings and emitted a bare type alias. The fix injects a copy of the parent schema (with AllOf cleared) as the final allOf member so MergeSchemas picks up the structural siblings normally; description propagation (issue #1960) is bundled in the same code path. Both behaviours are gated behind the new OldAllOfSiblingMerging compatibility flag, following the project's established pattern.

Confidence Score: 5/5

Safe to merge; the breaking change is intentional, properly gated behind an opt-out flag, and the implementation is logically sound.

No P0 or P1 issues found. The two P2 findings are a documentation gap (the flag silently also controls description propagation) and a latent shallow-copy hazard in Extensions that is safe under current mergeOpenapiSchemas behaviour. Both are minor and do not affect correctness of the generated output.

pkg/codegen/schema.go — shallow copy of Extensions in the injected stub; pkg/codegen/configuration.go — flag comment omits the description-propagation side-effect.

Important Files Changed

Filename	Overview
pkg/codegen/schema.go	Core fix: injects parent schema (AllOf cleared) as final allOf member so structural siblings are merged; gated on OldAllOfSiblingMerging flag; description propagation bundled in same flag.
pkg/codegen/configuration.go	Adds OldAllOfSiblingMerging CompatibilityOptions field with appropriate YAML tag and documentation.
configuration-schema.json	Adds old-allof-sibling-merging entry to the JSON schema, kept in sync with configuration.go as required.
internal/test/all_of/openapi.yaml	Adds PersonWithMorePropertiesOutsideOfAllOf regression test schema exercising the allOf+sibling-properties bug from issue #697.
internal/test/all_of/v1/openapi.gen.go	Regenerated: adds PersonWithMorePropertiesOutsideOfAllOf struct embedding Person; description comments now populated for existing types.
internal/test/all_of/v2/openapi.gen.go	Regenerated: adds flattened PersonWithMorePropertiesOutsideOfAllOf struct (v2 merges all fields inline); description comments updated.
internal/test/components/components.gen.go	Description comments updated for EnumUnion, EnumUnion2, and OneOfObject12 — driven by the description-propagation fix (issue #1960) bundled in this PR.
internal/test/issues/issue-1087/deps/deps.gen.go	Breaking change: Error type alias (= BaseError) replaced with a distinct struct that adds the reason field. This is the new default behavior; existing users must set OldAllOfSiblingMerging: true to opt out.

_{Reviews (2): Last reviewed commit: "fix(codegen): gate allOf sibling merge b..." | Re-trigger Greptile}

Address Greptile P1 review on the previous commit: 1. append(schema.AllOf, ...) could mutate kin-openapi's backing array if the parsed slice carried spare capacity. Allocate a fresh slice with make() before appending. 2. Merging the parent's structural siblings into an allOf result is a breaking change to generated Go: a schema like {allOf: [Base], properties: {extra}} previously produced `type X = Base` (interchangeable with Base) and now produces a distinct struct (not interchangeable). Per project practice (cf. OldMergeSchemas, OldEnumConflicts, OldAliasing) gate behavior changes behind a compatibility flag. Add Compatibility.OldAllOfSiblingMerging (yaml: old-allof-sibling-merging, default false). Update configuration-schema.json with a matching description so users hit by downstream breakage can find the knob. The Description copy is gated on the same flag for consistency. Greptile also suggested narrowing hasStructuralSiblings to drop the pure-required case. Keeping Required in the trigger because that is the exact scenario issue oapi-codegen#931 reports: a parent that adds `required: [foo]` to an allOf chain currently produces a Go type whose `Foo` field is `*string` with omitempty, ignoring the spec's required constraint. Required is structural in OAS terms — it changes field type in the generated struct — and the compatibility flag is the safety net for users whose downstream code depended on the alias output.

mromaszewicz · 2026-05-03T08:03:26Z

@greptileai comments addressed, review again, make sure to look at latest commit.

…pi-codegen#1415) * Respect parent properties in merging allOf schemas Signed-off-by: Kotaro Inoue <k.musaino@gmail.com> * Add test case to check if parent propeties are included in the merged schema Signed-off-by: Kotaro Inoue <k.musaino@gmail.com> * Make generate Signed-off-by: Kotaro Inoue <k.musaino@gmail.com> * fix(codegen): only inject parent siblings into allOf merge when structural The unconditional injection of the parent (with AllOf cleared) as a final allOf member correctly merges structural siblings like properties/required, but it also forces n>=2 in mergeSchemas for pure wrappers like `{allOf: [$ref: X]}`, which previously took the n==1 fast-path and returned the referenced type. Without the fast-path those wrappers regenerate as inlined anonymous structs and lose their named-type identity (regression visible in examples/output-options/preferskipoptionalpointer). Add hasStructuralSiblings() so injection runs only when the parent has Properties, Required, or AdditionalProperties. Pure wrappers keep the fast-path. Also copy the parent's Description into the merged result — the merge does not carry Description, so it was being silently dropped. Addresses oapi-codegen#697, oapi-codegen#931, oapi-codegen#1710, oapi-codegen#1960, oapi-codegen#2102. Partial fix for oapi-codegen#1139 (anonymous-schema hoisting requires further work). * fix(codegen): gate allOf sibling merge behind compatibility flag Address Greptile P1 review on the previous commit: 1. append(schema.AllOf, ...) could mutate kin-openapi's backing array if the parsed slice carried spare capacity. Allocate a fresh slice with make() before appending. 2. Merging the parent's structural siblings into an allOf result is a breaking change to generated Go: a schema like {allOf: [Base], properties: {extra}} previously produced `type X = Base` (interchangeable with Base) and now produces a distinct struct (not interchangeable). Per project practice (cf. OldMergeSchemas, OldEnumConflicts, OldAliasing) gate behavior changes behind a compatibility flag. Add Compatibility.OldAllOfSiblingMerging (yaml: old-allof-sibling-merging, default false). Update configuration-schema.json with a matching description so users hit by downstream breakage can find the knob. The Description copy is gated on the same flag for consistency. Greptile also suggested narrowing hasStructuralSiblings to drop the pure-required case. Keeping Required in the trigger because that is the exact scenario issue oapi-codegen#931 reports: a parent that adds `required: [foo]` to an allOf chain currently produces a Go type whose `Foo` field is `*string` with omitempty, ignoring the spec's required constraint. Required is structural in OAS terms — it changes field type in the generated struct — and the compatibility flag is the safety net for users whose downstream code depended on the alias output. --------- Signed-off-by: Kotaro Inoue <k.musaino@gmail.com> Co-authored-by: Marcin Romaszewicz <marcinr@gmail.com>

musaprg marked this pull request as ready for review January 2, 2024 15:22

musaprg mentioned this pull request Jan 2, 2024

録画ルールとマッチする番組数 miscord-dev/epgstation-exporter#1

Closed

musaprg changed the title ~~Respect parent properties in case of merged schemas by allOf~~ fix: Respect parent properties in case of merged schemas by allOf Feb 14, 2024

musaprg changed the title ~~fix: Respect parent properties in case of merged schemas by allOf~~ Fix: Respect parent properties in case of merged schemas by allOf Feb 14, 2024

musaprg added 3 commits April 28, 2025 21:28

Respect parent properties in merging allOf schemas

ba6c597

Signed-off-by: Kotaro Inoue <k.musaino@gmail.com>

Add test case to check if parent propeties are included in the merged…

cb92612

… schema Signed-off-by: Kotaro Inoue <k.musaino@gmail.com>

Make generate

0d104fc

Signed-off-by: Kotaro Inoue <k.musaino@gmail.com>

musaprg force-pushed the fix-issue-697 branch from b066ebb to 0d104fc Compare April 28, 2025 12:28

musaprg requested a review from a team as a code owner April 28, 2025 12:28

Merge remote-tracking branch 'upstream/main' into pr/1415

375dc81

# Conflicts: # internal/test/all_of/v1/openapi.gen.go # internal/test/all_of/v2/openapi.gen.go

mromaszewicz added the bug Something isn't working label May 3, 2026

greptile-apps Bot reviewed May 3, 2026

View reviewed changes

Comment thread pkg/codegen/schema.go Outdated

Comment thread internal/test/issues/issue-1087/deps/deps.gen.go

Comment thread pkg/codegen/schema.go

mromaszewicz mentioned this pull request May 3, 2026

Properly create models for allOf schemas with properties additionally defined #717

Closed

mromaszewicz added the notable changes Used for release notes to highlight these more highly label May 3, 2026

mromaszewicz approved these changes May 3, 2026

View reviewed changes

mromaszewicz merged commit 4887ce9 into oapi-codegen:main May 3, 2026
19 checks passed

musaprg deleted the fix-issue-697 branch May 16, 2026 09:55

mromaszewicz mentioned this pull request Jul 12, 2026

discriminator helpers generate invalid assignments for pointer-typed Code fields #2297

Closed

mromaszewicz removed the notable changes Used for release notes to highlight these more highly label Jul 16, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Fix: Respect parent properties in case of merged schemas by allOf#1415

Fix: Respect parent properties in case of merged schemas by allOf#1415
mromaszewicz merged 6 commits into
oapi-codegen:mainfrom
musaprg:fix-issue-697

musaprg commented Jan 2, 2024 •

edited by mromaszewicz

Loading

Uh oh!

musaprg commented Jan 27, 2024

Uh oh!

tymofij commented Apr 17, 2025

Uh oh!

mromaszewicz commented May 3, 2026

Uh oh!

mromaszewicz commented May 3, 2026

Uh oh!

greptile-apps Bot commented May 3, 2026 •

edited

Loading

Important Files Changed

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mromaszewicz commented May 3, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Uh oh!

Conversation

musaprg commented Jan 2, 2024 • edited by mromaszewicz Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

musaprg commented Jan 27, 2024

Uh oh!

tymofij commented Apr 17, 2025

Uh oh!

mromaszewicz commented May 3, 2026

Uh oh!

mromaszewicz commented May 3, 2026

Uh oh!

greptile-apps Bot commented May 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Summary

Confidence Score: 5/5

Important Files Changed

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mromaszewicz commented May 3, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

musaprg commented Jan 2, 2024 •

edited by mromaszewicz

Loading

greptile-apps Bot commented May 3, 2026 •

edited

Loading