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:

    elif attr == 'list_value':
        if len(val) == 0:
            continue
        l_pb = value_pb.list_value
        for item in val:

[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.py https://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.

elif attr == 'list_value':
        l_pb = value_pb.list_value
        if len(val) == 0:
            x = l_pb.add()
            l_pb.remove(x)

or

if len(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.

[lucemia]

@dhermes
It seems to me that _set_protobuf_list will face the same issue as _set_protobuf_value.
It cannot delete Property itself due to the protobuf api limit..., can it ?

[dhermes]

It seems unlikely that this is fully possible without a re-write involving changing the arguments passed, potentially in multiple functions.

[lucemia]

@dhermes
For PropertyFilter usage, does that make sense to pass an empty list to Value?
If not, I think I can focus on solve the Property usage case first.

[dhermes]

ISTM both problems can be solved in the same way but maybe I'm missing something.

You can show the fix for Property and we can discuss?

[lucemia]

To solve the Property, I come up with two ways. (am I missing something?)

the original code in connection.py

for name, value in properties.items():
            prop = insert.property.add()
            # Set the name of the property.
            prop.name = name

            # Set the appropriate value.
            helpers._set_protobuf_value(prop.value, value)

            if name in exclude_from_indexes:
                if not isinstance(value, list):
                    prop.value.indexed = False

                for sub_value in prop.value.list_value:
                    sub_value.indexed = False

1. check the value type in first place

for name, value in properties.items():
    if isinstance(value, list) and len(value) == 0:
          continue
    prop = insert.property.add()
    # ....

2. define a new helper function

     for name, value in properties.items():
            helpers._set_protobuf_property(insert.property, name, value, 
                                           name in exclude_from_indexes)

[dhermes]

Number 2. seems a much more robust solution

[lucemia]

ok i will create a PR with sol2. Thanks!

[dhermes]

Closing in favor of #512