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

Exception hierarchy under FeignException #823

Closed
jerzykrlk opened this issue Oct 24, 2018 · 5 comments
Closed

Exception hierarchy under FeignException #823

jerzykrlk opened this issue Oct 24, 2018 · 5 comments
Labels
enhancement For recommending new capabilities waiting for votes Enhancements or changes proposed that need more support before consideration

Comments

@jerzykrlk
Copy link
Contributor

With default error encoder, Feign client will throw FeignException, which is quite general, compared to variety of HTTP error codes and their REST semantics.
In a rest-heavy application, introducing subclasses simplify quite a lot of my code.

Sample piece of code, now:

        try {
            return libraryClient.getBook(1);
        } catch (FeignException e) {
            if (e.status() == 404) {
                // handle the 404 error
            }
            throw e; // can't do anything about other http errors here
        }

And the same piece code, expected:

        try {
            return libraryClient.getBook(1);
        } catch (HttpNotFoundException e) {
            // handle the 404 error
        }

This change seems quite easy, compared ti the code it saves. I could prepare a pull request, if you like the idea.

@navaare
Copy link

navaare commented Oct 25, 2018

@jerzykrlk
How much code does it save in fact? Does everybody will be happy with that default behavior? All you need is to define your own exception passing a lot more info at defined ErrorDecoder for feign client.

@jerzykrlk
Copy link
Contributor Author

Hi,

I think the main benefit would not be less code, but just clener code.

In my apps, there are many places where I catch FeignException, manually check the status code - and if it's something else, rethrow the exception.

For example:

  • a call to a remote service returns HTTP 401,
  • my code catches FeignException, checks for status 401, reauthenticates,
  • otherwise - rethrows the FeignException.

I think it doesn't look good, especially with the same thing repeated in many places in code in many apps.

I already have a custom ErrorDecoder that does that - but I believe it'd be a good add-on to the Default error decoder.

This whole idea is a follow-up of the same feature in Spring RestTemplate - see SPR-15404 and this commit

All HTTP-related exceptions would inherit from FeignException, so the change would be backward compatible.

Could you have a look at #825? - I think it illustrates my idea clearly.

@kdavisk6 kdavisk6 added enhancement For recommending new capabilities waiting for votes Enhancements or changes proposed that need more support before consideration labels Oct 26, 2018
@kdavisk6
Copy link
Member

The role of the ErrorDecoder is to allow users to maintain full control. I do see the benefit, but it may not be Feign's place to make this determination across the board for all users nor desired. In practice, we typically do not accept enhancement requests without additional votes. I've marked this issue as such. If it receives enough support, we can consider this more.

@jerzykrlk
Copy link
Contributor Author

Ok, thank you for the update.

You already have first vote, I guess... :)

@kdavisk6
Copy link
Member

kdavisk6 commented Dec 5, 2018

Closed via #825

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement For recommending new capabilities waiting for votes Enhancements or changes proposed that need more support before consideration
Projects
None yet
Development

No branches or pull requests

3 participants