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

Add endpoint to clear the cdm_cache and achilles_cache #2406

Open
wants to merge 18 commits into
base: master
Choose a base branch
from

Conversation

amarsan
Copy link
Collaborator

@amarsan amarsan commented Nov 11, 2024

supports OHDSI/Atlas#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.

@anthonysena anthonysena linked an issue Nov 11, 2024 that may be closed by this pull request
@chrisknoll
Copy link
Collaborator

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.

.editorconfig Outdated

[*]
indent_style = space
indent_size = 4
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we're doing this let's do 2 space indent.

@chrisknoll
Copy link
Collaborator

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
Copy link
Collaborator Author

amarsan commented Nov 15, 2024

@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:

  • You mentioned it might be nice to delete the caches in parallel. I could not get the Spring Boot @transactional() system to work when using parallelStream() on the source collection, so I gave up on that front. Right now everything is deleted serially.
  • I could not get a full integration test to work, starting from the controller. The issue seems to be that it's very challenging to mock out an authenticated Subject, given the approach we are using to configuring Shiro with Spring Boot when security is enabled. All of our existing integration tests are run without security set up, so the code we have to mock a Subject works because apparently the app in that case gets the subject using SecurityUtils.getSubject(). However, when AtlasRegularSecurity is enabled, the app gets a Subject in a different way, and for the life of me I could not figure out a way to hijack that process and inject the appropriate mocks. So I instead refactored and wrote tests for the CDMCacheService and AchillesCacheService methods.
  • The Github action that runs our tests does not fail if a test fails. It appears that at least one of the existing integration tests fails. I'd rather try to fix the action and the test in a separate PR.

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 this pull request may close these issues.

Clear Server Cache button in ATLAS does not work
2 participants