Merge pull request #1450 from dhermes/fix-1402

Dropping _has_field.

Author: dhermes

gcloud/_helpers.py

@@ -388,49 +388,6 @@ 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 <exceptions.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:

gcloud/bigtable/cluster.py

@@ -20,7 +20,6 @@
 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)
@@ -240,8 +239,12 @@ def table(self, table_id):
         return Table(table_id, self)
 
     def _update_from_pb(self, cluster_pb):
-        self.display_name = _get_pb_property_value(cluster_pb, 'display_name')
-        self.serve_nodes = _get_pb_property_value(cluster_pb, 'serve_nodes')
+        if not cluster_pb.display_name:  # Simple field (string)
+            raise ValueError('Cluster protobuf does not contain display_name')
+        if not cluster_pb.serve_nodes:  # Simple field (int32)
+            raise ValueError('Cluster protobuf does not contain serve_nodes')
+        self.display_name = cluster_pb.display_name
+        self.serve_nodes = cluster_pb.serve_nodes
 
     @classmethod
     def from_pb(cls, cluster_pb, client):

gcloud/bigtable/test_cluster.py

@@ -207,6 +207,53 @@ def test_table_factory(self):
         self.assertEqual(table.table_id, table_id)
         self.assertEqual(table._cluster, cluster)
 
+    def test__update_from_pb_success(self):
+        from gcloud.bigtable._generated import (
+            bigtable_cluster_data_pb2 as data_pb2)
+        from gcloud.bigtable.cluster import _DEFAULT_SERVE_NODES
+
+        display_name = 'display_name'
+        serve_nodes = 8
+        cluster_pb = data_pb2.Cluster(
+            display_name=display_name,
+            serve_nodes=serve_nodes,
+        )
+
+        cluster = self._makeOne(None, None, None)
+        self.assertEqual(cluster.display_name, None)
+        self.assertEqual(cluster.serve_nodes, _DEFAULT_SERVE_NODES)
+        cluster._update_from_pb(cluster_pb)
+        self.assertEqual(cluster.display_name, display_name)
+        self.assertEqual(cluster.serve_nodes, serve_nodes)
+
+    def test__update_from_pb_no_display_name(self):
+        from gcloud.bigtable._generated import (
+            bigtable_cluster_data_pb2 as data_pb2)
+        from gcloud.bigtable.cluster import _DEFAULT_SERVE_NODES
+
+        cluster_pb = data_pb2.Cluster(serve_nodes=331)
+        cluster = self._makeOne(None, None, None)
+        self.assertEqual(cluster.display_name, None)
+        self.assertEqual(cluster.serve_nodes, _DEFAULT_SERVE_NODES)
+        with self.assertRaises(ValueError):
+            cluster._update_from_pb(cluster_pb)
+        self.assertEqual(cluster.display_name, None)
+        self.assertEqual(cluster.serve_nodes, _DEFAULT_SERVE_NODES)
+
+    def test__update_from_pb_no_serve_nodes(self):
+        from gcloud.bigtable._generated import (
+            bigtable_cluster_data_pb2 as data_pb2)
+        from gcloud.bigtable.cluster import _DEFAULT_SERVE_NODES
+
+        cluster_pb = data_pb2.Cluster(display_name='name')
+        cluster = self._makeOne(None, None, None)
+        self.assertEqual(cluster.display_name, None)
+        self.assertEqual(cluster.serve_nodes, _DEFAULT_SERVE_NODES)
+        with self.assertRaises(ValueError):
+            cluster._update_from_pb(cluster_pb)
+        self.assertEqual(cluster.display_name, None)
+        self.assertEqual(cluster.serve_nodes, _DEFAULT_SERVE_NODES)
+
     def test_from_pb_success(self):
         from gcloud.bigtable._generated import (
             bigtable_cluster_data_pb2 as data_pb2)

gcloud/datastore/connection.py

@@ -16,7 +16,6 @@
 
 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
@@ -402,7 +401,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 _has_field(key_pb.partition_id, 'dataset_id'):
+    if key_pb.partition_id.dataset_id:  # Simple field (string)
         new_key_pb = _entity_pb2.Key()
         new_key_pb.CopyFrom(key_pb)
         new_key_pb.partition_id.ClearField('dataset_id')

gcloud/datastore/helpers.py

@@ -23,7 +23,6 @@
 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
@@ -110,7 +109,7 @@ def _get_meaning(value_pb, is_list=False):
         if all_meanings:
             raise ValueError('Different meanings set on values '
                              'within a list_value')
-    elif _has_field(value_pb, 'meaning'):
+    elif value_pb.meaning:  # Simple field (int32)
         meaning = value_pb.meaning
 
     return meaning
@@ -160,7 +159,7 @@ def entity_from_protobuf(pb):
     :returns: The entity derived from the protobuf.
     """
     key = None
-    if _has_field(pb, 'key'):
+    if pb.HasField('key'):  # Message field (Key)
         key = key_from_protobuf(pb.key)
 
     entity_props = {}
@@ -260,18 +259,18 @@ def key_from_protobuf(pb):
     path_args = []
     for element in pb.path_element:
         path_args.append(element.kind)
-        if _has_field(element, 'id'):
+        if element.id:  # Simple field (int64)
             path_args.append(element.id)
         # This is safe: we expect proto objects returned will only have
         # one of `name` or `id` set.
-        if _has_field(element, 'name'):
+        if element.name:  # Simple field (string)
             path_args.append(element.name)
 
     project = None
-    if _has_field(pb.partition_id, 'dataset_id'):
+    if pb.partition_id.dataset_id:  # Simple field (string)
         project = pb.partition_id.dataset_id
     namespace = None
-    if _has_field(pb.partition_id, 'namespace'):
+    if pb.partition_id.namespace:  # Simple field (string)
         namespace = pb.partition_id.namespace
 
     return Key(*path_args, namespace=namespace, project=project)
@@ -351,29 +350,30 @@ def _get_value_from_value_pb(value_pb):
     :returns: The value provided by the Protobuf.
     """
     result = None
-    if _has_field(value_pb, 'timestamp_microseconds_value'):
+    # Simple field (int64)
+    if value_pb.HasField('timestamp_microseconds_value'):
         microseconds = value_pb.timestamp_microseconds_value
         result = _datetime_from_microseconds(microseconds)
 
-    elif _has_field(value_pb, 'key_value'):
+    elif value_pb.HasField('key_value'):  # Message field (Key)
         result = key_from_protobuf(value_pb.key_value)
 
-    elif _has_field(value_pb, 'boolean_value'):
+    elif value_pb.HasField('boolean_value'):  # Simple field (bool)
         result = value_pb.boolean_value
 
-    elif _has_field(value_pb, 'double_value'):
+    elif value_pb.HasField('double_value'):  # Simple field (double)
         result = value_pb.double_value
 
-    elif _has_field(value_pb, 'integer_value'):
+    elif value_pb.HasField('integer_value'):  # Simple field (int64)
         result = value_pb.integer_value
 
-    elif _has_field(value_pb, 'string_value'):
+    elif value_pb.HasField('string_value'):  # Simple field (string)
         result = value_pb.string_value
 
-    elif _has_field(value_pb, 'blob_value'):
+    elif value_pb.HasField('blob_value'):  # Simple field (bytes)
         result = value_pb.blob_value
 
-    elif _has_field(value_pb, 'entity_value'):
+    elif value_pb.HasField('entity_value'):  # Message field (Entity)
         result = entity_from_protobuf(value_pb.entity_value)
 
     elif value_pb.list_value:
@@ -429,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 _has_field(key_pb.partition_id, 'dataset_id'):
+    if key_pb.partition_id.dataset_id:  # Simple field (string)
         # 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

gcloud/datastore/test_connection.py

@@ -900,8 +900,8 @@ def request(self, **kw):
 
 
 def _compare_key_pb_after_request(test, key_before, key_after):
-    from gcloud._helpers import _has_field
-    test.assertFalse(_has_field(key_after.partition_id, 'dataset_id'))
+    # Unset values are False-y.
+    test.assertEqual(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),

gcloud/datastore/test_helpers.py

@@ -198,7 +198,6 @@ 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)
@@ -209,7 +208,7 @@ def _compareEntityProto(self, entity_pb1, entity_pb2):
             name1, val1 = pair1
             name2, val2 = pair2
             self.assertEqual(name1, name2)
-            if _has_field(val1, 'entity_value'):
+            if val1.HasField('entity_value'):  # Message field (Entity)
                 self.assertEqual(val1.meaning, val2.meaning)
                 self._compareEntityProto(val1.entity_value,
                                          val2.entity_value)
@@ -768,8 +767,6 @@ 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)
@@ -785,14 +782,13 @@ 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(_has_field(path_element[0], 'name'))
+        # Unset values are False-y.
+        self.assertEqual(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)
@@ -807,7 +803,8 @@ 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(_has_field(path_element[0], 'name'))
+        # Unset values are False-y.
+        self.assertEqual(path_element[0].name, '')
 
         PREFIXED = PREFIX + UNPREFIXED
         self.assertEqual(result, PREFIXED)

gcloud/datastore/test_key.py

@@ -333,7 +333,6 @@ 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'
@@ -343,16 +342,16 @@ def test_to_protobuf_defaults(self):
 
         # Check partition ID.
         self.assertEqual(pb.partition_id.dataset_id, self._DEFAULT_PROJECT)
+        # Unset values are False-y.
         self.assertEqual(pb.partition_id.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)
+        # Unset values are False-y.
         self.assertEqual(elem.name, '')
-        self.assertFalse(_has_field(elem, 'name'))
+        # Unset values are False-y.
         self.assertEqual(elem.id, 0)
-        self.assertFalse(_has_field(elem, 'id'))
 
     def test_to_protobuf_w_explicit_project(self):
         _PROJECT = 'PROJECT-ALT'
@@ -383,14 +382,13 @@ 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(_has_field(pb.path_element[0], 'kind'))
+        # Unset values are False-y.
+        self.assertEqual(pb.path_element[0].kind, '')
 
     def test_is_partial_no_name_or_id(self):
         key = self._makeOne('KIND', project=self._DEFAULT_PROJECT)