fix(codegen): allow using x-go-type and x-go-type-skip-optional-pointer together#1957
Conversation
|
Thanks! Will take a look hopefully today, and see if I can get some tests added to cover this |
|
Do you have an example (minimal spec) that this change improves the codegen for? In my limited testing, I can't seem to get much meaningfully different when applying this PR I can see that looking a this with an |
|
With this I expect something like that: instead of Currently, If I remove all the Which is correct. Which is also correct. I get Which is still correct. I get: Which is incorrect. This PR should be fixing that, and makes sure we're not using a pointer when |
|
Thanks! From my testing: openapi: "3.0.0"
info:
version: 1.0.0
title: Generate models # TODO
paths:
/root:
get:
operationId: getRoot
parameters:
- in: query
name: at
schema:
type: string
format: date-time
x-go-type-skip-optional-pointer: true
x-go-type: googleuuid.UUID
x-go-type-import:
path: github.com/google/uuid
name: googleuuid
responses:
"200":
description: Some data
components:
schemas:
SomeType:
type: object
properties:
at:
type: string
format: date-time
x-go-type-skip-optional-pointer: true
x-go-type: googleuuid.UUID
x-go-type-import:
path: github.com/google/uuid
name: googleuuidIt looks like I'll add a bit more of a test to show the before/after for this, thanks! |
606f819 to
136c334
Compare
136c334 to
a720a1f
Compare
As part of future changes, we'll be modifying the way this works, so we should capture the current behaviour.
…er together When using `x-go-type`, `GenerateGoSchema` returns early, meaning that any extensions that are checked after this one are no-op. This PR moves the code for `x-go-type-skip-optional-pointer` higher up, so it can be used alongside `x-go-type`. This corrects this piece of behaviour, and amends our generated code examples to correctly apply `x-go-type-skip-optional-pointer` when using an optional field, or inside an `AllOf`.
a720a1f to
cf79d5e
Compare
…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>
…nicely (#2087) * demonstration of overlays and allOf not playing together nicely * Reorganize bug test case 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 #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> * fix: honor x-go-type on allOf schemas, stop extension leakage (#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 #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> * Implement Greptile suggestions 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> * Defensively clone extensions before deleting identity keys 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> --------- Co-authored-by: Marcin Romaszewicz <marcinr@gmail.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Address greptile review feedback and adapt to upstream main: - pkg/codegen/schema.go: simplify the allOf branch. Upstream main now catches x-go-type at the schema level via the combined extensions early return, so the PR's allOf-specific x-go-type override block is unreachable; remove it. Keep x-go-type-skip-optional-pointer propagation as a conditional override (only when the parent sets it explicitly), which preserves the decorator-idiom behavior MergeSchemas relies on for issue oapi-codegen#1957. Read from combined extensions for parity with the rest of GenerateGoSchema. - pkg/codegen/test_specs/x-go-type-pet-allof.yaml: add trailing newline.
…1610) * fix: allow x-go-type and x-go-type-skip-optional-pointer for allOf This patch ensures that if a schema is defined using allOf the main schema entry's x-go-type and x-go-type-skip-optional-pointer values are considered instead of a random allOf member's. * Add unit tests * update PR for upstream changes Address greptile review feedback and adapt to upstream main: - pkg/codegen/schema.go: simplify the allOf branch. Upstream main now catches x-go-type at the schema level via the combined extensions early return, so the PR's allOf-specific x-go-type override block is unreachable; remove it. Keep x-go-type-skip-optional-pointer propagation as a conditional override (only when the parent sets it explicitly), which preserves the decorator-idiom behavior MergeSchemas relies on for issue #1957. Read from combined extensions for parity with the rest of GenerateGoSchema. - pkg/codegen/test_specs/x-go-type-pet-allof.yaml: add trailing newline. --------- Co-authored-by: Jamie Tanna <jamie.tanna@elastic.co> Co-authored-by: Marcin Romaszewicz <mromaszewicz@users.noreply.github.com> Co-authored-by: Marcin Romaszewicz <marcinr@gmail.com>
When using
x-go-type,GenerateGoSchemareturns early, meaning that any extensions that are checked after this one are no-op.This PR moves the code for
x-go-type-skip-optional-pointerhigher up, so it can be used alongsidex-go-type.This PR only fixes a bug, not the root issue. There may be more similar issues, and the code makes it easy to introduce new similar bugs. Ultimately we'd need to refactor
GenerateGoSchemato:This isn't the goal of that PR though, so maybe in another one.