Merge pull request #253 from tseaver/174-verify_entity_repr

Fix #174: verify Entity.__repr__ logic

gcloud/datastore/entity.py

@@ -19,6 +19,10 @@
 from gcloud.datastore.key import Key
 
 
+class NoKey(RuntimeError):
+    """Exception raised by Entity methods which require a key."""
+
+
 class Entity(dict):  # pylint: disable=too-many-public-methods
     """:type dataset: :class:`gcloud.datastore.dataset.Dataset`
     :param dataset: The dataset in which this entity belongs.
@@ -82,8 +86,8 @@ def dataset(self):
           be `None`. It also means that if you change the key on the entity,
           this will refer to that key's dataset.
         """
-        if self.key():
-            return self.key().dataset()
+        if self._key:
+            return self._key.dataset()
 
     def key(self, key=None):
         """Get or set the :class:`.datastore.key.Key` on the current entity.
@@ -117,8 +121,8 @@ def kind(self):
           which knows its Kind.
         """
 
-        if self.key():
-            return self.key().kind()
+        if self._key:
+            return self._key.kind()
 
     @classmethod
     def from_key(cls, key):
@@ -162,6 +166,18 @@ def from_protobuf(cls, pb, dataset=None):  # pylint: disable=invalid-name
 
         return entity
 
+    @property
+    def _must_key(self):
+        """Return our key, or raise NoKey if not set.
+
+        :rtype: :class:`gcloud.datastore.key.Key`.
+        :returns: our key
+        :raises: NoKey if key is None
+        """
+        if self._key is None:
+            raise NoKey('no key')
+        return self._key
+
     def reload(self):
         """Reloads the contents of this entity from the datastore.
 
@@ -174,11 +190,8 @@ def reload(self):
           exists remotely, however it will *not* override any properties that
           exist only locally.
         """
-
-        # Note that you must have a valid key, otherwise this makes no sense.
-        # pylint: disable=maybe-no-member
-        entity = self.dataset().get_entity(self.key().to_protobuf())
-        # pylint: enable=maybe-no-member
+        key = self._must_key
+        entity = key.dataset().get_entity(key.to_protobuf())
 
         if entity:
             self.update(entity)
@@ -196,27 +209,24 @@ def save(self):
         :rtype: :class:`gcloud.datastore.entity.Entity`
         :returns: The entity with a possibly updated Key.
         """
-        # pylint: disable=maybe-no-member
-        key_pb = self.dataset().connection().save_entity(
-            dataset_id=self.dataset().id(),
-            key_pb=self.key().to_protobuf(), properties=dict(self))
-        # pylint: enable=maybe-no-member
+        key = self._must_key
+        dataset = key.dataset()
+        connection = dataset.connection()
+        key_pb = connection.save_entity(
+            dataset_id=dataset.id(),
+            key_pb=key.to_protobuf(),
+            properties=dict(self))
 
         # If we are in a transaction and the current entity needs an
         # automatically assigned ID, tell the transaction where to put that.
-        transaction = self.dataset().connection().transaction()
-        # pylint: disable=maybe-no-member
-        if transaction and self.key().is_partial():
+        transaction = connection.transaction()
+        if transaction and key.is_partial():
             transaction.add_auto_id_entity(self)
-        # pylint: enable=maybe-no-member
 
         if isinstance(key_pb, datastore_pb.Key):
             updated_key = Key.from_protobuf(key_pb)
             # Update the path (which may have been altered).
-            # pylint: disable=maybe-no-member
-            key = self.key().path(updated_key.path())
-            # pylint: enable=maybe-no-member
-            self.key(key)
+            self._key = key.path(updated_key.path())
 
         return self
 
@@ -228,20 +238,14 @@ def delete(self):
           set on the entity. Whatever is stored remotely using the key on the
           entity will be deleted.
         """
-        # NOTE: pylint thinks key() is an Entity, hence key().to_protobuf()
-        #       is not defined. This is because one branch of the return
-        #       in the key() definition returns self.
-        # pylint: disable=maybe-no-member
-        self.dataset().connection().delete_entity(
-            dataset_id=self.dataset().id(), key_pb=self.key().to_protobuf())
-        # pylint: enable=maybe-no-member
+        key = self._must_key
+        dataset = key.dataset()
+        dataset.connection().delete_entity(
+            dataset_id=dataset.id(), key_pb=key.to_protobuf())
 
     def __repr__(self):
-        # An entity should have a key all the time (even if it's partial).
-        if self.key():
-            # pylint: disable=maybe-no-member
-            return '<Entity%s %s>' % (self.key().path(),
+        if self._key:
+            return '<Entity%s %s>' % (self._key.path(),
                                       super(Entity, self).__repr__())
-            # pylint: enable=maybe-no-member
         else:
             return '<Entity %s>' % (super(Entity, self).__repr__())

gcloud/datastore/test_entity.py

@@ -85,6 +85,13 @@ def test_from_protobuf(self):
         self.assertEqual(key.kind(), _KIND)
         self.assertEqual(key.id(), _ID)
 
+    def test_reload_no_key(self):
+        from gcloud.datastore.entity import NoKey
+
+        entity = self._makeOne(None, None)
+        entity['foo'] = 'Foo'
+        self.assertRaises(NoKey, entity.reload)
+
     def test_reload_miss(self):
         dataset = _Dataset()
         key = _Key(dataset)
@@ -105,6 +112,13 @@ def test_reload_hit(self):
         self.assertTrue(entity.reload() is entity)
         self.assertEqual(entity['foo'], 'Bar')
 
+    def test_save_no_key(self):
+        from gcloud.datastore.entity import NoKey
+
+        entity = self._makeOne(None, None)
+        entity['foo'] = 'Foo'
+        self.assertRaises(NoKey, entity.save)
+
     def test_save_wo_transaction_wo_auto_id_wo_returned_key(self):
         connection = _Connection()
         dataset = _Dataset(connection)
@@ -167,6 +181,13 @@ def test_save_w_returned_key(self):
                          (_DATASET_ID, 'KEY', {'foo': 'Foo'}))
         self.assertEqual(key._path, [{'kind': _KIND, 'id': _ID}])
 
+    def test_delete_no_key(self):
+        from gcloud.datastore.entity import NoKey
+
+        entity = self._makeOne(None, None)
+        entity['foo'] = 'Foo'
+        self.assertRaises(NoKey, entity.delete)
+
     def test_delete(self):
         connection = _Connection()
         dataset = _Dataset(connection)
@@ -177,8 +198,23 @@ def test_delete(self):
         self.assertTrue(entity.delete() is None)
         self.assertEqual(connection._deleted, (_DATASET_ID, 'KEY'))
 
+    def test___repr___no_key_empty(self):
+        entity = self._makeOne(None, None)
+        self.assertEqual(repr(entity), '<Entity {}>')
+
+    def test___repr___w_key_non_empty(self):
+        connection = _Connection()
+        dataset = _Dataset(connection)
+        key = _Key(dataset)
+        key.path('/bar/baz')
+        entity = self._makeOne()
+        entity.key(key)
+        entity['foo'] = 'Foo'
+        self.assertEqual(repr(entity), "<Entity/bar/baz {'foo': 'Foo'}>")
+
 
 class _Key(object):
+    _MARKER = object()
     _key = 'KEY'
     _partial = False
     _path = None
@@ -195,7 +231,9 @@ def to_protobuf(self):
     def is_partial(self):
         return self._partial
 
-    def path(self, path):
+    def path(self, path=_MARKER):
+        if path is self._MARKER:
+            return self._path
         self._path = path