support redirect requests natively #808

shiftkey · 2015-05-22T07:48:12Z

Fixes #798
Fixes #748
Fixes #812

Building on the awesome start that @darrelmiller started over in darrelmiller@e5d7d6e

cry forever, then get up and ponder on what to do with all the suppressed warnings about incorrect disposal
write integration tests for redirecting repositories:
- https://github.com/robconery/massive -> https://github.com/fbouma/massive
write some tests around creating data and handling the 307
- setup a public repository to test this
RepositoryContentsClient.GetArchiveLink depends on not following the redirect, prolly need to address this
documentation
set the timeout for downloading the archive above the default

@jasonrudolph anything I've missed here?

darrelmiller · 2015-05-22T13:34:36Z

I just noticed the RedirectHandler CopyResult uses a .Result. That's because I pulled the code from a .net 4.0 project. It should probably be fixed to use async/await. I'd do it but I'm not sure how to add a commit to a PR.

jasonrudolph · 2015-05-22T15:31:35Z

Thanks for digging into this, @shiftkey. ⚡

@jasonrudolph anything I've missed here?

My C# leaves much to be desired, but @kdaigle and I sketched out this plaintext spec that might help answer your question:

MUST follow redirect requests if HTTP status code is 301, 302, or 307. (These are the redirect codes actively used by the GitHub API.)

MUST redirect a 30x status code if the HTTP method is HEAD, OPTIONS, GET, POST, PUT, PATCH, or DELETE.

MUST use the original request's HTTP method when following a redirect where the HTTP status code is 307.

SHOULD use the original request's HTTP method when following a redirect where the HTTP status code is 301 or 302.

MUST use the original request's authentication credentials when following a redirect where the original host matches the redirect host.

MUST NOT use the original request's authentication credentials when following a redirect where the original host does not match the redirect host.

SHOULD only follow 3 redirects.

Does that help?

darrelmiller · 2015-05-22T16:41:33Z

I'm not doing redirect counting to limit redirects. I can fix that.
I'm not checking host before copying auth header. I can fix that.
Currently I change the method on a 301 or 302 because it is allowed. Changing the behaviour to not change the method is easy enough.

darrelmiller · 2015-05-22T22:23:54Z

I have done all the fixes to the RedirectHandler and added a bunch of tests for it. However, I have absolutely no @!#$@#$ clue how to push the changes up to anywhere on Github.
I pulled down the pr/808 branch and made the changes there, but am unable to push that branch with the fixes to my fork.

shiftkey · 2015-05-23T02:53:27Z

@darrelmiller if you're working on your fork and it's in a branch I know about, I can pull in your changes into this PR...

darrelmiller · 2015-05-23T16:24:13Z

@shiftkey I don't really know how. But I succeeded in pushing my changes to my fork. darrelmiller@cf9c845

darrelmiller · 2015-05-23T16:34:25Z

Couple of comments:

The 301 and 302 behaviour is now different to what HttpWebRequest does by default. But the new behaviour is what was originally intended for 301/302. (Possibly the most useful diagram I have ever seen on this subject is here https://tools.ietf.org/html/rfc7238#section-1)

Keeping the auth header around based on host is consistent with what the .Net CredentialCache does. However, based on my interpretation of the HTTP specs, it should really be based on the realm. Which would require a 401 dance to find out the new realm. I would file this under "nice idea in theory but no-one does it". The pedant in me felt the need to mention it.

jasonrudolph · 2015-05-26T13:22:16Z

The 301 and 302 behaviour is now different to what HttpWebRequest does by default. But the new behaviour is what was originally intended for 301/302. (Possibly the most useful diagram I have ever seen on this subject is here https://tools.ietf.org/html/rfc7238#section-1)

@darrelmiller: Thanks for linking to that diagram. Wonderfully succinct and useful. ⚡

shiftkey · 2015-05-31T08:59:06Z

@darrelmiller your latest changes are applied onto this PR - thanks for that 💖.

i'll look at those failing tests, and putting together some additional tests for the GitHub API.

…don't exist

a less awful class

shiftkey · 2015-05-31T10:10:00Z

Octokit/Http/HttpClientAdapter.cs

@@ -109,6 +103,8 @@ protected virtual HttpRequestMessage BuildRequestMessage(IRequest request)
            {
                var fullUri = new Uri(request.BaseAddress, request.Endpoint);
                requestMessage = new HttpRequestMessage(request.Method, fullUri);
+
+                requestMessage.Properties["AllowAutoRedirect"] = request.AllowAutoRedirect;


@darrelmiller I'm almost inclined to obsolete request.AllowAutoRedirect and simplify things - as we need to control this behaviour in our very specific way. Any objections?

It felt like a nice feature to have, but I'm struggling to think of a scenario where I would actually need it.

It was one of the first things I'd removed when refactoring earlier attempts. Good enough for me 🔥🔥🔥🔥🔥

shiftkey · 2015-05-31T11:31:17Z

Just added a passing test against the new redirects - I'll add some more later this week, but this is feeling rather close...

darrelmiller · 2015-05-31T12:39:53Z

👍

shiftkey · 2015-06-05T06:46:50Z

Added in a test for issue creation and finding a repository. Ready for review.

shiftkey · 2015-06-11T10:51:57Z

Just added one more little task before I'm happy with this.

Conflicts: Octokit/Octokit-Mono.csproj Octokit/Octokit-MonoAndroid.csproj Octokit/Octokit-Monotouch.csproj Octokit/Octokit-Portable.csproj Octokit/Octokit-netcore45.csproj

shiftkey · 2015-06-15T08:05:32Z

Extracted #817 rather than drag this out any longer.

Ready for review 🐶

shiftkey · 2015-06-27T12:46:45Z

@haacked would love some 👀 on this before I merge it and continue down the path of extracting more HTTP-friendly abstractions...

darrelmiller · 2015-06-28T19:44:22Z

Just one observation that would probably have been much more useful before we started all this.

If you are actually using the CredentialCache with credentials associated to the correct origin server, then when you redirect to a URL with the same host then the credentials are carried over. Using a credential cache avoids setting creds in a default header and takes care of applying the correct credentials on every request.

Maybe it shouldn't be the responsibility of redirection code to be carrying over the authorization header.

shiftkey · 2015-06-28T23:48:07Z

If you are actually using the CredentialCache with credentials associated to the correct origin server, then when you redirect to a URL with the same host then the credentials are carried over.

Yep, I'd like to address this at some point - we should mimic this behaviour in our abstraction and move it into HttpClient.

darrelmiller · 2015-06-29T01:03:45Z

Funny you should mention that, but I just finished writing an AuthenticationService/HttpCredentialCache that plugs into the message handler pipeline. It's not on Github yet though.

shiftkey · 2015-07-14T17:45:10Z

@haacked bump for code review plz

Not worth commenting on in a code review, so I fixed it myself.

haacked · 2015-07-17T17:21:33Z

Octokit/Http/HttpClientAdapter.cs

+            var redirectCount = 0;
+            if (request.Properties.Keys.Contains(RedirectCountKey))
+            {
+                redirectCount = (int)request.Properties[RedirectCountKey];


Couldn't this fail if there's no value here? Or do we always seed this with a value?

I think this would only fail if someone pre-filled the property with a value that cannot be cast to an int. A value is assigned to the property when it is initially created, right after this If ends.

haacked · 2015-07-17T17:24:57Z

Looks good to me. Just one question.

support redirect requests natively

jasonrudolph · 2015-07-17T18:06:57Z

@darrelmiller @shiftkey: 👏 🌟

shiftkey · 2015-07-17T20:51:34Z

I'll run through the integration tests and see if I can put together a 🚢 on Monday (my time).

darrelmiller and others added 5 commits May 22, 2015 16:33

First attempt to reuse HttpClient

14fe8bc

massaging and fighting with fxcop

7f95f55

suppressing a bunch of stuff because murtaugh's law

4d77f0e

missed an overload

f0011cc

missed a supresssion

894609e

shiftkey mentioned this pull request May 22, 2015

[RFC] HttpClientAdapter and Octokit - Future Plans #781

Closed

test hacks for great good

eb7e14b

Fixed issues with RedirectHandler and added tests

ea9f3a4

shiftkey added 5 commits May 31, 2015 18:58

get the portable tests back running by muting all these methods that …

5d0c8d6

…don't exist

extracting all the cross-platform setup of HttpMessageHandler into

e5e4b4c

a less awful class

a bit more cleanup

549ff92

internalize this for now

d551eb2

💄

3c32814

shiftkey reviewed May 31, 2015
View reviewed changes

shiftkey mentioned this pull request May 31, 2015

specifying a proxy when connecting to a GitHub Enterprise server #809

Closed

shiftkey added 3 commits May 31, 2015 20:12

wrote some docs

be57a70

renaming things is hard

d7d7efd

review and cleanup suppressions

74ac314

the test, it passes

3859ff3

throw an error if the redirect count has been exceeded

442bbb4

oops, corrected the test

eafd63d

darrelmiller mentioned this pull request Jun 9, 2015

Using "Personal Access Tokens" #812

Closed

specify the timeout interval when downloading the archive

982dca5

shiftkey mentioned this pull request Jun 15, 2015

control request timeout when downloading archive #817

Closed

Merge branch 'master' into redirect-requests

0acdb8c

Conflicts: Octokit/Octokit-Mono.csproj Octokit/Octokit-MonoAndroid.csproj Octokit/Octokit-Monotouch.csproj Octokit/Octokit-Portable.csproj Octokit/Octokit-netcore45.csproj

shiftkey mentioned this pull request Jun 15, 2015

added a tailored CodeFormatter to Octokit #807

Merged

6 tasks

This was referenced Jun 16, 2015

Get Archive Link for Repository Contents #748

Closed

Support Etags #352

Open

shiftkey mentioned this pull request Jul 3, 2015

deadlock in ASP.NET MVC #831

Closed

🎨 Really anal fix-ups of extra newlines

5be1cae

Not worth commenting on in a code review, so I fixed it myself.

haacked reviewed Jul 17, 2015
View reviewed changes

🎨 Minor formatting changes

8e77f00

haacked added a commit that referenced this pull request Jul 17, 2015

Merge pull request #808 from octokit/redirect-requests

795aff1

support redirect requests natively

haacked merged commit 795aff1 into master Jul 17, 2015

haacked deleted the redirect-requests branch July 17, 2015 17:46

shiftkey mentioned this pull request Oct 5, 2015

Update octokit to 0.16.0 github/VisualStudio#101

Merged

shiftkey mentioned this pull request Dec 8, 2015

removed obsolete member AllowAutoRedirect #980

Merged

shiftkey mentioned this pull request Mar 14, 2016

[META] Questions about HTTP Extensibility in Octokit #1116

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support redirect requests natively #808

support redirect requests natively #808

shiftkey commented May 22, 2015

darrelmiller commented May 22, 2015

jasonrudolph commented May 22, 2015

darrelmiller commented May 22, 2015

darrelmiller commented May 22, 2015

shiftkey commented May 23, 2015

darrelmiller commented May 23, 2015

darrelmiller commented May 23, 2015

jasonrudolph commented May 26, 2015

shiftkey commented May 31, 2015

shiftkey May 31, 2015

darrelmiller May 31, 2015

shiftkey May 31, 2015

shiftkey commented May 31, 2015

darrelmiller commented May 31, 2015

shiftkey commented Jun 5, 2015

shiftkey commented Jun 11, 2015

shiftkey commented Jun 15, 2015

shiftkey commented Jun 27, 2015

darrelmiller commented Jun 28, 2015

shiftkey commented Jun 28, 2015

darrelmiller commented Jun 29, 2015

shiftkey commented Jul 14, 2015

haacked Jul 17, 2015

darrelmiller Jul 17, 2015

haacked commented Jul 17, 2015

jasonrudolph commented Jul 17, 2015

shiftkey commented Jul 17, 2015

support redirect requests natively #808

support redirect requests natively #808

Conversation

shiftkey commented May 22, 2015

darrelmiller commented May 22, 2015

jasonrudolph commented May 22, 2015

darrelmiller commented May 22, 2015

darrelmiller commented May 22, 2015

shiftkey commented May 23, 2015

darrelmiller commented May 23, 2015

darrelmiller commented May 23, 2015

jasonrudolph commented May 26, 2015

shiftkey commented May 31, 2015

shiftkey May 31, 2015

Choose a reason for hiding this comment

darrelmiller May 31, 2015

Choose a reason for hiding this comment

shiftkey May 31, 2015

Choose a reason for hiding this comment

shiftkey commented May 31, 2015

darrelmiller commented May 31, 2015

shiftkey commented Jun 5, 2015

shiftkey commented Jun 11, 2015

shiftkey commented Jun 15, 2015

shiftkey commented Jun 27, 2015

darrelmiller commented Jun 28, 2015

shiftkey commented Jun 28, 2015

darrelmiller commented Jun 29, 2015

shiftkey commented Jul 14, 2015

haacked Jul 17, 2015

Choose a reason for hiding this comment

darrelmiller Jul 17, 2015

Choose a reason for hiding this comment

haacked commented Jul 17, 2015

jasonrudolph commented Jul 17, 2015

shiftkey commented Jul 17, 2015