From 0422435be9ac40912fa0f965b6cd1cc56bb922e7 Mon Sep 17 00:00:00 2001 From: Sipke Schoorstra Date: Mon, 10 Jun 2024 10:51:23 +0200 Subject: [PATCH] Fix workflow variable scope inconsistency (#5558) * Refactor variable handling in ExpressionExecutionContextExtensions The core change in this commit is the refactoring of the handling of variables within the ExpressionExecutionContextExtensions. The methods GetVariable, CreateVariable, and GetVariableBlock have been altered for clarity and simplified reducing redundant code. Unnecessary parameters and returns in method documentation have been removed, and overall code formatting has been improved to enhance readability. * Simplify MemoryRegister creation in Workflow.cs The creation of the MemoryRegister object in the file Workflow.cs was simplified to one line. The previous method, which declared a new object then called the Declare method before returning, was removed. * Map controller routes in server web program Added a line of code in the Elsa.Server.Web program.cs file to map controller routes. This change ensures that HTTP requests are correctly directed to their corresponding controller actions. * Update workflow state extraction logic The logic in the WorkflowStateExtractor has been updated to retain the root Workflow activity context even if it's completed. This change is necessary to keep workflow-level variables accessible. * Remove RequiresUnreferencedCode attribute from ConvertTo method The RequiresUnreferencedCode attribute was removed from the ConvertTo method in the ObjectConverter class. * Remove unused services and rename test file Unused services in the AutoUpdateTests.cs class were removed, reducing clutter and improving code readability. Additionally, the DeleteWorkflow_Clustered.cs test file has been renamed to DeleteWorkflowClustered.cs for better naming consistency. * Add CountdownStep activity and CountdownWorkflow for testing This commit introduces new component tests for simulations involving counters. It includes a new CountdownStep activity that decrements a counter variable, as well as a CountdownWorkflow which consists of a loop based on the aforementioned activity. It also involves a CountdownWorkflowTests class for testing counter persistence across workflow runs. * Remove unnecessary whitespace in CountdownWorkflowTests This commit eliminates the superfluous whitespace in the CountdownWorkflowTests.cs file. It maintains the proper formatting and ensures code consistency across the test component. * Refactor CountdownWorkflowTests constructor Simplified the constructor of the CountdownWorkflowTests class. The changes remove the unnecessary constructor body and pass the 'app' object directly to the base AppComponentTest class, enhancing the code's readability and maintainability. * Add application roles and configure them in MassTransit A new enum ApplicationRole has been added for distinguishing among different roles (Hybrid, Api, Worker) an application can take. In the configuration of MassTransit, it is now possible to disable the consumers based on application role, which can help optimize the usage of resources and increase application efficiency. * Update Program.cs Switch to Memory broker * Update Program.cs Simplify DisableConsumers assignment. * Update ApplicationRole.cs Rename Hybrid to Default. * Update appsettings.json --- .../Elsa.Server.Web/Enums/ApplicationRole.cs | 4 +- src/bundles/Elsa.Server.Web/Program.cs | 7 +-- src/bundles/Elsa.Server.Web/appsettings.json | 4 +- .../Helpers/ObjectConverter.cs | 1 - .../Activities/Workflow.cs | 4 +- .../ExpressionExecutionContextExtensions.cs | 35 ++++++------ .../Services/WorkflowStateExtractor.cs | 4 +- .../Variables/Activities/CountdownStep.cs | 13 +++++ .../Variables/CountdownWorkflowTests.cs | 53 +++++++++++++++++++ .../Variables/Workflows/CountdownWorkflow.cs | 35 ++++++++++++ .../WorkflowActivities/AutoUpdateTests.cs | 8 --- ...lustered.cs => DeleteWorkflowClustered.cs} | 0 12 files changed, 131 insertions(+), 37 deletions(-) create mode 100644 test/component/Elsa.Workflows.ComponentTests/Scenarios/Variables/Activities/CountdownStep.cs create mode 100644 test/component/Elsa.Workflows.ComponentTests/Scenarios/Variables/CountdownWorkflowTests.cs create mode 100644 test/component/Elsa.Workflows.ComponentTests/Scenarios/Variables/Workflows/CountdownWorkflow.cs rename test/component/Elsa.Workflows.ComponentTests/Scenarios/WorkflowActivities/Workflows/{DeleteWorkflow_Clustered.cs => DeleteWorkflowClustered.cs} (100%) diff --git a/src/bundles/Elsa.Server.Web/Enums/ApplicationRole.cs b/src/bundles/Elsa.Server.Web/Enums/ApplicationRole.cs index 60cfbe097..6eac67b27 100644 --- a/src/bundles/Elsa.Server.Web/Enums/ApplicationRole.cs +++ b/src/bundles/Elsa.Server.Web/Enums/ApplicationRole.cs @@ -2,7 +2,7 @@ namespace Elsa.Server.Web; public enum ApplicationRole { - Hybrid, + Default, Api, Worker -} \ No newline at end of file +} diff --git a/src/bundles/Elsa.Server.Web/Program.cs b/src/bundles/Elsa.Server.Web/Program.cs index 87df5a0f3..dd1c5475b 100644 --- a/src/bundles/Elsa.Server.Web/Program.cs +++ b/src/bundles/Elsa.Server.Web/Program.cs @@ -326,8 +326,7 @@ services { elsa.UseMassTransit(massTransit => { - if (appRole == ApplicationRole.Api) - massTransit.DisableConsumers = true; + massTransit.DisableConsumers = appRole == ApplicationRole.Api; if (useMassTransitBroker == MassTransitBroker.AzureServiceBus) { @@ -410,6 +409,8 @@ app.UseJsonSerializationErrorHandler(); // Elsa HTTP Endpoint activities. app.UseWorkflows(); +app.MapControllers(); + // Swagger API documentation. if (app.Environment.IsDevelopment()) { @@ -432,4 +433,4 @@ public partial class Program /// Set by the test runner to configure the module for testing. /// public static Action? ConfigureForTest { get; set; } -} \ No newline at end of file +} diff --git a/src/bundles/Elsa.Server.Web/appsettings.json b/src/bundles/Elsa.Server.Web/appsettings.json index 247a862e5..cf7b67665 100644 --- a/src/bundles/Elsa.Server.Web/appsettings.json +++ b/src/bundles/Elsa.Server.Web/appsettings.json @@ -96,7 +96,7 @@ } ] }, - "AppRole": "Hybrid", + "AppRole": "Default", "Runtime": { "WorkflowInboxCleanup": { "SweepInterval": "00:00:10:00", @@ -126,4 +126,4 @@ ] } } -} \ No newline at end of file +} diff --git a/src/modules/Elsa.Expressions/Helpers/ObjectConverter.cs b/src/modules/Elsa.Expressions/Helpers/ObjectConverter.cs index c526fc4b0..571397bee 100644 --- a/src/modules/Elsa.Expressions/Helpers/ObjectConverter.cs +++ b/src/modules/Elsa.Expressions/Helpers/ObjectConverter.cs @@ -51,7 +51,6 @@ public static class ObjectConverter /// /// Attempts to convert the source value into the destination type. /// - [RequiresUnreferencedCode("The JsonSerializer type is not trim-compatible.")] public static T? ConvertTo(this object? value, ObjectConverterOptions? converterOptions = null) => value != null ? (T?)value.ConvertTo(typeof(T), converterOptions) : default; private static JsonSerializerOptions? _defaultSerializerOptions; diff --git a/src/modules/Elsa.Workflows.Core/Activities/Workflow.cs b/src/modules/Elsa.Workflows.Core/Activities/Workflow.cs index f85eae7d8..1116beb80 100644 --- a/src/modules/Elsa.Workflows.Core/Activities/Workflow.cs +++ b/src/modules/Elsa.Workflows.Core/Activities/Workflow.cs @@ -117,9 +117,7 @@ public class Workflow : Composite, ICloneable /// public MemoryRegister CreateRegister() { - var register = new MemoryRegister(); - register.Declare(Variables); - return register; + return new MemoryRegister(); } /// diff --git a/src/modules/Elsa.Workflows.Core/Extensions/ExpressionExecutionContextExtensions.cs b/src/modules/Elsa.Workflows.Core/Extensions/ExpressionExecutionContextExtensions.cs index 4ec3e1f0c..07f0422b1 100644 --- a/src/modules/Elsa.Workflows.Core/Extensions/ExpressionExecutionContextExtensions.cs +++ b/src/modules/Elsa.Workflows.Core/Extensions/ExpressionExecutionContextExtensions.cs @@ -66,8 +66,7 @@ public static class ExpressionExecutionContextExtensions /// /// Returns the of the specified /// - public static bool TryGetWorkflowExecutionContext(this ExpressionExecutionContext context, out WorkflowExecutionContext workflowExecutionContext) => - context.TransientProperties.TryGetValue(WorkflowExecutionContextKey, out workflowExecutionContext!); + public static bool TryGetWorkflowExecutionContext(this ExpressionExecutionContext context, out WorkflowExecutionContext workflowExecutionContext) => context.TransientProperties.TryGetValue(WorkflowExecutionContextKey, out workflowExecutionContext!); /// /// Returns the of the specified @@ -77,16 +76,11 @@ public static class ExpressionExecutionContextExtensions /// /// Returns the of the specified /// - /// - /// public static ActivityExecutionContext GetActivityExecutionContext(this ExpressionExecutionContext context) => (ActivityExecutionContext)context.TransientProperties[ActivityExecutionContextKey]; /// /// Returns the of the specified /// - /// - /// - /// public static bool TryGetActivityExecutionContext(this ExpressionExecutionContext context, out ActivityExecutionContext activityExecutionContext) => context.TransientProperties.TryGetValue(ActivityExecutionContextKey, out activityExecutionContext!); /// @@ -104,31 +98,41 @@ public static class ExpressionExecutionContextExtensions /// public static object? Get(this ExpressionExecutionContext context, Output output) => context.GetBlock(output.MemoryBlockReference).Value; - /// /// Returns the value of the variable with the specified name. /// - public static T? GetVariable(this ExpressionExecutionContext context, string name) => (T?)context.GetVariable(name)?.Value; + public static T? GetVariable(this ExpressionExecutionContext context, string name) + { + var block = context.GetVariableBlock(name); + return (T?)block?.Value; + } /// /// Returns the variable with the specified name. /// public static Variable? GetVariable(this ExpressionExecutionContext context, string name, bool localScopeOnly = false) + { + var block = context.GetVariableBlock(name, localScopeOnly); + return block?.Metadata is VariableBlockMetadata metadata ? metadata.Variable : default; + } + + private static MemoryBlock? GetVariableBlock(this ExpressionExecutionContext context, string name, bool localScopeOnly = false) { foreach (var block in context.Memory.Blocks.Where(b => b.Value.Metadata is VariableBlockMetadata)) { var metadata = block.Value.Metadata as VariableBlockMetadata; if (metadata!.Variable.Name == name) - return metadata.Variable; + return block.Value; } - return localScopeOnly ? null : context.ParentContext?.GetVariable(name); + return localScopeOnly ? null : context.ParentContext?.GetVariableBlock(name); } /// /// Creates a named variable in the context. /// - public static Variable CreateVariable(this ExpressionExecutionContext context, string name, T? value, Type? storageDriverType = null, Action? configure = default) + public static Variable CreateVariable(this ExpressionExecutionContext context, string name, T? value, Type? storageDriverType = null, + Action? configure = default) { var existingVariable = context.GetVariable(name, localScopeOnly: true); @@ -174,7 +178,6 @@ public static class ExpressionExecutionContextExtensions var contextWithVariable = context.FindContextContainingBlock(variable.Id) ?? context; // Set the value on the variable. - variable.Value = value; variable.Set(contextWithVariable, value, configure); // Return the variable. @@ -354,14 +357,14 @@ public static class ExpressionExecutionContextExtensions { return context.GetInput(inputDefinition.Name); } - + private static JsonSerializerOptions? _serializerOptions; private static JsonSerializerOptions GetSerializerOptions(ExpressionExecutionContext context) { - if(_serializerOptions != null) + if (_serializerOptions != null) return _serializerOptions; - + var serializerOptions = context.GetRequiredService().GetOptions().Clone(); serializerOptions.ReferenceHandler = ReferenceHandler.Preserve; _serializerOptions = serializerOptions; diff --git a/src/modules/Elsa.Workflows.Core/Services/WorkflowStateExtractor.cs b/src/modules/Elsa.Workflows.Core/Services/WorkflowStateExtractor.cs index b321cc0fd..009079217 100644 --- a/src/modules/Elsa.Workflows.Core/Services/WorkflowStateExtractor.cs +++ b/src/modules/Elsa.Workflows.Core/Services/WorkflowStateExtractor.cs @@ -260,9 +260,9 @@ public class WorkflowStateExtractor : IWorkflowStateExtractor private static IEnumerable GetActiveActivityExecutionContexts(IEnumerable activityExecutionContexts) { - // Filter out completed activity execution contexts. + // Filter out completed activity execution contexts, except for the root Workflow activity context, which stores workflow-level variables. // This will currently break scripts accessing activity output directly, but there's a workaround for that via variable capturing. // We may ultimately restore direct output access, but in a different way. - return activityExecutionContexts.Where(x => !x.IsCompleted).ToList(); + return activityExecutionContexts.Where(x => !x.IsCompleted || x.ParentActivityExecutionContext == null).ToList(); } } \ No newline at end of file diff --git a/test/component/Elsa.Workflows.ComponentTests/Scenarios/Variables/Activities/CountdownStep.cs b/test/component/Elsa.Workflows.ComponentTests/Scenarios/Variables/Activities/CountdownStep.cs new file mode 100644 index 000000000..50df28340 --- /dev/null +++ b/test/component/Elsa.Workflows.ComponentTests/Scenarios/Variables/Activities/CountdownStep.cs @@ -0,0 +1,13 @@ +using Elsa.Extensions; + +namespace Elsa.Workflows.ComponentTests.Scenarios.Variables.Activities; + +public class CountdownStep : Activity +{ + protected override void Execute(ActivityExecutionContext context) + { + var counter = context.GetVariable("Counter"); + context.SetVariable("Counter", counter - 1); + context?.CreateBookmark(); + } +} \ No newline at end of file diff --git a/test/component/Elsa.Workflows.ComponentTests/Scenarios/Variables/CountdownWorkflowTests.cs b/test/component/Elsa.Workflows.ComponentTests/Scenarios/Variables/CountdownWorkflowTests.cs new file mode 100644 index 000000000..b0f3f7b9b --- /dev/null +++ b/test/component/Elsa.Workflows.ComponentTests/Scenarios/Variables/CountdownWorkflowTests.cs @@ -0,0 +1,53 @@ +using Elsa.Expressions.Helpers; +using Elsa.Extensions; +using Elsa.Workflows.ComponentTests.Scenarios.Variables.Workflows; +using Elsa.Workflows.Management.Contracts; +using Elsa.Workflows.Models; +using Elsa.Workflows.Runtime.Contracts; +using Elsa.Workflows.Runtime.Parameters; +using Elsa.Workflows.Services; +using Elsa.Workflows.State; +using Microsoft.Extensions.DependencyInjection; + +namespace Elsa.Workflows.ComponentTests.Scenarios.Variables; + +public class CountdownWorkflowTests(App app) : AppComponentTest(app) +{ + [Fact(DisplayName = "Variable is persisted across workflow runs")] + public async Task VariableIsPersistedAcrossWorkflowRuns() + { + var workflowRuntime = Scope.ServiceProvider.GetRequiredService(); + var workflowInstanceStore = Scope.ServiceProvider.GetRequiredService(); + var startParams = new StartWorkflowRuntimeParams(); + var result = await workflowRuntime.StartWorkflowAsync(CountdownWorkflow.DefinitionId, startParams); + var workflowInstanceId = result.WorkflowInstanceId; + var bookmarks = new Stack(result.Bookmarks); + var expectedCounter = 3; + + while (bookmarks.Any()) + { + var workflowInstance = await workflowInstanceStore.FindAsync(workflowInstanceId); + var workflowState = workflowInstance!.WorkflowState; + var rootWorkflowActivityExecutionContext = workflowState.ActivityExecutionContexts.Single(x => x.ParentContextId == null); + var variables = GetVariablesDictionary(rootWorkflowActivityExecutionContext); + var actualCounter = variables["Workflow1:variable-1"].ConvertTo(); + Assert.Equal(--expectedCounter, actualCounter); + + var bookmark = bookmarks.Pop(); + var resumeWorkflowRuntimeOptions = new ResumeWorkflowRuntimeParams + { + BookmarkId = bookmark?.Id, + }; + + result = await workflowRuntime.ResumeWorkflowAsync(workflowInstanceId, resumeWorkflowRuntimeOptions); + + if (result == null) + break; + + foreach (var newBookmark in result.Bookmarks) bookmarks.Push(newBookmark); + } + } + + private IDictionary GetVariablesDictionary(ActivityExecutionContextState context) => + context.Properties.GetOrAdd(WorkflowStorageDriver.VariablesDictionaryStateKey, () => new Dictionary()); +} \ No newline at end of file diff --git a/test/component/Elsa.Workflows.ComponentTests/Scenarios/Variables/Workflows/CountdownWorkflow.cs b/test/component/Elsa.Workflows.ComponentTests/Scenarios/Variables/Workflows/CountdownWorkflow.cs new file mode 100644 index 000000000..53bf6a05b --- /dev/null +++ b/test/component/Elsa.Workflows.ComponentTests/Scenarios/Variables/Workflows/CountdownWorkflow.cs @@ -0,0 +1,35 @@ +using Elsa.Extensions; +using Elsa.Workflows.Activities; +using Elsa.Workflows.ComponentTests.Scenarios.Variables.Activities; +using Elsa.Workflows.Contracts; + +namespace Elsa.Workflows.ComponentTests.Scenarios.Variables.Workflows; + +public class CountdownWorkflow : WorkflowBase +{ + public static readonly string DefinitionId = Guid.NewGuid().ToString(); + + protected override void Build(IWorkflowBuilder builder) + { + builder.WithDefinitionId(DefinitionId); + var counter = builder.WithVariable("Counter", 3).WithWorkflowStorage(); + + builder.Root = new Sequence + { + Activities = + { + new While(context => counter.Get(context) > 0) + { + Body = new Sequence + { + Activities = + { + new WriteLine(context => $"Counter: {counter.Get(context)}"), + new CountdownStep() + } + } + } + } + }; + } +} \ No newline at end of file diff --git a/test/component/Elsa.Workflows.ComponentTests/Scenarios/WorkflowActivities/AutoUpdateTests.cs b/test/component/Elsa.Workflows.ComponentTests/Scenarios/WorkflowActivities/AutoUpdateTests.cs index 8276dd1ff..ef173b921 100644 --- a/test/component/Elsa.Workflows.ComponentTests/Scenarios/WorkflowActivities/AutoUpdateTests.cs +++ b/test/component/Elsa.Workflows.ComponentTests/Scenarios/WorkflowActivities/AutoUpdateTests.cs @@ -1,6 +1,3 @@ -using Elsa.Common.Models; -using Elsa.Http; -using Elsa.Http.Bookmarks; using Elsa.Http.Contracts; using Elsa.Workflows.Contracts; using Elsa.Workflows.Management.Contracts; @@ -18,9 +15,6 @@ public class AutoUpdateTests : AppComponentTest private readonly IWorkflowDefinitionPublisher _publisher; private readonly ISignalManager _signalManager; private readonly ITriggerChangeTokenSignalEvents _changeTokenEvents; - private readonly IWorkflowDefinitionManager _definitionManager; - private readonly IWorkflowDefinitionService _workflowDefinitionService; - private readonly IHttpWorkflowsCacheManager _httpCacheManager; private readonly IWorkflowDefinitionCacheManager _workflowCacheManager; @@ -42,8 +36,6 @@ public class AutoUpdateTests : AppComponentTest _hasher = Scope.ServiceProvider.GetRequiredService(); _definitionCacheManager = Scope.ServiceProvider.GetRequiredService(); _publisher = Scope.ServiceProvider.GetRequiredService(); - _definitionManager = Scope.ServiceProvider.GetRequiredService(); - _workflowDefinitionService = Scope.ServiceProvider.GetRequiredService(); _httpCacheManager = Scope.ServiceProvider.GetRequiredService(); _workflowCacheManager = Scope.ServiceProvider.GetRequiredService(); diff --git a/test/component/Elsa.Workflows.ComponentTests/Scenarios/WorkflowActivities/Workflows/DeleteWorkflow_Clustered.cs b/test/component/Elsa.Workflows.ComponentTests/Scenarios/WorkflowActivities/Workflows/DeleteWorkflowClustered.cs similarity index 100% rename from test/component/Elsa.Workflows.ComponentTests/Scenarios/WorkflowActivities/Workflows/DeleteWorkflow_Clustered.cs rename to test/component/Elsa.Workflows.ComponentTests/Scenarios/WorkflowActivities/Workflows/DeleteWorkflowClustered.cs