Add Default Workflow and Activity Commit Strategy Support (aborted due to force push on main)#7137
Add Default Workflow and Activity Commit Strategy Support (aborted due to force push on main)#7137dwoldo wants to merge 0 commit intoelsa-workflows:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds support for configuring optional global default commit strategies for both workflows and activities. The feature provides a fallback mechanism when no explicit commit strategy is configured at the workflow or activity level, allowing developers to set application-wide defaults while maintaining override capabilities.
Key changes:
- Added
DefaultWorkflowCommitStrategyandDefaultActivityCommitStrategyproperties toCommitStateOptionsto store global fallback strategies - Modified middleware components (
DefaultActivityInvokerMiddleware,DefaultActivitySchedulerMiddleware,BackgroundActivityInvokerMiddleware) to implement fallback logic that checks for default strategies when no explicit strategy is specified - Introduced fluent API extension methods (
WithDefaultWorkflowCommitStrategy,WithDefaultActivityCommitStrategy) for easy configuration
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
src/modules/Elsa.Workflows.Core/CommitStates/Options/CommitStateOptions.cs |
Added properties for default workflow and activity commit strategy instances with XML documentation |
src/modules/Elsa.Workflows.Core/CommitStates/CommitStrategiesFeature.cs |
Added SetDefaultWorkflowCommitStrategy and SetDefaultActivityCommitStrategy methods to configure global defaults |
src/modules/Elsa.Workflows.Core/CommitStates/Extensions/ModuleExtensions.cs |
Added fluent extension methods WithDefaultWorkflowCommitStrategy and WithDefaultActivityCommitStrategy for easy configuration |
src/modules/Elsa.Workflows.Core/Middleware/Activities/DefaultActivityInvokerMiddleware.cs |
Modified to check for default activity/workflow commit strategies when no explicit strategy is specified, implementing full resolution chain |
src/modules/Elsa.Workflows.Core/Middleware/Workflows/DefaultActivitySchedulerMiddleware.cs |
Modified to fall back to default workflow commit strategy when no explicit strategy is configured |
src/modules/Elsa.Workflows.Runtime/Middleware/Activities/BackgroundActivityInvokerMiddleware.cs |
Updated constructor to accept IOptions<CommitStateOptions> and pass it to base class |
src/modules/Elsa.Workflows.Core/CommitStates/USAGE_EXAMPLE.md |
Added usage documentation for workflow commit strategies (incomplete - missing activity strategy documentation) |
test/integration/Elsa.Workflows.IntegrationTests/Scenarios/DefaultWorkflowCommitStrategy/Tests.cs |
Added 5 integration tests validating default workflow commit strategy behavior |
test/integration/Elsa.Workflows.IntegrationTests/Scenarios/DefaultWorkflowCommitStrategy/SimpleWorkflowWithoutWorkflowCommitStrategy.cs |
Test workflow without explicit commit strategy |
test/integration/Elsa.Workflows.IntegrationTests/Scenarios/DefaultWorkflowCommitStrategy/WorkflowWithExplicitWorkflowCommitStrategy.cs |
Test workflow with explicit commit strategy to validate override behavior |
test/integration/Elsa.Workflows.IntegrationTests/Scenarios/DefaultWorkflowCommitStrategy/CommitTracker.cs |
Test helper to track commit invocations |
test/integration/Elsa.Workflows.IntegrationTests/Scenarios/DefaultActivityCommitStrategy/Tests.cs |
Added 6 integration tests validating default activity commit strategy behavior and fallback chain |
test/integration/Elsa.Workflows.IntegrationTests/Scenarios/DefaultActivityCommitStrategy/SimpleWorkflowWithoutActivityCommitStrategy.cs |
Test workflow without explicit activity commit strategy |
test/integration/Elsa.Workflows.IntegrationTests/Scenarios/DefaultActivityCommitStrategy/WorkflowWithExplicitActivityCommitStrategy.cs |
Test workflow with explicit activity commit strategy to validate override behavior |
test/integration/Elsa.Workflows.IntegrationTests/Scenarios/DefaultActivityCommitStrategy/CommitTracker.cs |
Test helper to track commit invocations (duplicate implementation) |
...integration/Elsa.Workflows.IntegrationTests/Scenarios/DefaultActivityCommitStrategy/Tests.cs
Outdated
Show resolved
Hide resolved
...integration/Elsa.Workflows.IntegrationTests/Scenarios/DefaultActivityCommitStrategy/Tests.cs
Outdated
Show resolved
Hide resolved
...ion/Elsa.Workflows.IntegrationTests/Scenarios/DefaultWorkflowCommitStrategy/CommitTracker.cs
Outdated
Show resolved
Hide resolved
src/modules/Elsa.Workflows.Core/Middleware/Activities/DefaultActivityInvokerMiddleware.cs
Outdated
Show resolved
Hide resolved
src/modules/Elsa.Workflows.Core/Middleware/Activities/DefaultActivityInvokerMiddleware.cs
Outdated
Show resolved
Hide resolved
src/modules/Elsa.Workflows.Core/Middleware/Workflows/DefaultActivitySchedulerMiddleware.cs
Outdated
Show resolved
Hide resolved
|
@KnibbsyMan Ready to review. |
|
|
||
| /// <summary> | ||
| /// Sets the specified activity commit strategy as the global default for all activities that do not specify their own strategy. | ||
| /// The strategy will be automatically registered if not already present. |
There was a problem hiding this comment.
The documentation states "The strategy will be automatically registered if not already present", but this is incorrect. The default strategy is intentionally NOT added to the registry - it only serves as a fallback. This is verified by the test DefaultStrategyNotInRegistry() which confirms the strategy is not visible in the registry. The documentation should state: "The strategy will not be added to the registry and serves only as a fallback."
| /// The strategy will be automatically registered if not already present. | |
| /// The strategy will not be added to the registry and serves only as a fallback. |
| } | ||
|
|
||
| [Fact(DisplayName = "Default workflow strategy is not visible in commit strategy registry")] | ||
| public async Task DefaultWorkflowStrategyNotInRegistry() |
There was a problem hiding this comment.
This test method is declared as async Task but doesn't contain any await statements. It should be changed to public void instead of public async Task for consistency and clarity. Compare with the similar test DefaultStrategyNotInRegistry() in DefaultActivityCommitStrategy/Tests.cs (line 115) which correctly uses public void.
| public async Task DefaultWorkflowStrategyNotInRegistry() | |
| public void DefaultWorkflowStrategyNotInRegistry() |
|
|
||
| /// <summary> | ||
| /// Sets the specified workflow commit strategy as the global default for all workflows that do not specify their own strategy. | ||
| /// The strategy will be automatically registered if not already present. |
There was a problem hiding this comment.
The documentation states "The strategy will be automatically registered if not already present", but this is incorrect. The default strategy is intentionally NOT added to the registry - it only serves as a fallback. This is verified by the test DefaultWorkflowStrategyNotInRegistry() which confirms the strategy is not visible in the registry. The documentation should state: "The strategy will not be added to the registry and serves only as a fallback."
| /// The strategy will be automatically registered if not already present. | |
| /// The strategy will not be added to the registry and serves only as a fallback. |
Add Default Workflow and Activity Commit Strategy Support
Summary
This PR introduces optional global default commit strategies for both workflows and activities, providing a fallback mechanism when no explicit commit strategy is configured. This feature allows developers to set application-wide defaults while still maintaining the ability to override them at the workflow or activity level.
Addresses #7135 #7135
Problem
Currently, if a workflow or activity doesn't specify a commit strategy, no commits occur during execution (except for the final commit). This requires developers to explicitly configure commit strategies for every workflow and activity, even when they want consistent behavior across their application.
Solution
Implemented two complementary features:
1. Default Workflow Commit Strategy
WithDefaultWorkflowCommitStrategy()extension method2. Default Activity Commit Strategy
WithDefaultActivityCommitStrategy()extension methodResolution Order
For Activities:
For Workflows:
Usage Example
Key Features
IWorkflowCommitStrategyandIActivityCommitStrategyinstances directlyChanges
Core Implementation
CommitStateOptions.cs: AddedDefaultWorkflowCommitStrategyandDefaultActivityCommitStrategypropertiesCommitStrategiesFeature.cs: AddedSetDefaultWorkflowCommitStrategy()andSetDefaultActivityCommitStrategy()configuration methodsModuleExtensions.cs: AddedWithDefaultWorkflowCommitStrategy()andWithDefaultActivityCommitStrategy()extension methodsDefaultActivitySchedulerMiddleware.cs: Modified to fall back to default workflow strategyDefaultActivityInvokerMiddleware.cs: Modified to implement full resolution chain with default strategiesTesting
DefaultWorkflowCommitStrategy/Tests.cs: 5 integration tests covering workflow-level defaultsDefaultActivityCommitStrategy/Tests.cs: 6 integration tests covering activity-level defaultsTest Coverage
Workflow Default Strategy Tests:
Activity Default Strategy Tests:
Breaking Changes
None. This is a purely additive feature that maintains backward compatibility.
Additional Notes
Sequence) trigger commit strategy evaluation when processing child activity completions, which is why some tests show 6 commits instead of 3 (3 WriteLine activities + 3 Sequence composite completions)Documentation
Usage examples and API documentation have been added via XML comments on all new public methods.
This change is