diff --git a/src/modules/Elsa.Workflows.Runtime/Comparers/WorkflowTriggerEqualityComparer.cs b/src/modules/Elsa.Workflows.Runtime/Comparers/WorkflowTriggerEqualityComparer.cs index 100f91dafe..68a20e660c 100644 --- a/src/modules/Elsa.Workflows.Runtime/Comparers/WorkflowTriggerEqualityComparer.cs +++ b/src/modules/Elsa.Workflows.Runtime/Comparers/WorkflowTriggerEqualityComparer.cs @@ -1,4 +1,6 @@ using System.Text.Json; +using System.Text.Json.Serialization; +using System.Text.Json.Serialization.Metadata; using Elsa.Expressions.Services; using Elsa.Workflows.Runtime.Entities; using Elsa.Workflows.Serialization.Converters; @@ -11,19 +13,30 @@ namespace Elsa.Workflows.Runtime.Comparers; public class WorkflowTriggerEqualityComparer : IEqualityComparer { private readonly JsonSerializerOptions _settings; - + /// /// Initializes a new instance of the class. /// public WorkflowTriggerEqualityComparer() { - _settings = new JsonSerializerOptions + _settings = new() { // Enables serialization of ValueTuples, which use fields instead of properties. IncludeFields = true, - PropertyNameCaseInsensitive = true + PropertyNameCaseInsensitive = true, + // Use camelCase to match IPayloadSerializer, which stores payloads using camelCase. + // Without this, fresh payloads (typed CLR objects) serialize with PascalCase while + // DB-loaded payloads (JsonElement) preserve their stored camelCase keys, causing + // the diff to always report all triggers as changed. + PropertyNamingPolicy = JsonNamingPolicy.CamelCase, + DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull, }; - + + // Mirror the converters used by IPayloadSerializer so that enum, TimeSpan, and + // polymorphic object properties serialize identically to their stored representation. + _settings.Converters.Add(new JsonStringEnumConverter()); + _settings.Converters.Add(JsonMetadataServices.TimeSpanConverter); + _settings.Converters.Add(new PolymorphicObjectConverterFactory()); _settings.Converters.Add(new TypeJsonConverter(WellKnownTypeRegistry.CreateDefault())); } @@ -44,9 +57,13 @@ public int GetHashCode(StoredTrigger obj) private string Serialize(StoredTrigger storedTrigger) { + // Normalize the payload to a canonical JSON string so that both typed CLR objects + // and JsonElement instances (from DB round-trips) produce identical output. + var normalizedPayload = NormalizePayload(storedTrigger.Payload); + var input = new { - storedTrigger.Payload, + Payload = normalizedPayload, storedTrigger.Name, storedTrigger.ActivityId, storedTrigger.WorkflowDefinitionId, @@ -55,4 +72,21 @@ private string Serialize(StoredTrigger storedTrigger) }; return JsonSerializer.Serialize(input, _settings); } -} \ No newline at end of file + + /// + /// Normalizes a payload to a canonical JSON string representation. + /// This ensures that typed CLR objects and JsonElements (which preserve their original + /// property name casing from DB storage) produce identical output when compared. + /// + private string? NormalizePayload(object? payload) + { + if (payload == null) + return null; + + // Serialize to camelCase JSON — this normalizes both: + // - CLR objects (whose PascalCase properties get converted to camelCase) + // - JsonElement values (whose camelCase keys are preserved as-is) + return JsonSerializer.Serialize(payload, _settings); + } +} + diff --git a/test/component/Elsa.Workflows.ComponentTests/Scenarios/TriggerIndexingIdempotency/TriggerIndexingIdempotencyTests.cs b/test/component/Elsa.Workflows.ComponentTests/Scenarios/TriggerIndexingIdempotency/TriggerIndexingIdempotencyTests.cs new file mode 100644 index 0000000000..cf3923b045 --- /dev/null +++ b/test/component/Elsa.Workflows.ComponentTests/Scenarios/TriggerIndexingIdempotency/TriggerIndexingIdempotencyTests.cs @@ -0,0 +1,118 @@ +using Elsa.Http; +using Elsa.Workflows.Activities; +using Elsa.Workflows.ComponentTests.Abstractions; +using Elsa.Workflows.ComponentTests.Fixtures; +using Elsa.Workflows.Management; +using Elsa.Workflows.Management.Entities; +using Elsa.Workflows.Runtime; +using Elsa.Workflows.Runtime.Entities; +using Microsoft.Extensions.DependencyInjection; + +namespace Elsa.Workflows.ComponentTests.Scenarios.TriggerIndexingIdempotency; + +/// +/// Tests that trigger indexing is idempotent: re-indexing an unchanged workflow should +/// not produce any trigger changes (no delete-reinsert cycle). +/// +/// The WorkflowTriggerEqualityComparer must use consistent serialization settings +/// (matching IPayloadSerializer's camelCase convention) so that freshly computed triggers +/// and DB-loaded triggers are correctly recognized as equal. Without this, the diff +/// reports all triggers as changed on every indexing call, causing unnecessary churn +/// and creating a window for unique constraint violations in multi-pod environments. +/// +public class TriggerIndexingIdempotencyTests(App app) : AppComponentTest(app) +{ + [Fact(DisplayName = "Re-indexing an unchanged workflow should not produce any trigger changes")] + public async Task ReIndexingUnchangedWorkflow_ShouldBeIdempotent() + { + // Arrange + var workflow = CreateTestWorkflow(); + var definition = await SaveWorkflowDefinitionAsync(workflow); + var indexer = Scope.ServiceProvider.GetRequiredService(); + + // Act: First indexing — triggers are inserted. + var firstResult = await indexer.IndexTriggersAsync(definition); + Assert.Single(firstResult.AddedTriggers); + Assert.Empty(firstResult.RemovedTriggers); + + // Act: Second indexing — triggers already exist in the DB. + var secondResult = await indexer.IndexTriggersAsync(definition); + + // Assert: The second indexing should be a no-op. + Assert.Empty(secondResult.AddedTriggers); + Assert.Empty(secondResult.RemovedTriggers); + Assert.Single(secondResult.UnchangedTriggers); + } + + [Fact(DisplayName = "Re-indexing should preserve trigger IDs when nothing changed")] + public async Task ReIndexingUnchangedWorkflow_ShouldPreserveTriggerIds() + { + // Arrange + var workflow = CreateTestWorkflow(); + var definition = await SaveWorkflowDefinitionAsync(workflow); + var indexer = Scope.ServiceProvider.GetRequiredService(); + + // Act: Index twice. + await indexer.IndexTriggersAsync(definition); + var originalTriggerId = (await GetTriggersAsync(workflow.Identity.DefinitionId)).Single().Id; + + await indexer.IndexTriggersAsync(definition); + var triggerIdAfterSecondIndex = (await GetTriggersAsync(workflow.Identity.DefinitionId)).Single().Id; + + // Assert: The trigger ID should be preserved — no delete-reinsert happened. + Assert.Equal(originalTriggerId, triggerIdAfterSecondIndex); + } + + private static Workflow CreateTestWorkflow() + { + var definitionId = Guid.NewGuid().ToString(); + var workflowId = Guid.NewGuid().ToString(); + + return new() + { + Identity = new(definitionId, 1, workflowId), + Root = new Sequence + { + Activities = + { + new HttpEndpoint + { + Path = new($"/blob-import-test/{Guid.NewGuid()}"), + SupportedMethods = new(["GET"]), + CanStartWorkflow = true + } + } + }, + Publication = new(IsLatest: true, IsPublished: true) + }; + } + + private async Task SaveWorkflowDefinitionAsync(Workflow workflow) + { + var workflowDefinitionStore = Scope.ServiceProvider.GetRequiredService(); + var activitySerializer = Scope.ServiceProvider.GetRequiredService(); + + var definition = new WorkflowDefinition + { + Id = workflow.Identity.Id, + DefinitionId = workflow.Identity.DefinitionId, + Version = workflow.Identity.Version, + IsLatest = true, + IsPublished = true, + Name = "Blob Import Test Workflow", + Description = "Simulates a workflow imported from blob storage", + MaterializerName = "Json", + StringData = activitySerializer.Serialize(workflow.Root) + }; + + await workflowDefinitionStore.SaveAsync(definition); + return definition; + } + + private async Task> GetTriggersAsync(string workflowDefinitionId) + { + var triggerStore = Scope.ServiceProvider.GetRequiredService(); + return (await triggerStore.FindManyAsync(new() { WorkflowDefinitionId = workflowDefinitionId })).ToList(); + } +} + diff --git a/test/unit/Elsa.Workflows.Runtime.UnitTests/Comparers/WorkflowTriggerEqualityComparerTests.cs b/test/unit/Elsa.Workflows.Runtime.UnitTests/Comparers/WorkflowTriggerEqualityComparerTests.cs new file mode 100644 index 0000000000..2a2d95f869 --- /dev/null +++ b/test/unit/Elsa.Workflows.Runtime.UnitTests/Comparers/WorkflowTriggerEqualityComparerTests.cs @@ -0,0 +1,166 @@ +using System.Text.Json; +using System.Text.Json.Serialization; +using Elsa.Workflows.Helpers; +using Elsa.Workflows.Runtime.Comparers; +using Elsa.Workflows.Runtime.Entities; + +namespace Elsa.Workflows.Runtime.UnitTests.Comparers; + +/// +/// Regression tests for the payload serialization mismatch between +/// and IPayloadSerializer. +/// +/// Before the fix, the comparer used PascalCase (default) while IPayloadSerializer used camelCase. +/// This caused the trigger diff to always report existing triggers as "removed" and freshly +/// computed triggers as "added", even when they represented the same logical trigger. +/// The resulting delete-then-reinsert churn on every indexing call created a window for +/// unique constraint violations in multi-pod environments. +/// +/// The fix ensures the comparer uses the same camelCase PropertyNamingPolicy and normalizes +/// payloads to canonical JSON strings before comparison. +/// +public class WorkflowTriggerEqualityComparerTests +{ + /// + /// A simple payload class that mimics real trigger payloads like HttpEndpointBookmarkPayload. + /// + private record TestPayload(string Path, string Method); + + [Fact(DisplayName = "Fresh and round-tripped triggers with identical logical content should be considered equal")] + public void FreshAndRoundTrippedTriggers_ShouldBeEqual() + { + // Arrange: a freshly computed trigger (as produced by TriggerIndexer.CreateWorkflowTriggersAsync) + var freshPayload = new TestPayload("/api/test", "GET"); + var freshTrigger = CreateTrigger("trigger-1", freshPayload); + + // Arrange: the same trigger after a DB round-trip (save via IPayloadSerializer, then load) + var roundTrippedPayload = SimulatePayloadRoundTrip(freshPayload); + var loadedTrigger = CreateTrigger("trigger-1", roundTrippedPayload); + + var comparer = new WorkflowTriggerEqualityComparer(); + + // Act + var areEqual = comparer.Equals(freshTrigger, loadedTrigger); + + // Assert: these should be equal since they represent the same logical trigger. + // Before the fix, they were NOT equal because: + // - freshPayload serialized as: {"Path":"/api/test","Method":"GET"} (PascalCase) + // - roundTrippedPayload (a JsonElement) serialized as: {"path":"/api/test","method":"GET"} (camelCase preserved from DB) + Assert.True(areEqual, + "Fresh trigger and DB-round-tripped trigger should be considered equal. " + + "The payload serialization mismatch between WorkflowTriggerEqualityComparer (PascalCase) " + + "and IPayloadSerializer (camelCase) causes them to differ."); + } + + [Fact(DisplayName = "Diff should produce empty Added/Removed sets when comparing fresh triggers against their round-tripped equivalents")] + public void Diff_FreshVsRoundTripped_ShouldProduceNoDifferences() + { + // Arrange: simulate Pod A having indexed triggers (now in DB, round-tripped) + var payload = new TestPayload("/api/orders", "POST"); + var roundTrippedPayload = SimulatePayloadRoundTrip(payload); + + var existingTrigger = CreateTrigger("existing-id-from-pod-a", roundTrippedPayload, hash: "hash-1"); + + // Arrange: Pod B freshly computes the same trigger (as TriggerIndexer would) + var freshTrigger = CreateTrigger("new-id-from-pod-b", payload, hash: "hash-1"); // Different ID, same logical trigger + + var currentTriggers = new List { existingTrigger }; + var newTriggers = new List { freshTrigger }; + + // Act: this is exactly what TriggerIndexer.IndexTriggersInternalAsync does + var diff = Diff.For(currentTriggers, newTriggers, new WorkflowTriggerEqualityComparer()); + + // Assert: the diff should find no changes. + // Before the fix, it reported Removed=[existingTrigger] and Added=[freshTrigger] + // because the payload JSON representations differed (camelCase vs PascalCase). + Assert.Empty(diff.Added); + Assert.Empty(diff.Removed); + Assert.Single(diff.Unchanged); + } + + [Fact(DisplayName = "Documents the underlying System.Text.Json casing behavior that necessitated the fix")] + public void PayloadSerializationMismatch_ProducesDifferentJson() + { + // This test documents the raw System.Text.Json behavior difference. + // Without PropertyNamingPolicy = CamelCase + payload normalization, CLR objects + // and JsonElement values produce different JSON, which was the root cause. + var payload = new TestPayload("/api/test", "GET"); + + // What WorkflowTriggerEqualityComparer produces for a fresh payload (PascalCase, no naming policy): + var freshJson = JsonSerializer.Serialize(payload, ComparerOptions); + // Result: {"Path":"/api/test","Method":"GET"} + + // What IPayloadSerializer stores in the DB (camelCase): + var storedJson = JsonSerializer.Serialize(payload, PayloadSerializerOptions); + // Result: {"path":"/api/test","method":"GET"} + + // After loading from DB, Deserialize returns a JsonElement + var roundTrippedPayload = JsonSerializer.Deserialize(storedJson, PayloadSerializerOptions); + + // When WorkflowTriggerEqualityComparer serializes the round-tripped payload: + var roundTrippedJson = JsonSerializer.Serialize(roundTrippedPayload, ComparerOptions); + // Result: {"path":"/api/test","method":"GET"} — camelCase preserved from JsonElement! + + // The mismatch: + Assert.NotEqual(freshJson, roundTrippedJson); // This proves the bug exists + Assert.Equal("{\"Path\":\"/api/test\",\"Method\":\"GET\"}", freshJson); + Assert.Equal("{\"path\":\"/api/test\",\"method\":\"GET\"}", roundTrippedJson); + } + + /// + /// IPayloadSerializer options: camelCase with case-insensitive deserialization. + /// + private static readonly JsonSerializerOptions PayloadSerializerOptions = new() + { + PropertyNamingPolicy = JsonNamingPolicy.CamelCase, + PropertyNameCaseInsensitive = true, + DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull, + }; + + /// + /// Comparer options used by WorkflowTriggerEqualityComparer (without naming policy). + /// + private static readonly JsonSerializerOptions ComparerOptions = new() + { + IncludeFields = true, + PropertyNameCaseInsensitive = true + }; + + /// + /// Simulates the IPayloadSerializer round-trip: serialize with camelCase, then deserialize to object. + /// This is what happens in TriggerStore.OnSaveAsync -> DB -> TriggerStore.OnLoadAsync. + /// + private static object SimulatePayloadRoundTrip(object payload) + { + // Serialize (OnSaveAsync) + var json = JsonSerializer.Serialize(payload, PayloadSerializerOptions); + + // Deserialize back to object (OnLoadAsync calls Deserialize) + // System.Text.Json deserializes to JsonElement when target type is object. + var deserialized = JsonSerializer.Deserialize(json, PayloadSerializerOptions); + + return deserialized!; + } + + /// + /// Creates a StoredTrigger with default values that can be overridden. + /// + private static StoredTrigger CreateTrigger( + string id, + object payload, + string workflowDefinitionId = "workflow-1", + string workflowDefinitionVersionId = "workflow-1:v1", + string name = "Elsa.HttpEndpoint", + string activityId = "activity-1", + string hash = "some-hash") => new() + { + Id = id, + WorkflowDefinitionId = workflowDefinitionId, + WorkflowDefinitionVersionId = workflowDefinitionVersionId, + Name = name, + ActivityId = activityId, + Hash = hash, + Payload = payload + }; +} +