Add idempotency tests for trigger indexing and fix serialization mismatch in WorkflowTriggerEqualityComparer

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<StoredTrigger>
 {
     private readonly JsonSerializerOptions _settings;
-    
+
     /// <summary>
     /// Initializes a new instance of the <see cref="WorkflowTriggerEqualityComparer"/> class.
     /// </summary>
     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);
     }
-}
+
+    /// <summary>
+    /// 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.
+    /// </summary>
+    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);
+    }
+}
+

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;
+
+/// <summary>
+/// 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.
+/// </summary>
+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<ITriggerIndexer>();
+
+        // 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<ITriggerIndexer>();
+
+        // 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<WorkflowDefinition> SaveWorkflowDefinitionAsync(Workflow workflow)
+    {
+        var workflowDefinitionStore = Scope.ServiceProvider.GetRequiredService<IWorkflowDefinitionStore>();
+        var activitySerializer = Scope.ServiceProvider.GetRequiredService<IActivitySerializer>();
+
+        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<List<StoredTrigger>> GetTriggersAsync(string workflowDefinitionId)
+    {
+        var triggerStore = Scope.ServiceProvider.GetRequiredService<ITriggerStore>();
+        return (await triggerStore.FindManyAsync(new() { WorkflowDefinitionId = workflowDefinitionId })).ToList();
+    }
+}
+

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;
+
+/// <summary>
+/// Regression tests for the payload serialization mismatch between
+/// <see cref="WorkflowTriggerEqualityComparer"/> 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.
+/// </summary>
+public class WorkflowTriggerEqualityComparerTests
+{
+    /// <summary>
+    /// A simple payload class that mimics real trigger payloads like HttpEndpointBookmarkPayload.
+    /// </summary>
+    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<StoredTrigger> { existingTrigger };
+        var newTriggers = new List<StoredTrigger> { 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<object> returns a JsonElement
+        var roundTrippedPayload = JsonSerializer.Deserialize<object>(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);
+    }
+
+    /// <summary>
+    /// IPayloadSerializer options: camelCase with case-insensitive deserialization.
+    /// </summary>
+    private static readonly JsonSerializerOptions PayloadSerializerOptions = new()
+    {
+        PropertyNamingPolicy = JsonNamingPolicy.CamelCase,
+        PropertyNameCaseInsensitive = true,
+        DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull,
+    };
+
+    /// <summary>
+    /// Comparer options used by WorkflowTriggerEqualityComparer (without naming policy).
+    /// </summary>
+    private static readonly JsonSerializerOptions ComparerOptions = new()
+    {
+        IncludeFields = true,
+        PropertyNameCaseInsensitive = true
+    };
+
+    /// <summary>
+    /// Simulates the IPayloadSerializer round-trip: serialize with camelCase, then deserialize to object.
+    /// This is what happens in TriggerStore.OnSaveAsync -> DB -> TriggerStore.OnLoadAsync.
+    /// </summary>
+    private static object SimulatePayloadRoundTrip(object payload)
+    {
+        // Serialize (OnSaveAsync)
+        var json = JsonSerializer.Serialize(payload, PayloadSerializerOptions);
+
+        // Deserialize back to object (OnLoadAsync calls Deserialize<object>)
+        // System.Text.Json deserializes to JsonElement when target type is object.
+        var deserialized = JsonSerializer.Deserialize<object>(json, PayloadSerializerOptions);
+
+        return deserialized!;
+    }
+
+    /// <summary>
+    /// Creates a StoredTrigger with default values that can be overridden.
+    /// </summary>
+    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
+    };
+}
+