Add idempotency tests for trigger indexing and fix serialization mismatch in WorkflowTriggerEqualityComparer
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
This commit is contained in:
Sipke Schoorstra
parent
689f19f2f2
commit
51d3f70b42
|
|
@ -1,4 +1,5 @@
|
|||
using System.Text.Json;
|
||||
using System.Text.Json.Serialization;
|
||||
using Elsa.Expressions.Services;
|
||||
using Elsa.Workflows.Runtime.Entities;
|
||||
using Elsa.Workflows.Serialization.Converters;
|
||||
|
|
@ -21,7 +22,13 @@ public class WorkflowTriggerEqualityComparer : IEqualityComparer<StoredTrigger>
|
|||
{
|
||||
// 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,
|
||||
};
|
||||
|
||||
_settings.Converters.Add(new TypeJsonConverter(WellKnownTypeRegistry.CreateDefault()));
|
||||
|
|
@ -44,9 +51,13 @@ public class WorkflowTriggerEqualityComparer : IEqualityComparer<StoredTrigger>
|
|||
|
||||
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 +66,20 @@ public class WorkflowTriggerEqualityComparer : IEqualityComparer<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);
|
||||
}
|
||||
}
|
||||
|
|
@ -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();
|
||||
}
|
||||
}
|
||||
|
||||
|
|
@ -0,0 +1,181 @@
|
|||
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);
|
||||
|
||||
/// <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)
|
||||
{
|
||||
// IPayloadSerializer uses camelCase and various converters
|
||||
var serializerOptions = new JsonSerializerOptions
|
||||
{
|
||||
PropertyNamingPolicy = JsonNamingPolicy.CamelCase,
|
||||
PropertyNameCaseInsensitive = true,
|
||||
DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull,
|
||||
};
|
||||
|
||||
// Serialize (OnSaveAsync)
|
||||
var json = JsonSerializer.Serialize(payload, serializerOptions);
|
||||
|
||||
// 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, serializerOptions);
|
||||
|
||||
return deserialized!;
|
||||
}
|
||||
|
||||
[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 = new StoredTrigger
|
||||
{
|
||||
Id = "trigger-1",
|
||||
WorkflowDefinitionId = "workflow-1",
|
||||
WorkflowDefinitionVersionId = "workflow-1:v1",
|
||||
Name = "Elsa.HttpEndpoint",
|
||||
ActivityId = "activity-1",
|
||||
Hash = "some-hash",
|
||||
Payload = freshPayload
|
||||
};
|
||||
|
||||
// Arrange: the same trigger after a DB round-trip (save via IPayloadSerializer, then load)
|
||||
var roundTrippedPayload = SimulatePayloadRoundTrip(freshPayload);
|
||||
var loadedTrigger = new StoredTrigger
|
||||
{
|
||||
Id = "trigger-1",
|
||||
WorkflowDefinitionId = "workflow-1",
|
||||
WorkflowDefinitionVersionId = "workflow-1:v1",
|
||||
Name = "Elsa.HttpEndpoint",
|
||||
ActivityId = "activity-1",
|
||||
Hash = "some-hash",
|
||||
Payload = 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 = new StoredTrigger
|
||||
{
|
||||
Id = "existing-id-from-pod-a",
|
||||
WorkflowDefinitionId = "workflow-1",
|
||||
WorkflowDefinitionVersionId = "workflow-1:v1",
|
||||
Name = "Elsa.HttpEndpoint",
|
||||
ActivityId = "activity-1",
|
||||
Hash = "hash-1",
|
||||
Payload = roundTrippedPayload
|
||||
};
|
||||
|
||||
// Arrange: Pod B freshly computes the same trigger (as TriggerIndexer would)
|
||||
var freshTrigger = new StoredTrigger
|
||||
{
|
||||
Id = "new-id-from-pod-b", // Different ID, same logical trigger
|
||||
WorkflowDefinitionId = "workflow-1",
|
||||
WorkflowDefinitionVersionId = "workflow-1:v1",
|
||||
Name = "Elsa.HttpEndpoint",
|
||||
ActivityId = "activity-1",
|
||||
Hash = "hash-1",
|
||||
Payload = payload // Original typed object
|
||||
};
|
||||
|
||||
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 comparerOptions = new JsonSerializerOptions
|
||||
{
|
||||
IncludeFields = true,
|
||||
PropertyNameCaseInsensitive = true
|
||||
};
|
||||
var freshJson = JsonSerializer.Serialize(payload, comparerOptions);
|
||||
// Result: {"Path":"/api/test","Method":"GET"}
|
||||
|
||||
// What IPayloadSerializer stores in the DB (camelCase):
|
||||
var payloadSerializerOptions = new JsonSerializerOptions
|
||||
{
|
||||
PropertyNamingPolicy = JsonNamingPolicy.CamelCase,
|
||||
PropertyNameCaseInsensitive = true,
|
||||
DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull,
|
||||
};
|
||||
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);
|
||||
}
|
||||
}
|
||||
|
||||
Loading…
Reference in a new issue