Makes operator checking deterministic in Query.filter.

[dhermes]

The order of dictionary keys is not guaranteed between runs of
code so expressions ending in '>=' or '<=' were sometimes
matching for '='.

@pcostell the issue with flakiness was non-deterministic behavior of Python internals. Yay for finding the bug!

This was holding up #281, but it also forced us to use ancestor queries, which is a good thing.

@tseaver This is the quick-and-dirtiest way to fix this issue but I am a fan of changing from

query.filter('foo >=', 10)

to

query.filter('foo', '>=', 10)

WDYT?

Also @jgeewax WDYT since your original design. (Maybe you had reasons for combination property name and operator?)

[coveralls]

Coverage remained the same when pulling af1a39f on dhermes:query-operator-fix-dict-random-order into b6d3e74 on GoogleCloudPlatform:master.

[jgeewax]

I'm -1 to making it filter(field, operator, value) -- the reason being that this is how google.appengine.ext.db/ndb handled filtering (I just modeled it after them).

Another option to fix this is to use a regex:

• The regex used in appengine.db
• The filter method using the regex

I'd also feel much better if there were failing test to reproduce this as it's kind of a tricky bug... Any chance you could toss one in? Or add a ticket to make sure we have one to avoid regressing?

[dhermes]

@jgeewax RE: emulating db, this library is more low-level. If/when we build something more high-level, (for me) the "right" way to do this is the ndb way

qry = Account.query(Account.userid == 42)

I would also guess that they were like that in db for historical reasons (i.e. a different proto def).

As for a regression test, #281 is what made us discover this bug in the first place and I just commited fc23837.

[coveralls]

Coverage remained the same when pulling fc23837 on dhermes:query-operator-fix-dict-random-order into b6d3e74 on GoogleCloudPlatform:master.

[jgeewax]

👍 -- I think for this library, filter('field >=' value) is the right abstraction.

For a higher-level library (maybe we'll even pull ndb over ...) I absolutely agree with you that the ndb way is ideal (for example, it's far nicer than the Django ORM way..)

[Alfus]

I think filter('field', op, value) is technically better but more cumbersome. As long as filter('field', value) is not valid and filter('field with spaces =', value) is valid I think this function signature is workable (though it likely does not support property names with trailing white spaces).

Another option that doesn't require a full ORM is just adding a Property class with operator overloading: filter(Property('field') >= value)

[dhermes]

@Alfus that is on the roadmap, but as part of a higher-level abstraction than this one. Here we essentially want to mimic the API. In that vein, the .proto def has three fields rather than two, but I have no strong preference.

[dhermes]

Rebased after #292

[coveralls]

Coverage remained the same when pulling 799b6b7 on dhermes:query-operator-fix-dict-random-order into f7d0441 on GoogleCloudPlatform:master.

[tseaver]

LGTM pending Travis.

I don't know if we want to default to assuming equality in the case that there is no operator, but that shouldn't be a blocker.

[coveralls]

Coverage remained the same when pulling f47ffa1 on dhermes:query-operator-fix-dict-random-order into f7d0441 on GoogleCloudPlatform:master.

[dhermes]

Missing a few lines of test coverage so added 1fb43bd

Will wait out Travis again then merge. Somewhat surprised coveralls said coverage was unchanged.

[coveralls]

Coverage remained the same when pulling 1fb43bd on dhermes:query-operator-fix-dict-random-order into f7d0441 on GoogleCloudPlatform:master.