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)

Author: rwhogg

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

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():