Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

Nivl
Copy link
Contributor

@Nivl Nivl commented Apr 22, 2025

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
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
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
Copy link
Contributor Author

Nivl commented May 6, 2025

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