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

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

Merged
merged 3 commits into from 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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8ff1c93
Only update response cache for successful api responses
daverant Feb 3, 2024
b8e4e35
Merge branch 'main' into response-cache/update-on-success-only
nickfloyd Feb 15, 2024
9cd0436
Merge branch 'main' into response-cache/update-on-success-only
nickfloyd Mar 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
123 changes: 105 additions & 18 deletions Octokit.Tests/Caching/CachingHttpClientTests.cs
View file
Open in desktop
@@ -1,5 +1,6 @@
using System;
using System.Collections.Generic;
using System.Collections.Immutable;
using System.Linq;
using System.Net;
using System.Net.Http;
Expand Down Expand Up @@ -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<IHttpClient>();
Expand Down Expand Up @@ -146,20 +147,50 @@ public async Task UsesGithubResponseIfEtagIsPresentAndGithubReturnsNon304(HttpSt
responseCache.Received(1).SetAsync(request, Arg.Is<CachedResponse.V1>(v1 => new ResponseComparer().Equals(v1, CachedResponse.V1.Create(githubResponse))));
}

public static IEnumerable<object[]> NonNotModifiedHttpStatusCodesWithSetCacheFailure()
public static IEnumerable<object[]> 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<object>())
{
yield return new[] { statusCode, cacheFail };
yield return new[] { statusCode, cacheFail };
}
}
}

private static readonly IImmutableList<HttpStatusCode> _successStatusCodes = Enumerable
.Range(200, 100)
.Where(code => Enum.IsDefined(typeof(HttpStatusCode), code))
.Cast<HttpStatusCode>()
.ToImmutableList();

private static readonly IImmutableList<string> _invalidETags = new[]
{
null, string.Empty
}.ToImmutableList();

public static IEnumerable<object[]> 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<object>())
{
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<IHttpClient>();
Expand All @@ -171,6 +202,7 @@ public async Task UsesGithubResponseIfCachedEntryIsNull(bool setCacheThrows)
var cancellationToken = CancellationToken.None;

var githubResponse = Substitute.For<IResponse>();
githubResponse.StatusCode.Returns(httpStatusCode);

underlyingClient.Send(Arg.Is<IRequest>(req => req == request && !req.Headers.Any()), cancellationToken).ReturnsForAnyArgs(githubResponse);
responseCache.GetAsync(request).ReturnsNull();
Expand All @@ -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<IHttpClient>();
Expand All @@ -208,6 +239,7 @@ public async Task UsesGithubResponseIfGetCachedEntryThrows(bool setCacheThrows)
var cancellationToken = CancellationToken.None;

var githubResponse = Substitute.For<IResponse>();
githubResponse.StatusCode.Returns(httpStatusCode);

underlyingClient.Send(Arg.Is<IRequest>(req => req == request && !req.Headers.Any()), cancellationToken).ReturnsForAnyArgs(githubResponse);
responseCache.GetAsync(Args.Request).ThrowsForAnyArgs<Exception>();
Expand All @@ -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<IHttpClient>();
Expand All @@ -251,6 +280,7 @@ public async Task UsesGithubResponseIfCachedEntryEtagIsNullOrEmpty(string etag,
var cancellationToken = CancellationToken.None;

var githubResponse = Substitute.For<IResponse>();
githubResponse.StatusCode.Returns(httpStatusCode);

underlyingClient.Send(Arg.Is<IRequest>(req => req == request && !req.Headers.Any()), cancellationToken).ReturnsForAnyArgs(githubResponse);
responseCache.GetAsync(request).Returns(cachedV1Response);
Expand All @@ -272,6 +302,63 @@ public async Task UsesGithubResponseIfCachedEntryEtagIsNullOrEmpty(string etag,
responseCache.Received(1).GetAsync(request);
responseCache.Received(1).SetAsync(request, Arg.Is<CachedResponse.V1>(v1 => new ResponseComparer().Equals(v1, CachedResponse.V1.Create(githubResponse))));
}

public static IEnumerable<object[]> DoesNotUpdateCacheData()
{
var codesToExclude = _successStatusCodes
.Add(HttpStatusCode.NotModified);
var failureCodes = Enum
.GetValues(typeof(HttpStatusCode))
.Cast<HttpStatusCode>()
.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<IHttpClient>();
var responseCache = Substitute.For<IResponseCache>();
var request = Substitute.For<IRequest>();
request.Method.Returns(HttpMethod.Get);
request.Headers.Returns(new Dictionary<string, string>());

var cachedResponse = Substitute.For<IResponse>();
cachedResponse.Headers.Returns(etag == null ? new Dictionary<string, string>() : new Dictionary<string, string> { { "ETag", etag } });

var cachedV1Response = CachedResponse.V1.Create(cachedResponse);

var githubResponse = Substitute.For<IResponse>();
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<IRequest>(), Arg.Any<CachedResponse.V1>());
}
}

public class TheSetRequestTimeoutMethod
Expand Down
8 changes: 6 additions & 2 deletions Octokit/Caching/CachingHttpClient.cs
View file
Open in desktop
Expand Up @@ -40,12 +40,12 @@ public async Task<IResponse> 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;
}

Expand All @@ -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)
Expand Down
11 changes: 10 additions & 1 deletion Octokit/Helpers/HttpClientExtensions.cs → Octokit/Helpers/HttpExtensions.cs
View file
Open in desktop
Expand Up @@ -4,7 +4,7 @@

namespace Octokit
{
public static class HttpClientExtensions
public static class HttpExtensions
{
public static Task<IResponse> Send(this IHttpClient httpClient, IRequest request)
{
Expand All @@ -13,5 +13,14 @@ public static Task<IResponse> Send(this IHttpClient httpClient, IRequest request

return httpClient.Send(request, CancellationToken.None);
}

/// <summary>
/// Gets a value that indicates whether the HTTP response was successful.
/// </summary>
public static bool IsSuccessStatusCode(this IResponse response)
{
Ensure.ArgumentNotNull(response, nameof(response));
return (int) response.StatusCode >= 200 && (int) response.StatusCode <= 299;
}
}
}