Skip to content

fix(sql-execute): fall back to exception type when error message is null - #634

Open
ydflow wants to merge 1 commit into
spring-ai-alibaba:mainfrom
ydflow:fix/sql-execute-null-error-message
Open

ydflow wants to merge 1 commit into
spring-ai-alibaba:mainfrom
ydflow:fix/sql-execute-null-error-message

Conversation

@ydflow

@ydflow ydflow commented Sep 22, 2026

Copy link
Copy Markdown

What this PR does / why we need it

SqlExecuteNode#executeSqlQuery reads e.getMessage() straight into two places:

.onErrorResume(e -> {
    String errorMessage = e.getMessage();
    result.put(SQL_REGENERATE_REASON, SqlRetryDto.sqlExecute(errorMessage));
    return Flux.just(ChatResponseUtil.createResponse("SQL执行失败: " + errorMessage));
});

NullPointerException, StackOverflowError and several JDBC driver wrappers carry a null message. In that case the streamed text the user sees becomes the literal

$$$SQL执行失败: null

and SqlRetryDto.sqlExecute(null) stores null as the retry reason. The user gets no actionable information about why their query failed, and the log line is the only place the real cause exists.

Root cause

Throwable#getMessage() is nullable by contract, but the value is used unconditionally in user-facing output. log.error("...", sqlQuery, e) is fine with a null message (it only formats into the log line), which is why this only shows up on the UI side.

The fix

Fall back to the exception class name when the message is blank:

// NPE / StackOverflowError and several JDBC wrappers carry a null message;
// fall back to the exception type so the user never sees the literal "null"
String errorMessage = StringUtils.isNotBlank(e.getMessage()) ? e.getMessage() : e.getClass().getName();

org.apache.commons.lang3.StringUtils is already imported in this file.

  • When the message exists, behaviour is byte-for-byte unchanged.
  • When it does not, the user sees e.g. SQL执行失败: java.lang.NullPointerException instead of SQL执行失败: null — still terse, but it names the failure class and is what a support/debug conversation needs.
  • The logged stack trace is untouched, so no diagnostic information is lost.

How to verify

RED (before the fix)

mvn -o -pl data-agent-management -am -Dtest=SqlExecuteNodeTest -Dsurefire.failIfNoSpecifiedTests=false test

Tests run: 16, Failures: 1, Errors: 0, Skipped: 0
  SqlExecuteNodeTest.apply_sqlExecutionErrorWithoutMessage_doesNotLeakNullIntoUserFacingText
    user-facing text must not leak the literal 'null', was: 开始执行SQL...
    $$$SQL执行失败: null

The assertion fires on the exact streamed text the node emits, so the failure is in the production path, not in a stub.

GREEN (after the fix)

Same command → Tests run: 16, Failures: 0, Errors: 0, Skipped: 0, BUILD SUCCESS.

Revert check

Reverting only the errorMessage line reproduces the single failure; restoring it turns green again.

Full module

mvn -o -pl data-agent-management -am -Dspotless.apply.skip=true test

Tests run: 1713, Failures: 6, Errors: 0, Skipped: 0

The 6 failures (PromptHelperTest 4, NodeTracingLifecycleListenerTest 1, PlannerNodeTest 1) are pre-existing on upstream main and unrelated to this change — isolated by reverting to the upstream versions of the touched files and re-running those three suites (Tests run: 76, Failures: 6). The total went from 1712 to 1713 because this PR adds one test.

mvn -o -pl data-agent-management -am -Dspotless.apply.skip=true checkstyle:check → You have 0 Checkstyle violations. BUILD SUCCESS.

Special notes for reviews

  • No new imports, no API/schema change, no behaviour change when the exception has a message.
  • The same e.getMessage()-into-user-text pattern exists in ~20 controller catch blocks (e.g. DatasourceController, AgentDatasourceController, AgentKnowledgeController). Those go through GlobalExceptionHandler and fixing them all would balloon this into a large cross-cutting PR, so they are deliberately left out of scope. Happy to follow up separately if maintainers want that.
  • The other two onErrorResume blocks in SqlExecuteNode (chart-config paths) only pass the message to log.warn, which tolerates null, and never surface it to the user — no change needed there.

SqlExecuteNode#executeSqlQuery reads e.getMessage() straight into both the
user-facing streamed text ("SQL执行失败: " + errorMessage) and
SqlRetryDto.sqlExecute(). NPE, StackOverflowError and several JDBC driver
wrappers carry a null message, so the UI rendered the literal
"SQL执行失败: null" and the retry reason stored null.

Fall back to the exception class name when the message is blank, so the
user always sees an actionable reason. The logged stack trace is
unchanged.

Add a regression test that throws a message-less RuntimeException and
asserts no "null" leaks into the streamed text while the failure notice
and retry reason are still produced.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant