Don't mutate default values for properties

Don't mutate default values for properties

[guido, 2012-12-05 17:46:35]

https://codereview.appspot.com/6870063/diff/1/ndb/model.py File ndb/model.py (right): https://codereview.appspot.com/6870063/diff/1/ndb/model.py#newcode1018 ndb/model.py:1018: if result is default and default is not None: It would probably be better to rewrite this as result = entity._values.get(self._name) if result is None and default is not None: <something> https://codereview.appspot.com/6870063/diff/1/ndb/model.py#newcode1019 ndb/model.py:1019: if isinstance(result, Model): I think it would be better to put the extra code in the specific model classes -- isinstance() has a strong code smell. https://codereview.appspot.com/6870063/diff/1/ndb/model.py#newcode1022 ndb/model.py:1022: result = copy.deepcopy(default) If there's any way to avoid deepcopy I'd appreciate it; deepcopy always causes surprises (usually by copying too much). https://codereview.appspot.com/6870063/diff/1/ndb/model_test.py File ndb/model_test.py (right): https://codereview.appspot.com/6870063/diff/1/ndb/model_test.py#newcode4338 ndb/model_test.py:4338: def testMutableDefaultValuesForGeoPtProperties(self): The GeoPt type is supposed to be immutable so it should *not* need to be copied. (Of course deepcopy() doesn't know that. :-/ )

[Cd-MaN, 2012-12-05 17:50:48]

https://codereview.appspot.com/6870063/diff/1/ndb/model.py File ndb/model.py (right): https://codereview.appspot.com/6870063/diff/1/ndb/model.py#newcode1019 ndb/model.py:1019: if isinstance(result, Model): Would it be better if model implemented __copy__ and I would use copy.copy() for everything? (eliminating the deepcopy below?) https://codereview.appspot.com/6870063/diff/1/ndb/model_test.py File ndb/model_test.py (right): https://codereview.appspot.com/6870063/diff/1/ndb/model_test.py#newcode4338 ndb/model_test.py:4338: def testMutableDefaultValuesForGeoPtProperties(self): So is it a bug the fact that I can say "entity2.container.lat = 2.0"? If so, should I submit a patch to resolve it? Should it be in this issue or should I create a separate issue? On 2012/12/05 17:46:35, guido wrote: > The GeoPt type is supposed to be immutable so it should *not* need to be copied. > (Of course deepcopy() doesn't know that. :-/ )

[guido, 2012-12-05 18:02:33]

https://codereview.appspot.com/6870063/diff/1/ndb/model.py File ndb/model.py (right): https://codereview.appspot.com/6870063/diff/1/ndb/model.py#newcode1019 ndb/model.py:1019: if isinstance(result, Model): On 2012/12/05 17:50:48, Cd-MaN wrote: > Would it be better if model implemented __copy__ and I would use copy.copy() for > everything? (eliminating the deepcopy below?) Hm, for JSON (and probably for pickled objects) you probably need deepcopy. My hunch is that we probably shouldn't do anything -- you are already breaking backward compatibility, strictly speaking, by making a copy of a model given as a default. Finally, I think that Models already are copied correctly, because the copy and deepcopy functions look for the pickling API, and Model defines __getstate__. https://codereview.appspot.com/6870063/diff/1/ndb/model_test.py File ndb/model_test.py (right): https://codereview.appspot.com/6870063/diff/1/ndb/model_test.py#newcode4338 ndb/model_test.py:4338: def testMutableDefaultValuesForGeoPtProperties(self): On 2012/12/05 17:50:48, Cd-MaN wrote: > So is it a bug the fact that I can say "entity2.container.lat = 2.0"? If so, > should I submit a patch to resolve it? Should it be in this issue or should I > create a separate issue? Yes, that's a bug, but it will be hard to fix -- the GeoPt type is shared with the old db package. > On 2012/12/05 17:46:35, guido wrote: > > The GeoPt type is supposed to be immutable so it should *not* need to be > copied. > > (Of course deepcopy() doesn't know that. :-/ ) >

[Cd-MaN, 2012-12-06 22:44:34]

Hello, I uploaded a second patchset with the following changes: - it only targets *structed properties - it uses overriding of _retrieve_value in _StructuredGetForDictMixin as you suggested in your initial reply - I removed the irrelevant tests, including the one for GeoPt What do you think? Thanks, Attila

[guido, 2012-12-06 22:51:55]

Looks better, still one suggestion. Now I have to think about this more and decide whether this is sufficiently backwards compatible to commit.

[guido, 2012-12-06 22:52:07]

https://codereview.appspot.com/6870063/diff/7001/ndb/model.py File ndb/model.py (right): https://codereview.appspot.com/6870063/diff/7001/ndb/model.py#newcode2040 ndb/model.py:2040: result = type(default)._from_pb(default._to_pb()) I think you need to do this, in case the default is bogus: if isinstance(default, self._modelclass): result = self._modelclass._from_pb(default._to_pb())

[Cd-MaN, 2012-12-06 23:26:06]

Updated the patchset with the suggested verification. However, wouldn't such verification be better suited to __init__ or other such method which is called early in the object lifecycle? Also, this code doesn't warn users about the error, rather it just swallows it. Wouldn't it be better to raise an exception or something similar?

[guido, 2012-12-06 23:36:01]

You're right, but that definitely requires more unittests (of the constructor). https://codereview.appspot.com/6870063/diff/13001/ndb/model.py File ndb/model.py (right): https://codereview.appspot.com/6870063/diff/13001/ndb/model.py#newcode2041 ndb/model.py:2041: result = type(default)._from_pb(default._to_pb()) Instead of type(default), use self._modelclass here too.

[guido, 2012-12-07 01:44:11]

FWIW, I no longer have a problem with fixing this. It's a bug. So let's just get the details right...

[Cd-MaN, 2012-12-07 19:35:19]

Hello, I uploaded a new patchset with two changes: - I removed the verification for the type of the default. Will submit a patchset with the verification included separately in a couple of minutes (seems to me that they are somewhat distinct issues) - Left this line as is: type(default)._from_pb(default._to_pb()) And added a unittest which would fail if it was changed to self._modelclass._from_pb(default._to_pb()) (the gist of it is that it seems to me that having type(default) be a subclass of _modelclass is a valid usecase).

[Cd-MaN, 2012-12-07 19:38:54]

The part with type checking for the default values of (Local)StructuredProperty: http://codereview.appspot.com/6903051/

[GvR, 2012-12-07 20:13:11]

https://codereview.appspot.com/6870063/diff/17001/ndb/model_test.py File ndb/model_test.py (right): https://codereview.appspot.com/6870063/diff/17001/ndb/model_test.py#newcode4505 ndb/model_test.py:4505: self.assertEqual(2, entity.container.other_value) I see. This is why you use type(default) instead of self._modelclass. I'm not sure that this is a completely valid use case; have you tried to write such an entity and read it back? It won't come back as a ValueSubclass. What is your reason for wanting to support this use case?

[Cd-MaN, 2012-12-13 19:09:58]

https://codereview.appspot.com/6870063/diff/17001/ndb/model_test.py File ndb/model_test.py (right): https://codereview.appspot.com/6870063/diff/17001/ndb/model_test.py#newcode4505 ndb/model_test.py:4505: self.assertEqual(2, entity.container.other_value) I have no concrete use-case for supporting this, it is more a "try not to break existing code" thing. I did a quick check and it indeed comes back as Value after a roundtrip from the datastore (you can still read other_value from it if you muck around with the internals or declare Value to be an Expando). In conclusion: I'm happy to remove this and use self._modelclass (and also use the more restrictive type(self._default) != modelclass in [1]) if you think that this property should be strictly enforced. [1] http://codereview.appspot.com/6903051/