Skip to content

adds Recursive Discriminator support #4913

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+38 −2
Open

adds Recursive Discriminator support #4913

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
36 changes: 34 additions & 2 deletions modules/openapi-generator/src/main/resources/python/python-experimental/model_utils.mustache
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -592,8 +592,11 @@ def deserialize_model(model_data, model_class, path_to_item, check_type,

used_model_class = model_class
if model_class.discriminator() is not None:
used_model_class = model_class.get_discriminator_class(
from_server, model_data)
used_model_class = recursive_discriminator_lookup(model_class, from_server, model_data)

# This is the case if the used_model_class isn't a subclass but rather the parent class itself
if not used_model_class:
used_model_class = model_class

if issubclass(used_model_class, ModelSimple):
instance = used_model_class(value=model_data, **kw_args)
Expand All @@ -609,6 +612,35 @@ def deserialize_model(model_data, model_class, path_to_item, check_type,
instance = used_model_class(**kw_args)
return instance

recursive_discriminators_cache = {}
Copy link
Contributor

@spacether spacether Jan 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

In a comment can you describe the structure of the dict that we are storing here?
What are the keys what are the values?


def recursive_discriminators(model_class):
"""
Return the discriminator mapping for all possible subtypes (recursively found down to the leaves)
:param model_class:
:return:
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you describe the structure of the dict that we are returning here?
What are the keys what are the values?

"""
if model_class in recursive_discriminators_cache:
return recursive_discriminators_cache[model_class]
Copy link
Contributor

Choose a reason for hiding this comment

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

How are we sure that this model class is valid?
We are not checking it with from_server and model_data.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm, not sure I understand. The only purpose of this method is to search through the discriminators and add them to the cache - not to return the model class itself. That is done in recursive_discriminator_lookup. Does that make more sense? This is essentially a recursive replacement for model_class.discriminator... I could put it into the model_class definition instead... maybe that would be cleaner? unclear.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I understand

recursive_discriminated_types = model_class.discriminator()
if not recursive_discriminated_types:
return {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Our old invocation of get_discriminator_class returns a class.
Shouldn't we return model_class in this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This isn't equivalent to get_discriminator_class for performance reasons. This instead is equivalent to model_class.discriminator.

Copy link
Contributor

@spacether spacether Jan 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Why do we need to return an empty dict if we will never key into it?
Shouldn't we avoid making that last recursive call?
When would this be hit?

for discriminator_key, discriminator_dict in model_class.discriminator().items():
for child_class in discriminator_dict.values():
for child_discriminator_key, child_discriminator_dict in recursive_discriminators(child_class).items():
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you use clearer names here?
Is child_discriminator_key the discriminator propertyName or a model_class_name?

if child_discriminator_key in recursive_discriminated_types:
Copy link
Contributor

@spacether spacether Jan 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Do we need to do a check to make sure that child_discriminator_key == discriminator_key?
If not then that implies that we have multiple discriminator keys.
How are we making sure that we are using the correct python or javascript variable name to model_data values? The usage in recursive_discriminator_lookup only does one python to js key name conversion.

recursive_discriminated_types[child_discriminator_key].update(child_discriminator_dict)
else:
recursive_discriminated_types[child_discriminator_key] = child_discriminator_dict
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a test of this function that shows what we store going down both forks of this if/else?

recursive_discriminators_cache[model_class] = recursive_discriminated_types
return recursive_discriminated_types

def recursive_discriminator_lookup(model_class, from_server, model_data):
Copy link
Contributor

@spacether spacether Jan 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Why aren't we recursively calling this?

new_model_class = model_class.get_discriminator_class(from_server, model_data)
if new_model_class.discriminator() is None:
   return new_model_class
return recursive_discriminator_lookup(new_model_class, from_server, model_data)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had done this before, but the issue was that we have a recursive lookup for a single type potentially many, many times. So instead, I put in a simple cache and changed the logic to 'lookup the recursive discriminator in the cache for the given model_class'. If it isn't there, I calculate the whole recursive discriminator for that type and I store it in the cache. Then any subsequent lookup is just a single dictionary lookup. I had the exact logic that you had just laid out and the deserialization process took 30 seconds+ for a complex model. The cache brought that down to ~3

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes sense. When we get this working, please remove the get_discriminator_class if we are no longer using it.

Copy link
Contributor

@spacether spacether Jan 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Can you add a clearer description of our use case here?
Something like

A model can return a child model using it's discriminator.
But if that new model also has a discriminator we have 
not yet reached our final model. Keep recursively 
calling until we get there.

discr_propertyname_py = list(model_class.discriminator().keys())[0]
try:
return recursive_discriminators(model_class)[discr_propertyname_py][model_data[model_class.attribute_map[discr_propertyname_py]]]
Copy link
Contributor

@spacether spacether Jan 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This one line is doing the following:

  • retrieving a value from and possible storing data in recursive_discriminators_cache
  • accessing the discriminator map
  • with a value from model data
  • using the js key
  • using the python key

Please separate this into multiple lines for readability and ease of understanding

except KeyError as e:
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not raise an exception?
What does our get_discriminator_class class method do in this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm refactoring this to be identical to get_discriminator_class. That should resolve a couple of these comments - I agree it is fast and dirty ATM

return

def deserialize_file(response_data, configuration, content_disposition=None):
"""Deserializes body to file
Expand Down
4 changes: 4 additions & 0 deletions samples/client/petstore/python-experimental/petstore_api/model_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -848,6 +848,10 @@ def deserialize_model(model_data, model_class, path_to_item, check_type,
used_model_class = model_class.get_discriminator_class(
from_server, model_data)

# This is the case if the used_model_class isn't a subclass but rather the parent class itself
if not used_model_class:
Copy link
Contributor

@spacether spacether Jan 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Our old code always returned a model class.
Why do we need to return {} or a falsey value now?
Can you remove the case where this is returning a false value and return the correct class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was actually the original error I was looking at before I started looking at the recursive discriminator issue. The method returns nothing when the discriminator specified is not in the discriminator mapping. I believe in that case, a reasonable solution is to return the base class itself, but perhaps we should instead return an error here?

Copy link
Contributor

@spacether spacether Jan 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Per the spec here: https://github.com/OAI/OpenAPI-Specification/blob/master/versions/2.0.md#composition-and-inheritance-polymorphism
If a discriminator is used, then the child class must contain the discriminator propertyName, and by inference the child class must be in the discriminator map.
If our map is missing the info the user can add it to the spec, or in some cases (allOf inclusion of a parent) our Java code populates the discriminator map.
My diff here: #4906
Improves that inferred mapping for composed schemas (v3 spec feature because of oneOf).
Please raise an exception that lets the user know what went wrong.

used_model_class = model_class

if issubclass(used_model_class, ModelSimple):
instance = used_model_class(value=model_data, **kw_args)
return instance
Expand Down