feat: Introduce Actor Entity #1775
Conversation
Migration Created Mapping updated ActorNameInspector Updated to add ActorNameEntity <
PopulateActorNameInterceptor.cs now adds new ActorNameEntity if created Updated Actor MappingProfiles
📝 WalkthroughWalkthroughThis pull request introduces a new Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (10)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (9)
⏰ Context from checks skipped due to timeout of 90000ms (5)
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: 2
🧹 Nitpick comments (9)
src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Interceptors/PopulateActorNameInterceptor.cs (1)
93-114: Consider concurrency handling for repeated entity creation.
If multiple transactions create the sameActorNamesimultaneously, a unique constraint violation could occur. You may want to handle or retry on unique-index collisions to ensure data consistency and avoid unhandled DbUpdateExceptions. Otherwise, the logic to setactor.ActorNameEntityand clearactor.ActorNameis well-structured.src/Digdir.Domain.Dialogporten.Domain/Actors/ActorName.cs (1)
6-12: Evaluate optional immutability enforcement.
Properties are writable, although the class implementsIImmutableEntity. If this entity is truly intended to be immutable, consider removing setters (or making them private) for properties likeId,ActorId, andNameto more strictly enforce immutability.src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/DialogSeenLogs/Queries/Search/SearchSeenLogQuery.cs (1)
44-44: Consider optimizing query performance.The eager loading of
ActorNameEntitymight impact performance for large result sets. Consider:
- Using projection to select only required fields
- Implementing pagination for the results
Example implementation using projection:
- .ThenInclude(x => x.SeenBy).ThenInclude(x => x.ActorNameEntity) + .Select(d => new + { + d.Id, + d.Deleted, + SeenLog = d.SeenLog.Select(s => new + { + s.Id, + s.SeenAt, + SeenBy = new + { + s.SeenBy.Id, + ActorName = s.SeenBy.ActorNameEntity.Name ?? s.SeenBy.ActorName + } + }).ToList() + })src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/DialogTransmissions/Queries/Search/SearchTransmissionQuery.cs (1)
49-49: Consider adding ordering for ActorNameEntity inclusion.While other included entities have explicit ordering (e.g.,
OrderBy(x => x.CreatedAt)), the ActorNameEntity inclusion lacks ordering which might affect result consistency.- .ThenInclude(x => x.Sender).ThenInclude(x => x.ActorNameEntity) + .ThenInclude(x => x.Sender).ThenInclude(x => x.ActorNameEntity.OrderBy(x => x.CreatedAt))src/Digdir.Domain.Dialogporten.Application/Externals/IDialogDbContext.cs (1)
49-50: Add XML documentation for the ActorName property.For consistency with other properties in the interface, consider adding XML documentation to describe the purpose and usage of the ActorName DbSet.
+ /// <summary> + /// Gets or sets the DbSet for managing actor names in the database. + /// </summary> DbSet<ActorName> ActorName { get; }src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogTransmissions/Queries/Search/SearchTransmissionQuery.cs (1)
46-46: Maintain consistent ordering with ServiceOwner implementation.For consistency with the ServiceOwner implementation and other included entities, consider adding explicit ordering for ActorNameEntity.
- .ThenInclude(x => x.Sender).ThenInclude(x => x.ActorNameEntity) + .ThenInclude(x => x.Sender).ThenInclude(x => x.ActorNameEntity.OrderBy(x => x.CreatedAt))src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Queries/Get/GetDialogQuery.cs (1)
78-86: LGTM! Consider optimizing the query performance.The inclusion of ActorNameEntity relationships is correct and aligns with the new data structure. However, consider using split queries for better performance when retrieving large datasets.
var dialog = await _db.Dialogs + .AsSplitQuery() .Include(x => x.Transmissions).ThenInclude(x => x.Sender).ThenInclude(x => x.ActorNameEntity)src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/Dialogs/Queries/Get/GetDialogQuery.cs (1)
76-85: LGTM! Consider optimizing the query performance.The inclusion of ActorNameEntity relationships is correct and aligns with the new data structure. However, consider using split queries for better performance when retrieving large datasets.
var dialog = await _db.Dialogs + .AsSplitQuery() .Include(x => x.Transmissions).ThenInclude(x => x.Sender).ThenInclude(x => x.ActorNameEntity)src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/Common/Actors/MappingProfile.cs (1)
24-25: LGTM! Consider adding XML documentation.The mapping changes correctly handle both the identifier masking and the new ActorName entity structure. Consider adding XML documentation to explain the mapping logic for future maintainers.
CreateMap<Actor, ActorDto>() + /// <summary> + /// Maps Actor to ActorDto with the following rules: + /// - ActorId is masked using IdentifierMasker + /// - ActorName is retrieved from ActorNameEntity if available, falls back to direct ActorName + /// </summary> .ForMember(dest => dest.ActorType, opt => opt.MapFrom(src => src.ActorTypeId))
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Migrations/20250128083117_IntroduceActorEntity.Designer.csis excluded by!**/Migrations/**/*Designer.cssrc/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Migrations/DialogDbContextModelSnapshot.csis excluded by!**/Migrations/DialogDbContextModelSnapshot.cs
📒 Files selected for processing (23)
src/Digdir.Domain.Dialogporten.Application/Externals/IDialogDbContext.cs(2 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/Common/Actors/MappingProfile.cs(1 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogActivities/Queries/Get/GetActivityQuery.cs(1 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogLabelAssignmentLog/Queries/Search/SearchLabelAssignmentLogQuery.cs(1 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogSeenLogs/Queries/Get/GetSeenLogQuery.cs(1 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogSeenLogs/Queries/Search/SearchSeenLogQuery.cs(1 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogTransmissions/Queries/Get/GetTransmissionQuery.cs(1 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogTransmissions/Queries/Search/SearchTransmissionQuery.cs(1 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/Dialogs/Queries/Get/GetDialogQuery.cs(2 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Common/Actors/MappingProfile.cs(2 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/DialogActivities/Queries/Get/GetActivityQuery.cs(1 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/DialogSeenLogs/Queries/Get/GetSeenLogQuery.cs(1 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/DialogSeenLogs/Queries/Search/SearchSeenLogQuery.cs(1 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/DialogTransmissions/Queries/Get/GetTransmissionQuery.cs(1 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/DialogTransmissions/Queries/Search/SearchTransmissionQuery.cs(1 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogCommand.cs(0 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Queries/Get/GetDialogQuery.cs(1 hunks)src/Digdir.Domain.Dialogporten.Domain/Actors/Actor.cs(1 hunks)src/Digdir.Domain.Dialogporten.Domain/Actors/ActorName.cs(1 hunks)src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Configurations/Actors/ActorNameConfiguration.cs(1 hunks)src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/DialogDbContext.cs(1 hunks)src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Interceptors/PopulateActorNameInterceptor.cs(4 hunks)src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Migrations/20250128083117_IntroduceActorEntity.cs(1 hunks)
💤 Files with no reviewable changes (1)
- src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogCommand.cs
🔇 Additional comments (18)
src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Interceptors/PopulateActorNameInterceptor.cs (3)
6-6: No concerns for the added import.
The import for Identifiable extensions is aligned with the usage in later lines.
18-28: Constructor injection approach is consistent.
InjectingITransactionTimeand assigning it to_transactionTimecleanly integrates the needed property. The readability of this addition is good, and the null check ensures reliability.
64-65: Distinct retrieval of ActorIds looks fine.
Selecting the distinct actor IDs before querying is straightforward and helps avoid redundant lookups.src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Configurations/Actors/ActorNameConfiguration.cs (1)
7-12: Unique composite index is appropriate.
Defining a unique index on(ActorId, Name)effectively prevents duplicate names for the same actor and helps maintain domain consistency.src/Digdir.Domain.Dialogporten.Domain/Actors/Actor.cs (1)
17-18: Add data integrity constraints and documentation for ActorNameEntity.The coexistence of
ActorNamestring andActorNameEntitywithout clear constraints could lead to data inconsistency. Consider:
- Adding XML documentation explaining the relationship between these properties
- Implementing validation to ensure data consistency
- Adding migration guide for transitioning from string to entity-based names
Here's a suggested implementation:
public string? ActorName { get; set; } + /// <summary> + /// Entity representation of the actor's name. When present, this takes precedence over ActorName. + /// This property is part of the transition from string-based to entity-based actor names. + /// </summary> public ActorName? ActorNameEntity { get; set; } + /// <summary> + /// Gets the effective name of the actor, prioritizing ActorNameEntity if available. + /// </summary> + public string? GetEffectiveName() => ActorNameEntity?.Name ?? ActorName;src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogLabelAssignmentLog/Queries/Search/SearchLabelAssignmentLogQuery.cs (1)
39-39: Add tests and optimize query performance.Similar to SearchSeenLogQuery, consider performance optimizations. Additionally:
- Add unit tests for the new ActorNameEntity inclusion
- Add integration tests to verify the authorization behavior with the new entity
Would you like me to help generate the test cases for this query handler?
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/DialogSeenLogs/Queries/Get/GetSeenLogQuery.cs (1)
46-46: LGTM! Proper inclusion of ActorNameEntity.The addition of
.ThenInclude(x => x.ActorNameEntity)correctly extends the query to include the actor name data from the new dedicated table.src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/DialogActivities/Queries/Get/GetActivityQuery.cs (1)
43-43: LGTM! Proper inclusion of ActorNameEntity.The addition of
.ThenInclude(x => x.ActorNameEntity)correctly extends the query to include the actor name data while maintaining existing authorization checks.src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogActivities/Queries/Get/GetActivityQuery.cs (1)
44-44: LGTM! Proper inclusion of ActorNameEntity.The addition of
.ThenInclude(x => x.ActorNameEntity)correctly extends the query to include the actor name data while maintaining proper authorization throughIAltinnAuthorization.src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogSeenLogs/Queries/Search/SearchSeenLogQuery.cs (2)
47-47: LGTM! Proper inclusion of ActorNameEntity.The addition of
.ThenInclude(x => x.ActorNameEntity)correctly extends the query to include the actor name data while maintaining proper authorization checks.
Line range hint
75-75: Verify the current user check with ActorNameEntity.Please verify that the current user check using
ActorIdremains valid with the introduction ofActorNameEntity. The check should continue to work correctly for identifying the current user's seen logs.✅ Verification successful
Current user check remains valid with ActorNameEntity
The code properly handles both scenarios by using
ActorNameEntity.ActorIdwhen available and falling back toSeenBy.ActorIdwhen not. The identity comparison for current user verification remains accurate and reliable.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of ActorId in current user checks across the codebase # Search for similar patterns of current user checks using ActorId rg -A 5 "UserId\.ExternalIdWithPrefix.*ActorId" --type cs # Search for any tests validating the current user check rg -A 10 "test.*current.*user.*seen" --type cs -iLength of output: 2553
Script:
#!/bin/bash # Check ActorNameEntity structure and usage ast-grep --pattern 'class ActorNameEntity { $$$ }' # Check SeenBy property implementation ast-grep --pattern 'SeenBy { $$$ }' # Look for any migration or changes related to ActorNameEntity rg -A 5 "ActorNameEntity" --type csLength of output: 37626
src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogSeenLogs/Queries/Get/GetSeenLogQuery.cs (1)
49-49: Verify null handling for ActorNameEntity in the mapper.The query includes ActorNameEntity, but ensure that the mapper (SeenLogDto) properly handles cases where ActorNameEntity might be null.
✅ Verification successful
Proper null handling is already implemented for ActorNameEntity.
The mapping configuration correctly handles cases where ActorNameEntity is null by falling back to ActorName:
.ForMember(dest => dest.ActorName, opt => opt.MapFrom(src => src.ActorNameEntity == null ? src.ActorName! : src.ActorNameEntity.Name));🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for SeenLogDto mapping configuration ast-grep --pattern 'CreateMap<DialogSeenLog, SeenLogDto>()'Length of output: 709
Script:
#!/bin/bash # Get the complete mapping profile content for file in $(fd "MappingProfile.cs" --exec echo {}) do echo "=== $file ===" cat "$file" echo -e "\n" done # Also get the DTO definition to understand the mapping requirements rg -l "SeenLogDto" | xargs catLength of output: 115447
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/DialogTransmissions/Queries/Get/GetTransmissionQuery.cs (1)
52-52: LGTM! Appropriate eager loading of ActorNameEntity.The inclusion of
ActorNameEntityin the query is consistent with the new actor name normalization feature.src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogTransmissions/Queries/Get/GetTransmissionQuery.cs (1)
50-50: LGTM! Consistent implementation of ActorNameEntity loading.The change mirrors the ServiceOwner implementation, maintaining consistency across the codebase.
src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Migrations/20250128083117_IntroduceActorEntity.cs (2)
39-43: LGTM! Good use of unique index.The unique index on (ActorId, Name) will prevent duplicate actor names for the same actor ID.
24-27: LGTM! Appropriate column definitions.
- UUID for Id with gen_random_uuid() default
- Proper length constraints on strings
- UTC timestamp for CreatedAt
src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/DialogDbContext.cs (1)
51-51: LGTM! Consistent DbSet implementation.The new ActorName DbSet follows the established pattern in the context.
src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/Dialogs/Queries/Get/GetDialogQuery.cs (1)
143-145: Verify null handling in actor ID retrieval.The null-coalescing logic for actor ID retrieval is correct, but ensure that this change is consistently applied across all actor ID retrievals in the codebase.
✅ Verification successful
The null-coalescing pattern is a defensive programming practice, not a required pattern.
The null check in the actor ID retrieval is an extra safety measure. The database schema and entity model ensure that SeenBy is a required property, making direct ActorId access safe. Other locations in the codebase correctly access ActorId directly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other instances where actor ID is retrieved without checking ActorNameEntity ast-grep --pattern 'SeenBy.ActorId'Length of output: 35
Script:
#!/bin/bash # Search for SeenBy patterns and ActorNameEntity usages echo "=== Searching for SeenBy patterns ===" rg "SeenBy" -A 2 -B 2 echo -e "\n=== Searching for ActorNameEntity patterns ===" rg "ActorNameEntity" -A 2 -B 2 echo -e "\n=== Using AST to find property access patterns ===" ast-grep --pattern 'SeenBy.$_'Length of output: 76347
...dir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Common/Actors/MappingProfile.cs
Outdated
Show resolved
Hide resolved
| migrationBuilder.Sql(""" | ||
| INSERT INTO "ActorName" ("Id", "CreatedAt", "ActorId", "Name") | ||
| SELECT a."Id", -- Just borrow the Id from Actor to get uuid7 | ||
| a."CreatedAt", | ||
| a."ActorId", | ||
| a."ActorName" | ||
| FROM "Actor" a | ||
| WHERE a."ActorId" IS NOT NULL | ||
| AND a."ActorName" IS NOT NULL | ||
| ON CONFLICT DO NOTHING; | ||
|
|
||
| UPDATE "Actor" | ||
| SET "ActorNameEntityId" = ( | ||
| SELECT an."Id" | ||
| FROM "ActorName" an | ||
| WHERE "ActorId" = an."ActorId" | ||
| AND "ActorName" = an."Name") | ||
| WHERE "ActorId" IS NOT NULL | ||
| AND "ActorName" IS NOT NULL | ||
| AND "ActorNameEntityId" IS NULL; | ||
| """); |
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
Review comment is correct - migration SQL needs improvement
The concerns are valid and supported by the migration file evidence:
- Using Actor.Id as ActorName.Id bypasses the table's default UUID generation and could cause conflicts
- The migration would benefit from transaction boundaries for atomicity
- The ON CONFLICT clause should specify columns (ActorId, Name) as they have a unique constraint
- Logging conflicts would help track data inconsistencies
The suggested SQL improvements in the review comment properly address all these issues while maintaining the migration's intended functionality.
🔗 Analysis chain
Potential issues in data migration SQL.
- Using Actor.Id for ActorName.Id could cause conflicts if the same actor appears in multiple records.
- Consider wrapping the migration in a transaction for atomicity.
- Consider logging conflicts when ON CONFLICT DO NOTHING is triggered.
Consider this revised SQL:
- migrationBuilder.Sql("""
+ migrationBuilder.Sql("""
+ BEGIN;
INSERT INTO "ActorName" ("Id", "CreatedAt", "ActorId", "Name")
- SELECT a."Id", -- Just borrow the Id from Actor to get uuid7
+ SELECT gen_random_uuid(), -- Generate new UUIDs
a."CreatedAt",
a."ActorId",
a."ActorName"
FROM "Actor" a
WHERE a."ActorId" IS NOT NULL
AND a."ActorName" IS NOT NULL
- ON CONFLICT DO NOTHING;
+ ON CONFLICT ("ActorId", "Name") DO NOTHING
+ RETURNING "ActorId", "Name" INTO temp_conflicts;
UPDATE "Actor"
SET "ActorNameEntityId" = (
SELECT an."Id"
FROM "ActorName" an
WHERE "ActorId" = an."ActorId"
AND "ActorName" = an."Name")
WHERE "ActorId" IS NOT NULL
AND "ActorName" IS NOT NULL
AND "ActorNameEntityId" IS NULL;
+
+ -- Log conflicts if any
+ INSERT INTO "_MigrationLog" ("Message")
+ SELECT 'Conflict detected for ActorId: ' || "ActorId" || ', Name: ' || "Name"
+ FROM temp_conflicts;
+
+ COMMIT;
""");🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for Actor table definition and constraints
fd -e cs Migrations -x cat {} | grep -A 20 "CreateTable.*Actor"
# Look for any existing logging mechanisms
rg "_MigrationLog|MigrationHistory" -A 5
# Check database provider configuration
rg "UseNpgsql|UseSqlServer|UseMySQL" -A 2
Length of output: 1763
Script:
#!/bin/bash
# Find the migration file containing ActorName table creation
fd IntroduceActorEntity.cs -x cat {}
# Search for Actor table creation and related migrations
rg "CreateTable\(.*Actor" -A 20
rg "AddUniqueConstraint.*Actor" -A 5
Length of output: 4028
# Conflicts: # src/Digdir.Domain.Dialogporten.Application/Externals/IDialogDbContext.cs # src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogCommand.cs
|
ActorId must be nullable, update migration ( WHERE a."ActorId" IS NOT NULL ) |
Description
Moved ActorName into its own table.
Updated Queries and mapping for ActorDto.
Related Issue(s)
Verification
Documentation
docs-directory, Altinnpedia or a separate linked PR in altinn-studio-docs., if applicable)