openapi-generators / openapi-python-client

Generate modern Python clients from OpenAPI
MIT License
1.28k stars 197 forks source link

validate value of 'const' properties (helps with discriminated unions) #1024

Closed peter-greenatlas closed 5 months ago

peter-greenatlas commented 5 months ago

For 'const' properties, the parsed value will now be checked against the value in the schema at runtime and a ValueError will be raised if they do not match. Until now, the values would just be cast to a Literal without checking.

This helps with with parsing oneOf when matching a const property is the only way to resolve which of the types should be returned. i.e. if a oneOf contains two schemas, but the properties in one are a subset of the other, then the only way to know which one to return is to validate the const value.

I know that for the most part this library parses but doesn't validate, but I don't see a way around it in this case. The spec itself mentions validation when talking about oneOf:

In OAS 3.0, a response payload MAY be described to be exactly one of any number of types:
[...]
which means the payload MUST, by validation, match exactly one of the schemas described by Cat, Dog, or Lizard.

Note that this also addresses some of the use cases for discriminated unions. Per the spec, the a discriminator MAY act as a "hint" to shortcut validation and selection of the matching schema which may be a costly operation, depending on the complexity of the schema. so implementing discriminators would be a speedup but with this change we'll at least construct the correct type.

I'm happy to take any feedback, or abandon this if you don't like fact that it is validation. I originally went to try to implement discriminated unions fully, but this seemed like a much easier step towards that that gets the same result.

dbanty commented 5 months ago

I think this aligns with the rest of the project. For example, if a string that's supposed to be of format date-time cannot be parsed, a ValueError is also thrown (from dateutil). We will mark this as a breaking change so folks are aware but I don't think it strays from existing practice.