-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Add fine-grained HTTP error exceptions #825
Conversation
jerzykrlk
commented
Oct 26, 2018
- a hierarchy of HTTP-related exceptions under FeignException.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm good with the change, but, CI is not passing
b27e5ef
to
f24ac00
Compare
f24ac00
to
030fa68
Compare
I'm sorry about that. Fixed. |
@@ -85,4 +124,100 @@ static FeignException errorExecuting(Request request, IOException cause) { | |||
cause, | |||
null); | |||
} | |||
|
|||
public static class BadRequest extends FeignException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like all the 4xx ones should extend some sort of 4xx exception (say FiengClientException
) and the same with 5xx errors (maybe FeignServerException
).
Yes, that's how Spring's exception hierarchy looks like. I have personally neved had to catch Client/Server exceptions with Spring's RestTemplate. I rather catch one-two specific exceptions or all of them. So I skipped them here. That's my use case, though. If you see the point and would like them here, let me know - I'll try to add another PR with them, early next week. |
The only point I see is that 4xx means some form of bad request so don't try this again because it will never work, while 5xx means something failed on the server so if you try again later (say after some dependent service comes back online) it might work. I don't personally have an immediate use for it, but if you are already doing the multiple different types, then I think a hierarchical structure like that would be good. |
Sure, here's another pull request: #854 |