Invalid OA3 for unions

[bterlson]

We are not emitting proper OA3 for unions. In this example, I believe we need to emit oneOf somewhere. Right now we depend on references to the base type implicitly being a reference to some kind of union of anything that allOf's that type, but these semantics are not supported by the specification. @mikekistler agrees with this analysis.

oneOf two things need to happen for out output to be correct:

1. Any references to Base are replaced by an inline oneOf union referencing all of the derived types.
2. Base becomes an allOf of the derived types and we generate a new type with a name like BaseCommon that holds the cadl Base type's properties.

The latter is what @mikekistler prefers, and hinges on the fact that by putting discriminator on the base model you're essentially instructing cadl that "every time I reference this type, I'm referring to a union of its subtypes", but it does have the downside that we are generating a name in the openapi output (BaseCommon), and also munging a name users might expect to be generated as-is in client code (e.g. they might expect to see class A extends Base { } in their TS/C# code somewhere.

We have a third option here which is to delete @Discriminator on models and reserve it only for unions ala #335 where the Cadl author gives a good name to both the base type and the union separately.

[timotheeguerin]

I don't think oneOf is required for openapi3, in the specs there is a sample that use allOf uniquely https://swagger.io/specification/#schema-composition

[mikekistler]

This is a complicated topic so let's avoid potential confusion of using non-authoritative sources like swagger.io. Let's just use the official OpenAPI v3.0 spec at:

https://github.com/OAI/OpenAPI-Specification/blob/3.0.3/versions/3.0.3.md

The important part for this discussion is:

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.

Brian convinced me that what this really means is: Rather than repeating the property definition for the discriminator in every schema that will be named in an anyOf or oneOf, you can create a "parent schema" with the discriminator and allOf this into the "alternate schema" (of the anyOf or oneOf). So allOf does not replace an anyOf or oneOf, but rather it simply allows "avoiding redundancy" when constructing it.

@darrelmiller Could you give your thoughts on this?

[bterlson]

I saw the example @timotheeguerin posted and pondered for some time. The issue is it doesn't show usage of those types. I think to use dog and cat you would have to use a oneOf/anyOf at the usage site, or reference a schema that does so.

[timotheeguerin]

Talking a bit offline with @bterlson and looking at some of the openapi3 specs issues. In this one on particular OAI/OpenAPI-Specification#2165 (comment) there is some clarfication on the requirement of oneOf or anyOf

To be clear, the oneOf or enum constraint on Reptile isn't required for discriminator to work. If you're using an OpenAPI-compliant validator, it should extend validation to the specified schema as described above. The only purpose of the oneOf or enum in Reptile is to make sure that a request or response specified as Reptile will only pass validation if it is one of the known Reptile subtypes.

With this feels like we are doing the right thing for the extends scenario.

[markcowl]

estimate: 8

[markcowl]

remove from design board.

[mahomedalid]

Not sure if this is related, we are working in validate the generated openapi cadl schema against the generated openapi nswag schema (from code), we found that NSwag use to generate in the way cadl does few years ago and then they fixed it to match OA3 spec (RicoSuter/NSwag#1245).

[praneetloke]

I have an issue where the generated OAI 3 spec is incorrect in my opinion. I have no choice but to not use model inheritance.

model NameServersPreference {
    magicDNS: boolean;
}

model NameServers extends NameServersPreference {
    dns: string[];
}

which generates:

components:
...
...
    NameServers:
      type: object
      properties:
        dns:
          type: array
          items:
            type: string
          x-cadl-name: string[]
      required:
        - dns
      allOf:
        - $ref: '#/components/schemas/NameServersPreference'
    NameServersPreference:
      type: object
      properties:
        magicDNS:
          type: boolean
      required:
        - magicDNS
...
...

what I expected is this:

components:
...
...
    NameServers:
     # NameServers needs to have a list of models under allOf.
     # For ref: https://spec.openapis.org/oas/latest.html#models-with-composition
      allOf:
        - $ref: '#/components/schemas/NameServersPreference'
        - type: object
           properties:
             dns:
               type: array
               items:
                 type: string
               x-cadl-name: string[]
      required:
        - dns
    NameServersPreference:
      type: object
      properties:
        magicDNS:
          type: boolean
      required:
        - magicDNS
...
...

[bterlson]

@praneetloke can you say more about why you expected/want that output? I believe per JSON Schema both are valid, and they are more or less identical forms.

[praneetloke]

I believe per JSON Schema both are valid, and they are more or less identical forms.

To be honest, I didn't know it was legal per JSON schema. I just tried to lookup the spec for allOf and couldn't find examples of valid composition in JSON Schema. However, the OAI docs show examples where the downstream model only has an allOf definition that lists all of the objects and does not have a separate allOf definition in addition to an object definition as siblings.

[mikekistler]

It is completely valid for a JSON schema to include both properties and allOf. In fact, all JSON schema "assertions" (which is what properties is) and "applicators" (which is what allOf is) are fully independent. This is one of the design philosophies of JSON schema.

[praneetloke]

It is completely valid for a JSON schema to include both properties and allOf.

Yeah I just came across the topic of extending closed schemas where there is one such example.

I have to admit that in pretty much every use case of allOf that I have come across in OpenAPI specs, the inheriting model's own properties and the inherited properties are under allOf. That isn't to say that that is the only correct way to do model composition.

[bterlson]

I think we can agree that our output is valid, but are there downstream tools that expect it the allOf form of composition? If so I wouldn't be against a change or adding a flag

[mrnateriver]

NameServers:
  type: object
  properties:
    dns:
      type: array
  allOf:
    - $ref: '#/components/schemas/NameServersPreference'

Even though such type of composition is a valid JSON Schema object, it seems to break the following code generators that I've tried:

1. OpenAPITools/openapi-generator - kotlin
2. cjbooms/fabrikt

The first one yields empty models, the latter fails with an error that explicitly says that such composition structure is not supported.

This codegen, however, works fine:

1. OpenAPITools/openapi-generator - java

Considering that listing all constituents within allOf is also a perfectly valid JSON Schema, I think it could be a more universal approach.