From 272c2d0cc896664674d40e10517676bee7b7dfdd Mon Sep 17 00:00:00 2001 From: Brendan Forster Date: Thu, 9 Apr 2020 12:14:54 -0300 Subject: [PATCH 1/7] add failing test for parsing ApiInfo --- Octokit.Tests/Http/ApiInfoParserTests.cs | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/Octokit.Tests/Http/ApiInfoParserTests.cs b/Octokit.Tests/Http/ApiInfoParserTests.cs index 00e3215b4e..ed837640ac 100644 --- a/Octokit.Tests/Http/ApiInfoParserTests.cs +++ b/Octokit.Tests/Http/ApiInfoParserTests.cs @@ -32,6 +32,28 @@ public void ParsesApiInfoFromHeaders() Assert.Equal("5634b0b187fd2e91e3126a75006cc4fa", apiInfo.Etag); } + [Fact] + public void ParsesApiInfoFromCaseInsensitiveHeaders() + { + var headers = new Dictionary + { + { "x-accepted-oauth-scopes", "user" }, + { "x-oauth-scopes", "user, public_repo, repo, gist" }, + { "x-ratelimit-limit", "5000" }, + { "x-ratelimit-remaining", "4997" }, + { "etag", "5634b0b187fd2e91e3126a75006cc4fa" } + }; + + var apiInfo = ApiInfoParser.ParseResponseHeaders(headers); + + Assert.NotNull(apiInfo); + Assert.Equal(new[] { "user" }, apiInfo.AcceptedOauthScopes.ToArray()); + Assert.Equal(new[] { "user", "public_repo", "repo", "gist" }, apiInfo.OauthScopes.ToArray()); + Assert.Equal(5000, apiInfo.RateLimit.Limit); + Assert.Equal(4997, apiInfo.RateLimit.Remaining); + Assert.Equal("5634b0b187fd2e91e3126a75006cc4fa", apiInfo.Etag); + } + [Fact] public void BadHeadersAreIgnored() { From e87b851ad8a442a3da25ba05fe787affc225b8e6 Mon Sep 17 00:00:00 2001 From: Brendan Forster Date: Thu, 9 Apr 2020 12:15:41 -0300 Subject: [PATCH 2/7] workaround case-insensitive headers because we lack access to the response headers --- Octokit/Http/ApiInfoParser.cs | 25 +++++++++++++++++-------- Octokit/Http/RateLimit.cs | 21 +++++++++++++++++++-- 2 files changed, 36 insertions(+), 10 deletions(-) diff --git a/Octokit/Http/ApiInfoParser.cs b/Octokit/Http/ApiInfoParser.cs index 94a51c5649..2d7bf20f8f 100644 --- a/Octokit/Http/ApiInfoParser.cs +++ b/Octokit/Http/ApiInfoParser.cs @@ -16,6 +16,11 @@ internal static class ApiInfoParser static readonly Regex _linkRelRegex = new Regex("rel=\"(next|prev|first|last)\"", regexOptions); static readonly Regex _linkUriRegex = new Regex("<(.+)>", regexOptions); + static KeyValuePair LookupHeader(IDictionary headers, string key) + { + return headers.FirstOrDefault(h => string.Equals(h.Key, key, StringComparison.OrdinalIgnoreCase)); + } + public static ApiInfo ParseResponseHeaders(IDictionary responseHeaders) { Ensure.ArgumentNotNull(responseHeaders, nameof(responseHeaders)); @@ -25,28 +30,32 @@ public static ApiInfo ParseResponseHeaders(IDictionary responseH var acceptedOauthScopes = new List(); string etag = null; - if (responseHeaders.ContainsKey("X-Accepted-OAuth-Scopes")) + var acceptedOauthScopesKey = LookupHeader(responseHeaders, "X-Accepted-OAuth-Scopes"); + if (!acceptedOauthScopesKey.Equals(default(KeyValuePair))) { - acceptedOauthScopes.AddRange(responseHeaders["X-Accepted-OAuth-Scopes"] + acceptedOauthScopes.AddRange(acceptedOauthScopesKey.Value .Split(new[] { ',' }, StringSplitOptions.RemoveEmptyEntries) .Select(x => x.Trim())); } - if (responseHeaders.ContainsKey("X-OAuth-Scopes")) + var oauthScopesKey = LookupHeader(responseHeaders, "X-OAuth-Scopes"); + if (!oauthScopesKey.Equals(default(KeyValuePair))) { - oauthScopes.AddRange(responseHeaders["X-OAuth-Scopes"] + oauthScopes.AddRange(oauthScopesKey.Value .Split(new[] { ',' }, StringSplitOptions.RemoveEmptyEntries) .Select(x => x.Trim())); } - if (responseHeaders.ContainsKey("ETag")) + var etagKey = LookupHeader(responseHeaders, "ETag"); + if (!etagKey.Equals(default(KeyValuePair))) { - etag = responseHeaders["ETag"]; + etag = etagKey.Value; } - if (responseHeaders.ContainsKey("Link")) + var linkKey = LookupHeader(responseHeaders, "Link"); + if (!linkKey.Equals(default(KeyValuePair))) { - var links = responseHeaders["Link"].Split(','); + var links = linkKey.Value.Split(','); foreach (var link in links) { var relMatch = _linkRelRegex.Match(link); diff --git a/Octokit/Http/RateLimit.cs b/Octokit/Http/RateLimit.cs index a5c2f28800..a3d434cf51 100644 --- a/Octokit/Http/RateLimit.cs +++ b/Octokit/Http/RateLimit.cs @@ -3,6 +3,7 @@ using System.Diagnostics; using System.Diagnostics.CodeAnalysis; using System.Globalization; +using System.Linq; #if !NO_SERIALIZABLE using System.Runtime.Serialization; #endif @@ -65,11 +66,27 @@ public RateLimit(int limit, int remaining, long resetAsUtcEpochSeconds) [Parameter(Key = "reset")] public long ResetAsUtcEpochSeconds { get; private set; } + static KeyValuePair LookupHeader(IDictionary headers, string key) + { + return headers.FirstOrDefault(h => string.Equals(h.Key, key, StringComparison.OrdinalIgnoreCase)); + } + static long GetHeaderValueAsInt32Safe(IDictionary responseHeaders, string key) { - string value; long result; - return !responseHeaders.TryGetValue(key, out value) || value == null || !long.TryParse(value, out result) + + var foundKey = LookupHeader(responseHeaders, key); + if (foundKey.Equals(default(KeyValuePair))) + { + return 0; + } + + if (string.IsNullOrWhiteSpace(foundKey.Value)) + { + return 0; + } + + return !long.TryParse(foundKey.Value, out result) ? 0 : result; } From e835013ecc6214e8fd6bde0bbc34b730963bf555 Mon Sep 17 00:00:00 2001 From: Brendan Forster Date: Thu, 9 Apr 2020 13:07:17 -0300 Subject: [PATCH 3/7] address case where header is manually checked --- Octokit.Tests/Exceptions/AbuseExceptionTests.cs | 11 +++++++++++ Octokit/Exceptions/AbuseException.cs | 8 +++++--- 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/Octokit.Tests/Exceptions/AbuseExceptionTests.cs b/Octokit.Tests/Exceptions/AbuseExceptionTests.cs index 17b2616c14..80285bc11f 100644 --- a/Octokit.Tests/Exceptions/AbuseExceptionTests.cs +++ b/Octokit.Tests/Exceptions/AbuseExceptionTests.cs @@ -53,6 +53,17 @@ public void WithRetryAfterHeader_PopulatesRetryAfterSeconds() Assert.Equal(30, abuseException.RetryAfterSeconds); } + [Fact] + public void WithRetryAfterCaseInsensitiveHeader_PopulatesRetryAfterSeconds() + { + var headerDictionary = new Dictionary { { "retry-after", "20" } }; + + var response = new Response(HttpStatusCode.Forbidden, null, headerDictionary, "application/json"); + var abuseException = new AbuseException(response); + + Assert.Equal(20, abuseException.RetryAfterSeconds); + } + [Fact] public void WithZeroHeaderValue_RetryAfterSecondsIsZero() { diff --git a/Octokit/Exceptions/AbuseException.cs b/Octokit/Exceptions/AbuseException.cs index 8fceec4f3a..5e94754b95 100644 --- a/Octokit/Exceptions/AbuseException.cs +++ b/Octokit/Exceptions/AbuseException.cs @@ -1,6 +1,8 @@ using System; +using System.Collections.Generic; using System.Diagnostics; using System.Diagnostics.CodeAnalysis; +using System.Linq; using System.Net; #if !NO_SERIALIZABLE using System.Runtime.Serialization; @@ -44,11 +46,11 @@ public AbuseException(IResponse response, Exception innerException) private static int? ParseRetryAfterSeconds(IResponse response) { - string secondsValue; - if (!response.Headers.TryGetValue("Retry-After", out secondsValue)) { return null; } + var header = response.Headers.FirstOrDefault(h => string.Equals(h.Key, "Retry-After", StringComparison.OrdinalIgnoreCase)); + if (header.Equals(default(KeyValuePair))) { return null; } int retrySeconds; - if (!int.TryParse(secondsValue, out retrySeconds)) { return null; } + if (!int.TryParse(header.Value, out retrySeconds)) { return null; } if (retrySeconds < 0) { return null; } return retrySeconds; From 15148d36c2f4ca05cf413829b1a6800c8a6673fe Mon Sep 17 00:00:00 2001 From: Brendan Forster Date: Thu, 9 Apr 2020 13:30:33 -0300 Subject: [PATCH 4/7] make this workaround more readable --- Octokit/Http/ApiInfoParser.cs | 13 +++++++++---- Octokit/Http/RateLimit.cs | 7 ++++++- 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/Octokit/Http/ApiInfoParser.cs b/Octokit/Http/ApiInfoParser.cs index 2d7bf20f8f..e9cf2fb0bd 100644 --- a/Octokit/Http/ApiInfoParser.cs +++ b/Octokit/Http/ApiInfoParser.cs @@ -21,6 +21,11 @@ static KeyValuePair LookupHeader(IDictionary hea return headers.FirstOrDefault(h => string.Equals(h.Key, key, StringComparison.OrdinalIgnoreCase)); } + static bool Exists(KeyValuePair kvp) + { + return !kvp.Equals(default(KeyValuePair)); + } + public static ApiInfo ParseResponseHeaders(IDictionary responseHeaders) { Ensure.ArgumentNotNull(responseHeaders, nameof(responseHeaders)); @@ -31,7 +36,7 @@ public static ApiInfo ParseResponseHeaders(IDictionary responseH string etag = null; var acceptedOauthScopesKey = LookupHeader(responseHeaders, "X-Accepted-OAuth-Scopes"); - if (!acceptedOauthScopesKey.Equals(default(KeyValuePair))) + if (Exists(acceptedOauthScopesKey)) { acceptedOauthScopes.AddRange(acceptedOauthScopesKey.Value .Split(new[] { ',' }, StringSplitOptions.RemoveEmptyEntries) @@ -39,7 +44,7 @@ public static ApiInfo ParseResponseHeaders(IDictionary responseH } var oauthScopesKey = LookupHeader(responseHeaders, "X-OAuth-Scopes"); - if (!oauthScopesKey.Equals(default(KeyValuePair))) + if (Exists(oauthScopesKey)) { oauthScopes.AddRange(oauthScopesKey.Value .Split(new[] { ',' }, StringSplitOptions.RemoveEmptyEntries) @@ -47,13 +52,13 @@ public static ApiInfo ParseResponseHeaders(IDictionary responseH } var etagKey = LookupHeader(responseHeaders, "ETag"); - if (!etagKey.Equals(default(KeyValuePair))) + if (Exists(etagKey)) { etag = etagKey.Value; } var linkKey = LookupHeader(responseHeaders, "Link"); - if (!linkKey.Equals(default(KeyValuePair))) + if (Exists(linkKey)) { var links = linkKey.Value.Split(','); foreach (var link in links) diff --git a/Octokit/Http/RateLimit.cs b/Octokit/Http/RateLimit.cs index a3d434cf51..016ef4944a 100644 --- a/Octokit/Http/RateLimit.cs +++ b/Octokit/Http/RateLimit.cs @@ -71,12 +71,17 @@ static KeyValuePair LookupHeader(IDictionary hea return headers.FirstOrDefault(h => string.Equals(h.Key, key, StringComparison.OrdinalIgnoreCase)); } + static bool Exists(KeyValuePair kvp) + { + return !kvp.Equals(default(KeyValuePair)); + } + static long GetHeaderValueAsInt32Safe(IDictionary responseHeaders, string key) { long result; var foundKey = LookupHeader(responseHeaders, key); - if (foundKey.Equals(default(KeyValuePair))) + if (!Exists(foundKey)) { return 0; } From b7a01b1c032ab64defac8db7ca9403e8e35c7017 Mon Sep 17 00:00:00 2001 From: Brendan Forster Date: Thu, 9 Apr 2020 14:38:05 -0300 Subject: [PATCH 5/7] another header lookup for containskey --- .../Enterprise/EnterpriseProbeTests.cs | 20 +++++++++++++++---- Octokit/Clients/Enterprise/EnterpriseProbe.cs | 3 ++- 2 files changed, 18 insertions(+), 5 deletions(-) diff --git a/Octokit.Tests/Clients/Enterprise/EnterpriseProbeTests.cs b/Octokit.Tests/Clients/Enterprise/EnterpriseProbeTests.cs index 54df7aba7b..dd5f6849f8 100644 --- a/Octokit.Tests/Clients/Enterprise/EnterpriseProbeTests.cs +++ b/Octokit.Tests/Clients/Enterprise/EnterpriseProbeTests.cs @@ -1,4 +1,5 @@ using System; +using System.Collections.Generic; using System.Net; using System.Net.Http; using System.Reactive.Linq; @@ -19,10 +20,15 @@ public class TheProbeMethod [InlineData(HttpStatusCode.NotFound)] public async Task ReturnsExistsForResponseWithCorrectHeadersRegardlessOfResponse(HttpStatusCode httpStatusCode) { + var headers = new Dictionary() + { + { "Server", "REVERSE-PROXY" }, + { "X-GitHub-Request-Id", Guid.NewGuid().ToString() } + }; var response = Substitute.For(); response.StatusCode.Returns(httpStatusCode); - response.Headers["Server"].Returns("REVERSE-PROXY"); - response.Headers.ContainsKey("X-GitHub-Request-Id").Returns(true); + response.Headers.Returns(headers); + var productHeader = new ProductHeaderValue("GHfW", "99"); var httpClient = Substitute.For(); httpClient.Send(Args.Request, CancellationToken.None).Returns(Task.FromResult(response)); @@ -43,10 +49,16 @@ public async Task ReturnsExistsForResponseWithCorrectHeadersRegardlessOfResponse [Fact] public async Task ReturnsExistsForApiExceptionWithCorrectHeaders() { + + var headers = new Dictionary() + { + { "Server", "GitHub.com" }, + { "X-GitHub-Request-Id", Guid.NewGuid().ToString() } + }; var httpClient = Substitute.For(); var response = Substitute.For(); - response.Headers["Server"].Returns("GitHub.com"); - response.Headers.ContainsKey("X-GitHub-Request-Id").Returns(true); + response.Headers.Returns(headers); + var apiException = new ApiException(response); httpClient.Send(Args.Request, CancellationToken.None).ThrowsAsync(apiException); var productHeader = new ProductHeaderValue("GHfW", "99"); diff --git a/Octokit/Clients/Enterprise/EnterpriseProbe.cs b/Octokit/Clients/Enterprise/EnterpriseProbe.cs index 3584825ea6..45173529a0 100644 --- a/Octokit/Clients/Enterprise/EnterpriseProbe.cs +++ b/Octokit/Clients/Enterprise/EnterpriseProbe.cs @@ -1,4 +1,5 @@ using System; +using System.Linq; using System.Net; using System.Net.Http; using System.Text.RegularExpressions; @@ -103,7 +104,7 @@ public async Task Probe(Uri enterpriseBaseUrl) static bool IsEnterpriseResponse(IResponse response) { - return response.Headers.ContainsKey("X-GitHub-Request-Id"); + return response.Headers.Any(h => string.Equals(h.Key, "X-GitHub-Request-Id", StringComparison.OrdinalIgnoreCase)); } } From 51afe6442ea2b6a343275c85ff3b83295b1c0b6a Mon Sep 17 00:00:00 2001 From: Brendan Forster Date: Thu, 9 Apr 2020 14:40:29 -0300 Subject: [PATCH 6/7] found some dead code --- Octokit/Helpers/ConcurrentCache.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/Octokit/Helpers/ConcurrentCache.cs b/Octokit/Helpers/ConcurrentCache.cs index 404eff0e48..55e6d3d21f 100644 --- a/Octokit/Helpers/ConcurrentCache.cs +++ b/Octokit/Helpers/ConcurrentCache.cs @@ -11,6 +11,7 @@ namespace Octokit /// /// /// + [Obsolete("This component is no longer used, and will be removed in a future update")] public class ConcurrentCache { Dictionary _cache = new Dictionary(); From 2cad24c86edbeaed9fe9b17fcd95f7e3bcefab31 Mon Sep 17 00:00:00 2001 From: Brendan Forster Date: Thu, 9 Apr 2020 14:45:32 -0300 Subject: [PATCH 7/7] format ze code --- Octokit.Tests/Clients/Enterprise/EnterpriseProbeTests.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Octokit.Tests/Clients/Enterprise/EnterpriseProbeTests.cs b/Octokit.Tests/Clients/Enterprise/EnterpriseProbeTests.cs index dd5f6849f8..0d225f58a5 100644 --- a/Octokit.Tests/Clients/Enterprise/EnterpriseProbeTests.cs +++ b/Octokit.Tests/Clients/Enterprise/EnterpriseProbeTests.cs @@ -20,7 +20,7 @@ public class TheProbeMethod [InlineData(HttpStatusCode.NotFound)] public async Task ReturnsExistsForResponseWithCorrectHeadersRegardlessOfResponse(HttpStatusCode httpStatusCode) { - var headers = new Dictionary() + var headers = new Dictionary() { { "Server", "REVERSE-PROXY" }, { "X-GitHub-Request-Id", Guid.NewGuid().ToString() } @@ -50,7 +50,7 @@ public async Task ReturnsExistsForResponseWithCorrectHeadersRegardlessOfResponse public async Task ReturnsExistsForApiExceptionWithCorrectHeaders() { - var headers = new Dictionary() + var headers = new Dictionary() { { "Server", "GitHub.com" }, { "X-GitHub-Request-Id", Guid.NewGuid().ToString() }