Property with empty list 4

[lucemia]

for issue #403

Previous PR
#404

[dhermes]

1. I like this change.
2. What is the difficulty in getting this to work for PropertyFilter?
3. Please run tox locally before submitting changes.
4. I want to let @tseaver have a chance to see this before signing off.

[tseaver]

_set_protobuf_property looks like a win: I think we have a least a couple of versions of it lying around in the code. Once @dhermes' issues are addressed, LGTM.

[lucemia]

@dhermes
for Property with None value, my testing shows that the current behavior is the same as ndb. (both save a property will null value)
In that case we don't need to handle the None value.
I will updated the docstring for save_entity

ndb

from google.appengine.ext import ndb

class Kind(ndb.Model):
    foo = ndb.StringProperty()
    bar = ndb.StringProperty()
    bar1 = ndb.StringProperty(repeated=True)


a = Kind(bar=None, bar1=[])
a.put()

gcloud-python

from gcloud import datastore
from gcloud.credentials import *
from gcloud.datastore.key import *


client_email = "foo@mail.com"
private_key_path = "path/to/key.p12"
SCOPE = ('https://www.googleapis.com/auth/datastore ',
         'https://www.googleapis.com/auth/userinfo.email')
credentials = get_for_service_account_p12(client_email, private_key_path, SCOPE)

key_pb = Key('Kind', 1234, dataset_id="tagtooadex2").to_protobuf()

conn = datastore.connection.Connection(credentials)

result = conn.save_entity("tagtooadex2", key_pb, {
    'foo': u'Foo',
    'bar': None,
    'bar1': []
})

[dhermes]

@lucemia I was curious to see what actually is stored, which requires inspecting the retrieved protobuf.

After storing

>>> from gcloud import datastore
>>> from gcloud.datastore.entity import Entity
>>> from gcloud.datastore.key import Key
>>>
>>> datastore.set_default_connection()
>>> dataset_id = 'datasetfoo'
>>> datastore.set_default_dataset_id(dataset_id)
>>>
>>> key = Key('Kind', 1234)
>>> entity = Entity(key=key)
>>> entity['foo'] = None
>>> dict(entity)
{'foo': None}
>>> entity.save()

retrieving the same entity shows that the property is set but Property.value is empty:

>>> entity_pb, = datastore.get_connection().lookup(
...     dataset_id=dataset_id,
...     key_pbs=[key.to_protobuf()],
... )
>>> prop_foo, = entity_pb.property
>>> prop_foo.name
u'foo'
>>> prop_foo_value = prop_foo.value
>>> prop_foo_value._fields
{}

---

This happens because in helpers._set_protobuf_value, Property.value is cleared but the property is still set on the Entity.

It's unclear what is the right thing to support, maybe going with ndb is fine? However, the note you introduce makes it seem the property will be dropped all together (as is done for []).

---

Cleanup:

>>> print entity_pb
key {
  partition_id {
    dataset_id: "s~datasetfoo"
  }
  path_element {
    kind: "Kind"
    id: 1234
  }
}
property {
  name: "foo"
  value {
  }
}

>>> entity.key.delete()

[coveralls]

Coverage remained the same when pulling 479ea6a on lucemia:property-with-empty-list-4 into 6bde37d on GoogleCloudPlatform:master.

[lucemia]

@dhermes

It's unclear what is the right thing to support, maybe going with ndb is fine? However, the note you introduce makes it seem the property will be dropped all together (as is done for []).

Currently I go with ndb way. I updated the note to only include empty list case.

What is the difficulty in getting this to work for PropertyFilter?

Store a Property whose value is empty list sounds normal and should not raise any exception (currently I am working with).
But create a PropertyFilter whose value is empty list sounds weird.
Which behavior (write a helper function or raise exception) is recommend for PropertyFilter case is unclear for me.

ps. Rebase is welcome

In ndb, pass an empty list will cause a special FalseNode state.

s~tagtooadex2> Kind.query(Kind.bar.IN([]))
Query(kind='Kind', filters=<google.appengine.ext.ndb.query.FalseNode object at 0x104ab5210>)

s~tagtooadex2> Kind.query(Kind.bar.IN(['a']))
Query(kind='Kind', filters=FilterNode('bar', '=', 'a'))

and raise exception while query.

s~tagtooadex2> Kind.query(Kind.bar.IN([])).fetch()
WARNING:root:initial generator _run_to_list(query.py:956) raised BadQueryError(Cannot convert FalseNode to predicate)
Traceback (most recent call last):
  File "<console>", line 1, in <module>
  File "/Users/david/google-cloud-sdk/platform/google_appengine/google/appengine/ext/ndb/utils.py", line 142, in positional_wrapper
    return wrapped(*args, **kwds)
  File "/Users/david/google-cloud-sdk/platform/google_appengine/google/appengine/ext/ndb/query.py", line 1187, in fetch
    return self.fetch_async(limit, **q_options).get_result()
  File "/Users/david/google-cloud-sdk/platform/google_appengine/google/appengine/ext/ndb/tasklets.py", line 325, in get_result
    self.check_success()
  File "/Users/david/google-cloud-sdk/platform/google_appengine/google/appengine/ext/ndb/tasklets.py", line 371, in _help_tasklet_along
    value = gen.send(val)
  File "/Users/david/google-cloud-sdk/platform/google_appengine/google/appengine/ext/ndb/query.py", line 961, in _run_to_list
    dsquery = self._get_query(conn)
  File "/Users/david/google-cloud-sdk/platform/google_appengine/google/appengine/ext/ndb/query.py", line 908, in _get_query
    filters = filters._to_filter()
  File "/Users/david/google-cloud-sdk/platform/google_appengine/google/appengine/ext/ndb/query.py", line 425, in _to_filter
    'Cannot convert FalseNode to predicate')
BadQueryError: Cannot convert FalseNode to predicate

[dhermes]

I see, I was thinking the issue was the code, but you're saying the expected behavior is a bit subtle.

Also, what do you mean by "Rebase is welcome"?

[lucemia]

@dhermes

Also, what do you mean by "Rebase is welcome"?

Last pull request you ask about re-writeing history. I mean feel free to do so if necessary :)

[coveralls]

Coverage remained the same when pulling a250cb2 on lucemia:property-with-empty-list-4 into 0525530 on GoogleCloudPlatform:master.

[lucemia]

I have squash the commit