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

Cache questionnaires and standardize github client access #432

Merged
merged 7 commits into from
Jun 19, 2023

Conversation

blootsvoets
Copy link
Member

@blootsvoets blootsvoets commented Jun 15, 2023

  • Use GithubClient inside GithubProtocolFetcherStrategy to improve readability and remove deplication.
  • Use GithubService as a proxy for GithubClient in endpoint. This caches any values from the questionnaire.
  • Created CachedFunction that caches the result of a throwing function for a given amount of time.
  • Added more checks on the request being performed: number of bytes for content length, more strict URL validation (only https://api.github.com/ is allowed now instead of for example http://api.github.com.my.domain:9001/.

- Use GithubClient inside GithubProtocolFetcherStrategy to improve readability and remove deplication.
- Use GithubService as a proxy for GithubClient in endpoint. This caches any values from the questionnaire.
- Created FunctionCache that caches the result of a throwing function for a given amount of time.
- Added more checks on the request being performed: number of bytes for content length, more strict URL validation (only https://api.github.com is allowed now instead of for example http://api.github.com.my.domain.
@blootsvoets blootsvoets requested a review from mpgxvii June 15, 2023 12:24
Copy link
Member

@mpgxvii mpgxvii left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the improvements!

// Failed to get the Uri Map. try using the cached value
uriMap = projectProtocolUriMap.getCache();
}
return uriMap != null ? uriMap.keySet() : null;
Copy link
Member

Choose a reason for hiding this comment

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

Should we return an empty set here instead of returning null?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure -- I've kept the flow as it already existed. If this returns null the calling function returns an empty map, if this returns an empty set the calling function will create a map of ProtocolId to null. I haven't dived into why this behaviour is there, I've just kept it the same.

@blootsvoets blootsvoets merged commit 40d2930 into dev Jun 19, 2023
5 checks passed
@blootsvoets blootsvoets deleted the cacheQuestionnaires branch June 19, 2023 08:21
@mpgxvii mpgxvii mentioned this pull request Jun 23, 2023
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.

2 participants