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.

Sign up for GitHub

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

Jump to bottom

Response caching #2648

Merged
merged 20 commits into from
Feb 7, 2023
Merged

Response caching #2648

merged 20 commits into from
Feb 7, 2023

Conversation

reny-gearset
Copy link
Contributor

@reny-gearset reny-gearset commented Jan 3, 2023 •
edited
Loading

Resolves #2649

Behavior

Before the change?

  • There's no way to cache GET responses, users can easily overshoot the Github API rate limit when polling.

After the change?

  • By implementing IResponseCache and setting it in the ResponseCache property in GitHubClient, we provide a way to cache GET responses and retain the API rate limit usage when its Etag matches. Specifically, we set the If-None-Matched header in the request with the non-null and non-empty Etag value from the cached response before sending it to Github.

Other information

Additional info

Pull request checklist

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
  • Added the appropriate label for the given change

Does this introduce a breaking change?

Please see our docs on breaking changes to help!

  • Yes (Please add the Type: Breaking change label)
  • No

If Yes, what's the impact:

  • N/A

Pull request type

Please add the corresponding label for change this PR introduces:

  • Bugfix: Type: Bug
  • Feature/model/API additions: Type: Feature
  • Updates to docs or samples: Type: Documentation
  • Dependencies/code cleanup: Type: Maintenance

@reny-gearset reny-gearset mentioned this pull request Jan 4, 2023
Closed
1 task
@frankyfrankyren
caching layer for GET responses
4fddb5c
@frankyfrankyren
Setting the ResponseCache property sets the caching layer/wrapper - C…
37fbf4e 
…achingHttpClient around existing IHttpClient in Connection
@frankyfrankyren
sets the underlying IHttpClient when passing a CachingHttpClient into…
ef2b462 
… its constructor
@frankyfrankyren
added tests for CachingHttpClient constructor
d9c82dc
@frankyfrankyren
ensures constructors parameters are not null for CachingHttpClient
ab81983
@frankyfrankyren
added unit tests for CachingHttpClient::SetRequestTimeout
146f930
@frankyfrankyren
added unit tests for CachingHttpClient::Dispose
0f7dc19
@frankyfrankyren
added unit tests for CachingHttpClient::Send
cb8a7b2
@frankyfrankyren
ensure we throw early when request parameter is null in CachingHttpCl…
f76ea9f 
…ient::Send
@frankyfrankyren
added tests for CachedResponse.V1 constructor
8e5858e
@frankyfrankyren
ensure we throw when headers parameter is null in CachedResponse.V1 c…
5845227 
…onstructor
@frankyfrankyren
added tests for CachedResponse.V1::Create and CachedResponse.V1::ToRe…
7de3a05 
…sponse
@frankyfrankyren
ensure we throw early when response is null in CachedResponse.V1::Create
63f236c
@frankyfrankyren
added more tests for CachingHttpClient::Send
6c5d2d9
@reny-gearset reny-gearset marked this pull request as ready for review January 10, 2023 20:23
@reny-gearset
Copy link
Contributor Author

reny-gearset commented Jan 11, 2023 •
edited
Loading

@kfcampbell Thanks for sticking labels against the feature I raised - #2649 👍

This is my first PR against this project, is there anything else I need to do to push this forward?

@nickfloyd
Copy link
Contributor

Hey @reny-gearset, thank you for the contributions here! ❤️ | The item is on this board to be reviewed - we'll let you know if anything comes up!

@nickfloyd nickfloyd added Priority: Normal Type: Feature New feature or request labels Jan 23, 2023
nickfloyd
nickfloyd approved these changes Feb 6, 2023
View reviewed changes
Copy link
Contributor

@nickfloyd nickfloyd left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Hey @reny-gearset, Thanks again for getting this knocked out. This LGTM and should be good to merge once CI is ✅ ❤️

@nickfloyd
Copy link
Contributor

Hey @reny-gearset , just a quick note about the sourcelink failure in the windows CI build. It is unrelated to your change and is something we are working on. Apologies for the noise.

@nickfloyd nickfloyd mentioned this pull request Feb 6, 2023
Merged
5 tasks
@nickfloyd nickfloyd merged commit 66587ee into octokit:main Feb 7, 2023
@Revyn112
Copy link

@reny-gearset @nickfloyd any documentation or example on how to use this properly?

@Larusso
Copy link

Larusso commented Jan 23, 2024

As @Revyn112 stated. Is there some documentation what one needs to set there?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nickfloyd nickfloyd nickfloyd approved these changes

Assignees
No one assigned
Labels
Type: Feature New feature or request
Projects
🧰 Octokit Active
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[FEAT]: add caching funtionality for GET responses
5 participants
@reny-gearset @nickfloyd @Revyn112 @Larusso @frankyfrankyren