openapi-python-client icon indicating copy to clipboard operation
openapi-python-client copied to clipboard

add discriminator property support

Open eli-bl opened this issue 1 year ago • 3 comments

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 mapping that 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 mapping that 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 no mapping.
  • 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.

eli-bl avatar Oct 28 '24 18:10 eli-bl

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.

eli-bl avatar Oct 28 '24 22:10 eli-bl

Putting this back to draft because I have a couple ideas to simplify/clarify things.

eli-bl avatar Oct 30 '24 21:10 eli-bl

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.

eli-bl avatar Oct 31 '24 17:10 eli-bl