Skip to content

Conversation

@arminhammer
Copy link
Contributor

@arminhammer arminhammer commented Nov 11, 2025

What this PR does / why we need it:
This PR proposes fixes for two issues that I have run into that I would like to see addressed:

Issue 1:

In the official Serverless Workflows spec, it is possible to define Do and For type tasks. Both of these tasks have the do keyword in their definition. The example in the reference is:

document:
  dsl: '1.0.2'
  namespace: test
  name: for-example
  version: '0.1.0'
do:
  - checkup:
      for:
        each: pet
        in: .pets
        at: index
      while: .vet != null
      do:
        - waitForCheckup:
            listen:
              to:
                one:
                  with:
                    type: com.fake.petclinic.pets.checkup.completed.v2
            output:
              as: '.pets + [{ "id": $pet.id }]'  

Unfortunately the TaskDefinition enum in task.rs uses #[serde(untagged)], which causes issues when deserializing tasks that have overlapping fields, such as 'do' and 'for'.
Serde will attempt to deserialize into each variant in order, and since both 'do' and 'for' tasks contain a 'do' field, this can lead to incorrect deserialization.

/// Represents a value that can be any of the supported task definitions
#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
#[serde(untagged)]
pub enum TaskDefinition{
    ...
    /// Variant holding the definition of a 'do' task
    Do(DoTaskDefinition),
    /// Variant holding the definition of an 'emit' task
    Emit(EmitTaskDefinition),
    /// Variant holding the definition of a 'for' task
    For(ForTaskDefinition),
    ...
}

In the current code, because Do comes before For in the enum, a ForTaskDefinition will always be deserialized to a DoTaskDefinition, because Do it comes first in the list.

This PR adds a custom Deserializer for TaskDefinition, which can handle the issue between For and Do. It first checks to see whether for exists in the task definition, and matches a ForTaskDefinition if that is the case. The do case is placed at the end, so that collisions are avoided. Other task types should remain unaffected.

I added unit tests to test that the fix works, and that the custom validator fixes the issue.

Issue 2:

There is a small bug in the ForLoopDefinition in task.rs:

    #[serde(rename = "emit")]
    pub each: String,

"emit" should be "each". This typo causes deserialization to fail. This PR fixes the issue and the unit test test_for_loop_definition_each_field_deserialization validates the bug and the fix.

@arminhammer arminhammer force-pushed the task-definition-order-patch branch from 5e2b45e to f683890 Compare November 11, 2025 23:45
…ion bug

This commit addresses two issues:

1. Fixed TaskDefinition deserialization conflict between Do and For tasks
   - Added custom deserializer to handle overlapping 'do' field
   - For tasks are now correctly identified by the presence of 'for' field
   - Prevents ForTaskDefinition from being incorrectly deserialized as DoTaskDefinition

2. Fixed ForLoopDefinition serde rename attribute
   - Changed rename from "emit" to "each" on the each field
   - Added unit test to validate the fix

Signed-off-by: Armin Graf <[email protected]>
@arminhammer arminhammer force-pushed the task-definition-order-patch branch from f683890 to bdc8f31 Compare November 11, 2025 23:48
Copy link
Member

@cdavernas cdavernas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great to me, thank you ❤️

@cdavernas cdavernas merged commit e2cf517 into serverlessworkflow:main Nov 12, 2025
3 checks passed
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.

2 participants