From 5b9e23c2fb443517311181feabba4cf521136dbf Mon Sep 17 00:00:00 2001 From: Martin Scholz Date: Sun, 14 Aug 2016 22:57:50 +0200 Subject: [PATCH 1/2] 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 --- .../Clients/RepositoriesClientTests.cs | 25 ++++ Octokit.Tests.Integration/RedirectTests.cs | 2 +- Octokit.Tests/Http/RedirectHandlerTests.cs | 70 +++++------ Octokit/Http/HttpClientAdapter.cs | 111 ++++++++++-------- 4 files changed, 126 insertions(+), 82 deletions(-) diff --git a/Octokit.Tests.Integration/Clients/RepositoriesClientTests.cs b/Octokit.Tests.Integration/Clients/RepositoriesClientTests.cs index f6bc64a2..8ec921b4 100644 --- a/Octokit.Tests.Integration/Clients/RepositoriesClientTests.cs +++ b/Octokit.Tests.Integration/Clients/RepositoriesClientTests.cs @@ -605,6 +605,31 @@ public class RepositoriesClientTests 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 index b61416c2..7b4210b2 100644 --- a/Octokit.Tests.Integration/RedirectTests.cs +++ b/Octokit.Tests.Integration/RedirectTests.cs @@ -18,7 +18,7 @@ namespace Octokit.Tests.Integration 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 0d68b0ff..860f1cb0 100644 --- a/Octokit.Tests/Http/RedirectHandlerTests.cs +++ b/Octokit.Tests/Http/RedirectHandlerTests.cs @@ -14,10 +14,12 @@ namespace Octokit.Tests.Http [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 @@ namespace Octokit.Tests.Http 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 @@ namespace Octokit.Tests.Http 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 @@ namespace Octokit.Tests.Http 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 @@ namespace Octokit.Tests.Http 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 @@ namespace Octokit.Tests.Http 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 @@ namespace Octokit.Tests.Http 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 @@ namespace Octokit.Tests.Http 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 +170,9 @@ namespace Octokit.Tests.Http 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 16bb8868..d97092f6 100644 --- a/Octokit/Http/HttpClientAdapter.cs +++ b/Octokit/Http/HttpClientAdapter.cs @@ -20,6 +20,8 @@ namespace Octokit.Internal { 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 @@ namespace Octokit.Internal 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 @@ namespace Octokit.Internal 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) + public async Task 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); - // Can't redirect without somewhere to redirect too. Throw? - if (response.Headers.Location == null) return response; + // Send initial response + var response = await _http.SendAsync(request, HttpCompletionOption.ResponseContentRead, cancellationToken).ConfigureAwait(false); + + // 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 @@ namespace Octokit.Internal { 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 @@ namespace Octokit.Internal || 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 + + // 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) { - 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); - } + clonedRequest.Headers.Authorization = null; } - 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); + + // 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); + 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); + newRequest.Headers.TryAddWithoutValidation(header.Key, header.Value); } - return newrequest; + foreach (var property in oldRequest.Properties) + { + newRequest.Properties.Add(property); + } + + return newRequest; } } -} + + internal class RedirectHandler : DelegatingHandler + { + } +} \ No newline at end of file From 4072a6f5929eacdf0784d212c3c631026871cdf3 Mon Sep 17 00:00:00 2001 From: Steven Kirk Date: Tue, 16 Aug 2016 16:35:52 +0200 Subject: [PATCH 2/2] Fix issue #1446: Organization can fail deserialization when user is not an org owner.: (#1448) * Added failing unit test for #1446. * Make certain Account properties nullable. Make Collaborators, DiskUsage and PrivateGists properties nullable as they may be returned as null when fetching an organization of which the user is a member by not an owner. Fixes #1446. --- Octokit.Tests/Models/OrganizationTests.cs | 58 +++++++++++++++++++++++ Octokit.Tests/Octokit.Tests.csproj | 1 + Octokit/Models/Response/Account.cs | 6 +-- 3 files changed, 62 insertions(+), 3 deletions(-) create mode 100644 Octokit.Tests/Models/OrganizationTests.cs diff --git a/Octokit.Tests/Models/OrganizationTests.cs b/Octokit.Tests/Models/OrganizationTests.cs new file mode 100644 index 00000000..dbe53b4e --- /dev/null +++ b/Octokit.Tests/Models/OrganizationTests.cs @@ -0,0 +1,58 @@ +using System.Linq; +using Octokit; +using Octokit.Internal; +using Xunit; + +public class OrganizationTest +{ + [Fact] + public void CanBeDeserializedWithNullPrivateGistsDiskUsageAndCollaborators() + { + const string json = @"{ + ""login"": ""octocat"", + ""id"": 1234, + ""url"": ""https://api.github.com/orgs/octocat"", + ""repos_url"": ""https://api.github.com/orgs/octocat/repos"", + ""events_url"": ""https://api.github.com/orgs/octocat/events"", + ""hooks_url"": ""https://api.github.com/orgs/octocat/hooks"", + ""issues_url"": ""https://api.github.com/orgs/octocat/issues"", + ""members_url"": ""https://api.github.com/orgs/octocat/members{/member}"", + ""public_members_url"": ""https://api.github.com/orgs/octocat/public_members{/member}"", + ""avatar_url"": ""https://avatars.githubusercontent.com/u/1234?v=3"", + ""description"": ""Test org."", + ""name"": ""Octocat"", + ""company"": null, + ""blog"": ""http://octocat.abc"", + ""location"": """", + ""email"": """", + ""public_repos"": 13, + ""public_gists"": 0, + ""followers"": 0, + ""following"": 0, + ""html_url"": ""https://github.com/octocat"", + ""created_at"": ""2012-09-11T21:54:25Z"", + ""updated_at"": ""2016-08-02T05:44:12Z"", + ""type"": ""Organization"", + ""total_private_repos"": 1, + ""owned_private_repos"": 1, + ""private_gists"": null, + ""disk_usage"": null, + ""collaborators"": null, + ""billing_email"": null, + ""plan"": { + ""name"": ""organization"", + ""space"": 976562499, + ""private_repos"": 9999, + ""filled_seats"": 45, + ""seats"": 45 + } +}"; + var serializer = new SimpleJsonSerializer(); + + var org = serializer.Deserialize(json); + + Assert.Equal("octocat", org.Login); + Assert.Equal(1234, org.Id); + } +} + diff --git a/Octokit.Tests/Octokit.Tests.csproj b/Octokit.Tests/Octokit.Tests.csproj index 7523d237..1554853b 100644 --- a/Octokit.Tests/Octokit.Tests.csproj +++ b/Octokit.Tests/Octokit.Tests.csproj @@ -184,6 +184,7 @@ + diff --git a/Octokit/Models/Response/Account.cs b/Octokit/Models/Response/Account.cs index 142eb334..95384119 100644 --- a/Octokit/Models/Response/Account.cs +++ b/Octokit/Models/Response/Account.cs @@ -55,7 +55,7 @@ namespace Octokit /// /// Number of collaborators the account has. /// - public int Collaborators { get; protected set; } + public int? Collaborators { get; protected set; } /// /// Company the account works for. @@ -70,7 +70,7 @@ namespace Octokit /// /// Amount of disk space the account is using. /// - public int DiskUsage { get; protected set; } + public int? DiskUsage { get; protected set; } /// /// The account's email. @@ -138,7 +138,7 @@ namespace Octokit /// /// Number of private gists the account has created. /// - public int PrivateGists { get; protected set; } + public int? PrivateGists { get; protected set; } /// /// Number of public gists the account has created.