[NO-ISSUE] Add wait to DSL#1294
Conversation
Signed-off-by: Matheus Cruz <matheuscruz.dev@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR adds first-class wait task support to the Serverless Workflow Java Fluent API/DSL, along with runtime and test coverage, and fixes a duration-aggregation bug in the wait executor.
Changes:
- Introduces
WaitTaskBuilderand fluent interfaces to addwaittasks (including auto-naming support). - Adds DSL helper methods for constructing wait tasks from duration expressions or timeout builders.
- Fixes
WaitExecutorduration composition and adds new unit/integration tests for wait behavior.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| impl/test/src/test/java/io/serverlessworkflow/impl/test/WaitTest.java | New runtime/integration-style tests validating that workflows actually pause for a duration. |
| impl/test/pom.xml | Adds Awaitility dependency for timing assertions (currently duplicated). |
| impl/core/src/main/java/io/serverlessworkflow/impl/executors/WaitExecutor.java | Fixes duration accumulation to correctly add seconds/minutes/hours/days. |
| fluent/spec/src/test/java/io/serverlessworkflow/fluent/spec/TaskItemDefaultNamingTest.java | Adds coverage for wait task auto-naming and inline-duration mapping. |
| fluent/spec/src/test/java/io/serverlessworkflow/fluent/spec/dsl/DSLTest.java | Adds DSL helper coverage for wait tasks. |
| fluent/spec/src/main/java/io/serverlessworkflow/fluent/spec/WaitTaskBuilder.java | New builder for configuring wait tasks via expression, Duration, or TimeoutBuilder. |
| fluent/spec/src/main/java/io/serverlessworkflow/fluent/spec/TaskItemListBuilder.java | Adds fluent wait(...) task insertion into task lists. |
| fluent/spec/src/main/java/io/serverlessworkflow/fluent/spec/spi/WaitFluent.java | New SPI fluent interface for wait(...) task addition. |
| fluent/spec/src/main/java/io/serverlessworkflow/fluent/spec/spi/DoFluent.java | Extends the fluent task API to include wait(...) (SPI surface change). |
| fluent/spec/src/main/java/io/serverlessworkflow/fluent/spec/dsl/DSL.java | Adds DSL helper overloads for building wait tasks. |
| fluent/spec/src/main/java/io/serverlessworkflow/fluent/spec/DoTaskBuilder.java | Wires wait(...) into the top-level do builder. |
| fluent/spec/src/main/java/io/serverlessworkflow/fluent/spec/BaseTaskItemListBuilder.java | Adds TYPE_WAIT for deterministic auto-naming. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
impl/test/src/test/java/io/serverlessworkflow/impl/test/WaitTest.java
Outdated
Show resolved
Hide resolved
impl/test/src/test/java/io/serverlessworkflow/impl/test/WaitTest.java
Outdated
Show resolved
Hide resolved
fluent/spec/src/main/java/io/serverlessworkflow/fluent/spec/spi/DoFluent.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Wait functionality is already covered here
What I think should be done is:
- delete this file (which will susbtantially increase the time running test in case of failure and I made a huge effort to keep the mvn clean install time short)
- In LifeCycleEventTest class, to generate, using DSL, a workflow equivalen to wait-set.yaml.
- Refactor the code to run the same test with the workflow loaded from the yaml and the workflow generated with the fluent
There was a problem hiding this comment.
The same approach should be taken for fork-wait.yaml (which will also verify fork dsl is working)
See https://github.com/serverlessworkflow/sdk-java/blob/main/impl/test/src/test/java/io/serverlessworkflow/impl/test/ForkWaitTest.java#L52-L76
There was a problem hiding this comment.
Note that the two test mentioned above, indirectly verify that wait is working without explicit testing, so the test suite still run fast
The target of this PR should be to ensure that we can generate the same type of workflow using fluent that we are generating writing YAML
impl/core/src/main/java/io/serverlessworkflow/impl/executors/WaitExecutor.java
Show resolved
Hide resolved
Signed-off-by: Matheus Cruz <matheuscruz.dev@gmail.com>
Signed-off-by: Matheus Cruz <matheuscruz.dev@gmail.com>
|
cc: @domhanak |
| private static WorkflowInstance waitSetInstance(WorkflowSource source) throws IOException { | ||
| return switch (source) { | ||
| case DSL -> appl.workflowDefinition(waitTestWorkflow()).instance(Map.of()); | ||
| case YAML -> | ||
| appl.workflowDefinition( | ||
| WorkflowReader.readWorkflowFromClasspath("workflows-samples/wait-set.yaml")) | ||
| .instance(Map.of()); | ||
| }; | ||
| } |
There was a problem hiding this comment.
hmmmm,there should be a different way to do this, the only thing that change here is the Workflow object
| if (source == WorkflowSource.DSL) { | ||
| return forkWaitWorkflow(); | ||
| } | ||
| return readWorkflowFromClasspath("workflows-samples/fork-wait.yaml"); |
There was a problem hiding this comment.
| if (source == WorkflowSource.DSL) { | |
| return forkWaitWorkflow(); | |
| } | |
| return readWorkflowFromClasspath("workflows-samples/fork-wait.yaml"); | |
| return source == WorkflowSource.DSL ? forkWaitWorkflow() : readWorkflowFromClasspath("workflows-samples/fork-wait.yaml"); |
Signed-off-by: Matheus Cruz <matheuscruz.dev@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| final TimeoutBuilder timeoutBuilder = new TimeoutBuilder(); | ||
| waitConsumer.accept(timeoutBuilder); | ||
| this.waitTask.setWait(timeoutBuilder.build().getAfter()); | ||
| return this; | ||
| } | ||
|
|
There was a problem hiding this comment.
WaitTaskBuilder.wait(Consumer) does not null-check the consumer and does not validate that the provided TimeoutBuilder actually sets a duration (inline or expression). As written, callers can build a wait task with wait=null or with a TimeoutAfter missing both durationInline and durationExpression, which will later cause an NPE in WaitExecutor when it calls Duration.parse(...). Consider requiring a non-null consumer and throwing an IllegalStateException when no duration is configured.
| final TimeoutBuilder timeoutBuilder = new TimeoutBuilder(); | |
| waitConsumer.accept(timeoutBuilder); | |
| this.waitTask.setWait(timeoutBuilder.build().getAfter()); | |
| return this; | |
| } | |
| Objects.requireNonNull(waitConsumer, "waitConsumer must not be null"); | |
| final TimeoutBuilder timeoutBuilder = new TimeoutBuilder(); | |
| waitConsumer.accept(timeoutBuilder); | |
| final TimeoutAfter after = timeoutBuilder.build().getAfter(); | |
| validateTimeoutAfter(after); | |
| this.waitTask.setWait(after); | |
| return this; | |
| } | |
| private static void validateTimeoutAfter(TimeoutAfter after) { | |
| if (after == null | |
| || (after.getDurationInline() == null && after.getDurationExpression() == null)) { | |
| throw new IllegalStateException( | |
| "wait timeout must configure either durationInline or durationExpression"); | |
| } | |
| } |
| return this; | ||
| } | ||
|
|
||
| public WaitTaskBuilder wait(String durationExpression) { |
There was a problem hiding this comment.
WaitTaskBuilder.wait(String durationExpression) allows null/blank input, which can build an invalid wait task and later trigger an NPE at runtime when WaitExecutor parses the duration. Consider validating durationExpression is non-null/non-blank (and possibly giving a clearer error message for invalid ISO-8601 durations).
| public WaitTaskBuilder wait(String durationExpression) { | |
| public WaitTaskBuilder wait(String durationExpression) { | |
| Objects.requireNonNull(durationExpression, "durationExpression must not be null"); | |
| if (durationExpression.isBlank()) { | |
| throw new IllegalArgumentException("durationExpression must not be blank"); | |
| } |
|
|
||
| private enum WorkflowSource { | ||
| DSL, | ||
| YAML | ||
| } |
There was a problem hiding this comment.
The WorkflowSource enum is unused in this test class. Consider removing it to avoid dead code and keep the test focused on the actual workflow sources provided by waitSetWorkflowSources().
| private enum WorkflowSource { | |
| DSL, | |
| YAML | |
| } |
| private static Workflow waitTestWorkflow() { | ||
| return WorkflowBuilder.workflow("wait-test-java-dsl", "test", "0.1.0") | ||
| .tasks( | ||
| // wait 500 ms | ||
| DSL.wait( | ||
| "waitABit", | ||
| timeoutBuilder -> | ||
| timeoutBuilder.duration(durationBuilder -> durationBuilder.milliseconds(500))), | ||
| DSL.set("useExpression", setTaskBuilder -> setTaskBuilder.put("name", "Javierito"))) | ||
| .build(); |
There was a problem hiding this comment.
The PR description mentions adding a new integration test impl/test/src/test/java/io/serverlessworkflow/impl/test/WaitTest.java, but that file is not present in the change set; instead, wait behavior seems covered via the new DSL-built workflow in this class and parameterized tests. Please update the PR description to match the actual tests being added/modified (or add the missing test if it was intended).
| SELF branches(Consumer<L> branchesConsumer); | ||
|
|
||
| default SELF branch(Consumer<L> branchConsumer) { | ||
| return branch(null, branchConsumer); | ||
| } | ||
|
|
||
| SELF branch(String name, Consumer<L> branchConsumer); | ||
|
|
There was a problem hiding this comment.
This PR introduces a new ForkTaskFluent.branch(...) / ForkTaskBuilder.branch(...) API (wrapping branch tasks in a DoTask) in addition to the advertised wait-task DSL work. Please call this out in the PR description/release notes, since it expands the public fluent API beyond wait support.
Signed-off-by: Matheus Cruz <matheuscruz.dev@gmail.com>
Signed-off-by: Matheus Cruz <matheuscruz.dev@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (name == null || name.isBlank()) { | ||
| name = "branch-" + this.items.size(); | ||
| } | ||
| final FuncTaskItemListBuilder branchItems = new FuncTaskItemListBuilder(this.items); |
There was a problem hiding this comment.
The new branch(…, Consumer<…>) implementation ignores the provided branchConsumer and builds the branch's DoTask from a FuncTaskItemListBuilder backed by this.items. Because branchItems.build() returns an unmodifiable view of the same underlying list, the created branch will effectively reference the fork’s full branches list (and can even end up self-referential once the new TaskItem is added), producing an invalid workflow structure. Create a fresh FuncTaskItemListBuilder for the branch body (not backed by this.items), invoke branchConsumer.accept(...) to populate it, then wrap that built list in the DoTask before appending the branch TaskItem to this.items.
| final FuncTaskItemListBuilder branchItems = new FuncTaskItemListBuilder(this.items); | |
| final FuncTaskItemListBuilder branchItems = new FuncTaskItemListBuilder(this.items.size()); | |
| if (branchConsumer != null) { | |
| branchConsumer.accept(branchItems); | |
| } |
impl/core/src/main/java/io/serverlessworkflow/impl/executors/WaitExecutor.java
Show resolved
Hide resolved
Signed-off-by: Matheus Cruz <matheuscruz.dev@gmail.com>
This pull request introduces a new "wait" task type to the Serverless Workflow Java Fluent API, enabling workflows to pause execution for a specified duration.
New "wait" task support:
WaitTaskBuilderfor configuring wait tasks with duration expressions, inline durations, or timeout builders. (fluent/spec/src/main/java/io/serverlessworkflow/fluent/spec/WaitTaskBuilder.java)TaskItemListBuilder,DoTaskBuilder,DoFluent, and newWaitFluent) to support adding wait tasks fluently, including auto-naming and configuration. (fluent/spec/src/main/java/io/serverlessworkflow/fluent/spec/TaskItemListBuilder.java,DoTaskBuilder.java,spi/DoFluent.java,spi/WaitFluent.java) [1] [2] [3] [4]fluent/spec/src/main/java/io/serverlessworkflow/fluent/spec/dsl/DSL.java)Testing and validation:
TaskItemDefaultNamingTest.java,DSLTest.java) [1] [2]impl/test/src/test/java/io/serverlessworkflow/impl/test/WaitTest.java)Bug fix:
WaitExecutorwhere duration components were not being added correctly, ensuring accurate wait times. (impl/core/src/main/java/io/serverlessworkflow/impl/executors/WaitExecutor.java)Dependencies:
impl/test/pom.xml)