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.

Sign up for GitHub

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

Jump to bottom

1048 simple illustration - more details in the default feignexception… #1095

Merged
merged 2 commits into from Oct 22, 2019
Merged

1048 simple illustration - more details in the default feignexception… #1095

merged 2 commits into from Oct 22, 2019

Conversation

jerzykrlk
Copy link
Contributor

… message

@jerzykrlk jerzykrlk mentioned this pull request Oct 12, 2019
Open
@jerzykrlk jerzykrlk force-pushed the more-details-in-feignexception-message-poc branch 2 times, most recently from 7b41a4d to 942e453 Compare October 12, 2019 14:06
velo
velo reviewed Oct 13, 2019
View reviewed changes
Copy link
Member

@velo velo left a comment

Choose a reason for hiding this comment

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

I liked the idea...

Wonder if would be interesting to combine with #1091.

FWIW, looking for a second opinion on 1091

That would also allow to show which method was invoked.

core/src/main/java/feign/FeignException.java Outdated
@@ -131,6 +136,11 @@ public static FeignException errorStatus(String methodKey, Response response) {
} catch (IOException ignored) { // NOPMD
}

String message = new FeignExceptionMessageBuilder()
.withResponse(response)
.withRequestMethod(response.request().httpMethod())
Copy link
Member

Choose a reason for hiding this comment

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

could read the method from the response, eliminating the need to call withRequestMethod method

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@velo velo added enhancement For recommending new capabilities feedback provided Feedback has been provided to the author ready to merge Will be merged if no other member ask for changes labels Oct 13, 2019
@jerzykrlk jerzykrlk force-pushed the more-details-in-feignexception-message-poc branch 5 times, most recently from 65c61d9 to e2cb5b8 Compare October 14, 2019 20:33
@jerzykrlk jerzykrlk force-pushed the more-details-in-feignexception-message-poc branch from e2cb5b8 to 4bf0ea8 Compare October 14, 2019 20:42
@jerzykrlk
Copy link
Contributor Author

Hi @velo, thanks for feedback.

I've polished up the code, and I've abbreviated the response body when it's too long (well, more than 200 chars didn't seem readable to me).

I'm not sure about #1091 - give me some time to understand that.

Can I have one more question for the future?

Quite often, I want to further modify the response body preview. For example - some APIs render 5xx of 4xx pages with HTML. Well, not the perfect world, but I have to live with that. So I need to strip all the HTML to find out, what happened.

I'd like to make extracting the response body customisable and injectable, but I can't seem to find a good way with current code (static method of FeignException doing all the hard work). How could I do that?

@velo
Copy link
Member

velo commented Oct 22, 2019

How could I do that?

ErrorDecoder may be?!

@velo velo merged commit 76a4524 into OpenFeign:master Oct 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@velo velo velo approved these changes

Assignees
No one assigned
Labels
enhancement For recommending new capabilities feedback provided Feedback has been provided to the author ready to merge Will be merged if no other member ask for changes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@jerzykrlk @velo