Fix datastore: unprefixed ids in client

[mpeg]

If datastore id comes back prefixed from the API, and Client.datastore_id is not prefixed, the 'Keys do not match dataset ID' exception is raised.

Fixed to use _dataset_ids_equal for the comparison instead, which compares ignoring valid prefixes and seems to be the preferred comparison method used elsewhere in the code.

[dhermes]

@mpeg Thanks for bringing this up. I'm trying to understand what this fixes.

Can you provide a stacktrace when that ValueError was raised previously?

[mpeg]

@dhermes Sure ! It's my first patch so wasn't sure how much detail to give

For the dataset Client class, I've got the dataset_id set from an env variable to mpeg-personal (my real dataset name)

Here's a REPL and stack trace highlighting the way I encountered this issue:

>>> from gcloud import datastore
>>> gcd = datastore.Client(namespace='namespace')
>>> gcd.dataset_id
'mpeg-personal'
>>> existing_key = gcd.get(gcd.key('Test', 5629499534213120)).key
>>> existing_key
<Key[{'id': 5629499534213120, 'kind': 'Test'}], dataset=e~mpeg-personal>
>>> gcd.get(existing_key)
Traceback (most recent call last):
 File "<stdin>", line 1, in <module>
 File "/usr/local/lib/python3.4/dist-packages/gcloud/datastore/client.py", line 263, in get
 deferred=deferred)
 File "/usr/local/lib/python3.4/dist-packages/gcloud/datastore/client.py", line 293, in get_multi
 raise ValueError('Keys do not match dataset ID')
ValueError: Keys do not match dataset ID

It all comes down to that string comparison done in the lines my patch changes, which does a comparison ignoring dataset prefixes (as the API will accept an unprefixed dataset_id).

The unexpected behaviour for me was that I couldn't store the key that came back and reuse it.

[dhermes]

Really great stacktrace, reproduced on my end! Thanks. Hopefully this will all be mitigated with v1beta3 but for now this fix is needed. Will comment on PR.

[dhermes]

So the only real issue with this is that we don't have a unit test to make sure the failure doesn't occur again. I can help with that if you like or you can give it a spin yourself?

I really appreciate the contribution.

[mpeg]

Sure, I can add the unit test, I'm thinking to the gcloud.datastore.test_client tests, add a test that gets a mock entity with a prefixed dataset_id ?

[dhermes]

You could style it after

https://github.com/GoogleCloudPlatform/gcloud-python/blob/6692df26f262ee6298629decb278fd02eeae727e/gcloud/datastore/test_client.py#L342-L351

[mpeg]

Added a test, styled after the test_get_multi_hit_multiple_keys_same_dataset and test_get_multi_hit_multiple_keys_different_dataset tests, as it's an extension of those, makes sure:

• dataset ids that are prefixed with "e~" or "s~" don't raise the ValueError exception
• prefixed dataset ids actually get the correct entity values (mostly in case of the mock classes changing in the future)

[dhermes]

Really great test thanks! I made some comments but overall it is about perfect (which is not something you expect from a first contrib).

[dhermes]

It just occurred to me that it'd be smart to have another similar test that does fail when the IDs disagree, e.g.

dataset_id1 = 'e~foo'
dataset_id2 = 's~bar'

[mpeg]

Yeah I agree with your style comments (sorry about the pep8 failure-to-indent), didn't add a failing test like that since it's largely covered by test_get_multi_hit_multiple_keys_different_dataset but perhaps would be good in case it all changes in the future and the issue comes back.

[mpeg]

Should all be good now, added the extra test case for e~foo and s~bar too.

Thanks for the help, hopefully first of many

Btw, I'm no git wizard but I can rebase my branch to squash the extra commits if you'd prefer, let me know

[dhermes]

LGTM

Thanks again!