Add additional property support by subscripting model #252

packyg · 2020-11-25T21:08:10Z

Implements #218

Add support for additionalProperties by holding extra props in a private dict.
Can be accessed with pass-through subscripting dunder methods, including a @property to get the keys (see the bottom of model.pyi)

Updated from_dict to pop all named properties (from a shallow copy), with the rest going in the catch-all
- Required saving the value to a temp variable for inspection
to_dict sets the additional properties first, giving precedent to named props

This would be a big win for my team - we had been relying on the previous behavior of having dicts when type=object to handle cases that we haven't or can't model very well.

packyg · 2020-11-25T21:09:27Z

@dbanty I'm interested to see if you like this approach. If you do, I'll go ahead and update the tests and add new test cases

(cc @bowenwr @dtkav)

dbanty

Overall I think it looks good. I'm a little uneasy about putting set/getitem directly on the model but I don't have a good reason for being uneasy so it's probably fine 😅. I pointed out a couple places I think we could clean up some of the dict changes. Other than that, I would like @emannguitar to take a look at this as well to get his opinion.

dbanty · 2020-11-28T23:19:18Z

openapi_python_client/templates/model.pyi

            {% for property in model.required_properties + model.optional_properties %}
            {% if property.required %}
            "{{ property.name }}": {{ property.python_name }},
            {% endif %}
            {% endfor %}
-        }
+        })


Why not leave the regular properties in the field dict declaration? Would be a bit faster I think rather than having to update after the fact, even without having additional properties.

My thinking here was that if a user for some reason set the same property both ways, the modeled version of the property takes precedence

model.some_json_property = "val1" model["someJsonProperty"] = "val2" model.to_dict() # {"someJsonProperty": "val1"}

dbanty · 2020-11-28T23:33:54Z

openapi_python_client/templates/model.pyi

 {% for property in model.required_properties + model.optional_properties %}
            {{ property.python_name }}={{ property.python_name }},
 {% endfor %}
        )
+
+        {% if model.additional_properties %}
+        {{model.reference.module_name}}._additional_properties.update(d)


Why update vs set directly? I would think assignment would be faster but maybe not.

For some reason I had it in my head that attr wouldn't like this. But these models aren't frozen so I'll make that change.

dbanty · 2020-11-28T23:43:06Z

openapi_python_client/templates/model.pyi

+
+    @property
+    def additional_properties(self) -> Dict[str, {{ additional_property_type }}]:
+        return self._additional_properties


What's the benefit of having this internal w/ @property vs just declaring it as additional_properties originally?

There isn't one really. I had thought I might need to add some special handling here but ended up not needing it.

I've gone ahead and just made additional_properties public.

emann

LGTM outside of @dbanty's comments

emann · 2020-11-30T16:56:38Z

Actually two small things:

Per the OpenAPI spec, type: object by itself (i.e. without specifying additionalProperties: true or additionalProperties: {}) is also a free-form object
Given that a free-form object is really just an arbitrary dict at the end of the day, they should just be dicts instead of a model with nothing but _aditional_properties

packyg

Thanks for the feedback @dbanty and @emannguitar.

I realized I also didn't properly handle serialization/deserialization when the schema of the additionalProperties is specified and the dict holds models. I will take a crack at that.

Per the OpenAPI spec, type: object by itself (i.e. without specifying additionalProperties: true or additionalProperties: {}) is also a free-form object

This wasn't immediately clear to me. I know this is the case for JSON schema, wasn't sure it was for OpenAPI, but it appears so: "Consistent with JSON Schema, additionalProperties defaults to true."
I will revisit to account for this.

Given that a free-form object is really just an arbitrary dict at the end of the day, they should just be dicts instead of a model with nothing but _aditional_properties

This is true and was my initial thought for objects with additionalProperties and no named properties, but it gets more complicated when we get to the case I mentioned at the top of this comment and it's a Dict[str, SomeModelClass]

packyg · 2020-11-30T17:40:11Z

openapi_python_client/templates/model.pyi

            {% for property in model.required_properties + model.optional_properties %}
            {% if property.required %}
            "{{ property.name }}": {{ property.python_name }},
            {% endif %}
            {% endfor %}
-        }
+        })


My thinking here was that if a user for some reason set the same property both ways, the modeled version of the property takes precedence

model.some_json_property = "val1" model["someJsonProperty"] = "val2" model.to_dict() # {"someJsonProperty": "val1"}

packyg · 2020-11-30T17:45:26Z

openapi_python_client/templates/model.pyi

 {% for property in model.required_properties + model.optional_properties %}
            {{ property.python_name }}={{ property.python_name }},
 {% endfor %}
        )
+
+        {% if model.additional_properties %}
+        {{model.reference.module_name}}._additional_properties.update(d)


For some reason I had it in my head that attr wouldn't like this. But these models aren't frozen so I'll make that change.

packyg · 2020-11-30T17:54:20Z

openapi_python_client/templates/model.pyi

+
+    @property
+    def additional_properties(self) -> Dict[str, {{ additional_property_type }}]:
+        return self._additional_properties


There isn't one really. I had thought I might need to add some special handling here but ended up not needing it.

I've gone ahead and just made additional_properties public.

…g models, test edge case parsing

packyg · 2020-12-03T03:07:55Z

OK, I've now updated this to properly interpret additionalProperties per the spec (defaulting to true)

I also added support for additionalProperties where the elements are modeled. This requires calling to_dict/from_dict for serialization/deserialization, so we can't just use a raw dict there.

I'll start working on tests soon, but again wanted to get this out there.

packyg · 2020-12-03T03:10:14Z

openapi_python_client/templates/property_templates/model_property.pyi

+{% if initial_value != None %}
 {{ property.python_name }} = {{ initial_value }}
-if {{ source }} is not None:
-    {{ property.python_name }} = {{ property.reference.class_name }}.from_dict(cast(Dict[str, Any], {{ source }}))
+{% elif property.nullable %}
+{{ property.python_name }} = None
+{% else %}
+{{ property.python_name }} = UNSET
+{% endif %}


This addresses what I think was a bug before

(Below has to do with the fact we are now poping the dict values)

packyg · 2020-12-03T03:10:56Z

openapi_python_client/templates/property_templates/model_property.pyi

@@ -16,7 +23,7 @@ if {{ source }} is not None:
 {{ destination }} = {{ source }}.to_dict()
 {% endif %}
 {% else %}
-{{ destination }}{% if declare_type %}: {{ property.get_type_string() }}{% endif %} = UNSET
+{{ destination }}{% if declare_type %}: Union[{% if property.nullable %}None, {% endif %}Unset, Dict[str, Any]]{% endif %} = UNSET


Same here - the type was wrong here

…erties values

…is singular

codecov · 2020-12-04T00:17:52Z

Codecov Report

Merging #252 (6ccdde4) into main (4556d9a) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main      #252   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           46        46           
  Lines         1302      1314   +12     
=========================================
+ Hits          1302      1314   +12

Impacted Files	Coverage Δ
...penapi_python_client/parser/properties/__init__.py	`100.00% <100.00%> (ø)`
..._python_client/parser/properties/model_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 4556d9a...6ccdde4. Read the comment docs.

emann

LGTM!

@dbanty I generated a client for inventory to test and this seems to fix the issues we were having

dbanty

Awesome, fantastic work! I'll put together a release including this feature shortly.

packyg added 3 commits November 25, 2020 12:58

Add additional property support by subscripting model

a96621d

Update e2e golden records

f8ca735

Add models with additionalProperties to golden record

62fad1e

dbanty linked an issue Nov 28, 2020 that may be closed by this pull request

Support for additionalProperties and "free form" objects #218

Closed

dbanty requested changes Nov 28, 2020

View reviewed changes

dbanty requested a review from emann November 28, 2020 23:47

emann reviewed Nov 30, 2020

View reviewed changes

packyg added 2 commits November 30, 2020 09:44

PR Feedback: Set additional properties directly

f6028d3

PR Feedback: Just make additionalProperties public

4c0fbd0

packyg commented Nov 30, 2020

View reviewed changes

packyg added 7 commits November 30, 2020 13:59

Handle edge cases of additionalProperties to interpret per the spec

b13d282

Update golden record's spec to specify no additional props on existin…

7b72c9a

…g models, test edge case parsing

Support modeled additionalProperties

3a827f1

Typing fixes for model properties

78015fc

Add ModelWithPrimitiveAdditionalProperties to e2e test spec

e3e0fb4

Regen golden records

45ce82b

Merge branch 'main' into additionalProperties-support

b2285cd

packyg commented Dec 3, 2020

View reviewed changes

packyg requested review from dbanty and emann December 3, 2020 03:11

packyg added 6 commits December 2, 2020 19:19

Update templates to not attempt to pop the same attr multiple times

0d175c0

Update golden record

1f38a05

Typing fixes for additional_properties when building model prop

da0bc8e

Remove unused import from model_property

827d367

Update existing tests to account for additional_properties

daf9628

Parametrize test_build_model_property to test various additional_prop…

036f372

…erties values

Rename AdditionalProperties models to AdditionalProperty as each one …

e1ef9a7

…is singular

packyg added 2 commits December 3, 2020 16:28

Add test for bad additionalProperties schema

0cbe2de

uncomment part of test

6ccdde4

packyg marked this pull request as ready for review December 4, 2020 00:30

This was referenced Dec 4, 2020

feat: Add setup.py support BNCH-18722 benchling/openapi-python-client#11

Merged

feat: Add allOf support for model definitions BNCH-18722 benchling/openapi-python-client#12

Merged

emann approved these changes Dec 8, 2020

View reviewed changes

dbanty approved these changes Dec 8, 2020

View reviewed changes

dbanty merged commit 3df7be3 into openapi-generators:main Dec 8, 2020

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

Update CHANGELOG from #218, #252, and #259

c6dbb36

packyg mentioned this pull request Dec 9, 2020

Fix bug in model property template that was not handling unset properly #262

Merged

joshzana mentioned this pull request Dec 15, 2020

Invalid python generated when using nested dictionaries #266

Closed

eli-bl deleted the additionalProperties-support branch November 26, 2024 22:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add additional property support by subscripting model #252

Add additional property support by subscripting model #252

packyg commented Nov 25, 2020 •

edited

Loading

packyg commented Nov 25, 2020 •

edited

Loading

dbanty left a comment

dbanty Nov 28, 2020

packyg Nov 30, 2020

dbanty Nov 28, 2020

packyg Nov 30, 2020

dbanty Nov 28, 2020

packyg Nov 30, 2020

emann left a comment

emann commented Nov 30, 2020

packyg left a comment •

edited

Loading

packyg Nov 30, 2020

packyg Nov 30, 2020

packyg Nov 30, 2020

packyg commented Dec 3, 2020 •

edited

Loading

packyg Dec 3, 2020 •

edited

Loading

packyg Dec 3, 2020

codecov bot commented Dec 4, 2020 •

edited

Loading

emann left a comment

dbanty left a comment

Add additional property support by subscripting model #252

Add additional property support by subscripting model #252

Conversation

packyg commented Nov 25, 2020 • edited Loading

packyg commented Nov 25, 2020 • edited Loading

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

emann left a comment

Choose a reason for hiding this comment

emann commented Nov 30, 2020

packyg left a comment • edited Loading

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 3, 2020 • edited Loading

packyg Dec 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Dec 4, 2020 • edited Loading

Codecov Report

emann left a comment

Choose a reason for hiding this comment

dbanty left a comment

Choose a reason for hiding this comment

packyg commented Nov 25, 2020 •

edited

Loading

packyg commented Nov 25, 2020 •

edited

Loading

packyg left a comment •

edited

Loading

packyg commented Dec 3, 2020 •

edited

Loading

packyg Dec 3, 2020 •

edited

Loading

codecov bot commented Dec 4, 2020 •

edited

Loading