Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix redirect timeout on renamed repositories #1411

Merged
merged 20 commits into from
Aug 14, 2016
Merged

Fix redirect timeout on renamed repositories #1411

Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
dcf1cf0
Merge remote-tracking branch 'refs/remotes/octokit/master'
martinscholz83 Jun 28, 2016
bce9696
add test
martinscholz83 Jun 28, 2016
61dfff8
Merge remote-tracking branch 'refs/remotes/octokit/master'
martinscholz83 Jun 29, 2016
dd38e4c
Merge remote-tracking branch 'refs/remotes/octokit/master' into fix-t…
martinscholz83 Jun 29, 2016
dc65dab
Merge remote-tracking branch 'refs/remotes/octokit/master'
martinscholz83 Jun 30, 2016
f6191d2
Merge branch 'master' of https://github.com/maddin2016/octokit.net
martinscholz83 Jun 30, 2016
7f8cfba
Merge remote-tracking branch 'refs/remotes/octokit/master' into fix-t…
martinscholz83 Jul 4, 2016
84f0e7b
[WIP]
martinscholz83 Jul 4, 2016
b4d0ea9
put logic for redirects outside of delegating handler
martinscholz83 Jul 5, 2016
d2748c3
change send method
martinscholz83 Jul 5, 2016
f3efff1
format code
martinscholz83 Jul 5, 2016
b42240b
reorganized http client adapter
martinscholz83 Jul 5, 2016
c85508e
Merge remote-tracking branch 'refs/remotes/octokit/master' into fix-t…
martinscholz83 Jul 18, 2016
e6309cb
change HttpClientAdapter
martinscholz83 Jul 18, 2016
54225c9
rework http redirect tests - still an issue with accessing the respon…
ryangribble Jul 16, 2016
efe587b
Merge remote-tracking branch 'refs/remotes/octokit/master' into fix-t…
martinscholz83 Jul 23, 2016
2f4e03e
Merge branch 'fix-timeout-getting-multiple-repositories' of https://g…
martinscholz83 Jul 23, 2016
ff58f05
Merge remote-tracking branch 'refs/remotes/octokit/master' into fix-t…
martinscholz83 Jul 29, 2016
802091c
remove some unused lines in httpclientadapter
martinscholz83 Aug 10, 2016
815f0d1
Reworked redirect implementation to fully clone http request and re-u…
ryangribble Aug 10, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions Octokit.Tests.Integration/Clients/RepositoriesClientTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
{
Expand Down
72 changes: 38 additions & 34 deletions Octokit.Tests/Http/RedirectHandlerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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
Expand All @@ -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);
Expand All @@ -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<InvalidOperationException>(
() => invoker.SendAsync(httpRequestMessage, new CancellationToken()));
() => adapter.SendAsync(httpRequestMessage, new CancellationToken()));
}

static HttpRequestMessage CreateRequest(HttpMethod method)
Expand All @@ -163,14 +172,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);
}
}

Expand Down
71 changes: 44 additions & 27 deletions Octokit/Http/HttpClientAdapter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand All @@ -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);
}
}
Expand Down Expand Up @@ -168,18 +170,30 @@ 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);
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");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💄 let's 🔥 these commented lines

//savedContent = null;
}
}

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
Expand Down Expand Up @@ -210,25 +224,18 @@ protected override async Task<HttpResponseMessage> SendAsync(HttpRequestMessage
}
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)
{
newRequest.Headers.Authorization = null;
}

response = await SendAsync(newRequest, cancellationToken).ConfigureAwait(false);
}

Expand All @@ -238,18 +245,28 @@ protected override async Task<HttpResponseMessage> 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; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see this value being used anywhere (I suspect it also wasn't used before).


protected override Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
{
return base.SendAsync(request, cancellationToken);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This override also doesn't seem necessary now.

}
}
}
}