Merge pull request #7697 from ckan/7683-invalid-error-invalid

datastore_upsert, datastore_create error fixes

changes/7683.bugfix

@@ -0,0 +1,4 @@
+datastore_upsert, datastore_create error fixes
+
+datastore_upsert method=insert: prevent 500 on invalid data datastore_create
+datastore_create: invalid data errors now reported against records value (not "message")

ckanext/datastore/backend/__init__.py

@@ -53,16 +53,6 @@ class DatastoreException(Exception):
     pass
 
 
-class InvalidDataError(Exception):
-    """Exception that's raised if you try to add invalid data to the datastore.
-
-    For example if you have a column with type "numeric" and then you try to
-    add a non-numeric value like "foo" to it, this exception should be raised.
-
-    """
-    pass
-
-
 class DatastoreBackend:
     """Base class for all datastore backends.
 

ckanext/datastore/backend/postgres.py

@@ -36,7 +36,7 @@
 from psycopg2.extras import register_default_json, register_composite
 import distutils.version
 from sqlalchemy.exc import (ProgrammingError, IntegrityError,
-                            DBAPIError, DataError)
+                            DBAPIError, DataError, DatabaseError)
 
 import ckan.plugins as plugins
 from ckan.common import CKANConfig, config
@@ -46,7 +46,6 @@
     DatastoreException,
     _parse_sort_clause
 )
-from ckanext.datastore.backend import InvalidDataError
 
 log = logging.getLogger(__name__)
 
@@ -1108,11 +1107,6 @@ def alter_table(context: Context, data_dict: dict[str, Any]):
 
 
 def insert_data(context: Context, data_dict: dict[str, Any]):
-    """
-
-    :raises InvalidDataError: if there is an invalid value in the given data
-
-    """
     data_dict['method'] = _INSERT
     result = upsert_data(context, data_dict)
     return result
@@ -1156,11 +1150,7 @@ def upsert_data(context: Context, data_dict: dict[str, Any]):
 
         try:
             context['connection'].execute(sql_string, rows)
-        except sqlalchemy.exc.DataError as err:
-            raise InvalidDataError(
-                toolkit._("The data was invalid: {}"
-                          ).format(_programming_error_summary(err)))
-        except sqlalchemy.exc.DatabaseError as err:
+        except (DatabaseError, DataError) as err:
             raise ValidationError(
                 {u'records': [_programming_error_summary(err)]})
 
@@ -2007,8 +1997,6 @@ def create(self, context: Context, data_dict: dict[str, Any]):
         nor can the ordering of them be changed. They can be extended though.
         Any error results in total failure! For now pass back the actual error.
         Should be transactional.
-        :raises InvalidDataError: if there is an invalid value in the given
-                                  data
         '''
         engine = get_write_engine()
         context['connection'] = engine.connect()

ckanext/datastore/logic/action.py

@@ -15,9 +15,7 @@
 import ckan.plugins as p
 import ckanext.datastore.logic.schema as dsschema
 import ckanext.datastore.helpers as datastore_helpers
-from ckanext.datastore.backend import (
-    DatastoreBackend, InvalidDataError
-)
+from ckanext.datastore.backend import DatastoreBackend
 from ckanext.datastore.backend.postgres import identifier
 
 log = logging.getLogger(__name__)
@@ -156,10 +154,7 @@ def datastore_create(context: Context, data_dict: dict[str, Any]):
                 'alias': [u'"{0}" is not a valid alias name'.format(alias)]
             })
 
-    try:
-        result = backend.create(context, data_dict)
-    except InvalidDataError as err:
-        raise p.toolkit.ValidationError({'message': str(err)})
+    result = backend.create(context, data_dict)
 
     if data_dict.get('calculate_record_count', False):
         backend.calculate_record_count(data_dict['resource_id'])  # type: ignore

ckanext/datastore/tests/test_create.py

@@ -1180,7 +1180,7 @@ def test_datastore_create_with_invalid_data_value(self, app):
 
         assert res_dict["success"] is False
         assert res_dict["error"]["__type"] == "Validation Error"
-        assert res_dict["error"]["message"].startswith("The data was invalid")
+        assert 'invalid input syntax for type numeric' in str(res_dict["error"])
 
 
 

ckanext/datastore/tests/test_db.py

@@ -2,7 +2,6 @@
 
 import unittest.mock as mock
 import pytest
-import sqlalchemy.exc
 
 import ckan.lib.jobs as jobs
 import ckan.plugins as p
@@ -128,44 +127,6 @@ def _assert_created_index_on(
         )
 
 
-@mock.patch("ckanext.datastore.backend.postgres._get_fields")
-def test_upsert_with_insert_method_and_invalid_data(mock_get_fields_function):
-    """upsert_data() should raise InvalidDataError if given invalid data.
-
-    If the type of a field is numeric and upsert_data() is given a whitespace
-    value like "   ", it should raise DataError.
-
-    In this case we're testing with "method": "insert" in the data_dict.
-
-    """
-    mock_connection = mock.Mock()
-    mock_connection.execute.side_effect = sqlalchemy.exc.DataError(
-        "statement", "params", "orig", connection_invalidated=False
-    )
-
-    context = {"connection": mock_connection}
-    data_dict = {
-        "fields": [{"id": "value", "type": "numeric"}],
-        "records": [
-            {"value": 0},
-            {"value": 1},
-            {"value": 2},
-            {"value": 3},
-            {"value": "   "},  # Invalid numeric value.
-            {"value": 5},
-            {"value": 6},
-            {"value": 7},
-        ],
-        "method": "insert",
-        "resource_id": "fake-resource-id",
-    }
-
-    mock_get_fields_function.return_value = data_dict["fields"]
-
-    with pytest.raises(backend.InvalidDataError):
-        db.upsert_data(context, data_dict)
-
-
 class TestGetAllResourcesIdsInDatastore(object):
     @pytest.mark.ckan_config(u"ckan.plugins", u"datastore")
     @pytest.mark.usefixtures(u"with_plugins", u"clean_db")

ckanext/datastore/tests/test_upsert.py

@@ -780,6 +780,32 @@ def test_empty_string_instead_of_null(self):
         rec = search_result["records"][0]
         assert rec == {'_id': 1, 'pk': '1000', 'n': None, 'd': None}
 
+    @pytest.mark.ckan_config("ckan.plugins", "datastore")
+    @pytest.mark.usefixtures("clean_datastore", "with_plugins")
+    def test_insert_wrong_type(self):
+        resource = factories.Resource()
+        data = {
+            "resource_id": resource["id"],
+            "force": True,
+            "fields": [
+                {"id": "num", "type": "int"},
+            ],
+        }
+        helpers.call_action("datastore_create", **data)
+
+        data = {
+            "resource_id": resource["id"],
+            "force": True,
+            "method": "insert",
+            "records": [
+                {"num": "notanumber"}
+            ],
+        }
+
+        with pytest.raises(ValidationError) as context:
+            helpers.call_action("datastore_upsert", **data)
+        assert u'invalid input syntax for integer: "notanumber"' in str(context.value)
+
 
 class TestDatastoreUpdate(object):
     # Test action 'datastore_upsert' with 'method': 'update'