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

BREAKING CHANGE (behavior): Modify caching to only attempt to update the response cache if a 2xx response code is received from GitHub #2877

Merged

Conversation

daverant
Copy link
Contributor

@daverant daverant commented Feb 3, 2024

Resolves #2876

Apologies if I've filed a bug when it should come under feature/enhancement!
I couldn't find any docs on response caching, but happy to update them if there are any.


Before the change?

  • If the CachingHttpClient received a non-200 and non-304 response from GitHub, it still attempts to update the underlying response cache, resulting in a higher number of subsequent cache misses if IResponseCache implementations don't guard for it themselves.

After the change?

  • Only attempts to update the response cache if a 2xx response code is received from GitHub.

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)

Does this introduce a breaking change?

  • I don't believe this is classed as a breaking change, but it is a change in behaviour that could impact users who rely on it calling the response cache for non-200 responses.
  • Yes
  • No

@nickfloyd
Copy link
Contributor

I am going to classify this as a breaking change due to the change in behavior in potential expectations that you point out. This is a great change, thank you for doing it! ❤️

@nickfloyd nickfloyd added Status: Wont fix This will not be worked on Type: Breaking change Used to note any change that requires a major version bump Type: Bug Something isn't working as documented and removed Status: Wont fix This will not be worked on labels Mar 12, 2024
@nickfloyd nickfloyd changed the title Only update response cache for successful api responses BREAKING CHANGE (behavior): Only update response cache for successful api responses Mar 12, 2024
@nickfloyd nickfloyd changed the title BREAKING CHANGE (behavior): Only update response cache for successful api responses BREAKING CHANGE (behavior): Modify caching to only attempt to update the response cache if a 2xx response code is received from GitHub Mar 12, 2024
@nickfloyd nickfloyd merged commit 92ff70b into octokit:main Mar 12, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Breaking change Used to note any change that requires a major version bump Type: Bug Something isn't working as documented
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[BUG]: Failed API calls update response cache
2 participants