Restore exception semantics for RequestParams#145212
Restore exception semantics for RequestParams#145212felixbarny merged 5 commits intoelastic:mainfrom
Conversation
…xception directly Move the IllegalArgumentException → BadParameterException conversion to RestRequest.request(), where it belongs, rather than inside RequestParams which has no dependency on RestRequest.
elasticsearchmachine
added
needs:triage
felixbarny
added
>test
elasticsearchmachine
added
Team:Security
|
Pinging @elastic/es-security (Team:Security) |
There was a problem hiding this comment.
LGTM w.r.t. the security test case; keeping the 400 instead of throwing 500 is what we want here 👍
Requesting a review from one of the original PR's reviewers since they'll have more context.
I think it's worth adding some light unit tests for this and the two other methods -- a quick check of fromQueryString doesn't show any coverage of it invoked on invalid inputs.
…Params Now that RequestParams throws IllegalArgumentException directly (rather than wrapping it as BadParameterException), the testReservedParameters test needs to match.
DaveCTurner
left a comment
DaveCTurner
left a comment
There was a problem hiding this comment.
LGTM2 tho needs some tests.
Also I think BadParameterException should map to a 400 anyway? I mean I know it doesn't today but we could change that too.
That's because I forgot to update the tests. Should be covered in d91ea45. |
Maybe it should extend |
Move the IllegalArgumentException → BadParameterException conversion to RestRequest.request(), where it belongs, rather than inside RequestParams which has no dependency on RestRequest.
4 participants
#144506 introduced
RequestParamsand had its factory methods wrapIllegalArgumentExceptionfromRestUtils.decodeQueryStringasRestRequest.BadParameterException. This caused a regression: the exception was thrown before reachingRestRequest.request(), so it was not caught and translated into a 400 response — instead bubbling up as a 500.This PR restores the previous behaviour by having
RequestParamsthrowIllegalArgumentExceptiondirectly (asRestUtils.decodeQueryStringalways did), and moving theIllegalArgumentException→BadParameterExceptionconversion back intoRestRequest.request(), where the 400 translation happens.Closes #145197