[#1725] Move sort validation to datastore_validate()

ckanext/datastore/db.py

@@ -83,19 +83,6 @@ def _is_valid_table_name(name):
     return _is_valid_field_name(name)
 
 
-def _validate_int(i, field_name, non_negative=False):
-    try:
-        i = int(i)
-    except ValueError:
-        raise ValidationError({
-            field_name: ['{0} is not an integer'.format(i)]
-        })
-    if non_negative and i < 0:
-        raise ValidationError({
-            field_name: ['{0} is not a non-negative integer'.format(i)]
-        })
-
-
 def _get_engine(data_dict):
     '''Get either read or write engine.'''
     connection_url = data_dict['connection_url']
@@ -823,6 +810,9 @@ def validate(context, data_dict):
     if 'fields' in data_dict_copy:
         fields = datastore_helpers.get_list(data_dict_copy['fields'])
         data_dict_copy['fields'] = fields
+    if 'sort' in data_dict_copy:
+        fields = datastore_helpers.get_list(data_dict_copy['sort'], False)
+        data_dict_copy['sort'] = fields
 
     for plugin in p.PluginImplementations(interfaces.IDatastore):
         data_dict_copy = plugin.datastore_validate(context,
@@ -832,19 +822,23 @@ def validate(context, data_dict):
     # Remove default elements in data_dict
     del data_dict_copy['connection_url']
     del data_dict_copy['resource_id']
+    data_dict_copy.pop('id', None)
 
     for key, values in data_dict_copy.iteritems():
         if not values:
             continue
-        if key == 'fields':
-            raise ValidationError({
-                'fields': [u'field "{0}" not in table'.format(values[0])]
-            })
-        elif key == 'filters':
-            the_filter = values.keys()[0]
-            raise ValidationError({
-                'filters': [u'filter "{0}" not in table'.format(the_filter)]
-            })
+        if isinstance(values, basestring):
+            value = values
+        elif isinstance(values, (list, tuple)):
+            value = values[0]
+        elif isinstance(values, dict):
+            value = values.keys()[0]
+        else:
+            value = values
+
+        raise ValidationError({
+            key: [u'invalid value "{0}"'.format(value)]
+        })
 
     return True
 
@@ -879,10 +873,6 @@ def search_data(context, data_dict):
     else:
         sort_clause = ''
 
-    # FIXME: Limit could be ALL
-    _validate_int(limit, 'limit', non_negative=True)
-    _validate_int(offset, 'offset', non_negative=True)
-
     sql_string = u'''SELECT {select}
                     FROM "{resource}" {ts_query}
                     {where} {sort} LIMIT {limit} OFFSET {offset}'''.format(

ckanext/datastore/helpers.py

@@ -21,6 +21,14 @@ def is_single_statement(sql):
     return len(sqlparse.split(sql)) <= 1
 
 
+def validate_int(i, non_negative=False):
+    try:
+        i = int(i)
+    except ValueError:
+        return False
+    return i >= 0 or not non_negative
+
+
 def _strip(input):
     if isinstance(input, basestring) and len(input) and input[0] == input[-1]:
         return input.strip().strip('"')

ckanext/datastore/logic/action.py

@@ -114,7 +114,9 @@ def datastore_create(context, data_dict):
             res['url_type'] = 'datastore'
             p.toolkit.get_action('resource_update')(context, res)
     else:
-        _check_read_only(context, data_dict)
+        if not data_dict.pop('force', False):
+            resource_id = data_dict['resource_id']
+            _check_read_only(context, resource_id)
 
     data_dict['connection_url'] = pylons.config['ckan.datastore.write_url']
 
@@ -186,7 +188,9 @@ def datastore_upsert(context, data_dict):
 
     p.toolkit.check_access('datastore_upsert', context, data_dict)
 
-    _check_read_only(context, data_dict)
+    if not data_dict.pop('force', False):
+        resource_id = data_dict['resource_id']
+        _check_read_only(context, resource_id)
 
     data_dict['connection_url'] = pylons.config['ckan.datastore.write_url']
 
@@ -234,7 +238,9 @@ def datastore_delete(context, data_dict):
 
     p.toolkit.check_access('datastore_delete', context, data_dict)
 
-    _check_read_only(context, data_dict)
+    if not data_dict.pop('force', False):
+        resource_id = data_dict['resource_id']
+        _check_read_only(context, resource_id)
 
     data_dict['connection_url'] = pylons.config['ckan.datastore.write_url']
 
@@ -455,14 +461,12 @@ def _resource_exists(context, data_dict):
     return results.rowcount > 0
 
 
-def _check_read_only(context, data_dict):
+def _check_read_only(context, resource_id):
     ''' Raises exception if the resource is read-only.
     Make sure the resource id is in resource_id
     '''
-    if data_dict.get('force'):
-        return
     res = p.toolkit.get_action('resource_show')(
-        context, {'id': data_dict['resource_id']})
+        context, {'id': resource_id})
     if res.get('url_type') != 'datastore':
         raise p.toolkit.ValidationError({
             'read-only': ['Cannot edit read-only resource. Either pass'

ckanext/datastore/plugin.py

@@ -281,8 +281,64 @@ def datastore_validate(self, context, data_dict, all_field_ids):
             if key in all_field_ids:
                 del filters[key]
 
+        q = data_dict.get('q')
+        if q:
+            if isinstance(q, basestring):
+                del data_dict['q']
+
+        language = data_dict.get('language')
+        if language:
+            if isinstance(language, basestring):
+                del data_dict['language']
+
+        plain = data_dict.get('plain')
+        if plain:
+            if isinstance(plain, bool):
+                del data_dict['plain']
+
+        sort_clauses = data_dict.get('sort')
+        if sort_clauses:
+            invalid_clauses = [c for c in sort_clauses
+                               if not self._is_valid_sort(c, all_field_ids)]
+            data_dict['sort'] = invalid_clauses
+
+        limit = data_dict.get('limit')
+        if limit:
+            is_positive_int = datastore_helpers.validate_int(limit,
+                                                             non_negative=True)
+            is_all = isinstance(limit, basestring) and limit.lower() == 'all'
+            if is_positive_int or is_all:
+                del data_dict['limit']
+
+        offset = data_dict.get('offset')
+        if offset:
+            is_positive_int = datastore_helpers.validate_int(offset,
+                                                             non_negative=True)
+            if is_positive_int:
+                del data_dict['offset']
+
         return data_dict
 
+    def _is_valid_sort(self, clause, all_field_ids):
+        clause = clause.encode('utf-8')
+        clause_parts = shlex.split(clause)
+
+        if len(clause_parts) == 1:
+            field, sort = clause_parts[0], 'asc'
+        elif len(clause_parts) == 2:
+            field, sort = clause_parts
+        else:
+            return False
+
+        field, sort = unicode(field, 'utf-8'), unicode(sort, 'utf-8')
+
+        if field not in all_field_ids:
+            return False
+        if sort.lower() not in ('asc', 'desc'):
+            return False
+
+        return True
+
     def datastore_delete(self, context, data_dict, all_field_ids, query_dict):
         query_dict['where'] += self._where(data_dict, all_field_ids)
         return query_dict
@@ -362,23 +418,9 @@ def _sort(self, data_dict, field_ids):
                 field, sort = clause_parts[0], 'asc'
             elif len(clause_parts) == 2:
                 field, sort = clause_parts
-            else:
-                raise ValidationError({
-                    'sort': ['not valid syntax for sort clause']
-                })
+
             field, sort = unicode(field, 'utf-8'), unicode(sort, 'utf-8')
 
-            if field not in field_ids:
-                raise ValidationError({
-                    'sort': [u'field "{0}" not in table'.format(
-                        field)]
-                })
-            if sort.lower() not in ('asc', 'desc'):
-                raise ValidationError({
-                    'sort': ['sorting can only be asc or desc']
-                })
-            clause_parsed.append(u'"{0}" {1}'.format(
-                field, sort)
-            )
+            clause_parsed.append(u'"{0}" {1}'.format(field, sort))
 
         return clause_parsed

ckanext/datastore/tests/test_search.py

@@ -13,11 +13,11 @@
 import ckanext.datastore.db as db
 from ckanext.datastore.tests.helpers import extract, rebuild_all_dbs
 
-import ckan.new_tests.factories as factories
 import ckan.new_tests.helpers as helpers
 
 assert_equals = nose.tools.assert_equals
 assert_raises = nose.tools.assert_raises
+assert_in = nose.tools.assert_in
 
 
 class TestDatastoreSearch(tests.WsgiAppCase):
@@ -281,7 +281,7 @@ def test_search_invalid(self):
                             extra_environ=auth, status=409)
         res_dict = json.loads(res.body)
         assert res_dict['success'] is False
-        assert res_dict['error']['sort'][0] == u'field "f\xfc\xfc" not in table'
+        assert_in(u'f\xfc\xfc', res_dict['error']['sort'][0])
 
     def test_search_limit(self):
         data = {'resource_id': self.data['resource_id'],