Improve Redirect Interceptor #388

kittinunf · 2018-07-16T08:45:34Z

What's in this PR?

The original PR by @ParkerK is here #208. However, due to lack of my attention over the time, it has been stale without me taking much care about it. 🙏 😢 😭

I think it might be best to revive it. And improve it a little along the way. With the suggestion from @SleeplessByte, I think we will have the right Redirect interceptor this time.

This PR should supersede #208. All credits go to him 🍺 🥂

codecov · 2018-07-16T09:17:41Z

Codecov Report

Merging #388 into master will increase coverage by 0.16%.
The diff coverage is 76.92%.

@@             Coverage Diff              @@
##             master     #388      +/-   ##
============================================
+ Coverage     75.92%   76.09%   +0.16%     
- Complexity      194      195       +1     
============================================
  Files            33       33              
  Lines           943      958      +15     
  Branches        166      174       +8     
============================================
+ Hits            716      729      +13     
- Misses          139      142       +3     
+ Partials         88       87       -1

Impacted Files	Coverage Δ	Complexity Δ
...n/kotlin/com/github/kittinunf/fuel/core/Request.kt	`78.41% <100%> (+0.47%)`	`60 <1> (+1)`	⬆️
.../kotlin/com/github/kittinunf/fuel/core/Response.kt	`68.88% <20%> (-6.12%)`	`20 <0> (ø)`
...f/fuel/core/interceptors/RedirectionInterceptor.kt	`82.6% <88.88%> (+20.1%)`	`0 <0> (ø)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1992bca...40e6d29. Read the comment docs.

markGilchrist

I am happy to approve if you feel these ideas for change are not an improvement

markGilchrist · 2018-07-17T06:20:57Z

fuel/src/main/kotlin/com/github/kittinunf/fuel/core/interceptors/RedirectionInterceptor.kt

+
+                val newMethod = when (response.statusCode) {
+                    HttpsURLConnection.HTTP_MOVED_PERM,
+                    HttpsURLConnection.HTTP_MOVED_TEMP,


would it be worth having these in a collection, so if you add them it doesn't get bigger and bigger, also the collection of valid redirects could be reused in other parts of the code base?

Agree! Will do!

markGilchrist · 2018-07-17T06:22:13Z

fuel/src/main/kotlin/com/github/kittinunf/fuel/core/interceptors/RedirectionInterceptor.kt

-                                }
-                        )
+
+                    if (redirectedUrl != null && redirectedUrl.isNotEmpty()) {


would you consider redirectedUrl?.isNotEmpty() = true

Aw right, will do.

markGilchrist · 2018-07-17T06:26:32Z

fuel/src/main/kotlin/com/github/kittinunf/fuel/core/interceptors/RedirectionInterceptor.kt

+                    }
+                }
+
+                if (response.statusCode in HttpsURLConnection.HTTP_MULT_CHOICE..HTTP_PERMANENT_REDIRECT) {


I think this range would be useful for reuse, would you consider defining it in HttpsUrlConnection?

But we don't have HttpsUrlConnection object here though. When you said defining it in HttpsUrlConnection? it means extenstion of HttpsUrlConnection ?

yes unless you feel that is an error?

Hmm, an extension HttpsUrlConnection needs an object of HttpsUrlConnection, doesn't it?

Let's say we have

val HttpsUrlConnection.redirectStatusRange get () = HttpsURLConnection.HTTP_MULT_CHOICE..HTTP_PERMANENT_REDIRECT

How do we use this?

sorry I see what you mean

kittinunf · 2018-07-17T08:31:00Z

@markGilchrist not at all, sir! I have fixed most of them but #388 (comment). Please advice 🥇

markGilchrist

I am happy to merge as my suggestion wasn't completely thought out

kittinunf · 2018-07-17T12:12:17Z

I wanna have @SleeplessByte to see this PR and hopefully say some few wise words 😄

SleeplessByte

Overall muchos better @kittinunf. Kudos on the work <3

SleeplessByte · 2018-07-17T17:56:40Z

fuel/src/main/kotlin/com/github/kittinunf/fuel/core/interceptors/RedirectionInterceptor.kt

+private val redirectStatusWithGets = listOf(HttpsURLConnection.HTTP_MOVED_PERM,
+        HttpsURLConnection.HTTP_MOVED_TEMP,
+        HttpsURLConnection.HTTP_SEE_OTHER)
+


For whomever is interested. This list is correct as per:

301 Moved Permanently

Note: For historical reasons, a user agent MAY change the request method from POST to GET for the subsequent request. If this behavior is undesired, the 307 (Temporary Redirect) status code can be used instead.

302 Found

Note: For historical reasons, a user agent MAY change the request method from POST to GET for the subsequent request. If this behavior is undesired, the 307 (Temporary Redirect) status code can be used instead.

303 See Other

A user agent can perform a retrieval request targeting that URI (a GET or HEAD request if using HTTP), which might also be redirected, and present the eventual result as an answer to the original request.

This list MUST NOT contain the following:

307 Temporary Redirect

Note: This status code is similar to 302 (Found), except that it does not allow changing the request method from POST to GET. This specification defines no equivalent counterpart for 301 (Moved Permanently) ([RFC7238], however, defines the status code 308 (Permanent Redirect) for this purpose).

308 Permanent Redirect

Note: This status code is similar to 301 (Moved Permanently) ([RFC7231], Section 6.4.2), except that it does not allow changing the request method from POST to GET.

this is the highest quality comment on a pr I have ever seen

SleeplessByte · 2018-07-17T18:03:32Z

fuel/src/main/kotlin/com/github/kittinunf/fuel/core/interceptors/RedirectionInterceptor.kt

+                    }
+                }
+
+                if (response.statusCode in HttpsURLConnection.HTTP_MULT_CHOICE..HTTP_PERMANENT_REDIRECT) {


You may consider simplyfying this by targeting everything in the 3xx class (and using 300..399), although this seems fine to me. However you probably want redirection to be optional, as the HTTP spec also defines it as optional! (note to unsafe requests).

https://tools.ietf.org/html/rfc7231#section-6.4

The 3xx (Redirection) class of status code indicates that further action needs to be taken by the user agent in order to fulfill the request. If a Location header field (Section 7.1.2) is provided, the user agent MAY automatically redirect its request to the URI referenced by the Location field value, even if the specific status code is not understood. Automatic redirection needs to done with care for methods not known to be safe, as defined in Section 4.2.1, since the user might not wish to redirect an unsafe request.

I would name the range .redirectionClass or .STATUS_REDIRECTION and if you do, maybe also define the .informationalClass / .STATUS_INFORMATIONAL, .successfulClass / .STATUS_SUCCESSFUL, .clientErrorClass / .STATUS_CLIENT_ERROR and .serverErrorClass / .STATUS_SERVER_ERROR.

There is no such thing as a redirect status, thus redirect status implies range.

Nice comment. @SleeplessByte Thanks so much for your insightful knowledge on this topic.

I do have one question when you said the redirection should be optional, what does the implementation should look like exactly? In what condition it is optional, in what condition the redirection should fall through?

Maybe if the location target is not there, we do nothing?

Regarding the status class, it is a great idea. I think I will try to do as you suggested. 👍

Almost all libraries have a simple flag "follow redirections" which is almost always opt-in. cURL uses -L I think, Android's HttpGet has setFollowRedirects(bool)

I think a correct way of handling is doing both

adding a flag

checking for the presence of the location URL (and the followability)

Now following redirects is optional.

This is fixed by b345f5b#diff-c6f14509edd9ece29c446e94eb74871bR33

SleeplessByte · 2018-07-17T18:14:15Z

fuel/src/main/kotlin/com/github/kittinunf/fuel/core/interceptors/RedirectionInterceptor.kt

+                                    URL(request.url, newUrl)
+                                }).toString())
+
+                        // redirect
                        next(request, manager.request(encoding).response().second)


What happens with all the other headers? If you only take the "default manager headers" that contract should be defined very explicitly. I think this is the wrong behaviour. For example Range-requests break when being redirected.

This is a good collection of comments around this subject. Note: it is NOT defined in any RFC yet, however, libcurl has a solid implementation that seems to make a lot of sense. It boils down to:

Copy all headers if you're redirecting to the same host

Copy all headers except for Authorization if you're redirecting to a different host.

I suggest implementing the same behaviour; either in this PR or a follow-up PR.

As per https://tools.ietf.org/html/rfc7231#section-6.4, you SHOULD implement a method to detect infinite loops. I suggest the arbitrary value of 10 hops as maximum, as 5 is deemed too little.

A client SHOULD detect and intervene in cyclical redirections (i.e., "infinite" redirection loops). Note: An earlier version of this specification recommended a maximum of five redirections ([RFC2068], Section 10.3). Content developers need to be aware that some clients might implement such a fixed limitation.

Currently, with this implementation, infinite loops would go on infinitely and slowely but surely deplete both your data plan as well as the battery.

SleeplessByte · 2018-07-17T18:20:28Z

fuel/src/main/kotlin/com/github/kittinunf/fuel/core/interceptors/RedirectionInterceptor.kt

-                        //error
-                        val error = FuelError(RedirectException(), response.data, response)
+                        // error
+                        val error = FuelError(


No. Redirection itself is always a MAY which means the behaviour is optional. Providing a Location is SHOULD and not MUST. It is not an error if there is no URL.

300 responses do not have a Location header, if there is no preferred response (see: Agent driven content-negotiation)

304 responses do not have defined that they SHOULD have a location.

Instead I would return the response with the 3xx status response and optionally the data. Not as an error.

Nice, will do.

SleeplessByte · 2018-07-17T18:23:03Z

fuel/src/test/kotlin/com/github/kittinunf/fuel/InterceptorTest.kt

+        assertThat(data, nullValue())
+        assertThat(data, not(containsString("http://httpbin.org/get")))
+        assertThat(error, notNullValue())
+    }
 }


Probably want to test that POST stays POST (307, 308) and POST becomes GET (301, 302)

Fixed by e90c13e

kittinunf · 2018-07-24T10:59:40Z

fuel/src/test/kotlin/com/github/kittinunf/fuel/InterceptorTest.kt

+        val (_, _, result) = manager.request(Method.GET,
+                "http://httpbin.org/redirect-to",
+                listOf("url" to "/get"))
+                .allowRedirect(false)


@SleeplessByte this is a flag to stop follow the redirection if the client wishes so. Do you think this makes sense?

Yah, anything like that works fine. Since it will follow up to max number of redirects, you might want to change it to plural. Can you set that at manager level as well?

Will work on the plural!

Can you set that at manager level as well?

I have thought about it and decided (by myself) that it is more flexible to do at the request level. Do you see any potential misuse or inconvenience?

People are lazy; but it's soooo trivial to pipe requests (using extension methods) through my own set to always set allowRedirect(false) or whatever, so I really couldn't care less. I think it's a fine design decision; as long as it's documented how to disallow redirects :)

kittinunf · 2018-07-24T11:00:35Z

fuel/src/test/kotlin/com/github/kittinunf/fuel/InterceptorTest.kt

+
+        val (data, error) = result
+
+        // TODO: This is current based on the current behavior, however we need to fix this as it should handle 100 - 399 gracefully not httpException


This should be fixed together with #379. Based on the current Fuel behavior, it is correct! However, it is wrong 😄.

I will be addressed in the different PR.

Sounds good!

kittinunf · 2018-07-24T11:03:59Z

There is one thing I haven't addressed which is the discussion in #388 (comment)

I have created this #395 so that we can comeback to this later.

kittinunf · 2018-07-26T00:19:10Z

I wanna get this thing rolling. Hopefully, @SleeplessByte is mildly satisfied with this. 🙃

Thanks so much for the reviews.

hudson155 · 2018-08-23T14:46:59Z

Does this work with FuelManager?

SleeplessByte · 2018-08-27T11:22:51Z

@hudson155 yes, these changes are in the core, so is used by anything. As long as the redirection interceptor is in your interceptors this will work.

markGilchrist reviewed Jul 17, 2018

View reviewed changes

markGilchrist approved these changes Jul 17, 2018

View reviewed changes

SleeplessByte requested changes Jul 17, 2018

View reviewed changes

kittinunf force-pushed the kv/improve-redirect-interceptor branch from 55f4045 to 0b510bc Compare July 24, 2018 04:03

kittinunf commented Jul 24, 2018

View reviewed changes

kittinunf added 14 commits July 25, 2018 03:42

Improve Redirect interceptor

f1b4434

Typos oops

ba0dc20

Logic error

4ab888e

Add more test cases

c7b0d22

Add test case support for others

e7ca4d1

Add test for redirect nourl

9ac9b54

Test on 302 without redirecturl

d1a31f8

Update with more tests

af43c83

Update according to the reviewer

ec02bed

Address reviewer concern, add test for 301,302,303

857f54b

Use status class instead

d5448e5

Do not throw error when the location is not found

345ef91

Add allowRedirect flag for redirection

ec5f30c

Add test to support Authorization header

446eddd

kittinunf force-pushed the kv/improve-redirect-interceptor branch from b5af7dc to 446eddd Compare July 24, 2018 18:42

SleeplessByte mentioned this pull request Jul 25, 2018

Automatic redirection on unsafe method POST with 302 response #375

Closed

kittinunf added 2 commits July 25, 2018 23:01

Use plural for this instead

04f0cf8

We need space before value

40e6d29

markGilchrist approved these changes Jul 25, 2018

View reviewed changes

kittinunf merged commit 7fe0e28 into master Jul 26, 2018

kittinunf mentioned this pull request Jul 26, 2018

Add HTTP status code 303 as valid redirect #208

Closed

kittinunf deleted the kv/improve-redirect-interceptor branch July 26, 2018 02:55

kittinunf mentioned this pull request Aug 1, 2018

Release v1.15.0 #403

Merged


		val (data, error) = result

		// TODO: This is current based on the current behavior, however we need to fix this as it should handle 100 - 399 gracefully not httpException

Improve Redirect Interceptor #388

Improve Redirect Interceptor #388

Conversation

kittinunf commented Jul 16, 2018 • edited Loading

What's in this PR?

codecov bot commented Jul 16, 2018 • edited Loading

Codecov Report

markGilchrist left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kittinunf commented Jul 17, 2018

markGilchrist left a comment

Choose a reason for hiding this comment

kittinunf commented Jul 17, 2018

SleeplessByte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SleeplessByte Jul 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SleeplessByte Jul 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kittinunf Jul 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kittinunf commented Jul 24, 2018 • edited Loading

kittinunf commented Jul 26, 2018 • edited Loading

hudson155 commented Aug 23, 2018

SleeplessByte commented Aug 27, 2018

kittinunf commented Jul 16, 2018 •

edited

Loading

codecov bot commented Jul 16, 2018 •

edited

Loading

SleeplessByte Jul 17, 2018 •

edited

Loading

SleeplessByte Jul 17, 2018 •

edited

Loading

kittinunf Jul 24, 2018 •

edited

Loading

kittinunf commented Jul 24, 2018 •

edited

Loading

kittinunf commented Jul 26, 2018 •

edited

Loading