Retry non-200 GET requests from AdoClient, BbsClient and GithubClient (…

…#620)

<!--
For the checkboxes below you must check each one to indicate that you
either did the relevant task, or considered it and decided there was
nothing that needed doing
-->

Closes #587! ✨ 

This PR ensures that all non-200 GET requests from `AdoClient`,
`BbsClient` and `GithubClient` gets retried via `RetryPolicy.HttpRetry`.
This will allow us to retry on transient network issues for more robust
and consistent migrations.

- [x] Did you write/update appropriate tests
- [x] Release notes updated (if appropriate)
- [x] Appropriate logging output
- [x] Issue linked
- [x] ~~Docs updated (or issue created)~~ (n/a)

<!--
For docs we should review the docs at:

https://docs.github.com/en/early-access/github/migrating-with-github-enterprise-importer
and the README.md in this repo

If a doc update is required based on the changes in this PR, it is
sufficient to create an issue and link to it here. The doc update can be
made later/separately.

The process to update the docs can be found here:
https://github.com/github/docs-early-access#opening-prs

The markdown files are here: 

https://github.com/github/docs-early-access/tree/main/content/github/migrating-with-github-enterprise-importer
-->

Co-authored-by: Dylan Smith <[email protected]>

RELEASENOTES.md

@@ -1 +1 @@
-
+- Retry all failed GET requests made to Github, Azure Devops, and Bitbucket Server.

src/Octoshift/AdoClient.cs

@@ -40,8 +40,7 @@ public AdoClient(OctoLogger log, HttpClient httpClient, IVersionProvider version
 
         public virtual async Task<string> GetAsync(string url)
         {
-            return await _retryPolicy.HttpRetry(async () => await SendAsync(HttpMethod.Get, url),
-                                            ex => ex.StatusCode == HttpStatusCode.ServiceUnavailable);
+            return await _retryPolicy.HttpRetry(async () => await SendAsync(HttpMethod.Get, url), _ => true);
         }
 
         public virtual async Task<string> DeleteAsync(string url) => await SendAsync(HttpMethod.Delete, url);

src/Octoshift/BbsClient.cs

@@ -1,5 +1,4 @@
 using System;
-using System.Net;
 using System.Net.Http;
 using System.Net.Http.Headers;
 using System.Text;
@@ -36,8 +35,7 @@ public BbsClient(OctoLogger log, HttpClient httpClient, IVersionProvider version
 
     public virtual async Task<string> GetAsync(string url)
     {
-        return await _retryPolicy.HttpRetry(async () => await SendAsync(HttpMethod.Get, url),
-                                        ex => ex.StatusCode == HttpStatusCode.ServiceUnavailable);
+        return await _retryPolicy.HttpRetry(async () => await SendAsync(HttpMethod.Get, url), _ => true);
     }
 
     public virtual async Task<string> PostAsync(string url, object body) => await SendAsync(HttpMethod.Post, url, body);
@@ -64,6 +62,8 @@ private async Task<string> SendAsync(HttpMethod httpMethod, string url, object b
         var content = await response.Content.ReadAsStringAsync();
         _log.LogVerbose($"RESPONSE ({response.StatusCode}): {content}");
 
+        response.EnsureSuccessStatusCode();
+
         return content;
 
     }

src/Octoshift/GithubClient.cs

@@ -16,11 +16,13 @@ public class GithubClient
     {
         private readonly HttpClient _httpClient;
         private readonly OctoLogger _log;
+        private readonly RetryPolicy _retryPolicy;
 
-        public GithubClient(OctoLogger log, HttpClient httpClient, IVersionProvider versionProvider, string personalAccessToken)
+        public GithubClient(OctoLogger log, HttpClient httpClient, IVersionProvider versionProvider, RetryPolicy retryPolicy, string personalAccessToken)
         {
             _log = log;
             _httpClient = httpClient;
+            _retryPolicy = retryPolicy;
 
             if (_httpClient != null)
             {
@@ -37,8 +39,15 @@ public GithubClient(OctoLogger log, HttpClient httpClient, IVersionProvider vers
 
         public virtual async Task<string> GetNonSuccessAsync(string url, HttpStatusCode status) => (await SendAsync(HttpMethod.Get, url, status: status)).Content;
 
-        public virtual async Task<string> GetAsync(string url, Dictionary<string, string> customHeaders = null) =>
-            (await SendAsync(HttpMethod.Get, url, customHeaders: customHeaders)).Content;
+        public virtual async Task<string> GetAsync(string url, Dictionary<string, string> customHeaders = null)
+        {
+            var (content, _) = await _retryPolicy.HttpRetry(
+                async () => await SendAsync(HttpMethod.Get, url, customHeaders: customHeaders),
+                _ => true
+            );
+
+            return content;
+        }
 
         public virtual async IAsyncEnumerable<JToken> GetAllAsync(string url, Dictionary<string, string> customHeaders = null)
         {

src/OctoshiftCLI.IntegrationTests/AdoToGithub.cs

@@ -36,7 +36,7 @@ public AdoToGithub(ITestOutputHelper output)
 
             var githubToken = Environment.GetEnvironmentVariable("GHEC_PAT");
             _githubHttpClient = new HttpClient();
-            var githubClient = new GithubClient(logger, _githubHttpClient, new VersionChecker(_versionClient, logger), githubToken);
+            var githubClient = new GithubClient(logger, _githubHttpClient, new VersionChecker(_versionClient, logger), new RetryPolicy(logger), githubToken);
             var githubApi = new GithubApi(githubClient, "https://api.github.com", new RetryPolicy(logger));
 
             _tokens = new Dictionary<string, string>

src/OctoshiftCLI.IntegrationTests/GhesToGithub.cs

@@ -47,11 +47,11 @@ public GhesToGithub(ITestOutputHelper output)
         _versionClient = new HttpClient();
 
         _sourceGithubHttpClient = new HttpClient();
-        _sourceGithubClient = new GithubClient(logger, _sourceGithubHttpClient, new VersionChecker(_versionClient, logger), sourceGithubToken);
+        _sourceGithubClient = new GithubClient(logger, _sourceGithubHttpClient, new VersionChecker(_versionClient, logger), new RetryPolicy(logger), sourceGithubToken);
         _sourceGithubApi = new GithubApi(_sourceGithubClient, GHES_API_URL, new RetryPolicy(logger));
 
         _targetGithubHttpClient = new HttpClient();
-        _targetGithubClient = new GithubClient(logger, _targetGithubHttpClient, new VersionChecker(_versionClient, logger), targetGithubToken);
+        _targetGithubClient = new GithubClient(logger, _targetGithubHttpClient, new VersionChecker(_versionClient, logger), new RetryPolicy(logger), targetGithubToken);
         _targetGithubApi = new GithubApi(_targetGithubClient, "https://api.github.com", new RetryPolicy(logger));
 
         _blobServiceClient = new BlobServiceClient(azureStorageConnectionString);

src/OctoshiftCLI.IntegrationTests/GithubToGithub.cs

@@ -33,7 +33,7 @@ public GithubToGithub(ITestOutputHelper output)
 
             _githubHttpClient = new HttpClient();
             _versionClient = new HttpClient();
-            _githubClient = new GithubClient(logger, _githubHttpClient, new VersionChecker(_versionClient, logger), githubToken);
+            _githubClient = new GithubClient(logger, _githubHttpClient, new VersionChecker(_versionClient, logger), new RetryPolicy(logger), githubToken);
             _githubApi = new GithubApi(_githubClient, "https://api.github.com", new RetryPolicy(logger));
 
             _helper = new TestHelper(_output, _githubApi, _githubClient);

src/OctoshiftCLI.Tests/AdoClientTests.cs

@@ -76,6 +76,41 @@ public async Task GetAsync_Encodes_The_Url()
                 ItExpr.IsAny<CancellationToken>());
         }
 
+        [Theory]
+        [InlineData(HttpStatusCode.Unauthorized)]
+        [InlineData(HttpStatusCode.ServiceUnavailable)]
+        public async Task GetAsync_Retries_On_Non_Success(HttpStatusCode httpStatusCode)
+        {
+            // Arrange
+            using var firstHttpResponse = new HttpResponseMessage(httpStatusCode)
+            {
+                Content = new StringContent("FIRST_RESPONSE")
+            };
+            using var secondHttpResponse = new HttpResponseMessage(HttpStatusCode.OK)
+            {
+                Content = new StringContent("SECOND_RESPONSE")
+            };
+            var handlerMock = new Mock<HttpMessageHandler>();
+            handlerMock
+                .Protected()
+                .SetupSequence<Task<HttpResponseMessage>>(
+                    "SendAsync",
+                    ItExpr.Is<HttpRequestMessage>(req => req.Method == HttpMethod.Get),
+                    ItExpr.IsAny<CancellationToken>())
+                .ReturnsAsync(firstHttpResponse)
+                .ReturnsAsync(secondHttpResponse);
+
+            using var httpClient = new HttpClient(handlerMock.Object);
+            var adoClient = new AdoClient(_mockOctoLogger.Object, httpClient, null, _retryPolicy, PERSONAL_ACCESS_TOKEN);
+
+            // Act
+            var returnedContent = await adoClient.GetAsync(URL);
+
+            // Assert
+            returnedContent.Should().Be("SECOND_RESPONSE");
+        }
+
+
         [Fact]
         public async Task GetAsync_Applies_Retry_Delay()
         {
@@ -188,17 +223,22 @@ public async Task GetAsync_Logs_The_Response_Status_Code_And_Content()
         public async Task GetAsync_Throws_HttpRequestException_On_Non_Success_Response()
         {
             // Arrange
-            using var httpResponse = new HttpResponseMessage(HttpStatusCode.InternalServerError);
-            var handlerMock = MockHttpHandler(_ => true, httpResponse);
+            var httpResponse = () => new HttpResponseMessage(HttpStatusCode.InternalServerError);
+            var handlerMock = new Mock<HttpMessageHandler>();
+            handlerMock
+                .Protected()
+                .Setup<Task<HttpResponseMessage>>(
+                    "SendAsync", ItExpr.IsAny<HttpRequestMessage>(), ItExpr.IsAny<CancellationToken>())
+                .ReturnsAsync(httpResponse);
 
             // Act
             // Assert
             await FluentActions
-                .Invoking(() =>
+                .Invoking(async () =>
                 {
                     using var httpClient = new HttpClient(handlerMock.Object);
                     var adoClient = new AdoClient(_mockOctoLogger.Object, httpClient, null, _retryPolicy, PERSONAL_ACCESS_TOKEN);
-                    return adoClient.GetAsync(URL);
+                    return await adoClient.GetAsync(URL);
                 })
                 .Should()
                 .ThrowExactlyAsync<HttpRequestException>();

src/OctoshiftCLI.Tests/BbsClientTests.cs

@@ -115,6 +115,40 @@ public async Task GetAsync_Logs_The_Response_Status_Code_And_Content()
         _mockOctoLogger.Verify(m => m.LogVerbose($"RESPONSE ({_httpResponse.StatusCode}): {EXPECTED_RESPONSE_CONTENT}"), Times.Once);
     }
 
+    [Theory]
+    [InlineData(HttpStatusCode.Unauthorized)]
+    [InlineData(HttpStatusCode.ServiceUnavailable)]
+    public async Task GetAsync_Retries_On_Non_Success(HttpStatusCode httpStatusCode)
+    {
+        // Arrange
+        using var firstHttpResponse = new HttpResponseMessage(httpStatusCode)
+        {
+            Content = new StringContent("FIRST_RESPONSE")
+        };
+        using var secondHttpResponse = new HttpResponseMessage(HttpStatusCode.OK)
+        {
+            Content = new StringContent("SECOND_RESPONSE")
+        };
+        var handlerMock = new Mock<HttpMessageHandler>();
+        handlerMock
+            .Protected()
+            .SetupSequence<Task<HttpResponseMessage>>(
+                "SendAsync",
+                ItExpr.Is<HttpRequestMessage>(req => req.Method == HttpMethod.Get),
+                ItExpr.IsAny<CancellationToken>())
+            .ReturnsAsync(firstHttpResponse)
+            .ReturnsAsync(secondHttpResponse);
+
+        using var httpClient = new HttpClient(handlerMock.Object);
+        var bbsClient = new BbsClient(_mockOctoLogger.Object, httpClient, null, _retryPolicy, USERNAME, PASSWORD);
+
+        // Act
+        var returnedContent = await bbsClient.GetAsync(URL);
+
+        // Assert
+        returnedContent.Should().Be("SECOND_RESPONSE");
+    }
+
     [Fact]
     public async Task PostAsync_Encodes_The_Url()
     {