search

octokit / octokit.net

A GitHub API client library for .NET
https://octokitnet.readthedocs.io/en/latest/
MIT License
2.62k stars 1.07k forks source link

[BUG]: Failed API calls update response cache #2876

Closed daverant closed 2 months ago

daverant commented 3 months ago

What happened?

The response cache in CachingHttpClient is called even when GitHub returns a non-2xx and non-304 status code. It's likely that implementations of IResponseCache will only want to cache data for successful responses. Without this check, it's easy for implementations to miss this check and flush valid responses from the cache when receiving a non-2xx response, resulting in an increase of subsequent cache misses.

This is an opinionated change - it's reasonable to expect implementations to cater for this themselves, but feels like it would help users more than hinder (I can't think of a use case for caching non-successful responses).

Versions

octokit.net v9.1.2, net7.0

Relevant log output

No response

Code of Conduct

kfcampbell commented 3 months ago

I'm generally okay with this approach. @nickfloyd are you?

I'm also curious how redirect status codes (like a 301 or 307) affect caching.