adds Recursive Discriminator support #4913

eak24 · 2020-01-02T17:34:21Z

For now this is just a draft to start discussion with @spacether.

Linked issue:
#4912

PR checklist

Read the contribution guidelines.
If contributing template-only or documentation-only changes which will change sample output, build the project before.
Run the shell script(s) under ./bin/ (or Windows batch scripts under.\bin\windows) to update Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit, and these must match the expectations made by your contribution. You only need to run ./bin/{LANG}-petstore.sh, ./bin/openapi3/{LANG}-petstore.sh if updating the code or mustache templates for a language ({LANG}) (e.g. php, ruby, python, etc).
File the PR against the correct branch: master, 4.3.x, 5.0.x. Default: master.
Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

spacether · 2020-01-02T17:54:12Z

@ethan92429 can you add a link in the description to #4912
If this is merged, that issue should be closed

spacether · 2020-01-02T17:58:26Z

modules/openapi-generator/src/main/resources/python/python-experimental/model_utils.mustache

+    :return:
+    """
+    if model_class in recursive_discriminators_cache:
+        return recursive_discriminators_cache[model_class]


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

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.

Ah, I understand

spacether · 2020-01-02T18:03:13Z

modules/openapi-generator/src/main/resources/python/python-experimental/model_utils.mustache

+        return recursive_discriminators_cache[model_class]
+    recursive_discriminated_types = model_class.discriminator()
+    if not recursive_discriminated_types:
+        return {}


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

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

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?

spacether · 2020-01-02T18:05:59Z

modules/openapi-generator/src/main/resources/python/python-experimental/model_utils.mustache

+    recursive_discriminators_cache[model_class] = recursive_discriminated_types
+    return recursive_discriminated_types
+
+def recursive_discriminator_lookup(model_class, from_server, model_data):


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)

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

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

spacether · 2020-01-02T18:09:01Z

samples/client/petstore/python-experimental/petstore_api/model_utils.py

@@ -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:


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?

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?

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.

spacether · 2020-01-02T18:09:47Z

modules/openapi-generator/src/main/resources/python/python-experimental/model_utils.mustache

+    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]]]
+    except KeyError as e:


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

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

spacether · 2020-01-02T18:19:35Z

@ethan92429 can you please add

a sample models in the python-experimental spec here https://github.com/OpenAPITools/openapi-generator/blob/master/modules/openapi-generator/src/test/resources/2_0/python-client-experimental/petstore-with-fake-endpoints-models-for-testing.yaml
- For clarity could you create new models that are not used by any other tests?
a test deserializing on of the base models into a grandchild model in https://github.com/OpenAPITools/openapi-generator/blob/master/samples/client/petstore/python-experimental/tests/test_deserialization.py

spacether · 2020-01-02T18:21:07Z

modules/openapi-generator/src/main/resources/python/python-experimental/model_utils.mustache

+    recursive_discriminators_cache[model_class] = recursive_discriminated_types
+    return recursive_discriminated_types
+
+def recursive_discriminator_lookup(model_class, from_server, model_data):


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.

spacether · 2020-01-02T18:36:40Z

modules/openapi-generator/src/main/resources/python/python-experimental/model_utils.mustache

+def recursive_discriminator_lookup(model_class, from_server, model_data):
+    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]]]


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

spacether · 2020-01-02T18:43:11Z

modules/openapi-generator/src/main/resources/python/python-experimental/model_utils.mustache

+                if child_discriminator_key in recursive_discriminated_types:
+                    recursive_discriminated_types[child_discriminator_key].update(child_discriminator_dict)
+                else:
+                    recursive_discriminated_types[child_discriminator_key] = child_discriminator_dict


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

spacether · 2020-01-02T18:47:00Z

modules/openapi-generator/src/main/resources/python/python-experimental/model_utils.mustache

+    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():
+                if child_discriminator_key in recursive_discriminated_types:


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.

spacether · 2020-01-02T18:52:20Z

modules/openapi-generator/src/main/resources/python/python-experimental/model_utils.mustache

+        return {}
+    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():


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

spacether · 2020-01-02T18:53:00Z

modules/openapi-generator/src/main/resources/python/python-experimental/model_utils.mustache

+    """
+    Return the discriminator mapping for all possible subtypes (recursively found down to the leaves)
+    :param model_class:
+    :return:


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

spacether · 2020-01-02T18:53:13Z

modules/openapi-generator/src/main/resources/python/python-experimental/model_utils.mustache

@@ -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 = {}


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

spacether · 2020-01-09T23:45:57Z

@ethan92429 are you okay?
Haven't seen anything here in the last week.

eak24 · 2020-01-10T00:39:16Z

Just busy with a couple other things and on vacation until Wednesday next week. Feel free to make the updates if you desire, and thanks for the review! I'll look more on Wednesday 😊

…

On Thu, Jan 9, 2020, 18:45 Justin Black ***@***.***> wrote: @ethan92429 <https://github.com/ethan92429> are you okay? Haven't seen anything here in the last week. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4913?email_source=notifications&email_token=ACBQBI53MYXO4VK4WQVEYATQ46ZLNA5CNFSM4KCEY2G2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEISF35Y#issuecomment-572808695>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACBQBI5AFAALIEGDJEYBDTTQ46ZLNANCNFSM4KCEY2GQ> .

spacether · 2020-01-10T01:13:04Z

Glad to hear it. There's no rush on this. Also you know the use case much better than I do. Hope that you have a fun and relaxing vacation!

spacether · 2020-01-29T07:08:28Z

@ethan92429 any update on your PR?

adds Recursive Discriminator support

0ea52ca

eak24 mentioned this pull request Jan 2, 2020

[Python] Multilevel inheritance discriminator support #4912

Closed

6 tasks

spacether reviewed Jan 2, 2020

View reviewed changes

Uh oh!

adds Recursive Discriminator support #4913

Are you sure you want to change the base?

adds Recursive Discriminator support #4913

Uh oh!

Conversation

eak24 commented Jan 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR checklist

Uh oh!

spacether commented Jan 2, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spacether Jan 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spacether Jan 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spacether Jan 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spacether Jan 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spacether commented Jan 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

spacether Jan 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spacether Jan 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spacether Jan 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spacether Jan 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spacether commented Jan 9, 2020

Uh oh!

eak24 commented Jan 10, 2020 via email

Uh oh!

spacether commented Jan 10, 2020

Uh oh!

spacether commented Jan 29, 2020

eak24 commented Jan 2, 2020 •

edited

Loading

spacether Jan 2, 2020 •

edited

Loading

spacether Jan 2, 2020 •

edited

Loading

spacether Jan 2, 2020 •

edited

Loading

spacether Jan 2, 2020 •

edited

Loading

spacether commented Jan 2, 2020 •

edited

Loading

spacether Jan 2, 2020 •

edited

Loading

spacether Jan 2, 2020 •

edited

Loading

spacether Jan 2, 2020 •

edited

Loading

spacether Jan 2, 2020 •

edited

Loading