add discriminator property support
This more fully fixes https://github.com/openapi-generators/openapi-python-client/issues/219, and supersedes https://github.com/openapi-generators/openapi-python-client/pull/717.
Any valid discriminator definition as described in OpenAPI 3.1.0 should work correctly with these changes, including:
- Discriminators with a
mappingthat specifies all valid discriminator values. - Discriminators with no
mapping, in which the discriminator value for each type is the simple name of the type (equivalent to{"Model1": {"$ref": "#/components/schemas/Model1"}}). - Discriminators with a
mappingthat specifies values for some classes, but not all of them; in this case the default for the unspecified classes is the same as if there was nomapping. - Unions of unions where each of the sub-unions has its own discriminator mapping.
Validation during parsing is not as thorough as it could be, since currently the property lists inside ModelProperty are not available at the time the UnionProperty is parsed (I do have some further changes in mind to improve this, but I wanted to keep the scope of this PR manageable). But there is some basic validation, like "you can't have a non-object schema, or an inline schema, if there's a discriminator"... so this is a breaking change in the sense that generation will now fail on some invalid specs that previously would have passed.
Here's a case where I'm not 100% sure what the behavior should be:
components:
schemas:
ModelA:
type: object
properties:
modelType:
type: string
ModelB:
type: object
properties:
modelType:
type: string
NullType:
type: null
NullableUnionType:
oneOf:
- $ref: "#/components/schemas/ModelA"
- $ref: "#/components/schemas/ModelB"
- $ref: "#/components/schemas/NullType"
discriminator:
property: modelType
Should this be considered valid? In my current implementation, it is not valid, because that's my literal reading of this statement from the spec: "The expectation now is that a property with name [the discriminator property name] MUST be present in the response payload." If the value is null, then it does not have a modelType property.
However, like many things in OpenAPI, the spec language doesn't really say "if you do this, it's a malformed spec"; it's just a thing that would not have a well-defined behavior if you were writing a validator or a client generator. When someone raised a question about this scenario in the OpenAPI spec repo, the official answer was basically that. They just advised people that instead of writing their specs that way, to avoid ambiguity they should do it like this:
NullableUnionType:
oneOf:
- $ref: "#/components/schemas/NullType"
- oneOf:
- $ref: "#/components/schemas/ModelA"
- $ref: "#/components/schemas/ModelB"
discriminator:
property: modelType
My implementation does support that nested approach. What I'm wondering is whether it would make sense to loosen it so it also supports the questionably-valid shorter version, i.e. have a specific loophole for letting one of the variants be null, simply because I'm guessing it's not uncommon for people to write specs that way.
And if so, should it also allow an inline - type: null instead of a named NullType? The OpenAPI language says that "when using the discriminator, inline specs will not be considered"... but in this example, it arguably would not be "using" the discriminator in a case where the deserialization logic detected a None value and returned early.
Putting this back to draft because I have a couple ideas to simplify/clarify things.
OK, I pushed some more changes to simplify things a bit.
There was some extra validation that I could add in a follow-up PR, which I don't think is essential to the feature. I was also thinking about adding a new category of end-to-end tests that verify the behavior of the generated code (importing a generated model class and calling from_dict on it, etc.), but I could do that as a follow-up too and it's not specific to this feature.