[#602] Deprecate IDatasetForm's check_data_dict

With the new three-stage dataset creation form in CKAN 2.0
check_data_dict() no longer works as intended. For example,
DefaultDatasetForm's check_data_dict() breaks any IDatasetForm plugins
that are trying to use convert_to_extras() (if they inherit from
DefaultDatasetForm).

Remove the method from IDatasetForm and DefaultDatasetForm, but still
call the method in all the places it would previously have been called
_if_ the active IDatasetForm plugin has the method.

Also remove three tests that seem to be testing check_data_dict()'s
behaviours.

Note that in the existing code, check_data_dict() seems to be called
twice when creating a package, once by the package controller and once
by package_create(), and the two are inconsistent in the number of
params they expect check_data_dict() to take! On updating a package it's
called just once, by package_update(). I have preserved these "legacy"
behaviours.

Also note that IGroupForm and IOrganizationForm still have
check_data_dict() methods, we probably want to get rid of them in the
future as well.

ckan/controllers/package.py

@@ -80,7 +80,16 @@ def _db_to_form_schema(self, package_type=None):
     def _check_data_dict(self, data_dict, package_type=None):
         '''Check if the return data is correct, mostly for checking out if
         spammers are submitting only part of the form'''
-        return lookup_package_plugin(package_type).check_data_dict(data_dict)
+
+        # check_data_dict() is deprecated. If the package_plugin has a
+        # check_data_dict() we'll call it, if it doesn't have the method we'll
+        # do nothing.
+        package_plugin = lookup_package_plugin(package_type)
+        check_data_dict = getattr(package_plugin, 'check_data_dict', None)
+        if check_data_dict:
+            return check_data_dict(data_dict)
+        else:
+            return data_dict
 
     def _setup_template_variables(self, context, data_dict, package_type=None):
         return lookup_package_plugin(package_type).\

ckan/lib/plugins.py

@@ -238,26 +238,6 @@ def db_to_form_schema_options(self, options):
     def db_to_form_schema(self):
         return logic.schema.db_to_form_package_schema()
 
-    def check_data_dict(self, data_dict, schema=None):
-        '''Check for spammers submitting only part of the form.'''
-
-        # Resources might not exist yet (eg. Add Dataset)
-        surplus_keys_schema = ['__extras', '__junk', 'state', 'groups',
-                               'extras_validation', 'save', 'return_to',
-                               'resources', 'type', 'owner_org', 'private',
-                               'log_message', 'tag_string', 'tags',
-                               'url', 'version', 'extras']
-
-        if not schema:
-            schema = self.form_to_db_schema()
-        schema_keys = schema.keys()
-        keys_in_schema = set(schema_keys) - set(surplus_keys_schema)
-
-        missing_keys = keys_in_schema - set(data_dict.keys())
-        if missing_keys:
-            log.info('incorrect form fields posted, missing %s' % missing_keys)
-            raise dictization_functions.DataError(data_dict)
-
     def setup_template_variables(self, context, data_dict):
         authz_fn = logic.get_action('group_list_authz')
         c.groups_authz = authz_fn(context, data_dict)

ckan/logic/action/create.py

@@ -117,12 +117,17 @@ def package_create(context, data_dict):
     _check_access('package_create', context, data_dict)
 
     if 'api_version' not in context:
-        # old plugins do not support passing the schema so we need
-        # to ensure they still work
-        try:
-            package_plugin.check_data_dict(data_dict, schema)
-        except TypeError:
-            package_plugin.check_data_dict(data_dict)
+        # check_data_dict() is deprecated. If the package_plugin has a
+        # check_data_dict() we'll call it, if it doesn't have the method we'll
+        # do nothing.
+        check_data_dict = getattr(package_plugin, 'check_datadict', None)
+        if check_data_dict:
+            try:
+                check_data_dict(data_dict, schema)
+            except TypeError:
+                # Old plugins do not support passing the schema so we need
+                # to ensure they still work
+                package_plugin.check_data_dict(data_dict)
 
     data, errors = _validate(data_dict, schema, context)
     log.debug('package_create validate_errs=%r user=%s package=%s data=%r',

ckan/logic/action/update.py

@@ -243,12 +243,17 @@ def package_update(context, data_dict):
         schema = package_plugin.form_to_db_schema()
 
     if 'api_version' not in context:
-        # old plugins do not support passing the schema so we need
-        # to ensure they still work
-        try:
-            package_plugin.check_data_dict(data_dict, schema)
-        except TypeError:
-            package_plugin.check_data_dict(data_dict)
+        # check_data_dict() is deprecated. If the package_plugin has a
+        # check_data_dict() we'll call it, if it doesn't have the method we'll
+        # do nothing.
+        check_data_dict = getattr(package_plugin, 'check_data_dict', None)
+        if check_data_dict:
+            try:
+                package_plugin.check_data_dict(data_dict, schema)
+            except TypeError:
+                # Old plugins do not support passing the schema so we need
+                # to ensure they still work.
+                package_plugin.check_data_dict(data_dict)
 
     data, errors = _validate(data_dict, schema, context)
     log.debug('package_update validate_errs=%r user=%s package=%s data=%r',

ckan/plugins/interfaces.py

@@ -621,19 +621,6 @@ def db_to_form_schema(self):
 
         '''
 
-    def check_data_dict(self, data_dict, schema=None):
-        '''Check if the given data_dict is correct, raise DataError if not.
-
-        This function is called when a user creates or updates a dataset.  The
-        given ``data_dict`` is the dataset submitted by the user via the
-        dataset form, before it has been validated and converted by the schema
-        returned by ``form_to_db_schema()`` above.
-
-        :raises ckan.lib.navl.dictization_functions.DataError: if the given
-          ``data_dict`` is not correct
-
-        '''
-
     def setup_template_variables(self, context, data_dict):
         '''Add variables to the template context for use in templates.
 

ckan/tests/functional/test_package.py

@@ -745,18 +745,6 @@ def test_edit_2_tags_and_groups(self):
         assert rev.author == self.admin.name
         assert rev.message == exp_log_message
 
-    def test_missing_fields(self):
-        # User edits and a field is left out in the commit parameters.
-        # (Spammers can cause this)
-        fv = self.res.forms['dataset-edit']
-        del fv.fields['notes']
-        res = fv.submit('save', status=400, extra_environ=self.extra_environ_admin)
-
-        fv = self.res.forms['dataset-edit']
-        prefix = ''
-        del fv.fields[prefix + 'license_id']
-        res = fv.submit('save', status=400, extra_environ=self.extra_environ_admin)
-
     def test_redirect_after_edit_using_param(self):
         return_url = 'http://random.site.com/dataset/<NAME>?test=param'
         # It's useful to know that this url encodes to:
@@ -1201,12 +1189,6 @@ def test_new_no_name(self):
         assert 'URL: Missing value' in res, res
         self._assert_form_errors(res)
 
-    def test_new_bad_param(self):
-        offset = url_for(controller='package', action='new', __bad_parameter='value')
-        res = self.app.post(offset, {'save':'1'},
-                            status=400, extra_environ=self.extra_environ_tester)
-        assert 'Integrity Error' in res.body
-
     def test_redirect_after_new_using_param(self):
         return_url = 'http://random.site.com/dataset/<NAME>?test=param'
         # It's useful to know that this url encodes to:
@@ -1313,31 +1295,6 @@ def test_new_existing_name(self):
         assert 'That URL is already in use.' in res, res
         self._assert_form_errors(res)
 
-    def test_missing_fields(self):
-        # A field is left out in the commit parameters.
-        # (Spammers can cause this)
-        offset = url_for(controller='package', action='new')
-        res = self.app.get(offset, extra_environ=self.extra_environ_tester)
-        assert 'Add - Datasets' in res, res
-        prefix = ''
-        fv = res.forms['dataset-edit']
-        fv[prefix + 'name'] = 'anything'
-        del fv.fields['notes']
-        self.pkg_names.append('anything')
-        res = fv.submit('save', status=400, extra_environ=self.extra_environ_tester)
-
-        offset = url_for(controller='package', action='new')
-        res = self.app.get(offset, extra_environ=self.extra_environ_tester)
-        assert 'Add - Datasets' in res
-        fv = res.forms['dataset-edit']
-        fv[prefix + 'name'] = 'anything'
-        prefix = ''
-        del fv.fields[prefix + 'notes']
-        # NOTE Missing dropdowns fields don't cause KeyError in
-        # _serialized_value so don't register as an error here like
-        # text field tested here.
-        res = fv.submit('save', status=400, extra_environ=self.extra_environ_tester)
-
     def test_new_plugin_hook(self):
         plugin = MockPackageControllerPlugin()
         plugins.load(plugin)

ckanext/example_idatasetform/plugin.py

@@ -154,10 +154,7 @@ def package_form(self):
         ExampleIDatasetFormPlugin.num_times_package_form_called += 1
         return lib_plugins.DefaultDatasetForm.package_form(self)
 
+    # check_data_dict() is deprecated, this method is only here to test that
+    # legacy support for the deprecated method works.
     def check_data_dict(self, data_dict, schema=None):
         ExampleIDatasetFormPlugin.num_times_check_data_dict_called += 1
-        # Disable DefaultDatasetForm's check_data_dict(), because it breaks
-        # with the new three-stage dataset creation when using
-        # convert_to_extras.
-        #return lib_plugins.DefaultDatasetForm.check_data_dict(self, data_dict,
-        #        schema)