From 5aa16a5d307c57306f5e40e074816b8fe0bd4758 Mon Sep 17 00:00:00 2001 From: Tres Seaver Date: Thu, 15 Jan 2015 09:44:33 -0500 Subject: [PATCH 01/11] Require that keys for 'put' / 'delete' match the 'dataset_id' of the batch. Fixes #447. --- gcloud/datastore/batch.py | 12 ++++++++++-- gcloud/datastore/test_batch.py | 21 +++++++++++++++++++-- 2 files changed, 29 insertions(+), 4 deletions(-) diff --git a/gcloud/datastore/batch.py b/gcloud/datastore/batch.py index 7de7814841e8..b0c9d98562b4 100644 --- a/gcloud/datastore/batch.py +++ b/gcloud/datastore/batch.py @@ -210,11 +210,15 @@ def put(self, entity): :type entity: :class:`gcloud.datastore.entity.Entity` :param entity: the entity to be saved. - :raises: ValueError if entity has no key assigned. + :raises: ValueError if entity has no key assigned, or if the key's + ``dataset_id`` does not match ours. """ if entity.key is None: raise ValueError("Entity must have a key") + if entity.key.dataset_id != self._dataset_id: + raise ValueError("Key must be from same dataset as batch") + _assign_entity_to_mutation( self.mutation, entity, self._auto_id_entities) @@ -224,11 +228,15 @@ def delete(self, key): :type key: :class:`gcloud.datastore.key.Key` :param key: the key to be deleted. - :raises: ValueError if key is not complete. + :raises: ValueError if key is not complete, or if the key's + ``dataset_id`` does not match ours. """ if key.is_partial: raise ValueError("Key must be complete") + if key.dataset_id != self._dataset_id: + raise ValueError("Key must be from same dataset as batch") + key_pb = key.to_protobuf() helpers._add_keys_to_request(self.mutation.delete, [key_pb]) diff --git a/gcloud/datastore/test_batch.py b/gcloud/datastore/test_batch.py index 6121bd83dc83..65aea6bad4e9 100644 --- a/gcloud/datastore/test_batch.py +++ b/gcloud/datastore/test_batch.py @@ -116,7 +116,7 @@ def test_add_auto_id_entity_w_partial_key(self): connection = _Connection() batch = self._makeOne(dataset_id=_DATASET, connection=connection) entity = _Entity() - key = entity.key = _Key(_Entity) + key = entity.key = _Key(_DATASET) key._id = None batch.add_auto_id_entity(entity) @@ -128,7 +128,7 @@ def test_add_auto_id_entity_w_completed_key(self): connection = _Connection() batch = self._makeOne(dataset_id=_DATASET, connection=connection) entity = _Entity() - entity.key = _Key(_Entity) + entity.key = _Key(_DATASET) self.assertRaises(ValueError, batch.add_auto_id_entity, entity) @@ -139,6 +139,15 @@ def test_put_entity_wo_key(self): self.assertRaises(ValueError, batch.put, _Entity()) + def test_put_entity_w_key_wrong_dataset_id(self): + _DATASET = 'DATASET' + connection = _Connection() + batch = self._makeOne(dataset_id=_DATASET, connection=connection) + entity = _Entity() + entity.key = _Key('OTHER') + + self.assertRaises(ValueError, batch.put, entity) + def test_put_entity_w_partial_key(self): _DATASET = 'DATASET' _PROPERTIES = {'foo': 'bar'} @@ -203,6 +212,14 @@ def test_delete_w_partial_key(self): self.assertRaises(ValueError, batch.delete, key) + def test_delete_w_key_wrong_dataset_id(self): + _DATASET = 'DATASET' + connection = _Connection() + batch = self._makeOne(dataset_id=_DATASET, connection=connection) + key = _Key('OTHER') + + self.assertRaises(ValueError, batch.delete, key) + def test_delete_w_completed_key(self): _DATASET = 'DATASET' connection = _Connection() From 24b9b8ef01cddb28e3f6f2a241ed3186a527d8f5 Mon Sep 17 00:00:00 2001 From: Tres Seaver Date: Thu, 15 Jan 2015 13:10:03 -0500 Subject: [PATCH 02/11] Remove '_get_dataset_id_from_keys'. --- gcloud/datastore/api.py | 33 +++------------------------------ gcloud/datastore/test_api.py | 31 ------------------------------- 2 files changed, 3 insertions(+), 61 deletions(-) diff --git a/gcloud/datastore/api.py b/gcloud/datastore/api.py index 329cb1e5f10e..bd3abe8080c3 100644 --- a/gcloud/datastore/api.py +++ b/gcloud/datastore/api.py @@ -60,31 +60,6 @@ def _require_connection(connection=None): return connection -def _get_dataset_id_from_keys(keys): - """Determines dataset ID from a list of keys. - - :type keys: list of :class:`gcloud.datastore.key.Key` - :param keys: The keys from the same dataset. - - :rtype: string - :returns: The dataset ID of the keys. - :raises: :class:`ValueError` if the key dataset IDs don't agree. - """ - if any(key is None for key in keys): - raise ValueError('None not allowed') - - dataset_id = keys[0].dataset_id - # Rather than creating a list or set of all dataset IDs, we iterate - # and check. We could allow the backend to check this for us if IDs - # with no prefix worked (GoogleCloudPlatform/google-cloud-datastore#59) - # or if we made sure that a prefix s~ or e~ was on each key. - for key in keys[1:]: - if key.dataset_id != dataset_id: - raise ValueError('All keys in get must be from the same dataset.') - - return dataset_id - - def get(keys, missing=None, deferred=None, connection=None): """Retrieves entities, along with their attributes. @@ -111,7 +86,7 @@ def get(keys, missing=None, deferred=None, connection=None): return [] connection = _require_connection(connection) - dataset_id = _get_dataset_id_from_keys(keys) + dataset_id, = set([key.dataset_id for key in keys]) transaction = Transaction.current() @@ -157,8 +132,7 @@ def put(entities, connection=None): in_batch = current is not None if not in_batch: keys = [entity.key for entity in entities] - dataset_id = _get_dataset_id_from_keys(keys) - current = Batch(dataset_id=dataset_id, connection=connection) + current = Batch(dataset_id=keys[0].dataset_id, connection=connection) for entity in entities: current.put(entity) if not in_batch: @@ -183,8 +157,7 @@ def delete(keys, connection=None): current = Batch.current() in_batch = current is not None if not in_batch: - dataset_id = _get_dataset_id_from_keys(keys) - current = Batch(dataset_id=dataset_id, connection=connection) + current = Batch(dataset_id=keys[0].dataset_id, connection=connection) for key in keys: current.delete(key) if not in_batch: diff --git a/gcloud/datastore/test_api.py b/gcloud/datastore/test_api.py index 52b9ea0f9d85..cf268a23b3e9 100644 --- a/gcloud/datastore/test_api.py +++ b/gcloud/datastore/test_api.py @@ -90,37 +90,6 @@ def test_implicit_set_passed_explicitly(self): self.assertTrue(self._callFUT(CONNECTION) is CONNECTION) -class Test__get_dataset_id_from_keys(unittest2.TestCase): - - def _callFUT(self, keys): - from gcloud.datastore.api import _get_dataset_id_from_keys - return _get_dataset_id_from_keys(keys) - - def _make_key(self, dataset_id): - - class _Key(object): - def __init__(self, dataset_id): - self.dataset_id = dataset_id - - return _Key(dataset_id) - - def test_empty(self): - self.assertRaises(IndexError, self._callFUT, []) - - def test_w_None(self): - self.assertRaises(ValueError, self._callFUT, [None]) - - def test_w_mismatch(self): - key1 = self._make_key('foo') - key2 = self._make_key('bar') - self.assertRaises(ValueError, self._callFUT, [key1, key2]) - - def test_w_match(self): - key1 = self._make_key('foo') - key2 = self._make_key('foo') - self.assertEqual(self._callFUT([key1, key2]), 'foo') - - class Test_get_function(unittest2.TestCase): def _callFUT(self, keys, missing=None, deferred=None, connection=None): From 9bb4a3f45a39022a97cae7809c46fc63cf0b8c2a Mon Sep 17 00:00:00 2001 From: Tres Seaver Date: Thu, 15 Jan 2015 14:51:49 -0500 Subject: [PATCH 03/11] Work around the fact that keys updated from the server have prefixes. See #528. --- gcloud/datastore/batch.py | 22 ++++++++++++--- gcloud/datastore/test_batch.py | 51 ++++++++++++++++++++++++++++++++++ 2 files changed, 69 insertions(+), 4 deletions(-) diff --git a/gcloud/datastore/batch.py b/gcloud/datastore/batch.py index b0c9d98562b4..d141b4641a57 100644 --- a/gcloud/datastore/batch.py +++ b/gcloud/datastore/batch.py @@ -193,6 +193,22 @@ def add_auto_id_entity(self, entity): self._auto_id_entities.append(entity) + def _match_dataset_id(self, other_id): + """Ensure that `other_id` matches our `dataset_id`. + + Helper for :meth:`put` and :meth:`delete`. + + :type other_id: string + :param other_id: the dataset ID to compare + + :raises: ValueError if `other_id` does not match (even after stripping + any prefix). + """ + other_id = other_id.rsplit('~', 1)[-1] + our_id = self._dataset_id.rsplit('~', 1)[-1] + if other_id != our_id: + raise ValueError("Key must be from same dataset as batch") + def put(self, entity): """Remember an entity's state to be saved during ``commit``. @@ -216,8 +232,7 @@ def put(self, entity): if entity.key is None: raise ValueError("Entity must have a key") - if entity.key.dataset_id != self._dataset_id: - raise ValueError("Key must be from same dataset as batch") + self._match_dataset_id(entity.key.dataset_id) _assign_entity_to_mutation( self.mutation, entity, self._auto_id_entities) @@ -234,8 +249,7 @@ def delete(self, key): if key.is_partial: raise ValueError("Key must be complete") - if key.dataset_id != self._dataset_id: - raise ValueError("Key must be from same dataset as batch") + self._match_dataset_id(key.dataset_id) key_pb = key.to_protobuf() helpers._add_keys_to_request(self.mutation.delete, [key_pb]) diff --git a/gcloud/datastore/test_batch.py b/gcloud/datastore/test_batch.py index 65aea6bad4e9..579fee92a612 100644 --- a/gcloud/datastore/test_batch.py +++ b/gcloud/datastore/test_batch.py @@ -203,6 +203,41 @@ def test_put_entity_w_completed_key(self): deletes = list(batch.mutation.delete) self.assertEqual(len(deletes), 0) + def test_put_entity_w_completed_key_prefixed_dataset_id(self): + _DATASET = 'DATASET' + _PROPERTIES = { + 'foo': 'bar', + 'baz': 'qux', + 'spam': [1, 2, 3], + 'frotz': [], # will be ignored + } + connection = _Connection() + batch = self._makeOne(dataset_id=_DATASET, connection=connection) + entity = _Entity(_PROPERTIES) + entity.exclude_from_indexes = ('baz', 'spam') + key = entity.key = _Key('s~' + _DATASET) + + batch.put(entity) + + insert_auto_ids = list(batch.mutation.insert_auto_id) + self.assertEqual(len(insert_auto_ids), 0) + upserts = list(batch.mutation.upsert) + self.assertEqual(len(upserts), 1) + + upsert = upserts[0] + self.assertEqual(upsert.key, key._key) + props = dict([(prop.name, prop.value) for prop in upsert.property]) + self.assertTrue(props['foo'].indexed) + self.assertFalse(props['baz'].indexed) + self.assertTrue(props['spam'].indexed) + self.assertFalse(props['spam'].list_value[0].indexed) + self.assertFalse(props['spam'].list_value[1].indexed) + self.assertFalse(props['spam'].list_value[2].indexed) + self.assertFalse('frotz' in props) + + deletes = list(batch.mutation.delete) + self.assertEqual(len(deletes), 0) + def test_delete_w_partial_key(self): _DATASET = 'DATASET' connection = _Connection() @@ -236,6 +271,22 @@ def test_delete_w_completed_key(self): self.assertEqual(len(deletes), 1) self.assertEqual(deletes[0], key._key) + def test_delete_w_completed_key_w_prefixed_dataset_id(self): + _DATASET = 'DATASET' + connection = _Connection() + batch = self._makeOne(dataset_id=_DATASET, connection=connection) + key = _Key('s~' + _DATASET) + + batch.delete(key) + + insert_auto_ids = list(batch.mutation.insert_auto_id) + self.assertEqual(len(insert_auto_ids), 0) + upserts = list(batch.mutation.upsert) + self.assertEqual(len(upserts), 0) + deletes = list(batch.mutation.delete) + self.assertEqual(len(deletes), 1) + self.assertEqual(deletes[0], key._key) + def test_commit(self): _DATASET = 'DATASET' connection = _Connection() From 25aae75ab585575db5a74ca9770d3322cab29240 Mon Sep 17 00:00:00 2001 From: Tres Seaver Date: Thu, 15 Jan 2015 15:25:31 -0500 Subject: [PATCH 04/11] Infer connection / dataset ID from current batch... ...before falling back to the implicit defaults. --- gcloud/datastore/api.py | 20 ++++++++++++++------ gcloud/datastore/test_api.py | 28 +++++++++++++++++++++++++++- 2 files changed, 41 insertions(+), 7 deletions(-) diff --git a/gcloud/datastore/api.py b/gcloud/datastore/api.py index bd3abe8080c3..6da2b96e58de 100644 --- a/gcloud/datastore/api.py +++ b/gcloud/datastore/api.py @@ -36,9 +36,13 @@ def _require_dataset_id(dataset_id=None): and cannot be inferred from the environment. """ if dataset_id is None: - if _implicit_environ.DATASET_ID is None: - raise EnvironmentError('Dataset ID could not be inferred.') - dataset_id = _implicit_environ.DATASET_ID + top = Batch.current() + if top is not None: + dataset_id = top.dataset_id + else: + if _implicit_environ.DATASET_ID is None: + raise EnvironmentError('Dataset ID could not be inferred.') + dataset_id = _implicit_environ.DATASET_ID return dataset_id @@ -54,9 +58,13 @@ def _require_connection(connection=None): cannot be inferred from the environment. """ if connection is None: - if _implicit_environ.CONNECTION is None: - raise EnvironmentError('Connection could not be inferred.') - connection = _implicit_environ.CONNECTION + top = Batch.current() + if top is not None: + connection = top.connection + else: + if _implicit_environ.CONNECTION is None: + raise EnvironmentError('Connection could not be inferred.') + connection = _implicit_environ.CONNECTION return connection diff --git a/gcloud/datastore/test_api.py b/gcloud/datastore/test_api.py index cf268a23b3e9..88ce1a8dce13 100644 --- a/gcloud/datastore/test_api.py +++ b/gcloud/datastore/test_api.py @@ -35,6 +35,18 @@ def test_implicit_unset(self): with self.assertRaises(EnvironmentError): self._callFUT() + def test_implicit_unset_w_existing_batch(self): + ID = 'DATASET' + with self._monkey(None): + with _NoCommitBatch(dataset_id=ID, connection=object()): + self.assertEqual(self._callFUT(), ID) + + def test_implicit_unset_w_existing_transaction(self): + ID = 'DATASET' + with self._monkey(None): + with _NoCommitTransaction(dataset_id=ID, connection=object()): + self.assertEqual(self._callFUT(), ID) + def test_implicit_unset_passed_explicitly(self): ID = 'DATASET' with self._monkey(None): @@ -73,6 +85,20 @@ def test_implicit_unset(self): with self.assertRaises(EnvironmentError): self._callFUT() + def test_implicit_unset_w_existing_batch(self): + ID = 'DATASET' + CONNECTION = object() + with self._monkey(None): + with _NoCommitBatch(dataset_id=ID, connection=CONNECTION): + self.assertEqual(self._callFUT(), CONNECTION) + + def test_implicit_unset_w_existing_transaction(self): + ID = 'DATASET' + CONNECTION = object() + with self._monkey(None): + with _NoCommitTransaction(dataset_id=ID, connection=CONNECTION): + self.assertEqual(self._callFUT(), CONNECTION) + def test_implicit_unset_passed_explicitly(self): CONNECTION = object() with self._monkey(None): @@ -579,7 +605,7 @@ def __exit__(self, *args): class _NoCommitTransaction(object): - def __init__(self, dataset_id, connection, transaction_id): + def __init__(self, dataset_id, connection, transaction_id='TRANSACTION'): from gcloud.datastore.transaction import Transaction xact = self._transaction = Transaction(dataset_id, connection) xact._id = transaction_id From bccf8fc95771cf3b88e8d4a7997ff508ecc40664 Mon Sep 17 00:00:00 2001 From: Tres Seaver Date: Thu, 15 Jan 2015 15:27:00 -0500 Subject: [PATCH 05/11] Add explicit 'dataset_id' to 'get'/'put'/'delete'. If not passed, fall back to value inferred from batch / environment. --- gcloud/datastore/api.py | 53 +++++++-- gcloud/datastore/test_api.py | 205 ++++++++++++++++++++++++----------- 2 files changed, 185 insertions(+), 73 deletions(-) diff --git a/gcloud/datastore/api.py b/gcloud/datastore/api.py index 6da2b96e58de..cbefeae00cef 100644 --- a/gcloud/datastore/api.py +++ b/gcloud/datastore/api.py @@ -68,7 +68,7 @@ def _require_connection(connection=None): return connection -def get(keys, missing=None, deferred=None, connection=None): +def get(keys, missing=None, deferred=None, connection=None, dataset_id=None): """Retrieves entities, along with their attributes. :type keys: list of :class:`gcloud.datastore.key.Key` @@ -86,22 +86,34 @@ def get(keys, missing=None, deferred=None, connection=None): :type connection: :class:`gcloud.datastore.connection.Connection` :param connection: Optional. The connection used to connect to datastore. + If not passed, inferred from the environment. + + :type dataset_id: :class:`gcloud.datastore.connection.Connection` + :param dataset_id: Optional. The dataset ID used to connect to datastore. + If not passed, inferred from the environment. :rtype: list of :class:`gcloud.datastore.entity.Entity` :returns: The requested entities. + :raises: EnvironmentError if ``connection`` or ``dataset_id`` not passed, + and cannot be inferred from the environment. ValueError if + one or more of ``keys`` has a dataset ID which does not match + the passed / inferred dataset ID. """ if not keys: return [] connection = _require_connection(connection) - dataset_id, = set([key.dataset_id for key in keys]) + dataset_id = _require_dataset_id(dataset_id) + if list(set([key.dataset_id for key in keys])) != [dataset_id]: + raise ValueError('Keys do not match dataset ID') transaction = Transaction.current() entity_pbs = connection.lookup( dataset_id=dataset_id, key_pbs=[k.to_protobuf() for k in keys], - missing=missing, deferred=deferred, + missing=missing, + deferred=deferred, transaction_id=transaction and transaction.id, ) @@ -122,7 +134,7 @@ def get(keys, missing=None, deferred=None, connection=None): return entities -def put(entities, connection=None): +def put(entities, connection=None, dataset_id=None): """Save the entities in the Cloud Datastore. :type entities: list of :class:`gcloud.datastore.entity.Entity` @@ -130,24 +142,34 @@ def put(entities, connection=None): :type connection: :class:`gcloud.datastore.connection.Connection` :param connection: Optional connection used to connect to datastore. + If not passed, inferred from the environment. + + :type dataset_id: :class:`gcloud.datastore.connection.Connection` + :param dataset_id: Optional. The dataset ID used to connect to datastore. + If not passed, inferred from the environment. + + :raises: EnvironmentError if ``connection`` or ``dataset_id`` not passed, + and cannot be inferred from the environment. ValueError if + one or more entities has a key with a dataset ID not matching + the passed / inferred dataset ID. """ if not entities: return - connection = connection or _implicit_environ.CONNECTION + connection = _require_connection(connection) + dataset_id = _require_dataset_id(dataset_id) current = Batch.current() in_batch = current is not None if not in_batch: - keys = [entity.key for entity in entities] - current = Batch(dataset_id=keys[0].dataset_id, connection=connection) + current = Batch(dataset_id=dataset_id, connection=connection) for entity in entities: current.put(entity) if not in_batch: current.commit() -def delete(keys, connection=None): +def delete(keys, connection=None, dataset_id=None): """Delete the keys in the Cloud Datastore. :type keys: list of :class:`gcloud.datastore.key.Key` @@ -155,17 +177,28 @@ def delete(keys, connection=None): :type connection: :class:`gcloud.datastore.connection.Connection` :param connection: Optional connection used to connect to datastore. + If not passed, inferred from the environment. + + :type dataset_id: :class:`gcloud.datastore.connection.Connection` + :param dataset_id: Optional. The dataset ID used to connect to datastore. + If not passed, inferred from the environment. + + :raises: EnvironmentError if ``connection`` or ``dataset_id`` not passed, + and cannot be inferred from the environment. ValueError if + one or more keys has a dataset ID not matching the passed / + inferred dataset ID. """ if not keys: return - connection = connection or _implicit_environ.CONNECTION + connection = _require_connection(connection) + dataset_id = _require_dataset_id(dataset_id) # We allow partial keys to attempt a delete, the backend will fail. current = Batch.current() in_batch = current is not None if not in_batch: - current = Batch(dataset_id=keys[0].dataset_id, connection=connection) + current = Batch(dataset_id=dataset_id, connection=connection) for key in keys: current.delete(key) if not in_batch: diff --git a/gcloud/datastore/test_api.py b/gcloud/datastore/test_api.py index 88ce1a8dce13..275a02c3b867 100644 --- a/gcloud/datastore/test_api.py +++ b/gcloud/datastore/test_api.py @@ -118,10 +118,45 @@ def test_implicit_set_passed_explicitly(self): class Test_get_function(unittest2.TestCase): - def _callFUT(self, keys, missing=None, deferred=None, connection=None): + def _callFUT(self, keys, missing=None, deferred=None, + connection=None, dataset_id=None): from gcloud.datastore.api import get return get(keys, missing=missing, deferred=deferred, - connection=connection) + connection=connection, dataset_id=dataset_id) + + def _make_entity_pb(self, dataset_id, kind, integer_id, + name=None, str_val=None): + from gcloud.datastore import _datastore_v1_pb2 as datastore_pb + + entity_pb = datastore_pb.Entity() + entity_pb.key.partition_id.dataset_id = dataset_id + path_element = entity_pb.key.path_element.add() + path_element.kind = kind + path_element.id = integer_id + if name is not None and str_val is not None: + prop = entity_pb.property.add() + prop.name = name + prop.value.string_value = str_val + + return entity_pb + + def test_wo_connection(self): + from gcloud.datastore.key import Key + + DATASET_ID = 'DATASET' + key = Key('Kind', 1234, dataset_id=DATASET_ID) + self.assertRaises(EnvironmentError, + self._callFUT, [key], dataset_id=DATASET_ID) + + def test_wo_dataset_id(self): + from gcloud.datastore.key import Key + from gcloud.datastore.test_connection import _Connection + + DATASET_ID = 'DATASET' + connection = _Connection() + key = Key('Kind', 1234, dataset_id=DATASET_ID) + self.assertRaises(EnvironmentError, + self._callFUT, [key], connection=connection) def test_no_keys(self): results = self._callFUT([]) @@ -134,7 +169,8 @@ def test_miss(self): DATASET_ID = 'DATASET' connection = _Connection() key = Key('Kind', 1234, dataset_id=DATASET_ID) - results = self._callFUT([key], connection=connection) + results = self._callFUT([key], connection=connection, + dataset_id=DATASET_ID) self.assertEqual(results, []) def test_miss_w_missing(self): @@ -159,7 +195,8 @@ def test_miss_w_missing(self): key = Key(KIND, ID, dataset_id=DATASET_ID) missing = [] - entities = self._callFUT([key], connection=connection, missing=missing) + entities = self._callFUT([key], connection=connection, + missing=missing, dataset_id=DATASET_ID) self.assertEqual(entities, []) self.assertEqual([missed.key.to_protobuf() for missed in missing], [key.to_protobuf()]) @@ -177,27 +214,11 @@ def test_miss_w_deferred(self): deferred = [] entities = self._callFUT([key], connection=connection, - deferred=deferred) + deferred=deferred, dataset_id=DATASET_ID) self.assertEqual(entities, []) self.assertEqual([def_key.to_protobuf() for def_key in deferred], [key.to_protobuf()]) - def _make_entity_pb(self, dataset_id, kind, integer_id, - name=None, str_val=None): - from gcloud.datastore import _datastore_v1_pb2 as datastore_pb - - entity_pb = datastore_pb.Entity() - entity_pb.key.partition_id.dataset_id = dataset_id - path_element = entity_pb.key.path_element.add() - path_element.kind = kind - path_element.id = integer_id - if name is not None and str_val is not None: - prop = entity_pb.property.add() - prop.name = name - prop.value.string_value = str_val - - return entity_pb - def test_hit(self): from gcloud.datastore.key import Key from gcloud.datastore.test_connection import _Connection @@ -215,7 +236,8 @@ def test_hit(self): connection = _Connection(entity_pb) key = Key(KIND, ID, dataset_id=DATASET_ID) - result, = self._callFUT([key], connection=connection) + result, = self._callFUT([key], connection=connection, + dataset_id=DATASET_ID) new_key = result.key # Check the returned value is as expected. @@ -244,7 +266,7 @@ def test_hit_multiple_keys_same_dataset(self): key1 = Key(KIND, ID1, dataset_id=DATASET_ID) key2 = Key(KIND, ID2, dataset_id=DATASET_ID) retrieved1, retrieved2 = self._callFUT( - [key1, key2], connection=connection) + [key1, key2], connection=connection, dataset_id=DATASET_ID) # Check values match. self.assertEqual(retrieved1.key.path, key1.path) @@ -264,7 +286,8 @@ def test_hit_multiple_keys_different_dataset(self): key1 = Key('KIND', 1234, dataset_id=DATASET_ID1) key2 = Key('KIND', 1234, dataset_id=DATASET_ID2) with self.assertRaises(ValueError): - self._callFUT([key1, key2], connection=object()) + self._callFUT([key1, key2], connection=object(), + dataset_id=DATASET_ID1) def test_implicit_wo_transaction(self): from gcloud.datastore import _implicit_environ @@ -323,7 +346,8 @@ def test_w_transaction(self): key = Key(KIND, ID, dataset_id=DATASET_ID) with _NoCommitTransaction(DATASET_ID, CUSTOM_CONNECTION, TRANSACTION): - result, = self._callFUT([key], connection=CUSTOM_CONNECTION) + result, = self._callFUT([key], connection=CUSTOM_CONNECTION, + dataset_id=DATASET_ID) expected_called_with = { 'dataset_id': DATASET_ID, @@ -343,9 +367,46 @@ def test_w_transaction(self): class Test_put_function(unittest2.TestCase): - def _callFUT(self, entities, connection=None): + def _callFUT(self, entities, connection=None, dataset_id=None): from gcloud.datastore.api import put - return put(entities, connection=connection) + return put(entities, connection=connection, dataset_id=dataset_id) + + def test_no_connection(self): + from gcloud.datastore import _implicit_environ + from gcloud.datastore.test_batch import _Entity + from gcloud.datastore.test_batch import _Key + + # Build basic mocks needed to delete. + _DATASET = 'DATASET' + entity = _Entity(foo=u'bar') + entity.key = _Key(_DATASET) + + self.assertEqual(_implicit_environ.CONNECTION, None) + with self.assertRaises(EnvironmentError): + self._callFUT([entity], dataset_id=_DATASET) + + def test_no_dataset_id(self): + from gcloud.datastore import _implicit_environ + from gcloud.datastore.test_batch import _Connection + from gcloud.datastore.test_batch import _Entity + from gcloud.datastore.test_batch import _Key + + # Build basic mocks needed to delete. + _DATASET = 'DATASET' + connection = _Connection() + entity = _Entity(foo=u'bar') + entity.key = _Key(_DATASET) + + self.assertEqual(_implicit_environ.CONNECTION, None) + with self.assertRaises(EnvironmentError): + self._callFUT([entity], connection=connection) + + def test_no_entities(self): + from gcloud.datastore import _implicit_environ + + self.assertEqual(_implicit_environ.CONNECTION, None) + result = self._callFUT([]) + self.assertEqual(result, None) def test_no_batch_w_partial_key(self): from gcloud.datastore.test_batch import _Connection @@ -359,7 +420,8 @@ def test_no_batch_w_partial_key(self): key = entity.key = _Key(_DATASET) key._id = None - result = self._callFUT([entity], connection=connection) + result = self._callFUT([entity], connection=connection, + dataset_id=_DATASET) self.assertEqual(result, None) self.assertEqual(len(connection._committed), 1) dataset_id, mutation = connection._committed[0] @@ -425,33 +487,45 @@ def test_implicit_connection(self): self.assertEqual(properties[0].value.string_value, u'bar') self.assertEqual(len(CURR_BATCH.mutation.delete), 0) - def test_no_entities(self): - from gcloud.datastore import _implicit_environ - self.assertEqual(_implicit_environ.CONNECTION, None) - result = self._callFUT([]) - self.assertEqual(result, None) +class Test_delete_function(unittest2.TestCase): + + def _callFUT(self, keys, connection=None, dataset_id=None): + from gcloud.datastore.api import delete + return delete(keys, connection=connection, dataset_id=dataset_id) def test_no_connection(self): from gcloud.datastore import _implicit_environ - from gcloud.datastore.test_batch import _Entity from gcloud.datastore.test_batch import _Key # Build basic mocks needed to delete. _DATASET = 'DATASET' - entity = _Entity(foo=u'bar') - entity.key = _Key(_DATASET) + key = _Key(_DATASET) self.assertEqual(_implicit_environ.CONNECTION, None) - with self.assertRaises(ValueError): - self._callFUT([entity]) + with self.assertRaises(EnvironmentError): + self._callFUT([key], dataset_id=_DATASET) + def test_no_dataset_id(self): + from gcloud.datastore import _implicit_environ + from gcloud.datastore.test_batch import _Connection + from gcloud.datastore.test_batch import _Key -class Test_delete_function(unittest2.TestCase): + # Build basic mocks needed to delete. + _DATASET = 'DATASET' + connection = _Connection() + key = _Key(_DATASET) - def _callFUT(self, keys, connection=None): - from gcloud.datastore.api import delete - return delete(keys, connection=connection) + self.assertEqual(_implicit_environ.CONNECTION, None) + with self.assertRaises(EnvironmentError): + self._callFUT([key], connection=connection) + + def test_no_keys(self): + from gcloud.datastore import _implicit_environ + + self.assertEqual(_implicit_environ.CONNECTION, None) + result = self._callFUT([]) + self.assertEqual(result, None) def test_no_batch(self): from gcloud.datastore.test_batch import _Connection @@ -462,14 +536,15 @@ def test_no_batch(self): connection = _Connection() key = _Key(_DATASET) - result = self._callFUT([key], connection=connection) + result = self._callFUT([key], connection=connection, + dataset_id=_DATASET) self.assertEqual(result, None) self.assertEqual(len(connection._committed), 1) dataset_id, mutation = connection._committed[0] self.assertEqual(dataset_id, _DATASET) self.assertEqual(list(mutation.delete), [key.to_protobuf()]) - def test_existing_batch(self): + def test_w_existing_batch(self): from gcloud.datastore.test_batch import _Connection from gcloud.datastore.test_batch import _Key @@ -480,7 +555,7 @@ def test_existing_batch(self): # Set up Batch on stack so we can check it is used. with _NoCommitBatch(_DATASET, connection) as CURR_BATCH: - result = self._callFUT([key], connection=connection) + result = self._callFUT([key]) self.assertEqual(result, None) self.assertEqual(len(CURR_BATCH.mutation.insert_auto_id), 0) @@ -490,9 +565,7 @@ def test_existing_batch(self): self.assertEqual(deletes[0], key._key) self.assertEqual(len(connection._committed), 0) - def test_implicit_connection(self): - from gcloud._testing import _Monkey - from gcloud.datastore import _implicit_environ + def test_w_existing_transaction(self): from gcloud.datastore.test_batch import _Connection from gcloud.datastore.test_batch import _Key @@ -501,10 +574,9 @@ def test_implicit_connection(self): connection = _Connection() key = _Key(_DATASET) - with _Monkey(_implicit_environ, CONNECTION=connection): - # Set up Batch on stack so we can check it is used. - with _NoCommitBatch(_DATASET, connection) as CURR_BATCH: - result = self._callFUT([key]) + # Set up Batch on stack so we can check it is used. + with _NoCommitTransaction(_DATASET, connection) as CURR_BATCH: + result = self._callFUT([key]) self.assertEqual(result, None) self.assertEqual(len(CURR_BATCH.mutation.insert_auto_id), 0) @@ -514,24 +586,31 @@ def test_implicit_connection(self): self.assertEqual(deletes[0], key._key) self.assertEqual(len(connection._committed), 0) - def test_no_keys(self): - from gcloud.datastore import _implicit_environ - - self.assertEqual(_implicit_environ.CONNECTION, None) - result = self._callFUT([]) - self.assertEqual(result, None) - - def test_no_connection(self): + def test_implicit_connection_and_dataset_id(self): + from gcloud._testing import _Monkey from gcloud.datastore import _implicit_environ + from gcloud.datastore.test_batch import _Connection from gcloud.datastore.test_batch import _Key # Build basic mocks needed to delete. _DATASET = 'DATASET' + connection = _Connection() key = _Key(_DATASET) - self.assertEqual(_implicit_environ.CONNECTION, None) - with self.assertRaises(ValueError): - self._callFUT([key]) + with _Monkey(_implicit_environ, + CONNECTION=connection, + DATASET_ID=_DATASET): + # Set up Batch on stack so we can check it is used. + with _NoCommitBatch(_DATASET, connection) as CURR_BATCH: + result = self._callFUT([key]) + + self.assertEqual(result, None) + self.assertEqual(len(CURR_BATCH.mutation.insert_auto_id), 0) + self.assertEqual(len(CURR_BATCH.mutation.upsert), 0) + deletes = list(CURR_BATCH.mutation.delete) + self.assertEqual(len(deletes), 1) + self.assertEqual(deletes[0], key._key) + self.assertEqual(len(connection._committed), 0) class Test_allocate_ids_function(unittest2.TestCase): From 258a2cd00c3a0548afd167bd69c5b72d05d91c66 Mon Sep 17 00:00:00 2001 From: Tres Seaver Date: Thu, 15 Jan 2015 16:18:24 -0500 Subject: [PATCH 06/11] Re-allow use of dataset ID from keys as a fallback. See: https://github.com/GoogleCloudPlatform/gcloud-python/pull/552#discussion_r23043288 --- gcloud/datastore/api.py | 16 +++++++++--- gcloud/datastore/test_api.py | 48 ++++++++++++++++++++++++++---------- 2 files changed, 48 insertions(+), 16 deletions(-) diff --git a/gcloud/datastore/api.py b/gcloud/datastore/api.py index cbefeae00cef..0887455c10eb 100644 --- a/gcloud/datastore/api.py +++ b/gcloud/datastore/api.py @@ -103,7 +103,11 @@ def get(keys, missing=None, deferred=None, connection=None, dataset_id=None): return [] connection = _require_connection(connection) - dataset_id = _require_dataset_id(dataset_id) + try: + dataset_id = _require_dataset_id(dataset_id) + except EnvironmentError: + dataset_id = keys[0].dataset_id + if list(set([key.dataset_id for key in keys])) != [dataset_id]: raise ValueError('Keys do not match dataset ID') @@ -157,7 +161,10 @@ def put(entities, connection=None, dataset_id=None): return connection = _require_connection(connection) - dataset_id = _require_dataset_id(dataset_id) + try: + dataset_id = _require_dataset_id(dataset_id) + except EnvironmentError: + dataset_id = entities[0].key.dataset_id current = Batch.current() in_batch = current is not None @@ -192,7 +199,10 @@ def delete(keys, connection=None, dataset_id=None): return connection = _require_connection(connection) - dataset_id = _require_dataset_id(dataset_id) + try: + dataset_id = _require_dataset_id(dataset_id) + except EnvironmentError: + dataset_id = keys[0].dataset_id # We allow partial keys to attempt a delete, the backend will fail. current = Batch.current() diff --git a/gcloud/datastore/test_api.py b/gcloud/datastore/test_api.py index 275a02c3b867..f246dcd2a12f 100644 --- a/gcloud/datastore/test_api.py +++ b/gcloud/datastore/test_api.py @@ -148,30 +148,36 @@ def test_wo_connection(self): self.assertRaises(EnvironmentError, self._callFUT, [key], dataset_id=DATASET_ID) - def test_wo_dataset_id(self): + def test_no_keys(self): + results = self._callFUT([]) + self.assertEqual(results, []) + + def test_miss(self): from gcloud.datastore.key import Key from gcloud.datastore.test_connection import _Connection DATASET_ID = 'DATASET' connection = _Connection() key = Key('Kind', 1234, dataset_id=DATASET_ID) - self.assertRaises(EnvironmentError, - self._callFUT, [key], connection=connection) - - def test_no_keys(self): - results = self._callFUT([]) + results = self._callFUT([key], connection=connection, + dataset_id=DATASET_ID) self.assertEqual(results, []) - def test_miss(self): + def test_miss_wo_dataset_id(self): from gcloud.datastore.key import Key from gcloud.datastore.test_connection import _Connection DATASET_ID = 'DATASET' connection = _Connection() key = Key('Kind', 1234, dataset_id=DATASET_ID) - results = self._callFUT([key], connection=connection, - dataset_id=DATASET_ID) + results = self._callFUT([key], connection=connection) self.assertEqual(results, []) + expected = { + 'dataset_id': DATASET_ID, + 'key_pbs': [key.to_protobuf()], + 'transaction_id': None, + } + self.assertEqual(connection._called_with, expected) def test_miss_w_missing(self): from gcloud.datastore import _datastore_v1_pb2 as datastore_pb @@ -398,8 +404,18 @@ def test_no_dataset_id(self): entity.key = _Key(_DATASET) self.assertEqual(_implicit_environ.CONNECTION, None) - with self.assertRaises(EnvironmentError): - self._callFUT([entity], connection=connection) + result = self._callFUT([entity], connection=connection) + + self.assertEqual(result, None) + self.assertEqual(len(connection._committed), 1) + dataset_id, mutation = connection._committed[0] + self.assertEqual(dataset_id, _DATASET) + upserts = list(mutation.upsert) + self.assertEqual(len(upserts), 1) + self.assertEqual(upserts[0].key, entity.key.to_protobuf()) + properties = list(upserts[0].property) + self.assertEqual(properties[0].name, 'foo') + self.assertEqual(properties[0].value.string_value, u'bar') def test_no_entities(self): from gcloud.datastore import _implicit_environ @@ -517,8 +533,14 @@ def test_no_dataset_id(self): key = _Key(_DATASET) self.assertEqual(_implicit_environ.CONNECTION, None) - with self.assertRaises(EnvironmentError): - self._callFUT([key], connection=connection) + + result = self._callFUT([key], connection=connection) + + self.assertEqual(result, None) + self.assertEqual(len(connection._committed), 1) + dataset_id, mutation = connection._committed[0] + self.assertEqual(dataset_id, _DATASET) + self.assertEqual(list(mutation.delete), [key.to_protobuf()]) def test_no_keys(self): from gcloud.datastore import _implicit_environ From d5d640013912a6602acf6f12e68864e056a9c2a7 Mon Sep 17 00:00:00 2001 From: Tres Seaver Date: Tue, 27 Jan 2015 14:25:46 -0500 Subject: [PATCH 07/11] Re-include keys when searching for a dataset ID. Per: https://github.com/GoogleCloudPlatform/gcloud-python/pull/552#issuecomment-70166094. --- gcloud/datastore/api.py | 48 +++++++++++---------- gcloud/datastore/test_api.py | 84 ++++++++++++++++++++++++++++++++---- 2 files changed, 101 insertions(+), 31 deletions(-) diff --git a/gcloud/datastore/api.py b/gcloud/datastore/api.py index 0887455c10eb..56e466c7da67 100644 --- a/gcloud/datastore/api.py +++ b/gcloud/datastore/api.py @@ -24,26 +24,38 @@ from gcloud.datastore import helpers -def _require_dataset_id(dataset_id=None): +def _require_dataset_id(dataset_id=None, keys=()): """Infer a dataset ID from the environment, if not passed explicitly. + Order or precedence: + + - Passed `dataset_id` (if not None). + - `dataset_id` of current batch / transaction (if current exists). + - `dataset_id` of first key in `keys` + - `dataset_id` inferred from the environment (if `set_default_dataset_id` + has been called). + :type dataset_id: string :param dataset_id: Optional. + :type dataset_id: list of :class:`gcloud.datastore.key.Key` + :param dataset_id: Optional. + :rtype: string :returns: A dataset ID based on the current environment. :raises: :class:`EnvironmentError` if ``dataset_id`` is ``None``, and cannot be inferred from the environment. """ - if dataset_id is None: - top = Batch.current() - if top is not None: - dataset_id = top.dataset_id - else: - if _implicit_environ.DATASET_ID is None: - raise EnvironmentError('Dataset ID could not be inferred.') - dataset_id = _implicit_environ.DATASET_ID - return dataset_id + if dataset_id is not None: + return dataset_id + top = Batch.current() + if top is not None: + return top.dataset_id + if len(keys) > 0: + return keys[0].dataset_id + if _implicit_environ.DATASET_ID is None: + raise EnvironmentError('Dataset ID could not be inferred.') + return _implicit_environ.DATASET_ID def _require_connection(connection=None): @@ -103,10 +115,7 @@ def get(keys, missing=None, deferred=None, connection=None, dataset_id=None): return [] connection = _require_connection(connection) - try: - dataset_id = _require_dataset_id(dataset_id) - except EnvironmentError: - dataset_id = keys[0].dataset_id + dataset_id = _require_dataset_id(dataset_id, keys) if list(set([key.dataset_id for key in keys])) != [dataset_id]: raise ValueError('Keys do not match dataset ID') @@ -161,10 +170,8 @@ def put(entities, connection=None, dataset_id=None): return connection = _require_connection(connection) - try: - dataset_id = _require_dataset_id(dataset_id) - except EnvironmentError: - dataset_id = entities[0].key.dataset_id + keys = [entity.key for entity in entities] + dataset_id = _require_dataset_id(dataset_id, keys) current = Batch.current() in_batch = current is not None @@ -199,10 +206,7 @@ def delete(keys, connection=None, dataset_id=None): return connection = _require_connection(connection) - try: - dataset_id = _require_dataset_id(dataset_id) - except EnvironmentError: - dataset_id = keys[0].dataset_id + dataset_id = _require_dataset_id(dataset_id, keys) # We allow partial keys to attempt a delete, the backend will fail. current = Batch.current() diff --git a/gcloud/datastore/test_api.py b/gcloud/datastore/test_api.py index f246dcd2a12f..95853d1272d5 100644 --- a/gcloud/datastore/test_api.py +++ b/gcloud/datastore/test_api.py @@ -19,51 +19,95 @@ class Test__require_dataset_id(unittest2.TestCase): _MARKER = object() - def _callFUT(self, passed=_MARKER): + def _callFUT(self, passed=_MARKER, keys=()): from gcloud.datastore.api import _require_dataset_id if passed is self._MARKER: - return _require_dataset_id() - return _require_dataset_id(passed) + return _require_dataset_id(keys=keys) + return _require_dataset_id(dataset_id=passed, keys=keys) def _monkey(self, dataset_id): from gcloud.datastore import _implicit_environ from gcloud._testing import _Monkey return _Monkey(_implicit_environ, DATASET_ID=dataset_id) - def test_implicit_unset(self): + def test_implicit_unset_wo_keys(self): with self._monkey(None): with self.assertRaises(EnvironmentError): self._callFUT() - def test_implicit_unset_w_existing_batch(self): + def test_implicit_unset_w_keys(self): + from gcloud.datastore.test_batch import _Key + ID = 'DATASET' + with self._monkey(None): + self.assertEqual(self._callFUT(keys=[_Key(ID)]), ID) + + def test_implicit_unset_w_existing_batch_wo_keys(self): ID = 'DATASET' with self._monkey(None): with _NoCommitBatch(dataset_id=ID, connection=object()): self.assertEqual(self._callFUT(), ID) - def test_implicit_unset_w_existing_transaction(self): + def test_implicit_unset_w_existing_batch_w_keys(self): + from gcloud.datastore.test_batch import _Key + ID = 'DATASET' + OTHER = 'OTHER' + with self._monkey(None): + with _NoCommitBatch(dataset_id=ID, connection=object()): + self.assertEqual(self._callFUT(keys=[_Key(OTHER)]), ID) + + def test_implicit_unset_w_existing_transaction_wo_keys(self): ID = 'DATASET' with self._monkey(None): with _NoCommitTransaction(dataset_id=ID, connection=object()): self.assertEqual(self._callFUT(), ID) - def test_implicit_unset_passed_explicitly(self): + def test_implicit_unset_w_existing_transaction_w_keys(self): + from gcloud.datastore.test_batch import _Key + ID = 'DATASET' + OTHER = 'OTHER' + with self._monkey(None): + with _NoCommitTransaction(dataset_id=ID, connection=object()): + self.assertEqual(self._callFUT(keys=[_Key(OTHER)]), ID) + + def test_implicit_unset_passed_explicitly_wo_keys(self): ID = 'DATASET' with self._monkey(None): self.assertEqual(self._callFUT(ID), ID) - def test_id_implicit_set(self): + def test_implicit_unset_passed_explicitly_w_keys(self): + from gcloud.datastore.test_batch import _Key + ID = 'DATASET' + OTHER = 'OTHER' + with self._monkey(None): + self.assertEqual(self._callFUT(ID, keys=[_Key(OTHER)]), ID) + + def test_id_implicit_set_wo_keys(self): IMPLICIT_ID = 'IMPLICIT' with self._monkey(IMPLICIT_ID): stored_id = self._callFUT() self.assertTrue(stored_id is IMPLICIT_ID) - def test_id_implicit_set_passed_explicitly(self): + def test_id_implicit_set_w_keys(self): + from gcloud.datastore.test_batch import _Key + IMPLICIT_ID = 'IMPLICIT' + OTHER = 'OTHER' + with self._monkey(IMPLICIT_ID): + self.assertEqual(self._callFUT(keys=[_Key(OTHER)]), OTHER) + + def test_id_implicit_set_passed_explicitly_wo_keys(self): ID = 'DATASET' IMPLICIT_ID = 'IMPLICIT' with self._monkey(IMPLICIT_ID): self.assertEqual(self._callFUT(ID), ID) + def test_id_implicit_set_passed_explicitly_w_keys(self): + from gcloud.datastore.test_batch import _Key + ID = 'DATASET' + IMPLICIT_ID = 'IMPLICIT' + OTHER = 'OTHER' + with self._monkey(IMPLICIT_ID): + self.assertEqual(self._callFUT(ID, keys=[_Key(OTHER)]), ID) + class Test__require_connection(unittest2.TestCase): @@ -566,6 +610,28 @@ def test_no_batch(self): self.assertEqual(dataset_id, _DATASET) self.assertEqual(list(mutation.delete), [key.to_protobuf()]) + def test_wo_batch_w_key_different_than_default_dataset_id(self): + from gcloud._testing import _Monkey + from gcloud.datastore import _implicit_environ + from gcloud.datastore.test_batch import _Connection + from gcloud.datastore.test_batch import _Key + + # Build basic mocks needed to delete. + _DEFAULT_DATASET = 'DEFAULT' + _DATASET = 'DATASET' + connection = _Connection() + key = _Key(_DATASET) + + with _Monkey(_implicit_environ, + CONNECTION=connection, + DATASET_ID=_DEFAULT_DATASET): + result = self._callFUT([key]) + self.assertEqual(result, None) + self.assertEqual(len(connection._committed), 1) + dataset_id, mutation = connection._committed[0] + self.assertEqual(dataset_id, _DATASET) + self.assertEqual(list(mutation.delete), [key.to_protobuf()]) + def test_w_existing_batch(self): from gcloud.datastore.test_batch import _Connection from gcloud.datastore.test_batch import _Key From d198d69bcbbe16a8ae86371d3f7955394735c9e6 Mon Sep 17 00:00:00 2001 From: Tres Seaver Date: Tue, 27 Jan 2015 18:23:21 -0500 Subject: [PATCH 08/11] Fix typos. Incorporates feedback from @dhermes: - https://github.com/GoogleCloudPlatform/gcloud-python/pull/552#discussion_r23650168 - https://github.com/GoogleCloudPlatform/gcloud-python/pull/552#discussion_r23650191 --- gcloud/datastore/api.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/gcloud/datastore/api.py b/gcloud/datastore/api.py index 56e466c7da67..4624f3abfd43 100644 --- a/gcloud/datastore/api.py +++ b/gcloud/datastore/api.py @@ -27,7 +27,7 @@ def _require_dataset_id(dataset_id=None, keys=()): """Infer a dataset ID from the environment, if not passed explicitly. - Order or precedence: + Order of precedence: - Passed `dataset_id` (if not None). - `dataset_id` of current batch / transaction (if current exists). @@ -38,8 +38,8 @@ def _require_dataset_id(dataset_id=None, keys=()): :type dataset_id: string :param dataset_id: Optional. - :type dataset_id: list of :class:`gcloud.datastore.key.Key` - :param dataset_id: Optional. + :type keys: list of :class:`gcloud.datastore.key.Key` + :param keys: Optional. :rtype: string :returns: A dataset ID based on the current environment. From db4c728d2904a3218685ad1a84fdc9b8e34c9815 Mon Sep 17 00:00:00 2001 From: Tres Seaver Date: Tue, 27 Jan 2015 18:29:37 -0500 Subject: [PATCH 09/11] Rework '_require_dataset_id' to avoid making caller pass all keys. Instead, caller passes only the first key. Incorporates feedback from @dhermes: - https://github.com/GoogleCloudPlatform/gcloud-python/pull/552#discussion_r23650231 --- gcloud/datastore/api.py | 19 +++++++++---------- gcloud/datastore/test_api.py | 18 +++++++++--------- 2 files changed, 18 insertions(+), 19 deletions(-) diff --git a/gcloud/datastore/api.py b/gcloud/datastore/api.py index 4624f3abfd43..dca79c12aaeb 100644 --- a/gcloud/datastore/api.py +++ b/gcloud/datastore/api.py @@ -24,22 +24,22 @@ from gcloud.datastore import helpers -def _require_dataset_id(dataset_id=None, keys=()): +def _require_dataset_id(dataset_id=None, first_key=None): """Infer a dataset ID from the environment, if not passed explicitly. Order of precedence: - Passed `dataset_id` (if not None). - `dataset_id` of current batch / transaction (if current exists). - - `dataset_id` of first key in `keys` + - `dataset_id` of first key - `dataset_id` inferred from the environment (if `set_default_dataset_id` has been called). :type dataset_id: string :param dataset_id: Optional. - :type keys: list of :class:`gcloud.datastore.key.Key` - :param keys: Optional. + :type first_key: :class:`gcloud.datastore.key.Key` or None + :param first_key: Optional: first key being manipulated. :rtype: string :returns: A dataset ID based on the current environment. @@ -51,8 +51,8 @@ def _require_dataset_id(dataset_id=None, keys=()): top = Batch.current() if top is not None: return top.dataset_id - if len(keys) > 0: - return keys[0].dataset_id + if first_key is not None: + return first_key.dataset_id if _implicit_environ.DATASET_ID is None: raise EnvironmentError('Dataset ID could not be inferred.') return _implicit_environ.DATASET_ID @@ -115,7 +115,7 @@ def get(keys, missing=None, deferred=None, connection=None, dataset_id=None): return [] connection = _require_connection(connection) - dataset_id = _require_dataset_id(dataset_id, keys) + dataset_id = _require_dataset_id(dataset_id, keys[0]) if list(set([key.dataset_id for key in keys])) != [dataset_id]: raise ValueError('Keys do not match dataset ID') @@ -170,8 +170,7 @@ def put(entities, connection=None, dataset_id=None): return connection = _require_connection(connection) - keys = [entity.key for entity in entities] - dataset_id = _require_dataset_id(dataset_id, keys) + dataset_id = _require_dataset_id(dataset_id, entities[0].key) current = Batch.current() in_batch = current is not None @@ -206,7 +205,7 @@ def delete(keys, connection=None, dataset_id=None): return connection = _require_connection(connection) - dataset_id = _require_dataset_id(dataset_id, keys) + dataset_id = _require_dataset_id(dataset_id, keys[0]) # We allow partial keys to attempt a delete, the backend will fail. current = Batch.current() diff --git a/gcloud/datastore/test_api.py b/gcloud/datastore/test_api.py index 95853d1272d5..abf238d796a0 100644 --- a/gcloud/datastore/test_api.py +++ b/gcloud/datastore/test_api.py @@ -19,11 +19,11 @@ class Test__require_dataset_id(unittest2.TestCase): _MARKER = object() - def _callFUT(self, passed=_MARKER, keys=()): + def _callFUT(self, passed=_MARKER, first_key=None): from gcloud.datastore.api import _require_dataset_id if passed is self._MARKER: - return _require_dataset_id(keys=keys) - return _require_dataset_id(dataset_id=passed, keys=keys) + return _require_dataset_id(first_key=first_key) + return _require_dataset_id(dataset_id=passed, first_key=first_key) def _monkey(self, dataset_id): from gcloud.datastore import _implicit_environ @@ -39,7 +39,7 @@ def test_implicit_unset_w_keys(self): from gcloud.datastore.test_batch import _Key ID = 'DATASET' with self._monkey(None): - self.assertEqual(self._callFUT(keys=[_Key(ID)]), ID) + self.assertEqual(self._callFUT(first_key=_Key(ID)), ID) def test_implicit_unset_w_existing_batch_wo_keys(self): ID = 'DATASET' @@ -53,7 +53,7 @@ def test_implicit_unset_w_existing_batch_w_keys(self): OTHER = 'OTHER' with self._monkey(None): with _NoCommitBatch(dataset_id=ID, connection=object()): - self.assertEqual(self._callFUT(keys=[_Key(OTHER)]), ID) + self.assertEqual(self._callFUT(first_key=_Key(OTHER)), ID) def test_implicit_unset_w_existing_transaction_wo_keys(self): ID = 'DATASET' @@ -67,7 +67,7 @@ def test_implicit_unset_w_existing_transaction_w_keys(self): OTHER = 'OTHER' with self._monkey(None): with _NoCommitTransaction(dataset_id=ID, connection=object()): - self.assertEqual(self._callFUT(keys=[_Key(OTHER)]), ID) + self.assertEqual(self._callFUT(first_key=_Key(OTHER)), ID) def test_implicit_unset_passed_explicitly_wo_keys(self): ID = 'DATASET' @@ -79,7 +79,7 @@ def test_implicit_unset_passed_explicitly_w_keys(self): ID = 'DATASET' OTHER = 'OTHER' with self._monkey(None): - self.assertEqual(self._callFUT(ID, keys=[_Key(OTHER)]), ID) + self.assertEqual(self._callFUT(ID, first_key=_Key(OTHER)), ID) def test_id_implicit_set_wo_keys(self): IMPLICIT_ID = 'IMPLICIT' @@ -92,7 +92,7 @@ def test_id_implicit_set_w_keys(self): IMPLICIT_ID = 'IMPLICIT' OTHER = 'OTHER' with self._monkey(IMPLICIT_ID): - self.assertEqual(self._callFUT(keys=[_Key(OTHER)]), OTHER) + self.assertEqual(self._callFUT(first_key=_Key(OTHER)), OTHER) def test_id_implicit_set_passed_explicitly_wo_keys(self): ID = 'DATASET' @@ -106,7 +106,7 @@ def test_id_implicit_set_passed_explicitly_w_keys(self): IMPLICIT_ID = 'IMPLICIT' OTHER = 'OTHER' with self._monkey(IMPLICIT_ID): - self.assertEqual(self._callFUT(ID, keys=[_Key(OTHER)]), ID) + self.assertEqual(self._callFUT(ID, first_key=_Key(OTHER)), ID) class Test__require_connection(unittest2.TestCase): From a6cea31338978c65bda55c368862b0b04e2bd29c Mon Sep 17 00:00:00 2001 From: Tres Seaver Date: Wed, 28 Jan 2015 12:02:34 -0500 Subject: [PATCH 10/11] Land '_dataset_ids_equal' in helpers. From https://github.com/dhermes/gcloud-python/commit/90d50639609ac3edca8d24da2e64672ea3b5afc5. --- gcloud/datastore/helpers.py | 52 ++++++++++++++++++++++++++++++++ gcloud/datastore/test_helpers.py | 24 +++++++++++++++ 2 files changed, 76 insertions(+) diff --git a/gcloud/datastore/helpers.py b/gcloud/datastore/helpers.py index c0559ff9be5d..c6178b369ad3 100644 --- a/gcloud/datastore/helpers.py +++ b/gcloud/datastore/helpers.py @@ -319,3 +319,55 @@ def _add_keys_to_request(request_field_pb, key_pbs): for key_pb in key_pbs: key_pb = _prepare_key_for_request(key_pb) request_field_pb.add().CopyFrom(key_pb) + + +def _dataset_ids_equal(dataset_id1, dataset_id2): + """Compares two dataset IDs for fuzzy equality. + + Each may be prefixed or unprefixed (but not null, since dataset ID + is required on a key). The only allowed prefixes are 's~' and 'e~'. + + Two identical prefixed match + + >>> 's~foo' == 's~foo' + >>> 'e~bar' == 'e~bar' + + while non-identical prefixed don't + + >>> 's~foo' != 's~bar' + >>> 's~foo' != 'e~foo' + + As for non-prefixed, they can match other non-prefixed or + prefixed: + + >>> 'foo' == 'foo' + >>> 'foo' == 's~foo' + >>> 'foo' == 'e~foo' + >>> 'foo' != 'bar' + >>> 'foo' != 's~bar' + + (Ties are resolved since 'foo' can only be an alias for one of + s~foo or e~foo in the backend.) + + :type dataset_id1: string + :param dataset_id1: A dataset ID. + + :type dataset_id2: string + :param dataset_id2: A dataset ID. + + :rtype: boolean + :returns: Boolean indicating if the IDs are the same. + """ + if dataset_id1 == dataset_id2: + return True + + if dataset_id1.startswith('s~') or dataset_id1.startswith('e~'): + # If `dataset_id1` is prefixed and not matching, then the only way + # they can match is if `dataset_id2` is unprefixed. + return dataset_id1[2:] == dataset_id2 + elif dataset_id2.startswith('s~') or dataset_id2.startswith('e~'): + # Here we know `dataset_id1` is unprefixed and `dataset_id2` + # is prefixed. + return dataset_id1 == dataset_id2[2:] + + return False diff --git a/gcloud/datastore/test_helpers.py b/gcloud/datastore/test_helpers.py index 6602d5ed6a61..b087444ab97d 100644 --- a/gcloud/datastore/test_helpers.py +++ b/gcloud/datastore/test_helpers.py @@ -517,3 +517,27 @@ def test_prepare_dataset_id_unset(self): key = datastore_pb.Key() new_key = self._callFUT(key) self.assertTrue(new_key is key) + + +class Test__dataset_ids_equal(unittest2.TestCase): + + def _callFUT(self, dataset_id1, dataset_id2): + from gcloud.datastore.helpers import _dataset_ids_equal + return _dataset_ids_equal(dataset_id1, dataset_id2) + + def test_identical_prefixed(self): + self.assertTrue(self._callFUT('s~foo', 's~foo')) + self.assertTrue(self._callFUT('e~bar', 'e~bar')) + + def test_different_prefixed(self): + self.assertFalse(self._callFUT('s~foo', 's~bar')) + self.assertFalse(self._callFUT('s~foo', 'e~foo')) + + def test_all_unprefixed(self): + self.assertTrue(self._callFUT('foo', 'foo')) + self.assertFalse(self._callFUT('foo', 'bar')) + + def test_unprefixed_with_prefixed(self): + self.assertTrue(self._callFUT('foo', 's~foo')) + self.assertTrue(self._callFUT('foo', 'e~foo')) + self.assertFalse(self._callFUT('foo', 's~bar')) From 49977da5b0c42fc687f1b8de14c3e75ba495578a Mon Sep 17 00:00:00 2001 From: Tres Seaver Date: Wed, 28 Jan 2015 12:03:02 -0500 Subject: [PATCH 11/11] Use 'helpers._dataset_ids_equal'. --- gcloud/datastore/batch.py | 24 ++++++------------------ 1 file changed, 6 insertions(+), 18 deletions(-) diff --git a/gcloud/datastore/batch.py b/gcloud/datastore/batch.py index d141b4641a57..bc9c370fdeea 100644 --- a/gcloud/datastore/batch.py +++ b/gcloud/datastore/batch.py @@ -193,22 +193,6 @@ def add_auto_id_entity(self, entity): self._auto_id_entities.append(entity) - def _match_dataset_id(self, other_id): - """Ensure that `other_id` matches our `dataset_id`. - - Helper for :meth:`put` and :meth:`delete`. - - :type other_id: string - :param other_id: the dataset ID to compare - - :raises: ValueError if `other_id` does not match (even after stripping - any prefix). - """ - other_id = other_id.rsplit('~', 1)[-1] - our_id = self._dataset_id.rsplit('~', 1)[-1] - if other_id != our_id: - raise ValueError("Key must be from same dataset as batch") - def put(self, entity): """Remember an entity's state to be saved during ``commit``. @@ -232,7 +216,9 @@ def put(self, entity): if entity.key is None: raise ValueError("Entity must have a key") - self._match_dataset_id(entity.key.dataset_id) + if not helpers._dataset_ids_equal(self._dataset_id, + entity.key.dataset_id): + raise ValueError("Key must be from same dataset as batch") _assign_entity_to_mutation( self.mutation, entity, self._auto_id_entities) @@ -249,7 +235,9 @@ def delete(self, key): if key.is_partial: raise ValueError("Key must be complete") - self._match_dataset_id(key.dataset_id) + if not helpers._dataset_ids_equal(self._dataset_id, + key.dataset_id): + raise ValueError("Key must be from same dataset as batch") key_pb = key.to_protobuf() helpers._add_keys_to_request(self.mutation.delete, [key_pb])