From 29f5a853174857545e225fe2f0c682dfa0bc3884 Mon Sep 17 00:00:00 2001 From: "Bob \"Wombat\" Hogg" Date: Wed, 16 Nov 2022 06:32:48 -0500 Subject: [PATCH] fix(model): Ensure repeated props have same kind when converting from ds (#824) Legacy NDB had a bug where, with repeated Expando properties, it could end up writing arrays of different length if some entities had missing values for certain subproperties. When Cloud NDB read this out, it might have only loaded entities corresponding to the shorter array length. See issue #129 . To fix this, with PR #176 , we made Cloud NDB check if there are length differences and pad out the array as necessary. However, depending on the order properties are returned from Datastore in, we may have accidentally padded with subentities of the wrong kind, because it was possible to skip over updating the kind if we alternated between updating repeated properties. (Eg: A.a with 2 elements, B.b with 3 elements, A.c with 3 elements -> A would end up with an element of B's kind) --- google/cloud/ndb/model.py | 9 +++++++-- tests/unit/test_model.py | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 2 deletions(-) diff --git a/google/cloud/ndb/model.py b/google/cloud/ndb/model.py index 6b046af4..01ca9ad7 100644 --- a/google/cloud/ndb/model.py +++ b/google/cloud/ndb/model.py @@ -543,6 +543,7 @@ def _entity_from_ds_entity(ds_entity, model_class=None): Args: ds_entity (google.cloud.datastore_v1.types.Entity): An entity to be deserialized. + model_class (class): Optional; ndb Model class type. Returns: .Model: The deserialized entity. @@ -623,10 +624,14 @@ def new_entity(key): # different lengths for the subproperties, which was a # bug. We work around this when reading out such values # by making sure our repeated property is the same - # length as the longest suproperty. + # length as the longest subproperty. + # Make sure to create a key of the same kind as + # the other entries in the value list while len(subvalue) > len(value): # Need to make some more subentities - value.append(new_entity(key._key)) + expando_kind = structprop._model_class._get_kind() + expando_key = key_module.Key(expando_kind, None) + value.append(new_entity(expando_key._key)) # Branch coverage bug, # See: https://github.com/nedbat/coveragepy/issues/817 diff --git a/tests/unit/test_model.py b/tests/unit/test_model.py index 157cba80..5d07414a 100644 --- a/tests/unit/test_model.py +++ b/tests/unit/test_model.py @@ -5606,6 +5606,40 @@ class ThisKind(model.Model): assert entity.baz[2].bar == "iminjail" assert entity.copacetic is True + @staticmethod + @pytest.mark.usefixtures("in_context") + def test_legacy_repeated_structured_property_uneven_expandos(): + class Expando1(model.Expando): + bar = model.StringProperty() + + class Expando2(model.Expando): + qux = model.StringProperty() + + class ThisKind(model.Model): + foo = model.StructuredProperty(model_class=Expando1, repeated=True) + baz = model.StructuredProperty(model_class=Expando2, repeated=True) + + key = datastore.Key("ThisKind", 123, project="testing") + datastore_entity = datastore.Entity(key=key) + datastore_entity.items = mock.Mock( + return_value=( + # Order matters here + ("foo.bar", ["foo_bar_1"]), + ("baz.qux", ["baz_qux_1", "baz_qux_2"]), + ("foo.custom_1", ["foo_c1_1", "foo_c1_2"]), # longer than foo.bar + ) + ) + entity = model._entity_from_ds_entity(datastore_entity) + assert isinstance(entity, ThisKind) + assert len(entity.foo) == 2 + assert len(entity.baz) == 2 + assert entity.foo[0].bar == "foo_bar_1" + assert entity.foo[0].custom_1 == "foo_c1_1" + assert entity.foo[1].bar is None + assert entity.foo[1].custom_1 == "foo_c1_2" + assert entity.baz[0].qux == "baz_qux_1" + assert entity.baz[1].qux == "baz_qux_2" + @staticmethod @pytest.mark.usefixtures("in_context") def test_legacy_repeated_structured_property_with_name():