Request field added to FeignException #1039
Conversation
There was a problem hiding this comment.
Thank you for submitting this PR. I have one request regarding the changes to FeignException constructor. To make it clearer that Request is optional, we can have 2 constructors, one that resembles the original, where the internal Request is null, and your new overloaded version, that accepts a request and verifies that it is not null:
protected FeignException(int status, String message, Throwable cause) {
this.request = null;
}
protected FeignException(int status, String message, Request request, Throwable cause) {
checkNotNull(request);
}This makes our intention clearer and doesn't require as many changes to the test cases, pushing nulls around.
|
Thank you for your feedback. I thought about making old constructors @deprecated... Do you think the good way is to deprecate them or just return as is? |
|
The older constructors are still relevant and there are additional use cases where we may want to allow exceptions without a request. That being said, there is no need for deprecation, just additional options. |
…ors are defended from null in request argument
|
done |
| super(message, cause); | ||
| this.status = status; | ||
| this.content = content; | ||
| this.request = checkNotNull(request, "request"); |
There was a problem hiding this comment.
We should have a real message here. This will result in an NPE with a message of "request". Can you change to read something like "request is required"
There was a problem hiding this comment.
I just grep project to check how checkNotNull invokes and made same. But ok, changed! )
|
done |
kdavisk6
added
ready to merge
Fixes #845 This change allows Feign Exceptions to be created with the original request as an optional parameter. Changes include: * Request field added to FeignException * New constructors are defended from null in request argument * Tests to check null instead of request, null message updated
Fixes #845 This change allows Feign Exceptions to be created with the original request as an optional parameter. Changes include: * Request field added to FeignException * New constructors are defended from null in request argument * Tests to check null instead of request, null message updated
Fixes: #845
Request field added to FeignException. Other exceptions changed too except Encode Exception (Request is null here)