Skip to content

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

Merged
sfmskywalker merged 3 commits intorelease/3.6.0from
fix/trigger-indexing-idempotency
Feb 25, 2026
Merged

Add idempotency tests for trigger indexing and fix serialization mismatch in WorkflowTriggerEqualityComparer#7320
sfmskywalker merged 3 commits intorelease/3.6.0from
fix/trigger-indexing-idempotency

Conversation

@sfmskywalker
Copy link
Member

Summary

  • Add comprehensive idempotency tests for trigger indexing to ensure triggers can be repeatedly indexed without duplication
  • Fix serialization mismatch in WorkflowTriggerEqualityComparer to properly compare workflow triggers
  • Add unit tests for WorkflowTriggerEqualityComparer to verify equality comparison logic

Test plan

  • Run the new TriggerIndexingIdempotencyTests component tests
  • Run the new WorkflowTriggerEqualityComparerTests unit tests
  • Verify no regressions in existing trigger indexing functionality

🤖 Generated with Claude Code

…atch in WorkflowTriggerEqualityComparer

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Feb 24, 2026

Greptile Summary

This PR fixes a critical serialization mismatch bug in WorkflowTriggerEqualityComparer that was causing trigger indexing to incorrectly detect all triggers as changed on every indexing operation. The root cause was that the comparer used PascalCase (default) serialization while IPayloadSerializer (used for DB storage) uses camelCase, causing fresh CLR objects and DB-loaded JsonElements to produce different JSON representations even when logically identical.

Key changes:

  • Added PropertyNamingPolicy = JsonNamingPolicy.CamelCase and DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull to match IPayloadSerializer settings
  • Added NormalizePayload method that serializes payloads to canonical JSON strings before comparison
  • Comprehensive test coverage with both unit and component tests demonstrating the fix

Impact: This fix prevents unnecessary delete-reinsert cycles during trigger indexing, which eliminates a race condition window that could cause unique constraint violations in multi-pod environments.

Confidence Score: 4/5

  • Safe to merge with minor consideration for converter completeness
  • The fix correctly addresses the serialization mismatch bug with proper camelCase naming and payload normalization. Comprehensive test coverage demonstrates the fix works. Score is 4 instead of 5 because the comparer is missing some JSON converters that JsonPayloadSerializer has (TimeSpanConverter, JsonStringEnumConverter, etc.), which could theoretically cause issues with complex payload types, though current tests pass.
  • src/modules/Elsa.Workflows.Runtime/Comparers/WorkflowTriggerEqualityComparer.cs - consider adding remaining converters from JsonPayloadSerializer

Important Files Changed

Filename Overview
src/modules/Elsa.Workflows.Runtime/Comparers/WorkflowTriggerEqualityComparer.cs Fixed serialization mismatch by adding camelCase naming policy and payload normalization to match IPayloadSerializer behavior
test/component/Elsa.Workflows.ComponentTests/Scenarios/TriggerIndexingIdempotency/TriggerIndexingIdempotencyTests.cs Added comprehensive component tests verifying idempotency of trigger indexing and trigger ID preservation
test/unit/Elsa.Workflows.Runtime.UnitTests/Comparers/WorkflowTriggerEqualityComparerTests.cs Added thorough unit tests documenting the serialization mismatch bug and verifying the fix works correctly

Last reviewed commit: 51d3f70

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Feb 24, 2026

Additional Comments (1)

src/modules/Elsa.Workflows.Runtime/Comparers/WorkflowTriggerEqualityComparer.cs
Missing converters that JsonPayloadSerializer has (lines 80-85 in JsonPayloadSerializer.cs): JsonStringEnumConverter, TimeSpanConverter, PolymorphicObjectConverterFactory, VariableConverterFactory, FuncExpressionValueConverter. If trigger payloads contain enums, TimeSpans, polymorphic objects, or variables, serialization may not match.

Current test uses simple record types, so this isn't caught. Consider adding these converters or test with payloads containing TimeSpan? (like HttpEndpointBookmarkPayload.RequestTimeout).

…reation with helper methods and extract serializer options to static fields for clarity.
…imeSpan, and polymorphic objects in serializer options.
@sfmskywalker sfmskywalker merged commit a7e064a into release/3.6.0 Feb 25, 2026
2 checks passed
@sfmskywalker sfmskywalker deleted the fix/trigger-indexing-idempotency branch February 25, 2026 13:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant