Feign: Allow for 3xx range headers

[clatko]

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]

The Client implementation (ex Client.Default) is responsible for addressing
the 3xx range. It is considered an error when the Client propagates a 300
as it means it has exhausted its means to address redirects for example.

What scenario are you encountering where this became an issue (are you
using a non-default client?)

Best first action is to move the clarification about 300 to javadoc on the
Client interface.

[clatko]

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]

Hi, Chris.

My intent was to explain why this slipped for 3 years. My suggestion to
document that was not an alternative to changing things, just that would be
something smart to do and I could safely cherry-pick on pretty much all
branches. I didn't mean to sound defensive if I did.

I agree it should be fixed. Wanna raise a pull request with a mockwebserver
test that sends a redirect to an https endpoint?

[adriancole]

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]

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 https://github.com/Netflix/feign/pull/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]

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]

Keep us posted!

[sbuettner]

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]

@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]

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

[adriancole]

@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]

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]

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]

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]

@adriancole Everything good. :smiley: 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 could be useful if you are for example only interested in the headers returned by the resource.

[adriancole]

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

[adriancole]

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.

[msparer]

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]

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

[dhgoulden]

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]

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]

@mguilherme did you try the ops workaround?

[mguilherme]

@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 :+1:

[nicky1]

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.

[juqkai]

@Configuration
public class FeignConfig implements RequestInterceptor {
    @Bean
    public Request.Options options(){
        return new Request.Options(10000, 60000, false);
    }
}