chore: Only allow for opting out of aggregate side effects in unit of work. #1389
Conversation
📝 WalkthroughWalkthroughThe pull request introduces several modifications primarily focused on renaming methods and changing terminology related to side effects in the context of database operations. The method Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
src/Digdir.Library.Entity.EntityFrameworkCore/Features/Updatable/UpdatableExtensions.cs (1)
31-33: Consider adding XML documentation to the helper methods.While the method names are clear, adding XML documentation would improve the maintainability of the library code by explicitly documenting the purpose and behavior of each method.
Here's a suggested improvement:
+ /// <summary> + /// Determines if the entity is not soft deleted. + /// </summary> + /// <param name="x">The entity entry to check.</param> + /// <returns>True if the entity is not soft deleted, false otherwise.</returns> private static bool IsNotSoftDeleted(EntityEntry<IUpdateableEntity> x) => x.Entity is not ISoftDeletableEntity { Deleted: true }; + /// <summary> + /// Checks if the entity was added without an explicit UpdatedAt timestamp. + /// </summary> + /// <param name="x">The entity entry to check.</param> + /// <returns>True if the entity is newly added without an UpdatedAt value, false otherwise.</returns> private static bool AddedWithoutExplicitUpdatedAt(EntityEntry<IUpdateableEntity> x) => x.State is EntityState.Added && x.Entity.UpdatedAt == default; + /// <summary> + /// Checks if the entity has been modified. + /// </summary> + /// <param name="x">The entity entry to check.</param> + /// <returns>True if the entity state is Modified, false otherwise.</returns> private static bool Modified(EntityEntry<IUpdateableEntity> x) => x.State is EntityState.Modified;src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/Dialogs/Queries/Get/GetDialogQuery.cs (1)
Line range hint
133-135: Consider enhancing error handling messages.While using
UnreachableExceptionis appropriate here since these errors should never occur, the error messages could be more specific about why these cases are unreachable when updatingSeenAt. This would help future maintainers understand the assumptions.Consider this improvement:
- domainError => throw new UnreachableException("Should not get domain error when updating SeenAt."), - concurrencyError => throw new UnreachableException("Should not get concurrencyError when updating SeenAt.")); + domainError => throw new UnreachableException( + "Domain error should be impossible when updating SeenAt as it's a simple timestamp update."), + concurrencyError => throw new UnreachableException( + "Concurrency error should be impossible when updating SeenAt as it doesn't use concurrency tokens."));src/Digdir.Library.Entity.EntityFrameworkCore/Features/Aggregate/AggregateExtensions.cs (1)
61-61: Consider adding XML documentation for method chaining.The return of
changeTrackerenables method chaining, which should be documented for clarity.Add XML documentation above the method:
+ /// <summary> + /// Handles aggregate entities by processing their state changes and invoking appropriate handlers. + /// </summary> + /// <param name="changeTracker">The change tracker to process.</param> + /// <param name="utcNow">The current UTC timestamp.</param> + /// <param name="cancellationToken">A token to cancel the operation.</param> + /// <returns>The change tracker instance to enable method chaining.</returns> internal static async Task<ChangeTracker> HandleAggregateEntities(this ChangeTracker changeTracker,src/Digdir.Library.Entity.EntityFrameworkCore/EntityLibraryEfCoreExtensions.cs (3)
43-43: Typographical error in parameter documentation forutcNow.The XML documentation for the
utcNowparameter contains a typo: "tok" should be "took".Apply this diff to fix the typo:
-/// <param name="utcNow">The time in UTC in which the changes tok place.</param> +/// <param name="utcNow">The time in UTC in which the changes took place.</param>
45-45: Update<returns>documentation to reflect the asynchronous return type.The
<returns>documentation states that the method returnsChangeTracker, but it actually returns aTask<ChangeTracker>. Update the documentation to reflect the correct return type and asynchronous nature.Apply this diff to correct the documentation:
-/// <returns>The same <see cref="ChangeTracker"/> instance so that multiple calls can be chained.</returns> +/// <returns>A <see cref="Task{ChangeTracker}"/> that represents the asynchronous operation. The task result contains the same <see cref="ChangeTracker"/> instance so that multiple calls can be chained.</returns>
35-35: Inconsistent spelling ofUpdatablein documentation.The interface is referred to as
IUpdateableEntity, but the correct spelling isIUpdatableEntity.Apply this diff to correct the interface name:
-/// <item><see cref="IUpdateableEntity"/></item> +/// <item><see cref="IUpdatableEntity"/></item>
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
src/Digdir.Domain.Dialogporten.Application/Externals/IUnitOfWork.cs(1 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/Dialogs/Queries/Get/GetDialogQuery.cs(1 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Queries/Get/GetDialogQuery.cs(1 hunks)src/Digdir.Domain.Dialogporten.Infrastructure/UnitOfWork.cs(3 hunks)src/Digdir.Library.Entity.EntityFrameworkCore/EntityLibraryEfCoreExtensions.cs(3 hunks)src/Digdir.Library.Entity.EntityFrameworkCore/Features/Aggregate/AggregateExtensions.cs(2 hunks)src/Digdir.Library.Entity.EntityFrameworkCore/Features/Updatable/UpdatableExtensions.cs(2 hunks)
🔇 Additional comments (11)
src/Digdir.Domain.Dialogporten.Application/Externals/IUnitOfWork.cs (2)
10-10: LGTM! The rename improves semantic clarity.
The change from WithoutAuditableSideEffects() to WithoutAggregateSideEffects() better reflects the Domain-Driven Design principles by focusing on aggregate-level operations rather than just auditing concerns.
10-10: Verify all implementations and usages are updated.
Since this is a breaking change in a public interface, we should verify that all implementations and callers have been updated accordingly.
✅ Verification successful
Let me analyze the implementation further to ensure all aspects are covered.
Let me check the test file to ensure it's properly updated.
Breaking change in IUnitOfWork has been properly implemented across the codebase
Based on the verification results:
- The new method
WithoutAggregateSideEffectsis correctly implemented inUnitOfWork.cs - There is only one implementation of
IUnitOfWorkin the codebase - The method is properly used in two query handlers
- Test files use
Substitute.For<IUnitOfWork>()for mocking, which automatically handles the interface changes - No traces of the old method name were found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that old method name is completely removed and new method name is consistently used
# Test 1: Check for any remaining occurrences of the old method name
echo "Checking for old method name..."
rg "WithoutAuditableSideEffects"
# Test 2: Verify implementations of the new method
echo "Checking implementations of IUnitOfWork..."
ast-grep --pattern 'class $_ : IUnitOfWork'
# Test 3: Verify usage of the new method
echo "Checking usage of new method..."
rg "WithoutAggregateSideEffects"
Length of output: 877
Script:
#!/bin/bash
# Let's verify the implementation details and check for any potential missing updates
# Check the implementation in UnitOfWork.cs
echo "Checking UnitOfWork implementation..."
rg -A 5 "WithoutAggregateSideEffects" "src/Digdir.Domain.Dialogporten.Infrastructure/UnitOfWork.cs"
# Check if there are any other implementations of IUnitOfWork
echo "Checking for other IUnitOfWork implementations..."
rg "class.*:.*IUnitOfWork"
# Check for any test files that might need updates
echo "Checking test files..."
fd -e cs -e fs test --exec rg -l "WithoutAggregateSideEffects|IUnitOfWork" {}
Length of output: 842
Script:
#!/bin/bash
# Check the content of the test file
echo "Checking test file content..."
rg -A 5 "WithoutAggregateSideEffects|IUnitOfWork" "./tests/Digdir.Domain.Dialogporten.Application.Unit.Tests/Features/V1/ServiceOwner/Dialogs/Commands/CreateDialogTests.cs"
# Double check for any mock implementations in test files
echo "Checking for mock implementations..."
ast-grep --pattern 'class $_ : IUnitOfWork'
Length of output: 1215
src/Digdir.Library.Entity.EntityFrameworkCore/Features/Updatable/UpdatableExtensions.cs (1)
20-21: Well-structured refactoring of filtering logic!
The extraction of complex conditions into separate, well-named predicate methods improves code readability and maintainability while following the Single Responsibility Principle.
src/Digdir.Domain.Dialogporten.Infrastructure/UnitOfWork.cs (3)
31-31: LGTM! Field renaming improves semantic clarity.
The renaming from _auditableSideEffects to _aggregateSideEffects better reflects the actual purpose of controlling aggregate-level side effects.
54-57: LGTM! Method implementation is clean and follows fluent pattern.
The renamed method maintains a clear, single responsibility of disabling aggregate side effects while following the fluent interface pattern.
97-101: Document the relationship between auditable and aggregate entity handling.
The code structure implies that auditable entity handling is now a core operation while aggregate entity handling can be opted out. Please add documentation to clarify:
- The distinction between auditable and aggregate entity handling
- Why auditable handling is mandatory while aggregate handling is optional
- Whether the order of these operations is significant
Let's verify the usage pattern across the codebase:
✅ Verification successful
Based on the gathered information, I can now provide a final response:
The distinction between auditable and aggregate handling is clear and justified
The code structure reveals:
-
Auditable handling (
HandleAuditableEntities) manages core entity lifecycle properties:- Identifiable entities
- Immutable entities
- Creation timestamps
- Update timestamps
- Soft deletion
- Lookup entities
-
Aggregate handling (
HandleAggregateEntities) manages domain-specific aggregate behaviors:- Creation events/side effects
- Update events/side effects
- Deletion events/side effects
- Restoration events/side effects
The order is significant because:
- Auditable handling must run first to ensure core entity states are properly set
- Aggregate handlers can then use these updated states for their domain-specific behaviors
Making auditable handling mandatory while allowing aggregate handling to be optional is the correct approach since:
- Core entity lifecycle tracking should never be bypassed
- Domain-specific aggregate behaviors may need to be skipped in certain scenarios (e.g., read operations as shown in GetDialogQuery usage)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Analyze the usage of HandleAuditableEntities and HandleAggregateEntities
# to understand their relationship and implementation details
# Search for the implementation of both handlers
echo "Searching for handler implementations:"
ast-grep --pattern 'HandleAuditableEntities($_)'
ast-grep --pattern 'HandleAggregateEntities($_)'
# Search for other places where WithoutAggregateSideEffects is used
echo "Searching for WithoutAggregateSideEffects usage:"
rg "WithoutAggregateSideEffects"
Length of output: 853
Script:
#!/bin/bash
# Let's look for the implementation details of these handlers and related entities
# Search for ChangeTracker extensions containing these handlers
echo "Searching for ChangeTracker extensions:"
rg -A 10 "HandleAuditableEntities|HandleAggregateEntities" --type cs
# Search for auditable and aggregate related interfaces/base classes
echo "Searching for related entity interfaces:"
rg "IAuditable|IAggregate" --type cs -A 5
# Look for any documentation or comments about these patterns
echo "Searching for documentation comments about these patterns:"
rg -B 2 "/// .*[Aa]uditable|/// .*[Aa]ggregate" --type cs
Length of output: 34389
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Queries/Get/GetDialogQuery.cs (2)
117-117: LGTM: Method rename aligns with architectural changes.
The rename from WithoutAuditableSideEffects() to WithoutAggregateSideEffects() reflects the shift in focus from auditing to aggregate-level side effects handling.
Line range hint 119-122: Verify assumptions about impossible error cases.
The error handling assumes that neither domain errors nor concurrency errors can occur during SeenAt updates. While this might be true in the current implementation, it's worth verifying these assumptions to ensure robustness.
Let's verify these assumptions by checking the implementation:
✅ Verification successful
Let me gather more information about the UpdateSeenAt implementation and any domain validations.
Let me search for the implementation of UpdateSeenAt to verify the assumptions about domain and concurrency errors.
Based on the code analysis, I can now generate the final response:
The assumptions about impossible error cases are correct - domain and concurrency errors cannot occur during SeenAt updates.
Looking at the implementation of UpdateSeenAt in DialogEntity.cs, we can see that:
- The method only performs a simple check to avoid duplicate logs and then adds a new
SeenLogentry - There are no domain validations that could fail
- The
SeenLogis a simple collection property with no concurrency control - The
SeenLogentity is marked asIImmutableEntitywhich means it doesn't participate in concurrency checks - The operation is purely additive - it never modifies existing data that could cause conflicts
The error handling in the code correctly assumes these cases are unreachable.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if there are any domain validations or concurrency checks in Dialog.UpdateSeenAt
# Search for UpdateSeenAt implementation
echo "Searching for UpdateSeenAt implementation..."
ast-grep --pattern 'UpdateSeenAt($$$) {
$$$
}'
# Search for any concurrency token or timestamp fields that might affect UpdateSeenAt
echo "Searching for concurrency-related fields..."
rg -A 2 'ConcurrencyStamp|RowVersion|LastModified'
Length of output: 5711
Script:
#!/bin/bash
# Search for Dialog entity and UpdateSeenAt implementation
echo "Searching for Dialog entity and UpdateSeenAt..."
rg -A 10 "class Dialog"
# Search for any validation or error handling related to SeenAt
echo "Searching for SeenAt related validations..."
rg -A 5 "SeenAt"
# Search for any domain error definitions that might be related
echo "Searching for domain error definitions..."
rg "DomainError|ValidationError"
Length of output: 168507
Script:
#!/bin/bash
# Search for UpdateSeenAt implementation and related code
echo "Searching for UpdateSeenAt implementation..."
rg -A 15 "UpdateSeenAt\("
# Search for any concurrency checks in the Dialog entity
echo "Searching for concurrency related code in Dialog entity..."
rg -A 5 "class DialogEntity"
# Search for any domain validation related to SeenAt
echo "Searching for SeenAt validation..."
rg -A 5 "SeenLog|UpdateSeenAt" --type cs
Length of output: 340482
src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/Dialogs/Queries/Get/GetDialogQuery.cs (1)
129-129: LGTM! Verify consistent terminology across codebase.
The rename from WithoutAuditableSideEffects() to WithoutAggregateSideEffects() aligns with the PR objective of focusing on aggregate-level side effects.
Let's verify the consistency of this terminology change across the codebase:
✅ Verification successful
Terminology change is consistent across the codebase
The verification shows:
- No instances of old
WithoutAuditableSideEffectsmethod remain - New
WithoutAggregateSideEffectsis consistently used in:- Infrastructure layer (UnitOfWork implementation)
- Application layer interface (IUnitOfWork)
- Both EndUser and ServiceOwner dialog query handlers
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining instances of the old terminology and verify the new terminology is used consistently
# Test 1: Search for any remaining instances of the old method name
echo "Checking for remaining instances of WithoutAuditableSideEffects:"
rg "WithoutAuditableSideEffects"
# Test 2: Verify the new method name is used consistently
echo -e "\nVerifying usage of WithoutAggregateSideEffects:"
rg "WithoutAggregateSideEffects"
Length of output: 840
src/Digdir.Library.Entity.EntityFrameworkCore/Features/Aggregate/AggregateExtensions.cs (1)
17-17: LGTM! Verify the impact of the breaking change.
The change from Task to Task<ChangeTracker> is a breaking change that aligns with the PR objective of allowing opt-out of aggregate side effects.
Let's verify the impact of this breaking change:
✅ Verification successful
Let me analyze the results:
- Found 3 files with
HandleAggregateEntitiescalls:EntityLibraryEfCoreExtensions.cs: Public extension method already returnsTask<ChangeTracker>AggregateExtensions.cs: The internal implementation we're reviewingUnitOfWork.cs: Usesawaitbut doesn't use the return value
Let me verify if there are any other potential usages with a different search pattern:
Breaking change is properly handled across the codebase
The change from Task to Task<ChangeTracker> is safe because:
- The public API in
EntityLibraryEfCoreExtensions.csalready returnsTask<ChangeTracker>with proper XML documentation - The only consumer in
UnitOfWork.csusesawaitwithout capturing the return value, so it's unaffected - No other usages found in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct calls to HandleAggregateEntities to ensure they handle the returned ChangeTracker
rg -l "HandleAggregateEntities\(" | while read -r file; do
echo "=== $file ==="
rg -A 2 "HandleAggregateEntities\(" "$file"
done
Length of output: 1527
Script:
#!/bin/bash
# Search for any references to HandleAggregateEntities, including indirect ones
rg -l "HandleAggregateEntities" | while read -r file; do
echo "=== $file ==="
rg -B 2 -A 2 "HandleAggregateEntities" "$file"
done
Length of output: 1916
src/Digdir.Library.Entity.EntityFrameworkCore/EntityLibraryEfCoreExtensions.cs (1)
69-75: Confirm the removal of asynchronous behavior in HandleAuditableEntities.
The method HandleAuditableEntities has been changed from asynchronous to synchronous. Ensure that this change does not affect any asynchronous operations that depend on it elsewhere in the codebase.
Run the following script to search for usages of HandleAuditableEntities that expect an asynchronous method:
✅ Verification successful
The change from async to sync is correct and safe
The git history shows that HandleAuditableEntities was previously async, but it was only awaited because it called HandleAggregateEntities which was async. The current synchronous implementation is correct as all the chained methods it now calls (HandleLookupEntities, HandleIdentifiableEntities, etc.) are synchronous operations. The only usage found in UnitOfWork.cs is also using it synchronously.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify if any code depends on the asynchronous version of `HandleAuditableEntities`.
# Test: Search for all usages of `HandleAuditableEntities` and check for `await` keywords. Expect: No usages awaiting this method.
rg -A 2 'await.*HandleAuditableEntities' --glob '!src/Digdir.Library.Entity.EntityFrameworkCore/EntityLibraryEfCoreExtensions.cs'
Length of output: 131
Script:
#!/bin/bash
# Let's try a broader search to find any usages of HandleAuditableEntities
rg -A 5 'HandleAuditableEntities' --glob '!src/Digdir.Library.Entity.EntityFrameworkCore/EntityLibraryEfCoreExtensions.cs'
# Also search for any async methods that might be calling this method
ast-grep --pattern 'async $_ $_($$$) { $$$ HandleAuditableEntities $$$ }'
# Let's also check the git history to see if this was previously async
git log -p --all -S "HandleAuditableEntities"
Length of output: 13912
MagnusSandgren
deleted the
chore/split-auditable-and-aggregate-entity-handling
branch
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Documentation