Skip to content

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

Merged
nickfloyd merged 3 commits intooctokit:mainfrom
daverant:response-cache/update-on-success-only
Mar 12, 2024
Merged

BREAKING CHANGE (behavior): Modify caching to only attempt to update the response cache if a 2xx response code is received from GitHub#2877
nickfloyd merged 3 commits intooctokit:mainfrom
daverant:response-cache/update-on-success-only

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
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