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

Added FeignRequestException, FeignResponseException #769

Merged

Conversation

finke-ba
Copy link
Contributor

@finke-ba finke-ba commented Aug 14, 2018

Idea was to use two different classes for holding Request and Response objects.

Couple of moments that I'm not sure about:

  1. Add request object to RetryableException and to errorExecuting method.
  2. In FeignResponseException I'm copying response body to another input stream due to root input stream already closed on the moment of handling exception.
  3. I deleted the response body from the message variable what makes this commit back incompatible.
  4. Test added put in correct place/class.

I'm looking forward to questions and comments.

Fixes #642

@kdavisk6
Copy link
Member

The only difference between FeignRequestException and FeignResponseException is the inclusion of a body attribute. I would reconsider this approach. If the intent is to make the body available, we can add a body attribute to FeignException and get the same result, without the cascading changes.

Can you give that a try?

@kdavisk6 kdavisk6 self-requested a review August 15, 2018 20:53
Copy link
Member

@kdavisk6 kdavisk6 left a comment

Choose a reason for hiding this comment

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

See my comments

@kdavisk6 kdavisk6 added the enhancement For recommending new capabilities label Aug 15, 2018
@finke-ba
Copy link
Contributor Author

@kdavisk6 Thanks for comment!
The main idea to create two additional exceptions was to have Request and Response object in Exception class. It gives more flexibility instead of adding just a body. Maybe it's already too much.
But please check carefully comments for this PR #664 first, because I used the ideas from that.

@kdavisk6
Copy link
Member

The notions of Request and Response are abstractions that are within OpenFeign. User's of the library are defining interfaces, with methods, not Requests and Responses. By exposing those abstractions through FeignException, we are leaking those out to users, requiring them to at least be aware of those abstractions and how to work with them. This is the definition of Leaky Abstractions, and it's something we should strive to avoid.

The original issue (#642), indicated that a separate field, with the appropriate body, would be sufficient, and I agree. Another suggestion that might clear this up is to not call the field body, but instead call it details or something else. body or RequestBody is another abstraction we should avoid leaking.

User's can then inspect the Exception details for additional information.

@finke-ba
Copy link
Contributor Author

finke-ba commented Aug 16, 2018

Ok, that sounds reasonable. I'll create another implementation with one additional attribute in FeignException .

@finke-ba
Copy link
Contributor Author

@kdavisk6 , hello!
Please check my update.

Copy link
Member

@kdavisk6 kdavisk6 left a comment

Choose a reason for hiding this comment

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

Thank your making these changes.

@kdavisk6
Copy link
Member

If there are no more comments in the next few days, I will accept this change.

@kdavisk6 kdavisk6 modified the milestones: 10.1.0, 10.0.2 Aug 17, 2018
@finke-ba
Copy link
Contributor Author

finke-ba commented Aug 17, 2018

That is awesome.
@kdavisk6 thank you a lot for your help!

@kdavisk6 kdavisk6 modified the milestones: 10.0.2, 10.1.0 Aug 17, 2018
@kdavisk6 kdavisk6 merged commit 3237229 into OpenFeign:master Aug 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement For recommending new capabilities
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FeignException with content as separate field
2 participants