Add strict type checking for variable parsing (#6536)

* Add strict type checking for variable parsing

Introduced a `StrictMode` flag for variables, enforcing stricter type validation during parsing and conversions. Updated related methods and tests to reflect the stricter parsing behavior, ensuring consistent type compatibility and error handling.

* Refactor StrictMode handling and improve ObjectConverter logic

Moved StrictMode flag from Variable to ObjectConverter for better cohesion and updated related references. Additionally, removed unused parameters and simplified ObjectConverterOptions to streamline configuration and maintain consistency across the codebase.

* Refactor type conversion logic and update logging defaults

Improve type conversion reliability by introducing `TryConvertValue` and refining exception handling. Update JSON scenarios to use "Inherit" as the default for log persistence modes, ensuring consistency across configurations.

* Refactor exception handling in ObjectConverter.

Introduce a helper method `ReturnOrThrow` to streamline and centralize exception handling logic. This change also adds support for non-strict mode, allowing value fallback instead of throwing exceptions when enabled.

* Add numeric type checks and improve type conversion handling

Introduce `IsNumericType` extension method to identify numeric types. Enhance type conversion logic in `ObjectConverter` to handle numeric, boolean, and string types more accurately. Update variable deserialization workflows to handle conversion failures gracefully.

* Set a default comment for ObjectConverter.StrictMode assignment

Added a comment clarifying that StrictMode is set to its default value. This improves code readability and helps maintainers understand the intent.

* Fix formatting inconsistencies and improve code clarity

Removed redundant whitespace and adjusted formatting to align with coding standards. These changes enhance the readability and maintainability of the code without altering functionality.

* Fix typo in Program.cs variable comment

Corrected a minor issue in the comment for `identityTokenSection` to remove the unnecessary "Modify" text. This change improves code readability and removes potential confusion for developers.

* Simplify imports in ModifyVariableHandler.cs

Removed unused `Microsoft.Extensions.Options` and `Elsa.Workflows.Options` imports to clean up dependencies and improve maintainability. This change reduces clutter without affecting the existing functionality.
This commit is contained in:
Sipke Schoorstra 2025-03-31 18:11:14 +02:00 committed by GitHub
parent 8bf58c5c60
commit 952cbb4edc
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
19 changed files with 127 additions and 60 deletions

View file

@ -15,6 +15,7 @@ using Elsa.EntityFrameworkCore.Modules.Identity;
using Elsa.EntityFrameworkCore.Modules.Management;
using Elsa.EntityFrameworkCore.Modules.Runtime;
using Elsa.EntityFrameworkCore.Modules.Tenants;
using Elsa.Expressions.Helpers;
using Elsa.Extensions;
using Elsa.Features.Services;
using Elsa.Identity.Multitenancy;
@ -52,6 +53,7 @@ using Elsa.Workflows.LogPersistence;
using Elsa.Workflows.Management;
using Elsa.Workflows.Management.Compression;
using Elsa.Workflows.Management.Stores;
using Elsa.Workflows.Memory;
using Elsa.Workflows.Options;
using Elsa.Workflows.Runtime.Distributed.Extensions;
using Elsa.Workflows.Runtime.Options;
@ -105,6 +107,8 @@ const bool disableVariableWrappers = false;
const bool disableVariableCopying = false;
const bool useManualOtelInstrumentation = true;
ObjectConverter.StrictMode = true; // Default.
var builder = WebApplication.CreateBuilder(args);
var services = builder.Services;
var configuration = builder.Configuration;

View file

@ -44,7 +44,7 @@ public static class HandlerExtensions
/// <param name="cancellationToken">The cancellation token.</param>
public static Task InvokeAsync(this INotificationHandler handler, MethodBase handleMethod, INotification notification, CancellationToken cancellationToken)
{
return (Task)handleMethod.Invoke(handler, new object?[] { notification, cancellationToken })!;
return (Task)handleMethod.Invoke(handler, [notification, cancellationToken])!;
}
/// <summary>
@ -56,7 +56,7 @@ public static class HandlerExtensions
/// <param name="cancellationToken">The cancellation token.</param>
public static Task<TResult> InvokeAsync<TResult>(this ICommandHandler handler, MethodBase handleMethod, ICommand command, CancellationToken cancellationToken)
{
var task = (Task<TResult>)handleMethod.Invoke(handler, new object?[] { command, cancellationToken })!;
var task = (Task<TResult>)handleMethod.Invoke(handler, [command, cancellationToken])!;
return task;
}
}

View file

@ -44,4 +44,14 @@ public static class TypeExtensions
/// Returns the element type of the specified collection type.
/// </summary>
public static Type GetCollectionElementType(this Type type) => type.GenericTypeArguments[0];
/// <summary>
/// Determines whether the specified type is a numeric type.
/// </summary>
/// <param name="type">The type to check.</param>
/// <returns>True if the specified type is numeric, otherwise false.</returns>
public static bool IsNumericType(this Type type)
{
return type.IsPrimitive || type == typeof(decimal) || type == typeof(float) || type == typeof(double) || type == typeof(int) || type == typeof(long) || type == typeof(short) || type == typeof(byte) || type == typeof(uint) || type == typeof(ulong) || type == typeof(ushort) || type == typeof(sbyte);
}
}

View file

@ -1,6 +1,7 @@
using System.Collections;
using System.ComponentModel;
using System.Diagnostics.CodeAnalysis;
using System.Diagnostics.SymbolStore;
using System.Dynamic;
using System.Globalization;
using System.Text.Encodings.Web;
@ -26,6 +27,8 @@ public record ObjectConverterOptions(JsonSerializerOptions? SerializerOptions =
/// </summary>
public static class ObjectConverter
{
public static bool StrictMode = true; // Set to false to revert to original flexible behavior.
/// <summary>
/// Attempts to convert the source value into the destination type.
/// </summary>
@ -106,8 +109,19 @@ public static class ObjectConverter
{
if (jsonNode is not JsonArray jsonArray)
{
var valueKind = jsonNode.GetValueKind();
if (valueKind == JsonValueKind.Null)
return null;
if (valueKind == JsonValueKind.Undefined)
return null;
return underlyingTargetType switch
{
{ } t when t == typeof(bool) && valueKind == JsonValueKind.False => false,
{ } t when t == typeof(bool) && valueKind == JsonValueKind.True => true,
{ } t when t.IsNumericType() && valueKind == JsonValueKind.Number => ConvertTo(jsonNode.ToString(), t),
{ } t when t == typeof(string) && valueKind == JsonValueKind.String => jsonNode.ToString(),
{ } t when t == typeof(string) => jsonNode.ToString(),
{ } t when t == typeof(ExpandoObject) && jsonNode.GetValueKind() == JsonValueKind.Object => JsonSerializer.Deserialize<ExpandoObject>(jsonNode.ToJsonString()),
{ } t when t != typeof(object) || converterOptions?.DeserializeJsonObjectToObject == true => jsonNode.Deserialize(targetType, serializerOptions),
@ -187,14 +201,22 @@ public static class ObjectConverter
var targetTypeConverter = TypeDescriptor.GetConverter(underlyingTargetType);
if (targetTypeConverter.CanConvertFrom(underlyingSourceType))
return targetTypeConverter.IsValid(value)
? targetTypeConverter.ConvertFrom(null, CultureInfo.InvariantCulture, value)
: targetType.GetDefaultValue();
{
var isValid = targetTypeConverter.IsValid(value);
if (isValid)
return targetTypeConverter.ConvertFrom(null, CultureInfo.InvariantCulture, value);
}
var sourceTypeConverter = TypeDescriptor.GetConverter(underlyingSourceType);
if (sourceTypeConverter.CanConvertTo(underlyingTargetType))
{
var isValid = targetTypeConverter.IsValid(value);
if (isValid)
return sourceTypeConverter.ConvertTo(value, underlyingTargetType);
}
if (underlyingTargetType.IsEnum)
{
@ -249,7 +271,7 @@ public static class ObjectConverter
}
}
if(underlyingTargetType.IsArray)
if (underlyingTargetType.IsArray)
{
var executedEnumerable = enumerable.Cast<object>().ToList();
var underlyingTargetElementType = underlyingTargetType.GetElementType()!;
@ -261,6 +283,7 @@ public static class ObjectConverter
array.SetValue(convertedItem, index);
index++;
}
return array;
}
}
@ -269,8 +292,20 @@ public static class ObjectConverter
{
return Convert.ChangeType(value, underlyingTargetType, CultureInfo.InvariantCulture);
}
catch (FormatException e)
{
return ReturnOrThrow(e);
}
catch (InvalidCastException e)
{
return ReturnOrThrow(e);
}
object ReturnOrThrow(Exception e)
{
if (!StrictMode)
return value;
throw new TypeConversionException($"Failed to convert an object of type {sourceType} to {underlyingTargetType}", value, underlyingTargetType, e);
}
}

View file

@ -15,7 +15,7 @@ public class MemoryBlock
/// <summary>
/// Constructor.
/// </summary>
public MemoryBlock(object? value, object? metadata = default)
public MemoryBlock(object? value, object? metadata = null)
{
Value = value;
Metadata = metadata;

View file

@ -22,7 +22,7 @@ public class MemoryBlockReference
/// <summary>
/// The ID of the memory block.
/// </summary>
public string Id { get; set; } = default!;
public string Id { get; set; } = null!;
/// <summary>
/// Declares the memory block.
@ -62,7 +62,7 @@ public class MemoryBlockReference
/// <summary>
/// Sets the value of the memory block.
/// </summary>
public void Set(MemoryRegister memoryRegister, object? value, Action<MemoryBlock>? configure = default)
public void Set(MemoryRegister memoryRegister, object? value, Action<MemoryBlock>? configure = null)
{
var block = GetBlock(memoryRegister);
block.Value = value;
@ -72,7 +72,7 @@ public class MemoryBlockReference
/// <summary>
/// Sets the value of the memory block.
/// </summary>
public void Set(ExpressionExecutionContext context, object? value, Action<MemoryBlock>? configure = default) => context.Set(this, value, configure);
public void Set(ExpressionExecutionContext context, object? value, Action<MemoryBlock>? configure = null) => context.Set(this, value, configure);
/// <summary>
/// Returns the <see cref="MemoryBlock"/> pointed to by the specified memory block reference.

View file

@ -225,7 +225,6 @@ public class HttpEndpoint : Trigger<HttpRequest>
// Handle Form Fields
if (request.HasFormContentType)
{
var formFields = request.Form.ToObjectDictionary();
ParsedContent.Set(context, formFields);

View file

@ -18,37 +18,37 @@ namespace Elsa.Workflows.Activities;
public class SetVariable<T> : CodeActivity
{
/// <inheritdoc />
public SetVariable([CallerFilePath] string? source = default, [CallerLineNumber] int? line = default) : base(source, line)
public SetVariable([CallerFilePath] string? source = null, [CallerLineNumber] int? line = null) : base(source, line)
{
}
/// <inheritdoc />
public SetVariable(Variable<T> variable, Input<T> value, [CallerFilePath] string? source = default, [CallerLineNumber] int? line = default) : this(source, line)
public SetVariable(Variable<T> variable, Input<T> value, [CallerFilePath] string? source = null, [CallerLineNumber] int? line = null) : this(source, line)
{
Variable = variable;
Value = value;
}
/// <inheritdoc />
public SetVariable(Variable<T> variable, Variable<T> value, [CallerFilePath] string? source = default, [CallerLineNumber] int? line = default)
public SetVariable(Variable<T> variable, Variable<T> value, [CallerFilePath] string? source = null, [CallerLineNumber] int? line = null)
: this(variable, new Input<T>(value), source, line)
{
}
/// <inheritdoc />
public SetVariable(Variable<T> variable, Func<ExpressionExecutionContext, T> value, [CallerFilePath] string? source = default, [CallerLineNumber] int? line = default)
public SetVariable(Variable<T> variable, Func<ExpressionExecutionContext, T> value, [CallerFilePath] string? source = null, [CallerLineNumber] int? line = null)
: this(variable, new Input<T>(value), source, line)
{
}
/// <inheritdoc />
public SetVariable(Variable<T> variable, Func<T> value, [CallerFilePath] string? source = default, [CallerLineNumber] int? line = default)
public SetVariable(Variable<T> variable, Func<T> value, [CallerFilePath] string? source = null, [CallerLineNumber] int? line = null)
: this(variable, new Input<T>(value), source, line)
{
}
/// <inheritdoc />
public SetVariable(Variable<T> variable, T value, [CallerFilePath] string? source = default, [CallerLineNumber] int? line = default)
public SetVariable(Variable<T> variable, T value, [CallerFilePath] string? source = null, [CallerLineNumber] int? line = null)
: this(variable, new Input<T>(value), source, line)
{
}
@ -57,7 +57,7 @@ public class SetVariable<T> : CodeActivity
/// The variable to assign the value to.
/// </summary>
[Input(Description = "The variable to assign the value to.")]
public Variable<T> Variable { get; set; } = default!;
public Variable<T> Variable { get; set; } = null!;
/// <summary>
/// The value to assign.
@ -81,7 +81,7 @@ public class SetVariable<T> : CodeActivity
public class SetVariable : CodeActivity
{
/// <inheritdoc />
public SetVariable([CallerFilePath] string? source = default, [CallerLineNumber] int? line = default) : base(source, line)
public SetVariable([CallerFilePath] string? source = null, [CallerLineNumber] int? line = null) : base(source, line)
{
}
@ -89,7 +89,7 @@ public class SetVariable : CodeActivity
/// The variable to assign the value to.
/// </summary>
[Input(Description = "The variable to assign the value to.")]
public Variable Variable { get; set; } = default!;
public Variable Variable { get; set; } = null!;
/// <summary>
/// The value to assign.

View file

@ -1,4 +1,5 @@
using Elsa.Expressions.Helpers;
using Elsa.Expressions.Models;
// ReSharper disable once CheckNamespace
namespace Elsa.Extensions;
@ -29,8 +30,9 @@ public static class DictionaryExtensions
return false;
}
value = ConvertValue<T>(item);
return true;
var result = TryConvertValue<T>(item);
value = result.Success ? (T)result.Value! : default!;
return result.Success;
}
public static bool TryGetValue<TKey, T>(this IDictionary<TKey, object> dictionary, IEnumerable<TKey> keys, out T value)
@ -39,8 +41,9 @@ public static class DictionaryExtensions
{
if (dictionary.TryGetValue(key, out var item))
{
value = ConvertValue<T>(item);
return true;
var result = TryConvertValue<T>(item);
value = result.Success ? (T)result.Value! : default!;
return result.Success;
}
}
@ -98,4 +101,9 @@ public static class DictionaryExtensions
}
private static T? ConvertValue<T>(object? value) => value.ConvertTo<T>();
private static Result TryConvertValue<T>(object? value)
{
return value.TryConvertTo<T>();
}
}

View file

@ -8,7 +8,9 @@ using Elsa.Workflows;
using Elsa.Workflows.Activities;
using Elsa.Workflows.Memory;
using Elsa.Workflows.Models;
using Elsa.Workflows.Options;
using Humanizer;
using Microsoft.Extensions.Options;
// ReSharper disable once CheckNamespace
namespace Elsa.Extensions;
@ -137,8 +139,7 @@ public static class ExpressionExecutionContextExtensions
/// <summary>
/// Creates a named variable in the context.
/// </summary>
public static Variable CreateVariable<T>(this ExpressionExecutionContext context, string name, T? value, Type? storageDriverType = null,
Action<MemoryBlock>? configure = null)
public static Variable CreateVariable<T>(this ExpressionExecutionContext context, string name, T? value, Type? storageDriverType = null, Action<MemoryBlock>? configure = null)
{
var existingVariable = context.GetVariable(name, localScopeOnly: true);
@ -150,11 +151,13 @@ public static class ExpressionExecutionContextExtensions
StorageDriverType = storageDriverType ?? typeof(WorkflowInstanceStorageDriver)
};
var parsedValue = variable.ParseValue(value);
// Find the first parent context that has a variable container.
// If not found, use the current context.
var variableContainerContext = context.GetVariableContainerContext();
variableContainerContext.Set(variable, value, configure);
variableContainerContext.Set(variable, parsedValue, configure);
return variable;
}
@ -184,7 +187,8 @@ public static class ExpressionExecutionContextExtensions
var contextWithVariable = context.FindContextContainingBlock(variable.Id) ?? context;
// Set the value on the variable.
variable.Set(contextWithVariable, value, configure);
var parsedValue = variable.ParseValue(value);
variable.Set(contextWithVariable, parsedValue, configure);
// Return the variable.
return variable;
@ -199,7 +203,8 @@ public static class ExpressionExecutionContextExtensions
{
// Set the value on the output.
var outputMemoryBlockReference = output.MemoryBlockReference();
context.Set(outputMemoryBlockReference, value, configure);
var parsedValue = output.ParseValue(value);
context.Set(outputMemoryBlockReference, parsedValue, configure);
// If the referenced output is a workflow output definition, set the value on the workflow execution context.
var workflowExecutionContext = context.GetWorkflowExecutionContext();

View file

@ -1,4 +1,5 @@
using System.Runtime.CompilerServices;
using Elsa.Expressions.Helpers;
using Elsa.Expressions.Models;
using Elsa.Workflows;
using Elsa.Workflows.Memory;
@ -20,7 +21,7 @@ public static class OutputExtensions
/// <summary>
/// Sets the output to the specified value.
/// </summary>
public static void Set<T>(this Output<T>? output, ActivityExecutionContext context, T? value, [CallerArgumentExpression("output")] string? outputName = default) => context.Set(output, value, outputName);
public static void Set<T>(this Output<T>? output, ActivityExecutionContext context, T? value, [CallerArgumentExpression("output")] string? outputName = null) => context.Set(output, value, outputName);
/// <summary>
/// Sets the output to the specified value.
@ -45,10 +46,10 @@ public static class OutputExtensions
var memoryBlockReference = output?.MemoryBlockReference();
if (memoryBlockReference is null)
return default;
return null;
if(!context.ExpressionExecutionContext.TryGetBlock(memoryBlockReference, out var memoryBlock))
return default;
return null;
var parsedContentVariableType = (memoryBlock.Metadata as VariableBlockMetadata)?.Variable.GetType();
return parsedContentVariableType?.GenericTypeArguments.FirstOrDefault();
@ -62,4 +63,10 @@ public static class OutputExtensions
var memoryBlockReference = output?.MemoryBlockReference();
return memoryBlockReference is not null && context.ExpressionExecutionContext.TryGetBlock(memoryBlockReference, out _);
}
public static object? ParseValue(this Output output, object? value)
{
var genericType = output.GetType();
return VariableExtensions.ParseValue(genericType, value);
}
}

View file

@ -66,13 +66,7 @@ public static class VariableExtensions
public static void Set(this Variable variable, ActivityExecutionContext context, object? value)
{
// Validate type compatibility.
if (!variable.TryParseValue(value, out var parsedValue))
{
var variableType = variable.GetVariableType();
throw new InvalidCastException($"The value '{value}' is not compatible with the variable '{variable.Name}' of type '{variableType.FullName}'.");
}
var parsedValue = variable.ParseValue(value);
// Set the value.
((MemoryBlockReference)variable).Set(context, parsedValue);
}
@ -83,7 +77,13 @@ public static class VariableExtensions
[RequiresUnreferencedCode("Calls System.Text.Json.JsonSerializer.Serialize<TValue>(TValue, JsonSerializerOptions)")]
public static object? ParseValue(this Variable variable, object? value)
{
var genericType = variable.GetType().GenericTypeArguments.FirstOrDefault();
var genericType = variable.GetType();
return ParseValue(genericType, value);
}
public static object? ParseValue(Type type, object? value)
{
var genericType = type.GenericTypeArguments.FirstOrDefault();
var converterOptions = new ObjectConverterOptions(SerializerOptions);
return genericType == null ? value : value?.ConvertTo(genericType, converterOptions);
}

View file

@ -1,4 +1,3 @@
using System.Text.Json.Serialization;
using Elsa.Expressions.Helpers;
using Elsa.Expressions.Models;
using Humanizer;

View file

@ -1,3 +1,4 @@
using Elsa.Expressions.Helpers;
using Elsa.Expressions.Models;
using Elsa.Extensions;
using Elsa.Workflows.Memory;
@ -51,6 +52,9 @@ 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.GetVariableType().FullName, value);
if (!ObjectConverter.StrictMode)
variable.Set(register, value);
continue;
}

View file

@ -47,7 +47,8 @@ public class WorkflowInstanceStorageDriver(IPayloadSerializer payloadSerializer)
DeserializeJsonObjectToObject = true,
SerializerOptions = payloadSerializer.GetOptions()
};
var parsedValue = node.ConvertTo(variableType, options);
var result = node.TryConvertTo(variableType, options);
var parsedValue = result.Success ? result.Value : node;
return new (parsedValue);
}

View file

@ -79,7 +79,7 @@
"logPersistenceMode": {
"default": "Exclude",
"inputs": {
"text": "Default"
"text": "Inherit"
},
"outputs": {}
},

View file

@ -52,7 +52,7 @@
"logPersistenceMode": {
"default": "Include",
"inputs": {
"text": "Default"
"text": "Inherit"
},
"outputs": {}
},
@ -117,7 +117,7 @@
"version": 1,
"customProperties": {
"logPersistenceMode": {
"default": "Default",
"default": "Inherit",
"inputs": {
"text": "Include"
},
@ -154,7 +154,7 @@
"version": 1,
"customProperties": {
"logPersistenceMode": {
"default": "Default",
"default": "Inherit",
"inputs": {
"text": "Exclude"
},

View file

@ -75,10 +75,10 @@
"canStartWorkflow": false,
"runAsynchronously": false,
"logPersistenceMode": {
"default": "Default",
"default": "Inherit",
"inputs": {
"outputName": "Default",
"outputValue": "Default"
"outputName": "Inherit",
"outputValue": "Inherit"
},
"outputs": {}
}
@ -120,7 +120,7 @@
"canStartWorkflow": false,
"runAsynchronously": false,
"logPersistenceMode": {
"default": "Default",
"default": "Inherit",
"inputs": {
"outputName": "Include",
"outputValue": "Include"

View file

@ -46,13 +46,8 @@ public class Tests
var result = value.TryConvertTo<int>();
// Assert
// I would have expected this conversion to not be successful. It seems there are many cases like this
//Assert.False(result.Success);
//Assert.NotNull(result.Exception);
Assert.True(result.Success);
Assert.Equal(0, result.Value);
Assert.False(result.Success);
Assert.NotNull(result.Exception);
}
[Fact]