Skip to content

feat: Implement discriminators knowledge in code generation BNCH-30443 #87

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
GitOnUp merged 9 commits into from
Sep 25, 2021
Merged

feat: Implement discriminators knowledge in code generation BNCH-30443 #87

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
1d4fa96
Discriminator support
GitOnUp Sep 20, 2021
0632dff
Remove debugging prints
GitOnUp Sep 20, 2021
fd1f32b
WIP
GitOnUp Sep 24, 2021
bd5a1bf
Move json checkk to inner type
GitOnUp Sep 24, 2021
ad3ed2a
Move template to benchling-sdk
GitOnUp Sep 24, 2021
0195d84
Revert endpoint change
GitOnUp Sep 24, 2021
03c3dc2
Avoid unioning if for properties without templates, only allow one cl…
GitOnUp Sep 24, 2021
32d7c19
properties without templates can now have UnknownType
GitOnUp Sep 25, 2021
4b0275a
Merge branch 'benchling-sdk-m1-fixes-01282021' of github.com:benchlin…
GitOnUp Sep 25, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions openapi_python_client/parser/properties/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,8 @@ class UnionProperty(Property):
inner_properties: List[Property]
template: ClassVar[str] = "union_property.pyi"
has_properties_without_templates: bool = attr.ib(init=False)
discriminator_property: Optional[str]
discriminator_mappings: Dict[str, Property]

def __attrs_post_init__(self) -> None:
super().__attrs_post_init__()
Expand Down Expand Up @@ -423,13 +425,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
Copy link
Author

@GitOnUp GitOnUp Sep 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

discriminator_mappings: Dict[str, Property] = {}
for sub_prop_data in chain(data.anyOf, data.oneOf):
sub_prop, schemas = property_from_data(
name=name, required=required, data=sub_prop_data, schemas=schemas, parent_name=parent_name
)
if isinstance(sub_prop, PropertyError):
return PropertyError(detail=f"Invalid property in union {name}", data=sub_prop_data), schemas

sub_properties.append(sub_prop)
if data.discriminator is not None:
discriminated_by = inverted_mappings.get(sub_prop.reference.class_name)
if discriminated_by is not None:
discriminator_mappings[discriminated_by] = sub_prop

default = convert_chain((prop._type_string for prop in sub_properties), data.default)
return (
Expand All @@ -439,6 +450,8 @@ def build_union_property(
default=default,
inner_properties=sub_properties,
nullable=data.nullable,
discriminator_property=data.discriminator.propertyName if data.discriminator else None,
discriminator_mappings=discriminator_mappings
),
schemas,
)
Expand Down
11 changes: 11 additions & 0 deletions openapi_python_client/templates/property_templates/union_property.pyi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 %}
Copy link
Author

@GitOnUp GitOnUp Sep 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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 %}
Copy link
Author

@GitOnUp GitOnUp Sep 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@GitOnUp GitOnUp Sep 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@packyg packyg Sep 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@packyg packyg Sep 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

{% for inner_property in property.inner_properties_with_template() %}
{% if not loop.last or property.has_properties_without_templates %}
try:
Expand Down