Skip to content

fix: Fix deserialization of unions BNCH-20706 #41

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.

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 12 commits into from
Jan 28, 2021

Conversation

forest-benchling
Copy link

@forest-benchling forest-benchling commented Jan 27, 2021

Upstream PR: https://github.com/triaxtec/openapi-python-client/compare/main...benchling:forest-union-2?expand=1

Deserialization of unions in from_dict was not working properly, because it failed to account for cases where the union property is None or Unset. This PR adds explicit checks for None or Unset.

In addition, the type of the value to deserialize was previously set to Any, which was masking type errors. In order to fix these type errors, I introduced a new concept of check_type_for_construct for each property, which allows the type to be checked before attempting deserialization.

In the future, I think we should refine the try/except pattern; it's not good practice to rely on a catch-all except when trying to deserialize each type within the union, because we don't know what exception was actually raised.

Follow-ups:

NOTE: This fails openapi-specs tests, because it's dependent on fixing https://benchling-jira.atlassian.net/browse/BNCH-20726.

@forest-benchling forest-benchling changed the title Union of models fix: Union of models BNCH-20706 Jan 28, 2021
Base automatically changed from forest-optional to main-v.0.7.3 January 28, 2021 13:07
@forest-benchling forest-benchling changed the title fix: Union of models BNCH-20706 fix: Fix deserialization of unions BNCH-20706 Jan 28, 2021
Copy link

@bowenwr bowenwr left a comment

Choose a reason for hiding this comment

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

Geez, thanks for working through this. Let's figure out a way today to get everything merged in, re-baseline the fixes branch and see if we can get everything stable again but feature complete.

@forest-benchling
Copy link
Author

@packyg I'm gonna merge this in the interest of time and avoiding juggling too many balls at the last minute. It would still be great if you could take a read of it when you have time, for context.

@forest-benchling forest-benchling merged commit 8cba8e9 into main-v.0.7.3 Jan 28, 2021
@forest-benchling forest-benchling deleted the forest-union branch January 28, 2021 19:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants