Fix #121: Drop munging of 's~' prefix onto keys.

[tseaver]

Fixes #121.

Note: may conflict with PR #257 and #258.

[dhermes]

This LGTM but I'd be more comfortable having @pcostell or @proppy take a look before we merge.

[coveralls]

Coverage remained the same when pulling 538084d on tseaver:121-drop_dataset_prefixes_in_keys into 906700b on GoogleCloudPlatform:master.

[tseaver]

Sure. The patch is based on @pcostell's feedback in #121.

[silvolu]

LGTM, we've made a similar fix in gcloud-node. I'm happy to merge if the change is rebased.

[tseaver]

Rebased to address merge conflicts.

[tseaver]

@pcostell @silvolu this change breaks access to the API. For instance:

$ python -m gcloud.datastore.demo

# pragma NO COVER
# Welcome to the gCloud Datastore Demo! (hit enter) 

# We're going to walk through some of the basics...
# Don't worry though. You don't need to do anything, just keep hitting enter... 

# Let's start by importing the demo module and getting a dataset: 
>>> from gcloud.datastore import demo
>>> dataset = demo.get_dataset()

# Let's create a new entity of type "Thing" and name it 'Toy': 
>>> toy = dataset.entity('Thing')
>>> toy.update({'name': 'Toy'})

# Now let's save it to our datastore: 
>>> toy.save()
Traceback (most recent call last):
File "/opt/Python-2.7.6/lib/python2.7/runpy.py", line 162, in _run_module_as_main
    "__main__", fname, loader, pkg_name)
File "/opt/Python-2.7.6/lib/python2.7/runpy.py", line 72, in _run_code
    exec code in run_globals
File "/home/tseaver/projects/agendaless/Google/src/gcloud-python/gcloud/datastore/demo/__main__.py", line 5, in <module>
    demo.DemoRunner.from_module(datastore).run()
File "gcloud/demo.py", line 34, in run
    self.code(lines)
File "gcloud/demo.py", line 102, in code
    self._execute_lines(code_lines)
File "gcloud/demo.py", line 109, in _execute_lines
    exec('\n'.join(lines), self.GLOBALS, self.LOCALS)
File "<string>", line 1, in <module>
File "gcloud/datastore/entity.py", line 225, in save
    properties=dict(self))
File "gcloud/datastore/connection.py", line 384, in save_entity
    result = self.commit(dataset_id, mutation)
File "gcloud/datastore/connection.py", line 337, in commit
    datastore_pb.CommitResponse)
File "gcloud/datastore/connection.py", line 81, in _rpc
    data=request_pb.SerializeToString())
File "gcloud/datastore/connection.py", line 59, in _request
    raise Exception('Request failed. Error was: %s' % content)
Exception: Request failed. Error was: app s~gcloud-datastore-demo cannot access app gcloud-datastore-demo&#39;s data

[pcostell]

Ah the problem looks like where you removed the munging:

When you connect to the dataset, you should only use the project-id (i.e. name without s~).

When you use that dataset to get/put, the keys involved should leave the dataset empty to get the correct dataset.

[tseaver]

@pcostell We were never adding the 's~' prefix to the dataset object's own ID: we only added it to the 'partition_id.dataset_id' field in the protobuf generated for each key. Do you mean that we should leave 'partition_id.dataset_id' empty for keys created de novo? I presume we would preserve whatever the server sets for keys returned from lookup / query operations.

[pcostell]

@tseaver That's correct. If you've been given the correct dataset_id (generally from the backend) it's ok to leave it as is. Otherwise, you should drop the entire dataset. The backend will see the empty dataset and fill it in with the dataset for the current application.

[tseaver]

@pcostell looking at the gcloud-node fix, it appears we could just avoid copying the dataset ID into the key protobuf altogether, i.e., deleting these lines.

Does that seem correct? If not, I'm not sure how to distinguish the "set from the backend" dataset IDs from the ones passed in from the environment / configuration.

[pcostell]

Could you leave that in so that provided datasets don't get erased but then
change this:
https://github.com/GoogleCloudPlatform/gcloud-python/blob/fab02378b3facca8ca290b4c0f52732f2a47181a/gcloud/datastore/entity.py#L72

So that when it creates a new entity it doesn't force the dataset of the
key?

If that won't work, not copying the dataset ID at all SGTM. I think the
only problem is if you want to store a key that belongs to a different
dataset. However, Cloud Datastore is working on making that easier so it
could be good enough to disallow that use case until the backend has better
support.

On Tue Oct 21 2014 at 10:59:07 AM Tres Seaver notifications@github.com
wrote:

@pcostell https://github.com/pcostell looking at the gcloud-node fix
googleapis/google-cloud-node#97, it appears
we could just avoid copying the dataset ID into the key protobuf
altogether, i.e., deleting these lines
https://github.com/GoogleCloudPlatform/gcloud-python/blob/fab02378b3facca8ca290b4c0f52732f2a47181a/gcloud/datastore/key.py#L94-L100
.

Does that seem correct? If not, I'm not sure how to distinguish the "set
from the backend" dataset IDs from the ones passed in from the environment
/ configuration.

—
Reply to this email directly or view it on GitHub
#259 (comment)
.