Changing Query.filter to ask for operator and prop. name separately.

Also adding a test for property names with spaces in them.

Fixes googleapis#424.

gcloud/datastore/connection.py

@@ -250,7 +250,8 @@ def run_query(self, dataset_id, query_pb, namespace=None):
         >>> from gcloud import datastore
         >>> connection = datastore.get_connection()
         >>> dataset = connection.dataset('dataset-id')
-        >>> query = dataset.query().kind('MyKind').filter('property =', 'val')
+        >>> query = dataset.query().kind('MyKind').filter(
+        ...     'property', '=', 'val')
 
         Using the `fetch`` method...
 

gcloud/datastore/demo/demo.py

@@ -46,11 +46,11 @@
 print(query.limit(2).fetch())
 
 # Now let's check for Thing entities named 'Computer'
-print(query.filter('name =', 'Computer').fetch())
+print(query.filter('name', '=', 'Computer').fetch())
 
 # If you want to filter by multiple attributes,
 # you can string .filter() calls together.
-print(query.filter('name =', 'Computer').filter('age =', 10).fetch())
+print(query.filter('name', '=', 'Computer').filter('age', '=', 10).fetch())
 
 # You can also work inside a transaction.
 # (Check the official docs for explanations of what's happening here.)

gcloud/datastore/query.py

@@ -104,58 +104,51 @@ def to_protobuf(self):
         """
         return self._pb
 
-    def filter(self, expression, value):
-        """Filter the query based on an expression and a value.
+    def filter(self, property_name, operator, value):
+        """Filter the query based on a property name, operator and a value.
 
         This will return a clone of the current :class:`Query`
         filtered by the expression and value provided.
 
         Expressions take the form of::
 
-          .filter('<property> <operator>', <value>)
+          .filter('<property>', '<operator>', <value>)
 
         where property is a property stored on the entity in the datastore
         and operator is one of ``OPERATORS``
         (ie, ``=``, ``<``, ``<=``, ``>``, ``>=``)::
 
           >>> query = Query('Person')
-          >>> filtered_query = query.filter('name =', 'James')
-          >>> filtered_query = query.filter('age >', 50)
+          >>> filtered_query = query.filter('name', '=', 'James')
+          >>> filtered_query = query.filter('age', '>', 50)
 
         Because each call to ``.filter()`` returns a cloned ``Query`` object
         we are able to string these together::
 
           >>> query = Query('Person').filter(
-          ...     'name =', 'James').filter('age >', 50)
+          ...     'name', '=', 'James').filter('age', '>', 50)
 
-        :type expression: string
-        :param expression: An expression of a property and an
-                           operator (ie, ``=``).
+        :type property_name: string
+        :param property_name: A property name.
+
+        :type operator: string
+        :param operator: One of ``=``, ``<``, ``<=``, ``>``, ``>=``.
 
         :type value: integer, string, boolean, float, None, datetime
         :param value: The value to filter on.
 
         :rtype: :class:`Query`
         :returns: A Query filtered by the expression and value provided.
+        :raises: `ValueError` if `operation` is not one of the specified
+                 values.
         """
         clone = self._clone()
 
-        # Take an expression like 'property >=', and parse it into
-        # useful pieces.
-        property_name, operator = None, None
-        expression = expression.strip()
-
-        # 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 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)
+            error_message = 'Invalid expression: "%s"' % (operator,)
+            choices_message = 'Please use one of: =, <, <=, >, >=.'
+            raise ValueError(error_message, choices_message)
 
         # Build a composite filter AND'd together.
         composite_filter = clone._pb.filter.composite_filter
@@ -321,7 +314,7 @@ def fetch(self, limit=None):
 
           >>> from gcloud import datastore
           >>> dataset = datastore.get_dataset('dataset-id')
-          >>> query = dataset.query('Person').filter('name =', 'Sally')
+          >>> query = dataset.query('Person').filter('name', '=', 'Sally')
           >>> query.fetch()
           [<Entity object>, <Entity object>, ...]
           >>> query.fetch(1)

gcloud/datastore/test_query.py

@@ -80,19 +80,15 @@ 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')
+        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')
+        after = query.filter('firstname', '=', u'John')
         self.assertFalse(after is query)
         self.assertTrue(isinstance(after, self._getTargetClass()))
         q_pb = after.to_protobuf()
@@ -108,11 +104,11 @@ 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 = 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 = [
@@ -139,7 +135,7 @@ def test_filter_w_known_operator_and_entity(self):
         other = Entity()
         other['firstname'] = u'John'
         other['lastname'] = u'Smith'
-        after = query.filter('other =', other)
+        after = query.filter('other', '=', other)
         self.assertFalse(after is query)
         self.assertTrue(isinstance(after, self._getTargetClass()))
         q_pb = after.to_protobuf()
@@ -155,6 +151,23 @@ def test_filter_w_known_operator_and_entity(self):
         self.assertEqual(props[1].name, 'lastname')
         self.assertEqual(props[1].value.string_value, u'Smith')
 
+    def test_filter_w_whitespace_property_name(self):
+        from gcloud.datastore import datastore_v1_pb2 as datastore_pb
+
+        query = self._makeOne()
+        PROPERTY_NAME = '  property with lots of space '
+        after = query.filter(PROPERTY_NAME, '=', 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,
+                         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, PROPERTY_NAME)
+        self.assertEqual(p_pb.value.string_value, u'John')
+        self.assertEqual(p_pb.operator, datastore_pb.PropertyFilter.EQUAL)
+
     def test_ancestor_w_non_key_non_list(self):
         query = self._makeOne()
         self.assertRaises(TypeError, query.ancestor, object())
@@ -165,7 +178,7 @@ def test_ancestor_wo_existing_ancestor_query_w_key_and_propfilter(self):
         _ID = 123
         _NAME = u'NAME'
         key = Key(path=[{'kind': _KIND, 'id': _ID}])
-        query = self._makeOne().filter('name =', _NAME)
+        query = self._makeOne().filter('name', '=', _NAME)
         after = query.ancestor(key)
         self.assertFalse(after is query)
         self.assertTrue(isinstance(after, self._getTargetClass()))
@@ -226,7 +239,7 @@ def test_ancestor_clears_existing_ancestor_query_w_others(self):
         _KIND = 'KIND'
         _ID = 123
         _NAME = u'NAME'
-        query = self._makeOne().filter('name =', _NAME)
+        query = self._makeOne().filter('name', '=', _NAME)
         between = query.ancestor([_KIND, _ID])
         after = between.ancestor(None)
         self.assertFalse(after is query)

regression/datastore.py

@@ -155,7 +155,7 @@ def test_save_key_self_reference(self):
         self.case_entities_to_delete.append(entity)
 
         query = self.dataset.query('Person').filter(
-            'linkedTo =', key).limit(2)
+            'linkedTo', '=', key).limit(2)
 
         stored_persons = query.fetch()
         self.assertEqual(len(stored_persons), 1)
@@ -199,15 +199,15 @@ def test_limit_queries(self):
         self.assertEqual(len(new_character_entities), characters_remaining)
 
     def test_query_simple_filter(self):
-        query = self._base_query().filter('appearances >=', 20)
+        query = self._base_query().filter('appearances', '>=', 20)
         expected_matches = 6
         # We expect 6, but allow the query to get 1 extra.
         entities = query.fetch(limit=expected_matches + 1)
         self.assertEqual(len(entities), expected_matches)
 
     def test_query_multiple_filters(self):
         query = self._base_query().filter(
-            'appearances >=', 26).filter('family =', 'Stark')
+            'appearances', '>=', 26).filter('family', '=', 'Stark')
         expected_matches = 4
         # We expect 4, but allow the query to get 1 extra.
         entities = query.fetch(limit=expected_matches + 1)
@@ -225,7 +225,7 @@ def test_query___key___filter(self):
         rickard_key = datastore.key.Key(
             path=[populate_datastore.ANCESTOR, populate_datastore.RICKARD])
 
-        query = self._base_query().filter('__key__ =', rickard_key)
+        query = self._base_query().filter('__key__', '=', rickard_key)
         expected_matches = 1
         # We expect 1, but allow the query to get 1 extra.
         entities = query.fetch(limit=expected_matches + 1)