SQL: wrap runtime errors when decoding a cursor - #158437
HuzaifaChaudary wants to merge 3 commits into
Conversation
the cursor comes from the request, so a broken one can fail in ways that are not IOException. bad base64 throws IllegalArgumentException, a negative array length throws NegativeArraySizeException, an oversized one throws IllegalStateException. only IOException was caught, so those came back raw. closes elastic#157664
|
💚 CLA has been signed |
There was a problem hiding this comment.
🟢 Approval recommended
The change is narrowly scoped, matches the reported failure mode, and is backed by a targeted regression test.
Pull request overview
This PR hardens SQL cursor decoding against malformed cursor strings coming directly from user requests by ensuring runtime decoding failures are consistently returned as SqlIllegalArgumentException rather than leaking raw runtime exceptions to callers.
Changes:
- Broadened exception handling during cursor/formatter decoding to wrap non-
IOExceptionruntime failures intoSqlIllegalArgumentException. - Added a regression test covering several malformed cursor strings and asserting the user-facing error message.
- Avoided double-wrapping by rethrowing existing
SqlIllegalArgumentExceptionduring recursive cursor decoding.
File summaries
| File | Description |
|---|---|
| x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/session/Cursors.java | Wrap runtime cursor/formatter decode failures as SqlIllegalArgumentException and avoid double-wrapping on recursive decode. |
| x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/plugin/CursorTests.java | Add test asserting malformed cursors fail with a consistent SqlIllegalArgumentException message. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| } | ||
| } catch (IOException ex) { | ||
| } catch (SqlIllegalArgumentException ex) { | ||
| // thrown by the nested call above, already the right shape |
There was a problem hiding this comment.
right, and it is not only a wording problem. SqlStreamInput.fromString throws this same type itself when the stream is not a cursor stream:
throw new SqlIllegalArgumentException("Expected SQL cursor stream, received [{}]", in.getClass());that is inside the try, so the clause catches it too and the comment named the wrong single source. reworded to say what the clause is actually for , not wrapping an exception that is already the right type and message, wherever in the try it came from. comment only, the bytecode is unchanged.
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
the comment named the nested call as the only source. SqlStreamInput.fromString throws the same type from inside the try when the stream is not a cursor stream, so the clause is doing more than the comment claimed.
Closes #157664
The cursor in a SQL request is just a base64 string from the caller, so a broken one can fail in ways that are not
IOException, and onlyIOExceptionwas caught:IllegalArgumentExceptionfromBase64.getDecoder().decodeNegativeArraySizeExceptionfromStreamInput.readArraySize, which is the one in the stack trace on the issueIllegalStateExceptionfrom the same placeAll three came back raw instead of as
SqlIllegalArgumentException.Widened the catch in the two places that decode a cursor string. The nested call in
internalDecodeFromStringWithZonecan throwSqlIllegalArgumentExceptionitself, so that is rethrown as is rather than wrapped twice.The test feeds four broken cursors and checks the error, including plain bad base64 which is the easiest one to hit. Without the change it fails with
Illegal base64 character 20.:x-pack:plugin:sql:testpasses, 3385 tests in 106 classes.