Address PR #7195 review feedback: optimize caching, improve exceptions, add test coverage (#7196)

Copilot · sfmskywalker · web-flow · commit cce4f488ab89 · 2026-01-18T11:05:11.000+01:00
* Initial plan

* Apply PR review feedback: Fix exceptions, optimize caching, improve error handling

Co-authored-by: sfmskywalker &lt;938393+sfmskywalker@users.noreply.github.com&gt;

* Add unit tests for MaterializerRegistry and LocalWorkflowClient exception handling

Co-authored-by: sfmskywalker &lt;938393+sfmskywalker@users.noreply.github.com&gt;

* Add unit tests for BackgroundWorkflowCancellationDispatcher tenant headers

Co-authored-by: sfmskywalker &lt;938393+sfmskywalker@users.noreply.github.com&gt;

* Refactor `WorkflowMaterializerNotFoundException` to improve structure and usability, update related references, and simplify object initialization in test cases.

* Update `WorkflowDefinitionServiceTests` to use `WorkflowMaterializerNotFoundException` in place of `InvalidOperationException` for materializer not found scenario

---------

Co-authored-by: copilot-swe-agent[bot] &lt;198982749+Copilot@users.noreply.github.com&gt;
Co-authored-by: sfmskywalker &lt;938393+sfmskywalker@users.noreply.github.com&gt;
Co-authored-by: Sipke Schoorstra &lt;sipkeschoorstra@outlook.com&gt;
diff --git a/src/modules/Elsa.Caching/Services/CacheManager.cs b/src/modules/Elsa.Caching/Services/CacheManager.cs
@@ -39,6 +39,6 @@ public ValueTask TriggerTokenAsync(string key, CancellationToken cancellationTok
     /// <exception cref="InvalidOperationException">Thrown if the factory function returns null.</exception>
     public async Task<TItem> GetOrCreateAsync<TItem>(object key, Func<ICacheEntry, Task<TItem>> factory)
     {
-        return await memoryCache.GetOrCreateAsync(key, async entry => await factory(entry)) ?? throw new InvalidOperationException("Factory returned null.");
+        return await memoryCache.GetOrCreateAsync(key, async entry => await factory(entry)) ?? throw new InvalidOperationException($"Factory returned null for cache key: {key}.");
     }
 }
diff --git a/src/modules/Elsa.Workflows.Api/Endpoints/WorkflowDefinitions/GetByDefinitionId/Endpoint.cs b/src/modules/Elsa.Workflows.Api/Endpoints/WorkflowDefinitions/GetByDefinitionId/Endpoint.cs
@@ -31,7 +31,7 @@ public override async Task HandleAsync(Request request, CancellationToken cancel
         if (!materializerRegistry.IsMaterializerAvailable(definition.MaterializerName))
         {
             AddError($"The workflow materializer '{definition.MaterializerName}' is not available. The materializer may be disabled or not registered.");
-            await Send.ErrorsAsync(StatusCodes.Status503ServiceUnavailable, cancellationToken);
+            await Send.ErrorsAsync(StatusCodes.Status422UnprocessableEntity, cancellationToken);
             return;
         }
 
diff --git a/src/modules/Elsa.Workflows.Management/Contracts/IWorkflowDefinitionService.cs b/src/modules/Elsa.Workflows.Management/Contracts/IWorkflowDefinitionService.cs
@@ -115,8 +115,8 @@ public interface IWorkflowDefinitionService
     /// <param name="filter">The filter specifying criteria to identify the workflow definitions to search for.</param>
     /// <param name="cancellationToken">A token to observe while waiting for the task to complete.</param>
     /// <returns>
-    /// A <see cref="WorkflowGraphFindResult"/> containing the workflow graph and its associated definition,
-    /// or indicating that they do not exist.
+    /// A collection of <see cref="WorkflowGraphFindResult"/> instances, each containing a workflow graph and its associated definition
+    /// for every matching workflow definition. If no matching workflow definitions are found, the collection is empty.
     /// </returns>
     Task<IEnumerable<WorkflowGraphFindResult>> TryFindWorkflowGraphsAsync(WorkflowDefinitionFilter filter, CancellationToken cancellationToken = default);
 }
diff --git a/src/modules/Elsa.Workflows.Management/Exceptions/WorkflowMaterializerNotFoundException.cs b/src/modules/Elsa.Workflows.Management/Exceptions/WorkflowMaterializerNotFoundException.cs
@@ -0,0 +1,6 @@
+namespace Elsa.Workflows.Management.Exceptions;
+
+public class WorkflowMaterializerNotFoundException(string materializerName, string message = "Materializer not found. The materializer may be disabled or not registered") : Exception(message)
+{
+    public string MaterializerName { get; } = materializerName;
+}
diff --git a/src/modules/Elsa.Workflows.Management/Services/CachingWorkflowDefinitionService.cs b/src/modules/Elsa.Workflows.Management/Services/CachingWorkflowDefinitionService.cs
@@ -156,12 +156,17 @@ public async Task<IEnumerable<WorkflowGraphFindResult>> TryFindWorkflowGraphsAsy
         var results = new List<WorkflowGraphFindResult>();
         foreach (var workflowDefinition in workflowDefinitions)
         {
+            if (!materializerRegistry.IsMaterializerAvailable(workflowDefinition.MaterializerName))
+            {
+                var unavailableResult = new WorkflowGraphFindResult(workflowDefinition, null);
+                results.Add(unavailableResult);
+                continue;
+            }
+
             var cacheKey = cacheManager.CreateWorkflowVersionCacheKey(workflowDefinition.Id);
             var workflowGraph = await FindFromCacheAsync(
                 cacheKey,
-                async () => !materializerRegistry.IsMaterializerAvailable(workflowDefinition.MaterializerName) 
-                    ? null 
-                    : await MaterializeWorkflowAsync(workflowDefinition, cancellationToken),
+                async () => await MaterializeWorkflowAsync(workflowDefinition, cancellationToken),
                 wf => wf.Workflow.Identity.DefinitionId);
             
             var result = new WorkflowGraphFindResult(workflowDefinition, workflowGraph);
diff --git a/src/modules/Elsa.Workflows.Management/Services/MaterializerRegistry.cs b/src/modules/Elsa.Workflows.Management/Services/MaterializerRegistry.cs
@@ -3,18 +3,20 @@ namespace Elsa.Workflows.Management.Services;
 /// <inheritdoc />
 public class MaterializerRegistry(Func<IEnumerable<IWorkflowMaterializer>> materializers) : IMaterializerRegistry
 {
+    private readonly Lazy<IReadOnlyCollection<IWorkflowMaterializer>> _materializers = new(() => materializers().ToArray());
+
     /// <inheritdoc />
-    public IEnumerable<IWorkflowMaterializer> GetMaterializers() => materializers();
+    public IEnumerable<IWorkflowMaterializer> GetMaterializers() => _materializers.Value;
 
     /// <inheritdoc />
     public IWorkflowMaterializer? GetMaterializer(string name)
     {
-        return materializers().FirstOrDefault(x => x.Name == name);
+        return _materializers.Value.FirstOrDefault(x => x.Name == name);
     }
 
     /// <inheritdoc />
     public bool IsMaterializerAvailable(string name)
     {
-        return materializers().Any(x => x.Name == name);
+        return _materializers.Value.Any(x => x.Name == name);
     }
 }
diff --git a/src/modules/Elsa.Workflows.Management/Services/WorkflowDefinitionService.cs b/src/modules/Elsa.Workflows.Management/Services/WorkflowDefinitionService.cs
@@ -1,5 +1,6 @@
 using Elsa.Common.Models;
 using Elsa.Workflows.Management.Entities;
+using Elsa.Workflows.Management.Exceptions;
 using Elsa.Workflows.Management.Filters;
 using Elsa.Workflows.Management.Models;
 using Elsa.Workflows.Models;
@@ -21,7 +22,7 @@ public async Task<WorkflowGraph> MaterializeWorkflowAsync(WorkflowDefinition def
         var materializer = materializerRegistry.GetMaterializer(definition.MaterializerName);
 
         if (materializer == null)
-            throw new($"Materializer '{definition.MaterializerName}' not found. The materializer may be disabled or not registered.");
+            throw new WorkflowMaterializerNotFoundException(definition.MaterializerName);
 
         var workflow = await materializer.MaterializeAsync(definition, cancellationToken);
         return await workflowGraphBuilder.BuildAsync(workflow, cancellationToken);
diff --git a/src/modules/Elsa.Workflows.Runtime/Exceptions/WorkflowMaterializerNotFoundException.cs b/src/modules/Elsa.Workflows.Runtime/Exceptions/WorkflowMaterializerNotFoundException.cs
diff --git a/src/modules/Elsa.Workflows.Runtime/Services/LocalWorkflowClient.cs b/src/modules/Elsa.Workflows.Runtime/Services/LocalWorkflowClient.cs
@@ -1,5 +1,6 @@
 using Elsa.Workflows.Management;
 using Elsa.Workflows.Management.Entities;
+using Elsa.Workflows.Management.Exceptions;
 using Elsa.Workflows.Management.Filters;
 using Elsa.Workflows.Management.Mappers;
 using Elsa.Workflows.Management.Options;
@@ -208,8 +209,8 @@ private async Task<WorkflowGraph> GetWorkflowGraphAsync(WorkflowInstance workflo
     private async Task<WorkflowGraph> GetWorkflowGraphAsync(WorkflowDefinitionHandle definitionHandle, CancellationToken cancellationToken)
     {
         var result = await workflowDefinitionService.TryFindWorkflowGraphAsync(definitionHandle, cancellationToken);
-        if (!result.WorkflowDefinitionExists) throw new WorkflowGraphNotFoundException("Workflow graph not found.", definitionHandle);
-        if (!result.WorkflowGraphExists) throw new WorkflowMaterializerNotFoundException("Workflow materializer not found.", definitionHandle);
+        if (!result.WorkflowDefinitionExists) throw new WorkflowDefinitionNotFoundException("Workflow definition not found.", definitionHandle);
+        if (!result.WorkflowGraphExists) throw new WorkflowMaterializerNotFoundException(result.WorkflowDefinition!.MaterializerName);
         return result.WorkflowGraph!;
     }
 }
diff --git a/test/unit/Elsa.Workflows.Management.UnitTests/Services/CachingWorkflowDefinitionServiceTests.cs b/test/unit/Elsa.Workflows.Management.UnitTests/Services/CachingWorkflowDefinitionServiceTests.cs
@@ -481,7 +481,7 @@ private void SetupCacheManager()
     private WorkflowGraphFindResult CreateWorkflowGraphFindResult(string definitionId = "def-1", string materializerName = "materializer")
     {
         var definition = TestHelpers.CreateWorkflowDefinition(definitionId, materializerName);
-        var (workflow, workflowGraph) = CreateWorkflowAndGraph(definitionId);
+        var (_, workflowGraph) = CreateWorkflowAndGraph(definitionId);
         return new WorkflowGraphFindResult(definition, workflowGraph);
     }
 }
diff --git a/test/unit/Elsa.Workflows.Management.UnitTests/Services/MaterializerRegistryTests.cs b/test/unit/Elsa.Workflows.Management.UnitTests/Services/MaterializerRegistryTests.cs
@@ -0,0 +1,179 @@
+using Elsa.Workflows.Management;
+using Elsa.Workflows.Management.Services;
+using NSubstitute;
+
+namespace Elsa.Workflows.Management.UnitTests.Services;
+
+public class MaterializerRegistryTests
+{
+    [Fact]
+    public void GetMaterializers_Should_Return_All_Materializers()
+    {
+        // Arrange
+        var materializer1 = CreateMaterializer("materializer1");
+        var materializer2 = CreateMaterializer("materializer2");
+        var registry = CreateRegistry(materializer1, materializer2);
+
+        // Act
+        var result = registry.GetMaterializers().ToList();
+
+        // Assert
+        Assert.Equal(2, result.Count);
+        Assert.Contains(result, m => m.Name == "materializer1");
+        Assert.Contains(result, m => m.Name == "materializer2");
+    }
+
+    [Fact]
+    public void GetMaterializer_Should_Return_Materializer_By_Name()
+    {
+        // Arrange
+        var materializer1 = CreateMaterializer("test-materializer");
+        var materializer2 = CreateMaterializer("other-materializer");
+        var registry = CreateRegistry(materializer1, materializer2);
+
+        // Act
+        var result = registry.GetMaterializer("test-materializer");
+
+        // Assert
+        Assert.NotNull(result);
+        Assert.Equal("test-materializer", result.Name);
+    }
+
+    [Theory]
+    [InlineData("non-existent")]
+    [InlineData("")]
+    [InlineData("wrong-name")]
+    public void GetMaterializer_Should_Return_Null_When_Not_Found(string name)
+    {
+        // Arrange
+        var materializer = CreateMaterializer("existing-materializer");
+        var registry = CreateRegistry(materializer);
+
+        // Act
+        var result = registry.GetMaterializer(name);
+
+        // Assert
+        Assert.Null(result);
+    }
+
+    [Fact]
+    public void IsMaterializerAvailable_Should_Return_True_When_Materializer_Exists()
+    {
+        // Arrange
+        var materializer = CreateMaterializer("available-materializer");
+        var registry = CreateRegistry(materializer);
+
+        // Act
+        var result = registry.IsMaterializerAvailable("available-materializer");
+
+        // Assert
+        Assert.True(result);
+    }
+
+    [Theory]
+    [InlineData("not-available")]
+    [InlineData("")]
+    [InlineData("wrong-name")]
+    public void IsMaterializerAvailable_Should_Return_False_When_Materializer_Does_Not_Exist(string name)
+    {
+        // Arrange
+        var materializer = CreateMaterializer("existing-materializer");
+        var registry = CreateRegistry(materializer);
+
+        // Act
+        var result = registry.IsMaterializerAvailable(name);
+
+        // Assert
+        Assert.False(result);
+    }
+
+    [Fact]
+    public void Should_Handle_Empty_Materializer_Collection()
+    {
+        // Arrange
+        var registry = CreateRegistry();
+
+        // Act
+        var allMaterializers = registry.GetMaterializers().ToList();
+        var foundMaterializer = registry.GetMaterializer("any-name");
+        var isAvailable = registry.IsMaterializerAvailable("any-name");
+
+        // Assert
+        Assert.Empty(allMaterializers);
+        Assert.Null(foundMaterializer);
+        Assert.False(isAvailable);
+    }
+
+    [Fact]
+    public void Should_Cache_Materializers_On_First_Access()
+    {
+        // Arrange
+        var callCount = 0;
+        IEnumerable<IWorkflowMaterializer> MaterializersFactory()
+        {
+            callCount++;
+            return new[] { CreateMaterializer("test") };
+        }
+
+        var registry = new MaterializerRegistry(MaterializersFactory);
+
+        // Act
+        var first = registry.GetMaterializers().ToList();
+        var second = registry.GetMaterializers().ToList();
+        var third = registry.GetMaterializer("test");
+        var fourth = registry.IsMaterializerAvailable("test");
+
+        // Assert
+        Assert.Single(first);
+        Assert.Single(second);
+        Assert.NotNull(third);
+        Assert.True(fourth);
+        Assert.Equal(1, callCount); // Factory should only be called once
+    }
+
+    [Fact]
+    public void GetMaterializers_Should_Allow_Multiple_Enumerations()
+    {
+        // Arrange
+        var materializer1 = CreateMaterializer("mat1");
+        var materializer2 = CreateMaterializer("mat2");
+        var registry = CreateRegistry(materializer1, materializer2);
+
+        // Act
+        var first = registry.GetMaterializers().ToList();
+        var second = registry.GetMaterializers().ToList();
+
+        // Assert
+        Assert.Equal(2, first.Count);
+        Assert.Equal(2, second.Count);
+        Assert.Equal(first.Count, second.Count);
+    }
+
+    [Fact]
+    public void GetMaterializer_Should_Return_First_When_Multiple_Materializers_With_Same_Name()
+    {
+        // Arrange - FirstOrDefault returns the first match
+        var materializer1 = CreateMaterializer("duplicate-name");
+        var materializer2 = CreateMaterializer("duplicate-name");
+        var registry = CreateRegistry(materializer1, materializer2);
+
+        // Act
+        var result = registry.GetMaterializer("duplicate-name");
+
+        // Assert
+        Assert.NotNull(result);
+        Assert.Same(materializer1, result); // FirstOrDefault returns the first match
+    }
+
+    private static IWorkflowMaterializer CreateMaterializer(string name)
+    {
+        var materializer = Substitute.For<IWorkflowMaterializer>();
+        materializer.Name.Returns(name);
+        return materializer;
+    }
+
+    private static MaterializerRegistry CreateRegistry(params IWorkflowMaterializer[] materializers)
+    {
+        return new MaterializerRegistry(() => materializers);
+    }
+}
diff --git a/test/unit/Elsa.Workflows.Management.UnitTests/Services/WorkflowDefinitionServiceTests.cs b/test/unit/Elsa.Workflows.Management.UnitTests/Services/WorkflowDefinitionServiceTests.cs
@@ -1,6 +1,7 @@
 using Elsa.Common.Models;
 using Elsa.Workflows.Activities;
 using Elsa.Workflows.Management.Entities;
+using Elsa.Workflows.Management.Exceptions;
 using Elsa.Workflows.Management.Filters;
 using Elsa.Workflows.Management.Services;
 using Elsa.Workflows.Management.UnitTests.Helpers;
@@ -22,7 +23,7 @@ public async Task MaterializeWorkflowAsync_WithValidMaterializer_ReturnsMaterial
     {
         // Arrange
         var definition = TestHelpers.CreateWorkflowDefinition("def-1", "test-materializer");
-        var (workflow, workflowGraph) = SetupMaterializerAndGraphBuilder(definition);
+        var (_, workflowGraph) = SetupMaterializerAndGraphBuilder(definition);
 
         var service = CreateService();
 
@@ -43,8 +44,7 @@ public async Task MaterializeWorkflowAsync_WithMissingMaterializer_ThrowsExcepti
         var service = CreateService();
 
         // Act & Assert
-        var exception = await Assert.ThrowsAsync<Exception>(() => service.MaterializeWorkflowAsync(definition));
-        Assert.Contains("Materializer 'missing-materializer' not found", exception.Message);
+        await Assert.ThrowsAsync<WorkflowMaterializerNotFoundException>(() => service.MaterializeWorkflowAsync(definition));
     }
 
     [Fact]
diff --git a/test/unit/Elsa.Workflows.Runtime.UnitTests/Services/BackgroundWorkflowCancellationDispatcherTests.cs b/test/unit/Elsa.Workflows.Runtime.UnitTests/Services/BackgroundWorkflowCancellationDispatcherTests.cs
diff --git a/test/unit/Elsa.Workflows.Runtime.UnitTests/Services/DefaultWorkflowDefinitionStorePopulatorTests.cs b/test/unit/Elsa.Workflows.Runtime.UnitTests/Services/DefaultWorkflowDefinitionStorePopulatorTests.cs
diff --git a/test/unit/Elsa.Workflows.Runtime.UnitTests/Services/LocalWorkflowClientTests.cs b/test/unit/Elsa.Workflows.Runtime.UnitTests/Services/LocalWorkflowClientTests.cs

Original file line number	Diff line number	Diff line change
`@@ -39,6 +39,6 @@ public ValueTask TriggerTokenAsync(string key, CancellationToken cancellationTok`
`39`	`39`	`/// <exception cref="InvalidOperationException">Thrown if the factory function returns null.</exception>`
`40`	`40`	`public async Task<TItem> GetOrCreateAsync<TItem>(object key, Func<ICacheEntry, Task<TItem>> factory)`
`41`	`41`	`{`
`42`		`- return await memoryCache.GetOrCreateAsync(key, async entry => await factory(entry)) ?? throw new InvalidOperationException("Factory returned null.");`
	`42`	`+ return await memoryCache.GetOrCreateAsync(key, async entry => await factory(entry)) ?? throw new InvalidOperationException($"Factory returned null for cache key: {key}.");`
`43`	`43`	`}`
`44`	`44`	`}`
Original file line number	Diff line number	Diff line change
`@@ -31,7 +31,7 @@ public override async Task HandleAsync(Request request, CancellationToken cancel`
`31`	`31`	`if (!materializerRegistry.IsMaterializerAvailable(definition.MaterializerName))`
`32`	`32`	`{`
`33`	`33`	`AddError($"The workflow materializer '{definition.MaterializerName}' is not available. The materializer may be disabled or not registered.");`
`34`		`- await Send.ErrorsAsync(StatusCodes.Status503ServiceUnavailable, cancellationToken);`
	`34`	`+ await Send.ErrorsAsync(StatusCodes.Status422UnprocessableEntity, cancellationToken);`
`35`	`35`	`return;`
`36`	`36`	`}`
`37`	`37`
Original file line number	Diff line number	Diff line change
`@@ -3,18 +3,20 @@ namespace Elsa.Workflows.Management.Services;`
`3`	`3`	`/// <inheritdoc />`
`4`	`4`	`public class MaterializerRegistry(Func<IEnumerable<IWorkflowMaterializer>> materializers) : IMaterializerRegistry`
`5`	`5`	`{`
	`6`	`+ private readonly Lazy<IReadOnlyCollection<IWorkflowMaterializer>> _materializers = new(() => materializers().ToArray());`
	`7`	`+`
`6`	`8`	`/// <inheritdoc />`
`7`		`- public IEnumerable<IWorkflowMaterializer> GetMaterializers() => materializers();`
	`9`	`+ public IEnumerable<IWorkflowMaterializer> GetMaterializers() => _materializers.Value;`
`8`	`10`
`9`	`11`	`/// <inheritdoc />`
`10`	`12`	`public IWorkflowMaterializer? GetMaterializer(string name)`
`11`	`13`	`{`
`12`		`- return materializers().FirstOrDefault(x => x.Name == name);`
	`14`	`+ return _materializers.Value.FirstOrDefault(x => x.Name == name);`
`13`	`15`	`}`
`14`	`16`
`15`	`17`	`/// <inheritdoc />`
`16`	`18`	`public bool IsMaterializerAvailable(string name)`
`17`	`19`	`{`
`18`		`- return materializers().Any(x => x.Name == name);`
	`20`	`+ return _materializers.Value.Any(x => x.Name == name);`
`19`	`21`	`}`
`20`	`22`	`}`
Original file line number	Diff line number	Diff line change
`@@ -1,5 +1,6 @@`
`1`	`1`	`using Elsa.Workflows.Management;`
`2`	`2`	`using Elsa.Workflows.Management.Entities;`
	`3`	`+using Elsa.Workflows.Management.Exceptions;`
`3`	`4`	`using Elsa.Workflows.Management.Filters;`
`4`	`5`	`using Elsa.Workflows.Management.Mappers;`
`5`	`6`	`using Elsa.Workflows.Management.Options;`
`@@ -208,8 +209,8 @@ private async Task<WorkflowGraph> GetWorkflowGraphAsync(WorkflowInstance workflo`
`208`	`209`	`private async Task<WorkflowGraph> GetWorkflowGraphAsync(WorkflowDefinitionHandle definitionHandle, CancellationToken cancellationToken)`
`209`	`210`	`{`
`210`	`211`	`var result = await workflowDefinitionService.TryFindWorkflowGraphAsync(definitionHandle, cancellationToken);`
`211`		`- if (!result.WorkflowDefinitionExists) throw new WorkflowGraphNotFoundException("Workflow graph not found.", definitionHandle);`
`212`		`- if (!result.WorkflowGraphExists) throw new WorkflowMaterializerNotFoundException("Workflow materializer not found.", definitionHandle);`
	`212`	`+ if (!result.WorkflowDefinitionExists) throw new WorkflowDefinitionNotFoundException("Workflow definition not found.", definitionHandle);`
	`213`	`+ if (!result.WorkflowGraphExists) throw new WorkflowMaterializerNotFoundException(result.WorkflowDefinition!.MaterializerName);`
`213`	`214`	`return result.WorkflowGraph!;`
`214`	`215`	`}`
`215`	`216`	`}`
Original file line number	Diff line number	Diff line change
`@@ -481,7 +481,7 @@ private void SetupCacheManager()`
`481`	`481`	`private WorkflowGraphFindResult CreateWorkflowGraphFindResult(string definitionId = "def-1", string materializerName = "materializer")`
`482`	`482`	`{`
`483`	`483`	`var definition = TestHelpers.CreateWorkflowDefinition(definitionId, materializerName);`
`484`		`- var (workflow, workflowGraph) = CreateWorkflowAndGraph(definitionId);`
	`484`	`+ var (_, workflowGraph) = CreateWorkflowAndGraph(definitionId);`
`485`	`485`	`return new WorkflowGraphFindResult(definition, workflowGraph);`
`486`	`486`	`}`
`487`	`487`	`}`