@glassrc review

cliquet/resource/__init__.py

@@ -880,6 +880,12 @@ def _extract_filters(self, queryparams=None):
         for param, paramvalue in queryparams.items():
             param = param.strip()
 
+            error_details = {
+                'name': param,
+                'location': 'querystring',
+                'description': 'Invalid value for %s' % param
+            }
+
             # Ignore specific fields
             if param.startswith('_') and param not in ('_since',
                                                        '_to',
@@ -891,11 +897,6 @@ def _extract_filters(self, queryparams=None):
                 value = native_value(paramvalue.strip('"'))
 
                 if not isinstance(value, six.integer_types):
-                    error_details = {
-                        'name': param,
-                        'location': 'querystring',
-                        'description': 'Invalid value for %s' % param
-                    }
                     raise_invalid(self.request, **error_details)
 
                 if param == '_since':
@@ -921,16 +922,25 @@ def _extract_filters(self, queryparams=None):
                 operator, field = COMPARISON.EQ, param
 
             if not self.is_known_field(field):
-                error_details = {
-                    'location': 'querystring',
-                    'description': "Unknown filter field '{0}'".format(param)
-                }
+                error_msg = "Unknown filter field '{0}'".format(param)
+                error_details['description'] = error_msg
                 raise_invalid(self.request, **error_details)
 
             value = native_value(paramvalue)
             if operator in (COMPARISON.IN, COMPARISON.EXCLUDE):
                 value = set([native_value(v) for v in paramvalue.split(',')])
 
+                all_integers = all([isinstance(v, six.integer_types)
+                                    for v in value])
+                all_strings = all([isinstance(v, six.text_type)
+                                   for v in value])
+                has_invalid_value = (
+                    (field == self.model.id_field and not all_strings) or
+                    (field == self.model.modified_field and not all_integers)
+                )
+                if has_invalid_value:
+                    raise_invalid(self.request, **error_details)
+
             filters.append(Filter(field, value, operator))
 
         return filters

cliquet/storage/postgresql/__init__.py

@@ -601,12 +601,6 @@ def _format_conditions(self, filters, id_field, modified_field,
                     value = json.dumps(filtr.value).strip('"')
             else:
                 value = tuple(value)
-                if filtr.field == id_field:
-                    value = tuple([v if isinstance(v, six.string_types)
-                                   else None for v in value])
-                if filtr.field == modified_field:
-                    value = tuple([v if isinstance(v, six.integer_types)
-                                   else None for v in value])
 
             # Safely escape value
             value_holder = '%s_value_%s' % (prefix, i)

cliquet/tests/resource/test_filter.py

@@ -71,12 +71,12 @@ def test_filter_errors_are_json_formatted(self):
             error = e
         self.assertEqual(error.json, {
             'errno': ERRORS.INVALID_PARAMETERS.value,
-            'message': "querystring: Unknown filter field 'foo'",
+            'message': "Unknown filter field 'foo'",
             'code': 400,
             'error': 'Invalid parameters',
             'details': [{'description': "Unknown filter field 'foo'",
                          'location': 'querystring',
-                         'name': None}]})
+                         'name': 'foo'}]})
 
     def test_regexp_is_strict_for_min_and_max(self):
         self.patch_known_field.stop()
@@ -148,3 +148,19 @@ def test_exclude_values(self):
         result = self.resource.collection_get()
         values = [item['status'] for item in result['data']]
         self.assertEqual(sorted(values), [1, 1, 2, 2])
+
+    def test_include_returns_400_if_value_has_wrong_type(self):
+        self.resource.request.GET = {'in_id': '0,1'}
+        self.assertRaises(httpexceptions.HTTPBadRequest,
+                          self.resource.collection_get)
+        self.resource.request.GET = {'in_last_modified': 'a,b'}
+        self.assertRaises(httpexceptions.HTTPBadRequest,
+                          self.resource.collection_get)
+
+    def test_exclude_returns_400_if_value_has_wrong_type(self):
+        self.resource.request.GET = {'exclude_id': '0,1'}
+        self.assertRaises(httpexceptions.HTTPBadRequest,
+                          self.resource.collection_get)
+        self.resource.request.GET = {'exclude_last_modified': 'a,b'}
+        self.assertRaises(httpexceptions.HTTPBadRequest,
+                          self.resource.collection_get)

cliquet/tests/test_storage.py

@@ -369,18 +369,6 @@ def test_get_all_can_filter_with_list_of_values(self):
                                           **self.storage_kw)
         self.assertEqual(len(records), 2)
 
-    def test_get_all_returns_no_result_if_list_of_values_has_wrong_type(self):
-        for l in range(3):
-            self.create_record({'code': 'a'})
-        filters = [Filter('id', [1, 2], utils.COMPARISON.IN)]
-        records, _ = self.storage.get_all(filters=filters,
-                                          **self.storage_kw)
-        self.assertEqual(len(records), 0)
-        filters = [Filter('last_modified', ['x', 'y'], utils.COMPARISON.IN)]
-        records, _ = self.storage.get_all(filters=filters,
-                                          **self.storage_kw)
-        self.assertEqual(len(records), 0)
-
     def test_get_all_can_filter_with_numeric_values(self):
         for l in [1, 10, 6, 46]:
             self.create_record({'code': l})