Skip to content

Proof of concept for Visitor-pattern-based handling of Responses#287

Closed
uschi2000 wants to merge 1 commit into
OpenFeign:masterfrom
uschi2000:feature/visitor
Closed

Proof of concept for Visitor-pattern-based handling of Responses#287
uschi2000 wants to merge 1 commit into
OpenFeign:masterfrom
uschi2000:feature/visitor

Conversation

@uschi2000

Copy link
Copy Markdown

Hi @adriancole ,

This pull-request is a proof-of-concept for making response handling more flexible. (Note that I haven't fixed up the tests, so it doesn't compile.) The motivation is as follows: Our servers often return 404s for non-existing resources and we'd like to surface those as Optional#absent (Guava or Java8) in clients, rather than as exceptions. Given the current architecture of response handling in SynchronousResponseHandler this is not possible since the configured Decoder is only invoked for [200, 300) error codes. This PR shows how to use a Visitor pattern to make response handling more flexible and configurable.

I'd like to know if you'd in principle be happy to consider this or a similar approach, or if you are opposed based on some fundamental consideration. In the former case, I'm happy to get APIs and tests in good shape according to your suggestions. Back-compat of the Feign.Builder class will be an issue.

Cheers!
Robert

@cloudbees-pull-request-builder

Copy link
Copy Markdown

NetflixOSS » feign » feign-pull-requests #170 FAILURE
Looks like there's a problem with this pull request

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a boolean handles(Response response, MethodMetadata metadata) and have handle return an Object.

@codefromthecrypt

Copy link
Copy Markdown

My suggestion would be to have a look at the hystrix support. If we added a plugin arch, it should be able to address existing use cases of extensibility. IOTW, we don't really want to do this twice and I think the hystrix code merged recently probably shows how to achieve your concerns. If it doesn't, it would be good to note the gaps. https://github.com/Netflix/feign/tree/master/hystrix

@uschi2000

Copy link
Copy Markdown
Author

Cheers, will have a look.

@uschi2000 uschi2000 closed this Oct 28, 2015
@uschi2000

Copy link
Copy Markdown
Author

@adriancole , I may not understand right what you mean. When I look at the implementation of the Hystrix module, it looks like it's wrapping the reflection proxy in order to wrap the value returned from remote into a new HystrixCommand. This technique doesn't quite work for the my case described above since I need to change the way the MethodHandler itself works; seeing it's return value is not enough. Maybe I'm missing something?

@codefromthecrypt

Copy link
Copy Markdown

I guess what I was thinking about is that you are looking to supply a fallback value (ex absent on 404). This requires coordination in the same way hystrix does. Hystrix has a richer fallback system, so if anything I would expect more code there.

For example, to acheive the opinion you want, you are teaching feign optional (codec) and also how to interpret exceptions (ex 404 should fallback to absent is possible today with an invocation handler).

Coordinating multiple aspects is what hystrix is doing as well.

By first using the least code, we can understand how much pain this causes and how this leads to a cure. I'm hesitant to add concepts that don't also increase the clarity of similar code such as hystrix which also handles fallbacks.

Did this explanation help or hurt?

@uschi2000

Copy link
Copy Markdown
Author

@adriancole thanks for your suggestions. An implementation analogous to the Hystrix one looks roughly like this:

final class OptionalAwareInvocationHandler implements InvocationHandler {
    private final Map<Method, MethodHandler> dispatch;

    OptionalAwareInvocationHandler(Map<Method, MethodHandler> dispatch) {
        this.dispatch = Util.checkNotNull(dispatch, "dispatch");
    }

    @Override
    public Object invoke(final Object proxy, final Method method, final Object[] args) throws Throwable {
        Type type = method.getGenericReturnType();
        if (type instanceof ParameterizedType && ((ParameterizedType) type).getRawType().equals(Optional.class)) {
            try {
                return Optional.of(dispatch.get(method).invoke(args));
            } catch (NotFoundException e) {
                return Optional.absent();
            }
        } else {
            return dispatch.get(method).invoke(args);
        }
    }

    static final class Factory implements InvocationHandlerFactory {
        @Override
        public InvocationHandler create(Target target, Map<Method, MethodHandler> dispatch) {
            return new OptionalAwareInvocationHandler(dispatch);
        }
    }
}
public final class OptionalAwareFeign {
    private OptionalAwareFeign() {}

    public static Builder builder() {
        return new Builder();
    }

    public static final class Builder extends Feign.Builder {
        public Builder() {
            invocationHandlerFactory(new OptionalAwareInvocationHandler.Factory());
            contract(new OptionalDelegatingContract(new Contract.Default()));
        }

        @Override
        public Feign.Builder contract(Contract contract) {
            return super.contract(new OptionalDelegatingContract(contract));
        }
    }
}

A few things feel a little off about this:

  • Extending Feign.Builder is a bit off a hack, both in terms of implementation and usability
  • Implementing the Optional functionality at the level of the InvocationHandler is a little cumbersome: I first let "vanilla feign" do its work by delegating, then catch a subset of errors (NotFoundException, in this case) and turn them into Optional#absent. I believe it would be much more transparent if this functionality were implemented at the decoder level.
  • IMO error handling (ErrorDecoder.class) and decoding (Decoder.class) are the same abstract concept: You see an HTTP Response and need to surface an appropriate Java "answer". I don't think it's necessary to force users to see exceptions for anything outside [200-299) by delegating to an ErrorDecoder otherwise; this is the correct default, but there is no architectural reasons users should not be allowed to override this.

Bottom-line: I can run with the above work-around for now, but would love to see a more first-class way of allowing users to configure response decoding. Happy to help out with code as well, let me know.

@codefromthecrypt

Copy link
Copy Markdown

Thanks for the input!

@spencergibb @jdamick any other ideas on this? I've been highly resistant
to introducing a fallback handler in feign as it is a half-fix when
compared to hystrix. We did use this often in jclouds, almost always to
turn 404s in to absent or empty list.

The 404 scenario is common enough that I could see us entertaining an
option to either widen what decoder can interpret to any status (which is
imho instant tech debt), or maybe special casing 404 somehow. Ex. an
internal or package visible variable to allow decoder to interpret 404..

@uschi2000

Copy link
Copy Markdown
Author

Adrian, do I understand right that feign in your opinion should be opinionated about interpreting HTTP codes? I don't (yet) see the downside of providing sane defaults (as per the current behaviour), but allowing users to customize as they see fit.

@codefromthecrypt

codefromthecrypt commented Oct 29, 2015 via email

Copy link
Copy Markdown

@jdamick

jdamick commented Oct 29, 2015

Copy link
Copy Markdown
Contributor

perhaps the error decoder could give the option to not throw.. it seems like if there was a ThrowingErrorDecoder vs. Non that would more simply solve this problem?

@codefromthecrypt

codefromthecrypt commented Oct 29, 2015 via email

Copy link
Copy Markdown

@codefromthecrypt

codefromthecrypt commented Oct 29, 2015 via email

Copy link
Copy Markdown

@codefromthecrypt

codefromthecrypt commented Oct 29, 2015 via email

Copy link
Copy Markdown

@uschi2000

Copy link
Copy Markdown
Author

Thanks for sharing your philosophy, Adrian. It would be great to see 404->Optional handling and I trust you will find the right way of supporting it in feign. Until then, I will go with the InvocationHandler route.

@codefromthecrypt

Copy link
Copy Markdown

ok here's the gist. I think we should introduce 404 -> something via a decoder flag.

next in the long answers :)

We have one pattern for integration of systems that have a special return value. feign-hystrix is a great example, of this, but it also extends to similar things like Try<X>, Single<X>, etc. Basically, anything that has custom error handling which needs to be taken into consideration when creating a response.

I don't think 404 falls into this bucket, or needs to. If we had a way to address 404, which is semantically defined as absent, we gain a lot.

Firstly, we have a fast-path for addressing absence which doesn't accidentally also catch bad request, redirect, etc. In other words, it is a very safe policy: it doesn't overlap with retries or other behavior in feign.

Secondly, we don't create a cliff, where folks looking for error handling eventually realize they can't address all fallbacks with only an http response code (bc exceptions can happen far before you get a response from the server). This is a slippery slope that leads to things like Hystrix, so might as well encourage that for real errors.

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.

@uschi2000

Copy link
Copy Markdown
Author

Great, thanks a lot!

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.

5 participants