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

Sparql authorization cache fixes #450

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

litvinovg
Copy link
Contributor

@litvinovg litvinovg commented Mar 15, 2024

VIVO GitHub issue

Additional notes

This PR is stacked on top of #444
Only the last commit is related to this PR.

What does this pull request do?

Fixes failing authorization checks due to corrupted sparql results cache.

What's new?

Fixed SparqlSelectQueryResultsChecker
Added tests to verify that issue is resolved.
Log cases when model wasn't provided as errors.

How should this be tested?

  • Run tests without fix to see that cache is being corrupted.
  • Run tests with fix to verify that cache is not being corrupted.

Interested parties

@VIVO-project/vivo-committers

@litvinovg
Copy link
Contributor Author

Resolved merge conflict

chenejac
chenejac previously approved these changes Mar 28, 2024
Copy link
Contributor

@chenejac chenejac left a comment

Choose a reason for hiding this comment

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

@litvinovg works for me.

I have run tests and it is passing. I have tried to revert changes in SparqlSelectQueryResultChecker class and tests are failing.

Only the code in the last commit has been reviewed. I have investigated a little bit other approaches in copying a set and there is no clear recommendation whether copy constructor or clone method are better approaches comparing to using addAll method (approach used here). Therefore, I would suggest keeping it as is.

@chenejac chenejac removed the request for review from brianjlowe April 11, 2024 14:59
@litvinovg litvinovg force-pushed the sparql_authorization_cache_fixes branch from 25430cd to 4fa8966 Compare April 18, 2024 14:25
@chenejac chenejac requested a review from brianjlowe May 9, 2024 07:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants