Open bterlson opened 2 years ago
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
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?
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.
Talking a bit offline with @bterlson and looking at some of the openapi3 specs issues. In this one on particular https://github.com/OAI/OpenAPI-Specification/issues/2165#issuecomment-592957945 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.
estimate: 8
remove from design board.
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 (https://github.com/RicoSuter/NSwag/issues/1245).
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
...
...
@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.
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.
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.
It is completely valid for a JSON schema to include both
properties
andallOf
.
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.
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
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:
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:
Considering that listing all constituents within allOf
is also a perfectly valid JSON Schema, I think it could be a more universal approach.
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:
Base
are replaced by an inlineoneOf
union referencing all of the derived types.Base
becomes an allOf of the derived types and we generate a new type with a name likeBaseCommon
that holds the cadlBase
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.