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

Crash (500) when showing a package with no resources (IDatasetForm) #403

Closed
seanh opened this issue Feb 15, 2013 · 4 comments
Closed

Crash (500) when showing a package with no resources (IDatasetForm) #403

seanh opened this issue Feb 15, 2013 · 4 comments
Assignees
Milestone

Comments

@seanh
Copy link
Contributor

seanh commented Feb 15, 2013

If an IDatasetForm plugin is active (see #396) then when showing a dataset with no resource, the validation done in package_show strips the resources key from the dict. Templates that try to read this key then crash, causing lots of pages to 500.

We could change validate() to not strip out keys that have empty lists as their values.

Or we could fix the templates not to crash on dataset dicts with no resources key.

@ghost ghost assigned seanh Feb 15, 2013
@seanh
Copy link
Contributor Author

seanh commented Feb 15, 2013

I'm not quite sure what to do about this.

Option 1: Fix the templates to not crash when the package_dict has no resources. I could fix the legacy templates, and that would make the tests pass, but the new templates may also be crashing, and we don't have any tests for those. There could be dozens of bugs like this, where validate() strips some key from a dict and then a template crashes as a result, and we won't find them because (a) we don't have tests for the new templates and (b) these bugs are only triggered when an IDatasetForm plugin is active and one isn't normally active when we run the tests or do QA.

So that leaves Option 2: Fix validate() to never strip keys from data dicts when those keys have empty values. There is actually already an option in CKAN, fix_partial_updates, that seems to do this, but it's turned off by default. We could make it the default behaviour but it probably breaks some tests (I will test and see..)

Longer term I think we need CKAN to do validation using form_to_db_package_schema() and db_to_form_package_schema() even when there's no IDatasetForm plugin active, just so that we'll catch bugs like this when running the tests and QAing, but that is a different issue.

@seanh
Copy link
Contributor Author

seanh commented Feb 15, 2013

The tests all pass if you run them with fix_partial_updates on, but I think the way fix_partial_updates is implemented it may short-circuit some schema validators that you wouldn't want it to, so may need to fix it a different way

@wardi
Copy link
Contributor

wardi commented Feb 15, 2013

This is one of the things that https://github.com/okfn/ckan/commits/2671-fix-partial-updates broke.

Does that branch even make sense? (I know I've argued this in IRC)

@seanh
Copy link
Contributor Author

seanh commented Feb 20, 2013

I'm assuming since all the core tests are passing with an IDatasetForm active now that this is fixed even if I apparently didn't autoclose it in a commit message. Closing now

@seanh seanh closed this as completed Feb 20, 2013
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

No branches or pull requests

2 participants