@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?

@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.

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.

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.

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 ?

You could style it after

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

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)

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

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'

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.

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

LGTM

Thanks again!