Skip to content

Fix deserialization of unions #332

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
merged 23 commits into from
Mar 15, 2021
Merged

Fix deserialization of unions #332

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
4e30205
Fix tests
forest-benchling Feb 9, 2021
fda721a
Manually apply diff
forest-benchling Feb 9, 2021
10926f1
Regenerate tests
forest-benchling Feb 9, 2021
465943d
Fix tests
forest-benchling Feb 9, 2021
9ec7394
Fix tests
forest-benchling Feb 9, 2021
db33752
Remove unnecessary check
forest-benchling Feb 9, 2021
49185f0
Address union of one type
forest-benchling Feb 9, 2021
b71f718
Fix union of inline models
forest-benchling Feb 10, 2021
c251560
Fix union
forest-benchling Feb 10, 2021
0d79ce8
Merge upstream/main
forest-benchling Feb 10, 2021
3c536f7
More tests
forest-benchling Feb 10, 2021
8a72732
Change parse annotation to object
forest-benchling Feb 10, 2021
b5fa15d
Address suggestions
forest-benchling Feb 10, 2021
43969ae
Make inner types into a set, fix List type string, add tests
forest-benchling Feb 11, 2021
2c4fdf8
Merge upstream/main
forest-benchling Feb 11, 2021
caea77a
Add more E2E tests
forest-benchling Feb 11, 2021
da1d970
Merge upstream/main
forest-benchling Feb 11, 2021
682c5ab
Merge #335
forest-benchling Feb 11, 2021
39faebf
Merge remote-tracking branch 'upstream/main' into forest-union-3
forest-benchling Mar 3, 2021
55496e5
Add tests
forest-benchling Mar 3, 2021
1ddd486
Change item to type
forest-benchling Mar 3, 2021
8bc76af
Merge remote-tracking branch 'upstream/main' into forest-union-3
forest-benchling Mar 15, 2021
0d90970
Add another unit test
forest-benchling Mar 15, 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
45 changes: 35 additions & 10 deletions end_to_end_tests/golden-record-custom/custom_e2e/api/tests/defaults_tests_defaults_post.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,13 @@
Client = httpx.Client

import datetime
from typing import Dict, List, Union
from typing import Dict, List, Optional, Union

from dateutil.parser import isoparse

from ...models.an_enum import AnEnum
from ...models.http_validation_error import HTTPValidationError
from ...models.model_with_union_property import ModelWithUnionProperty
from ...types import UNSET, Unset


Expand Down Expand Up @@ -41,7 +42,10 @@ def httpx_request(
*,
client: Client,
string_prop: Union[Unset, str] = "the default string",
datetime_prop: Union[Unset, datetime.datetime] = isoparse("1010-10-10T00:00:00"),
not_required_not_nullable_datetime_prop: Union[Unset, datetime.datetime] = isoparse("1010-10-10T00:00:00"),
not_required_nullable_datetime_prop: Union[Unset, None, datetime.datetime] = isoparse("1010-10-10T00:00:00"),
required_not_nullable_datetime_prop: datetime.datetime = isoparse("1010-10-10T00:00:00"),
required_nullable_datetime_prop: Optional[datetime.datetime] = isoparse("1010-10-10T00:00:00"),
date_prop: Union[Unset, datetime.date] = isoparse("1010-10-10").date(),
float_prop: Union[Unset, float] = 3.14,
int_prop: Union[Unset, int] = 7,
Expand All @@ -50,11 +54,24 @@ def httpx_request(
union_prop: Union[Unset, float, str] = "not a float",
union_prop_with_ref: Union[Unset, float, AnEnum] = 0.6,
enum_prop: Union[Unset, AnEnum] = UNSET,
model_prop: Union[Unset, ModelWithUnionProperty] = UNSET,
) -> Response[Union[None, HTTPValidationError]]:

json_datetime_prop: Union[Unset, str] = UNSET
if not isinstance(datetime_prop, Unset):
json_datetime_prop = datetime_prop.isoformat()
json_not_required_not_nullable_datetime_prop: Union[Unset, str] = UNSET
if not isinstance(not_required_not_nullable_datetime_prop, Unset):
json_not_required_not_nullable_datetime_prop = not_required_not_nullable_datetime_prop.isoformat()

json_not_required_nullable_datetime_prop: Union[Unset, None, str] = UNSET
if not isinstance(not_required_nullable_datetime_prop, Unset):
json_not_required_nullable_datetime_prop = (
not_required_nullable_datetime_prop.isoformat() if not_required_nullable_datetime_prop else None
)

json_required_not_nullable_datetime_prop = required_not_nullable_datetime_prop.isoformat()

json_required_nullable_datetime_prop = (
required_nullable_datetime_prop.isoformat() if required_nullable_datetime_prop else None
)

json_date_prop: Union[Unset, str] = UNSET
if not isinstance(date_prop, Unset):
Expand All @@ -74,24 +91,31 @@ def httpx_request(
else:
json_union_prop = union_prop

json_union_prop_with_ref: Union[Unset, float, AnEnum]
json_union_prop_with_ref: Union[Unset, float, str]
if isinstance(union_prop_with_ref, Unset):
json_union_prop_with_ref = UNSET
elif isinstance(union_prop_with_ref, AnEnum):
json_union_prop_with_ref = UNSET
if not isinstance(union_prop_with_ref, Unset):
json_union_prop_with_ref = union_prop_with_ref
json_union_prop_with_ref = union_prop_with_ref.value

else:
json_union_prop_with_ref = union_prop_with_ref

json_enum_prop: Union[Unset, AnEnum] = UNSET
json_enum_prop: Union[Unset, str] = UNSET
if not isinstance(enum_prop, Unset):
json_enum_prop = enum_prop
json_enum_prop = enum_prop.value

json_model_prop: Union[Unset, Dict[str, Any]] = UNSET
if not isinstance(model_prop, Unset):
json_model_prop = model_prop.to_dict()

params: Dict[str, Any] = {
"string_prop": string_prop,
"datetime_prop": json_datetime_prop,
"not_required_not_nullable_datetime_prop": json_not_required_not_nullable_datetime_prop,
"not_required_nullable_datetime_prop": json_not_required_nullable_datetime_prop,
"required_not_nullable_datetime_prop": json_required_not_nullable_datetime_prop,
"required_nullable_datetime_prop": json_required_nullable_datetime_prop,
"date_prop": json_date_prop,
"float_prop": float_prop,
"int_prop": int_prop,
Expand All @@ -100,6 +124,7 @@ def httpx_request(
"union_prop": json_union_prop,
"union_prop_with_ref": json_union_prop_with_ref,
"enum_prop": json_enum_prop,
"model_prop": json_model_prop,
}
params = {k: v for k, v in params.items() if v is not UNSET and v is not None}

Expand Down
183 changes: 175 additions & 8 deletions end_to_end_tests/golden-record-custom/custom_e2e/models/a_model.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import datetime
from typing import Any, Dict, List, Optional, Type, TypeVar, Union
from typing import Any, Dict, List, Optional, Type, TypeVar, Union, cast

import attr
from dateutil.parser import isoparse
Expand All @@ -10,6 +10,8 @@
from ..models.a_model_nullable_model import AModelNullableModel
from ..models.an_enum import AnEnum
from ..models.different_enum import DifferentEnum
from ..models.free_form_model import FreeFormModel
from ..models.model_with_union_property import ModelWithUnionProperty
from ..types import UNSET, Unset

T = TypeVar("T", bound="AModel")
Expand All @@ -23,17 +25,21 @@ class AModel:
a_camel_date_time: Union[datetime.datetime, datetime.date]
a_date: datetime.date
required_not_nullable: str
one_of_models: Union[FreeFormModel, ModelWithUnionProperty]
model: AModelModel
a_nullable_date: Optional[datetime.date]
required_nullable: Optional[str]
nullable_one_of_models: Union[None, FreeFormModel, ModelWithUnionProperty]
nullable_model: Optional[AModelNullableModel]
nested_list_of_enums: Union[Unset, List[List[DifferentEnum]]] = UNSET
a_not_required_date: Union[Unset, datetime.date] = UNSET
attr_1_leading_digit: Union[Unset, str] = UNSET
not_required_nullable: Union[Unset, Optional[str]] = UNSET
not_required_nullable: Union[Unset, None, str] = UNSET
not_required_not_nullable: Union[Unset, str] = UNSET
not_required_model: Union[AModelNotRequiredModel, Unset] = UNSET
not_required_nullable_model: Union[Optional[AModelNotRequiredNullableModel], Unset] = UNSET
not_required_one_of_models: Union[Unset, FreeFormModel, ModelWithUnionProperty] = UNSET
not_required_nullable_one_of_models: Union[Unset, None, FreeFormModel, ModelWithUnionProperty, str] = UNSET
not_required_model: Union[Unset, AModelNotRequiredModel] = UNSET
not_required_nullable_model: Union[Unset, None, AModelNotRequiredNullableModel] = UNSET

def to_dict(self) -> Dict[str, Any]:
an_enum_value = self.an_enum_value.value
Expand All @@ -46,6 +52,12 @@ def to_dict(self) -> Dict[str, Any]:

a_date = self.a_date.isoformat()
required_not_nullable = self.required_not_nullable
if isinstance(self.one_of_models, FreeFormModel):
one_of_models = self.one_of_models.to_dict()

else:
one_of_models = self.one_of_models.to_dict()

model = self.model.to_dict()

nested_list_of_enums: Union[Unset, List[Any]] = UNSET
Expand All @@ -69,13 +81,53 @@ def to_dict(self) -> Dict[str, Any]:
required_nullable = self.required_nullable
not_required_nullable = self.not_required_nullable
not_required_not_nullable = self.not_required_not_nullable
nullable_one_of_models: Union[None, Dict[str, Any]]
if self.nullable_one_of_models is None:
nullable_one_of_models = None
elif isinstance(self.nullable_one_of_models, FreeFormModel):
nullable_one_of_models = self.nullable_one_of_models.to_dict()

else:
nullable_one_of_models = self.nullable_one_of_models.to_dict()

not_required_one_of_models: Union[Unset, Dict[str, Any]]
if isinstance(self.not_required_one_of_models, Unset):
not_required_one_of_models = UNSET
elif isinstance(self.not_required_one_of_models, FreeFormModel):
not_required_one_of_models = UNSET
if not isinstance(self.not_required_one_of_models, Unset):
not_required_one_of_models = self.not_required_one_of_models.to_dict()
Comment on lines +97 to +99
Copy link
Collaborator

Choose a reason for hiding this comment

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

Will have to check later why this is the case. I assume there's some difficulty with handling the type in the template which is why we're checking multiple isinstances here.

Copy link
Collaborator Author

@forest-benchling forest-benchling Feb 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This is the OpenAPI:

          "not_required_one_of_models": {
            "oneOf": [
              {
                "ref": "#components/schemas/FreeFormModel"
              },
              {
                "ref": "#components/schemas/ModelWithUnionProperty"
              }
            ],
            "nullable": false
          },

My guess is that the inner properties are not required? Kind of confusing what the semantics should be...


else:
not_required_one_of_models = UNSET
if not isinstance(self.not_required_one_of_models, Unset):
not_required_one_of_models = self.not_required_one_of_models.to_dict()

not_required_nullable_one_of_models: Union[Unset, None, Dict[str, Any], str]
if isinstance(self.not_required_nullable_one_of_models, Unset):
not_required_nullable_one_of_models = UNSET
elif self.not_required_nullable_one_of_models is None:
not_required_nullable_one_of_models = None
elif isinstance(self.not_required_nullable_one_of_models, FreeFormModel):
not_required_nullable_one_of_models = UNSET
if not isinstance(self.not_required_nullable_one_of_models, Unset):
not_required_nullable_one_of_models = self.not_required_nullable_one_of_models.to_dict()

elif isinstance(self.not_required_nullable_one_of_models, ModelWithUnionProperty):
not_required_nullable_one_of_models = UNSET
if not isinstance(self.not_required_nullable_one_of_models, Unset):
not_required_nullable_one_of_models = self.not_required_nullable_one_of_models.to_dict()

else:
not_required_nullable_one_of_models = self.not_required_nullable_one_of_models

nullable_model = self.nullable_model.to_dict() if self.nullable_model else None

not_required_model: Union[Unset, Dict[str, Any]] = UNSET
if not isinstance(self.not_required_model, Unset):
not_required_model = self.not_required_model.to_dict()

not_required_nullable_model: Union[None, Unset, Dict[str, Any]] = UNSET
not_required_nullable_model: Union[Unset, None, Dict[str, Any]] = UNSET
if not isinstance(self.not_required_nullable_model, Unset):
not_required_nullable_model = (
self.not_required_nullable_model.to_dict() if self.not_required_nullable_model else None
Expand All @@ -88,9 +140,11 @@ def to_dict(self) -> Dict[str, Any]:
"aCamelDateTime": a_camel_date_time,
"a_date": a_date,
"required_not_nullable": required_not_nullable,
"one_of_models": one_of_models,
"model": model,
"a_nullable_date": a_nullable_date,
"required_nullable": required_nullable,
"nullable_one_of_models": nullable_one_of_models,
"nullable_model": nullable_model,
}
)
Expand All @@ -104,6 +158,10 @@ def to_dict(self) -> Dict[str, Any]:
field_dict["not_required_nullable"] = not_required_nullable
if not_required_not_nullable is not UNSET:
field_dict["not_required_not_nullable"] = not_required_not_nullable
if not_required_one_of_models is not UNSET:
field_dict["not_required_one_of_models"] = not_required_one_of_models
if not_required_nullable_one_of_models is not UNSET:
field_dict["not_required_nullable_one_of_models"] = not_required_nullable_one_of_models
if not_required_model is not UNSET:
field_dict["not_required_model"] = not_required_model
if not_required_nullable_model is not UNSET:
Expand All @@ -116,15 +174,18 @@ def from_dict(cls: Type[T], src_dict: Dict[str, Any]) -> T:
d = src_dict.copy()
an_enum_value = AnEnum(d.pop("an_enum_value"))

def _parse_a_camel_date_time(data: Any) -> Union[datetime.datetime, datetime.date]:
data = None if isinstance(data, Unset) else data
def _parse_a_camel_date_time(data: object) -> Union[datetime.datetime, datetime.date]:
a_camel_date_time: Union[datetime.datetime, datetime.date]
try:
if not isinstance(data, str):
raise TypeError()
a_camel_date_time = isoparse(data)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think letting isoparse raise the error (if we're going to raise it anyway) would be better because it would give more context as to what is wrong. It's especially confusing to readers to see this since the input type of the function is str, so why would this not be one?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh I see, this is here to get the correct Union option more easily? Maybe then we need to exclude it if no Union? Not really necessary for this PR, there are plenty of things like this that need cleanup at some point.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, yeah, this is just a special case when there's a union of only 1 element. I think it's pretty unlikely to happen in actual usage. Let me try to fix it though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Turns out to be unnecessary if we're changing the input type to be object.


return a_camel_date_time
except: # noqa: E722
pass
if not isinstance(data, str):
raise TypeError()
a_camel_date_time = isoparse(data).date()

return a_camel_date_time
Expand All @@ -135,6 +196,24 @@ def _parse_a_camel_date_time(data: Any) -> Union[datetime.datetime, datetime.dat

required_not_nullable = d.pop("required_not_nullable")

def _parse_one_of_models(data: object) -> Union[FreeFormModel, ModelWithUnionProperty]:
one_of_models: Union[FreeFormModel, ModelWithUnionProperty]
try:
if not isinstance(data, dict):
raise TypeError()
one_of_models = FreeFormModel.from_dict(data)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here re: confusion. If we can't be confident in the data passed in, the type annotation should be : Any and we do the runtime check. If we are confident of what's passed in, we can do the correct type annotation and not a runtime check.

I think the only reason this would be the wrong data type is if the OpenAPI spec was wrong or someone made a breaking change to the API. Either way we can't properly handle all the cases that could fail. So I think just the proper type annotation without the runtime check is probably the better approach?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, that's a good point. Actually initially I didn't have a runtime check, but then MyPy throws an error, because it's not type-safe.

I'm starting to think that maybe the original Any annotation is better, since we can't be confident in the data passed in. But I think it should be object, not Any, because Any causes type errors to be ignored, whereas object will force us to be type-safe.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds good regarding object vs Any if that ends up working better.


return one_of_models
except: # noqa: E722
pass
if not isinstance(data, dict):
raise TypeError()
one_of_models = ModelWithUnionProperty.from_dict(data)

return one_of_models

one_of_models = _parse_one_of_models(d.pop("one_of_models"))

model = AModelModel.from_dict(d.pop("model"))

nested_list_of_enums = []
Expand Down Expand Up @@ -167,12 +246,96 @@ def _parse_a_camel_date_time(data: Any) -> Union[datetime.datetime, datetime.dat

not_required_not_nullable = d.pop("not_required_not_nullable", UNSET)

def _parse_nullable_one_of_models(data: object) -> Union[None, FreeFormModel, ModelWithUnionProperty]:
nullable_one_of_models: Union[None, FreeFormModel, ModelWithUnionProperty]
if data is None:
return data
try:
if not isinstance(data, dict):
raise TypeError()
nullable_one_of_models = FreeFormModel.from_dict(data)

return nullable_one_of_models
except: # noqa: E722
pass
if not isinstance(data, dict):
raise TypeError()
nullable_one_of_models = ModelWithUnionProperty.from_dict(data)

return nullable_one_of_models

nullable_one_of_models = _parse_nullable_one_of_models(d.pop("nullable_one_of_models"))

def _parse_not_required_one_of_models(data: object) -> Union[Unset, FreeFormModel, ModelWithUnionProperty]:
not_required_one_of_models: Union[Unset, FreeFormModel, ModelWithUnionProperty]
if isinstance(data, Unset):
return data
try:
if not isinstance(data, dict):
raise TypeError()
not_required_one_of_models = UNSET
_not_required_one_of_models = data
if not isinstance(_not_required_one_of_models, Unset):
not_required_one_of_models = FreeFormModel.from_dict(_not_required_one_of_models)

return not_required_one_of_models
except: # noqa: E722
pass
if not isinstance(data, dict):
raise TypeError()
not_required_one_of_models = UNSET
_not_required_one_of_models = data
if not isinstance(_not_required_one_of_models, Unset):
not_required_one_of_models = ModelWithUnionProperty.from_dict(_not_required_one_of_models)

return not_required_one_of_models

not_required_one_of_models = _parse_not_required_one_of_models(d.pop("not_required_one_of_models", UNSET))

def _parse_not_required_nullable_one_of_models(
data: object,
) -> Union[Unset, None, FreeFormModel, ModelWithUnionProperty, str]:
not_required_nullable_one_of_models: Union[Unset, None, FreeFormModel, ModelWithUnionProperty, str]
if data is None:
return data
if isinstance(data, Unset):
return data
try:
if not isinstance(data, dict):
raise TypeError()
not_required_nullable_one_of_models = UNSET
_not_required_nullable_one_of_models = data
if not isinstance(_not_required_nullable_one_of_models, Unset):
not_required_nullable_one_of_models = FreeFormModel.from_dict(_not_required_nullable_one_of_models)

return not_required_nullable_one_of_models
except: # noqa: E722
pass
try:
if not isinstance(data, dict):
raise TypeError()
not_required_nullable_one_of_models = UNSET
_not_required_nullable_one_of_models = data
if not isinstance(_not_required_nullable_one_of_models, Unset):
not_required_nullable_one_of_models = ModelWithUnionProperty.from_dict(
_not_required_nullable_one_of_models
)

return not_required_nullable_one_of_models
except: # noqa: E722
pass
return cast(Union[Unset, None, FreeFormModel, ModelWithUnionProperty, str], data)

not_required_nullable_one_of_models = _parse_not_required_nullable_one_of_models(
d.pop("not_required_nullable_one_of_models", UNSET)
)

nullable_model = None
_nullable_model = d.pop("nullable_model")
if _nullable_model is not None:
nullable_model = AModelNullableModel.from_dict(_nullable_model)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm surprised mypy doesn't get mad about us passing an Any to a function which takes Dict[str, Any]

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, it's because Any has weird semantics, in that it basically just means "ignore any type errors". If we were to change the signature to object, then it would error.


not_required_model: Union[AModelNotRequiredModel, Unset] = UNSET
not_required_model: Union[Unset, AModelNotRequiredModel] = UNSET
_not_required_model = d.pop("not_required_model", UNSET)
if not isinstance(_not_required_model, Unset):
not_required_model = AModelNotRequiredModel.from_dict(_not_required_model)
Expand All @@ -187,6 +350,7 @@ def _parse_a_camel_date_time(data: Any) -> Union[datetime.datetime, datetime.dat
a_camel_date_time=a_camel_date_time,
a_date=a_date,
required_not_nullable=required_not_nullable,
one_of_models=one_of_models,
model=model,
nested_list_of_enums=nested_list_of_enums,
a_nullable_date=a_nullable_date,
Expand All @@ -195,6 +359,9 @@ def _parse_a_camel_date_time(data: Any) -> Union[datetime.datetime, datetime.dat
required_nullable=required_nullable,
not_required_nullable=not_required_nullable,
not_required_not_nullable=not_required_not_nullable,
nullable_one_of_models=nullable_one_of_models,
not_required_one_of_models=not_required_one_of_models,
not_required_nullable_one_of_models=not_required_nullable_one_of_models,
nullable_model=nullable_model,
not_required_model=not_required_model,
not_required_nullable_model=not_required_nullable_model,
Expand Down
Loading