OHDSI / WebAPI

OHDSI WebAPI contains all OHDSI services that can be called from OHDSI applications
Apache License 2.0
130 stars 169 forks source link

Add endpoint to clear the cdm_cache and achilles_cache #2406

Open amarsan opened 2 weeks ago

amarsan commented 2 weeks ago

supports https://github.com/OHDSI/Atlas/issues/2847

This adds an endpoint to clear the cdm_cache and achilles_cache tables for sources that the caller has access to. The caller must be an admin.

I followed the pattern for warming a cache, wrapping the operation in a Job.

I wanted to write an integration test for this, but ran into the issue that since the user needs to be an admin, security roles and users need to be set up as part of the integration tests. I haven't tackled figuring that out yet. Guidance here would be much appreciated.

chrisknoll commented 1 week ago

Hi @amarsan . Excellent work, and I appreciate you following the existing patterns in the codebase (such as creating jobs and tasklets) to perform this activity in background. Some comments:

  1. I don't think we need an enable/disable flag for this. it's 'opt in' by clicking on the button. Usually when we have an enable config it's because otherwise the user admin would have no choice about some application behavior, so I don't think we need to disable the ability to refresh the cache. It might be misleading to let them click on the button and then not have it do anything.

  2. I don't think we need a tasklet to make these reqeusts async. The reason is that the admin will probably want to click a button, wait a few seconds (it's just a delete from where source_id = ? call, shouldn't take more than 30 secs) and when the response is recieved that it finishes, the admin can navigate to the app and see the effect of the de-cache. by making it async, the admin will need to watch a job for completion before checking if the counts are de-cached. I think this amounts to the endpoint just callling the two clearCache() functions (maybe do this in parallel?) and waiting for the completion before returning a success result vs. kicking off a job. If you have strong opinions on making it async, we can discuss.

Great work, tho, it was a lot of effort to follow the patterns in code, so that was a good effort. I think in this case we just don't need to disable the ability to clear a cache nor make the clear action async.

As far as writing a test case where you want to test a person is admin, I think I have a unit test for wildcard permissions that I had to 'inject' a user context into the test case that had the necessary permissions that I was testing...I think you can do the same thing by injecting a user context that has the admin role. Let me dig it up and i'll reference it here when I find it.

chrisknoll commented 1 week ago

Would the tests that we used to test wildcard permissions serve as a good model for doing your own permission test:

https://github.com/OHDSI/WebAPI/blob/master/src/test/java/org/ohdsi/webapi/security/PermissionTest.java

I believe you would just set up a JSON file to insert records into user-role to assign the user to the admin role.

amarsan commented 1 week ago

@chrisknoll this is ready for review now.

This PR has been a crash course for me in Spring Boot, Shiro, and async in Java. Some issues I ran into: