[WIP] Fix inconsistent projection name resolution in event handling#51
Conversation
Co-authored-by: JeanMarcMbouma <16613177+JeanMarcMbouma@users.noreply.github.com>
Co-authored-by: JeanMarcMbouma <16613177+JeanMarcMbouma@users.noreply.github.com>
Co-authored-by: JeanMarcMbouma <16613177+JeanMarcMbouma@users.noreply.github.com>
Co-authored-by: JeanMarcMbouma <16613177+JeanMarcMbouma@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes inconsistent projection name resolution across BbQ.Events components by introducing a shared ProjectionNameResolver utility class. Previously, DefaultProjectionEngine respected custom ProjectionOptions.ProjectionName values while DefaultReplayService and DefaultProjectionRebuilder only used type names, leading to mismatched projection identifiers during replay and rebuild operations.
Changes:
- Introduced
ProjectionNameResolverclass with consistent name resolution logic: useProjectionOptions.ProjectionNameif set, otherwise fall back to type name - Updated all three projection components (
DefaultProjectionEngine,DefaultReplayService,DefaultProjectionRebuilder) to use the shared resolver - Added comprehensive unit tests for the resolver
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/BbQ.Events/Engine/ProjectionNameResolver.cs | New utility class providing centralized projection name resolution logic |
| src/BbQ.Events/Engine/DefaultProjectionEngine.cs | Updated to use ProjectionNameResolver when building options from attributes |
| src/BbQ.Events/Engine/DefaultReplayService.cs | Updated to resolve projection names consistently in both GetRegisteredProjections and StreamEventsAndProcessAsync |
| src/BbQ.Events/Engine/DefaultProjectionRebuilder.cs | Updated to resolve projection names consistently in GetRegisteredProjections |
| tests/BbQ.Cqrs.Tests/ProjectionNameResolverTests.cs | Comprehensive unit tests covering all resolver edge cases including null, empty, whitespace, and custom names |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| }; | ||
|
|
||
| // Use ProjectionNameResolver for consistent name resolution | ||
| // Since ProjectionName is not explicitly set in options, the resolver will use the type name |
There was a problem hiding this comment.
The comment "Since ProjectionName is not explicitly set in options, the resolver will use the type name" is slightly misleading. The ProjectionName property has a default value of string.Empty (from ProjectionOptions.cs line 55), not null. The resolver checks string.IsNullOrWhiteSpace which correctly treats empty string as "not set", but the comment could be clearer.
Consider revising to: "Since ProjectionName is empty in the newly created options, the resolver will fall back to the type name"
| // Since ProjectionName is not explicitly set in options, the resolver will use the type name | |
| // Since ProjectionName is empty in the newly created options, the resolver will fall back to the type name |
| // Since ProjectionName is not explicitly set in options, the resolver will use the type name | ||
| options.ProjectionName = ProjectionNameResolver.Resolve(concreteType, options); |
There was a problem hiding this comment.
For clarity and consistency, consider passing null instead of options to ProjectionNameResolver.Resolve here. Since we're creating new options with no ProjectionName set, passing the options object is redundant. The call could be simplified to:
options.ProjectionName = ProjectionNameResolver.Resolve(concreteType, null);
This makes it clearer that we're explicitly choosing to use the type name fallback rather than checking an options object we know has no ProjectionName.
| // Since ProjectionName is not explicitly set in options, the resolver will use the type name | |
| options.ProjectionName = ProjectionNameResolver.Resolve(concreteType, options); | |
| // Since ProjectionName is not explicitly set in options, explicitly use the type name fallback | |
| options.ProjectionName = ProjectionNameResolver.Resolve(concreteType, null); |
| using BbQ.Events.Engine; | ||
| using NUnit.Framework; | ||
|
|
||
| namespace BbQ.Cqrs.Tests; | ||
|
|
||
| /// <summary> | ||
| /// Tests for ProjectionNameResolver to ensure consistent projection name resolution. | ||
| /// </summary> | ||
| [TestFixture] | ||
| public class ProjectionNameResolverTests | ||
| { | ||
| // Test projection class | ||
| private class TestProjection { } | ||
|
|
||
| [Test] | ||
| public void Resolve_WithNullProjectionType_ThrowsArgumentNullException() | ||
| { | ||
| // Act & Assert | ||
| Assert.Throws<ArgumentNullException>(() => | ||
| ProjectionNameResolver.Resolve(null!, null)); | ||
| } | ||
|
|
||
| [Test] | ||
| public void Resolve_WithNullOptions_ReturnsTypeName() | ||
| { | ||
| // Arrange | ||
| var projectionType = typeof(TestProjection); | ||
|
|
||
| // Act | ||
| var result = ProjectionNameResolver.Resolve(projectionType, null); | ||
|
|
||
| // Assert | ||
| Assert.That(result, Is.EqualTo("TestProjection")); | ||
| } | ||
|
|
||
| [Test] | ||
| public void Resolve_WithOptionsButNullProjectionName_ReturnsTypeName() | ||
| { | ||
| // Arrange | ||
| var projectionType = typeof(TestProjection); | ||
| var options = new ProjectionOptions | ||
| { | ||
| ProjectionName = null! | ||
| }; | ||
|
|
||
| // Act | ||
| var result = ProjectionNameResolver.Resolve(projectionType, options); | ||
|
|
||
| // Assert | ||
| Assert.That(result, Is.EqualTo("TestProjection")); | ||
| } | ||
|
|
||
| [Test] | ||
| public void Resolve_WithOptionsAndEmptyProjectionName_ReturnsTypeName() | ||
| { | ||
| // Arrange | ||
| var projectionType = typeof(TestProjection); | ||
| var options = new ProjectionOptions | ||
| { | ||
| ProjectionName = string.Empty | ||
| }; | ||
|
|
||
| // Act | ||
| var result = ProjectionNameResolver.Resolve(projectionType, options); | ||
|
|
||
| // Assert | ||
| Assert.That(result, Is.EqualTo("TestProjection")); | ||
| } | ||
|
|
||
| [Test] | ||
| public void Resolve_WithOptionsAndWhitespaceProjectionName_ReturnsTypeName() | ||
| { | ||
| // Arrange | ||
| var projectionType = typeof(TestProjection); | ||
| var options = new ProjectionOptions | ||
| { | ||
| ProjectionName = " " | ||
| }; | ||
|
|
||
| // Act | ||
| var result = ProjectionNameResolver.Resolve(projectionType, options); | ||
|
|
||
| // Assert | ||
| Assert.That(result, Is.EqualTo("TestProjection")); | ||
| } | ||
|
|
||
| [Test] | ||
| public void Resolve_WithOptionsAndCustomProjectionName_ReturnsCustomName() | ||
| { | ||
| // Arrange | ||
| var projectionType = typeof(TestProjection); | ||
| var customName = "CustomProjectionName"; | ||
| var options = new ProjectionOptions | ||
| { | ||
| ProjectionName = customName | ||
| }; | ||
|
|
||
| // Act | ||
| var result = ProjectionNameResolver.Resolve(projectionType, options); | ||
|
|
||
| // Assert | ||
| Assert.That(result, Is.EqualTo(customName)); | ||
| } | ||
|
|
||
| [Test] | ||
| public void Resolve_WithDifferentProjectionTypes_ReturnsCorrectTypeNames() | ||
| { | ||
| // Arrange & Act & Assert | ||
| Assert.That( | ||
| ProjectionNameResolver.Resolve(typeof(TestProjection), null), | ||
| Is.EqualTo("TestProjection")); | ||
|
|
||
| Assert.That( | ||
| ProjectionNameResolver.Resolve(typeof(ProjectionNameResolverTests), null), | ||
| Is.EqualTo("ProjectionNameResolverTests")); | ||
|
|
||
| Assert.That( | ||
| ProjectionNameResolver.Resolve(typeof(string), null), | ||
| Is.EqualTo("String")); | ||
| } | ||
|
|
||
| [Test] | ||
| public void Resolve_ConsistencyBetweenCalls_ReturnsSameResult() | ||
| { | ||
| // Arrange | ||
| var projectionType = typeof(TestProjection); | ||
| var options = new ProjectionOptions | ||
| { | ||
| ProjectionName = "ConsistentName" | ||
| }; | ||
|
|
||
| // Act | ||
| var result1 = ProjectionNameResolver.Resolve(projectionType, options); | ||
| var result2 = ProjectionNameResolver.Resolve(projectionType, options); | ||
|
|
||
| // Assert | ||
| Assert.That(result1, Is.EqualTo(result2)); | ||
| Assert.That(result1, Is.EqualTo("ConsistentName")); | ||
| } | ||
| } |
There was a problem hiding this comment.
The tests comprehensively cover the ProjectionNameResolver itself, but there are no integration tests that verify the fix works end-to-end. Consider adding a test that:
- Registers a projection with a custom ProjectionName via AddProjection configureOptions
- Verifies that DefaultReplayService.GetRegisteredProjections() returns the custom name (not the type name)
- Verifies that replay operations target the correct projection with the custom name
This would ensure the fix solves the original issue described in the PR.
Summary
Successfully fixed the inconsistent projection name resolution across BbQ.Events components.
Changes Made
Benefits
✅ Consistent projection naming across registration, runtime, replay, and rebuild
✅ No breaking changes - maintains backward compatibility
✅ Fixes subtle bugs from mismatched projection identifiers
✅ All existing tests pass
✅ No security vulnerabilities introduced
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.