feat: Implement discriminators knowledge in code generation BNCH-30443 #87
Conversation
| {% if property.discriminator_property != None %} | ||
| discriminator_value: str = data.get("{{property.discriminator_property}}") | ||
| if discriminator_value is not None: | ||
| {% for discriminated_by, inner_property in property.discriminator_mappings.items() %} | ||
| if discriminator_value == "{{discriminated_by}}": | ||
| {% from "property_templates/" + inner_property.template import construct %} | ||
| {{ construct(inner_property, "data", initial_value="UNSET") | indent(8) }} | ||
| return {{ property.python_name }} | ||
| {% endfor %} | ||
|
|
||
| {% endif %} |
There was a problem hiding this comment.
I originally wrote this to fall back nicely to the brute force approach if a discriminator is accidentally missing, but I think it's probably more correct to have that be enforced by the openapi linter and raise an error at the end of this if chain here if we haven't returned.
There was a problem hiding this comment.
Likewise, William had pointed out that we might want to have a default value for types for forward compatibility, but the existing mechanism doesn't do that, so we have to decide whether to add it for non-discriminated cases as well. I'm still thinking this through a little bit, but given that the UnionProperty type can also be used for "simple" types like str vs int (which can never be discriminated, if I'm reading the OpenAPI docs correctly), best case is the logic gets strange based on the types that can be unioned here.
I'm tempted to take a stand and say "if you want to support forward compatibility, use discriminators" and implement the "Unknown" case for that only.
There was a problem hiding this comment.
I'll need to return to this tomorrow after I've thought about it some, too.
But I think forward compatibility is the key consideration here. And the problems get only more acute the deeper a prop is embedded in the model hierarchy.
We could enforce your rule on discriminators for forward compatibility internally, but that might be a hard sell if we're trying to upstream this.
There was a problem hiding this comment.
best case is the logic gets strange based on the types that can be unioned here.
Can't we detect this from the value we get (as opposed to the spec)? Like if it's a primitive (i.e. just not a dict or list when serialized), then we don't do any deserialization - the raw value is the deserialzed value. If it is a dict, then we do the existing brute force approach (since in this case it'd be models union'ed with primitives for some reason and we can discriminate)
| @@ -9,6 +9,17 @@ def _parse_{{ property.python_name }}(data: {{ property.get_type_string(json=Tru | |||
| if isinstance(data, Unset): | |||
| return data | |||
| {% endif %} | |||
| {% if property.discriminator_property != None %} | |||
There was a problem hiding this comment.
This results in a chain that looks like this for RegisteredEntitiesList, which has a discriminator:
discriminator_value: str = data.get("entityType")
if discriminator_value is not None:
if discriminator_value == "dna_sequence":
entities_item = DnaSequence.from_dict(data)
return entities_item
if discriminator_value == "custom_entity":
entities_item = CustomEntity.from_dict(data)
return entities_item
if discriminator_value == "aa_sequence":
entities_item = AaSequence.from_dict(data)
return entities_item
if discriminator_value == "mixture":
entities_item = Mixture.from_dict(data)
return entities_item
if discriminator_value == "dna_oligo":
entities_item = DnaOligo.from_dict(data)
return entities_item
if discriminator_value == "rna_oligo":
entities_item = RnaOligo.from_dict(data)
return entities_item
GitOnUp
changed the title
| @@ -423,13 +427,22 @@ def build_union_property( | |||
| *, data: oai.Schema, name: str, required: bool, schemas: Schemas, parent_name: str | |||
| ) -> Tuple[Union[UnionProperty, PropertyError], Schemas]: | |||
| sub_properties: List[Property] = [] | |||
| inverted_mappings = {} | |||
| for k, v in (data.discriminator.mapping if data.discriminator else {}).items(): | |||
| inverted_mappings[Reference.from_ref(v).class_name] = k | |||
There was a problem hiding this comment.
AD: Check for existence, error if so, but we might want to support that case in the future (several mapped names can reference the same type, e.g. for deprecation purposes)
…g/openapi-python-client into george/discriminators
The bulk of logic for this change is in https://github.com/benchling/benchling-sdk/pull/180.
This change adds knowledge of type mappings in the union property type for usage downstream.