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

Default view is re-added when removed before DataStore upload is complete #3011

Closed
torfsen opened this issue May 11, 2016 · 18 comments

Comments

Projects
None yet
4 participants
@torfsen
Copy link
Contributor

commented May 11, 2016

I'm doing the following via the API on a CKAN 2.5.2 installation that has DataStore + DataPusher enabled:

  1. Create a CSV resource
  2. Remove the default recline_view
  3. Add a new recline_grid_view view

I'd expect the resource to end up with a single view (the one I added in step 3). However, it ends up with two views: the one I created and a new default view (ID is different from the original default view).

It seems that the default view is added again once the data has been uploaded to the DataStore by the DataPusher. If steps 1-3 are done quickly enough (before the upload to the DataStore has completed) then the resource has only the intended recline_grid_view, which at that point shows an error (because the data is not yet available). As soon as the data is available (i.e. as soon as the error is gone after a reload) the resource has gotten an additional recline_view.

Here's a small script to demonstrate the problem:

ckan = ckanapi.RemoteCKAN(CKAN_URL, apikey=API_KEY)

res_dict = ckan.action.resource_create(package_id=PKG_ID, url=CSV_URL,
                                       name='Test', format='CSV')
view_dicts = ckan.action.resource_view_list(id=res_dict['id'])
for view_dict in view_dicts:
    ckan.action.resource_view_delete(id=view_dict['id'])
ckan.action.resource_view_create(resource_id=res_dict['id'], title='My View',
                                 view_type='recline_grid_view')

# Uncomment to make assertion fail
#import time
#time.sleep(10)

assert len(ckan.action.resource_view_list(id=res_dict['id'])) == 1

If you uncomment the sleep lines (to wait until the data is in the DataStore) then the assertion fails.

@amercader amercader self-assigned this May 12, 2016

@amercader

This comment has been minimized.

Copy link
Member

commented May 13, 2016

This is another side-effect of #2234 (which I'm very much starting to regret to ever have implemented). Essentially we added a call to resource_patch on datastore_create to set the datastore_active extra on the resource.

So what happens is:

  • Resource is created, default views are created
  • You delete the default view you don't want
  • In the meantime, DataPusher is doing its thing
  • When the data is uploaded to the DataStore, datastore_create calls resource_patch (to set up the datastore_active extra) which calls resource_update which calls package_update which calls package_create_default_resource_views and recreates the view.

@wardi I know you hate it but the only thing I can think of is passing a create_views = False entry on the context that ends up in package_update and avoids the default views creation.

@torfsen I know it's not ideal but on the meantime you can delete the extra view via the API if you are doing it as part of a script

@wardi

This comment has been minimized.

Copy link
Contributor

commented May 13, 2016

So datapusher is deleting and recreating the datastore table. That shows up as a new table so the default views are created, or are we calling package_create_default_resource_views on every package_update?

@amercader

This comment has been minimized.

Copy link
Member

commented May 13, 2016

are we calling package_create_default_resource_views on every package_update?

This. It has no relation with the DataPusher creating or dropping tables, other than it triggers an update when it imports data to the DataStore

@wardi

This comment has been minimized.

Copy link
Contributor

commented May 13, 2016

So.. anyone editing a dataset will cause all the default resource views to reappear too, right?

We need something like a white-out for resource views that have been removed so they don't get re-added, or we just need to me more selective about when that code is run, not on every package_update

@amercader

This comment has been minimized.

Copy link
Member

commented May 13, 2016

You are right.
Neither of the two options are easy as things stand right now. Views are not marked as deleted on the DB, they are just removed entirely so the white-out option is probably the hardest.

Thinking on when we want default views to be created, I think the most common cases are:

  • Creating a package (if resources are provided, ie creating it via the API or a custom combined form)
  • Creating resource on an existing package (like on the default UI)
  • When we finish pushing data to the DataStore (DataPusher does that now)

So just on package_create, resource_create and datapusher_hook.

If we only cover these cases, and not any updates we will miss situations like people changing the format of a resource and new views for that format not appearing, but that might be preferable to the current behaviour.

@wardi

This comment has been minimized.

Copy link
Contributor

commented May 13, 2016

👍 That sounds like a simpler solution.

@amercader

This comment has been minimized.

Copy link
Member

commented May 13, 2016

@torfsen Do you have time to work on patch along those lines?

@amercader

This comment has been minimized.

Copy link
Member

commented May 20, 2016

@k-nut this is also a good one to get an introduction about how DataPusher / DataStore / Resource views work

@torfsen

This comment has been minimized.

Copy link
Contributor Author

commented May 22, 2016

@amercader Sorry, I'm currently busy with other stuff.

As a workaround I'm currently polling datastore_info until the resource has been uploaded to the datastore before deleting the default views.

@k-nut

This comment has been minimized.

Copy link
Contributor

commented May 23, 2016

@amercader I am taking a look at this now.

k-nut added a commit to k-nut/ckan that referenced this issue May 23, 2016

k-nut added a commit to k-nut/ckan that referenced this issue May 23, 2016

[ckan#3011] Create default views on resource_create
- This was not neccessary before because we would call `package_update`
  in the `resource_create` function. Now that `package_update` does not
  create the default views anymore the `resource_create` function has to
  do it itself

k-nut added a commit to k-nut/ckan that referenced this issue May 23, 2016

[ckan#3011] Remove tests for package_update and resource_update
- The new logic is not to create the views anymore on `*_update` but
  just on `*_create`
@k-nut

This comment has been minimized.

Copy link
Contributor

commented May 23, 2016

I pushed some changes to my branch now. master...k-nut:3011-do-not-always-create-default-views

Essentially I removed the view creation from the update.py and added it to the resource_create function.

To be honest though I am not entirely sure what the parameters that are passed to add_views_to_resource should contain and if I am passing the right ones. The tests seem to pass with the current configuration.

But with those changes the assertion in the script provided by @torfsen does not fail anymore and only the wanted view is created.

@amercader

This comment has been minimized.

Copy link
Member

commented May 26, 2016

@k-nut Rather than calling ckan.lib.datapreview functions directly in here call the resource_create_default_resource_views action in a similar way that was done on package_update, passing the resource dataset dicts in the data_dict:

logic.get_action('resource_create_default_resource_views')(
    {'model': context['model'], 'user': context['user'],
     'ignore_auth': True},
    {'resource': resource, 'package': updated_package_dict})

This way people can override the action in their extensions if they want to modify the behaviour. And add a couple of tests on resource creation to replace the ones deleted for package update 👍

k-nut added a commit to k-nut/ckan that referenced this issue May 26, 2016

k-nut added a commit to k-nut/ckan that referenced this issue May 26, 2016

@k-nut k-nut referenced this issue May 26, 2016

Merged

Do not create default views on update #3061

0 of 5 tasks complete
@k-nut

This comment has been minimized.

Copy link
Contributor

commented May 26, 2016

@amercader as mentioned in the PR already there already is the test_default_views_created_on_resource_create test. Do you think this needs more tests?

@amercader

This comment has been minimized.

Copy link
Member

commented May 26, 2016

No, sorry I missed that. Existing tests are fine

@k-nut

This comment has been minimized.

Copy link
Contributor

commented May 26, 2016

It seems like the tests for the recline view extension are now failing because the datastore_create does not create the default views anymore?! https://circleci.com/gh/k-nut/ckan/9

@k-nut

This comment has been minimized.

Copy link
Contributor

commented May 26, 2016

Which is weird because it is calling resource_create https://github.com/ckan/ckan/blob/master/ckanext/datastore/logic/action.py#L96 Maybe something is wrong with the test?

@k-nut

This comment has been minimized.

Copy link
Contributor

commented May 26, 2016

So the problem in the test seems to be that can_view is called on ReclineView but this goes into the last else branch and returns False. That leads to the plugin not beeing discovered as a view and not being registered consequently.

@k-nut

This comment has been minimized.

Copy link
Contributor

commented May 26, 2016

This can be fixed by specifying the format of the resource in the test like this.

'resource': {'package_id': dataset['id'], 'format': 'csv'},

I am not sure if it is fine to change the test here or if the test points us to something being actually wrong with the code...

k-nut added a commit to k-nut/ckan that referenced this issue May 26, 2016

[ckan#3011] Spcifiy format in test
Not sure if this is a good idea but all the tests should at least pass
now

k-nut added a commit to k-nut/ckan that referenced this issue Jun 6, 2016

k-nut added a commit to k-nut/ckan that referenced this issue Jun 6, 2016

[ckan#3011] Create default views on resource_create
- This was not neccessary before because we would call `package_update`
  in the `resource_create` function. Now that `package_update` does not
  create the default views anymore the `resource_create` function has to
  do it itself

k-nut added a commit to k-nut/ckan that referenced this issue Jun 6, 2016

[ckan#3011] Remove tests for package_update and resource_update
- The new logic is not to create the views anymore on `*_update` but
  just on `*_create`

k-nut added a commit to k-nut/ckan that referenced this issue Jun 6, 2016

k-nut added a commit to k-nut/ckan that referenced this issue Jun 6, 2016

k-nut added a commit to k-nut/ckan that referenced this issue Jun 6, 2016

[ckan#3011] Spcifiy format in test
Not sure if this is a good idea but all the tests should at least pass
now

k-nut added a commit to k-nut/ckan that referenced this issue Jun 6, 2016

k-nut added a commit to k-nut/ckan that referenced this issue Jun 6, 2016

amercader added a commit that referenced this issue Aug 26, 2016

[#3011] Create default views on resource_create
- This was not neccessary before because we would call `package_update`
  in the `resource_create` function. Now that `package_update` does not
  create the default views anymore the `resource_create` function has to
  do it itself

amercader added a commit that referenced this issue Aug 26, 2016

[#3011] Remove tests for package_update and resource_update
- The new logic is not to create the views anymore on `*_update` but
  just on `*_create`

amercader added a commit that referenced this issue Aug 26, 2016

[#3011] Spcifiy format in test
Not sure if this is a good idea but all the tests should at least pass
now

amercader added a commit that referenced this issue Aug 26, 2016

@amercader amercader referenced this issue Nov 27, 2018

Merged

Add doc language around patching resources. #4563

1 of 5 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.