Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Resource creation fails with IDatasetForm extra fields with names sorting before and after 'extra_validation' #621

Closed
wardi opened this issue Mar 12, 2013 · 11 comments
Assignees
Milestone

Comments

@wardi
Copy link
Contributor

wardi commented Mar 12, 2013

Steps to reproduce:

  1. Enable an IDatasetForm plugin that has extra fields with names that sort both before and after the string 'extra_validation', such as the one in wardi@eeb4267
  2. Create a dataset (adding resources will still work if your plugin accepts its extra fields in the package_metadata_fields.html as the one above does)
  3. Click Edit package, then Add resource
  4. Try to save the resource

What happens:

A traceback is issued with an IndexError in ckan.lib.navl.dictization_functions.flatten()

Workaround:

I wrote a patch wardi@fa99298 that avoids the problem. This fix is rather inelegant, better would be to execute the 'extra_validation' rule before or after all the other rules. I leave that decision to the experts.

@tobes
Copy link
Contributor

tobes commented Mar 13, 2013

@wardi could you replace the links so that they point to the specific file/patch you are referring to. Also if you could add the traceback in a comment on this issue that would be really helpful

thanks

@wardi
Copy link
Contributor Author

wardi commented Mar 13, 2013

@tobes links updated and:

Module ckan.controllers.package:645 in new_resource          view
>>  get_action('resource_create')(context, data)
Module ckan.logic:324 in wrapped          view
>>  return _action(context, data_dict, **kw)
Module ckan.logic.action.create:251 in resource_create          view
>>  pkg_dict = _get_action('package_update')(context, pkg_dict)
Module ckan.logic:324 in wrapped          view
>>  return _action(context, data_dict, **kw)
Module ckan.logic.action.update:251 in package_update          view
>>  data, errors = _validate(data_dict, schema, context)
Module ckan.lib.navl.dictization_functions:228 in validate          view
>>  converted_data, errors = _validate(flattened, schema, context)
Module ckan.lib.navl.dictization_functions:289 in _validate          view
>>  convert(converter, key, converted_data, errors, context)
Module ckan.lib.navl.dictization_functions:186 in convert          view
>>  converter(key, converted_data, errors, context)
Module ckan.logic.validators:306 in duplicate_extras_key          view
>>  unflattened = unflatten(data)
Module ckan.lib.navl.dictization_functions:393 in unflatten          view
>>  current_pos = current_pos[key]
IndexError: list index out of range

@wardi
Copy link
Contributor Author

wardi commented Mar 15, 2013

From the mailing list http://lists.okfn.org/pipermail/ckan-dev/2013-March/004242.html

Fabian says this applies to another issue as well

I was also able to fix my old issue: http://lists.okfn.org/pipermail/ckan-dev/2013-January/003852.html
by adding your fix to the lib.navl.dictization_functions.py validate method:

    data_no_extras = dict(converted_data)
    if ('extras',) in converted_data:
        del data_no_extras[('extras',)]
    converted_data = unflatten(data_no_extras) 

    #converted_data = unflatten(converted_data)

@tobes
Copy link
Contributor

tobes commented Mar 15, 2013

@seanh have you any thoughts on this?

@seanh
Copy link
Contributor

seanh commented Mar 15, 2013

Yes, I think I need a drink

@tobes
Copy link
Contributor

tobes commented Mar 15, 2013

:)

@seanh
Copy link
Contributor

seanh commented Mar 15, 2013

The schema is a dict so I don't know how we can make it run the "extra_validation" rule before or after all the others, unless we special-case that rule, If @wardi's fix fixes this problem and doesn't break any of the tests (either the core tests or my IDatasetForm tests) then I would be tempted to take it as-is, on the grounds that validate() is best treated as a black box.

I want to merge my pending IDatasetForm pull request before I look at this one

@ghost ghost assigned seanh Mar 19, 2013
@amercader
Copy link
Member

@seanh Assigning this to you just so you are aware of it and in case is related to the IDatasetForm stuff you are working on now. Feel free to ignore it if not.

@seanh
Copy link
Contributor

seanh commented Mar 19, 2013

I think we need to get my other idatasetform pull requests merged first, then we can see if this bug still occurs and if this fix still fixes it

@seanh
Copy link
Contributor

seanh commented Mar 22, 2013

Confirmed this is still happening on master

@seanh
Copy link
Contributor

seanh commented Mar 22, 2013

Confirmed @wardi's commit fixes the problem, and all the tests (including example_idatasetform tests) still pass with this commit. I'm tempted to just take this commit but I'll have a look after lunch and see if there's a more elegant way...

seanh pushed a commit that referenced this issue Mar 22, 2013
seanh pushed a commit that referenced this issue Apr 10, 2013
Fix a crash when generating a 'duplicate extras key' error. To trigger
the crash, post a dataset dict to package_create() containing two extras
dicts with the same key. There is no test for this yet.
seanh pushed a commit that referenced this issue Apr 10, 2013
This code path was not covered by the tests.
seanh pushed a commit that referenced this issue Apr 10, 2013
This makes it consistent with other errors that CKAN returns.
@seanh seanh closed this as completed in c76c351 Apr 16, 2013
seanh pushed a commit that referenced this issue Apr 17, 2013
This fixes an issue where creating a new resource would crash in
validation if there was a custom field (using convert_to/from_extras)
with a name that sorted after extras validation alphabetically. Fixes #621.
seanh pushed a commit that referenced this issue Apr 17, 2013
Fix a crash when generating a 'duplicate extras key' error. To trigger
the crash, post a dataset dict to package_create() containing two extras
dicts with the same key. There is no test for this yet.
seanh pushed a commit that referenced this issue Apr 17, 2013
This code path was not covered by the tests.

Conflicts:

	ckan/tests/logic/test_action.py
seanh pushed a commit that referenced this issue Apr 17, 2013
This makes it consistent with other errors that CKAN returns.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants