From 8ff1c93a0fec29edff2b58bf0942756c8c65317f Mon Sep 17 00:00:00 2001 From: Dave Rant Date: Sat, 3 Feb 2024 15:16:41 +0000 Subject: [PATCH] Only update response cache for successful api responses --- .../Caching/CachingHttpClientTests.cs | 123 +++++++++++++++--- Octokit/Caching/CachingHttpClient.cs | 8 +- ...pClientExtensions.cs => HttpExtensions.cs} | 11 +- 3 files changed, 121 insertions(+), 21 deletions(-) rename Octokit/Helpers/{HttpClientExtensions.cs => HttpExtensions.cs} (50%) diff --git a/Octokit.Tests/Caching/CachingHttpClientTests.cs b/Octokit.Tests/Caching/CachingHttpClientTests.cs index 688766e055..1cc8e42208 100644 --- a/Octokit.Tests/Caching/CachingHttpClientTests.cs +++ b/Octokit.Tests/Caching/CachingHttpClientTests.cs @@ -1,5 +1,6 @@ using System; using System.Collections.Generic; +using System.Collections.Immutable; using System.Linq; using System.Net; using System.Net.Http; @@ -105,8 +106,8 @@ public async Task UsesCachedResponseIfEtagIsPresentAndGithubReturns304() } [Theory] - [MemberData(nameof(NonNotModifiedHttpStatusCodesWithSetCacheFailure))] - public async Task UsesGithubResponseIfEtagIsPresentAndGithubReturnsNon304(HttpStatusCode httpStatusCode, bool setCacheThrows) + [MemberData(nameof(SuccessHttpStatusCodesWithSetCacheFailure))] + public async Task UsesGithubResponseIfEtagIsPresentAndGithubReturnsSuccessCode(HttpStatusCode httpStatusCode, bool setCacheThrows) { // arrange var underlyingClient = Substitute.For(); @@ -146,20 +147,50 @@ public async Task UsesGithubResponseIfEtagIsPresentAndGithubReturnsNon304(HttpSt responseCache.Received(1).SetAsync(request, Arg.Is(v1 => new ResponseComparer().Equals(v1, CachedResponse.V1.Create(githubResponse)))); } - public static IEnumerable NonNotModifiedHttpStatusCodesWithSetCacheFailure() + public static IEnumerable SuccessHttpStatusCodesWithSetCacheFailure() { - foreach (var statusCode in Enum.GetValues(typeof(HttpStatusCode))) + var setCacheFails = new[] { true, false }; + + foreach (var cacheFail in setCacheFails) + { + foreach (var statusCode in _successStatusCodes.Cast()) + { + yield return new[] { statusCode, cacheFail }; + yield return new[] { statusCode, cacheFail }; + } + } + } + + private static readonly IImmutableList _successStatusCodes = Enumerable + .Range(200, 100) + .Where(code => Enum.IsDefined(typeof(HttpStatusCode), code)) + .Cast() + .ToImmutableList(); + + private static readonly IImmutableList _invalidETags = new[] + { + null, string.Empty + }.ToImmutableList(); + + public static IEnumerable SuccessHttpStatusCodesWithSetCacheFailureAndInvalidETags() + { + var setCacheFails = new[] { true, false }; + foreach (var etag in _invalidETags) { - if (statusCode.Equals(HttpStatusCode.NotModified)) continue; - yield return new[] { statusCode, true }; - yield return new[] { statusCode, false }; + foreach (var cacheFail in setCacheFails) + { + foreach (var statusCode in _successStatusCodes.Cast()) + { + yield return new[] { statusCode, cacheFail, etag }; + yield return new[] { statusCode, cacheFail, etag }; + } + } } } [Theory] - [InlineData(true)] - [InlineData(false)] - public async Task UsesGithubResponseIfCachedEntryIsNull(bool setCacheThrows) + [MemberData(nameof(SuccessHttpStatusCodesWithSetCacheFailure))] + public async Task UsesGithubResponseIfCachedEntryIsNull(HttpStatusCode httpStatusCode, bool setCacheThrows) { // arrange var underlyingClient = Substitute.For(); @@ -171,6 +202,7 @@ public async Task UsesGithubResponseIfCachedEntryIsNull(bool setCacheThrows) var cancellationToken = CancellationToken.None; var githubResponse = Substitute.For(); + githubResponse.StatusCode.Returns(httpStatusCode); underlyingClient.Send(Arg.Is(req => req == request && !req.Headers.Any()), cancellationToken).ReturnsForAnyArgs(githubResponse); responseCache.GetAsync(request).ReturnsNull(); @@ -194,9 +226,8 @@ public async Task UsesGithubResponseIfCachedEntryIsNull(bool setCacheThrows) } [Theory] - [InlineData(true)] - [InlineData(false)] - public async Task UsesGithubResponseIfGetCachedEntryThrows(bool setCacheThrows) + [MemberData(nameof(SuccessHttpStatusCodesWithSetCacheFailure))] + public async Task UsesGithubResponseIfGetCachedEntryThrows(HttpStatusCode httpStatusCode, bool setCacheThrows) { // arrange var underlyingClient = Substitute.For(); @@ -208,6 +239,7 @@ public async Task UsesGithubResponseIfGetCachedEntryThrows(bool setCacheThrows) var cancellationToken = CancellationToken.None; var githubResponse = Substitute.For(); + githubResponse.StatusCode.Returns(httpStatusCode); underlyingClient.Send(Arg.Is(req => req == request && !req.Headers.Any()), cancellationToken).ReturnsForAnyArgs(githubResponse); responseCache.GetAsync(Args.Request).ThrowsForAnyArgs(); @@ -231,11 +263,8 @@ public async Task UsesGithubResponseIfGetCachedEntryThrows(bool setCacheThrows) } [Theory] - [InlineData(null, true)] - [InlineData(null, false)] - [InlineData("", true)] - [InlineData("", false)] - public async Task UsesGithubResponseIfCachedEntryEtagIsNullOrEmpty(string etag, bool setCacheThrows) + [MemberData(nameof(SuccessHttpStatusCodesWithSetCacheFailureAndInvalidETags))] + public async Task UsesGithubResponseIfCachedEntryEtagIsNullOrEmpty(HttpStatusCode httpStatusCode, bool setCacheThrows, string etag) { // arrange var underlyingClient = Substitute.For(); @@ -251,6 +280,7 @@ public async Task UsesGithubResponseIfCachedEntryEtagIsNullOrEmpty(string etag, var cancellationToken = CancellationToken.None; var githubResponse = Substitute.For(); + githubResponse.StatusCode.Returns(httpStatusCode); underlyingClient.Send(Arg.Is(req => req == request && !req.Headers.Any()), cancellationToken).ReturnsForAnyArgs(githubResponse); responseCache.GetAsync(request).Returns(cachedV1Response); @@ -272,6 +302,63 @@ public async Task UsesGithubResponseIfCachedEntryEtagIsNullOrEmpty(string etag, responseCache.Received(1).GetAsync(request); responseCache.Received(1).SetAsync(request, Arg.Is(v1 => new ResponseComparer().Equals(v1, CachedResponse.V1.Create(githubResponse)))); } + + public static IEnumerable DoesNotUpdateCacheData() + { + var codesToExclude = _successStatusCodes + .Add(HttpStatusCode.NotModified); + var failureCodes = Enum + .GetValues(typeof(HttpStatusCode)) + .Cast() + .Except(codesToExclude) + .ToList(); + var hasCachedResponses = new[] { false, true }; + + foreach (var etag in _invalidETags) + { + foreach (var hasCachedResponse in hasCachedResponses) + { + foreach (var statusCode in failureCodes) + { + yield return new object[] + { + statusCode, hasCachedResponse, etag + }; + } + } + } + } + + [Theory] + [MemberData(nameof(DoesNotUpdateCacheData))] + public async Task DoesNotUpdateCacheIfGitHubResponseIsNotSuccessCode(HttpStatusCode httpStatusCode, bool hasCachedResponse, string etag) + { + // arrange + var underlyingClient = Substitute.For(); + var responseCache = Substitute.For(); + var request = Substitute.For(); + request.Method.Returns(HttpMethod.Get); + request.Headers.Returns(new Dictionary()); + + var cachedResponse = Substitute.For(); + cachedResponse.Headers.Returns(etag == null ? new Dictionary() : new Dictionary { { "ETag", etag } }); + + var cachedV1Response = CachedResponse.V1.Create(cachedResponse); + + var githubResponse = Substitute.For(); + githubResponse.StatusCode.Returns(httpStatusCode); + + underlyingClient.Send(request).Returns(githubResponse); + responseCache.GetAsync(request).Returns(hasCachedResponse ? cachedV1Response : null); + + var cachingHttpClient = new CachingHttpClient(underlyingClient, responseCache); + + // act + _ = await cachingHttpClient.Send(request, CancellationToken.None); + + // assert + responseCache.DidNotReceiveWithAnyArgs().SetAsync(Arg.Any(), Arg.Any()); + } } public class TheSetRequestTimeoutMethod diff --git a/Octokit/Caching/CachingHttpClient.cs b/Octokit/Caching/CachingHttpClient.cs index e78e3f067c..63a44f3ac2 100644 --- a/Octokit/Caching/CachingHttpClient.cs +++ b/Octokit/Caching/CachingHttpClient.cs @@ -40,12 +40,12 @@ public async Task Send(IRequest request, CancellationToken cancellati return cachedResponse; } - TrySetCachedResponse(request, conditionalResponse); + _ = TrySetCachedResponse(request, conditionalResponse); return conditionalResponse; } var response = await _httpClient.Send(request, cancellationToken); - TrySetCachedResponse(request, response); + _ = TrySetCachedResponse(request, response); return response; } @@ -65,6 +65,10 @@ private async Task TrySetCachedResponse(IRequest request, IResponse response) { try { + if(!response.IsSuccessStatusCode()) + { + return; + } await _responseCache.SetAsync(request, CachedResponse.V1.Create(response)); } catch (Exception) diff --git a/Octokit/Helpers/HttpClientExtensions.cs b/Octokit/Helpers/HttpExtensions.cs similarity index 50% rename from Octokit/Helpers/HttpClientExtensions.cs rename to Octokit/Helpers/HttpExtensions.cs index 51b22c8fd9..6c8609ac8a 100644 --- a/Octokit/Helpers/HttpClientExtensions.cs +++ b/Octokit/Helpers/HttpExtensions.cs @@ -4,7 +4,7 @@ namespace Octokit { - public static class HttpClientExtensions + public static class HttpExtensions { public static Task Send(this IHttpClient httpClient, IRequest request) { @@ -13,5 +13,14 @@ public static Task Send(this IHttpClient httpClient, IRequest request return httpClient.Send(request, CancellationToken.None); } + + /// + /// Gets a value that indicates whether the HTTP response was successful. + /// + public static bool IsSuccessStatusCode(this IResponse response) + { + Ensure.ArgumentNotNull(response, nameof(response)); + return (int) response.StatusCode >= 200 && (int) response.StatusCode <= 299; + } } } \ No newline at end of file