From 984eed53cc5317272bb83c6f6c5d48230bc91016 Mon Sep 17 00:00:00 2001 From: Alan Wu Date: Fri, 16 Mar 2018 14:34:43 -0700 Subject: [PATCH] Review Changes --- .../cloud/firestore_v1beta1/_helpers.py | 16 +- firestore/tests/unit/test__helpers.py | 192 ++++++++++++------ 2 files changed, 142 insertions(+), 66 deletions(-) diff --git a/firestore/google/cloud/firestore_v1beta1/_helpers.py b/firestore/google/cloud/firestore_v1beta1/_helpers.py index e973d7acbb1a5..2f872ad448a0c 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]): @@ -362,8 +366,6 @@ def parse(self): * The list of field paths to send (for updates and deletes). """ for key, value in six.iteritems(self.field_updates): - if isinstance(key, FieldPath): - key = key.to_api_repr() self.add_value_at_field_path(key, value) return self.update_values, self.field_paths @@ -914,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 8308dd5d9f9ed..aad9043ea4c88 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'] @@ -295,11 +315,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 +338,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 +366,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 +419,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 +477,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 +493,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 +511,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,39 +521,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): - from google.cloud.firestore_v1beta1._helpers import FieldPath + 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), - (FieldPath.from_string('e.f3').to_api_repr(), (3, 1)), - ('g', {'key': True}), - (FieldPath('h', 'i').to_api_repr(), '3'), + (_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'], - 'f3': field_updates['e.f3'] + '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['g'], + 'g': field_updates[_helpers.FieldPath.from_string('g')], 'h': { - 'i': field_updates['h.i'] + 'i': field_updates[_helpers.FieldPath('h', 'i')] }, + '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 @@ -498,11 +572,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. @@ -518,7 +594,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() @@ -527,17 +605,9 @@ def test_to_field_paths(self): update_values, {'a': {'b': field_updates[field_path]}}) self.assertEqual(field_paths, [field_path]) - def test_conflict_same_paths(self): - from google.cloud.firestore_v1beta1 import _helpers - field_path_string = 'a.b' - field_path_class = _helpers.FieldPath('a', 'b') - field_updates = {field_path_string: '', - field_path_class: ''} - with self.assertRaises(ValueError): - self._get_target_class().to_field_paths(field_updates) - 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 @@ -1085,6 +1155,7 @@ def test_it(self): 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']) @@ -1381,8 +1452,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,