oapi-codegen icon indicating copy to clipboard operation
oapi-codegen copied to clipboard

fix: #1530 support discriminator with multiple mappings of same schema

Open mgabeler-lee-6rs opened this issue 1 year ago • 1 comments

This is an improved version of @jKiler's #1534. Credit to them for getting this started.

My main addition is getting the codegen for FromFoo and MergeFoo to work better in this situation.

mgabeler-lee-6rs avatar Oct 02 '24 18:10 mgabeler-lee-6rs

thanks a lot! that PR supersedes #1745 and works better 👍 and fixes #1668 too

mweibel avatar Oct 04 '24 12:10 mweibel

Maintainers / @mgabeler-lee-6rs what is the plan for this PR?

tkuik avatar Oct 28 '24 16:10 tkuik

I've been using this PR in my production app for some time now with good results. It's not perfect, it's vulnerable to GIGO if you have an invalid OAS spec, but no worse than the released code.

Contribution guidelines explicitly request not tagging maintainers directly :man_shrugging:

mgabeler-lee-6rs avatar Oct 28 '24 19:10 mgabeler-lee-6rs

would like to see this merged

gerhardwagner avatar Jan 17 '25 09:01 gerhardwagner

this PR would solve my problems 😸

benazir-sba avatar Jan 17 '25 09:01 benazir-sba

Would need this merged too! Does this PR fix the issue that untyped string constants are used within the generated files?

image

lukasbash avatar Feb 24 '25 06:02 lukasbash

Does this PR fix the issue that untyped string constants are used within the generated files?

I haven't seen that issue personally, so I can't say for sure. Probably the easiest way to find out is to just try it :slightly_smiling_face:

mgabeler-lee-6rs avatar Feb 24 '25 13:02 mgabeler-lee-6rs

I have the same issue as @lukasbash and this PR doesn't fix the issue for me

champtar avatar Mar 26 '25 21:03 champtar

@champtar or @lukasbash can you share a repro for the issue you're seeing?

mgabeler-lee-6rs avatar Mar 26 '25 22:03 mgabeler-lee-6rs

@mgabeler-lee-6rs here a minimal reproducer that break with v2.4.1

openapi: 3.0.4
info:
  title: test
  version: v1
paths:
  /aaa:
    get:
      operationId: aaa
      responses:
        '200':
          description: successful operation
          content:
            application/json:
              schema:
                $ref: '#/components/schemas/objX'
components:
  schemas:
    objstr:
      type: string
    objint:
      type: integer
    objX:
      anyOf:
        - $ref: '#/components/schemas/objstr'
        - $ref: '#/components/schemas/objint'
      discriminator:
        propertyName: mytype
        mapping:
          "int": '#/components/schemas/objint'
          "str": '#/components/schemas/objstr'

champtar avatar Mar 27 '25 00:03 champtar

@champtar I tried adding that as a test case on this branch and it produces compiling code not showing the pattern lukasbash reported. This seems to be in part because your example didn't include a definition for the mytype property, so it has no reference to that in the generated code (the type of "invalid specs produce GIGO" I referenced earlier). However even if I add the property as an enum, so that there's a generated type for it and not just an alias to string, the code still compiles just fine because ...

Assigning an untyped string constant to a type that is defined as a string ... is not an error.

Do you have some more complex setup outside your minimal repro where the discriminator field type is generated as something other than type Foo string such that assigning a string to it is actually an error?


Edited to add: Your example is additionally invalid because you are defining a discriminator property on an (implicitly) object schema, but the two discriminator possibilities both then say that it is not an object, so your schema is unsatisfiable.

mgabeler-lee-6rs avatar Mar 27 '25 13:03 mgabeler-lee-6rs

I've pushed up an adapted version of your example case, with the valid code it creates. If you can find further adjustments that show it producing invalid code, I'm happy to improve the PR :)

mgabeler-lee-6rs avatar Mar 27 '25 13:03 mgabeler-lee-6rs

@mgabeler-lee-6rs my example might be invalid, but it pass online validators like https://editor.swagger.io/ :(

I was trying to construct a minimal example and not share 5k lines of spec, here a better one actually closer to the spec I'm trying to deal with

openapi: 3.0.4
info:
  title: test
  version: v1
paths:
  /aaa:
    get:
      operationId: aaa
      responses:
        "200":
          description: successful operation
          content:
            application/json:
              schema:
                $ref: "#/components/schemas/objX"
components:
  schemas:
    objstr:
      type: object
      #required: [objectType]
      properties:
        objectType:
          type: string
          enum: ["str"]
    objint:
      type: object
      #required: [objectType]
      properties:
        objectType:
          type: string
          enum: ["int"]
    objX:
      anyOf:
        - $ref: "#/components/schemas/objstr"
        - $ref: "#/components/schemas/objint"
      discriminator:
        propertyName: objectType
        mapping:
          "int": "#/components/schemas/objint"
          "str": "#/components/schemas/objstr"

Basically the spec I'm dealing with is missing the required: [objectType]

https://spec.openapis.org/oas/v3.1.0.html#fixed-fields-20

The expectation now is that a property with name petType MUST be present in the response payload, and the value will correspond to the name of a schema defined in the OAS document.

champtar avatar Mar 27 '25 15:03 champtar

Aah, a schema with the discriminator field optional. So the issue isn't about using an untyped string constant, but rather using a string instead of a pointer to a string :heavy_check_mark:

I'll see if I can update this PR to handle that better. However it might be necessary to simply add x-go-type-skip-optional-pointer onto the discriminator fields

mgabeler-lee-6rs avatar Mar 27 '25 17:03 mgabeler-lee-6rs

How I read the spec (https://spec.openapis.org/oas/v3.1.0.html#fixed-fields-20) the field MUST be present, is there a notion of implicit required in OpenAPI ? Else it's just a buggy schema.

champtar avatar Mar 27 '25 18:03 champtar

It certainly reads kinda like it's an implicit required, but I'm not completely clear on that.

I've got something half-way working to process this, but the challenge I'm running into is that the point at which we are rendering the code that needs to know the type of e.g. Objstr.ObjectType, the codegen system doesn't have a reference to the real codegen info for the Objstr type, it only knows its name (because it was given via a $ref), so the codegen can't know if the discriminator field in it is normal, a pointer, or a nullable.

I've pushed up what I can do to make this better without invasive surgery, and the test example now shows the two options for how to get your example schema to generate valid code.

mgabeler-lee-6rs avatar Mar 27 '25 18:03 mgabeler-lee-6rs

@mgabeler-lee-6rs FYI in 3.1.1 it's clearer https://github.com/OAI/OpenAPI-Specification/blob/main/versions/3.1.1.md#fixed-fields-22

This property SHOULD be required in the payload schema, as the behavior when the property is absent is undefined.

champtar avatar Mar 29 '25 23:03 champtar

even more clear definition of said behavior will be available in the upcoming OAS 3.2 minor release slated for Summer '25. That doesn't fix the older versions, but it definitely provides direction for implementations which is backwards compatible with earlier versions.

https://github.com/Redocly/redocly-cli/issues/2028#issuecomment-2766747469

jeremyfiel avatar Apr 01 '25 00:04 jeremyfiel

Kusari Analysis Results

Analysis for commit: 2fef48ee9dd6cfbe123f43eb1a7972ad02b17681, performed at: 2025-07-16T14:00:57Z

@kusari-inspector rerun - Trigger a re-analysis of this PR

@kusari-inspector feedback [your message] - Send feedback to our AI and team


Recommendation

✅ PROCEED with this Pull Request

Summary

No Flagged Issues Detected

All values appear to be within acceptable risk parameters.

No pinned version dependency changes, code issues or exposed secrets detected!

Found this helpful? Give it a 👍 or 👎 reaction!

kusari-inspector[bot] avatar Jul 16 '25 14:07 kusari-inspector[bot]