Replace Reader logic with open data compatible cached functions#554
Open
patrick-austin wants to merge 4 commits into
Open
Replace Reader logic with open data compatible cached functions#554patrick-austin wants to merge 4 commits into
patrick-austin wants to merge 4 commits into
Conversation
Contributor
There was a problem hiding this comment.
Work well:
- Tested with single and multiple sub apis
- Checked semantic commit, release will be 11.1.0
I have a PR that updates the release process and removes the safety step. I’ve just realised that, since the Docker job depends on the safety job, this PR will not produce a Docker image on Harbor when it's merged into main.
extra testing to be discussed separately (Should not longer expose "hidden" datasets in an Investigation corresponding to a session DOI)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Open data for Diamond will allow anon/ORCID authenticated users to view Investigations, Datasets and Datafiles associated with DataPublication entities. Previously, the "reader" logic assumed that if you can see an Investigation, you can see the Datasets within it. This is no longer the case, as being able to see an Investigation now could mean it has a non-opened session DOI, or is in an open session/user DOI but only Datasets of DatasetType raw should be visible (enforced by directly associating them via a DataCollection).
To address this, instead of just checking if the user can view the Investigation, do some more sophisticated checks which take into account the new open data logic as well. For performance, the results of this can be cached.
@patrick-austin TODO: Need to actually benchmark this to check performance is at least comparable to how it was with the old method. Shouldn't merge this PR until I've verified this (so currently a draft), but hopefully it's good as is or with minor changes to make it more performant. May be worth reviewing now or later - either is fine by me.Happy enough with the performance, and reverted the breaking config change so this can be tagged as
feat:. Should be ready for review, but no rush. Will put any breaking changes (config, new API etc.) in a separate branch, branched from this one,Specific changes
Refactordatagateway_api.use_reader_for_performancetoreaderin the config.Future change will implement a ReadOnly API which will require the same reader account. To follow the same approach as for the dg-api/search-api, intention would be to define aread_only_apisection to the config and then re-usereader(so cannot have it belonging to either "api" config.passwordtoSecretStr, for secrecy.create_reader_clientto be aclassmethod(as it had no dependence onself)ttl_cachedclassmethods which check if the current user is a reader, or the Dataset is open.Testing Instructions
Existing functionality should still apply
Should not longer expose "hidden" datasets in an Investigation corresponding to a session DOI
Review code
Check GitHub Actions build
If
icatdb Generator Script Consistency TestCI job fails, is this because of a deliberate change made to the script to change generated data (which isn't actually a problem) or is here an underlying issue with the changes made?Review changes to test coverage
Does this change mean a new patch, minor or major version should be made? If so, does one of the commit messages feature
fix:,feat:orBREAKING CHANGE:so a release is automatically made via GitHub Actions upon merge?featon any subsequent commits?