Skip to content

fix: fail typed JavaScript results on conversion errors - #26920

Draft
totally-not-ai[bot] wants to merge 4 commits into
mainfrom
fix/typed-js-result-conversion-errors
Draft

totally-not-ai[bot] wants to merge 4 commits into
mainfrom
fix/typed-js-result-conversion-errors

Conversation

@totally-not-ai

@totally-not-ai totally-not-ai Bot commented Nov 9, 2026 •

Copy link
Copy Markdown
Contributor

Part of #26919

Behavior deviation · flow-server · apps using typed executeJs/callJsFunction results

When a typed JavaScript result can't be converted to the requested type (for example the browser returns "abc" for an Integer), the conversion error escaped the result handler. A typed toCompletableFuture(...) then never completed, so join()/get() hung and whenComplete callbacks never ran, and a typed then(..., errorHandler) threw instead of calling the error handler.

Risks:

  • ⚠️ Behaviour change: a conversion error now goes to the error handler (or fails the future with JavaScriptException) instead of being thrown from the response handling; with no error handler it is logged as a warning.

  • ⚠️ Behaviour change: JacksonCodec.decodeAs(JsonNode, Class) (internal) throws IllegalArgumentException instead of ClassCastException when the target is a JSON node type the value isn't.

  • ✅ No public API change, no serialization or threading impact.

  • Typed then(Class|TypeReference, …) decode inside a try and route IllegalArgumentException to the error handler; handlers added after it still run.

  • Typed toCompletableFuture(Class|TypeReference) reuse the typed then, so conversion errors complete the future exceptionally.

  • Exceptions thrown by the user's own result handler still propagate unchanged.

  • decodeAs(JsonNode, Class) reports a node type mismatch (e.g. ObjectNode for an array) as IllegalArgumentException naming the node type.

The other part of the issue (aligning null-argument checks and JSON-null handling across the three decodeAs overloads) is left for after #26890, which touches the same code.

Fixes #26919

A typed then or toCompletableFuture on PendingJavaScriptResult threw
the conversion error from the result handler, so the future never
completed and handlers added after it were skipped. Route the error
to the error handler instead, log it when there is none, and make
JacksonCodec.decodeAs(JsonNode, Class) report a JSON node type
mismatch as IllegalArgumentException like the other failures.

Part of #26919
@github-actions

github-actions Bot commented Nov 9, 2026 •

Copy link
Copy Markdown
Contributor

Test Results

 1 677 files  ±0   1 766 suites  ±0   2h 8m 52s ⏱️ - 3m 51s
13 338 tests +6  13 270 ✅ +6  68 💤 ±0  0 ❌ ±0 
13 972 runs  +6  13 904 ✅ +6  68 💤 ±0  0 ❌ ±0 

Results for commit 76d78f7. ± Comparison against base commit a381afa.

♻️ This comment has been updated with latest results.

@totally-not-ai

totally-not-ai Bot commented Nov 9, 2026

Copy link
Copy Markdown
Contributor Author

Added a Javadoc description to the new decodingHandler helper in PendingJavaScriptResult (S1176). The other 13 findings are on test code this PR doesn't change: the flagged lines in PendingJavaScriptInvocationTest (398–525) and JacksonCodecTest (373, 797) are existing tests whose line numbers only moved because of new imports or tests added above them. I left them as they are to keep this PR to one concern.

@sonarqubecloud

sonarqubecloud Bot commented Nov 9, 2026

Copy link
Copy Markdown

@totally-not-ai

totally-not-ai Bot commented Nov 9, 2026

Copy link
Copy Markdown
Contributor Author

Documented the parameters and return value of the decodingHandler helper in PendingJavaScriptResult (S1176). The other findings are unchanged from the previous analysis: they are on existing test code in PendingJavaScriptInvocationTest and JacksonCodecTest that this PR doesn't modify, so they stay out of scope here.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Normalize JacksonCodec.decodeAs overloads and fail typed PendingJavaScriptResult futures on conversion errors

0 participants