Throw better error if invalid scroll id is used #5738
Conversation
| @@ -96,13 +96,17 @@ public static ParsedScrollId parseScrollId(String scrollId) { | |||
| try { | |||
| byte[] decode = Base64.decode(scrollId, Base64.URL_SAFE); | |||
| UnicodeUtil.UTF8toUTF16(decode, 0, decode.length, spare); | |||
| } catch (IOException e) { | |||
| } catch (Exception | AssertionError e) { | |||
There was a problem hiding this comment.
I don't think we should catch assertions errors anywhere in our code..., what happens when they are disabled?
There was a problem hiding this comment.
agreed we should not catch those
There was a problem hiding this comment.
I don't like it as well... But a test (SearchScrollTests line 325) makes an assertion fail in UnicodeUtil#UTF8toUTF16 and this was my attempt to make it fail consistently with the other check I added in this method.
I can catch AssertionError in the test instead? Also the when the scroll id in test is used outside the test framework it will fail on the check I below here.
|
The AssertionError is now catched in the test and if running without -ea an ElasticSearchIllegalArgumentException is thrown, however I can't test this since the tests run with assertion enabled. |
|
LGTM |
lcawl
added
:Core/Infra/Core
PR for #5730 but only dealing with invalid scroll_ids and not non existing ones.