Guard sdk-java from NoSuchElementException - #1594
Conversation
Signed-off-by: Matheus Cruz <matheuscruz.dev@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #1593 by preventing workflow definition creation/execution from crashing when a workflow has an empty (or null) top-level task list.
Changes:
- Guard
TaskExecutorHelper.createExecutorList(...)againstnull/empty task lists to avoidNoSuchElementException. - Update workflow execution startup to gracefully handle a
nullstart task (i.e., “no tasks”) and complete with the (filtered) input. - Add unit tests asserting that an empty/null top-level task list results in a
nullstart task.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| impl/core/src/main/java/io/serverlessworkflow/impl/executors/TaskExecutorHelper.java | Adds an early-return guard for null/empty task lists to prevent iterator .next() crashes. |
| impl/core/src/main/java/io/serverlessworkflow/impl/WorkflowMutableInstance.java | Introduces a safe execution path when the workflow has no start task, completing immediately with the input. |
| impl/core/src/test/java/io/serverlessworkflow/impl/TaskExecutorHelperTest.java | Adds regression tests for empty and null top-level workflow task lists. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (taskItems == null || taskItems.isEmpty()) { | ||
| return null; | ||
| } |
| private CompletableFuture<WorkflowModel> processTaskList(WorkflowContext workflowContext) { | ||
| WorkflowModel inputModel = | ||
| workflowContext | ||
| .definition() | ||
| .inputFilter() | ||
| .map(f -> f.apply(workflowContext, null, input)) | ||
| .orElse(input); | ||
| TaskExecutor<?> startTask = workflowContext.definition().startTask(); | ||
| if (startTask == null) { | ||
| return CompletableFuture.completedFuture(inputModel); | ||
| } | ||
| return TaskExecutorHelper.processTaskList( | ||
| startTask, workflowContext, Optional.empty(), inputModel); | ||
| } |
Signed-off-by: Matheus Cruz <matheuscruz.dev@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (2)
impl/core/src/test/java/io/serverlessworkflow/impl/TaskExecutorHelperTest.java:55
- The fix relies on the runtime path where a workflow has no tasks (i.e.,
startTask()is null) completing successfully via the new null-guard inTaskExecutorHelper.processTaskList. The current tests only assertstartTask()is null; consider also asserting that starting the workflow completes as a no-op and returns the original input model, so this regression is covered end-to-end.
@Test
void emptyTaskListShouldProduceNullStartTask() {
Workflow workflow = new Workflow();
workflow.setDocument(new Document().withDsl("1.0.0").withName("empty").withVersion("0.1.0"));
workflow.setDo(Collections.emptyList());
WorkflowModelFactory modelFactory = Mockito.mock(WorkflowModelFactory.class);
try (WorkflowApplication app =
WorkflowApplication.builder().withModelFactory(modelFactory).build()) {
WorkflowDefinition definition = app.workflowDefinition(workflow);
assertThat(definition.startTask()).isNull();
}
}
impl/core/src/main/java/io/serverlessworkflow/impl/WorkflowMutableInstance.java:116
- The new helper method parameter
workflowContextshadows the instance field of the same name, which makes it easy to accidentally reference the wrong one in future edits. Renaming the parameter (e.g., tocontext) improves readability without changing behavior.
private CompletableFuture<WorkflowModel> processTaskList(WorkflowContext workflowContext) {
WorkflowModel inputModel =
workflowContext
.definition()
.inputFilter()
Signed-off-by: Matheus Cruz <matheuscruz.dev@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (2)
impl/core/src/test/java/io/serverlessworkflow/impl/TaskExecutorHelperTest.java:43
- Test name refers to a "TaskList", but the behavior under test is specifically a null workflow
dolist (i.e.,workflow.setDo(null)). Renaming improves clarity and avoids ambiguity about which list is being tested.
void nullTaskListShouldProduceNullStartTask() {
impl/core/src/test/java/io/serverlessworkflow/impl/TaskExecutorHelperTest.java:29
- Test name refers to a "TaskList", but the behavior under test is specifically an empty workflow
dolist (i.e.,workflow.setDo(Collections.emptyList())). Renaming improves clarity and makes it easier to correlate with the workflow property involved.
This issue also appears on line 43 of the same file.
void emptyTaskListShouldProduceNullStartTask() {
There was a problem hiding this comment.
Hmmmm, I need a yaml reproducer for this one. If there is not such reproducer, then the fix should be in the fluent dsl cpde, to prevent a not valid workflow definition to be generated rather that adding defensive checks in the implementation code
If, as I suspected, there is not way that the YAML file is validated with an empty task list, then the solution should be to do a check in the build method of the fluent DSL to prevent that. Or, in other word, there should be not workflow object created with empty task list. And it is is, it is perfectly ok for the implementation code to throw any unexpected exception, because the workflow object is not a valid one.
I prefer to throw an exception centralizing the validation and additionally to create an empty list when there is no task added while using the DSL. |
Im not fan of double validation (which is what you are doing in this PR, because the yaml is already validated) this "validation" should be done in the DSL. |
| @@ -117,6 +109,17 @@ protected final CompletableFuture<WorkflowModel> startExecution( | |||
| return future; | |||
| } | |||
|
|
|||
| private CompletableFuture<WorkflowModel> processTaskList(WorkflowContext workflowContext) { | |||
| WorkflowModel inputModel = | |||
| workflowContext | |||
| .definition() | |||
| .inputFilter() | |||
| .map(f -> f.apply(workflowContext, null, input)) | |||
| .orElse(input); | |||
| return TaskExecutorHelper.processTaskList( | |||
| workflowContext.definition().startTask(), workflowContext, Optional.empty(), inputModel); | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Is this really changing something or it is just a not needed refactor?
There was a problem hiding this comment.
I will remove this one in the next commit, I will add a validation at build method.
Many thanks for submitting your Pull Request ❤️!
What this PR does / why we need it:
Special notes for reviewers:
Additional information (if needed):
Closes #1593