-
Notifications
You must be signed in to change notification settings - Fork 24.6k
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.
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
Throw better error if invalid scroll id is used #5738
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed we should not catch those
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
PR for #5730 but only dealing with invalid scroll_ids and not non existing ones.