From 20e851b817b3420c0eaef4ac4b7deee29ff71a43 Mon Sep 17 00:00:00 2001 From: Daniel Sanche Date: Thu, 5 Dec 2024 13:50:33 -0800 Subject: [PATCH 01/12] prefer top-level meanings for list protos --- google/cloud/datastore/helpers.py | 20 +++++--------------- 1 file changed, 5 insertions(+), 15 deletions(-) diff --git a/google/cloud/datastore/helpers.py b/google/cloud/datastore/helpers.py index 6eaa3b89..d78c7427 100644 --- a/google/cloud/datastore/helpers.py +++ b/google/cloud/datastore/helpers.py @@ -49,7 +49,9 @@ def _get_meaning(value_pb, is_list=False): means it just returns a list of meanings. If all the list meanings agree, it just condenses them. """ - if is_list: + if value_pb.meaning: # Simple field (int32). + return value_pb.meaning + elif is_list: values = value_pb.array_value.values # An empty list will have no values, hence no shared meaning @@ -60,17 +62,7 @@ def _get_meaning(value_pb, is_list=False): # We check among all the meanings, some of which may be None, # the rest which may be enum/int values. all_meanings = [_get_meaning(sub_value_pb) for sub_value_pb in values] - unique_meanings = set(all_meanings) - - if len(unique_meanings) == 1: - # If there is a unique meaning, we preserve it. - return unique_meanings.pop() - else: # We know len(value_pb.array_value.values) > 0. - # If the meaning is not unique, just return all of them. - return all_meanings - - elif value_pb.meaning: # Simple field (int32). - return value_pb.meaning + return all_meanings return None @@ -182,9 +174,7 @@ def _set_pb_meaning_from_entity(entity, name, value, value_pb, is_list=False): return # For lists, we set meaning on each sub-element. - if is_list: - if not isinstance(meaning, list): - meaning = itertools.repeat(meaning) + if is_list and isinstance(meaning, list): val_iter = zip(value_pb.array_value.values, meaning) for sub_value_pb, sub_meaning in val_iter: if sub_meaning is not None: From 930000ed72911b05c00cf0828e8e27eba4412118 Mon Sep 17 00:00:00 2001 From: Daniel Sanche Date: Thu, 5 Dec 2024 13:56:44 -0800 Subject: [PATCH 02/12] all empty shouyld return None --- google/cloud/datastore/helpers.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/google/cloud/datastore/helpers.py b/google/cloud/datastore/helpers.py index d78c7427..73722103 100644 --- a/google/cloud/datastore/helpers.py +++ b/google/cloud/datastore/helpers.py @@ -62,7 +62,8 @@ def _get_meaning(value_pb, is_list=False): # We check among all the meanings, some of which may be None, # the rest which may be enum/int values. all_meanings = [_get_meaning(sub_value_pb) for sub_value_pb in values] - return all_meanings + if any(meaning is not None for meaning in all_meanings): + return all_meanings return None From 76638b376faaf318da42ab46d8370c76a5362b19 Mon Sep 17 00:00:00 2001 From: Daniel Sanche Date: Thu, 5 Dec 2024 13:56:59 -0800 Subject: [PATCH 03/12] fixed test --- tests/unit/test_helpers.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/unit/test_helpers.py b/tests/unit/test_helpers.py index 38702dba..019f058a 100644 --- a/tests/unit/test_helpers.py +++ b/tests/unit/test_helpers.py @@ -1179,7 +1179,8 @@ def test__get_meaning_w_array_value(): sub_value_pb2.string_value = "bye" result = _get_meaning(value_pb, is_list=True) - assert meaning == result + # should preserve sub-value meanings as list + assert [meaning, meaning] == result def test__get_meaning_w_array_value_multiple_meanings(): From 095dceca2f63a7e508162e4bc65ec4d68bb07aa9 Mon Sep 17 00:00:00 2001 From: Daniel Sanche Date: Thu, 5 Dec 2024 14:02:33 -0800 Subject: [PATCH 04/12] added tests --- tests/unit/test_helpers.py | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/tests/unit/test_helpers.py b/tests/unit/test_helpers.py index 019f058a..fad2d6b5 100644 --- a/tests/unit/test_helpers.py +++ b/tests/unit/test_helpers.py @@ -1183,6 +1183,25 @@ def test__get_meaning_w_array_value(): assert [meaning, meaning] == result +def test__get_meaning_w_array_value_root_meaning(): + from google.cloud.datastore_v1.types import entity as entity_pb2 + from google.cloud.datastore.helpers import _get_meaning + + value_pb = entity_pb2.Value() + meaning = 9 + value_pb.meaning = meaning + sub_value_pb1 = value_pb._pb.array_value.values.add() + sub_value_pb2 = value_pb._pb.array_value.values.add() + + sub_value_pb1.meaning = sub_value_pb2.meaning = meaning + sub_value_pb1.string_value = "hi" + sub_value_pb2.string_value = "bye" + + result = _get_meaning(value_pb, is_list=True) + # should preserve sub-value meanings as list + assert meaning == result + + def test__get_meaning_w_array_value_multiple_meanings(): from google.cloud.datastore_v1.types import entity as entity_pb2 from google.cloud.datastore.helpers import _get_meaning @@ -1219,6 +1238,21 @@ def test__get_meaning_w_array_value_meaning_partially_unset(): assert result == [meaning1, None] +def test__get_meaning_w_array_value_meaning_fully_unset(): + from google.cloud.datastore_v1.types import entity as entity_pb2 + from google.cloud.datastore.helpers import _get_meaning + + value_pb = entity_pb2.Value() + sub_value_pb1 = value_pb._pb.array_value.values.add() + sub_value_pb2 = value_pb._pb.array_value.values.add() + + sub_value_pb1.string_value = "hi" + sub_value_pb2.string_value = "bye" + + result = _get_meaning(value_pb, is_list=True) + assert result is None + + def _make_geopoint(*args, **kwargs): from google.cloud.datastore.helpers import GeoPoint From 8b6f9d8b0bafb1c4b9ca313129faca3a35a8bf32 Mon Sep 17 00:00:00 2001 From: Daniel Sanche Date: Thu, 5 Dec 2024 14:33:15 -0800 Subject: [PATCH 05/12] support both root meaning, and sub-value meanings --- google/cloud/datastore/helpers.py | 43 +++++++++++++++---------------- tests/unit/test_helpers.py | 41 ++++++++++++++++++++++------- 2 files changed, 52 insertions(+), 32 deletions(-) diff --git a/google/cloud/datastore/helpers.py b/google/cloud/datastore/helpers.py index 73722103..c9db8b12 100644 --- a/google/cloud/datastore/helpers.py +++ b/google/cloud/datastore/helpers.py @@ -43,29 +43,25 @@ def _get_meaning(value_pb, is_list=False): :param is_list: Boolean indicating if the ``value_pb`` contains a list value. - :rtype: int + :rtype: int | Tuple[int, list[int | None] | None] :returns: The meaning for the ``value_pb`` if one is set, else - :data:`None`. For a list value, if there are disagreeing - means it just returns a list of meanings. If all the - list meanings agree, it just condenses them. + :data:`None`. For a list value, returns a tuple of + the root meaning of the list, and a list of meanings + of each sub-value. If subvalues are all empty, returns + :data:`None` instead of a list. """ - if value_pb.meaning: # Simple field (int32). - return value_pb.meaning - elif is_list: + if is_list: + root_meaning = value_pb.meaning or None values = value_pb.array_value.values - # An empty list will have no values, hence no shared meaning - # set among them. - if len(values) == 0: - return None - # We check among all the meanings, some of which may be None, # the rest which may be enum/int values. - all_meanings = [_get_meaning(sub_value_pb) for sub_value_pb in values] - if any(meaning is not None for meaning in all_meanings): - return all_meanings - - return None + sub_meanings = [_get_meaning(sub_value_pb) for sub_value_pb in values] + if not any(meaning is not None for meaning in sub_meanings): + sub_meanings = None + return root_meaning, sub_meanings + else: + return value_pb.meaning or None def _new_value_pb(entity_pb, name): @@ -175,11 +171,14 @@ def _set_pb_meaning_from_entity(entity, name, value, value_pb, is_list=False): return # For lists, we set meaning on each sub-element. - if is_list and isinstance(meaning, list): - val_iter = zip(value_pb.array_value.values, meaning) - for sub_value_pb, sub_meaning in val_iter: - if sub_meaning is not None: - sub_value_pb.meaning = sub_meaning + if is_list: + root_meaning, sub_meaning_list = meaning + if root_meaning is not None: + value_pb.meaning = root_meaning + if sub_meaning_list: + for sub_value_pb, sub_meaning in zip(value_pb.array_value.values, sub_meaning_list): + if sub_meaning is not None: + sub_value_pb.meaning = sub_meaning else: value_pb.meaning = meaning diff --git a/tests/unit/test_helpers.py b/tests/unit/test_helpers.py index fad2d6b5..749eeba2 100644 --- a/tests/unit/test_helpers.py +++ b/tests/unit/test_helpers.py @@ -361,19 +361,21 @@ def test_entity_to_protobuf_w_variable_meanings(): entity = Entity() name = "quux" entity[name] = values = [1, 20, 300] - meaning = 9 - entity._meanings[name] = ([None, meaning, None], values) + root_meaning = 31 + sub_meaning = 9 + entity._meanings[name] = ((root_meaning, [None, sub_meaning, None]), values) entity_pb = entity_to_protobuf(entity) # Construct the expected protobuf. expected_pb = entity_pb2.Entity() value_pb = _new_value_pb(expected_pb, name) + value_pb.meaning = root_meaning value0 = value_pb.array_value.values.add() value0.integer_value = values[0] # The only array entry with a meaning is the middle one. value1 = value_pb.array_value.values.add() value1.integer_value = values[1] - value1.meaning = meaning + value1.meaning = sub_meaning value2 = value_pb.array_value.values.add() value2.integer_value = values[2] @@ -1162,7 +1164,7 @@ def test__get_meaning_w_empty_array_value(): value_pb._pb.array_value.values.pop() result = _get_meaning(value_pb, is_list=True) - assert result is None + assert result == (None, None) def test__get_meaning_w_array_value(): @@ -1180,7 +1182,7 @@ def test__get_meaning_w_array_value(): result = _get_meaning(value_pb, is_list=True) # should preserve sub-value meanings as list - assert [meaning, meaning] == result + assert (None, [meaning, meaning]) == result def test__get_meaning_w_array_value_root_meaning(): @@ -1193,13 +1195,32 @@ def test__get_meaning_w_array_value_root_meaning(): sub_value_pb1 = value_pb._pb.array_value.values.add() sub_value_pb2 = value_pb._pb.array_value.values.add() - sub_value_pb1.meaning = sub_value_pb2.meaning = meaning sub_value_pb1.string_value = "hi" sub_value_pb2.string_value = "bye" result = _get_meaning(value_pb, is_list=True) # should preserve sub-value meanings as list - assert meaning == result + assert (meaning, None) == result + + +def test__get_meaning_w_array_value_root_and_sub_meanings(): + from google.cloud.datastore_v1.types import entity as entity_pb2 + from google.cloud.datastore.helpers import _get_meaning + + value_pb = entity_pb2.Value() + root_meaning = 9 + sub_meaning = 3 + value_pb.meaning = root_meaning + sub_value_pb1 = value_pb._pb.array_value.values.add() + sub_value_pb2 = value_pb._pb.array_value.values.add() + + sub_value_pb1.meaning = sub_value_pb2.meaning = sub_meaning + sub_value_pb1.string_value = "hi" + sub_value_pb2.string_value = "bye" + + result = _get_meaning(value_pb, is_list=True) + # should preserve sub-value meanings as list + assert (root_meaning, [sub_meaning, sub_meaning]) == result def test__get_meaning_w_array_value_multiple_meanings(): @@ -1218,7 +1239,7 @@ def test__get_meaning_w_array_value_multiple_meanings(): sub_value_pb2.string_value = "bye" result = _get_meaning(value_pb, is_list=True) - assert result == [meaning1, meaning2] + assert result == (None, [meaning1, meaning2]) def test__get_meaning_w_array_value_meaning_partially_unset(): @@ -1235,7 +1256,7 @@ def test__get_meaning_w_array_value_meaning_partially_unset(): sub_value_pb2.string_value = "bye" result = _get_meaning(value_pb, is_list=True) - assert result == [meaning1, None] + assert result == (None, [meaning1, None]) def test__get_meaning_w_array_value_meaning_fully_unset(): @@ -1250,7 +1271,7 @@ def test__get_meaning_w_array_value_meaning_fully_unset(): sub_value_pb2.string_value = "bye" result = _get_meaning(value_pb, is_list=True) - assert result is None + assert result == (None, None) def _make_geopoint(*args, **kwargs): From 4a28ea2635e1e45bc8c781cae3532ec3dc937c0b Mon Sep 17 00:00:00 2001 From: Daniel Sanche Date: Thu, 5 Dec 2024 14:34:25 -0800 Subject: [PATCH 06/12] avoid unneeded recursion --- google/cloud/datastore/helpers.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/google/cloud/datastore/helpers.py b/google/cloud/datastore/helpers.py index c9db8b12..6009ac91 100644 --- a/google/cloud/datastore/helpers.py +++ b/google/cloud/datastore/helpers.py @@ -43,7 +43,7 @@ def _get_meaning(value_pb, is_list=False): :param is_list: Boolean indicating if the ``value_pb`` contains a list value. - :rtype: int | Tuple[int, list[int | None] | None] + :rtype: int | Tuple[Optional[int], Optional[list[int | None]]] | None :returns: The meaning for the ``value_pb`` if one is set, else :data:`None`. For a list value, returns a tuple of the root meaning of the list, and a list of meanings @@ -56,7 +56,7 @@ def _get_meaning(value_pb, is_list=False): # We check among all the meanings, some of which may be None, # the rest which may be enum/int values. - sub_meanings = [_get_meaning(sub_value_pb) for sub_value_pb in values] + sub_meanings = [sub_value_pb.meaning or None for sub_value_pb in values] if not any(meaning is not None for meaning in sub_meanings): sub_meanings = None return root_meaning, sub_meanings From a8b8c65444c0686f45c49bc61acdb2543051fee0 Mon Sep 17 00:00:00 2001 From: Daniel Sanche Date: Fri, 6 Dec 2024 14:08:47 -0600 Subject: [PATCH 07/12] added new end to end test --- tests/unit/test_helpers.py | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/tests/unit/test_helpers.py b/tests/unit/test_helpers.py index 749eeba2..0c0e2525 100644 --- a/tests/unit/test_helpers.py +++ b/tests/unit/test_helpers.py @@ -1274,6 +1274,41 @@ def test__get_meaning_w_array_value_meaning_fully_unset(): assert result == (None, None) +def test__array_w_meaning_end_to_end(): + """ + Test proto->entity->proto with an array with a meaning field + """ + from google.cloud.datastore_v1.types import entity as entity_pb2 + from google.cloud.datastore.helpers import entity_from_protobuf + from google.cloud.datastore.helpers import entity_to_protobuf + orig_pb = entity_pb2.Entity() + value_pb = orig_pb._pb.properties.get_or_create("value") + value_pb.meaning = 31 + sub_value_pb1 = value_pb.array_value.values.add() + sub_value_pb1.double_value = 1 + sub_value_pb1.meaning = 1 + sub_value_pb2 = value_pb.array_value.values.add() + sub_value_pb2.double_value = 2 + sub_value_pb3 = value_pb.array_value.values.add() + sub_value_pb3.double_value = 3 + sub_value_pb3.meaning = 3 + # convert to entity + entity = entity_from_protobuf(orig_pb._pb) + assert entity._meanings["value"][0] == (31, [1, None, 3]) + assert entity._meanings["value"][1] == [1, 2, 3] + # convert back to pb + output_entity_pb = entity_to_protobuf(entity) + final_pb = output_entity_pb._pb.properties["value"] + assert final_pb.meaning == 31 + assert len(final_pb.array_value.values) == 3 + assert final_pb.array_value.values[0].meaning == 1 + assert final_pb.array_value.values[0].double_value == 1 + assert final_pb.array_value.values[1].meaning == 0 + assert final_pb.array_value.values[1].double_value == 2 + assert final_pb.array_value.values[2].meaning == 3 + assert final_pb.array_value.values[2].double_value == 3 + + def _make_geopoint(*args, **kwargs): from google.cloud.datastore.helpers import GeoPoint From c636d460d0ef85555ac87d8676b03020d90795b0 Mon Sep 17 00:00:00 2001 From: Daniel Sanche Date: Fri, 6 Dec 2024 14:10:33 -0600 Subject: [PATCH 08/12] fixed lint --- google/cloud/datastore/helpers.py | 5 +++-- tests/unit/test_helpers.py | 1 + 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/google/cloud/datastore/helpers.py b/google/cloud/datastore/helpers.py index 6009ac91..70dc4098 100644 --- a/google/cloud/datastore/helpers.py +++ b/google/cloud/datastore/helpers.py @@ -18,7 +18,6 @@ """ import datetime -import itertools from google.protobuf import struct_pb2 from google.type import latlng_pb2 @@ -176,7 +175,9 @@ def _set_pb_meaning_from_entity(entity, name, value, value_pb, is_list=False): if root_meaning is not None: value_pb.meaning = root_meaning if sub_meaning_list: - for sub_value_pb, sub_meaning in zip(value_pb.array_value.values, sub_meaning_list): + for sub_value_pb, sub_meaning in zip( + value_pb.array_value.values, sub_meaning_list + ): if sub_meaning is not None: sub_value_pb.meaning = sub_meaning else: diff --git a/tests/unit/test_helpers.py b/tests/unit/test_helpers.py index 0c0e2525..92095f0a 100644 --- a/tests/unit/test_helpers.py +++ b/tests/unit/test_helpers.py @@ -1281,6 +1281,7 @@ def test__array_w_meaning_end_to_end(): from google.cloud.datastore_v1.types import entity as entity_pb2 from google.cloud.datastore.helpers import entity_from_protobuf from google.cloud.datastore.helpers import entity_to_protobuf + orig_pb = entity_pb2.Entity() value_pb = orig_pb._pb.properties.get_or_create("value") value_pb.meaning = 31 From e420263c9214572377b2aca58145535141f731b7 Mon Sep 17 00:00:00 2001 From: Daniel Sanche Date: Fri, 6 Dec 2024 14:34:53 -0600 Subject: [PATCH 09/12] clarify that None means no changes --- google/cloud/datastore/helpers.py | 6 +++++- tests/unit/test_helpers.py | 23 +++++++++++++++++++++++ 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/google/cloud/datastore/helpers.py b/google/cloud/datastore/helpers.py index 70dc4098..2112512a 100644 --- a/google/cloud/datastore/helpers.py +++ b/google/cloud/datastore/helpers.py @@ -144,6 +144,10 @@ def entity_from_protobuf(pb): def _set_pb_meaning_from_entity(entity, name, value, value_pb, is_list=False): """Add meaning information (from an entity) to a protobuf. + value_pb is assumed to have no `meaning` data currently present. + This means if the entity's meaning data is None, this function will do nothing, + rather than removing any existing data. + :type entity: :class:`google.cloud.datastore.entity.Entity` :param entity: The entity to be turned into a protobuf. @@ -180,7 +184,7 @@ def _set_pb_meaning_from_entity(entity, name, value, value_pb, is_list=False): ): if sub_meaning is not None: sub_value_pb.meaning = sub_meaning - else: + elif meaning is not None: value_pb.meaning = meaning diff --git a/tests/unit/test_helpers.py b/tests/unit/test_helpers.py index 92095f0a..e3af1be8 100644 --- a/tests/unit/test_helpers.py +++ b/tests/unit/test_helpers.py @@ -1274,6 +1274,29 @@ def test__get_meaning_w_array_value_meaning_fully_unset(): assert result == (None, None) +@pytest.mark.parametrize("orig_meaning_data", [0, 1]) +def test__set_pb_meaning_w_array_value_fully_unset(orig_meaning_data): + """ + call _set_pb_meaning_from_entity with meaning=None data. + Should not touch proto's meaning field + """ + from google.cloud.datastore_v1.types import entity as entity_pb2 + from google.cloud.datastore.helpers import _set_pb_meaning_from_entity + from google.cloud.datastore.entity import Entity + + orig_pb = entity_pb2.Entity() + value_pb = orig_pb._pb.properties.get_or_create("value") + value_pb.meaning = orig_meaning_data + sub_value_pb1 = value_pb.array_value.values.add() + sub_value_pb1.meaning = orig_meaning_data + + entity = Entity(key="key") + entity._meanings = {"value": ((None, None), None)} + _set_pb_meaning_from_entity(entity, "value", None, value_pb, is_list=True) + assert value_pb.meaning == orig_meaning_data + assert value_pb.array_value.values[0].meaning == orig_meaning_data + + def test__array_w_meaning_end_to_end(): """ Test proto->entity->proto with an array with a meaning field From 0feba139730175c91ee1df3b511f3cf567c8b24c Mon Sep 17 00:00:00 2001 From: Daniel Sanche Date: Fri, 6 Dec 2024 14:52:17 -0600 Subject: [PATCH 10/12] to fix system tests, return None if no meaning or submeanings --- google/cloud/datastore/helpers.py | 12 ++++++++-- tests/unit/test_helpers.py | 37 ++++++++++++++++++++++++------- 2 files changed, 39 insertions(+), 10 deletions(-) diff --git a/google/cloud/datastore/helpers.py b/google/cloud/datastore/helpers.py index 2112512a..d0454315 100644 --- a/google/cloud/datastore/helpers.py +++ b/google/cloud/datastore/helpers.py @@ -58,7 +58,11 @@ def _get_meaning(value_pb, is_list=False): sub_meanings = [sub_value_pb.meaning or None for sub_value_pb in values] if not any(meaning is not None for meaning in sub_meanings): sub_meanings = None - return root_meaning, sub_meanings + if root_meaning is None and sub_meanings is None: + # no meanings to save + return None + else: + return root_meaning, sub_meanings else: return value_pb.meaning or None @@ -173,6 +177,10 @@ def _set_pb_meaning_from_entity(entity, name, value, value_pb, is_list=False): if orig_value is not value: return + # break early if no meaning data to set + if meaning is None: + return + # For lists, we set meaning on each sub-element. if is_list: root_meaning, sub_meaning_list = meaning @@ -184,7 +192,7 @@ def _set_pb_meaning_from_entity(entity, name, value, value_pb, is_list=False): ): if sub_meaning is not None: sub_value_pb.meaning = sub_meaning - elif meaning is not None: + else: value_pb.meaning = meaning diff --git a/tests/unit/test_helpers.py b/tests/unit/test_helpers.py index e3af1be8..f641f117 100644 --- a/tests/unit/test_helpers.py +++ b/tests/unit/test_helpers.py @@ -1164,7 +1164,7 @@ def test__get_meaning_w_empty_array_value(): value_pb._pb.array_value.values.pop() result = _get_meaning(value_pb, is_list=True) - assert result == (None, None) + assert result is None def test__get_meaning_w_array_value(): @@ -1271,11 +1271,12 @@ def test__get_meaning_w_array_value_meaning_fully_unset(): sub_value_pb2.string_value = "bye" result = _get_meaning(value_pb, is_list=True) - assert result == (None, None) + assert result is None -@pytest.mark.parametrize("orig_meaning_data", [0, 1]) -def test__set_pb_meaning_w_array_value_fully_unset(orig_meaning_data): +@pytest.mark.parametrize("orig_root_meaning", [0,1]) +@pytest.mark.parametrize("orig_sub_meaning", [0,1]) +def test__set_pb_meaning_w_array_value_fully_unset(orig_root_meaning, orig_sub_meaning): """ call _set_pb_meaning_from_entity with meaning=None data. Should not touch proto's meaning field @@ -1286,15 +1287,35 @@ def test__set_pb_meaning_w_array_value_fully_unset(orig_meaning_data): orig_pb = entity_pb2.Entity() value_pb = orig_pb._pb.properties.get_or_create("value") - value_pb.meaning = orig_meaning_data + value_pb.meaning = orig_root_meaning sub_value_pb1 = value_pb.array_value.values.add() - sub_value_pb1.meaning = orig_meaning_data + sub_value_pb1.meaning = orig_sub_meaning entity = Entity(key="key") entity._meanings = {"value": ((None, None), None)} _set_pb_meaning_from_entity(entity, "value", None, value_pb, is_list=True) - assert value_pb.meaning == orig_meaning_data - assert value_pb.array_value.values[0].meaning == orig_meaning_data + assert value_pb.meaning == orig_root_meaning + assert value_pb.array_value.values[0].meaning == orig_sub_meaning + + +@pytest.mark.parametrize("orig_meaning", [0, 1]) +def test__set_pb_meaning_w_value_unset(orig_meaning): + """ + call _set_pb_meaning_from_entity with meaning=None data. + Should not touch proto's meaning field + """ + from google.cloud.datastore_v1.types import entity as entity_pb2 + from google.cloud.datastore.helpers import _set_pb_meaning_from_entity + from google.cloud.datastore.entity import Entity + + orig_pb = entity_pb2.Entity() + value_pb = orig_pb._pb.properties.get_or_create("value") + value_pb.meaning = orig_meaning + + entity = Entity(key="key") + entity._meanings = {"value": (None, None)} + _set_pb_meaning_from_entity(entity, "value", None, value_pb, is_list=False) + assert value_pb.meaning == orig_meaning def test__array_w_meaning_end_to_end(): From ae9806071e301ca89d16b866a33969de36d9f4cf Mon Sep 17 00:00:00 2001 From: Daniel Sanche Date: Fri, 6 Dec 2024 15:01:01 -0600 Subject: [PATCH 11/12] fixed lint --- tests/unit/test_helpers.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/unit/test_helpers.py b/tests/unit/test_helpers.py index f641f117..a6f63a80 100644 --- a/tests/unit/test_helpers.py +++ b/tests/unit/test_helpers.py @@ -1274,8 +1274,8 @@ def test__get_meaning_w_array_value_meaning_fully_unset(): assert result is None -@pytest.mark.parametrize("orig_root_meaning", [0,1]) -@pytest.mark.parametrize("orig_sub_meaning", [0,1]) +@pytest.mark.parametrize("orig_root_meaning", [0, 1]) +@pytest.mark.parametrize("orig_sub_meaning", [0, 1]) def test__set_pb_meaning_w_array_value_fully_unset(orig_root_meaning, orig_sub_meaning): """ call _set_pb_meaning_from_entity with meaning=None data. From f8024bc252506353ad1f6d64345a5009b8e477bb Mon Sep 17 00:00:00 2001 From: Daniel Sanche Date: Tue, 10 Dec 2024 12:54:39 -0800 Subject: [PATCH 12/12] small refactor --- google/cloud/datastore/helpers.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/google/cloud/datastore/helpers.py b/google/cloud/datastore/helpers.py index d0454315..d491360c 100644 --- a/google/cloud/datastore/helpers.py +++ b/google/cloud/datastore/helpers.py @@ -177,12 +177,11 @@ def _set_pb_meaning_from_entity(entity, name, value, value_pb, is_list=False): if orig_value is not value: return - # break early if no meaning data to set if meaning is None: + # no meaning data to set return - - # For lists, we set meaning on each sub-element. - if is_list: + elif is_list: + # for lists, set meaning on the root pb and on each sub-element root_meaning, sub_meaning_list = meaning if root_meaning is not None: value_pb.meaning = root_meaning