Fix JS Variable Setting Bug in Workflow Variable Sync Logic (#6175)

* Fix `variables` handling override values set via `Set{VariableName}()` syntax

* Make `TenantsFeature` a dependency of `Multitenancy` to ensure streamlined initialization

From this point on, all application instances are multi-tenant with a single, default tenant. This streamlines the startup tasks that are designed around multitenancy.

* Add JS variable handling tests

* Ensure ExpandoObjects are correctly processed and synchronized between contexts and engines

Introduced a new helper method to process ExpandoObject conversions to JavaScript objects. Updated various handlers and functions to ensure ExpandoObjects are correctly processed and synchronized between contexts and engines. Added a new workflow test to validate the updated processing logic.

src/modules/Elsa.JavaScript/Extensions/EngineExtensions.cs

@@ -1,5 +1,8 @@
+using Elsa.JavaScript.Helpers;
+using Elsa.JavaScript.Options;
 using Jint;
 using Jint.Runtime.Interop;
+using Microsoft.Extensions.Options;
 
 // ReSharper disable once CheckNamespace
 namespace Elsa.Extensions;
@@ -18,4 +21,15 @@ public static class EngineExtensions
     /// Register the specified type <c>T</c> with the engine.
     /// </summary>
     public static void RegisterType(this Engine engine, Type type) => engine.SetValue(type.Name, TypeReference.CreateTypeReference(engine, type));
+
+    internal static void SyncVariablesContainer(this Engine engine, IOptions<JintOptions> options, string name, object? value)
+    {
+        if (!options.Value.DisableWrappers)
+        {
+            // To ensure both variable accessor syntaxes work, we need to update the variables container in the engine as well as the context to keep them in sync.
+            var variablesContainer = (IDictionary<string, object?>)engine.GetValue("variables").ToObject()!;
+            variablesContainer[name] = ObjectConverterHelper.ProcessVariableValue(engine, value);
+            engine.SetValue("variables", variablesContainer);
+        }
+    }
 }

src/modules/Elsa.JavaScript/Handlers/ConfigureEngineWithCommonFunctions.cs

@@ -5,9 +5,12 @@ using System.Text.Json.Serialization;
 using System.Text.RegularExpressions;
 using System.Text.Unicode;
 using Elsa.Extensions;
+using Elsa.JavaScript.Helpers;
 using Elsa.JavaScript.Notifications;
+using Elsa.JavaScript.Options;
 using Elsa.Mediator.Contracts;
 using JetBrains.Annotations;
+using Microsoft.Extensions.Options;
 
 namespace Elsa.JavaScript.Handlers;
 
@@ -15,7 +18,7 @@ namespace Elsa.JavaScript.Handlers;
 /// A handler that configures the Jint engine with common functions.
 /// </summary>
 [UsedImplicitly]
-public class ConfigureEngineWithCommonFunctions : INotificationHandler<EvaluatingJavaScript>
+public class ConfigureEngineWithCommonFunctions(IOptions<JintOptions> options) : INotificationHandler<EvaluatingJavaScript>
 {
     private readonly JsonSerializerOptions _jsonSerializerOptions = CreateJsonSerializerOptions();
 
@@ -32,7 +35,11 @@ public class ConfigureEngineWithCommonFunctions : INotificationHandler<Evaluatin
         engine.SetValue("getWorkflowInstanceId", (Func<string>)(() => context.GetActivityExecutionContext().WorkflowExecutionContext.Id));
         engine.SetValue("setCorrelationId", (Action<string?>)(value => context.GetActivityExecutionContext().WorkflowExecutionContext.CorrelationId = value));
         engine.SetValue("getCorrelationId", (Func<string?>)(() => context.GetActivityExecutionContext().WorkflowExecutionContext.CorrelationId));
-        engine.SetValue("setVariable", (Action<string, object>)((name, value) => context.SetVariableInScope(name, value)));
+        engine.SetValue("setVariable", (Action<string, object>)((name, value) =>
+        {
+            engine.SyncVariablesContainer(options, name, value);
+            context.SetVariableInScope(name, value);
+        }));
         engine.SetValue("getVariable", (Func<string, object?>)(name => context.GetVariableInScope(name)));
         engine.SetValue("getInput", (Func<string, object?>)(name => context.GetInput(name)));
         engine.SetValue("getOutputFrom", (Func<string, string?, object?>)((activityIdName, outputName) => context.GetOutput(activityIdName, outputName)));

src/modules/Elsa.JavaScript/Handlers/ConfigureEngineWithVariables.cs

@@ -23,18 +23,18 @@ public class ConfigureEngineWithVariables(IOptions<JintOptions> options) : INoti
     /// <inheritdoc />
     public Task HandleAsync(EvaluatingJavaScript notification, CancellationToken cancellationToken)
     {
-        if(options.Value.DisableWrappers)
+        if (options.Value.DisableWrappers)
             return Task.CompletedTask;
-        
+
         CopyVariablesIntoEngine(notification);
         return Task.CompletedTask;
     }
 
     public Task HandleAsync(EvaluatedJavaScript notification, CancellationToken cancellationToken)
     {
-        if(options.Value.DisableWrappers)
+        if (options.Value.DisableWrappers)
             return Task.CompletedTask;
-        
+
         CopyVariablesIntoWorkflowExecutionContext(notification);
         return Task.CompletedTask;
     }
@@ -56,6 +56,23 @@ public class ConfigureEngineWithVariables(IOptions<JintOptions> options) : INoti
         }
     }
 
+    private void CopyVariablesIntoEngine(EvaluatingJavaScript notification)
+    {
+        var engine = notification.Engine;
+        var context = notification.Context;
+        var variableNames = context.GetVariableNamesInScope().FilterInvalidVariableNames().ToList();
+        var variablesContainer = (IDictionary<string, object?>)new ExpandoObject();
+
+        foreach (var variableName in variableNames)
+        {
+            var variableValue = context.GetVariableInScope(variableName);
+            variableValue = ObjectConverterHelper.ProcessVariableValue(engine, variableValue);
+            variablesContainer[variableName] = variableValue;
+        }
+
+        engine.SetValue("variables", variablesContainer);
+    }
+
     private IEnumerable<string> GetInputNames(ExpressionExecutionContext context)
     {
         var activityExecutionContext = context.TryGetActivityExecutionContext(out var aec) ? aec : null;
@@ -73,32 +90,4 @@ public class ConfigureEngineWithVariables(IOptions<JintOptions> options) : INoti
             activityExecutionContext = activityExecutionContext.ParentActivityExecutionContext;
         }
     }
-
-    private void CopyVariablesIntoEngine(EvaluatingJavaScript notification)
-    {
-        var engine = notification.Engine;
-        var context = notification.Context;
-        var variableNames = context.GetVariableNamesInScope().FilterInvalidVariableNames().ToList();
-        var variablesContainer = (IDictionary<string, object?>)new ExpandoObject();
-
-        foreach (var variableName in variableNames)
-        {
-            var variableValue = context.GetVariableInScope(variableName);
-            variableValue = ProcessVariableValue(engine, variableValue);
-            variablesContainer[variableName] = variableValue;
-        }
-
-        engine.SetValue("variables", variablesContainer);
-    }
-
-    private object? ProcessVariableValue(Engine engine, object? variableValue)
-    {
-        if (variableValue == null)
-            return null;
-
-        if (variableValue is not ExpandoObject expandoObject)
-            return variableValue;
-
-        return ObjectConverterHelper.ConvertToJsObject(engine, expandoObject);
-    }
 }

src/modules/Elsa.JavaScript/Handlers/ConfigureEngineWithVariablesAndInputOutputAccessors.cs

@@ -21,12 +21,12 @@ public class ConfigureEngineWithVariablesAndInputOutputAccessors(IOptions<JintOp
     /// <inheritdoc />
     public async Task HandleAsync(EvaluatingJavaScript notification, CancellationToken cancellationToken)
     {
-        if(options.Value.DisableWrappers)
+        if (options.Value.DisableWrappers)
             return;
-        
+
         var engine = notification.Engine;
         var context = notification.Context;
-        
+
         // The order of the next 3 lines is important.
         CreateVariableAccessors(engine, context);
         CreateWorkflowInputAccessors(engine, context);
@@ -41,7 +41,11 @@ public class ConfigureEngineWithVariablesAndInputOutputAccessors(IOptions<JintOp
         {
             var pascalName = variableName.Pascalize();
             engine.SetValue($"get{pascalName}", (Func<object?>)(() => context.GetVariableInScope(variableName)));
-            engine.SetValue($"set{pascalName}", (Action<object?>)(value => context.SetVariableInScope(variableName, value)));
+            engine.SetValue($"set{pascalName}", (Action<object?>)(value =>
+            {
+                engine.SyncVariablesContainer(options, variableName, value);
+                context.SetVariableInScope(variableName, value);
+            }));
         }
     }
 
@@ -65,7 +69,7 @@ public class ConfigureEngineWithVariablesAndInputOutputAccessors(IOptions<JintOp
             engine.SetValue($"get{inputDefinition.Name}", (Func<object?>)(() => input?.Value));
         }
     }
-    
+
     private static async Task CreateActivityOutputAccessorsAsync(Engine engine, ExpressionExecutionContext context)
     {
         var activityOutputs = context.GetActivityOutputs();

src/modules/Elsa.JavaScript/Helpers/ObjectConverterHelper.cs

@@ -1,14 +1,30 @@
 using System.Collections;
+using System.Dynamic;
 using Elsa.Extensions;
+using Elsa.JavaScript.Options;
 using Jint;
 using Jint.Native;
 using Jint.Native.Object;
 using Jint.Runtime.Descriptors;
+using Microsoft.Extensions.Options;
 
 namespace Elsa.JavaScript.Helpers;
 
 internal static class ObjectConverterHelper
 {
+    
+    
+    public static object? ProcessVariableValue(Engine engine, object? variableValue)
+    {
+        if (variableValue == null)
+            return null;
+
+        if (variableValue is not ExpandoObject expandoObject)
+            return variableValue;
+
+        return ConvertToJsObject(engine, expandoObject);
+    }
+    
     public static ObjectInstance ConvertToJsObject(Engine engine, IDictionary<string, object?> expando)
     {
         var jsObject = engine.Intrinsics.Object.Construct([]);

src/modules/Elsa.Tenants/Features/TenantsFeature.cs

@@ -1,6 +1,7 @@
 using Elsa.Common.Features;
 using Elsa.Common.Multitenancy;
 using Elsa.Features.Abstractions;
+using Elsa.Features.Attributes;
 using Elsa.Features.Services;
 using Elsa.Tenants.Options;
 using Elsa.Tenants.Providers;
@@ -11,6 +12,7 @@ namespace Elsa.Tenants.Features;
 /// <summary>
 /// Configures multi-tenancy features.
 /// </summary>
+[DependencyOf(typeof(MultitenancyFeature))]
 public class TenantsFeature(IModule serviceConfiguration) : FeatureBase(serviceConfiguration)
 {
     /// <summary>

test/component/Elsa.Workflows.ComponentTests/Scenarios/JavaScriptVariables/JavaScriptVariablesWorkflow1.cs

@@ -0,0 +1,16 @@
+using Elsa.Extensions;
+using Elsa.JavaScript.Activities;
+
+namespace Elsa.Workflows.ComponentTests.Scenarios.JavaScriptVariables;
+
+public class JavaScriptVariablesWorkflow1 : WorkflowBase
+{
+    public static readonly string DefinitionId = Guid.NewGuid().ToString();
+
+    protected override void Build(IWorkflowBuilder builder)
+    {
+        builder.WithDefinitionId(DefinitionId);
+        builder.WithVariable("MagicNumber", 3).WithWorkflowStorage();
+        builder.Root = new RunJavaScript("setMagicNumber(42)", default, default);
+    }
+}

test/component/Elsa.Workflows.ComponentTests/Scenarios/JavaScriptVariables/JavaScriptVariablesWorkflow2.cs

@@ -0,0 +1,16 @@
+using Elsa.Extensions;
+using Elsa.JavaScript.Activities;
+
+namespace Elsa.Workflows.ComponentTests.Scenarios.JavaScriptVariables;
+
+public class JavaScriptVariablesWorkflow2 : WorkflowBase
+{
+    public static readonly string DefinitionId = Guid.NewGuid().ToString();
+
+    protected override void Build(IWorkflowBuilder builder)
+    {
+        builder.WithDefinitionId(DefinitionId);
+        builder.WithVariable("MagicNumber", 3).WithWorkflowStorage();
+        builder.Root = new RunJavaScript("variables.MagicNumber = 42", default, default);
+    }
+}

test/component/Elsa.Workflows.ComponentTests/Scenarios/JavaScriptVariables/JavaScriptVariablesWorkflow3.cs

@@ -0,0 +1,16 @@
+using Elsa.Extensions;
+using Elsa.JavaScript.Activities;
+
+namespace Elsa.Workflows.ComponentTests.Scenarios.JavaScriptVariables;
+
+public class JavaScriptVariablesWorkflow3 : WorkflowBase
+{
+    public static readonly string DefinitionId = Guid.NewGuid().ToString();
+
+    protected override void Build(IWorkflowBuilder builder)
+    {
+        builder.WithDefinitionId(DefinitionId);
+        builder.WithVariable("MagicNumber", 3).WithWorkflowStorage();
+        builder.Root = new RunJavaScript("setVariable('MagicNumber', 42)", default, default);
+    }
+}

test/component/Elsa.Workflows.ComponentTests/Scenarios/JavaScriptVariables/JavaScriptVariablesWorkflowTests.cs

@@ -0,0 +1,51 @@
+using Elsa.Expressions.Helpers;
+using Elsa.Extensions;
+using Elsa.Workflows.ComponentTests.Abstractions;
+using Elsa.Workflows.ComponentTests.Fixtures;
+using Elsa.Workflows.Management;
+using Elsa.Workflows.Models;
+using Elsa.Workflows.Runtime;
+using Elsa.Workflows.Runtime.Messages;
+using Elsa.Workflows.State;
+using Microsoft.Extensions.DependencyInjection;
+
+namespace Elsa.Workflows.ComponentTests.Scenarios.JavaScriptVariables;
+
+public class JavaScriptVariablesWorkflowTests(App app) : AppComponentTest(app)
+{
+    [Theory(DisplayName = "SetVariable JS function sets a variable and does not get overridden by variables API")]
+    [MemberData(nameof(GetWorkflowDefinitions))]
+    public async Task SetVariableRetainsValue(string workflowDefinitionId)
+    {
+        var workflowRuntime = Scope.ServiceProvider.GetRequiredService<IWorkflowRuntime>();
+        var workflowInstanceStore = Scope.ServiceProvider.GetRequiredService<IWorkflowInstanceStore>();
+        var workflowClient = await workflowRuntime.CreateClientAsync();
+        var runAndCreateRequest = new CreateAndRunWorkflowInstanceRequest
+        {
+            WorkflowDefinitionHandle = WorkflowDefinitionHandle.ByDefinitionId(workflowDefinitionId)
+        };
+        var runResponse = await workflowClient.CreateAndRunInstanceAsync(runAndCreateRequest);
+        var workflowInstanceId = runResponse.WorkflowInstanceId;
+        var workflowInstance = await workflowInstanceStore.FindAsync(workflowInstanceId);
+        var workflowState = workflowInstance!.WorkflowState;
+        var rootWorkflowActivityExecutionContext = workflowState.ActivityExecutionContexts.Single(x => x.ParentContextId == null);
+        var variables = GetVariablesDictionary(rootWorkflowActivityExecutionContext);
+        var magicNumber = variables["Workflow1:variable-1"].ConvertTo<int>();
+        Assert.Equal(42, magicNumber);
+    }
+
+    public static IEnumerable<object[]> GetWorkflowDefinitions()
+    {
+        return
+        [
+            [JavaScriptVariablesWorkflow1.DefinitionId],
+            [JavaScriptVariablesWorkflow2.DefinitionId],
+            [JavaScriptVariablesWorkflow3.DefinitionId]
+        ];
+    }
+
+    private VariablesDictionary GetVariablesDictionary(ActivityExecutionContextState context)
+    {
+        return context.Properties.GetOrAdd(WorkflowInstanceStorageDriver.VariablesDictionaryStateKey, () => new VariablesDictionary());
+    }
+}