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

Merged

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 litvinovg linked an issue Mar 20, 2024 that may be closed by this pull request
@chenejac chenejac self-requested a review March 26, 2024 13:36
@litvinovg litvinovg force-pushed the sparql_authorization_cache_fixes branch from cd2e9f3 to 76bf6f5 Compare March 26, 2024 16:16
@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
@chenejac chenejac removed the request for review from brianjlowe June 14, 2024 09:15
@chenejac chenejac merged commit 6029849 into vivo-project:main Jun 14, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Authorization sparql results cache is broken
4 participants