fix: handle deepcopy of openapi objects #9735

mfmarche · 2021-06-09T15:31:51Z

addresses issue [BUG][python] deepcopy of model gives KeyError: '_data_store' #9669
Add deepcopy and copy to OpenApiModel
pass discriminator inside deepcopy if exists
add test cases for deepcopy of models

PR checklist

Read the contribution guidelines.
Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
Run the following to build the project and update samples:
```
./mvnw clean package 
./bin/generate-samples.sh
./bin/utils/export_docs_generators.sh
```
Commit all changed files.
This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
These must match the expectations made by your contribution.
You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*.
For Windows users, please run the script in Git BASH.
File the PR against the correct branch: master, 5.1.x, 6.0.x
If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

@taxpon (2017/07) @frol (2017/07) @mbohlool (2017/07) @cbornet (2017/09) @kenjones-cisco (2017/11) @tomplus (2018/10) @Jyhess (2019/01) @arun-nalla (2019/11) @spacether (2019/11)

merges code from upstream PR: OpenAPITools#9735

spacether · 2021-06-24T05:13:21Z

Closing and re-opening for travis

...nerator/src/main/resources/python/model_templates/method_from_openapi_data_composed.mustache

spacether · 2021-06-24T16:56:10Z

modules/openapi-generator/src/main/resources/python/model_templates/methods_shared.mustache

+    def __copy__(self):
+        cls = self.__class__
+        result = cls.__new__(cls)


Is this copying a payload instantiated from client data cls(**kwargs) option1
Or one received from the server cls._from_openapi_data(**kwargs) option2?
If so should we not use __new__ and use one of the above options?

that seems reasonble, ideally just _from_openapidata should be used, since we are copying the "data", and don't want the type checking (ie. checking for read-only would not be required).

So how about changing this to use _from_openapi_data rather than __new__?

agree, i will make that change.

FYI: one can use the self._spec_property_naming to determine which code path to use when copying a self instance.

spacether · 2021-06-25T16:14:58Z

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

@@ -224,7 +225,8 @@ class OpenApiModel(object):
            self_inst.__init__(*args, **kwargs)

        new_inst = new_cls.__new__(new_cls, *args, **kwargs)
-        new_inst.__init__(*args, **kwargs)
+        # use _from_openapi_data to handle readOnly discriminator
+        new_inst = new_inst._from_openapi_data(*args, **kwargs)


Once we get rid of the new invocation in the copy method can this code be reverted?
If not then do we need to be two code paths here?

For from_openapi_data == True, invoke _from_openapi_data

For from_openapi_data == False invoke new then init
This same comment applies to the get_allof_instances update also

its the latter, its still needed (the openapi_data check). Agree, it would be more accurate to selectively call the correct init call. I will make that change.

Since this is inside a new, trying to figure out if this is called from init or from_openapi_data is tricky since new only has a single arg (the class name).

It seems the mechansim of validating the read-only vars at construction time (via init vs. from_openapi_data) leaves this code in a bit of a bind.

Can we use kwargs['_spec_property_naming'] to decide which code path to use?
I think that

_spec_property_naming = True is for the from_server use case / which is the from_openapi_data use case

_spec_property_naming = False is the client side instantiation use case

Our code sets it to True here:
https://github.com/OpenAPITools/openapi-generator/blob/master/modules/openapi-generator/src/main/resources/python/api_client.mustache#L340
when values are received from the server

Also we pas through _spec_propertty_naming in constant_args so can you revert your updates that add from_openapi_data as an argument and instead use _spec_propertty_naming?

my concern is, this spec_property_naming is set via the deserialize call.
If I instantiate an object with _from_openapi_data(), and then copy that, I think that means now that I have to manually set this parameter (_spec_property_naming).

Hmm that's true. Should all _from_openapi_data methods hard code _spec_property_naming = True?

possibly, though I'm not sure. My understanding is, _from_openapi_data is similiar to init, so vars are "pythonic". So that means it shouldn't be _spec_property_naming. I believe this tells me that we can't use_spec_property_naming for the purpose that I would like.

I don't quite appreciate the purpose of the _from vs. init. Couldn't the same thing be achieved and keep a single constructor initializer and just an option for checking read-only? I guess that is another discussion separate from this one.

_from_openapi data is used to instantiate models with data received from the server. When making model instances with readonly parameters the fn signature needed to make a model instances must contain different parameters if the data came from the server vs from the client. You can read more about it here: #9296 (comment). That conversation is separate from this one.

_from_openapi_data is used when instantiating models from the server so I'm pretty sure that we could depend on _spec_property_naming being true in it. We could throw an ApiValueError if _spec_property_naming is the arguments in _from_openapi_data.

spacether · 2021-08-18T19:05:26Z

@mfmarche this PR just landed:
#10155
Which uses cls._from_openapi_data() or cls() when making composed instances.
Are you still interested in merging this feature?
How about rebasing on master, your update should be simpler now and should depend upon using _spec_property_naming like that PR did.

mfmarche · 2021-09-03T19:54:35Z

Hi @spacether PR updated and rebased with changes as per review comments. Thanks.

spacether · 2021-09-05T19:56:04Z

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

@@ -186,6 +187,24 @@ def __getattr__(self, attr):
        """get the value of an attribute using dot notation: `instance.attr`"""
        return self.__getitem__(attr)

+    def __copy__(self):
+        cls = self.__class__
+        return cls._new_from_openapi_data(**self.__dict__)


Why is copy assuming a from server context? Why not use self._spec_property_naming to pick if we should use __new__ or _new_from_openapi_data?

This same question applies to __deepcopy__, why is it assuming a from server context when setting _spec_property_naming to True?
What if the deepcopy instance was made client side, not server side, then this code incorrectly forces _spec_property_naming to True when it should be False.

If I have an object, I would assume that should be able to copy it as is, without consideration for read-only args/etc. Now I can't think of a case where that would occur, but I'm not sure I have a grasp of all the ways objects can be created at moment. That was my reasoning, but if you still feel the logic you have described should be used, I will update to that. thanks.

You will be able to copy an instance of the model that is made using client side data, and you can do the same with an instance of the model made with data from the server, but those contexts should allow slightly different data to be stored in the instances. Changing _spec_property_naming changes where the model says its data came from, which we should not be doing.
Please make the suggested changes, thanks.

I have updated based on your recommendation.

A question regarding the the petstore_api tests, how exactly are those run? I typically run those manually, but I don't see a CI job that actually runs those?

They were run by travis, I need to switch them over to CircleCi because we hit a free limit usage on Travis.
Merging this is blocked by that fix.
Thank you for making the changes.

.../client/petstore/python_disallowAdditionalPropertiesIfNotPresent/petstore_api/model_utils.py

spacether · 2021-09-07T16:48:30Z

.../client/petstore/python_disallowAdditionalPropertiesIfNotPresent/petstore_api/model_utils.py

+            new_inst = cls.__new__(cls, **kwargs)
+
+        for k, v in self.__dict__.items():
+            setattr(new_inst, k, deepcopy(v, memo))


How about grabbing all of these properties before passing them into
_new_from_openapi_data and __new__?

kwargs = {} for k, v in self.__dict__.items(): kwargs[key] = deepcopy(v, memo) if self.get("_spec_property_naming", False): new_inst = cls._new_from_openapi_data(**kwargs) else: new_inst = cls.__new__(cls, **kwargs)

The reason setattr is used rather than kwargs, is that the dictionary set of values (such as the properties), are stored in dict["_data_store"]. In order to pass as kwargs, I would need to interpret the data (handle the _data_store as a special case), and pass that in as the kwargs. Rather, using setinst sets the key/value as is without any interpretation, a more resilent solution IMO.

modules/openapi-generator/src/main/resources/python/model_templates/methods_shared.mustache

spacether · 2021-09-09T05:20:24Z

@mfmarche can you rebase this on master?
#10354
just landed which turns python testing back on in circleci.

- Add __deepcopy__ and __copy__ to OpenApiModel - pass discriminator inside deepcopy if exists - add test cases for deepcopy of models

spacether

Thanks for rebasing; this looks great!

mfmarche added a commit to mfmarche/openapi-generator that referenced this pull request Jun 9, 2021

fix: merge fix for OpenAPITools#9669 (deepcopy)

7c2375d

merges code from upstream PR: OpenAPITools#9735

wing328 added Client: Python Enhancement: CI/Test Enhancement: General Issue: Bug labels Jun 10, 2021

spacether closed this Jun 24, 2021

spacether reopened this Jun 24, 2021

spacether added this to the 5.2.0 milestone Jun 24, 2021

spacether reviewed Jun 24, 2021

View reviewed changes

...nerator/src/main/resources/python/model_templates/method_from_openapi_data_composed.mustache Outdated Show resolved Hide resolved

spacether reviewed Jun 24, 2021

View reviewed changes

spacether reviewed Jun 25, 2021

View reviewed changes

wing328 modified the milestones: 5.2.0, 5.2.1 Jul 13, 2021

spacether mentioned this pull request Aug 16, 2021

[python] Fixes a breakage while deserializing the read-only attributes #10155

Merged

5 tasks

wing328 modified the milestones: 5.2.1, 5.3.0 Aug 17, 2021

mfmarche mentioned this pull request Aug 19, 2021

[BUG][Python] readOnly property inside of allOf causes "The input data was invalid for the allOf schema" #9596

Closed

6 tasks

mfmarche force-pushed the fix_9669 branch from d982821 to 14dfa65 Compare September 3, 2021 15:29

spacether reviewed Sep 5, 2021

View reviewed changes

mfmarche force-pushed the fix_9669 branch from 14dfa65 to 9d12585 Compare September 6, 2021 13:52

spacether reviewed Sep 7, 2021

View reviewed changes

.../client/petstore/python_disallowAdditionalPropertiesIfNotPresent/petstore_api/model_utils.py Outdated Show resolved Hide resolved

spacether reviewed Sep 7, 2021

View reviewed changes

modules/openapi-generator/src/main/resources/python/model_templates/methods_shared.mustache Show resolved Hide resolved

spacether reviewed Sep 7, 2021

View reviewed changes

modules/openapi-generator/src/main/resources/python/model_templates/methods_shared.mustache Outdated Show resolved Hide resolved

fix: handle deepcopy of openapi objects (OpenAPITools#9735)

5be8dbf

- Add __deepcopy__ and __copy__ to OpenApiModel - pass discriminator inside deepcopy if exists - add test cases for deepcopy of models

mfmarche force-pushed the fix_9669 branch from 9d12585 to 5be8dbf Compare September 9, 2021 12:43

spacether approved these changes Sep 9, 2021

View reviewed changes

spacether merged commit 9464999 into OpenAPITools:master Sep 9, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: handle deepcopy of openapi objects #9735

fix: handle deepcopy of openapi objects #9735

mfmarche commented Jun 9, 2021 •

edited

Loading

spacether commented Jun 24, 2021

spacether Jun 24, 2021 •

edited

Loading

mfmarche Jun 24, 2021

spacether Jun 25, 2021 •

edited

Loading

mfmarche Jun 25, 2021

spacether Jul 9, 2021

spacether Jun 25, 2021 •

edited

Loading

mfmarche Jun 25, 2021

mfmarche Jul 7, 2021

mfmarche Jul 7, 2021

spacether Jul 8, 2021

spacether Jul 9, 2021 •

edited

Loading

mfmarche Jul 9, 2021

spacether Jul 9, 2021 •

edited

Loading

mfmarche Jul 9, 2021

spacether Jul 13, 2021 •

edited

Loading

spacether commented Aug 18, 2021

mfmarche commented Sep 3, 2021

spacether Sep 5, 2021 •

edited

Loading

mfmarche Sep 5, 2021

spacether Sep 5, 2021 •

edited

Loading

mfmarche Sep 6, 2021

spacether Sep 7, 2021

spacether Sep 7, 2021 •

edited

Loading

mfmarche Sep 8, 2021

spacether commented Sep 9, 2021

spacether left a comment

fix: handle deepcopy of openapi objects #9735

fix: handle deepcopy of openapi objects #9735

Conversation

mfmarche commented Jun 9, 2021 • edited Loading

PR checklist

spacether commented Jun 24, 2021

spacether Jun 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spacether Jun 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spacether Jun 25, 2021 • 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

Choose a reason for hiding this comment

spacether Jul 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spacether Jul 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spacether Jul 13, 2021 • edited Loading

Choose a reason for hiding this comment

spacether commented Aug 18, 2021

mfmarche commented Sep 3, 2021

spacether Sep 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spacether Sep 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spacether Sep 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spacether commented Sep 9, 2021

spacether left a comment

Choose a reason for hiding this comment

mfmarche commented Jun 9, 2021 •

edited

Loading

spacether Jun 24, 2021 •

edited

Loading

spacether Jun 25, 2021 •

edited

Loading

spacether Jun 25, 2021 •

edited

Loading

spacether Jul 9, 2021 •

edited

Loading

spacether Jul 9, 2021 •

edited

Loading

spacether Jul 13, 2021 •

edited

Loading

spacether Sep 5, 2021 •

edited

Loading

spacether Sep 5, 2021 •

edited

Loading

spacether Sep 7, 2021 •

edited

Loading