From 6dfe5d4e14a8dddb9f0de0de1708649f5a7b48c3 Mon Sep 17 00:00:00 2001 From: Danny Hermes Date: Wed, 23 Dec 2015 01:06:26 -0800 Subject: [PATCH 1/3] Moving _get_pb_property_value from bigtable into core. Doing this so that we can factor out `_has_field` and use it across packages. This is because `HasField()` works differently in `proto3` and we use this behavior from time to time. --- gcloud/_helpers.py | 43 +++++++++++++++++++++ gcloud/bigtable/cluster.py | 25 +------------ gcloud/bigtable/test_cluster.py | 22 ----------- gcloud/test__helpers.py | 66 +++++++++++++++++++++++++++++++++ 4 files changed, 110 insertions(+), 46 deletions(-) diff --git a/gcloud/_helpers.py b/gcloud/_helpers.py index 010981568e29..c73c39f9b678 100644 --- a/gcloud/_helpers.py +++ b/gcloud/_helpers.py @@ -388,6 +388,49 @@ def _datetime_to_pb_timestamp(when): return timestamp_pb2.Timestamp(seconds=seconds, nanos=nanos) +def _has_field(message_pb, property_name): + """Determine if a field is set on a protobuf. + + :type message_pb: :class:`google.protobuf.message.Message` + :param message_pb: The message to check for ``property_name``. + + :type property_name: str + :param property_name: The property value to check against. + + :rtype: bool + :returns: Flag indicating if ``property_name`` is set on ``message_pb``. + """ + # NOTE: As of proto3, HasField() only works for message fields, not for + # singular (non-message) fields. First try to use HasField and + # if it fails (with a ValueError) we manually consult the fields. + try: + return message_pb.HasField(property_name) + except ValueError: + all_fields = set([field.name for field in message_pb._fields]) + return property_name in all_fields + + +def _get_pb_property_value(message_pb, property_name): + """Return a message field value. + + :type message_pb: :class:`google.protobuf.message.Message` + :param message_pb: The message to check for ``property_name``. + + :type property_name: str + :param property_name: The property value to check against. + + :rtype: object + :returns: The value of ``property_name`` set on ``message_pb``. + :raises: :class:`ValueError ` if the result returned + from the ``message_pb`` does not contain the ``property_name`` + value. + """ + if _has_field(message_pb, property_name): + return getattr(message_pb, property_name) + else: + raise ValueError('Message does not contain %s.' % (property_name,)) + + try: from pytz import UTC # pylint: disable=unused-import,wrong-import-position except ImportError: diff --git a/gcloud/bigtable/cluster.py b/gcloud/bigtable/cluster.py index efedc11e7110..e5344635947c 100644 --- a/gcloud/bigtable/cluster.py +++ b/gcloud/bigtable/cluster.py @@ -20,6 +20,7 @@ from google.longrunning import operations_pb2 from gcloud._helpers import _pb_timestamp_to_datetime +from gcloud._helpers import _get_pb_property_value from gcloud.bigtable._generated import bigtable_cluster_data_pb2 as data_pb2 from gcloud.bigtable._generated import ( bigtable_cluster_service_messages_pb2 as messages_pb2) @@ -47,30 +48,6 @@ } -def _get_pb_property_value(message_pb, property_name): - """Return a message field value. - - :type message_pb: :class:`google.protobuf.message.Message` - :param message_pb: The message to check for ``property_name``. - - :type property_name: str - :param property_name: The property value to check against. - - :rtype: object - :returns: The value of ``property_name`` set on ``message_pb``. - :raises: :class:`ValueError ` if the result returned - from the ``message_pb`` does not contain the ``property_name`` - value. - """ - # Make sure `property_name` is set on the response. - # NOTE: As of proto3, HasField() only works for message fields, not for - # singular (non-message) fields. - all_fields = set([field.name for field in message_pb._fields]) - if property_name not in all_fields: - raise ValueError('Message does not contain %s.' % (property_name,)) - return getattr(message_pb, property_name) - - def _prepare_create_request(cluster): """Creates a protobuf request for a CreateCluster request. diff --git a/gcloud/bigtable/test_cluster.py b/gcloud/bigtable/test_cluster.py index 8ff2ffa68bb8..fde71dca8c5d 100644 --- a/gcloud/bigtable/test_cluster.py +++ b/gcloud/bigtable/test_cluster.py @@ -631,28 +631,6 @@ def test_list_tables_failure_name_bad_before(self): self._list_tables_helper(table_id, table_name=bad_table_name) -class Test__get_pb_property_value(unittest2.TestCase): - - def _callFUT(self, message_pb, property_name): - from gcloud.bigtable.cluster import _get_pb_property_value - return _get_pb_property_value(message_pb, property_name) - - def test_it(self): - from gcloud.bigtable._generated import ( - bigtable_cluster_data_pb2 as data_pb2) - serve_nodes = 119 - cluster_pb = data_pb2.Cluster(serve_nodes=serve_nodes) - result = self._callFUT(cluster_pb, 'serve_nodes') - self.assertEqual(result, serve_nodes) - - def test_with_value_unset_on_pb(self): - from gcloud.bigtable._generated import ( - bigtable_cluster_data_pb2 as data_pb2) - cluster_pb = data_pb2.Cluster() - with self.assertRaises(ValueError): - self._callFUT(cluster_pb, 'serve_nodes') - - class Test__prepare_create_request(unittest2.TestCase): def _callFUT(self, cluster): diff --git a/gcloud/test__helpers.py b/gcloud/test__helpers.py index 5b6f329d2cb8..0a3b4ace0dec 100644 --- a/gcloud/test__helpers.py +++ b/gcloud/test__helpers.py @@ -526,6 +526,72 @@ def test_it(self): self.assertEqual(self._callFUT(dt_stamp), timestamp) +class Test__has_field(unittest2.TestCase): + + def _callFUT(self, message_pb, property_name): + from gcloud._helpers import _has_field + return _has_field(message_pb, property_name) + + def test_unset_simple_field(self): + from gcloud.bigtable._generated import ( + bigtable_cluster_data_pb2 as data_pb2) + cluster_pb = data_pb2.Cluster() + self.assertFalse(self._callFUT(cluster_pb, 'serve_nodes')) + + def test_set_simple_field(self): + from gcloud.bigtable._generated import ( + bigtable_cluster_data_pb2 as data_pb2) + serve_nodes = 119 + cluster_pb = data_pb2.Cluster(serve_nodes=serve_nodes) + self.assertTrue(self._callFUT(cluster_pb, 'serve_nodes')) + + def test_unset_message_field(self): + from gcloud.bigtable._generated import ( + bigtable_cluster_data_pb2 as data_pb2) + from gcloud.bigtable._generated.timestamp_pb2 import Timestamp + cluster_pb = data_pb2.Cluster() + # Check that no fields are attached to the protobuf message. + self.assertEqual(len(cluster_pb._fields), 0) + self.assertEqual(cluster_pb.delete_time, Timestamp()) + # Check that the field is now attached to the protobuf message, + # even though the value is unset. + self.assertEqual([field.name for field in cluster_pb._fields.keys()], + ['delete_time']) + # Make sure has field is still False. + self.assertFalse(self._callFUT(cluster_pb, 'delete_time')) + + def test_set_message_field(self): + from gcloud.bigtable._generated import ( + bigtable_cluster_data_pb2 as data_pb2) + from gcloud.bigtable._generated.timestamp_pb2 import Timestamp + + timestamp = Timestamp() + cluster_pb = data_pb2.Cluster(delete_time=timestamp) + self.assertTrue(self._callFUT(cluster_pb, 'delete_time')) + + +class Test__get_pb_property_value(unittest2.TestCase): + + def _callFUT(self, message_pb, property_name): + from gcloud._helpers import _get_pb_property_value + return _get_pb_property_value(message_pb, property_name) + + def test_it(self): + from gcloud.bigtable._generated import ( + bigtable_cluster_data_pb2 as data_pb2) + serve_nodes = 119 + cluster_pb = data_pb2.Cluster(serve_nodes=serve_nodes) + result = self._callFUT(cluster_pb, 'serve_nodes') + self.assertEqual(result, serve_nodes) + + def test_with_value_unset_on_pb(self): + from gcloud.bigtable._generated import ( + bigtable_cluster_data_pb2 as data_pb2) + cluster_pb = data_pb2.Cluster() + with self.assertRaises(ValueError): + self._callFUT(cluster_pb, 'serve_nodes') + + class _AppIdentity(object): def __init__(self, app_id): From 896c86ffcb6cf28c538945b43644e0cb198b7e85 Mon Sep 17 00:00:00 2001 From: Danny Hermes Date: Wed, 23 Dec 2015 01:06:37 -0800 Subject: [PATCH 2/3] Replacing uses of HasField with _has_field in datastore. --- gcloud/datastore/connection.py | 3 ++- gcloud/datastore/helpers.py | 31 +++++++++++++++-------------- gcloud/datastore/test_connection.py | 3 ++- gcloud/datastore/test_helpers.py | 11 +++++++--- gcloud/datastore/test_key.py | 12 +++++++---- 5 files changed, 36 insertions(+), 24 deletions(-) diff --git a/gcloud/datastore/connection.py b/gcloud/datastore/connection.py index 5267cb45b5b9..acbaa7f67007 100644 --- a/gcloud/datastore/connection.py +++ b/gcloud/datastore/connection.py @@ -16,6 +16,7 @@ import os +from gcloud._helpers import _has_field from gcloud import connection from gcloud.environment_vars import GCD_HOST from gcloud.exceptions import make_exception @@ -401,7 +402,7 @@ def _prepare_key_for_request(key_pb): # pragma: NO COVER copied from helpers :returns: A key which will be added to a request. It will be the original if nothing needs to be changed. """ - if key_pb.partition_id.HasField('dataset_id'): + if _has_field(key_pb.partition_id, 'dataset_id'): new_key_pb = _entity_pb2.Key() new_key_pb.CopyFrom(key_pb) new_key_pb.partition_id.ClearField('dataset_id') diff --git a/gcloud/datastore/helpers.py b/gcloud/datastore/helpers.py index fd13ceb901d7..10347717ed0a 100644 --- a/gcloud/datastore/helpers.py +++ b/gcloud/datastore/helpers.py @@ -23,6 +23,7 @@ import six from gcloud._helpers import _datetime_from_microseconds +from gcloud._helpers import _has_field from gcloud._helpers import _microseconds_from_datetime from gcloud.datastore._generated import entity_pb2 as _entity_pb2 from gcloud.datastore.entity import Entity @@ -109,7 +110,7 @@ def _get_meaning(value_pb, is_list=False): if all_meanings: raise ValueError('Different meanings set on values ' 'within a list_value') - elif value_pb.HasField('meaning'): + elif _has_field(value_pb, 'meaning'): meaning = value_pb.meaning return meaning @@ -159,7 +160,7 @@ def entity_from_protobuf(pb): :returns: The entity derived from the protobuf. """ key = None - if pb.HasField('key'): + if _has_field(pb, 'key'): key = key_from_protobuf(pb.key) entity_props = {} @@ -259,18 +260,18 @@ def key_from_protobuf(pb): path_args = [] for element in pb.path_element: path_args.append(element.kind) - if element.HasField('id'): + if _has_field(element, 'id'): path_args.append(element.id) # This is safe: we expect proto objects returned will only have # one of `name` or `id` set. - if element.HasField('name'): + if _has_field(element, 'name'): path_args.append(element.name) project = None - if pb.partition_id.HasField('dataset_id'): + if _has_field(pb.partition_id, 'dataset_id'): project = pb.partition_id.dataset_id namespace = None - if pb.partition_id.HasField('namespace'): + if _has_field(pb.partition_id, 'namespace'): namespace = pb.partition_id.namespace return Key(*path_args, namespace=namespace, project=project) @@ -350,29 +351,29 @@ def _get_value_from_value_pb(value_pb): :returns: The value provided by the Protobuf. """ result = None - if value_pb.HasField('timestamp_microseconds_value'): + if _has_field(value_pb, 'timestamp_microseconds_value'): microseconds = value_pb.timestamp_microseconds_value result = _datetime_from_microseconds(microseconds) - elif value_pb.HasField('key_value'): + elif _has_field(value_pb, 'key_value'): result = key_from_protobuf(value_pb.key_value) - elif value_pb.HasField('boolean_value'): + elif _has_field(value_pb, 'boolean_value'): result = value_pb.boolean_value - elif value_pb.HasField('double_value'): + elif _has_field(value_pb, 'double_value'): result = value_pb.double_value - elif value_pb.HasField('integer_value'): + elif _has_field(value_pb, 'integer_value'): result = value_pb.integer_value - elif value_pb.HasField('string_value'): + elif _has_field(value_pb, 'string_value'): result = value_pb.string_value - elif value_pb.HasField('blob_value'): + elif _has_field(value_pb, 'blob_value'): result = value_pb.blob_value - elif value_pb.HasField('entity_value'): + elif _has_field(value_pb, 'entity_value'): result = entity_from_protobuf(value_pb.entity_value) elif value_pb.list_value: @@ -428,7 +429,7 @@ def _prepare_key_for_request(key_pb): :returns: A key which will be added to a request. It will be the original if nothing needs to be changed. """ - if key_pb.partition_id.HasField('dataset_id'): + if _has_field(key_pb.partition_id, 'dataset_id'): # We remove the dataset_id from the protobuf. This is because # the backend fails a request if the key contains un-prefixed # project. The backend fails because requests to diff --git a/gcloud/datastore/test_connection.py b/gcloud/datastore/test_connection.py index fba3644b875d..2df5c34d3f43 100644 --- a/gcloud/datastore/test_connection.py +++ b/gcloud/datastore/test_connection.py @@ -900,7 +900,8 @@ def request(self, **kw): def _compare_key_pb_after_request(test, key_before, key_after): - test.assertFalse(key_after.partition_id.HasField('dataset_id')) + from gcloud._helpers import _has_field + test.assertFalse(_has_field(key_after.partition_id, 'dataset_id')) test.assertEqual(key_before.partition_id.namespace, key_after.partition_id.namespace) test.assertEqual(len(key_before.path_element), diff --git a/gcloud/datastore/test_helpers.py b/gcloud/datastore/test_helpers.py index 0efe8b4b7f8d..611cbc1ca2eb 100644 --- a/gcloud/datastore/test_helpers.py +++ b/gcloud/datastore/test_helpers.py @@ -198,6 +198,7 @@ def _callFUT(self, entity): return entity_to_protobuf(entity) def _compareEntityProto(self, entity_pb1, entity_pb2): + from gcloud._helpers import _has_field from gcloud.datastore.helpers import _property_tuples self.assertEqual(entity_pb1.key, entity_pb2.key) @@ -208,7 +209,7 @@ def _compareEntityProto(self, entity_pb1, entity_pb2): name1, val1 = pair1 name2, val2 = pair2 self.assertEqual(name1, name2) - if val1.HasField('entity_value'): + if _has_field(val1, 'entity_value'): self.assertEqual(val1.meaning, val2.meaning) self._compareEntityProto(val1.entity_value, val2.entity_value) @@ -767,6 +768,8 @@ def test_prefixed(self): self.assertEqual(PREFIXED, result) def test_unprefixed_bogus_key_miss(self): + from gcloud._helpers import _has_field + UNPREFIXED = 'PROJECT' PREFIX = 's~' CONNECTION = _Connection(PREFIX, from_missing=False) @@ -782,12 +785,14 @@ def test_unprefixed_bogus_key_miss(self): self.assertEqual(len(path_element), 1) self.assertEqual(path_element[0].kind, '__MissingLookupKind') self.assertEqual(path_element[0].id, 1) - self.assertFalse(path_element[0].HasField('name')) + self.assertFalse(_has_field(path_element[0], 'name')) PREFIXED = PREFIX + UNPREFIXED self.assertEqual(result, PREFIXED) def test_unprefixed_bogus_key_hit(self): + from gcloud._helpers import _has_field + UNPREFIXED = 'PROJECT' PREFIX = 'e~' CONNECTION = _Connection(PREFIX, from_missing=True) @@ -802,7 +807,7 @@ def test_unprefixed_bogus_key_hit(self): self.assertEqual(len(path_element), 1) self.assertEqual(path_element[0].kind, '__MissingLookupKind') self.assertEqual(path_element[0].id, 1) - self.assertFalse(path_element[0].HasField('name')) + self.assertFalse(_has_field(path_element[0], 'name')) PREFIXED = PREFIX + UNPREFIXED self.assertEqual(result, PREFIXED) diff --git a/gcloud/datastore/test_key.py b/gcloud/datastore/test_key.py index 966d849679e2..c4a03cf186bf 100644 --- a/gcloud/datastore/test_key.py +++ b/gcloud/datastore/test_key.py @@ -333,7 +333,9 @@ def test_completed_key_on_complete(self): self.assertRaises(ValueError, key.completed_key, 5678) def test_to_protobuf_defaults(self): + from gcloud._helpers import _has_field from gcloud.datastore._generated import entity_pb2 + _KIND = 'KIND' key = self._makeOne(_KIND, project=self._DEFAULT_PROJECT) pb = key.to_protobuf() @@ -342,15 +344,15 @@ def test_to_protobuf_defaults(self): # Check partition ID. self.assertEqual(pb.partition_id.dataset_id, self._DEFAULT_PROJECT) self.assertEqual(pb.partition_id.namespace, '') - self.assertFalse(pb.partition_id.HasField('namespace')) + self.assertFalse(_has_field(pb.partition_id, 'namespace')) # Check the element PB matches the partial key and kind. elem, = list(pb.path_element) self.assertEqual(elem.kind, _KIND) self.assertEqual(elem.name, '') - self.assertFalse(elem.HasField('name')) + self.assertFalse(_has_field(elem, 'name')) self.assertEqual(elem.id, 0) - self.assertFalse(elem.HasField('id')) + self.assertFalse(_has_field(elem, 'id')) def test_to_protobuf_w_explicit_project(self): _PROJECT = 'PROJECT-ALT' @@ -381,12 +383,14 @@ def test_to_protobuf_w_explicit_path(self): self.assertEqual(elems[1].id, _ID) def test_to_protobuf_w_no_kind(self): + from gcloud._helpers import _has_field + key = self._makeOne('KIND', project=self._DEFAULT_PROJECT) # Force the 'kind' to be unset. Maybe `to_protobuf` should fail # on this? The backend certainly will. key._path[-1].pop('kind') pb = key.to_protobuf() - self.assertFalse(pb.path_element[0].HasField('kind')) + self.assertFalse(_has_field(pb.path_element[0], 'kind')) def test_is_partial_no_name_or_id(self): key = self._makeOne('KIND', project=self._DEFAULT_PROJECT) From 9455bf2289899824e7da451c986d75aaa7c44ff4 Mon Sep 17 00:00:00 2001 From: Danny Hermes Date: Thu, 14 Jan 2016 21:08:07 -0800 Subject: [PATCH 3/3] Fixing import error of a no longer existing generated pb2. Caused by merging #1353 before #1329. --- gcloud/test__helpers.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/gcloud/test__helpers.py b/gcloud/test__helpers.py index 0a3b4ace0dec..88ee82b5c4a8 100644 --- a/gcloud/test__helpers.py +++ b/gcloud/test__helpers.py @@ -546,9 +546,9 @@ def test_set_simple_field(self): self.assertTrue(self._callFUT(cluster_pb, 'serve_nodes')) def test_unset_message_field(self): + from google.protobuf.timestamp_pb2 import Timestamp from gcloud.bigtable._generated import ( bigtable_cluster_data_pb2 as data_pb2) - from gcloud.bigtable._generated.timestamp_pb2 import Timestamp cluster_pb = data_pb2.Cluster() # Check that no fields are attached to the protobuf message. self.assertEqual(len(cluster_pb._fields), 0) @@ -561,9 +561,9 @@ def test_unset_message_field(self): self.assertFalse(self._callFUT(cluster_pb, 'delete_time')) def test_set_message_field(self): + from google.protobuf.timestamp_pb2 import Timestamp from gcloud.bigtable._generated import ( bigtable_cluster_data_pb2 as data_pb2) - from gcloud.bigtable._generated.timestamp_pb2 import Timestamp timestamp = Timestamp() cluster_pb = data_pb2.Cluster(delete_time=timestamp)