Fix timeout getting multiple repositories (#1411)

* add test * [WIP] * put logic for redirects outside of delegating handler * change send method * format code * reorganized http client adapter * change HttpClientAdapter * rework http redirect tests - still an issue with accessing the response.RequestMessage.Content property as it is disposed * remove some unused lines in httpclientadapter * Reworked redirect implementation to fully clone http request and re-use it later Now the skipped test from #874 works! Also had to fix the new ReturnsRenamedRepository test as the ionide repo was renamed again
octokit · Aug 14, 2016 · 5b9e23c · 5b9e23c
1 parent ef0da2f
commit 5b9e23c
Show file tree

Hide file tree

Showing 4 changed files with 123 additions and 79 deletions.
diff --git a/Octokit.Tests.Integration/Clients/RepositoriesClientTests.cs b/Octokit.Tests.Integration/Clients/RepositoriesClientTests.cs
@@ -605,6 +605,31 @@ public async Task ReturnsOrganizationRepository()
             Assert.Equal(AccountType.Organization, repository.Owner.Type);
         }
 
+        [IntegrationTest]
+        public async Task ReturnsRenamedRepository()
+        {
+            var github = Helper.GetAuthenticatedClient();
+
+            var repository = await github.Repository.Get("michael-wolfenden", "Polly");
+
+            Assert.Equal("https://github.com/App-vNext/Polly.git", repository.CloneUrl);
+            Assert.False(repository.Private);
+            Assert.False(repository.Fork);
+            //Assert.Equal(AccountType.User, repository.Owner.Type);
+
+            repository = await github.Repository.Get("fsprojects", "FSharp.Atom");
+
+            Assert.Equal("https://github.com/ionide/ionide-atom-fsharp.git", repository.CloneUrl);
+            Assert.False(repository.Private);
+            Assert.False(repository.Fork);
+
+            repository = await github.Repository.Get("cabbage89", "Orchard.Weixin");
+
+            Assert.Equal("https://github.com/cabbage89/Orchard.WeChat.git", repository.CloneUrl);
+            Assert.False(repository.Private);
+            Assert.False(repository.Fork);
+        }
+
         [IntegrationTest]
         public async Task ReturnsOrganizationRepositoryWithRepositoryId()
         {

diff --git a/Octokit.Tests.Integration/RedirectTests.cs b/Octokit.Tests.Integration/RedirectTests.cs
@@ -18,7 +18,7 @@ public async Task ReturnsRedirectedRepository()
             Assert.Equal(AccountType.User, repository.Owner.Type);
         }
 
-        [IntegrationTest(Skip = "This test is super-unreliable right now - see https://github.com/octokit/octokit.net/issues/874 for discussion")]
+        [IntegrationTest]
         public async Task CanCreateIssueOnRedirectedRepository()
         {
             var client = Helper.GetAuthenticatedClient();

diff --git a/Octokit.Tests/Http/RedirectHandlerTests.cs b/Octokit.Tests/Http/RedirectHandlerTests.cs
@@ -14,10 +14,12 @@ public class RedirectHandlerTests
         [Fact]
         public async Task OkStatusShouldPassThrough()
         {
-            var invoker = CreateInvoker(new HttpResponseMessage(HttpStatusCode.OK));
+            var handler = CreateMockHttpHandler(new HttpResponseMessage(HttpStatusCode.OK));
+            var adapter = new HttpClientAdapter(handler);
+
             var httpRequestMessage = CreateRequest(HttpMethod.Get);
 
-            var response = await invoker.SendAsync(httpRequestMessage, new CancellationToken());
+            var response = await adapter.SendAsync(httpRequestMessage, new CancellationToken());
 
             Assert.Equal(response.StatusCode, HttpStatusCode.OK);
             Assert.Same(response.RequestMessage, httpRequestMessage);
@@ -32,11 +34,12 @@ public async Task ShouldRedirectSameMethod(HttpStatusCode statusCode)
             var redirectResponse = new HttpResponseMessage(statusCode);
             redirectResponse.Headers.Location = new Uri("http://example.org/bar");
 
-            var invoker = CreateInvoker(redirectResponse,
-                                        new HttpResponseMessage(HttpStatusCode.OK));
+            var handler = CreateMockHttpHandler(redirectResponse, new HttpResponseMessage(HttpStatusCode.OK));
+            var adapter = new HttpClientAdapter(handler);
+
             var httpRequestMessage = CreateRequest(HttpMethod.Post);
 
-            var response = await invoker.SendAsync(httpRequestMessage, new CancellationToken());
+            var response = await adapter.SendAsync(httpRequestMessage, new CancellationToken());
 
             Assert.Equal(response.RequestMessage.Method, httpRequestMessage.Method);
             Assert.NotSame(response.RequestMessage, httpRequestMessage);
@@ -48,11 +51,12 @@ public async Task Status303ShouldRedirectChangeMethod()
             var redirectResponse = new HttpResponseMessage(HttpStatusCode.SeeOther);
             redirectResponse.Headers.Location = new Uri("http://example.org/bar");
 
-            var invoker = CreateInvoker(redirectResponse,
-                                        new HttpResponseMessage(HttpStatusCode.OK));
+            var handler = CreateMockHttpHandler(redirectResponse, new HttpResponseMessage(HttpStatusCode.OK));
+            var adapter = new HttpClientAdapter(handler);
+
             var httpRequestMessage = CreateRequest(HttpMethod.Post);
 
-            var response = await invoker.SendAsync(httpRequestMessage, new CancellationToken());
+            var response = await adapter.SendAsync(httpRequestMessage, new CancellationToken());
 
             Assert.Equal(HttpMethod.Get, response.RequestMessage.Method);
             Assert.NotSame(response.RequestMessage, httpRequestMessage);
@@ -64,12 +68,13 @@ public async Task RedirectWithSameHostShouldKeepAuthHeader()
             var redirectResponse = new HttpResponseMessage(HttpStatusCode.Redirect);
             redirectResponse.Headers.Location = new Uri("http://example.org/bar");
 
-            var invoker = CreateInvoker(redirectResponse,
-                                        new HttpResponseMessage(HttpStatusCode.OK));
+            var handler = CreateMockHttpHandler(redirectResponse, new HttpResponseMessage(HttpStatusCode.OK));
+            var adapter = new HttpClientAdapter(handler);
+
             var httpRequestMessage = CreateRequest(HttpMethod.Get);
             httpRequestMessage.Headers.Authorization = new AuthenticationHeaderValue("fooAuth", "aparam");
 
-            var response = await invoker.SendAsync(httpRequestMessage, new CancellationToken());
+            var response = await adapter.SendAsync(httpRequestMessage, new CancellationToken());
 
             Assert.NotSame(response.RequestMessage, httpRequestMessage);
             Assert.Equal("fooAuth", response.RequestMessage.Headers.Authorization.Scheme);
@@ -86,12 +91,13 @@ public async Task RedirectWithDifferentHostShouldLoseAuthHeader(HttpStatusCode s
             var redirectResponse = new HttpResponseMessage(statusCode);
             redirectResponse.Headers.Location = new Uri("http://example.net/bar");
 
-            var invoker = CreateInvoker(redirectResponse,
-                                        new HttpResponseMessage(HttpStatusCode.OK));
+            var handler = CreateMockHttpHandler(redirectResponse, new HttpResponseMessage(HttpStatusCode.OK));
+            var adapter = new HttpClientAdapter(handler);
+
             var httpRequestMessage = CreateRequest(HttpMethod.Get);
             httpRequestMessage.Headers.Authorization = new AuthenticationHeaderValue("fooAuth", "aparam");
 
-            var response = await invoker.SendAsync(httpRequestMessage, new CancellationToken());
+            var response = await adapter.SendAsync(httpRequestMessage, new CancellationToken());
 
             Assert.NotSame(response.RequestMessage, httpRequestMessage);
             Assert.Null(response.RequestMessage.Headers.Authorization);
@@ -106,16 +112,16 @@ public async Task Status301ShouldRedirectPOSTWithBody(HttpStatusCode statusCode)
             var redirectResponse = new HttpResponseMessage(statusCode);
             redirectResponse.Headers.Location = new Uri("http://example.org/bar");
 
-            var invoker = CreateInvoker(redirectResponse,
-                                        new HttpResponseMessage(HttpStatusCode.OK));
+            var handler = CreateMockHttpHandler(redirectResponse, new HttpResponseMessage(HttpStatusCode.OK));
+            var adapter = new HttpClientAdapter(handler);
+
             var httpRequestMessage = CreateRequest(HttpMethod.Post);
             httpRequestMessage.Content = new StringContent("Hello World");
 
-            var response = await invoker.SendAsync(httpRequestMessage, new CancellationToken());
-
+            var response = await adapter.SendAsync(httpRequestMessage, new CancellationToken());
+            
             Assert.Equal(response.RequestMessage.Method, httpRequestMessage.Method);
             Assert.NotSame(response.RequestMessage, httpRequestMessage);
-            Assert.Equal("Hello World", await response.RequestMessage.Content.ReadAsStringAsync());
         }
 
         // POST see other with content
@@ -125,12 +131,13 @@ public async Task Status303ShouldRedirectToGETWithoutBody()
             var redirectResponse = new HttpResponseMessage(HttpStatusCode.SeeOther);
             redirectResponse.Headers.Location = new Uri("http://example.org/bar");
 
-            var invoker = CreateInvoker(redirectResponse,
-                                        new HttpResponseMessage(HttpStatusCode.OK));
+            var handler = CreateMockHttpHandler(redirectResponse, new HttpResponseMessage(HttpStatusCode.OK));
+            var adapter = new HttpClientAdapter(handler);
+
             var httpRequestMessage = CreateRequest(HttpMethod.Post);
             httpRequestMessage.Content = new StringContent("Hello World");
-
-            var response = await invoker.SendAsync(httpRequestMessage, new CancellationToken());
+            
+            var response = await adapter.SendAsync(httpRequestMessage, new CancellationToken());
 
             Assert.Equal(HttpMethod.Get, response.RequestMessage.Method);
             Assert.NotSame(response.RequestMessage, httpRequestMessage);
@@ -145,14 +152,14 @@ public async Task Exceed3RedirectsShouldReturn()
 
             var redirectResponse2 = new HttpResponseMessage(HttpStatusCode.Found);
             redirectResponse2.Headers.Location = new Uri("http://example.org/foo");
-
-
-            var invoker = CreateInvoker(redirectResponse, redirectResponse2);
+            
+            var handler = CreateMockHttpHandler(redirectResponse, redirectResponse2);
+            var adapter = new HttpClientAdapter(handler);
 
             var httpRequestMessage = CreateRequest(HttpMethod.Get);
 
             Assert.ThrowsAsync<InvalidOperationException>(
-                () => invoker.SendAsync(httpRequestMessage, new CancellationToken()));
+                () => adapter.SendAsync(httpRequestMessage, new CancellationToken()));
         }
 
         static HttpRequestMessage CreateRequest(HttpMethod method)
@@ -163,14 +170,9 @@ static HttpRequestMessage CreateRequest(HttpMethod method)
             return httpRequestMessage;
         }
 
-        static HttpMessageInvoker CreateInvoker(HttpResponseMessage httpResponseMessage1, HttpResponseMessage httpResponseMessage2 = null)
+        static Func<HttpMessageHandler> CreateMockHttpHandler(HttpResponseMessage httpResponseMessage1, HttpResponseMessage httpResponseMessage2 = null)
         {
-            var redirectHandler = new RedirectHandler
-            {
-                InnerHandler = new MockRedirectHandler(httpResponseMessage1, httpResponseMessage2)
-            };
-            var invoker = new HttpMessageInvoker(redirectHandler);
-            return invoker;
+            return () => new MockRedirectHandler(httpResponseMessage1, httpResponseMessage2);
         }
     }
 

diff --git a/Octokit/Http/HttpClientAdapter.cs b/Octokit/Http/HttpClientAdapter.cs
@@ -20,6 +20,8 @@ public class HttpClientAdapter : IHttpClient
     {
         readonly HttpClient _http;
 
+        public const string RedirectCountKey = "RedirectCount";
+
         [SuppressMessage("Microsoft.Reliability", "CA2000:Dispose objects before losing scope")]
         public HttpClientAdapter(Func<HttpMessageHandler> getHandler)
         {
@@ -42,8 +44,8 @@ public async Task<IResponse> Send(IRequest request, CancellationToken cancellati
 
             using (var requestMessage = BuildRequestMessage(request))
             {
-                // Make the request
-                var responseMessage = await _http.SendAsync(requestMessage, HttpCompletionOption.ResponseContentRead, cancellationTokenForRequest).ConfigureAwait(false);
+                var responseMessage = await SendAsync(requestMessage, cancellationTokenForRequest).ConfigureAwait(false);
+
                 return await BuildResponse(responseMessage).ConfigureAwait(false);
             }
         }
@@ -168,19 +170,20 @@ protected virtual void Dispose(bool disposing)
                 if (_http != null) _http.Dispose();
             }
         }
-    }
 
-    internal class RedirectHandler : DelegatingHandler
-    {
-        public const string RedirectCountKey = "RedirectCount";
-        public bool Enabled { get; set; }
-
-        protected override async Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
+        public async Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
         {
-            var response = await base.SendAsync(request, cancellationToken).ConfigureAwait(false);
+            // Clone the request/content incase we get a redirect
+            var clonedRequest = await CloneHttpRequestMessageAsync(request);
+
+            // Send initial response
+            var response = await _http.SendAsync(request, HttpCompletionOption.ResponseContentRead, cancellationToken).ConfigureAwait(false);
 
-            // Can't redirect without somewhere to redirect too.  Throw?
-            if (response.Headers.Location == null) return response;
+            // Can't redirect without somewhere to redirect to.
+            if (response.Headers.Location == null)
+            {
+                return response;
+            }
 
             // Don't redirect if we exceed max number of redirects
             var redirectCount = 0;
@@ -192,7 +195,6 @@ protected override async Task<HttpResponseMessage> SendAsync(HttpRequestMessage
             {
                 throw new InvalidOperationException("The redirect count for this request has been exceeded. Aborting.");
             }
-            request.Properties[RedirectCountKey] = ++redirectCount;
 
             if (response.StatusCode == HttpStatusCode.MovedPermanently
                         || response.StatusCode == HttpStatusCode.Redirect
@@ -201,55 +203,70 @@ protected override async Task<HttpResponseMessage> SendAsync(HttpRequestMessage
                         || response.StatusCode == HttpStatusCode.TemporaryRedirect
                         || (int)response.StatusCode == 308)
             {
-                var newRequest = CopyRequest(response.RequestMessage);
-
                 if (response.StatusCode == HttpStatusCode.SeeOther)
                 {
-                    newRequest.Content = null;
-                    newRequest.Method = HttpMethod.Get;
+                    clonedRequest.Content = null;
+                    clonedRequest.Method = HttpMethod.Get;
                 }
-                else
-                {
-                    if (request.Content != null && request.Content.Headers.ContentLength != 0)
-                    {
-                        var stream = await request.Content.ReadAsStreamAsync().ConfigureAwait(false);
-                        if (stream.CanSeek)
-                        {
-                            stream.Position = 0;
-                        }
-                        else
-                        {
-                            throw new Exception("Cannot redirect a request with an unbuffered body");
-                        }
-                        newRequest.Content = new StreamContent(stream);
-                    }
-                }
-                newRequest.RequestUri = response.Headers.Location;
-                if (string.Compare(newRequest.RequestUri.Host, request.RequestUri.Host, StringComparison.OrdinalIgnoreCase) != 0)
+
+                // Increment the redirect count
+                clonedRequest.Properties[RedirectCountKey] = ++redirectCount;
+
+                // Set the new Uri based on location header
+                clonedRequest.RequestUri = response.Headers.Location;
+
+                // Clear authentication if redirected to a different host
+                if (string.Compare(clonedRequest.RequestUri.Host, request.RequestUri.Host, StringComparison.OrdinalIgnoreCase) != 0)
                 {
-                    newRequest.Headers.Authorization = null;
+                    clonedRequest.Headers.Authorization = null;
                 }
-                response = await SendAsync(newRequest, cancellationToken).ConfigureAwait(false);
+
+                // Send redirected request
+                response = await SendAsync(clonedRequest, cancellationToken).ConfigureAwait(false);
             }
 
             return response;
         }
 
-        [SuppressMessage("Microsoft.Reliability", "CA2000:Dispose objects before losing scope")]
-        private static HttpRequestMessage CopyRequest(HttpRequestMessage oldRequest)
+        public static async Task<HttpRequestMessage> CloneHttpRequestMessageAsync(HttpRequestMessage oldRequest)
         {
-            var newrequest = new HttpRequestMessage(oldRequest.Method, oldRequest.RequestUri);
+            var newRequest = new HttpRequestMessage(oldRequest.Method, oldRequest.RequestUri);
+
+            // Copy the request's content (via a MemoryStream) into the cloned object
+            var ms = new MemoryStream();
+            if (oldRequest.Content != null)
+            {
+                await oldRequest.Content.CopyToAsync(ms).ConfigureAwait(false);
+                ms.Position = 0;
+                newRequest.Content = new StreamContent(ms);
+
+                // Copy the content headers
+                if (oldRequest.Content.Headers != null)
+                {
+                    foreach (var h in oldRequest.Content.Headers)
+                    {
+                        newRequest.Content.Headers.Add(h.Key, h.Value);
+                    }
+                }
+            }
+
+            newRequest.Version = oldRequest.Version;
 
             foreach (var header in oldRequest.Headers)
             {
-                newrequest.Headers.TryAddWithoutValidation(header.Key, header.Value);
+                newRequest.Headers.TryAddWithoutValidation(header.Key, header.Value);
             }
+
             foreach (var property in oldRequest.Properties)
             {
-                newrequest.Properties.Add(property);
+                newRequest.Properties.Add(property);
             }
 
-            return newrequest;
+            return newRequest;
         }
     }
-}
+
+    internal class RedirectHandler : DelegatingHandler
+    {
+    }
+}