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.

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

Allow for 3xx range headers #249

Open
clatko opened this issue Jul 12, 2015 · 25 comments
Open

Allow for 3xx range headers #249

clatko opened this issue Jul 12, 2015 · 25 comments

Comments

@clatko
Copy link

@clatko clatko commented Jul 12, 2015

Everything above 299 is considered an error, which is plainly incorrect.

To get around this I had to:

  1. add an error decoder
  2. throw a special exception for 3xx range
  3. when catching runtime errors, check if it is the special 300 error
  4. convert to a valid response entity

Will investigate replacing this line:

if (response.status() >= 200 && response.status() < 300) {
in SynchronousMethodHandler and PR'ing it. I just wonder why nobody else has come across this issue.

Should I fix it?

@adriancole

This comment has been minimized.

Copy link
Member

@adriancole adriancole commented Jul 12, 2015

@clatko

This comment has been minimized.

Copy link
Author

@clatko clatko commented Jul 12, 2015

I am not using a non-default client. The redirect doesn't work because it crosses from http to https and java refuses to allow that.

This is a good thing.

I don't agree that if the client can't/doesn't handle a 300 it is an error. The proper response from the microservice is a 302 and I want feign to propagate this so the front end can do the redirect. With the default client using setInstanceFollowRedirects(true) you are assuming the 30x is handled and the only way to handle this is to get a 200 range response.

So to get the results I want, I have to either:

a) send a 200 back to feign from the microservice, intercept it and make make it a 302.
b) create my own client that uses setInstanceFollowRedirects(false), then change the status code to something feign can swallow and break a bunch of RFCs in the process.

c) Modify this section of the code to something reasonable:
if (response.status() >= 200 && response.status() < 300) {

@adriancole

This comment has been minimized.

Copy link
Member

@adriancole adriancole commented Jul 12, 2015

@adriancole

This comment has been minimized.

Copy link
Member

@adriancole adriancole commented Jul 13, 2015

ps another reason why this probably went unnoticed might be the scenario you described as going from http -> https. Since feign is usually used for apis, I suspect most have their Target as https in the first place.

@adriancole

This comment has been minimized.

Copy link
Member

@adriancole adriancole commented Jul 13, 2015

Sorry.. I was on the plane when trying to comment to most of this, just catching up in detail now.

It looks like you want to "convert [a 3xx error] to a valid response entity" using the a decoder? If that's the fix you are looking for, I would hesitate before spending more time on this, particularly for reasons discussed here #238.

For now, adding a Client wrapper that changes 302 to 200 is probably the least code workaround.

At any rate, I might be wrong in guessing your fix is to allow the decoder to be invoked in the redirect range. Mind sketching what you were planning to do?

@clatko

This comment has been minimized.

Copy link
Author

@clatko clatko commented Jul 13, 2015

I didn't mean to sound rude, just under the gun here. feign has been extremely helpful for our project and I thank you for making it available to the public. I already wrote the ErrorDecoder workaround, but it is such horrible code, it will live in my stash forever. (I throw a 300 range error, catch it in the controller and convert it to a non error).

The solution for now is just a one liner to change the 302 to a 200 at the microservice level. Works for now so I must move on. I will come back next week to this and will look at other potential solution or a PR on allowing for the 300 range.

@adriancole

This comment has been minimized.

Copy link
Member

@adriancole adriancole commented Jul 13, 2015

@sbuettner

This comment has been minimized.

Copy link

@sbuettner sbuettner commented Sep 9, 2015

Same problem here, but we have to deal with a 3rd-Party service where we cant change the return code. Any chances that the status handling will be changed?

@adriancole

This comment has been minimized.

Copy link
Member

@adriancole adriancole commented Sep 9, 2015

@sbuettner which part of same problem? :) are you having a redirect from http->https? Can you put a failing test or an example of request/response so we can make the right fix?

@sbuettner

This comment has been minimized.

Copy link

@sbuettner sbuettner commented Sep 9, 2015

That http status codes >= 300 are handled as an error.

@adriancole

This comment has been minimized.

Copy link
Member

@adriancole adriancole commented Sep 9, 2015

@sbuettner sorry I really need you to be specific. Is your goal to decode a 3xx response? Why is it the case that when redirects are exhausted the result is not an error?

@adriancole

This comment has been minimized.

Copy link
Member

@adriancole adriancole commented Sep 9, 2015

IOTW, "http status codes >= 300 are handled as an error" isn't clarifying what your goal is, it is just describing the roadblock.

Here's are some examples, with some precanned answers.. I'm guessing the last is more to your point?

  • My server returns a 302 when it succeeds. So, for example, I need to decode the body into a valid response.
    • Make a client wrapper that changes 302 to 200 until the server is fixed.
  • My server returns a 302, which I can do nothing about. I can pre-can a fallback value, but I have no way to do that without catching exceptions.
    • This probably falls in either the @Fallback bucket, or hacking things to specify the error range to <300.
  • I set my target to https, but my server returns links to http urls, which later redirect back to https. The latter isn't something the client is handling.
    • Maybe change the decoder that parses plain text links to decode them as https.
  • I believe 3xx range is something that a decoder should just do like it does 2xx.
    • We already have ErrorDecoder which is invoked for all of the non-success ranges. We also have RetryableException and Retryer that relate to error decoding (which may be used for 3xx range). Decoder was designed and documented only for the 2xx range, and really we should be removing codes (like 205, 204), not adding them. Do tests pass when you change SynchronousMethodHandler to treat 3xx as success? Should we now merge ErrorDecoder into Decoder? Or .. move docs about RetryableException to the normal decoder? Or.. should we just introduce RedirectHandler since this issue seems far more about customizing redirects than pretending they are success.

I'm not trying to be difficult or draconian, I just don't want us to compromise the experience for something that seems very special-case, and certainly is being addressed as exception handling at the moment. If we can make an opt-in change, we don't have to invalidate everyone's code that thinks 3xx is success.

If we had another way to address redirects, wouldn't we want to take that rather than invalidating the (success) Decoder concept? If not, we need to at least figure out how to communicate this as it will break people.

@adriancole

This comment has been minimized.

Copy link
Member

@adriancole adriancole commented Sep 9, 2015

ps in case it isn't clear, I'm in favor of adding RedirectHandler which could have two impls, a Decoder impl, which treats 3xx range as success, and ErrorDecoder impl, which is the current behavior.

@adriancole

This comment has been minimized.

Copy link
Member

@adriancole adriancole commented Sep 9, 2015

Well to be clear, I just don't want to widen the interface of decoder, as I don't think we should place redirect range responsibility on jackson, gson, etc. Any solution that avoids that would be my preference.

@sbuettner

This comment has been minimized.

Copy link

@sbuettner sbuettner commented Sep 9, 2015

@adriancole Everything good. 😃 This topic should be handled very carefully.

If the client returns a 302 since it cant open the url (http->https) or does not follow redirects it would be nice if one could use the Response type to handle it manually instead of throwing an exception.

@sbuettner

This comment has been minimized.

Copy link

@sbuettner sbuettner commented Sep 9, 2015

This could be useful if you are for example only interested in the headers returned by the resource.

@adriancole

This comment has been minimized.

Copy link
Member

@adriancole adriancole commented Sep 9, 2015

We should be returning Response unconditionally. I agree with that being a bug.

@adriancole

This comment has been minimized.

Copy link
Member

@adriancole adriancole commented Sep 9, 2015

ps. wow.. embarrassingly we have no redirect tests here or even in denominator. Changing the following line in SynchronousMethodHandler passes all tests

-      if (response.status() >= 200 && response.status() < 300) {
+      if (response.status() >= 200 && response.status() < 400) {

If we went with the above change, here's what the impact would be.

  • Change ErrorDecoder to say response has a code of >=400.
  • Change Decoder from talking about 2xx range to talking about 2xx - 3xx range.
    • Note that 3xx range responses..
      • without a body will quietly turn to null.
      • with text that doesn't match the type will throw an codec-specific exception as opposed to an exception of the form status ${response.status} reading ${methodKey}\n${body}.

To make impact more obvious, I'll backfill tests with things I've run into. For example, it is very common for redirects to have a form that's different than the requested resource. Also, 304 responses make little sense to silently decode as null.

adriancole added a commit that referenced this issue Sep 9, 2015
In unsuccessful scenarios, such as redirects, error handling converts a
`Response` into an exception. When a user defines a return type as
`Response`, we can assume they will want access to things like headers
even in an error scenario.

See #249
adriancole added a commit that referenced this issue Sep 9, 2015
In unsuccessful scenarios, such as redirects, error handling converts a
`Response` into an exception. When a user defines a return type as
`Response`, we can assume they will want access to things like headers
even in an error scenario.

See #249
@msparer

This comment has been minimized.

Copy link

@msparer msparer commented Feb 24, 2016

As far as I'm concerned, it would be enough to be able to configure the HttpUrlConnection of the default client Client.Default to setInstanceFollowRedirects to false. I think this is also necessary to comply with RFC 2616. For response code 302 it says

If the 302 status code is received in response to a request other
than GET or HEAD, the user agent MUST NOT automatically redirect the
request unless it can be confirmed by the user, since this might
change the conditions under which the request was issued.

I think it is false to assume that all users want to confirm an automatic redirect. E.g. I have a case where I POST to a service, get a 302, read the Location header and then PUT to this location. Posting to the redirect location is not allowed. There is currently no way to change the http method from POST to PUT using status codes (you can "force" a GET by returning 303 or force to use the same method again with a 307, but you can't change the method), I therefore need to get the response of the redirect to be able to initiate a new request using PUT.

I'm now getting around the problem by copying Client.Default and changing setInstanceFollowRedirects (true) to false (overriding methods isn't possible without hitting package restrictions).

The other hackish way would be to set a custom header in the response and let the Location header blank. This way HttpUrlConnection doesn't try a redirect and the 3xx response code can be handled by a custom ErrorHandler, which then could read the custom header and perform the redirect based on its content.

@adriancole

This comment has been minimized.

Copy link
Member

@adriancole adriancole commented Feb 25, 2016

@dhgoulden

This comment has been minimized.

Copy link

@dhgoulden dhgoulden commented Dec 21, 2016

I'd like to add my vote for better way to handle redirects. In my case, I'm working on a 'gateway' service that calls another service that requires the client of the gateway to login. The redirect to the login page should be passed back to the original client.

@mguilherme

This comment has been minimized.

Copy link

@mguilherme mguilherme commented Feb 13, 2017

I'm having a similar issue (it seems so), I'm trying to use PUT in an API that I don't control and from what I can see in postman (using inspect) it returns a 301 with a new GET Location and postman follows that redirect.
Using feign since it's a 301, an Exception is thrown and nothing is updated.
Is there a clean way of solving this?
If not can you point me in the right direction?

I'm using spring-cloud-starter-feign with spring boot

@spencergibb

This comment has been minimized.

Copy link
Contributor

@spencergibb spencergibb commented Feb 14, 2017

@mguilherme did you try the ops workaround?

@mguilherme

This comment has been minimized.

Copy link

@mguilherme mguilherme commented Feb 14, 2017

@spencergibb, thanks for your answer I was able to put it to work following the same concept of LaxRedirectStrategy but adding my own with PATCH using RestTemplate, I was about to do something similar with Feign but they released a new API version so I can use pure Feign with HttpClient for PATCH.
Everything is working now 👍

@nicky1

This comment has been minimized.

Copy link

@nicky1 nicky1 commented Oct 10, 2018

Another way would be to use OkHttp (as it is far easier to configure than HttpUrlConnection).

yes,i use okhttp instead default client httpurlconnection,but use okhttp client, the server protocol is http1.1, not http2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.