bugfix: correct indentation of additionalProperties that are union types #266 #268

packyg · 2020-12-15T22:35:19Z

Fixes #266

Primary fix was to model.pyi, passing in a correct value to indent when looping through additional properties

Added ModelWithAnyJsonProperties model to openapi.json for e2e testing

With the new test model, encountered a couple typing issues and fixed them:

In to_dict for union properties it uses isinstance to inspect the type, but in the case of lists where it would check isinstance(prop, List[str])
- This has been fixed by adding a get_instance_type_string to Property and using that
- ListPropertys are the only props with a special case get_instance_type_string
In from_dict for unioned properties where one of the options is a primitive, it would just return the value to the helper func, which is provided as Any and thus would complain about the type
- Fixed by casting the primitive
- This causes a strange looking return for the helper function, as in model_with_any_json_properties.py, the but it should still work as the types from this helper func internal to the from_dict function
```
return cast(str, prop_dict)
return cast(float, prop_dict)
return cast(int, prop_dict)
return cast(bool, prop_dict)
```

check for list not List in isinstance cast primitives returned by union property's _parse_*

packyg · 2020-12-15T22:37:05Z

cc @dbanty

codecov · 2020-12-15T22:38:17Z

Codecov Report

Merging #268 (bfab9d4) into main (3ca219c) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main      #268   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           46        46           
  Lines         1314      1325   +11     
=========================================
+ Hits          1314      1325   +11

Impacted Files	Coverage Δ
...penapi_python_client/parser/properties/__init__.py	`100.00% <100.00%> (ø)`
...penapi_python_client/parser/properties/property.py	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3ca219c...bfab9d4. Read the comment docs.

dbanty

Thanks for fixing this so quickly! There are a couple oddities around the Unions which aren't caused by this but I noticed here that I think we can clean up.

If you don't have the bandwidth to look into that cleanup in the near term, we can always open a separate issue to clean up in a future version so we can get this bug fix out, just let me know.

dbanty · 2020-12-16T16:44:20Z

end_to_end_tests/golden-record-custom/custom_e2e/models/model_with_any_json_properties.py

+                return cast(str, prop_dict)
+                return cast(float, prop_dict)
+                return cast(int, prop_dict)
+                return cast(bool, prop_dict)


Hmm, this is ugly. I'll see if I can find the cause and propose a solution further down.

It would do this before, just without the casts. It's because in union_property.pyi we loop through each property and if it doesn't have a template we just return it.

What is here is ugly, yes, but it works. However, I do think there is an issue if we declare a templated type before a primitive type - then the primitive type would cause it to return early. Something , like

try: additional_property = cast(List[str], data) return additional_property except: # noqa: E722 pass return cast(str, prop_dict) try: additional_property = ModelWithAnyJsonPropertiesAdditionalProperty.from_dict(data) return additional_property except: # noqa: E722 pass return cast(float, data) return cast(int, data) return cast(bool, data)

So I think we need to loop over templated properties first, then untemplated properties. Also we'll want to figure out lists, as casting it to a List will not error so that breaks the try/catch logic.

While responding to this comment, I realized that these should be using cast(<primitive type>, data) not cast(<primitive type>, prop_dict) - I added commits to this branch to change that

I'll try looking at this soon as I have time

dbanty · 2020-12-16T16:48:00Z

end_to_end_tests/golden-record/my_test_api_client/models/model_with_any_json_properties.py

+            elif isinstance(prop, list):
+                field_dict[prop_name] = prop
+
+            elif isinstance(prop, str):
+                field_dict[prop_name] = prop
+            elif isinstance(prop, float):
+                field_dict[prop_name] = prop
+            elif isinstance(prop, int):
+                field_dict[prop_name] = prop
+            else:
+                field_dict[prop_name] = prop


Feels like maybe we need a way to indicate whether or not a given property actually needs to be transformed and if not, just have an else. Then something similar with construct for from_dict.

dbanty · 2020-12-16T16:54:54Z

openapi_python_client/templates/property_templates/union_property.pyi

    {% else %}
-    return {{ source }}
+    return cast({{ inner_property.get_type_string() }}, {{ source }})
    {% endif %}
    {% endfor %}


So if we aren't running a construct macro because the inner prop doesn't have a template, then we really aren't doing anything with it. So instead of having this in a loop, we could just do this after the loop:

return cast({{ property.get_type_string() }}, data)

So basically, loop through all the things that require some construction, if none of those work, just return what we have and assume it's one of the values. Technically it's one of the values we didn't check yet, but that doesn't really matter to the return type since the calling function doesn't know which union member was selected.

Also, it doesn't look like we actually need to use source anymore since the param is data right? So we could pull that out of the macro.

We do still need source because it's used in the outer function to call this inner one (just below on line 22, right before the endmacro)

dbanty · 2020-12-16T17:01:45Z

openapi_python_client/templates/property_templates/union_property.pyi

 {% for inner_property in property.inner_properties %}
    {% if loop.first and property.required and not property.nullable %}{# No if UNSET or if None statement before this #}
-if isinstance({{ source }}, {{ inner_property.get_type_string(no_optional=True) }}):
+if isinstance({{ source }}, {{ inner_property.get_instance_type_string() }}):
    {% elif not loop.last %}
-elif isinstance({{ source }}, {{ inner_property.get_type_string(no_optional=True) }}):
+elif isinstance({{ source }}, {{ inner_property.get_instance_type_string() }}):
    {% else %}
 else:
    {% endif %}


Similar story here I think, we just change the order of some checks. Something like

{% for inner_property in (inner_property for inner_property in property.inner_properties if inner_property.template) %} {% if loop.first and property.required and not property.nullable %}{# No if UNSET or if None statement before this #} if isinstance({{ source }}, {{ inner_property.get_instance_type_string() }}): {% else %} elif isinstance({{ source }}, {{ inner_property.get_instance_type_string() }}): {% from "property_templates/" + inner_property.template import transform %} {{ transform(inner_property, source, destination, declare_type=False) | indent(4) }} {% endif %} {% endfor %} else: {{ destination }} = {{ source }}

Editing in GH UI, so probably some mistakes made, but hopefully you get the idea.

…ive values

packyg · 2020-12-19T01:52:40Z

Ok, I've updated this by adding inner_properties_with_template and inner_properties_without_template computed properties to UnionProperty and keyed off of those in the template. (It made more sense to me to put it on the UnionProperty object rather than putting that logic in in the template)

I thought for a moment that I would need to handle ListPropertys separate, but then realized those do in fact need transforms when it's a list of models.

As an aside - I'm off until 1/4 and not doing any dev work while I'm off, so I won't be checking in to follow up here until then. Feel free to take this and/or expand on it without me.

dbanty

Looks good, thanks again. I had one note about how the properties without templates are being used, I'll check this out and fix it so we can get this merged in.

Enjoy your time off!

dbanty · 2020-12-20T17:10:47Z

openapi_python_client/templates/property_templates/union_property.pyi

-    {% for inner_property in property.inner_properties %}
-    {% if inner_property.template and not loop.last %}
+    {% for inner_property in property.inner_properties_with_template %}
+    {% if not loop.last or property.inner_properties_without_template %}


This will loop through all the properties for each property with templates to recompute inner_properties_without_template. I generally prefer leaving methods that do work as methods instead of using @property to make it more obvious that "this does something".

I'll check this out and make a tweak to avoid any O(n^2) behavior which, while probably insignificant in most cases, I'd still like to avoid if possible.

- Added CHANGELOG entry - Change inner_properties_without_template to bool generated once - Change inner_properties_with_template to generator

dbanty · 2020-12-20T18:18:20Z

Closing in favor of #273 which is based on this but has a couple tweaks:

UnionProperty.inner_properties_without_template is a precalculated bool to avoid extra work
UnionProperty.inner_properties_with_template is a generator so if/any can be used instead of regenerating the whole array.
Added cast to UnionProperty. get_imports because for some reason this wasn't already being included in some places it was use (e2e regen failed).

packyg added 5 commits December 15, 2020 10:23

Fix indentation of union-ed addtionalProperties

ca2a18b

Typing fixes

ab6622d

check for list not List in isinstance cast primitives returned by union property's _parse_*

Add ModelWithAnyJsonProperties to e2e tests

45d1801

Regen golden record

85fc685

Add return type for get_instance_type_string

bd59aa6

dbanty requested changes Dec 16, 2020

View reviewed changes

packyg added 4 commits December 16, 2020 11:09

Union Property - use data from inner function when returning primit…

c5f21b5

…ive values

Update golden record

0521e36

Update union properties handling of props with and without template

9a679af

Update golden record

bfab9d4

packyg requested a review from dbanty December 19, 2020 01:55

dbanty reviewed Dec 20, 2020

View reviewed changes

dbanty added a commit that referenced this pull request Dec 20, 2020

Perf improvements to #268 (additionalProperties Union Fix):

a365384

- Added CHANGELOG entry - Change inner_properties_without_template to bool generated once - Change inner_properties_with_template to generator

dbanty mentioned this pull request Dec 20, 2020

Fix additionalProperty Union spacing and construction (#266) #273

Merged

dbanty closed this Dec 20, 2020

packyg deleted the issue-266 branch January 4, 2021 17:27

packyg mentioned this pull request Jan 11, 2021

feat: Change from_dict from staticmethod to classmethod BNCH-17564 benchling/openapi-python-client#23

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bugfix: correct indentation of additionalProperties that are union types #266 #268

bugfix: correct indentation of additionalProperties that are union types #266 #268

packyg commented Dec 15, 2020

packyg commented Dec 15, 2020

codecov bot commented Dec 15, 2020 •

edited

Loading

dbanty left a comment

dbanty Dec 16, 2020

packyg Dec 16, 2020

packyg Dec 16, 2020

dbanty Dec 16, 2020

dbanty Dec 16, 2020

packyg Dec 18, 2020

dbanty Dec 16, 2020

packyg commented Dec 19, 2020 •

edited

Loading

dbanty left a comment

dbanty Dec 20, 2020

dbanty commented Dec 20, 2020

bugfix: correct indentation of additionalProperties that are union types #266 #268

bugfix: correct indentation of additionalProperties that are union types #266 #268

Conversation

packyg commented Dec 15, 2020

packyg commented Dec 15, 2020

codecov bot commented Dec 15, 2020 • edited Loading

Codecov Report

dbanty left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

packyg commented Dec 19, 2020 • edited Loading

dbanty left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbanty commented Dec 20, 2020

codecov bot commented Dec 15, 2020 •

edited

Loading

packyg commented Dec 19, 2020 •

edited

Loading