Require that keys for 'put' / 'delete' match the 'dataset_id' of the batch

[tseaver]

Fixes#447.

[coveralls]

Coverage remained the same when pulling b6ccf75 on tseaver:447-require_key_dataset_ids_match_branch into 12ac983 on GoogleCloudPlatform:master.

[dhermes]

Shall we also ditch _get_dataset_id_from_keys and just rely on this behavior?

[tseaver]

_get_dataset_id_from_keys is used by datastore.get, too, and is used to figure out which dataset_id to use in datastore.put and datastore.delete when no batch / transaction is already in play.

[dhermes]

OK so let's not remove, but it doesn't make sense to check them at the beginning and then again when each Key / Entity is passed to Batch.put() / Batch.delete().

Proposal:

• Stop using _get_dataset_id_from_keys in put() and delete().
• Introduce optional dataset_id=None parameter to put() and delete()
• Fall back to implicit if no dataset ID is passed

[tseaver]

SGTM: For consistency, get() should work the same way.

[dhermes]

But get() is not tied to a batch. How do you mean the same way?

[tseaver]

I just mean that the API should be consistent: get() should take an optional dataset_id, and fall back to the implicit one, rather than deriving it from the keys (it should still check the keys to ensure that they all match).

[dhermes]

SGTM

[tseaver]

I'm adding a variant on the "inferred connection / dataset ID" bit: if there is an active batch or transaction, and you don't pass them to datastore.get() / datastore.put() / datastore.delete(), then we use the value set on the batch or transaction.

[coveralls]

Coverage remained the same when pulling 9006e61 on tseaver:447-require_key_dataset_ids_match_branch into be8f5fb on GoogleCloudPlatform:master.

[dhermes]

RE: Doing the right thing comparing dataset_ids, instead of waiting on #528, how do you feel about just moving regression.datastore._compare_dataset_ids into gcloud.datastore.api?

[dhermes]

RE: variant of implicit, that seems to be the right move. Reviewing now.

[tseaver]

I forgot regression.datastore._compare_dataset_ids was there, and implemented a stripped-down variant in Batch._match_dataset_id (1b87a7e)

[dhermes]

Yes I just saw it in the review. Unfortunately it would gave false positives like e~foo == s~foo and s~foo == s~notfoo~foo. (These are not likely to be possible values in the wild, but better safe than sorry.)

[tseaver]

For a given "project ID" (the un-prefixed dataset ID), is it possible to have more than one prefix? I'm pretty sure that the project IDs themselves will never have an embedded ~: why else would they have picked it to delimit the "location"-based IDs?

At any rate, we could move the regression.datastore_compare_dataset_ids code into helpers, and call it from within _match_dataset_id (which wants to raise an error).

[dhermes]

I'm fairly certain the two cases I described are impossible, but the extra effort required is minimal since I've already written the code and the tests.

Moving to helpers SGTM, take a peek at
dhermes@90d5063
for _dataset_ids_equal and Test__dataset_ids_equal.

[coveralls]

Coverage remained the same when pulling ae880b4 on tseaver:447-require_key_dataset_ids_match_branch into be8f5fb on GoogleCloudPlatform:master.

[tseaver]

Are you working to merge dhermes/google-cloud-python@90d5063? If so, we should update _match_dataset_id to use your helper function afterwards.

[dhermes]

I am holding off on it until #528 has some sort of resolution, since it doesn't solve an actual problem, just gives "nice" behavior.

[dhermes]

RE: Impossibility of ordering

I think you're correct that without pathological things like

key=Key('Foo')
key._dataset_id=None

the last one can't occur.

---

I spoke too soon in my recommendation. As implemented, the following will fail but should not:

fromgcloudimportdatastoredatastore.set_defaults(dataset_id='foo')
k=Key('Kind', 10, dataset_id='bar')
datastore.delete([k])

With this, api.delete() will create a new Batch with dataset_id = 'foo' and then will result in an error when Batch.delete() is called on this new batch.

This is why the ordering

• dataset_id if passed explicitly
• Batch.current().dataset_id
• entities[0].key.dataset_id
• _implicit_environ.DATASET_ID

is necessary.

[tseaver]

This is why the ordering

• dataset_id if passed explicitly
• Batch.current().dataset_id
• entities[0].key.dataset_id
• _implicit_environ.DATASET_ID

I think the last is a "can't get there" case, because keys will always have a dataset_id. If that is so, I can update the PR to match.

[dhermes]

It's not the Keys we're worried about, it's the Batch (more than just Key uses the default). See the scenario I mentioned below the 1st line above.

[tseaver]

@dhermes no worries.

[coveralls]

Coverage remained the same at 100.0% when pulling db4c728 on tseaver:447-require_key_dataset_ids_match_branch into 6bbb144 on GoogleCloudPlatform:master.

[dhermes]

@tseaver Everything looks good except moving _match_dataset_id to helpers and using the implementation / tests from dhermes@90d5063

[tseaver]

@dhermes PTAL

[coveralls]

Coverage remained the same at 100.0% when pulling 49977da on tseaver:447-require_key_dataset_ids_match_branch into 6bbb144 on GoogleCloudPlatform:master.

[dhermes]

LGTM