From f8a4db4d3373ba383f824ca6e387e4bdf400472a Mon Sep 17 00:00:00 2001 From: Danny Hermes Date: Fri, 24 Oct 2014 16:52:01 -0700 Subject: [PATCH 1/4] Makes operator checking deterministic in Query.filter. The order of dictionary keys is not guaranteed between runs of code so `expression`s ending in '>=' or '<=' were sometimes matching for '='. --- gcloud/datastore/query.py | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/gcloud/datastore/query.py b/gcloud/datastore/query.py index edfe00c7889d..f82fd8543dee 100644 --- a/gcloud/datastore/query.py +++ b/gcloud/datastore/query.py @@ -45,13 +45,15 @@ class Query(object): :param dataset: The namespace to which to restrict results. """ - OPERATORS = { - '<': datastore_pb.PropertyFilter.LESS_THAN, - '<=': datastore_pb.PropertyFilter.LESS_THAN_OR_EQUAL, - '>': datastore_pb.PropertyFilter.GREATER_THAN, - '>=': datastore_pb.PropertyFilter.GREATER_THAN_OR_EQUAL, - '=': datastore_pb.PropertyFilter.EQUAL, - } + # NOTE: Order is very important here since operators that end with + # '<=' and '>=' also end with '='. + OPERATORS = ( + ('<=', datastore_pb.PropertyFilter.LESS_THAN_OR_EQUAL), + ('>=', datastore_pb.PropertyFilter.GREATER_THAN_OR_EQUAL), + ('<', datastore_pb.PropertyFilter.LESS_THAN), + ('>', datastore_pb.PropertyFilter.GREATER_THAN), + ('=', datastore_pb.PropertyFilter.EQUAL), + ) """Mapping of operator strings and their protobuf equivalents.""" def __init__(self, kind=None, dataset=None, namespace=None): @@ -132,10 +134,12 @@ def filter(self, expression, value): property_name, operator = None, None expression = expression.strip() - for operator_string in self.OPERATORS: + for operator_string, pb_op_enum in self.OPERATORS: if expression.endswith(operator_string): - operator = self.OPERATORS[operator_string] + operator = pb_op_enum property_name = expression[0:-len(operator_string)].strip() + # After one match, we move on since >= and <= conflict with =. + break if not operator or not property_name: raise ValueError('Invalid expression: "%s"' % expression) From 799b6b76be145eb05c32f8bf39c789f501abf7e9 Mon Sep 17 00:00:00 2001 From: Danny Hermes Date: Sat, 25 Oct 2014 10:09:58 -0700 Subject: [PATCH 2/4] Adding test to make sure the correct operators get set on filter. --- gcloud/datastore/test_query.py | 34 +++++++++++++++++++++++++++++++++- 1 file changed, 33 insertions(+), 1 deletion(-) diff --git a/gcloud/datastore/test_query.py b/gcloud/datastore/test_query.py index eeac85676e96..530737b64ee3 100644 --- a/gcloud/datastore/test_query.py +++ b/gcloud/datastore/test_query.py @@ -68,16 +68,48 @@ def test_filter_w_unknown_operator(self): self.assertRaises(ValueError, query.filter, 'firstname ~~', 'John') def test_filter_w_known_operator(self): + from gcloud.datastore import datastore_v1_pb2 as datastore_pb + query = self._makeOne() after = query.filter('firstname =', u'John') self.assertFalse(after is query) self.assertTrue(isinstance(after, self._getTargetClass())) q_pb = after.to_protobuf() - self.assertEqual(q_pb.filter.composite_filter.operator, 1) # AND + self.assertEqual(q_pb.filter.composite_filter.operator, + datastore_pb.CompositeFilter.AND) f_pb, = list(q_pb.filter.composite_filter.filter) p_pb = f_pb.property_filter self.assertEqual(p_pb.property.name, 'firstname') self.assertEqual(p_pb.value.string_value, u'John') + self.assertEqual(p_pb.operator, datastore_pb.PropertyFilter.EQUAL) + + def test_filter_w_all_operators(self): + from gcloud.datastore import datastore_v1_pb2 as datastore_pb + + query = self._makeOne() + query = query.filter('leq_prop <=', u'val1') + query = query.filter('geq_prop >=', u'val2') + query = query.filter('lt_prop <', u'val3') + query = query.filter('gt_prop >', u'val4') + query = query.filter('eq_prop =', u'val5') + + query_pb = query.to_protobuf() + pb_values = [ + ('leq_prop', 'val1', + datastore_pb.PropertyFilter.LESS_THAN_OR_EQUAL), + ('geq_prop', 'val2', + datastore_pb.PropertyFilter.GREATER_THAN_OR_EQUAL), + ('lt_prop', 'val3', datastore_pb.PropertyFilter.LESS_THAN), + ('gt_prop', 'val4', datastore_pb.PropertyFilter.GREATER_THAN), + ('eq_prop', 'val5', datastore_pb.PropertyFilter.EQUAL), + ] + query_filter = query_pb.filter.composite_filter.filter + for filter_pb, pb_value in zip(query_filter, pb_values): + name, val, filter_enum = pb_value + prop_filter = filter_pb.property_filter + self.assertEqual(prop_filter.property.name, name) + self.assertEqual(prop_filter.value.string_value, val) + self.assertEqual(prop_filter.operator, filter_enum) def test_filter_w_known_operator_and_entity(self): import operator From f47ffa106f26ae4fdf16d1f6bace33942dbb9baf Mon Sep 17 00:00:00 2001 From: Danny Hermes Date: Mon, 27 Oct 2014 12:41:04 -0700 Subject: [PATCH 3/4] Using operator dict in datastore.query and splitting on whitespace. --- gcloud/datastore/query.py | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/gcloud/datastore/query.py b/gcloud/datastore/query.py index f82fd8543dee..9a6b56f70062 100644 --- a/gcloud/datastore/query.py +++ b/gcloud/datastore/query.py @@ -45,15 +45,13 @@ class Query(object): :param dataset: The namespace to which to restrict results. """ - # NOTE: Order is very important here since operators that end with - # '<=' and '>=' also end with '='. - OPERATORS = ( - ('<=', datastore_pb.PropertyFilter.LESS_THAN_OR_EQUAL), - ('>=', datastore_pb.PropertyFilter.GREATER_THAN_OR_EQUAL), - ('<', datastore_pb.PropertyFilter.LESS_THAN), - ('>', datastore_pb.PropertyFilter.GREATER_THAN), - ('=', datastore_pb.PropertyFilter.EQUAL), - ) + OPERATORS = { + '<=': datastore_pb.PropertyFilter.LESS_THAN_OR_EQUAL, + '>=': datastore_pb.PropertyFilter.GREATER_THAN_OR_EQUAL, + '<': datastore_pb.PropertyFilter.LESS_THAN, + '>': datastore_pb.PropertyFilter.GREATER_THAN, + '=': datastore_pb.PropertyFilter.EQUAL, + } """Mapping of operator strings and their protobuf equivalents.""" def __init__(self, kind=None, dataset=None, namespace=None): @@ -134,14 +132,16 @@ def filter(self, expression, value): property_name, operator = None, None expression = expression.strip() - for operator_string, pb_op_enum in self.OPERATORS: - if expression.endswith(operator_string): - operator = pb_op_enum - property_name = expression[0:-len(operator_string)].strip() - # After one match, we move on since >= and <= conflict with =. - break + # Use None to split on *any* whitespace. + expr_pieces = expression.rsplit(None, 1) + if len(expr_pieces) == 2: + property_name, operator = expr_pieces + property_name = property_name.strip() - if not operator or not property_name: + # If no whitespace in `expression`, `operator` will be `None` and + # self.OPERATORS[None] will be `None` as well. + pb_op_enum = self.OPERATORS.get(operator) + if pb_op_enum is None: raise ValueError('Invalid expression: "%s"' % expression) # Build a composite filter AND'd together. @@ -151,7 +151,7 @@ def filter(self, expression, value): # Add the specific filter property_filter = composite_filter.filter.add().property_filter property_filter.property.name = property_name - property_filter.operator = operator + property_filter.operator = pb_op_enum # Set the value to filter on based on the type. helpers._set_protobuf_value(property_filter.value, value) From 1fb43bd1fc334308c4b05cd67956bb605d08bd20 Mon Sep 17 00:00:00 2001 From: Danny Hermes Date: Mon, 27 Oct 2014 13:00:21 -0700 Subject: [PATCH 4/4] Adding test for datastore.query.filter with no operator used. This was to get test coverage back to 100%. --- gcloud/datastore/test_query.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/gcloud/datastore/test_query.py b/gcloud/datastore/test_query.py index 530737b64ee3..0324bc309fdd 100644 --- a/gcloud/datastore/test_query.py +++ b/gcloud/datastore/test_query.py @@ -63,6 +63,10 @@ def test_to_protobuf_w_kind(self): kq_pb, = list(q_pb.kind) self.assertEqual(kq_pb.name, _KIND) + def test_filter_w_no_operator(self): + query = self._makeOne() + self.assertRaises(ValueError, query.filter, 'firstname', 'John') + def test_filter_w_unknown_operator(self): query = self._makeOne() self.assertRaises(ValueError, query.filter, 'firstname ~~', 'John')