SPR-14818: Added MissingHeaderException type #1653
Conversation
|
@pebo Please sign the Contributor License Agreement! Click here to manually synchronize the status of this Pull Request. See the FAQ for frequently asked questions. |
|
@pebo Thank you for signing the Contributor License Agreement! |
| * limitations under the License. | ||
| */ | ||
|
|
||
| package org.springframework.web.bind; |
There was a problem hiding this comment.
Is this the right place to sit the exception?
There was a problem hiding this comment.
Don't know but the package contains similar exceptions like MissingPathVariableException
| @@ -82,8 +83,7 @@ protected Object resolveName(String name, MethodParameter parameter, NativeWebRe | |||
|
|
|||
| @Override | |||
| protected void handleMissingValue(String name, MethodParameter parameter) throws ServletRequestBindingException { | |||
| throw new ServletRequestBindingException("Missing request header '" + name + | |||
| "' for method parameter of type " + parameter.getNestedParameterType().getSimpleName()); | |||
| throw new MissingHeaderException(name, parameter); | |||
There was a problem hiding this comment.
In my humble opinion, this is fine as it used to be, as including a new class is adding also complexity. So, what are the advantages adding this exception class just to manage this one? But this is my opinion!!
There was a problem hiding this comment.
Agree with the added complexity. I found https://jira.spring.io/browse/SPR-14818 when trying to return a custom error message for missing headers in the ResponseEntityExceptionHandler#handleServletRequestBindingException callback. Now my workaround is to check the message text of the ServletRequestBindingException for the text "Missing request header".. is there a better way to implement the ResponseEntityExceptionHandler callback?
There was a problem hiding this comment.
I see what you mean, they throw more specific exception, more meaningful. I guess it is alright, let see if someone else give their opinion. So far, I don't have anything better from the top of my head!! ;) good work!
|
I don't see any issue adding the new exception. It properly extends the existing exception so is backwards compatible. The code is now more descriptive of the issue. This fixes a challenge we have, which we have had to solve with string parsing so this is a great PR in my opinion. Please approve |
|
@baynezy the PR is linked to an issue where you'll see that Juergen already commented. |
|
Dear pebo(@pebo) & fmcejudo(@fmcejudo), We are working on identifying redundant development and duplicated pull requests. We have found there is a pull request: #1218 which might be a potentially duplicate to this one. We would really appreciate if you could help us to validate and give us feedback. Thank you very much for your time! |
|
Resolved as part of a wider revision now, introducing |
No description provided.