Skip to content

fix(codegen): allow using x-go-type and x-go-type-skip-optional-pointer together#1957

Merged
jamietanna merged 2 commits into
oapi-codegen:mainfrom
Nivl:ml/fix-SkipOptionalPointer-with-extPropGoType
May 7, 2025
Merged

fix(codegen): allow using x-go-type and x-go-type-skip-optional-pointer together#1957
jamietanna merged 2 commits into
oapi-codegen:mainfrom
Nivl:ml/fix-SkipOptionalPointer-with-extPropGoType

Conversation

@Nivl

@Nivl Nivl commented Apr 22, 2025

Copy link
Copy Markdown
Contributor

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 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 GenerateGoSchema to:

  1. Add tests
  2. Not return early (to avoid having the same issue on). We return early at multiple places in this method.

This isn't the goal of that PR though, so maybe in another one.

@Nivl Nivl requested a review from a team as a code owner April 22, 2025 05:43
@jamietanna

Copy link
Copy Markdown
Member

Thanks! Will take a look hopefully today, and see if I can get some tests added to cover this

@jamietanna jamietanna self-assigned this May 5, 2025
@jamietanna jamietanna added bug Something isn't working area:extensions labels May 5, 2025
@jamietanna

Copy link
Copy Markdown
Member

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 allOf does actually provide some differences in the generated code for a case I'm interested in, but can't seem to see this PR as-is changing much, but maybe I'm missing something?

@Nivl

Nivl commented May 6, 2025

Copy link
Copy Markdown
Contributor Author

With this

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: pgtype.Timestamptz
            x-go-type-import:
              path: github.com/jackc/pgx/v5/pgtype
              name: pgtype

      responses:
        "200":
          description: Some data

I expect something like that:

type GetRootParams struct {
	At pgtype.Timestamptz `form:"at,omitempty" json:"at,omitempty"`
}

instead of

type GetRootParams struct {
	At *pgtype.Timestamptz `form:"at,omitempty" json:"at,omitempty"`
}

Currently, If I remove all the x-go- stuff I get:

type GetRootParams struct {
	At *time.Time `form:"at,omitempty" json:"at,omitempty"`
}

Which is correct.
if I set x-go-type-skip-optional-pointer: true I get

type GetRootParams struct {
	At time.Time `form:"at,omitempty" json:"at,omitempty"`
}

Which is also correct.
If I set

            x-go-type: pgtype.Timestamptz
            x-go-type-import:
              path: github.com/jackc/pgx/v5/pgtype
              name: pgtype

I get

type GetRootParams struct {
	At *pgtype.Timestamptz `form:"at,omitempty" json:"at,omitempty"`
}

Which is still correct.
But if I set both:

            x-go-type-skip-optional-pointer: true
            x-go-type: pgtype.Timestamptz
            x-go-type-import:
              path: github.com/jackc/pgx/v5/pgtype
              name: pgtype

I get:

type GetRootParams struct {
	At *pgtype.Timestamptz `form:"at,omitempty" json:"at,omitempty"`
}

Which is incorrect.

This PR should be fixing that, and makes sure we're not using a pointer when x-go-type-skip-optional-pointer is set to true.

@jamietanna jamietanna added this to the v2.5.0 milestone May 7, 2025
@jamietanna

Copy link
Copy Markdown
Member

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: googleuuid

It looks like SomeType is generated correctly before this change, but not the query parameter

I'll add a bit more of a test to show the before/after for this, thanks!

@jamietanna jamietanna force-pushed the ml/fix-SkipOptionalPointer-with-extPropGoType branch from 606f819 to 136c334 Compare May 7, 2025 07:31
@jamietanna jamietanna force-pushed the ml/fix-SkipOptionalPointer-with-extPropGoType branch from 136c334 to a720a1f Compare May 7, 2025 07:35
jamietanna and others added 2 commits May 7, 2025 08:37
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`.
@jamietanna jamietanna force-pushed the ml/fix-SkipOptionalPointer-with-extPropGoType branch from a720a1f to cf79d5e Compare May 7, 2025 07:38
@jamietanna jamietanna merged commit cc72767 into oapi-codegen:main May 7, 2025
19 checks passed
mromaszewicz added a commit to nelzblooket/oapi-codegen that referenced this pull request Apr 23, 2026
…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>
mromaszewicz added a commit that referenced this pull request Apr 23, 2026
…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>
mromaszewicz added a commit to turip/oapi-codegen that referenced this pull request Apr 29, 2026
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.
mromaszewicz added a commit that referenced this pull request Apr 29, 2026
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:extensions bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants