Conversation
- Moved logic related to the IAuditEntryRepository from the AuditService to the new service - Introduced new Async methods - Using ids (for easier transition from the previous Write method) - Using keys - Moved and updated integration tests related to the audit entries to a new test class `AuditEntryServiceTests` - Added unit tests class `AuditEntryServiceTests` and added a few unit tests - Added migration to add columns for `performingUserKey` and `affectedUserKey` and convert existing user ids - Adjusted usages of the old AuditService.Write method to use the new one (mostly notification handlers)
- Added new async and paged methods - Marked (now) redundant methods as obsolete - Updated all of the usages to use the non-obsolete methods - Added unit tests class `AuditServiceTests` and some unit tests - Updated existing integration test
…/improvement/audit-service-rework
# Conflicts: # src/Umbraco.Cms.Api.Management/Security/BackOfficeUserManagerAuditer.cs # src/Umbraco.Core/Handlers/AuditNotificationsHandler.cs # src/Umbraco.Core/Services/AuditEntryService.cs # src/Umbraco.Core/Services/AuditService.cs # src/Umbraco.Core/Services/IAuditEntryService.cs # src/Umbraco.Core/Services/IAuditService.cs # src/Umbraco.Infrastructure/Persistence/Repositories/Implement/AuditEntryRepository.cs # tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/AuditEntryServiceTests.cs # tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/AuditServiceTests.cs # tests/Umbraco.Tests.UnitTests/Umbraco.Core/Services/AuditEntryServiceTests.cs
There was a problem hiding this comment.
Pull Request Overview
Reworks the audit service to use asynchronous and paged APIs, marks legacy methods obsolete, updates all callers, and adds corresponding tests.
- Introduces
AddAsync,GetItemsAsync,GetItemsByKeyAsync,GetItemsByEntityAsync, andCleanLogsAsyncinIAuditServiceand deprecates old sync methods - Updates core services (e.g.,
PackagingService), background jobs, notification handlers, and DI registrations to use the new async audit methods - Revises unit and integration tests to call and verify the new async/pagination behaviors
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/.../LogScrubberJobTests.cs | Updated test to verify CleanLogsAsync is called |
| tests/.../AuditServiceTests.cs | Added unit tests for async add and paged fetch methods |
| tests/.../ContentEditingServiceTests.cs | Switched from AddNotificationHandler to AddNotificationAsyncHandler |
| tests/.../Umbraco.Infrastructure/Services/AuditServiceTests.cs (Integration) | Updated calls to AddAsync and GetPagedItemsByUserAsync |
| tests/.../ContentServiceTests.cs | Converted GetLogs calls to GetItemsByEntityAsync |
| src/Umbraco.Infrastructure/Services/Implement/PackagingService.cs | Replaced Add with AddAsync, aligned error handling for missing users |
| src/Umbraco.Infrastructure/Events/RelateOnTrashNotificationHandler.cs | Added INotificationAsyncHandler support and injected IUserIdKeyResolver |
| src/Umbraco.Infrastructure/DependencyInjection/UmbracoBuilder.CoreServices.cs | Registered AddNotificationAsyncHandler for relevant notifications |
| src/Umbraco.Infrastructure/BackgroundJobs/Jobs/LogScrubberJob.cs | Updated RunJobAsync to await CleanLogsAsync and adjusted signature |
| src/Umbraco.Core/Services/IAuditService.cs | Extended interface with async/paged methods, marked sync methods [Obsolete] |
| src/Umbraco.Core/Events/RelateOnCopyNotificationHandler.cs | Implemented INotificationAsyncHandler, injected IUserIdKeyResolver, and deprecated sync API |
| src/Umbraco.Cms.Api.Management/Security/BackOfficeUserManagerAuditer.cs | Cleaned up unused using directives |
Comments suppressed due to low confidence (4)
src/Umbraco.Core/Services/IAuditService.cs:112
- The XML summary for
GetPagedItemsByUserAsyncstill reads 'for a given entity' but this method returns items for a user; update the<summary>to reflect 'for a given user'.
/// Returns paged items in the audit trail for a given entity.
src/Umbraco.Infrastructure/Services/Implement/PackagingService.cs:86
- [nitpick] Blocking on the async
AddAsynccall using.GetAwaiter().GetResult()can lead to thread starvation or deadlocks; consider making this method async or providing a synchronous API instead.
_auditService.AddAsync(...).GetAwaiter().GetResult();
src/Umbraco.Infrastructure/Events/RelateOnTrashNotificationHandler.cs:20
- [nitpick] The class still implements the obsolete synchronous
INotificationHandler<ContentMovedToRecycleBinNotification>alongside the async handler; consider removing the sync interface to avoid duplicate or confusing registrations.
public sealed class RelateOnTrashNotificationHandler :
INotificationHandler<ContentMovedNotification>,
INotificationHandler<ContentMovedToRecycleBinNotification>,
INotificationAsyncHandler<ContentMovedToRecycleBinNotification>,
src/Umbraco.Core/Events/RelateOnCopyNotificationHandler.cs:12
- [nitpick] The handler implements both sync and async interfaces for the same notification; removing the obsolete synchronous
INotificationHandler<ContentCopiedNotification>can simplify the class and avoid unintended double invocation.
public class RelateOnCopyNotificationHandler :
INotificationHandler<ContentCopiedNotification>,
INotificationAsyncHandler<ContentCopiedNotification>
| _auditService.AddAsync( | ||
| AuditType.PackagerInstall, | ||
| user?.Key ?? Constants.Security.SuperUserKey, | ||
| -1, | ||
| "Package", | ||
| $"Package data installed for package '{compiledPackage.Name}'.").GetAwaiter().GetResult(); |
There was a problem hiding this comment.
Failures to retrieve the user fall back silently to SuperUserKey; for consistency with other operations, you may want to detect a missing user and handle that error path explicitly before logging.
| _auditService.AddAsync( | |
| AuditType.PackagerInstall, | |
| user?.Key ?? Constants.Security.SuperUserKey, | |
| -1, | |
| "Package", | |
| $"Package data installed for package '{compiledPackage.Name}'.").GetAwaiter().GetResult(); | |
| if (user == null) | |
| { | |
| _auditService.AddAsync( | |
| AuditType.PackagerInstall, | |
| Constants.Security.SuperUserKey, | |
| -1, | |
| "Package", | |
| $"Package data installed for package '{compiledPackage.Name}', but the user with ID {userId} could not be found.").GetAwaiter().GetResult(); | |
| } | |
| else | |
| { | |
| _auditService.AddAsync( | |
| AuditType.PackagerInstall, | |
| user.Key, | |
| -1, | |
| "Package", | |
| $"Package data installed for package '{compiledPackage.Name}'.").GetAwaiter().GetResult(); | |
| } |
Description
AddAsync- replacesAdd(async + accepts user key)GetItemsAsync- replacesGetLogs(async + pagination)GetItemsByEntityAsync- replacesGetPagedItemsByEntityandGetLogs(objectId)(async + pagination)CleanLogsAsync- replacesCleanLogs(async)AuditServiceTestsand some unit testsTesting
At the moment, this service is only being used to:
The rest of the audit operations are being done by calling the AuditRepository directly. PR #19357, done on top of this one, tackles that.