[#621] Do 'extras_validation' before other validation

[seanh]

Fixes #621.

[seanh]

@kindly I think you might want to review this. @wardi reported an issue (#621) that if you're using convert_to/from_extras() and you have both an extra field whose name sorts before "extras_validation" aphabetically and one that sorts after that, then after you've created a package (with a resource) if you then try to edit the package again and add a new resource to it, the duplicate_extras_key() validator crashes.

This validator tries to unflatten the dict, and it's unflatten() that crashes.

I find the validation code extremely difficult to follow, but I think the problem may have something to do with attempting to unflatten a dict that has already been partially unflattened (or had unflattened lists added to it) by convert_from_extras().

I'm not sure if the solution in this pr is the best one, wardi has an alternative solution in #621. Maybe a better solution than either is possible:

• Fix unflatten() to not crash in this case?
• It looks like validate() flattens the data dict, then duplicate_extras_key() tries to unflatten it again, maybe the unflattened dict can sometimes be passed straight into validators instead of the flattened one?

I don't really understand what the flattening is for anyway.

When a fix for this gets accepted we should add a test before merging it.

[wardi]

Does this also fix this other seemingly related issue #621 (comment)
http://lists.okfn.org/pipermail/ckan-dev/2013-March/004242.html

[tobes]

@kindly I've assigned to you for review re-assign if you wish
@seanh - should this be considered for 2.0? can you set the milestone to 2.0 or 2.1

[seanh]

@wardi Yes, I've tested Fabian's plugin (well, I copied his custom fields into ckanext/example_idatasetform, since we've completely changed IDatasetForm since he wrote that example) it still crashes on master with the same traceback as your case gets, and on this branch it succeeds.

[seanh]

I've put this into 2.0, if we can get it in in time that would be great since we know at least two users who are stuck with this issue

[wardi]

@seanh Great, then it's already better than my fix.

[seanh]

@kindly I can fix this issue using __before as you said:

diff --git a/ckan/logic/schema.py b/ckan/logic/schema.py
index 07b22b6..5158462 100644
--- a/ckan/logic/schema.py
+++ b/ckan/logic/schema.py
@@ -116,6 +116,7 @@ def default_create_tag_schema():

 def default_create_package_schema():
     schema = {
+        '__before': [duplicate_extras_key, ignore],
         'id': [empty],
         'revision_id': [ignore],
         'name': [not_empty, unicode, name_validator, package_name_validator],
@@ -139,7 +140,6 @@ def default_create_package_schema():
         'tags': default_tags_schema(),
         'tag_string': [ignore_missing, tag_string_convert],
         'extras': default_extras_schema(),
-        'extras_validation': [duplicate_extras_key, ignore],
         'save': [ignore],
         'return_to': [ignore],
         'relationships_as_object': default_relationship_schema(),

On testing manually this gets rid of the bug (which does not have an automatic test), and all the core and example_idatasetform tests still pass too.

But as you said it would (and despite the tests passing) this breaks some error messages, because 'extras_validation' is special-cased in a bunch of places:

$ ack extras_validation ckan
ckan/lib/plugins.py
374:                               'extras_validation', 'save', 'return_to',

Binary file ckan/lib/plugins.pyc matches

Binary file ckan/logic/action/__init__.pyc matches

ckan/logic/action/__init__.py
57:        elif key == 'extras_validation':

Binary file ckan/logic/__init__.pyc matches

ckan/logic/__init__.py
86:                elif key == 'extras_validation':

ckan/logic/schema.py
288:    #schema['extras_validation'] = [duplicate_extras_key, ignore]

Binary file ckan/logic/schema.pyc matches

The ones that look like they might matter are ckan/logic/action/__init__.py:

def error_summary(error_dict):
    ''' Do some i18n stuff on the error_dict keys '''

    def prettify(field_name):
        field_name = re.sub('(?<!\w)[Uu]rl(?!\w)', 'URL',
                            field_name.replace('_', ' ').capitalize())
        return _(field_name.replace('_', ' '))

    summary = {}
    for key, error in error_dict.iteritems():
        if key == 'resources':
            summary[_('Resources')] = _('Package resource(s) invalid')
        elif key == 'extras':
            summary[_('Extras')] = _('Missing Value')
        elif key == 'extras_validation':
            summary[_('Extras')] = error[0]
        else:
            summary[_(prettify(key))] = error[0]
    return summary

and the ValidationError class in ckan/logic/__init__.py:

    @property
    def error_summary(self):
        ''' autogenerate the summary if not supplied '''
        def summarise(error_dict):
            ''' Do some i18n stuff on the error_dict keys '''

            def prettify(field_name):
                field_name = re.sub('(?<!\w)[Uu]rl(?!\w)', 'URL',
                                    field_name.replace('_', ' ').capitalize())
                return _(field_name.replace('_', ' '))

            summary = {}
            for key, error in error_dict.iteritems():
                if key == 'resources':
                    summary[_('Resources')] = _('Package resource(s) invalid')
                elif key == 'extras':
                    summary[_('Extras')] = _('Missing Value')
                elif key == 'extras_validation':
                    summary[_('Extras')] = error[0]
                elif key == 'tags':
                    summary[_('Tags')] = error[0]
                else:
                    summary[_(prettify(key))] = error[0]
            return summary

        if self._error_summary:
            return self._error_summary
        return summarise(self.error_dict)

In both cases I guess I could make it look for the '__before' key instead of 'extras_validation' when creating the special error message, but then what if there's other stuff in __before that's not to do with extras?

My solution of adding an '__extras_validation' step into validate() seems like it might be the easiest way of handling this?

[kindly]

I think special casing an __extras_validation is worse then us special casing what message we show.

There is a better solution. Put extras_validation in __before, but when inserting the error into the error dict just use extras_validation key as before. So:

On this line
https://github.com/okfn/ckan/blob/master/ckan/logic/validators.py#L316
just add the hard coded key instead.

errors[('extra_validation',)] = [(_('Duplicate key "%s"') % extras_keys[0])]

This means that you will not have to change any of the summary code at all.

I think this is ok as this validator is hardly going to be reusable anyway except for this case.

[seanh]

Resubmitted as #722