Discriminator in oneOf unions appears twice in response

[happenslol]

What version of ogen are you using?

$ go list -m github.com/ogen-go/ogen
# github.com/ogen-go/ogen v1.1.0

Can this issue be reproduced with the latest version?

Yes

What did you do?

I'm using the following schema:

openapi: 3.0.0
info: {title: Test}
paths:
  /test:
    post:
      operationId: test
      responses:
        '200':
          content:
            application/json:
              schema:
                $ref: '#/components/schemas/Union'
components:
  schemas:
    Union:
      oneOf:
        - $ref: '#/components/schemas/First'
        - $ref: '#/components/schemas/Second'
      discriminator:
        propertyName: type
        mapping:
          first: '#/components/schemas/First'
          second: '#/components/schemas/Second'
    First:
      type: object
      required: [type, foo]
      properties:
        type: {type: string, enum: [first]}
        foo: {type: integer}
    Second:
      type: object
      required: [type, bar]
      properties:
        type: {type: string, enum: [second]}
        bar: {type: integer}

With this schema and a basic handler implementation that just returns the following:

func (h *handler) Test(ctx context.Context) (api.Union, error) {
	return api.Union{
		Type: api.FirstUnion,
		First: api.First{
			Type: api.FirstTypeFirst,
			Foo:  1,
		},
	}, nil
}

The type field is encoded twice in the response:

{
    "type": "first",
    "type": "first",
    "foo": 1
}

What did you expect to see?

The discriminator field should only be encoded once, e.g.:

{
    "type": "first",
    "foo": 1
}

What did you see instead?

As stated above, the field appears twice:

{
    "type": "first",
    "type": "first",
    "foo": 1
}

Even when leaving out the type field inside the First struct, I get the following:

{
    "type": "first",
    "type": "",
    "foo": 1
}

[dezlitz]

We also came up against a subtle bug with the way this works in ogen. In our case from v1.1.0 on we got this in the response.

{
    "type": "first",
    "type": "",
    "foo": 1
}

Our clients that were depending on this suddenly started seeing a blank string for the type field and errored out.

Root cause seems to be the way ogen handles the discriminator. If you omit the type field inside First and Second schemas like this,

components:
  schemas:
    Union:
      oneOf:
        - $ref: '#/components/schemas/First'
        - $ref: '#/components/schemas/Second'
      discriminator:
        propertyName: type
        mapping:
          first: '#/components/schemas/First'
          second: '#/components/schemas/Second'
    First:
      type: object
      required: [foo]
      properties:
        type: {enum: [first]}
        foo: {type: integer}
    Second:
      type: object
      required: [bar]
      properties:
        bar: {type: integer}

ogen will create the top level .Type property in the go struct which plays nice with the NewFirst() and NewSecond() helpers generated. Once the response is encoded to JSON, the value from the mapping is placed into the type json field along side other First and Second properites, (foo and bar in this case). This will ensure only one type field on the JSON response. Also, you will not see a nested .Type field in the First and Second go structs.

The problem with this approach is that it is not very explicit nor does it seem to comply with the OpenAPI spec,

In both the oneOf and anyOf use cases, all possible schemas MUST be listed explicitly. To avoid redundancy, the discriminator MAY be added to a parent schema definition, and all schemas comprising the parent schema in an allOf construct may be used as an alternate schema.

https://spec.openapis.org/oas/latest.html#discriminator-object

However, if we add both the type discriminator AND explicit type fields we get this go struct as mentioned above,

return api.Union{
   	Type: api.FirstUnion,
   	First: api.First{
   		Type: api.FirstTypeFirst,
   		Foo:  1,
   	},
   }, nil

The helper function NewFirst() will automatically set the parent .Type field but not the child. And when it comes to encoding this to JSON it will encode this field twice.

Ideally, we adhere to the OpenAPI spec and allow (require?) the discriminator to be set in the oneOf discriminator field AND inside each oneOf as a property. In that case, I would expect the child .Type fields in go not to be generated to avoid confusion.

[happenslol]

In my case, I'm generating my openapi schema from typespec, so sadly it's not an option to just remove the type from the union members.

[tdakkota]

In that case, I would expect the child .Type fields in go not to be generated to avoid confusion.

It causes problems if child schema used somewhere else. See #1155.

[happenslol]

It causes problems if child schema used somewhere else. See #1155.

I don't have a problem with the child type being generated. I just think it should not appear twice in the serialized response, since that's invalid json, and there's no workaround. Maybe this is something that should be fixed at the serialization level?

Edit: Just as a quick aside, why not just remove the type from the parent?

[happenslol]

Awesome, thanks a lot!

[dezlitz]

Just tested the latest commit, looks like the bug has been fixed by reverting to previous behaviour. 👏

It causes problems if child schema used somewhere else.

This makes sense. Maybe just needs to be documented that the child .Type property in the example above is simply ignored if used directly under a oneOf.