Fix #260: break datastore cycles by moving 'from_protobuf' classmethods

docs/datastore-api.rst

@@ -56,3 +56,9 @@ Transactions
   :members:
   :undoc-members:
   :show-inheritance:
+
+Helper functions
+----------------
+
+.. automodule:: gcloud.datastore.helpers
+  :members:

gcloud/datastore/connection.py

@@ -2,7 +2,7 @@
 
 from gcloud import connection
 from gcloud.datastore import datastore_v1_pb2 as datastore_pb
-from gcloud.datastore import _helpers
+from gcloud.datastore import helpers
 from gcloud.datastore.dataset import Dataset
 
 
@@ -376,7 +376,7 @@ def save_entity(self, dataset_id, key_pb, properties):
             prop.name = name
 
             # Set the appropriate value.
-            _helpers._set_protobuf_value(prop.value, value)
+            helpers._set_protobuf_value(prop.value, value)
 
         # If this is in a transaction, we should just return True. The
         # transaction will handle assigning any keys as necessary.

gcloud/datastore/dataset.py

@@ -1,5 +1,10 @@
 """Create / interact with gcloud datastore datasets."""
 
+from gcloud.datastore import helpers
+from gcloud.datastore.entity import Entity
+from gcloud.datastore.query import Query
+from gcloud.datastore.transaction import Transaction
+
 
 class Dataset(object):
     """A dataset in the Cloud Datastore.
@@ -70,8 +75,6 @@ def query(self, *args, **kwargs):
         :rtype: :class:`gcloud.datastore.query.Query`
         :returns: a new Query instance, bound to this dataset.
         """
-        # This import is here to avoid circular references.
-        from gcloud.datastore.query import Query
         kwargs['dataset'] = self
         return Query(*args, **kwargs)
 
@@ -84,8 +87,6 @@ def entity(self, kind):
         :rtype: :class:`gcloud.datastore.entity.Entity`
         :returns: a new Entity instance, bound to this dataset.
         """
-        # This import is here to avoid circular references.
-        from gcloud.datastore.entity import Entity
         return Entity(dataset=self, kind=kind)
 
     def transaction(self, *args, **kwargs):
@@ -98,8 +99,6 @@ def transaction(self, *args, **kwargs):
         :rtype: :class:`gcloud.datastore.transaction.Transaction`
         :returns: a new Transaction instance, bound to this dataset.
         """
-        # This import is here to avoid circular references.
-        from gcloud.datastore.transaction import Transaction
         kwargs['dataset'] = self
         return Transaction(*args, **kwargs)
 
@@ -125,15 +124,13 @@ def get_entities(self, keys):
         :rtype: list of :class:`gcloud.datastore.entity.Entity`
         :return: The requested entities.
         """
-        # This import is here to avoid circular references.
-        from gcloud.datastore.entity import Entity
-
         entity_pbs = self.connection().lookup(
             dataset_id=self.id(),
             key_pbs=[k.to_protobuf() for k in keys]
         )
 
         entities = []
         for entity_pb in entity_pbs:
-            entities.append(Entity.from_protobuf(entity_pb, dataset=self))
+            entities.append(helpers.entity_from_protobuf(
+                entity_pb, dataset=self))
         return entities

gcloud/datastore/entity.py

@@ -146,32 +146,6 @@ def from_key(cls, key, dataset=None):
 
         return cls(dataset).key(key)
 
-    @classmethod
-    def from_protobuf(cls, pb, dataset=None):
-        """Factory method for creating an entity based on a protobuf.
-
-        The protobuf should be one returned from the Cloud Datastore
-        Protobuf API.
-
-        :type pb: :class:`gcloud.datastore.datastore_v1_pb2.Entity`
-        :param pb: The Protobuf representing the entity.
-
-        :returns: The :class:`Entity` derived from the
-                  :class:`gcloud.datastore.datastore_v1_pb2.Entity`.
-        """
-
-        # This is here to avoid circular imports.
-        from gcloud.datastore import _helpers
-
-        key = Key.from_protobuf(pb.key)
-        entity = cls.from_key(key, dataset)
-
-        for property_pb in pb.property:
-            value = _helpers._get_value_from_property_pb(property_pb)
-            entity[property_pb.name] = value
-
-        return entity
-
     @property
     def _must_key(self):
         """Return our key, or raise NoKey if not set.
@@ -248,9 +222,11 @@ def save(self):
             transaction.add_auto_id_entity(self)
 
         if isinstance(key_pb, datastore_pb.Key):
-            updated_key = Key.from_protobuf(key_pb)
+            path = [
+                {'kind': element.kind, 'id': element.id, 'name': element.name}
+                for element in key_pb.path_element]
             # Update the path (which may have been altered).
-            self._key = key.path(updated_key.path())
+            self._key = key.path(path)
 
         return self
 

gcloud/datastore/_helpers.py → gcloud/datastore/helpers.py

@@ -1,7 +1,9 @@
 """Helper functions for dealing with Cloud Datastore's Protobuf API.
 
-These functions are *not* part of the API.
+The non-private functions are part of the API.
 """
+__all__ = ('entity_from_protobuf', 'key_from_protobuf')
+
 import calendar
 import datetime
 
@@ -14,6 +16,60 @@
 INT_VALUE_CHECKER = Int64ValueChecker()
 
 
+def entity_from_protobuf(pb, dataset=None):
+    """Factory method for creating an entity based on a protobuf.
+
+    The protobuf should be one returned from the Cloud Datastore
+    Protobuf API.
+
+    :type pb: :class:`gcloud.datastore.datastore_v1_pb2.Entity`
+    :param pb: The Protobuf representing the entity.
+
+    :rtype: :class:`gcloud.datastore.entity.Entity`
+    :returns: The entity derived from the protobuf.
+    """
+    key = key_from_protobuf(pb.key)
+    entity = Entity.from_key(key, dataset)
+
+    for property_pb in pb.property:
+        value = _get_value_from_property_pb(property_pb)
+        entity[property_pb.name] = value
+
+    return entity
+
+
+def key_from_protobuf(pb):
+    """Factory method for creating a key based on a protobuf.
+
+    The protobuf should be one returned from the Cloud Datastore
+    Protobuf API.
+
+    :type pb: :class:`gcloud.datastore.datastore_v1_pb2.Key`
+    :param pb: The Protobuf representing the key.
+
+    :rtype: :class:`gcloud.datastore.key.Key`
+    :returns: a new `Key` instance
+    """
+    path = []
+    for element in pb.path_element:
+        element_dict = {'kind': element.kind}
+
+        if element.HasField('id'):
+            element_dict['id'] = element.id
+
+        # This is safe: we expect proto objects returned will only have
+        # one of `name` or `id` set.
+        if element.HasField('name'):
+            element_dict['name'] = element.name
+
+        path.append(element_dict)
+
+    dataset_id = pb.partition_id.dataset_id or None
+    namespace = pb.partition_id.namespace
+
+    return Key(path, namespace, dataset_id)
+
+
 def _get_protobuf_attribute_and_value(val):
     """Given a value, return the protobuf attribute name and proper value.
 
@@ -105,7 +161,7 @@ def _get_value_from_value_pb(value_pb):
         result = naive.replace(tzinfo=pytz.utc)
 
     elif value_pb.HasField('key_value'):
-        result = Key.from_protobuf(value_pb.key_value)
+        result = key_from_protobuf(value_pb.key_value)
 
     elif value_pb.HasField('boolean_value'):
         result = value_pb.boolean_value
@@ -123,7 +179,7 @@ def _get_value_from_value_pb(value_pb):
         result = value_pb.blob_value
 
     elif value_pb.HasField('entity_value'):
-        result = Entity.from_protobuf(value_pb.entity_value)
+        result = entity_from_protobuf(value_pb.entity_value)
 
     elif value_pb.list_value:
         result = [_get_value_from_value_pb(x) for x in value_pb.list_value]

gcloud/datastore/key.py

@@ -42,38 +42,6 @@ def _clone(self):
         """
         return copy.deepcopy(self)
 
-    @classmethod
-    def from_protobuf(cls, pb):
-        """Factory method for creating a key based on a protobuf.
-
-        The protobuf should be one returned from the Cloud Datastore
-        Protobuf API.
-
-        :type pb: :class:`gcloud.datastore.datastore_v1_pb2.Key`
-        :param pb: The Protobuf representing the key.
-
-        :rtype: :class:`gcloud.datastore.key.Key`
-        :returns: a new `Key` instance
-        """
-        path = []
-        for element in pb.path_element:
-            element_dict = {'kind': element.kind}
-
-            if element.HasField('id'):
-                element_dict['id'] = element.id
-
-            # This is safe: we expect proto objects returned will only have
-            # one of `name` or `id` set.
-            if element.HasField('name'):
-                element_dict['name'] = element.name
-
-            path.append(element_dict)
-
-        dataset_id = pb.partition_id.dataset_id or None
-        namespace = pb.partition_id.namespace
-
-        return cls(path, namespace, dataset_id)
-
     def to_protobuf(self):
         """Return a protobuf corresponding to the key.
 

gcloud/datastore/query.py

@@ -3,8 +3,7 @@
 import base64
 
 from gcloud.datastore import datastore_v1_pb2 as datastore_pb
-from gcloud.datastore import _helpers
-from gcloud.datastore.entity import Entity
+from gcloud.datastore import helpers
 from gcloud.datastore.key import Key
 
 
@@ -151,7 +150,7 @@ def filter(self, expression, value):
         property_filter.operator = operator
 
         # Set the value to filter on based on the type.
-        _helpers._set_protobuf_value(property_filter.value, value)
+        helpers._set_protobuf_value(property_filter.value, value)
         return clone
 
     def ancestor(self, ancestor):
@@ -343,7 +342,7 @@ def fetch(self, limit=None):
         entity_pbs, end_cursor = query_results[:2]
 
         self._cursor = end_cursor
-        return [Entity.from_protobuf(entity, dataset=self.dataset())
+        return [helpers.entity_from_protobuf(entity, dataset=self.dataset())
                 for entity in entity_pbs]
 
     def cursor(self):

gcloud/datastore/test_entity.py

@@ -76,48 +76,6 @@ def test_from_key_w_dataset(self):
         self.assertEqual(key.kind(), _KIND)
         self.assertEqual(key.id(), _ID)
 
-    def test_from_protobuf_wo_dataset(self):
-        from gcloud.datastore import datastore_v1_pb2 as datastore_pb
-
-        entity_pb = datastore_pb.Entity()
-        entity_pb.key.partition_id.dataset_id = _DATASET_ID
-        entity_pb.key.path_element.add(kind=_KIND, id=_ID)
-        entity_pb.key.partition_id.dataset_id = _DATASET_ID
-        prop_pb = entity_pb.property.add()
-        prop_pb.name = 'foo'
-        prop_pb.value.string_value = 'Foo'
-        klass = self._getTargetClass()
-        entity = klass.from_protobuf(entity_pb)
-        self.assertTrue(entity.dataset() is None)
-        self.assertEqual(entity.kind(), _KIND)
-        self.assertEqual(entity['foo'], 'Foo')
-        key = entity.key()
-        self.assertEqual(key._dataset_id, _DATASET_ID)
-        self.assertEqual(key.kind(), _KIND)
-        self.assertEqual(key.id(), _ID)
-
-    def test_from_protobuf_w_dataset(self):
-        from gcloud.datastore import datastore_v1_pb2 as datastore_pb
-        from gcloud.datastore.dataset import Dataset
-
-        entity_pb = datastore_pb.Entity()
-        entity_pb.key.partition_id.dataset_id = _DATASET_ID
-        entity_pb.key.path_element.add(kind=_KIND, id=_ID)
-        entity_pb.key.partition_id.dataset_id = _DATASET_ID
-        prop_pb = entity_pb.property.add()
-        prop_pb.name = 'foo'
-        prop_pb.value.string_value = 'Foo'
-        dataset = Dataset(_DATASET_ID)
-        klass = self._getTargetClass()
-        entity = klass.from_protobuf(entity_pb, dataset)
-        self.assertTrue(entity.dataset() is dataset)
-        self.assertEqual(entity.kind(), _KIND)
-        self.assertEqual(entity['foo'], 'Foo')
-        key = entity.key()
-        self.assertEqual(key._dataset_id, _DATASET_ID)
-        self.assertEqual(key.kind(), _KIND)
-        self.assertEqual(key.id(), _ID)
-
     def test__must_key_no_key(self):
         from gcloud.datastore.entity import NoKey
 
@@ -224,7 +182,7 @@ def test_save_w_returned_key(self):
         self.assertEqual(entity['foo'], 'Foo')
         self.assertEqual(connection._saved,
                          (_DATASET_ID, 'KEY', {'foo': 'Foo'}))
-        self.assertEqual(key._path, [{'kind': _KIND, 'id': _ID}])
+        self.assertEqual(key._path, [{'kind': _KIND, 'id': _ID, 'name': ''}])
 
     def test_delete_no_key(self):
         from gcloud.datastore.entity import NoKey