fix empty set

[lucemia]

for issue #403

for test case. I am curious about whether to create a new test case test_save_entity_w_transaction_list_value and included #399.

[coveralls]

Coverage decreased (-0.01%) when pulling 79ff1a3 on lucemia:property-with-empty-list into 5457564 on GoogleCloudPlatform:master.

[tseaver]

If we aren't going to marshall the value, then we should probably be preventing assignment of it in __setitem__() and update(). Otherwise, we might need to fabricate a custom entity_value to be used for this case.

[lucemia]

@tseaver
preventing assignment via __setitem__ and update() looks will fail in the following case..

entity=Entity(dataset, 'test')
entity['a'] = []
entity['a'].append(1) 

[tseaver]

Silently dropping the attribute-with-empty-list during save() will mean that it is missing when loaded again.

[dhermes]

Is this PR dead? It's certainly silent.

[tseaver]

@dhermes how would you envision dealing with an entity where the property value (in Python) is an empty list? We can't marshall that to the protobuf "normally", becase an empty list_value field doesn't satisfy the criterion that the property have a value set.

I posted in #403 (comment) a reference to an alternate implementation, which creates an entity_value for the property with an empty list, but didn't create a PR from that branch.

[dhermes]

@tseaver Sorry I wasn't trying to claim #403 is dead / fixed, just this PR. I looked at the alternate implementation, but am not a big fan of things like '_gcloud:empty_list'.

Has anyone been able to send an empty list using just proto objects?

[dhermes]

@tseaver I commented on the original issue as well, but I think the "correct" / simplest fix would be to change the code you focus on in your solution:

elifattr=='list_value':
iflen(val) ==0:
continuel_pb=value_pb.list_valueforiteminval:

[tseaver]

We would need to document that some values (empty lists, None, etc.) cannot be saved to the back-end.

[dhermes]

I agree. I wonder if the datastore folks also document that it's not possible to do?

[lucemia]

If we document this behavior,

We would need to document that some values (empty lists, None, etc.) cannot be saved to the back-end.

I guess we don't need to have '_gcloud:empty_list' ?

[dhermes]

Just a tiny code change which never adds the property and documents that empty lists aren't stored.

[lucemia]

@dhermes
if I fixed the helpers.py to

elif attr == 'list_value':
if len(val) == 0:
return
l_pb = value_pb.list_value
for item in val:
i_pb = l_pb.add()
_set_protobuf_value(i_pb, item)
else: # scalar, just assign
setattr(value_pb, attr, val)

it will still raise the following exception

Traceback (most recent call last):
File "test.py", line 26, in <module>
entity.save()
File "/Users/david/repos/gcloud-python/gcloud/datastore/entity.py", line 245, in save
exclude_from_indexes=self.exclude_from_indexes())
File "/Users/david/repos/gcloud-python/gcloud/datastore/connection.py", line 482, in save_entity
result = self.commit(dataset_id, mutation)
File "/Users/david/repos/gcloud-python/gcloud/datastore/connection.py", line 382, in commit
datastore_pb.CommitResponse)
File "/Users/david/repos/gcloud-python/gcloud/datastore/connection.py", line 95, in _rpc
data=request_pb.SerializeToString())
File "/Users/david/anaconda/lib/python2.7/site-packages/google/protobuf/internal/python_message.py", line 813, in SerializeToString
self.DESCRIPTOR.full_name, ','.join(self.FindInitializationErrors())))
google.protobuf.message.EncodeError: Message api.services.datastore.CommitRequest is missing required fields: mutation.insert_auto_id[0].property[0].value

so I check the list value in connection.pyhttps://github.com/GoogleCloudPlatform/gcloud-python/pull/404/files

[dhermes]

Right, adding the property should never happen. My bad for indicating otherwise.

[coveralls]

Coverage remained the same when pulling 9e2d1f0 on lucemia:property-with-empty-list into 5457564 on GoogleCloudPlatform:master.

[lucemia]

The test case is added :)

[coveralls]

Coverage remained the same when pulling 15a507e on lucemia:property-with-empty-list into 5457564 on GoogleCloudPlatform:master.

[dhermes]

You should also add a test for save_entities. This makes me think the change should happen in the to_protobuf method.

Also, for such a small change, we have quite a few do-nothing commits in this PR. Do you mind squashing into a single commit and re-writing your history? I'm happy to help if you like.

[lucemia]

@dhermes
I find a workaround today only need to modify the _set_protobuf_value in helpers.py.

elifattr=='list_value':
l_pb=value_pb.list_valueiflen(val) ==0:
x=l_pb.add()
l_pb.remove(x)

or

iflen(val) ==0:
l_pb.extend([])

[dhermes]

@lucemia That's great! I'm a little confused as to why it works. Wouldn't we be better clearing value_pb from its parent?

---

Also, provided we can agree on this proposed workaround, want to just close this PR and open a new one (so as not to have to worry about re-writing history)?

Make sure to polish up and run tox locally before committing / submitting the PR.

[lucemia]

Wouldn't we be better clearing value_pb from its parent?

Thx @dhermes

I am now very sure which parent you mention, would you mind to provide more detail therefore I can add it to my new PR?

I will open a new PR with new workaround and close current one.

[dhermes]

Well the issue is that the only thing _set_protobuf_value receives is an instance of datastore_v1_pb2.Value (via value_pb).

However, we actually want to remove a datastore_v1_pb2.Property (which owns value_pb) from a datastore_v1_pb2.Entity object such as an auto insert in a mutation:

>>>insert_auto=mutation.insert_auto_id.add()
>>>prop=insert_auto.property.add()
>>># Do some stuff>>># Oops didn't mean to add the property>>>insert_auto.property.remove(prop)

Unfortunately, it appears that there is no public API for retrieving a parent, so our best way forward may be a re-thinking of which data gets passed to _set_protobuf_value.

I took a stab with the non-public API provided by protobuf and it is not very pleasant. (Though it's also irrelevant since we can't rely on unsupported behavior.)

---

No need to close this one until we get the right solution actually sorted out.

[lucemia]

@dhermes

I created two branch and did some test with the following code.

conn=datastore.connection.Connection(credentials)
result=conn.save_entity(DATASET_ID, key_pb, {
'foo': u'Foo',
'bar': None,
'bar1': []
})

While saving a empty list to cloud datastore:

method1 fix the _set_protobuf_value

lucemia@5e4d5b9
this method will save the property with null value.

method2 fix the save_entity

lucemia@2c3defa
this method will drop the property.

I also test the same condition on ndb (with remote_api_shell), the ndb will drop repeated property with empty list (like method2). So I think the original approach (fix the save_entity) works better. If it sounds make sense, I will rebase the branch property-with-empty-list.