From bce9696b9722a7a158448a2a2af8fdc76432c9f3 Mon Sep 17 00:00:00 2001 From: lrz-hal Date: Tue, 28 Jun 2016 20:52:04 +0200 Subject: [PATCH 01/10] add test --- .../Clients/RepositoriesClientTests.cs | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/Octokit.Tests.Integration/Clients/RepositoriesClientTests.cs b/Octokit.Tests.Integration/Clients/RepositoriesClientTests.cs index 6f65dac2ec..14a87a46ef 100644 --- a/Octokit.Tests.Integration/Clients/RepositoriesClientTests.cs +++ b/Octokit.Tests.Integration/Clients/RepositoriesClientTests.cs @@ -604,6 +604,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-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() { From 84f0e7b6d7d75c9e45f6e369588f8cf809098e4d Mon Sep 17 00:00:00 2001 From: Martin Scholz Date: Mon, 4 Jul 2016 18:24:40 +0200 Subject: [PATCH 02/10] [WIP] --- Octokit/Http/HttpClientAdapter.cs | 173 +++++++++++++++++------------- 1 file changed, 98 insertions(+), 75 deletions(-) diff --git a/Octokit/Http/HttpClientAdapter.cs b/Octokit/Http/HttpClientAdapter.cs index 16bb88682d..6403fe7749 100644 --- a/Octokit/Http/HttpClientAdapter.cs +++ b/Octokit/Http/HttpClientAdapter.cs @@ -40,12 +40,103 @@ public async Task Send(IRequest request, CancellationToken cancellati var cancellationTokenForRequest = GetCancellationTokenForRequest(request, cancellationToken); - using (var requestMessage = BuildRequestMessage(request)) + var response = await GetSuccessfulResponse(() => GetRequest(request), cancellationTokenForRequest); + + return await BuildResponse(response).ConfigureAwait(false); + } + + private HttpRequestMessage GetRequest(IRequest request) + { + var requestMessage = BuildRequestMessage(request); + return requestMessage; + } + + private async Task GetSuccessfulResponse(Func request, CancellationToken cancellationToken) + { + var requestToSend = request(); + var response = await SendRequest(requestToSend, cancellationToken).ConfigureAwait(false); + + // Can't redirect without somewhere to redirect too. Throw? + if (response.Headers.Location == null) return response; + + if (response.StatusCode == HttpStatusCode.SeeOther) + { + requestToSend.Content = null; + requestToSend.Method = HttpMethod.Get; + } + else + { + if (requestToSend.Content != null && response.RequestMessage.Content.Headers.ContentLength != 0) + { + var stream = await response.RequestMessage.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); + } + else + { + newRequest.Content = null; + } + } + newRequest.RequestUri = response.Headers.Location; + + if (string.Compare(newRequest.RequestUri.Host, newRequest.RequestUri.Host, StringComparison.OrdinalIgnoreCase) != 0) { - // Make the request - var responseMessage = await _http.SendAsync(requestMessage, HttpCompletionOption.ResponseContentRead, cancellationTokenForRequest).ConfigureAwait(false); - return await BuildResponse(responseMessage).ConfigureAwait(false); + newRequest.Headers.Authorization = null; + } + + // Don't redirect if we exceed max number of redirects + //var redirectCount = 0; + //if (requestMessage.Properties.Keys.Contains(RedirectCountKey)) + //{ + // redirectCount = (int)request.Properties[RedirectCountKey]; + //} + //if (redirectCount > 3) + //{ + // 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 + || response.StatusCode == HttpStatusCode.Found + || response.StatusCode == HttpStatusCode.SeeOther + || response.StatusCode == HttpStatusCode.TemporaryRedirect + || (int)response.StatusCode == 308) + { + response = await GetSuccessfulResponse(() => CopyRequest(response.RequestMessage, response), cancellationToken).ConfigureAwait(false); } + + return response; + } + + private async Task SendRequest(HttpRequestMessage request, CancellationToken cancellationToken) + { + var response = await _http.SendAsync(request, HttpCompletionOption.ResponseContentRead, cancellationToken).ConfigureAwait(false); + return response; + } + + [SuppressMessage("Microsoft.Reliability", "CA2000:Dispose objects before losing scope")] + private HttpRequestMessage CopyRequest(HttpRequestMessage oldRequest, HttpResponseMessage response) + { + var newRequest = new HttpRequestMessage(oldRequest.Method, oldRequest.RequestUri); + + foreach (var header in oldRequest.Headers) + { + newRequest.Headers.TryAddWithoutValidation(header.Key, header.Value); + } + foreach (var property in oldRequest.Properties) + { + newRequest.Properties.Add(property); + } + + return newRequest; } [SuppressMessage("Microsoft.Reliability", "CA2000:Dispose objects before losing scope")] @@ -177,79 +268,11 @@ internal class RedirectHandler : DelegatingHandler protected override async Task SendAsync(HttpRequestMessage request, CancellationToken cancellationToken) { - var response = await base.SendAsync(request, cancellationToken).ConfigureAwait(false); - - // Can't redirect without somewhere to redirect too. Throw? - if (response.Headers.Location == null) return response; - - // Don't redirect if we exceed max number of redirects - var redirectCount = 0; - if (request.Properties.Keys.Contains(RedirectCountKey)) - { - redirectCount = (int)request.Properties[RedirectCountKey]; - } - if (redirectCount > 3) - { - 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 - || response.StatusCode == HttpStatusCode.Found - || response.StatusCode == HttpStatusCode.SeeOther - || response.StatusCode == HttpStatusCode.TemporaryRedirect - || (int)response.StatusCode == 308) - { - var newRequest = CopyRequest(response.RequestMessage); + return await base.SendAsync(request, cancellationToken).ConfigureAwait(false); - if (response.StatusCode == HttpStatusCode.SeeOther) - { - newRequest.Content = null; - newRequest.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) - { - newRequest.Headers.Authorization = null; - } - response = await SendAsync(newRequest, cancellationToken).ConfigureAwait(false); - } - - return response; + } - [SuppressMessage("Microsoft.Reliability", "CA2000:Dispose objects before losing scope")] - private static HttpRequestMessage CopyRequest(HttpRequestMessage oldRequest) - { - var newrequest = new HttpRequestMessage(oldRequest.Method, oldRequest.RequestUri); - - foreach (var header in oldRequest.Headers) - { - newrequest.Headers.TryAddWithoutValidation(header.Key, header.Value); - } - foreach (var property in oldRequest.Properties) - { - newrequest.Properties.Add(property); - } - - return newrequest; - } + } } From b4d0ea97565a98ed6bdcaece80e53d3553e6efd8 Mon Sep 17 00:00:00 2001 From: Martin Scholz Date: Tue, 5 Jul 2016 10:30:51 +0200 Subject: [PATCH 03/10] put logic for redirects outside of delegating handler --- Octokit/Http/HttpClientAdapter.cs | 101 +++++++++++++++--------------- 1 file changed, 50 insertions(+), 51 deletions(-) diff --git a/Octokit/Http/HttpClientAdapter.cs b/Octokit/Http/HttpClientAdapter.cs index 6403fe7749..735bd54404 100644 --- 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 getHandler) { @@ -40,7 +42,7 @@ public async Task Send(IRequest request, CancellationToken cancellati var cancellationTokenForRequest = GetCancellationTokenForRequest(request, cancellationToken); - var response = await GetSuccessfulResponse(() => GetRequest(request), cancellationTokenForRequest); + var response = await GetSuccessfulResponse(GetRequest(request), cancellationTokenForRequest); return await BuildResponse(response).ConfigureAwait(false); } @@ -51,79 +53,81 @@ private HttpRequestMessage GetRequest(IRequest request) return requestMessage; } - private async Task GetSuccessfulResponse(Func request, CancellationToken cancellationToken) + private async Task GetSuccessfulResponse(HttpRequestMessage request, CancellationToken cancellationToken) { - var requestToSend = request(); - var response = await SendRequest(requestToSend, cancellationToken).ConfigureAwait(false); + var response = await SendRequest(request, cancellationToken).ConfigureAwait(false); // Can't redirect without somewhere to redirect too. Throw? if (response.Headers.Location == null) return response; - if (response.StatusCode == HttpStatusCode.SeeOther) + // Don't redirect if we exceed max number of redirects + var redirectCount = 0; + if (request.Properties.Keys.Contains(RedirectCountKey)) + { + redirectCount = (int)request.Properties[RedirectCountKey]; + } + if (redirectCount > 3) { - requestToSend.Content = null; - requestToSend.Method = HttpMethod.Get; + throw new InvalidOperationException("The redirect count for this request has been exceeded. Aborting."); } - else + request.Properties[RedirectCountKey] = ++redirectCount; + + if (response.StatusCode == HttpStatusCode.MovedPermanently + || response.StatusCode == HttpStatusCode.Redirect + || response.StatusCode == HttpStatusCode.Found + || response.StatusCode == HttpStatusCode.SeeOther + || response.StatusCode == HttpStatusCode.TemporaryRedirect + || (int)response.StatusCode == 308) { - if (requestToSend.Content != null && response.RequestMessage.Content.Headers.ContentLength != 0) + var newRequest = CopyRequest(request); + + if (response.StatusCode == HttpStatusCode.SeeOther) { - var stream = await response.RequestMessage.Content.ReadAsStreamAsync().ConfigureAwait(false); - if (stream.CanSeek) + request.Content = null; + request.Method = HttpMethod.Get; + } + else + { + if (request.Content != null && request.Content.Headers.ContentLength != 0) { - stream.Position = 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); } else { - throw new Exception("Cannot redirect a request with an unbuffered body"); + newRequest.Content = null; } - newRequest.Content = new StreamContent(stream); } - else + newRequest.RequestUri = response.Headers.Location; + + if (string.Compare(newRequest.RequestUri.Host, newRequest.RequestUri.Host, StringComparison.OrdinalIgnoreCase) != 0) { - newRequest.Content = null; + newRequest.Headers.Authorization = null; } - } - newRequest.RequestUri = response.Headers.Location; - if (string.Compare(newRequest.RequestUri.Host, newRequest.RequestUri.Host, StringComparison.OrdinalIgnoreCase) != 0) - { - newRequest.Headers.Authorization = null; - } - - // Don't redirect if we exceed max number of redirects - //var redirectCount = 0; - //if (requestMessage.Properties.Keys.Contains(RedirectCountKey)) - //{ - // redirectCount = (int)request.Properties[RedirectCountKey]; - //} - //if (redirectCount > 3) - //{ - // 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 - || response.StatusCode == HttpStatusCode.Found - || response.StatusCode == HttpStatusCode.SeeOther - || response.StatusCode == HttpStatusCode.TemporaryRedirect - || (int)response.StatusCode == 308) - { - response = await GetSuccessfulResponse(() => CopyRequest(response.RequestMessage, response), cancellationToken).ConfigureAwait(false); + response = await GetSuccessfulResponse(newRequest, cancellationToken).ConfigureAwait(false); } return response; } private async Task SendRequest(HttpRequestMessage request, CancellationToken cancellationToken) - { + { var response = await _http.SendAsync(request, HttpCompletionOption.ResponseContentRead, cancellationToken).ConfigureAwait(false); return response; - } + } [SuppressMessage("Microsoft.Reliability", "CA2000:Dispose objects before losing scope")] - private HttpRequestMessage CopyRequest(HttpRequestMessage oldRequest, HttpResponseMessage response) + private static HttpRequestMessage CopyRequest(HttpRequestMessage oldRequest) { var newRequest = new HttpRequestMessage(oldRequest.Method, oldRequest.RequestUri); @@ -262,17 +266,12 @@ protected virtual void Dispose(bool disposing) } internal class RedirectHandler : DelegatingHandler - { - public const string RedirectCountKey = "RedirectCount"; + { public bool Enabled { get; set; } protected override async Task SendAsync(HttpRequestMessage request, CancellationToken cancellationToken) { return await base.SendAsync(request, cancellationToken).ConfigureAwait(false); - - } - - } } From d2748c333be4a9170ceca6fbcedbcc674ce7be78 Mon Sep 17 00:00:00 2001 From: Martin Scholz Date: Tue, 5 Jul 2016 10:43:55 +0200 Subject: [PATCH 04/10] change send method --- Octokit/Http/HttpClientAdapter.cs | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/Octokit/Http/HttpClientAdapter.cs b/Octokit/Http/HttpClientAdapter.cs index 735bd54404..0c1f1eb0ee 100644 --- a/Octokit/Http/HttpClientAdapter.cs +++ b/Octokit/Http/HttpClientAdapter.cs @@ -42,15 +42,12 @@ public async Task Send(IRequest request, CancellationToken cancellati var cancellationTokenForRequest = GetCancellationTokenForRequest(request, cancellationToken); - var response = await GetSuccessfulResponse(GetRequest(request), cancellationTokenForRequest); - - return await BuildResponse(response).ConfigureAwait(false); - } + using(var requestMessage = BuildRequestMessage(request)) + { + var response = await GetSuccessfulResponse(requestMessage, cancellationTokenForRequest); - private HttpRequestMessage GetRequest(IRequest request) - { - var requestMessage = BuildRequestMessage(request); - return requestMessage; + return await BuildResponse(response).ConfigureAwait(false); + } } private async Task GetSuccessfulResponse(HttpRequestMessage request, CancellationToken cancellationToken) From f3efff1dea6ddba22b810b0eecceccac20d644f8 Mon Sep 17 00:00:00 2001 From: Martin Scholz Date: Tue, 5 Jul 2016 10:46:18 +0200 Subject: [PATCH 05/10] format code --- Octokit/Http/HttpClientAdapter.cs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/Octokit/Http/HttpClientAdapter.cs b/Octokit/Http/HttpClientAdapter.cs index 0c1f1eb0ee..8ec7354166 100644 --- a/Octokit/Http/HttpClientAdapter.cs +++ b/Octokit/Http/HttpClientAdapter.cs @@ -42,7 +42,7 @@ public async Task Send(IRequest request, CancellationToken cancellati var cancellationTokenForRequest = GetCancellationTokenForRequest(request, cancellationToken); - using(var requestMessage = BuildRequestMessage(request)) + using (var requestMessage = BuildRequestMessage(request)) { var response = await GetSuccessfulResponse(requestMessage, cancellationTokenForRequest); @@ -110,7 +110,6 @@ private async Task GetSuccessfulResponse(HttpRequestMessage newRequest.Headers.Authorization = null; } - response = await GetSuccessfulResponse(newRequest, cancellationToken).ConfigureAwait(false); } @@ -263,7 +262,7 @@ protected virtual void Dispose(bool disposing) } internal class RedirectHandler : DelegatingHandler - { + { public bool Enabled { get; set; } protected override async Task SendAsync(HttpRequestMessage request, CancellationToken cancellationToken) From b42240b7721b7748d4597ab2d69b9cd00902a433 Mon Sep 17 00:00:00 2001 From: Martin Scholz Date: Tue, 5 Jul 2016 14:11:56 +0200 Subject: [PATCH 06/10] reorganized http client adapter --- Octokit/Http/HttpClientAdapter.cs | 177 ++++++++++++++---------------- 1 file changed, 83 insertions(+), 94 deletions(-) diff --git a/Octokit/Http/HttpClientAdapter.cs b/Octokit/Http/HttpClientAdapter.cs index 8ec7354166..ad46866c93 100644 --- a/Octokit/Http/HttpClientAdapter.cs +++ b/Octokit/Http/HttpClientAdapter.cs @@ -44,101 +44,12 @@ public async Task Send(IRequest request, CancellationToken cancellati using (var requestMessage = BuildRequestMessage(request)) { - var response = await GetSuccessfulResponse(requestMessage, cancellationTokenForRequest); + var responseMessage = await SendAsync(requestMessage, cancellationTokenForRequest).ConfigureAwait(false); - return await BuildResponse(response).ConfigureAwait(false); + return await BuildResponse(responseMessage).ConfigureAwait(false); } } - private async Task GetSuccessfulResponse(HttpRequestMessage request, CancellationToken cancellationToken) - { - var response = await SendRequest(request, cancellationToken).ConfigureAwait(false); - - // Can't redirect without somewhere to redirect too. Throw? - if (response.Headers.Location == null) return response; - - // Don't redirect if we exceed max number of redirects - var redirectCount = 0; - if (request.Properties.Keys.Contains(RedirectCountKey)) - { - redirectCount = (int)request.Properties[RedirectCountKey]; - } - if (redirectCount > 3) - { - 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 - || response.StatusCode == HttpStatusCode.Found - || response.StatusCode == HttpStatusCode.SeeOther - || response.StatusCode == HttpStatusCode.TemporaryRedirect - || (int)response.StatusCode == 308) - { - var newRequest = CopyRequest(request); - - if (response.StatusCode == HttpStatusCode.SeeOther) - { - request.Content = null; - request.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); - } - else - { - newRequest.Content = null; - } - } - newRequest.RequestUri = response.Headers.Location; - - if (string.Compare(newRequest.RequestUri.Host, newRequest.RequestUri.Host, StringComparison.OrdinalIgnoreCase) != 0) - { - newRequest.Headers.Authorization = null; - } - - response = await GetSuccessfulResponse(newRequest, cancellationToken).ConfigureAwait(false); - } - - return response; - } - - private async Task SendRequest(HttpRequestMessage request, CancellationToken cancellationToken) - { - var response = await _http.SendAsync(request, HttpCompletionOption.ResponseContentRead, cancellationToken).ConfigureAwait(false); - return response; - } - - [SuppressMessage("Microsoft.Reliability", "CA2000:Dispose objects before losing scope")] - private static HttpRequestMessage CopyRequest(HttpRequestMessage oldRequest) - { - var newRequest = new HttpRequestMessage(oldRequest.Method, oldRequest.RequestUri); - - foreach (var header in oldRequest.Headers) - { - newRequest.Headers.TryAddWithoutValidation(header.Key, header.Value); - } - foreach (var property in oldRequest.Properties) - { - newRequest.Properties.Add(property); - } - - return newRequest; - } - [SuppressMessage("Microsoft.Reliability", "CA2000:Dispose objects before losing scope")] static CancellationToken GetCancellationTokenForRequest(IRequest request, CancellationToken cancellationToken) { @@ -259,15 +170,93 @@ protected virtual void Dispose(bool disposing) if (_http != null) _http.Dispose(); } } + + private async Task SendAsync(HttpRequestMessage request, CancellationToken cancellationToken) + { + var response = await _http.SendAsync(request, HttpCompletionOption.ResponseContentRead, cancellationToken).ConfigureAwait(false); + + // Can't redirect without somewhere to redirect to. Throw? + if (response.Headers.Location == null) return response; + + // Don't redirect if we exceed max number of redirects + var redirectCount = 0; + if (request.Properties.Keys.Contains(RedirectCountKey)) + { + redirectCount = (int)request.Properties[RedirectCountKey]; + } + if (redirectCount > 3) + { + 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 + || response.StatusCode == HttpStatusCode.Found + || response.StatusCode == HttpStatusCode.SeeOther + || 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; + } + 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) + { + newRequest.Headers.Authorization = null; + } + + response = await SendAsync(newRequest, cancellationToken).ConfigureAwait(false); + } + + return response; + } + + [SuppressMessage("Microsoft.Reliability", "CA2000:Dispose objects before losing scope")] + private static HttpRequestMessage CopyRequest(HttpRequestMessage oldRequest) + { + var newRequest = new HttpRequestMessage(oldRequest.Method, oldRequest.RequestUri); + + foreach (var header in oldRequest.Headers) + { + newRequest.Headers.TryAddWithoutValidation(header.Key, header.Value); + } + foreach (var property in oldRequest.Properties) + { + newRequest.Properties.Add(property); + } + + return newRequest; + } } internal class RedirectHandler : DelegatingHandler { public bool Enabled { get; set; } - protected override async Task SendAsync(HttpRequestMessage request, CancellationToken cancellationToken) + protected override Task SendAsync(HttpRequestMessage request, CancellationToken cancellationToken) { - return await base.SendAsync(request, cancellationToken).ConfigureAwait(false); + return base.SendAsync(request, cancellationToken); } } -} +} \ No newline at end of file From e6309cbf1405029cc489d573af06d556e7d06336 Mon Sep 17 00:00:00 2001 From: maddin2016 Date: Mon, 18 Jul 2016 11:51:12 +0200 Subject: [PATCH 07/10] change HttpClientAdapter --- Octokit/Http/HttpClientAdapter.cs | 39 ++++++++++++++++++------------- 1 file changed, 23 insertions(+), 16 deletions(-) diff --git a/Octokit/Http/HttpClientAdapter.cs b/Octokit/Http/HttpClientAdapter.cs index 16bb88682d..ad46866c93 100644 --- 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 getHandler) { @@ -42,8 +44,8 @@ public async Task 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,18 +170,12 @@ 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 SendAsync(HttpRequestMessage request, CancellationToken cancellationToken) + private async Task SendAsync(HttpRequestMessage request, CancellationToken cancellationToken) { - var response = await base.SendAsync(request, cancellationToken).ConfigureAwait(false); + var response = await _http.SendAsync(request, HttpCompletionOption.ResponseContentRead, cancellationToken).ConfigureAwait(false); - // Can't redirect without somewhere to redirect too. Throw? + // Can't redirect without somewhere to redirect to. Throw? if (response.Headers.Location == null) return response; // Don't redirect if we exceed max number of redirects @@ -229,6 +225,7 @@ protected override async Task SendAsync(HttpRequestMessage { newRequest.Headers.Authorization = null; } + response = await SendAsync(newRequest, cancellationToken).ConfigureAwait(false); } @@ -238,18 +235,28 @@ protected override async Task SendAsync(HttpRequestMessage [SuppressMessage("Microsoft.Reliability", "CA2000:Dispose objects before losing scope")] private static HttpRequestMessage CopyRequest(HttpRequestMessage oldRequest) { - var newrequest = new HttpRequestMessage(oldRequest.Method, oldRequest.RequestUri); + var newRequest = new HttpRequestMessage(oldRequest.Method, oldRequest.RequestUri); 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 + { + public bool Enabled { get; set; } + + protected override Task SendAsync(HttpRequestMessage request, CancellationToken cancellationToken) + { + return base.SendAsync(request, cancellationToken); } } -} +} \ No newline at end of file From 54225c94bc6c12177f7ecf70698199e7d17b3bfe Mon Sep 17 00:00:00 2001 From: Ryan Gribble Date: Sat, 16 Jul 2016 17:51:50 +1000 Subject: [PATCH 08/10] rework http redirect tests - still an issue with accessing the response.RequestMessage.Content property as it is disposed --- Octokit.Tests/Http/RedirectHandlerTests.cs | 72 ++++++++++++---------- Octokit/Http/HttpClientAdapter.cs | 34 ++++++---- 2 files changed, 60 insertions(+), 46 deletions(-) diff --git a/Octokit.Tests/Http/RedirectHandlerTests.cs b/Octokit.Tests/Http/RedirectHandlerTests.cs index 0d68b0ff02..581fe48317 100644 --- 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,18 @@ 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()); + //TODO: response.RequestMessage.Content has been disposed so cant access it here + //Assert.Equal("Hello World", await response.RequestMessage.Content.ReadAsStringAsync()); } // POST see other with content @@ -125,12 +133,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 +154,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( - () => invoker.SendAsync(httpRequestMessage, new CancellationToken())); + () => adapter.SendAsync(httpRequestMessage, new CancellationToken())); } static HttpRequestMessage CreateRequest(HttpMethod method) @@ -163,14 +172,9 @@ static HttpRequestMessage CreateRequest(HttpMethod method) return httpRequestMessage; } - static HttpMessageInvoker CreateInvoker(HttpResponseMessage httpResponseMessage1, HttpResponseMessage httpResponseMessage2 = null) + static Func 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 index ad46866c93..fa82d4bb03 100644 --- a/Octokit/Http/HttpClientAdapter.cs +++ b/Octokit/Http/HttpClientAdapter.cs @@ -171,8 +171,26 @@ protected virtual void Dispose(bool disposing) } } - private async Task SendAsync(HttpRequestMessage request, CancellationToken cancellationToken) + public async Task SendAsync(HttpRequestMessage request, CancellationToken cancellationToken) { + StreamContent savedContent = null; + bool unbuffered = false; + if (request.Content != null && request.Content.Headers.ContentLength != 0) + { + var stream = await request.Content.ReadAsStreamAsync().ConfigureAwait(false); + if (stream.CanSeek) + { + stream.Position = 0; + savedContent = new StreamContent(stream); + } + else + { + unbuffered = true; + //throw new Exception("Cannot redirect a request with an unbuffered body"); + //savedContent = null; + } + } + var response = await _http.SendAsync(request, HttpCompletionOption.ResponseContentRead, cancellationToken).ConfigureAwait(false); // Can't redirect without somewhere to redirect to. Throw? @@ -206,19 +224,11 @@ private async Task SendAsync(HttpRequestMessage request, Ca } else { - if (request.Content != null && request.Content.Headers.ContentLength != 0) + if (unbuffered) { - 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); + throw new Exception("Cannot redirect a request with an unbuffered body"); } + newRequest.Content = savedContent; } newRequest.RequestUri = response.Headers.Location; if (string.Compare(newRequest.RequestUri.Host, request.RequestUri.Host, StringComparison.OrdinalIgnoreCase) != 0) From 802091c4805c9239b84911bd9396f173337d8e1b Mon Sep 17 00:00:00 2001 From: Martin Scholz Date: Wed, 10 Aug 2016 10:56:13 +0200 Subject: [PATCH 09/10] remove some unused lines in httpclientadapter --- Octokit/Http/HttpClientAdapter.cs | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/Octokit/Http/HttpClientAdapter.cs b/Octokit/Http/HttpClientAdapter.cs index fa82d4bb03..5cd231b434 100644 --- a/Octokit/Http/HttpClientAdapter.cs +++ b/Octokit/Http/HttpClientAdapter.cs @@ -186,8 +186,6 @@ public async Task SendAsync(HttpRequestMessage request, Can else { unbuffered = true; - //throw new Exception("Cannot redirect a request with an unbuffered body"); - //savedContent = null; } } @@ -261,12 +259,6 @@ private static HttpRequestMessage CopyRequest(HttpRequestMessage oldRequest) } internal class RedirectHandler : DelegatingHandler - { - public bool Enabled { get; set; } - - protected override Task SendAsync(HttpRequestMessage request, CancellationToken cancellationToken) - { - return base.SendAsync(request, cancellationToken); - } + { } } \ No newline at end of file From 815f0d1461d1c7f0af67efcd1d6c7258b933e0d1 Mon Sep 17 00:00:00 2001 From: Ryan Gribble Date: Wed, 10 Aug 2016 23:40:25 +1000 Subject: [PATCH 10/10] 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 --- .../Clients/RepositoriesClientTests.cs | 2 +- Octokit.Tests.Integration/RedirectTests.cs | 2 +- Octokit.Tests/Http/RedirectHandlerTests.cs | 2 - Octokit/Http/HttpClientAdapter.cs | 82 ++++++++++--------- 4 files changed, 47 insertions(+), 41 deletions(-) diff --git a/Octokit.Tests.Integration/Clients/RepositoriesClientTests.cs b/Octokit.Tests.Integration/Clients/RepositoriesClientTests.cs index 14a87a46ef..bff337cdaa 100644 --- a/Octokit.Tests.Integration/Clients/RepositoriesClientTests.cs +++ b/Octokit.Tests.Integration/Clients/RepositoriesClientTests.cs @@ -618,7 +618,7 @@ public async Task ReturnsRenamedRepository() repository = await github.Repository.Get("fsprojects", "FSharp.Atom"); - Assert.Equal("https://github.com/ionide/ionide-fsharp.git", repository.CloneUrl); + Assert.Equal("https://github.com/ionide/ionide-atom-fsharp.git", repository.CloneUrl); Assert.False(repository.Private); Assert.False(repository.Fork); diff --git a/Octokit.Tests.Integration/RedirectTests.cs b/Octokit.Tests.Integration/RedirectTests.cs index b61416c2c0..7b4210b260 100644 --- 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 index 581fe48317..860f1cb01a 100644 --- a/Octokit.Tests/Http/RedirectHandlerTests.cs +++ b/Octokit.Tests/Http/RedirectHandlerTests.cs @@ -122,8 +122,6 @@ public async Task Status301ShouldRedirectPOSTWithBody(HttpStatusCode statusCode) Assert.Equal(response.RequestMessage.Method, httpRequestMessage.Method); Assert.NotSame(response.RequestMessage, httpRequestMessage); - //TODO: response.RequestMessage.Content has been disposed so cant access it here - //Assert.Equal("Hello World", await response.RequestMessage.Content.ReadAsStringAsync()); } // POST see other with content diff --git a/Octokit/Http/HttpClientAdapter.cs b/Octokit/Http/HttpClientAdapter.cs index 5cd231b434..d97092f6ec 100644 --- a/Octokit/Http/HttpClientAdapter.cs +++ b/Octokit/Http/HttpClientAdapter.cs @@ -173,26 +173,17 @@ protected virtual void Dispose(bool disposing) public async Task SendAsync(HttpRequestMessage request, CancellationToken cancellationToken) { - StreamContent savedContent = null; - bool unbuffered = false; - if (request.Content != null && request.Content.Headers.ContentLength != 0) - { - var stream = await request.Content.ReadAsStreamAsync().ConfigureAwait(false); - if (stream.CanSeek) - { - stream.Position = 0; - savedContent = new StreamContent(stream); - } - else - { - unbuffered = true; - } - } + // 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 to. 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; @@ -204,7 +195,6 @@ public async Task SendAsync(HttpRequestMessage request, Can { 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 @@ -213,42 +203,60 @@ public async Task SendAsync(HttpRequestMessage request, Can || 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 (unbuffered) - { - throw new Exception("Cannot redirect a request with an unbuffered body"); - } - newRequest.Content = savedContent; - } - 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 CloneHttpRequestMessageAsync(HttpRequestMessage oldRequest) { 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); } + foreach (var property in oldRequest.Properties) { newRequest.Properties.Add(property); @@ -259,6 +267,6 @@ private static HttpRequestMessage CopyRequest(HttpRequestMessage oldRequest) } internal class RedirectHandler : DelegatingHandler - { + { } } \ No newline at end of file