Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Allow for customization of statusCode interpretation#238

Closed
nblair wants to merge 1 commit intoOpenFeign:8.xOpenFeign/feign:8.xfrom
nblair:custom-status-handlingnblair/feign:custom-status-handlingCopy head branch name to clipboard
Closed

Allow for customization of statusCode interpretation#238
nblair wants to merge 1 commit intoOpenFeign:8.xOpenFeign/feign:8.xfrom
nblair:custom-status-handlingnblair/feign:custom-status-handlingCopy head branch name to clipboard

Conversation

@nblair
Copy link

@nblair nblair commented Jun 3, 2015

We just started looking at Feign for implementing client libraries for some REST APIs.

We wanted to setup an interface like so:

public interface FooAPI {

  /**
   * Retrieve the {@link Foo} that has a matching value for {@link Foo#getId()}.
   * 
   * @param id the id of the {@link Foo}
   * @return the corresponding {@link Foo}, or null if not found
   */
 @RequestLine("GET /api/v1/foo/{id}")
 Foo retrieveById(@Param("id") String id);

The REST API's contract states that /api/v1/foo/{id} will return a 404 if no resource exists with that id.

In the current 8.3 release of Feign, a return code of 404 is treated as something that should raise an Exception. Our method signature would have to look like this instead:

public interface FooAPI {

  /**
   * Retrieve the {@link Foo} that has a matching value for {@link Foo#getId()}.
   * 
   * @param id the id of the {@link Foo}
   * @return the corresponding {@link Foo}
   * @throws FooNotFoundException if no {@link Foo} exists with the id
   */
 @RequestLine("GET /api/v1/foo/{id}")
 Foo retrieveById(@Param("id") String id) throws FooNotFoundException;

We'd prefer not to have to create an exception for this scenario.
This pull request adds a new interface encapsulating logic currently at line 106 of:

https://github.com/Netflix/feign/blob/8.x/core/src/main/java/feign/SynchronousMethodHandler.java

Previously, inline
logic in SynchronousMethodHandler only treated 200 class status codes as
something to parse; everything else, including 404, as reason to throw exception (ErrorDecoder then takes over).

This change allows callers to inject StatusInterpreter.Include404 if they desire a contract that can return null for 404 like the one presented at the top rather than raising an Exception. The interface allows for further customization if desired.

The default behavior is unchanged.

Adds new interface encapsulating logic of determining whether or not a
response should be parsed or raise an exception. Previously, inline
logic in SynchronousMethodHandler only treated 200 class status codes as
something to parse; everything else, including 404, as exceptable.
Callers can inject StatusInterpreter.Include404 if they desire 404
status codes to result in Feign returning null for their methods, rather
than raising an Exception.
@cloudbees-pull-request-builder

NetflixOSS » feign » feign-pull-requests #112 SUCCESS
This pull request looks good

@codefromthecrypt
Copy link
Contributor

codefromthecrypt commented Jun 3, 2015 via email

@codefromthecrypt
Copy link
Contributor

So, this is definitely in the area of static-simple fallbacks, somewhat related to the hystrix discussion.

When designing feign, originally, the error decoder had fallback handling as a part of it. This was inspired by prior art in jclouds, intended to be a bit simpler.

When doing an implementation with an Async pattern (Observer), we realized that this fallback handling was something invocation-specific, so in order to reuse the ErrorDecoder, we had to decouple it from fallbacks (46be6bd)

If we were to resurrect fallback handling, I'd prefer to not conflate it with the decoder. For example, if fallback processing occurs with context of the request, it could be statically mapped (ex. 404 -> default value or null), or others could choose to compose one out of a decoder.

For now, I'd recommend either working off your fork or abusing InvocationHandlerFactory to supply default values :) It is likely something related will come out of #189 so I'd likely watch there.

codefromthecrypt pushed a commit that referenced this pull request Oct 31, 2015
This adds the `Feign.Builder.decode404()` flag which indicates decoders
should process responses with 404 status. It also changes all
first-party decoders (like gson) to return well-known empty values by
default. Further customization is possible by wrapping or creating a
custom decoder.

Prior to this change, we used custom invocation handlers as the way to
add fallback values based on exception or return status. `feign-hystrix`
uses this to return `HystrixCommand<X>`, but the general pattern applies
to anything that has a type representing both success and failure, such
as `Try<X>` or `Observable<X>`.

As we define it here, 404 status is not a retry or fallback policy, it
is just empty semantics. By limiting Feign's special processing to 404,
we gain a lot with very little supporting code.

For example, we have a fast-path for addressing absence which doesn't
accidentally turn bad request, redirect, silently to null. Processing
404 is a very safe policy: it doesn't overlap with retries or other
behavior in feign which could turn into support questions.

Also, we don't create a cliff, where folks looking for error handling
eventually realize they can't address all fallback policy with only an
http response code (bc exceptions can happen far before you get a
response from the server). By noting 404 as a special case, we avoid a
slippery slope of incrementally half-implementing Hystrix or similar.

This design supports the common pattern of empty semantics with the
least code on both users and the maintainers of Feign. We can still
recommend other libraries such as Hystrix or Ribbon for advanced retry
or fallback policy.

Most importantly, addressing 404 is extremely high value. You are just the last to mention it. For example, I would use this in denominator. Moreover, it doesn't require us to introduce or break an interface. This means all users, such as spring-cloud, can easily opt-into this. It could substantially reduce their code in the same way it can reduce your code, but without new types.

See #238 #287
codefromthecrypt pushed a commit that referenced this pull request Oct 31, 2015
This adds the `Feign.Builder.decode404()` flag which indicates decoders
should process responses with 404 status. It also changes all
first-party decoders (like gson) to return well-known empty values by
default. Further customization is possible by wrapping or creating a
custom decoder.

Prior to this change, we used custom invocation handlers as the way to
add fallback values based on exception or return status. `feign-hystrix`
uses this to return `HystrixCommand<X>`, but the general pattern applies
to anything that has a type representing both success and failure, such
as `Try<X>` or `Observable<X>`.

As we define it here, 404 status is not a retry or fallback policy, it
is just empty semantics. By limiting Feign's special processing to 404,
we gain a lot with very little supporting code.

For example, we have a fast-path for addressing absence which doesn't
accidentally turn bad request, redirect, silently to null. Processing
404 is a very safe policy: it doesn't overlap with retries or other
behavior in feign which could turn into support questions.

Also, we don't create a cliff, where folks looking for error handling
eventually realize they can't address all fallback policy with only an
http response code (bc exceptions can happen far before you get a
response from the server). By noting 404 as a special case, we avoid a
slippery slope of incrementally half-implementing Hystrix or similar.

This design supports the common pattern of empty semantics with the
least code on both users and the maintainers of Feign. We can still
recommend other libraries such as Hystrix or Ribbon for advanced retry
or fallback policy.

See #238 #287
codefromthecrypt pushed a commit that referenced this pull request Oct 31, 2015
This adds the `Feign.Builder.decode404()` flag which indicates decoders
should process responses with 404 status. It also changes all
first-party decoders (like gson) to return well-known empty values by
default. Further customization is possible by wrapping or creating a
custom decoder.

Prior to this change, we used custom invocation handlers as the way to
add fallback values based on exception or return status. `feign-hystrix`
uses this to return `HystrixCommand<X>`, but the general pattern applies
to anything that has a type representing both success and failure, such
as `Try<X>` or `Observable<X>`.

As we define it here, 404 status is not a retry or fallback policy, it
is just empty semantics. By limiting Feign's special processing to 404,
we gain a lot with very little supporting code.

If instead we opened all codes, Feign could easily turn bad request,
redirect, or server errors silently to null. This sort of configuration
issue is hard to troubleshoot. 404 -> empty is a very safe policy vs
all codes.

Moreover, we don't create a cliff, where folks seeking fallback policy
eventually realize they can't if only given a response code. Fallback
systems like Hystrix address exceptions that occur before or in lieu of
a response. By special-casing 404, we avoid a slippery slope of half-
implementing Hystrix.

Finally, 404 handling has been commonly requested: it has a clear use-
case, and through that value. This design supports that without breaking
compatibility, or impacting existing integrations such as Hystrix or
Ribbon.

See #238 #287
codefromthecrypt pushed a commit that referenced this pull request Oct 31, 2015
This adds the `Feign.Builder.decode404()` flag which indicates decoders
should process responses with 404 status. It also changes all
first-party decoders (like gson) to return well-known empty values by
default. Further customization is possible by wrapping or creating a
custom decoder.

Prior to this change, we used custom invocation handlers as the way to
add fallback values based on exception or return status. `feign-hystrix`
uses this to return `HystrixCommand<X>`, but the general pattern applies
to anything that has a type representing both success and failure, such
as `Try<X>` or `Observable<X>`.

As we define it here, 404 status is not a retry or fallback policy, it
is just empty semantics. By limiting Feign's special processing to 404,
we gain a lot with very little supporting code.

If instead we opened all codes, Feign could easily turn bad request,
redirect, or server errors silently to null. This sort of configuration
issue is hard to troubleshoot. 404 -> empty is a very safe policy vs
all codes.

Moreover, we don't create a cliff, where folks seeking fallback policy
eventually realize they can't if only given a response code. Fallback
systems like Hystrix address exceptions that occur before or in lieu of
a response. By special-casing 404, we avoid a slippery slope of half-
implementing Hystrix.

Finally, 404 handling has been commonly requested: it has a clear use-
case, and through that value. This design supports that without breaking
compatibility, or impacting existing integrations such as Hystrix or
Ribbon.

See #238 #287
velo pushed a commit that referenced this pull request Oct 8, 2024
This adds the `Feign.Builder.decode404()` flag which indicates decoders
should process responses with 404 status. It also changes all
first-party decoders (like gson) to return well-known empty values by
default. Further customization is possible by wrapping or creating a
custom decoder.

Prior to this change, we used custom invocation handlers as the way to
add fallback values based on exception or return status. `feign-hystrix`
uses this to return `HystrixCommand<X>`, but the general pattern applies
to anything that has a type representing both success and failure, such
as `Try<X>` or `Observable<X>`.

As we define it here, 404 status is not a retry or fallback policy, it
is just empty semantics. By limiting Feign's special processing to 404,
we gain a lot with very little supporting code.

If instead we opened all codes, Feign could easily turn bad request,
redirect, or server errors silently to null. This sort of configuration
issue is hard to troubleshoot. 404 -> empty is a very safe policy vs
all codes.

Moreover, we don't create a cliff, where folks seeking fallback policy
eventually realize they can't if only given a response code. Fallback
systems like Hystrix address exceptions that occur before or in lieu of
a response. By special-casing 404, we avoid a slippery slope of half-
implementing Hystrix.

Finally, 404 handling has been commonly requested: it has a clear use-
case, and through that value. This design supports that without breaking
compatibility, or impacting existing integrations such as Hystrix or
Ribbon.

See #238 #287
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.