Query with invalid paramter should raise an error

CHANGES.rst

@@ -4,6 +4,17 @@ Changelog
 4.5 (unreleased)
 ----------------
 
+Backwards incompatible changes
+++++++++++++++++++++++++++++++
+
+- Raise a ``ValueError`` if a query uses invalid index parameters. This
+  prevents the query from being changed without feedback to the user and
+  delivering implausible search results.
+  (`#67 <https://github.com/zopefoundation/Products.ZCatalog/pull/67>`_)
+
+Bug fixes
++++++++++
+
 - Fix rewriting of query to avoid wrong optimization of CompositeIndex.
   (`#59 <https://github.com/zopefoundation/Products.ZCatalog/issues/59>`_)
 

setup.py

@@ -16,7 +16,7 @@
 
 setup(
     name='Products.ZCatalog',
-    version='4.5.dev0',
+    version='5.0.dev0',
     url='https://github.com/zopefoundation/Products.ZCatalog',
     license='ZPL 2.1',
     description="Zope's indexing and search solution.",

src/Products/PluginIndexes/FieldIndex/tests.py

@@ -256,6 +256,13 @@ def testRange(self):
         self._checkApply(record, expect)
 
         # Make sure that range tests with incompatible paramters
-        # don't return empty sets.
+        # raise a RuntimeError
         record['foo']['operator'] = 'and'
-        self._checkApply(record, expect)
+        self.assertRaises(ValueError, self._checkApply, record, expect)
+
+        # alternative syntax of record
+        record = {'foo': [-99, 3],
+                  'foo_range': 'min:max',
+                  'foo_operator': 'and'}
+
+        self.assertRaises(ValueError, self._checkApply, record, expect)

src/Products/ZCatalog/query.py

@@ -56,6 +56,8 @@ def __init__(self, request, iid, options=(), operators=('or', 'and'),
         self.id = iid
         self.operators = operators
         self.operator = default_operator
+        self.options = options
+
         if iid not in request:
             self.keys = None
             return
@@ -71,12 +73,11 @@ def __init__(self, request, iid, options=(), operators=('or', 'and'),
             else:
                 keys = [query]
 
-            for op in options:
+            for op in param.keys():
                 if op == 'query':
                     continue
 
-                if op in param:
-                    self.set(op, param[op])
+                self.set(op, param[op])
 
         else:
             # query is tuple, list, string, number, or something else
@@ -85,9 +86,10 @@ def __init__(self, request, iid, options=(), operators=('or', 'and'),
             else:
                 keys = [param]
 
-            for op in options:
-                field = iid + "_" + op
-                if field in request:
+            for field in request.keys():
+                if field.startswith(iid + '_'):
+                    iid_tmp, op = field.split('_')
+
                     self.set(op, request[field])
 
         self.keys = keys
@@ -103,10 +105,12 @@ def operator(self):
 
     @operator.setter
     def operator(self, value):
+        iid = self.id
         value = value.lower()
         if value not in self.operators:
-            raise RuntimeError('operator not valid: %r' % value)
-        self._operator = value.lower()
+            raise ValueError(('index {0!r}: operator {1!r}'
+                              ' is not valid').format(iid, value))
+        self._operator = value
 
     def get(self, key, default_v=None):
         value = getattr(self, key, _marker)
@@ -115,4 +119,8 @@ def get(self, key, default_v=None):
         return default_v
 
     def set(self, key, value):
-        setattr(self, key, value)
+        if key in self.options:
+            setattr(self, key, value)
+        else:
+            raise ValueError(('index {0!r}: option {1!r}'
+                              ' is not valid').format(self.id, key))

src/Products/ZCatalog/tests/test_query.py

@@ -66,3 +66,35 @@ def test_get_not_int(self):
         parser = self._makeOne(request, 'path', ('query', 'not'))
         self.assertEqual(parser.get('keys'), ['foo'])
         self.assertEqual(parser.get('not'), [0])
+
+    def test_operator_dict(self):
+        request = {'path': {'query': 'foo', 'operator': 'bar'}}
+        self.assertRaises(ValueError,
+                          self._makeOne,
+                          request,
+                          'path',
+                          ('query', 'operator'),
+                          ('or', 'and'))
+
+    def test_operator_string(self):
+        request = {'path': 'foo', 'path_operator': 'bar'}
+        self.assertRaises(ValueError,
+                          self._makeOne,
+                          request, 'path',
+                          ('query', 'operator'),
+                          ('or', 'and'))
+
+    def test_options_dict(self):
+        request = {'path': {'query': 'foo', 'baropt': 'dummy'}}
+        self.assertRaises(ValueError,
+                          self._makeOne,
+                          request,
+                          'path',
+                          ('query', 'operator'))
+
+    def test_options_string(self):
+        request = {'path': 'foo', 'path_baropt': 'dummy'}
+        self.assertRaises(ValueError,
+                          self._makeOne,
+                          request, 'path',
+                          ('query', 'operator'))