From 59cfef1045ac350a21e16c7eccc79141b8dee29e Mon Sep 17 00:00:00 2001 From: chemelnucfin Date: Tue, 20 Mar 2018 12:22:36 -0700 Subject: [PATCH] Distinguish FieldPath classes from field path strings (#4466) --- .../cloud/firestore_v1beta1/_helpers.py | 14 +- firestore/tests/unit/test__helpers.py | 205 ++++++++++++++---- 2 files changed, 169 insertions(+), 50 deletions(-) diff --git a/firestore/google/cloud/firestore_v1beta1/_helpers.py b/firestore/google/cloud/firestore_v1beta1/_helpers.py index 371e08d944b0..2f872ad448a0 100644 --- a/firestore/google/cloud/firestore_v1beta1/_helpers.py +++ b/firestore/google/cloud/firestore_v1beta1/_helpers.py @@ -162,7 +162,8 @@ def to_api_repr(self): """ api_repr = [] for part in self.parts: - if re.match(self.simple_field_name, part): + match = re.match(self.simple_field_name, part) + if match and match.group(0) == part: api_repr.append(part) else: replaced = part.replace('\\', '\\\\').replace('`', '\\`') @@ -281,14 +282,15 @@ def path_end_conflict(self, field_path, conflicting_paths): Returns: ValueError: Always. """ - conflict_parts = [field_path] + conflict_parts = list(field_path.parts) while conflicting_paths is not self.PATH_END: # Grab any item, we are just looking for one example. part, conflicting_paths = next(six.iteritems(conflicting_paths)) conflict_parts.append(part) conflict = get_field_path(conflict_parts) - msg = self.FIELD_PATH_CONFLICT.format(field_path, conflict) + msg = self.FIELD_PATH_CONFLICT.format( + field_path.to_api_repr(), conflict) return ValueError(msg) def add_field_path_end( @@ -340,7 +342,9 @@ def add_value_at_field_path(self, field_path, value): Raises: ValueError: If there is an ambiguity. """ - parts = parse_field_path(field_path) + if isinstance(field_path, six.string_types): + field_path = FieldPath.from_string(field_path) + parts = field_path.parts to_update = self.get_update_values(value) curr_paths = self.unpacked_field_paths for index, part in enumerate(parts[:-1]): @@ -912,7 +916,7 @@ def canonicalize_field_paths(field_paths): .. _Document: https://cloud.google.com/firestore/docs/reference/rpc/google.firestore.v1beta1#google.firestore.v1beta1.Document # NOQA """ - return [FieldPath.from_string(path).to_api_repr() for path in field_paths] + return [path.to_api_repr() for path in field_paths] def pbs_for_update(client, document_path, field_updates, option): diff --git a/firestore/tests/unit/test__helpers.py b/firestore/tests/unit/test__helpers.py index 3a69cf393f28..8b0f262d642b 100644 --- a/firestore/tests/unit/test__helpers.py +++ b/firestore/tests/unit/test__helpers.py @@ -135,12 +135,17 @@ def test_unicode(self): def test_to_api_repr_a(self): parts = 'a' field_path = self._make_one(parts) - self.assertEqual('a', field_path.to_api_repr()) + self.assertEqual(field_path.to_api_repr(), 'a') def test_to_api_repr_backtick(self): parts = '`' field_path = self._make_one(parts) - self.assertEqual('`\``', field_path.to_api_repr()) + self.assertEqual(field_path.to_api_repr(), '`\``') + + def test_to_api_repr_dot(self): + parts = '.' + field_path = self._make_one(parts) + self.assertEqual(field_path.to_api_repr(), '`.`') def test_to_api_repr_slash(self): parts = '\\' @@ -167,6 +172,15 @@ def test_to_api_repr_number_non_simple(self): field_path = self._make_one(parts) self.assertEqual(field_path.to_api_repr(), '`03`') + def test_to_api_repr_simple_with_dot(self): + field_path = self._make_one('a.b') + self.assertEqual(field_path.to_api_repr(), '`a.b`') + + def test_to_api_repr_non_simple_with_dot(self): + parts = 'a.一' + field_path = self._make_one(parts) + self.assertEqual(field_path.to_api_repr(), '`a.一`') + def test_to_api_repr_simple(self): parts = 'a0332432' field_path = self._make_one(parts) @@ -181,6 +195,12 @@ def test_to_api_repr_chain(self): def test_from_string(self): field_path = self._get_target_class().from_string('a.b.c') self.assertEqual(field_path.parts, ('a', 'b', 'c')) + self.assertEqual(field_path.to_api_repr(), 'a.b.c') + + def test_from_string_non_simple(self): + field_path = self._get_target_class().from_string('a.一') + self.assertEqual(field_path.parts, ('a', '一')) + self.assertEqual(field_path.to_api_repr(), 'a.`一`') def test_list_splat(self): parts = ['a', 'b', 'c'] @@ -203,6 +223,11 @@ def test_empty_string_fails(self): with self.assertRaises(ValueError): self._get_target_class().from_string(parts) + def test_empty_field_name_fails(self): + parts = 'a..b' + with self.assertRaises(ValueError): + self._get_target_class().from_string(parts) + def test_list_fails(self): parts = ['a', 'b', 'c'] with self.assertRaises(ValueError): @@ -295,11 +320,12 @@ def test_path_end_conflict_one_match(self): helper = self._make_one(None) key = 'end' conflicting_paths = {key: helper.PATH_END} - field_path = 'start' + field_path = _helpers.FieldPath.from_string('start') err_val = helper.path_end_conflict(field_path, conflicting_paths) self.assertIsInstance(err_val, ValueError) - conflict = _helpers.get_field_path([field_path, key]) - err_msg = helper.FIELD_PATH_CONFLICT.format(field_path, conflict) + conflict = _helpers.get_field_path([field_path.to_api_repr(), key]) + err_msg = helper.FIELD_PATH_CONFLICT.format( + field_path.to_api_repr(), conflict) self.assertEqual(err_val.args, (err_msg,)) def test_path_end_conflict_multiple_matches(self): @@ -317,18 +343,22 @@ def test_path_end_conflict_multiple_matches(self): ('nope', helper.PATH_END), )) - field_path = 'start' + field_path = _helpers.FieldPath.from_string('start') err_val = helper.path_end_conflict(field_path, conflicting_paths) self.assertIsInstance(err_val, ValueError) - conflict = _helpers.get_field_path([field_path, middle_part, end_part]) - err_msg = helper.FIELD_PATH_CONFLICT.format(field_path, conflict) + conflict = _helpers.get_field_path( + [field_path.to_api_repr(), middle_part, end_part]) + err_msg = helper.FIELD_PATH_CONFLICT.format( + field_path.to_api_repr(), conflict) self.assertEqual(err_val.args, (err_msg,)) def test_add_field_path_end_success(self): + from google.cloud.firestore_v1beta1 import _helpers + helper = self._make_one(None) curr_paths = {} to_update = {} - field_path = 'a.b.c' + field_path = _helpers.FieldPath.from_string('a.b.c') value = 1029830 final_part = 'c' ret_val = helper.add_field_path_end( @@ -341,41 +371,49 @@ def test_add_field_path_end_success(self): self.assertEqual(helper.field_paths, [field_path]) def test_add_field_path_end_failure(self): + from google.cloud.firestore_v1beta1 import _helpers + helper = self._make_one(None) curr_paths = {'c': {'d': helper.PATH_END}} to_update = {'c': {'d': 'jewelry'}} - helper.field_paths = ['a.b.c.d'] + helper.field_paths = [_helpers.FieldPath.from_string('a.b.c.d')] - field_path = 'a.b.c' + field_path = _helpers.FieldPath.from_string('a.b.c') value = 1029830 final_part = 'c' with self.assertRaises(ValueError) as exc_info: helper.add_field_path_end( field_path, value, final_part, curr_paths, to_update) - err_msg = helper.FIELD_PATH_CONFLICT.format(field_path, 'a.b.c.d') + err_msg = helper.FIELD_PATH_CONFLICT.format( + field_path.to_api_repr(), 'a.b.c.d') self.assertEqual(exc_info.exception.args, (err_msg,)) self.assertEqual(curr_paths, {'c': {'d': helper.PATH_END}}) self.assertEqual(to_update, {'c': {'d': 'jewelry'}}) - self.assertEqual(helper.field_paths, ['a.b.c.d']) + self.assertEqual( + helper.field_paths, [_helpers.FieldPath.from_string('a.b.c.d')]) def test_add_value_at_field_path_first_with_field(self): - helper = self._make_one(None) + from google.cloud.firestore_v1beta1 import _helpers - field_path = 'zap' + helper = self._make_one(None) + field_path = _helpers.FieldPath.from_string('zap') value = 121 ret_val = helper.add_value_at_field_path(field_path, value) self.assertIsNone(ret_val) - self.assertEqual(helper.update_values, {field_path: value}) + self.assertEqual( + helper.update_values, {field_path.to_api_repr(): value}) self.assertEqual(helper.field_paths, [field_path]) self.assertEqual( - helper.unpacked_field_paths, {field_path: helper.PATH_END}) + helper.unpacked_field_paths, + {field_path.to_api_repr(): helper.PATH_END}) def test_add_value_at_field_path_first_with_path(self): - helper = self._make_one(None) + from google.cloud.firestore_v1beta1 import _helpers - field_path = 'a.b.c' + helper = self._make_one(None) + field_path = _helpers.FieldPath.from_string('a.b.c') value = b'\x01\x02' ret_val = helper.add_value_at_field_path(field_path, value) @@ -386,29 +424,54 @@ def test_add_value_at_field_path_first_with_path(self): helper.unpacked_field_paths, {'a': {'b': {'c': helper.PATH_END}}}) def test_add_value_at_field_paths_at_same_level(self): - helper = self._make_one(None) + from google.cloud.firestore_v1beta1 import _helpers - field_path = 'a.c' + helper = self._make_one(None) + field_path = _helpers.FieldPath.from_string('a.c') value = False helper.update_values = {'a': {'b': 80}} - helper.field_paths = ['a.b'] + helper.field_paths = [_helpers.FieldPath.from_string('a.b')] helper.unpacked_field_paths = {'a': {'b': helper.PATH_END}} - ret_val = helper.add_value_at_field_path(field_path, value) self.assertIsNone(ret_val) self.assertEqual(helper.update_values, {'a': {'b': 80, 'c': value}}) - self.assertEqual(helper.field_paths, ['a.b', field_path]) + self.assertEqual( + helper.field_paths, + [_helpers.FieldPath.from_string('a.b'), field_path]) self.assertEqual( helper.unpacked_field_paths, {'a': {'b': helper.PATH_END, 'c': helper.PATH_END}}) + def test_add_value_at_field_paths_non_simple_field_names(self): + from google.cloud.firestore_v1beta1 import _helpers + + helper = self._make_one(None) + field_path = _helpers.FieldPath.from_string('a.一') + value = [1, 2, 3] + helper.update_values = {'a': {'b': 80}} + helper.field_paths = [_helpers.FieldPath.from_string('a.b')] + helper.unpacked_field_paths = {'a': {'b': helper.PATH_END}} + helper.add_value_at_field_path(field_path, value) + + self.assertEqual(helper.update_values, {'a': {'b': 80, + '一': value} + }) + self.assertEqual( + helper.field_paths, + [_helpers.FieldPath.from_string('a.b'), field_path]) + self.assertEqual( + helper.unpacked_field_paths, + {'a': {'b': helper.PATH_END, + '一': helper.PATH_END}}) + def test_add_value_at_field_path_delete(self): + from google.cloud.firestore_v1beta1 import _helpers from google.cloud.firestore_v1beta1.constants import DELETE_FIELD helper = self._make_one(None) - field_path = 'foo.bar' + field_path = _helpers.FieldPath.from_string('foo.bar') value = DELETE_FIELD ret_val = helper.add_value_at_field_path(field_path, value) @@ -419,12 +482,14 @@ def test_add_value_at_field_path_delete(self): helper.unpacked_field_paths, {'foo': {'bar': helper.PATH_END}}) def test_add_value_at_field_path_failure_adding_more_specific_path(self): + from google.cloud.firestore_v1beta1 import _helpers + helper = self._make_one(None) - field_path = 'DD.F' + field_path = _helpers.FieldPath.from_string('DD.F') value = 99 helper.update_values = {'DD': {'E': 19}} - helper.field_paths = ['DD'] + helper.field_paths = [_helpers.FieldPath.from_string('DD')] helper.unpacked_field_paths = {'DD': helper.PATH_END} with self.assertRaises(ValueError) as exc_info: helper.add_value_at_field_path(field_path, value) @@ -433,13 +498,17 @@ def test_add_value_at_field_path_failure_adding_more_specific_path(self): self.assertEqual(exc_info.exception.args, (err_msg,)) # Make sure inputs are unchanged. self.assertEqual(helper.update_values, {'DD': {'E': 19}}) - self.assertEqual(helper.field_paths, ['DD']) + self.assertEqual( + helper.field_paths, + [_helpers.FieldPath.from_string('DD')]) self.assertEqual(helper.unpacked_field_paths, {'DD': helper.PATH_END}) def test_add_value_at_field_path_failure_adding_more_generic_path(self): + from google.cloud.firestore_v1beta1 import _helpers + helper = self._make_one(None) - field_path = 'x.y' + field_path = _helpers.FieldPath.from_string('x.y') value = {'t': False} helper.update_values = {'x': {'y': {'z': 104.5}}} helper.field_paths = ['x.y.z'] @@ -447,7 +516,8 @@ def test_add_value_at_field_path_failure_adding_more_generic_path(self): with self.assertRaises(ValueError) as exc_info: helper.add_value_at_field_path(field_path, value) - err_msg = helper.FIELD_PATH_CONFLICT.format(field_path, 'x.y.z') + err_msg = helper.FIELD_PATH_CONFLICT.format( + field_path.to_api_repr(), 'x.y.z') self.assertEqual(exc_info.exception.args, (err_msg,)) # Make sure inputs are unchanged. self.assertEqual(helper.update_values, {'x': {'y': {'z': 104.5}}}) @@ -456,32 +526,48 @@ def test_add_value_at_field_path_failure_adding_more_generic_path(self): helper.unpacked_field_paths, {'x': {'y': {'z': helper.PATH_END}}}) def test_parse(self): + import six + from google.cloud.firestore_v1beta1 import _helpers + # "Cheat" and use OrderedDict-s so that iteritems() is deterministic. field_updates = collections.OrderedDict(( - ('a.b.c', 10), - ('d', None), - ('e.f1', [u'no', b'yes']), - ('e.f2', 4.5), - ('g', {'key': True}), + (_helpers.FieldPath.from_string('a.b.c'), 10), + (_helpers.FieldPath.from_string('d'), None), + (_helpers.FieldPath.from_string('e.f1'), [u'no', b'yes']), + (_helpers.FieldPath.from_string('e.f2'), 4.5), + (_helpers.FieldPath.from_string('e.f3'), (3, 1)), + (_helpers.FieldPath.from_string('g'), {'key': True}), + (_helpers.FieldPath('h', 'i'), '3'), + (_helpers.FieldPath('j.k', 'l.m'), set(['2', '3'])), + (_helpers.FieldPath('a', '一'), {1: 2}), + (_helpers.FieldPath('a.一'), {3: 4}), )) helper = self._make_one(field_updates) update_values, field_paths = helper.parse() - expected_updates = { 'a': { 'b': { - 'c': field_updates['a.b.c'], + 'c': field_updates[_helpers.FieldPath.from_string('a.b.c')], }, + '一': field_updates[_helpers.FieldPath('a', '一')] }, - 'd': field_updates['d'], + 'd': field_updates[_helpers.FieldPath.from_string('d')], 'e': { - 'f1': field_updates['e.f1'], - 'f2': field_updates['e.f2'], + 'f1': field_updates[_helpers.FieldPath.from_string('e.f1')], + 'f2': field_updates[_helpers.FieldPath.from_string('e.f2')], + 'f3': field_updates[_helpers.FieldPath.from_string('e.f3')] + }, + 'g': field_updates[_helpers.FieldPath.from_string('g')], + 'h': { + 'i': field_updates[_helpers.FieldPath('h', 'i')] }, - 'g': field_updates['g'], + 'j.k': { + 'l.m': field_updates[_helpers.FieldPath('j.k', 'l.m')] + }, + 'a.一': field_updates[_helpers.FieldPath('a.一')] } self.assertEqual(update_values, expected_updates) - self.assertEqual(field_paths, list(field_updates.keys())) + self.assertEqual(field_paths, list(six.iterkeys(field_updates))) def test_parse_with_delete(self): from google.cloud.firestore_v1beta1.constants import DELETE_FIELD @@ -491,11 +577,13 @@ def test_parse_with_delete(self): ('a', 10), ('b', DELETE_FIELD), )) - helper = self._make_one(field_updates) update_values, field_paths = helper.parse() self.assertEqual(update_values, {'a': field_updates['a']}) - self.assertEqual(field_paths, list(field_updates.keys())) + self.assertEqual( + [field_path.parts[0] for field_path in field_paths], + list(field_updates.keys()) + ) def test_parse_with_conflict(self): # "Cheat" and use OrderedDict-s so that iteritems() is deterministic. @@ -511,7 +599,9 @@ def test_parse_with_conflict(self): self.assertEqual(exc_info.exception.args, (err_msg,)) def test_to_field_paths(self): - field_path = 'a.b' + from google.cloud.firestore_v1beta1 import _helpers + + field_path = _helpers.FieldPath.from_string('a.b') field_updates = {field_path: 99} klass = self._get_target_class() @@ -520,6 +610,17 @@ def test_to_field_paths(self): update_values, {'a': {'b': field_updates[field_path]}}) self.assertEqual(field_paths, [field_path]) + def test_conflict_same_field_paths(self): + from google.cloud.firestore_v1beta1 import _helpers + + field_path_from_string = _helpers.FieldPath.from_string('a.b') + field_path_class = _helpers.FieldPath('a', 'b') + # User error in this case + field_updates = {field_path_from_string: '', + field_path_class: ''} + self.assertEqual(field_path_from_string, field_path_class) + self.assertEqual(len(field_updates), 1) + class Test_verify_path(unittest.TestCase): @@ -982,6 +1083,7 @@ def test_many_types(self): ArrayValue) from google.cloud.firestore_v1beta1.proto.document_pb2 import MapValue from google.cloud._helpers import UTC + from google.cloud.firestore_v1beta1._helpers import FieldPath dt_seconds = 1394037350 dt_nanos = 667285000 @@ -1009,6 +1111,8 @@ def test_many_types(self): 'fred': _value_pb(string_value=u'zap'), 'thud': _value_pb(boolean_value=False), })), + FieldPath('a', 'b', 'c').to_api_repr(): + _value_pb(boolean_value=False) } expected = { 'foo': None, @@ -1026,6 +1130,7 @@ def test_many_types(self): 'fred': u'zap', 'thud': False, }, + 'a.b.c': False } self.assertEqual(self._call_fut(value_fields), expected) @@ -1053,6 +1158,13 @@ def _call_fut(field_path): def test_it(self): self.assertEqual(self._call_fut('a.b.c'), ['a', 'b', 'c']) + def test_api_repr(self): + from google.cloud.firestore_v1beta1._helpers import FieldPath + + self.assertEqual( + self._call_fut(FieldPath('a', 'b', 'c').to_api_repr()), + ['a', 'b', 'c']) + class Test_get_nested_value(unittest.TestCase): @@ -1345,8 +1457,11 @@ class Test_canonicalize_field_paths(unittest.TestCase): def test_canonicalize_field_paths(self): from google.cloud.firestore_v1beta1 import _helpers + field_paths = ['0abc.deq', 'abc.654', '321.0deq._321', u'0abc.deq', u'abc.654', u'321.0deq._321'] + field_paths = [ + _helpers.FieldPath.from_string(path) for path in field_paths] convert = _helpers.canonicalize_field_paths(field_paths) self.assertListEqual( convert,