aglinxinyuan opened a new pull request, #7802:
URL: https://github.com/apache/texera/pull/7802
### What changes were proposed in this PR?
Three defects in `WorkflowWebsocketResource`, all previously characterized
without being cemented (#7303, #7676) so a fix would not have to fight a test
asserting the broken behaviour.
**1 & 2. A workflow with no execution NPEs instead of reporting "not
initialized".**
`WorkflowService.executionService` is a `BehaviorSubject` with **no initial
value** (`WorkflowService.scala:140`), so `getValue` is `null` until an
execution is published.
- `case other =>` used `workflowStateOpt.map(_.executionService.getValue)`,
which wraps that null into `Some(null)` — walking past the `case None` arm that
exists to report the friendly error, then NPEing on `value.wsInput`. Now uses
the already-computed `executionStateOpt`, which is built with `Option(...)`, so
`Some(null)` cannot form and the existing `None` arm actually fires.
- The `ModifyLogicRequest` arm had the same gap in a different shape: its
guard tested the *workflow* where it meant the *execution*. Now
`executionStateOpt.getOrElse(throw new IllegalStateException("workflow
execution is not initialized"))`.
Line 89 of this file already used `Option(...)` correctly, as does
`WorkflowService` at its lines 208 and 349 — the fix adopts the established
in-tree idiom rather than inventing one.
**A shape decision worth reviewing.** For the `ModifyLogicRequest` arm I did
*not* simply swap the outer condition to `executionStateOpt.isDefined`. That
variant makes a workflow-without-execution **silently do nothing** instead of
reporting, which is not the intent — and it would collide with the pre-existing
test "ignore a ModifyLogicRequest that arrives before any workflow is
attached", which pins the no-workflow case as `noException` plus `sent shouldBe
empty`. Keeping the workflow guard and reporting the absent execution satisfies
both.
**3. An unparseable frame no longer escapes the error mapper.**
`objectMapper.readValue` moved from above the `try` to its first statement, so
a frame the mapper cannot bind is reported like any handler failure.
`sessionState` and `executionStateOpt` stay outside, because the `catch` arm
needs them — both routing arms still work.
All messages use the existing wording, `"workflow execution is not
initialized"`.
### The fixes are pinned
Four new tests. Verified in both directions, with the production file
reverted and restored:
| | production reverted | with fixes |
|---|---|---|
| `WorkflowWebsocketResourceSpec` | **15 passed, 4 failed** | **19 passed, 0
failed** |
The before-state failures are the right ones, from the JUnit XML (sbt's only
reporter here is `-u`, so the console shows no per-test lines):
| new test | failure without the fix |
|---|---|
| runtime command with a workflow but no execution | `Expected
java.lang.IllegalStateException … java.lang.NullPointerException was thrown` |
| `ModifyLogicRequest` before any execution exists | `Expected
java.lang.IllegalStateException … java.lang.NullPointerException was thrown` |
| unparseable frame instead of escaping unmapped | `List() was not equal to
List("WorkflowErrorEvent")` |
| unparseable frame recorded in the metadata store | `List() was not equal
to List(COMPILATION_ERROR)` |
None of the 15 pre-existing tests regressed.
### Spec comments were updated, not just tests added
The spec's header paragraph on malformed frames, its "deliberately not
covered" entry for `ModifyLogicRequest`, and two in-test notes all documented
these as known-and-unpinned. Leaving them would have left the spec asserting
one thing and explaining the opposite, so they are rewritten to match.
Trap avoidance, all previously encountered in this file: no assertions on
`ClusterListener.numWorkerNodesInCluster` (its default is `0`, so such an
assertion passes even against a hard-coded literal); `PrivilegeEnum.WRITE` is
fed rather than the `NONE` default; and the new tests use
`TestWorkflowService`, which overrides `disconnect()`, so `afterEach` never
reaches the null `AmberRuntime._actorSystem` — the same pattern the existing
tests use.
### Verification
- `WorkflowWebsocketResourceSpec`: **19/19**.
- Blast radius: `TexeraWebSocketRequestSpec`, `SessionStateSpec`,
`ServletAwareConfiguratorSpec`, `WebsocketInputSpec` — 28/28 across 4 suites.
`TexeraWebSocketRequestSpec` pins `InvalidTypeIdException` at the mapper level,
which these changes leave untouched.
- `scalafmtCheck`, `Test/scalafmtCheck`, `scalafixAll --check` all pass.
- Production diff is 15 lines in one file.
### Any related issues, documentation, discussions?
Closes #7801
### How was this PR tested?
```
STORAGE_ICEBERG_CATALOG_TYPE=postgres sbt "WorkflowExecutionService/testOnly
org.apache.texera.web.resource.WorkflowWebsocketResourceSpec"
```
```
[info] Suites: completed 1, aborted 0
[info] Tests: succeeded 19, failed 0, canceled 0, ignored 0, pending 0
```
### Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Opus 5)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
