Bug Report: demonstration of overlays and allOf not playing together nicely#2087
Conversation
Kusari Analysis Results:
No pinned version dependency changes, code issues or exposed secrets detected! Note View full detailed analysis result for more information on the output and the checks that were run.
Found this helpful? Give it a 👍 or 👎 reaction! |
| Id int `json:"id"` | ||
| Name string `json:"name"` | ||
| } | ||
| type ClientWithId = OverlayClient |
There was a problem hiding this comment.
I would expect this to be OverlayClientWithId
| type ClientWithId = OverlayClient | |
| type ClientWithId = OverlayClientWithId |
Relocate the overlay+allOf regression reproduction from examples/anyof-allof-oneof/ to internal/test/extensions/allof-merge-extensions/. The examples/ directory is for happy-path usage, not regression artifacts. The submitter's overlay.yaml, overlays.go, and overlays_test.go are moved via git rename (preserving per-line blame); package renamed to match new directory. examples/anyof-allof-oneof/ reverts to a clean anyOf/allOf/oneOf usage example. The bug is tracked at oapi-codegen#2335 and is unrelated to overlays — it's in the allOf+x-go-type interaction in pkg/codegen/schema.go and pkg/codegen/merge_schemas.go. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…odegen#2335) Two bugs were interacting to silently drop user intent: 1. In GenerateGoSchema, the `allOf` branch early-returned before the `x-go-type` check, so an outer `allOf` schema's own `x-go-type` was ignored. The documented contract ("completely override the definition of this schema") was violated whenever the schema used `allOf`. Fix: move the `x-go-type` check above the `allOf` branch. 2. In mergeOpenapiSchemas, extensions from all allOf members were blindly unioned into the merged schema's extensions. When a member's extensions included an identity-bound directive like `x-go-type`, that directive would leak up and become the merged schema's alias target -- e.g. `Client` with `x-go-type: OverlayClient` composed with `{properties:{id}}` via `allOf` would emit `type ClientWithId = OverlayClient`, silently dropping the Id field. The naive fix of dropping all extensions breaks the common "$ref + sibling extension" idiom (see issue oapi-codegen#1957), where allOf is used not for composition but as a workaround for OpenAPI 3.0's $ref-sibling restriction to attach extensions to a ref. Fix: in mergeSchemas, detect which of the two uses applies by checking whether any allOf member is extension-only. If so (decorator idiom), extensions flow through as before. If not (real composition), drop extensions from the merged result -- each source schema's extensions belong to that schema, not to the composed type. Regression test lives in internal/test/extensions/allof-merge-extensions/. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Greptile SummaryThis PR fixes a bug where Confidence Score: 4/5The core fix is correct and well-tested; two prior-thread edge-case concerns about decorator-idiom detection remain open and could reintroduce the bug in unusual schema shapes. The primary regression (x-go-type leaking through allOf into derived types) is correctly fixed and covered by two targeted test cases. The selective delete of only type-identity extensions (x-go-type, x-go-type-name, x-go-type-import) is sound. The x-go-type early-return move in schema.go is logically correct. Score is 4 rather than 5 because the prior review thread raised two P1 concerns — false positives in isExtensionOnlySchema (structural fields like nullable not checked) and the decorator-idiom check inspecting m.Value instead of the SchemaRef wrapper — that have not been addressed and could cause the original aliasing bug to resurface on certain schema patterns. pkg/codegen/merge_schemas.go — the isExtensionOnlySchema implementation and the decorator-idiom detection loop warrant attention before merge.
|
| Filename | Overview |
|---|---|
| pkg/codegen/merge_schemas.go | Core fix: adds decorator-idiom detection and selective deletion of x-go-type/x-go-type-name/x-go-type-import from merged schemas; prior-thread edge-case concerns about isExtensionOnlySchema false positives and m.Value inspection remain open. |
| pkg/codegen/schema.go | x-go-type check moved before AllOf handling so an explicit override on the outer schema always wins; logically correct and handles the ClientWithId overlay case properly. |
| internal/test/extensions/allof-merge-extensions/overlays_test.go | Adds two regression tests: one for the overlay alias path (ClientWithId → OverlayClientWithId) and one for the leaked-extension path (DerivedNoOverride must not be aliased to OverlayBaseOnly and must preserve the Extra field). |
| internal/test/extensions/allof-merge-extensions/spec.yaml | Well-designed spec covering both regression scenarios: direct overlay aliasing (Client/ClientWithId) and base-only extension leakage (BaseOnly/DerivedNoOverride). |
| internal/test/extensions/allof-merge-extensions/overlay.yaml | Overlay applies x-go-type to Client, ClientWithId, and BaseOnly; intentionally omits DerivedNoOverride to exercise the extension-leakage regression path. |
| internal/test/extensions/allof-merge-extensions/overlays.go | Hand-written overlay types; OverlayBaseOnly deliberately lacks an Extra field to make the aliasing bug a compilation error. |
| internal/test/extensions/allof-merge-extensions/config.yaml | Standard codegen config wiring the overlay and output file; no issues. |
| internal/test/extensions/allof-merge-extensions/generate.go | Standard go:generate directive; no issues. |
Reviews (2): Last reviewed commit: "Implement Greptile suggestions" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Three review findings addressed on top of the previous fix commit: 1. Narrow the non-decorator extension clearing from "wipe everything" to targeted delete() calls for x-go-type, x-go-type-name, and x-go-type-import. User-defined schema-level extensions and any other non-identity directives now survive allOf composition -- we only have concrete evidence of incorrect aliasing for the identity-bound three. 2. Tighten isExtensionOnlySchema via a zero-value approach. Zero out Extensions plus the documentary/metadata fields that don't affect the generated Go type (Title, Description, Default, Example, ExternalDocs, Deprecated, ReadOnly, WriteOnly, AllowEmptyValue, XML) and the kin-openapi Origin tracker, then reflect.DeepEqual against the zero Schema. Shorter, and default-safe against future kin-openapi field additions: any new structural field would be non-zero and correctly disqualify decorator classification. 3. Extend the regression test to cover the harder Fix 2 path. The original scenario had x-go-type on both Client and ClientWithId, which Fix 1 alone makes pass. New schemas BaseOnly/DerivedNoOverride exercise the case where only the base schema has an x-go-type overlay -- the derived schema must still be emitted as its own struct (with both Name and Extra fields) rather than aliased to OverlayBaseOnly. The struct literal in TestBaseOnlyOverlay won't compile under the buggy codegen because OverlayBaseOnly has no Extra field, giving the test a compile-time check in addition to the runtime assertion. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@greptileai, i think you are incorrect, in that all your suggestions have been noted and fixed in my amended PR. |
|
You're right, and I apologize for the inaccurate summary. Looking at the current diff: Concern 1 (false positives in Concern 2 (checking Both concerns are properly addressed. The implementation is sound. Tip: You can give me more context with .greptile/rules so I get this right next time. |
The delete() calls in mergeSchemas operated on schema.Extensions directly. In the current code this is safe: valueWithPropagatedRef returns a shallow struct copy whose Extensions map is shared with the caller, but mergeOpenapiSchemas always reallocates Extensions before merging, and the n>=2 guard ensures that reallocation has happened by the time we reach the delete. The delete thus operates on a fresh map, not the caller's shared one. That safety is an invariant held by two separate pieces of code (the n>=1 short-circuit, and mergeOpenapiSchemas' make()), neither of which is obvious from reading the delete. Cloning the map before mutating makes the non-aliasing explicit locally and keeps the code correct even if either piece changes (e.g. an allocation-skipping optimization in mergeOpenapiSchemas). maps.Clone(nil) returns nil and delete(nil, ...) is a no-op, so no nil-guard is needed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2 participants
Fixes: #2335
@nelzblooket reports:
Following the instructions:
Version of
oapi-codegen- frommainpulled today:Go version:
Problem statement:
When doing an overlay of a type that has an
allOfwith a reference to another overlay, the subset overlay is being created as the alias in the generated file.I've added commits to the original problem report to move the test to a more appropriate place, and fix the resulting issues. This problem is a bug in how
allOfhandles the merging of extended properties, and the overlays aren't relevant directly.