From b1f839a517d2a54e27c88bf60d42035e179641eb Mon Sep 17 00:00:00 2001 From: Sipke Schoorstra Date: Thu, 12 Dec 2024 23:03:20 +0100 Subject: [PATCH 1/3] Improve variable parsing with error handling and logging Added `TryParseValue` method to handle parse errors gracefully and prevent crashes. Updated `VariablePersistenceManager` to log warnings when variable parsing fails, providing better debugging support and resilience during workflow execution. --- .../Extensions/VariableExtensions.cs | 29 +++++++++++++++++++ .../Services/VariablePersistenceManager.cs | 16 +++++++--- 2 files changed, 41 insertions(+), 4 deletions(-) diff --git a/src/modules/Elsa.Workflows.Core/Extensions/VariableExtensions.cs b/src/modules/Elsa.Workflows.Core/Extensions/VariableExtensions.cs index 05e2ac287..a37a58879 100644 --- a/src/modules/Elsa.Workflows.Core/Extensions/VariableExtensions.cs +++ b/src/modules/Elsa.Workflows.Core/Extensions/VariableExtensions.cs @@ -4,6 +4,7 @@ using System.Text.Json; using System.Text.Json.Serialization; using System.Text.Unicode; using Elsa.Expressions.Helpers; +using Elsa.Expressions.Models; using Elsa.Workflows; using Elsa.Workflows.Memory; using Elsa.Workflows.Serialization.Converters; @@ -62,6 +63,16 @@ public static class VariableExtensions variable.StorageDriverType = storageDriverType; return variable; } + + public static void Set(this Variable variable, ActivityExecutionContext context, object? value) + { + // Validate type compatibility. + if (!variable.TryParseValue(value, out var parsedValue)) + throw new InvalidCastException($"The value '{value}' is not compatible with the variable '{variable.Name}' of type '{variable.GetType().Name}'."); + + // Set the value. + ((MemoryBlockReference)variable).Set(context, parsedValue); + } /// /// Converts the specified value into a type that is compatible with the variable. @@ -73,6 +84,24 @@ public static class VariableExtensions var converterOptions = new ObjectConverterOptions(SerializerOptions); return genericType == null ? value : value?.ConvertTo(genericType, converterOptions); } + + /// + /// Converts the specified value into a type that is compatible with the variable. + /// + [RequiresUnreferencedCode("Calls System.Text.Json.JsonSerializer.Serialize(TValue, JsonSerializerOptions)")] + public static bool TryParseValue(this Variable variable, object? value, out object? parsedValue) + { + try + { + parsedValue = variable.ParseValue(value); + return true; + } + catch + { + parsedValue = null; + return false; + } + } /// /// Return the type of the variable. diff --git a/src/modules/Elsa.Workflows.Core/Services/VariablePersistenceManager.cs b/src/modules/Elsa.Workflows.Core/Services/VariablePersistenceManager.cs index d4e17ce3f..443acd0a7 100644 --- a/src/modules/Elsa.Workflows.Core/Services/VariablePersistenceManager.cs +++ b/src/modules/Elsa.Workflows.Core/Services/VariablePersistenceManager.cs @@ -1,17 +1,19 @@ using Elsa.Expressions.Models; using Elsa.Extensions; using Elsa.Workflows.Memory; +using Microsoft.Extensions.Logging; namespace Elsa.Workflows; /// -public class VariablePersistenceManager(IStorageDriverManager storageDriverManager) : IVariablePersistenceManager +public class VariablePersistenceManager(IStorageDriverManager storageDriverManager, ILogger logger) : IVariablePersistenceManager { /// - public async Task LoadVariablesAsync(WorkflowExecutionContext workflowExecutionContext, IEnumerable? excludeTags = default) + public async Task LoadVariablesAsync(WorkflowExecutionContext workflowExecutionContext, IEnumerable? excludeTags = null) { var cancellationToken = workflowExecutionContext.CancellationToken; var contexts = workflowExecutionContext.ActivityExecutionContexts.ToList(); + var excludeTagsList = excludeTags?.ToList(); foreach (var context in contexts) { @@ -34,15 +36,21 @@ public class VariablePersistenceManager(IStorageDriverManager storageDriverManag if (driver == null) continue; - if (excludeTags != null && driver.Tags.Any(excludeTags!.Contains)) + if (excludeTagsList != null && driver.Tags.Any(excludeTagsList.Contains)) continue; var id = GetStateId(variable); var value = await driver.ReadAsync(id, storageDriverContext); if (value == null) continue; - var parsedValue = variable.ParseValue(value); register.Declare(variable); + + if (!variable.TryParseValue(value, out var parsedValue)) + { + logger.LogWarning("Failed to parse value for variable {VariableId} of type {VariableType} with value {Value}", variable.Id, variable.GetType().Name, value); + continue; + } + variable.Set(register, parsedValue); } } From 9aed23278e84cd9225194e7a8e94e3539bb8156e Mon Sep 17 00:00:00 2001 From: Sipke Schoorstra Date: Thu, 12 Dec 2024 23:06:50 +0100 Subject: [PATCH 2/3] Improve error message for invalid variable type cast Updated the exception message to use the full type name of the variable, providing clearer details for debugging type compatibility issues during variable value parsing. --- .../Elsa.Workflows.Core/Extensions/VariableExtensions.cs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/modules/Elsa.Workflows.Core/Extensions/VariableExtensions.cs b/src/modules/Elsa.Workflows.Core/Extensions/VariableExtensions.cs index a37a58879..125040e79 100644 --- a/src/modules/Elsa.Workflows.Core/Extensions/VariableExtensions.cs +++ b/src/modules/Elsa.Workflows.Core/Extensions/VariableExtensions.cs @@ -68,7 +68,10 @@ public static class VariableExtensions { // Validate type compatibility. if (!variable.TryParseValue(value, out var parsedValue)) - throw new InvalidCastException($"The value '{value}' is not compatible with the variable '{variable.Name}' of type '{variable.GetType().Name}'."); + { + var variableType = variable.GetVariableType(); + throw new InvalidCastException($"The value '{value}' is not compatible with the variable '{variable.Name}' of type '{variableType.FullName}'."); + } // Set the value. ((MemoryBlockReference)variable).Set(context, parsedValue); From 2cd90ad01120d473ae5a2a36c01a39f17a7a2158 Mon Sep 17 00:00:00 2001 From: Sipke Schoorstra Date: Thu, 12 Dec 2024 23:08:48 +0100 Subject: [PATCH 3/3] Fix logging to display full variable type on parse failure Updated the log message to use the full variable type name instead of the base type. This provides more detailed context for debugging failed variable parsing issues. --- .../Elsa.Workflows.Core/Services/VariablePersistenceManager.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/modules/Elsa.Workflows.Core/Services/VariablePersistenceManager.cs b/src/modules/Elsa.Workflows.Core/Services/VariablePersistenceManager.cs index 443acd0a7..663e0057d 100644 --- a/src/modules/Elsa.Workflows.Core/Services/VariablePersistenceManager.cs +++ b/src/modules/Elsa.Workflows.Core/Services/VariablePersistenceManager.cs @@ -47,7 +47,7 @@ public class VariablePersistenceManager(IStorageDriverManager storageDriverManag if (!variable.TryParseValue(value, out var parsedValue)) { - logger.LogWarning("Failed to parse value for variable {VariableId} of type {VariableType} with value {Value}", variable.Id, variable.GetType().Name, value); + logger.LogWarning("Failed to parse value for variable {VariableId} of type {VariableType} with value {Value}", variable.Id, variable.GetVariableType().FullName, value); continue; }