docs: add performance benchmarks to ContentService refactor design
- Added revision 1.5 with 33 performance benchmarks for baseline comparison - Benchmarks cover CRUD (7), Query (6), Publish (7), Move (8), Version (4) operations - Added baseline comparison infrastructure with JSON output format - Includes execution commands and sample comparison output 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
This commit is contained in:
@@ -1,9 +1,20 @@
|
||||
# ContentService Refactoring Design
|
||||
|
||||
**Date**: 2025-12-19
|
||||
**Status**: Approved
|
||||
**Status**: Revised (post-test strategy)
|
||||
**Branch**: refactor/ContentService
|
||||
|
||||
### Revision History
|
||||
|
||||
| Rev | Description |
|
||||
|-----|-------------|
|
||||
| 1.0 | Initial design document |
|
||||
| 1.1 | Post-architectural review clarifications |
|
||||
| 1.2 | Added performance optimizations (database, memory, concurrency, caching) |
|
||||
| 1.3 | Added detailed test strategy (15 tests for coverage gaps) |
|
||||
| 1.4 | Added phase gates with test execution commands and regression protocol |
|
||||
| 1.5 | Added performance benchmarks (33 tests for baseline comparison) |
|
||||
|
||||
## Overview
|
||||
|
||||
Refactor the `ContentService` class (~3800 lines) into smaller, focused components for improved maintainability, testability, and performance.
|
||||
@@ -21,49 +32,160 @@ Refactor the `ContentService` class (~3800 lines) into smaller, focused componen
|
||||
- Existing public method signatures unchanged
|
||||
- New interfaces can be added
|
||||
- Obsolete constructors to be commented out (not removed)
|
||||
- **Must avoid naming collision with existing `IContentPublishingService`** (see Naming Decisions)
|
||||
|
||||
## Naming Decisions
|
||||
|
||||
### Resolving IContentPublishingService Collision
|
||||
|
||||
An `IContentPublishingService` already exists in the codebase (`src/Umbraco.Core/Services/ContentPublishingService.cs`). This is an async API-layer service that orchestrates publishing via `IContentService`.
|
||||
|
||||
**Decision**: Rename the proposed publishing interface to `IContentPublishOperationService` to avoid collision. The existing `IContentPublishingService` remains unchanged as the higher-level async API orchestrator.
|
||||
|
||||
## Architecture
|
||||
|
||||
### Service Dependency Direction
|
||||
|
||||
Services follow a unidirectional dependency pattern (no circular dependencies):
|
||||
|
||||
```
|
||||
┌─────────────────────────────┐
|
||||
│ ContentService (Facade) │
|
||||
│ - Delegates to all below │
|
||||
└─────────────┬───────────────┘
|
||||
│ depends on
|
||||
┌───────────────────────┼───────────────────────┐
|
||||
│ │ │
|
||||
▼ ▼ ▼
|
||||
┌─────────────────┐ ┌─────────────────────┐ ┌─────────────────┐
|
||||
│ IContentCrud- │ │ IContentPublish- │ │ IContentMove- │
|
||||
│ Service │ │ OperationService │ │ Service │
|
||||
│ (CRUD only) │ │ (publish/schedule) │ │ (move/copy/bin) │
|
||||
└────────┬────────┘ └─────────┬───────────┘ └────────┬────────┘
|
||||
│ │ │
|
||||
│ │ can call │ can call
|
||||
│ ▼ ▼
|
||||
│ ┌─────────────────────────────────────────────┐
|
||||
└──────────► │ IContentCrudService │
|
||||
│ (Save/Delete operations as needed) │
|
||||
└─────────────────────────────────────────────┘
|
||||
```
|
||||
|
||||
**Rules**:
|
||||
- `IContentCrudService` has NO dependencies on other content services
|
||||
- `IContentPublishOperationService` MAY call `IContentCrudService` for Save operations
|
||||
- `IContentMoveService` MAY call `IContentCrudService` for Save/Delete operations
|
||||
- The facade orchestrates cross-service operations when needed
|
||||
|
||||
### New Public Service Interfaces (3)
|
||||
|
||||
| Interface | Responsibility | Est. Lines |
|
||||
|-----------|---------------|------------|
|
||||
| `IContentCrudService` | Create, Get, Save, Delete | ~400 |
|
||||
| `IContentPublishingService` | Publish, Unpublish, Scheduling, Branch | ~800 |
|
||||
| `IContentPublishOperationService` | Publish, Unpublish, Scheduling, Branch | ~1000* |
|
||||
| `IContentMoveService` | Move, RecycleBin, Copy, Sort | ~350 |
|
||||
|
||||
### Internal Helper Classes (4)
|
||||
*Revised estimate: `CommitDocumentChangesInternal` alone is 330+ lines
|
||||
|
||||
### New Public Service Interfaces - Revised Classification (5)
|
||||
|
||||
Based on actual API usage patterns, Query and Versioning are promoted to public:
|
||||
|
||||
| Interface | Responsibility | Est. Lines | Visibility |
|
||||
|-----------|---------------|------------|------------|
|
||||
| `IContentCrudService` | Create, Get, Save, Delete | ~400 | **Public** |
|
||||
| `IContentPublishOperationService` | Publish, Unpublish, Scheduling | ~1000 | **Public** |
|
||||
| `IContentMoveService` | Move, RecycleBin, Copy, Sort | ~350 | **Public** |
|
||||
| `IContentQueryService` | Count, Paged queries, Hierarchy | ~250 | **Public** |
|
||||
| `IContentVersionService` | Versions, Rollback, DeleteVersions | ~200 | **Public** |
|
||||
|
||||
### Internal Helper Classes (2)
|
||||
|
||||
| Class | Responsibility | Est. Lines |
|
||||
|-------|---------------|------------|
|
||||
| `ContentVersioningHelper` | Versions, Rollback, DeleteVersions | ~150 |
|
||||
| `ContentQueryHelper` | Count, Paged queries, Hierarchy | ~200 |
|
||||
| `ContentPermissionHelper` | Get/Set permissions | ~50 |
|
||||
| `ContentBlueprintHelper` | Blueprint CRUD | ~200 |
|
||||
| `ContentPermissionManager` | Get/Set permissions | ~50 |
|
||||
| `ContentBlueprintManager` | Blueprint CRUD | ~200 |
|
||||
|
||||
*Renamed from "Helper" to "Manager" per .NET naming conventions*
|
||||
|
||||
### ContentService Facade (~200 lines)
|
||||
|
||||
Thin wrapper delegating to services and helpers for backward compatibility.
|
||||
Thin wrapper delegating to services and managers for backward compatibility.
|
||||
|
||||
## Transaction/Scope Ownership
|
||||
|
||||
### Scope Creation Rules
|
||||
|
||||
**Pattern: Caller-Creates-Scope (Ambient Scope)**
|
||||
|
||||
All public service methods check for an existing ambient scope and use it if present. If no ambient scope exists, the method creates its own.
|
||||
|
||||
```csharp
|
||||
public OperationResult Save(IContent content, int? userId = null)
|
||||
{
|
||||
using ICoreScope scope = ScopeProvider.CreateCoreScope();
|
||||
// ... implementation
|
||||
scope.Complete();
|
||||
return result;
|
||||
}
|
||||
```
|
||||
|
||||
**Cross-Service Call Behavior**:
|
||||
- When Service A calls Service B, Service B detects the ambient scope from A
|
||||
- Nested scopes participate in the parent transaction
|
||||
- Only the outermost scope's `Complete()` commits the transaction
|
||||
- If any nested scope fails to complete, the entire transaction rolls back
|
||||
|
||||
**Facade Orchestration**:
|
||||
For operations requiring multiple services (e.g., `MoveToRecycleBin` which may need to unpublish), the facade creates the scope:
|
||||
|
||||
```csharp
|
||||
// In ContentService facade
|
||||
public OperationResult MoveToRecycleBin(IContent content, int userId)
|
||||
{
|
||||
using ICoreScope scope = ScopeProvider.CreateCoreScope();
|
||||
|
||||
// Unpublish if published (uses ambient scope)
|
||||
if (content.Published)
|
||||
{
|
||||
_publishOperationService.Unpublish(content, "*", userId);
|
||||
}
|
||||
|
||||
// Move to bin (uses same ambient scope)
|
||||
var result = _moveService.MoveToRecycleBinInternal(content, userId);
|
||||
|
||||
scope.Complete();
|
||||
return result;
|
||||
}
|
||||
```
|
||||
|
||||
### Lock Coordination
|
||||
|
||||
Write locks (`scope.WriteLock(Constants.Locks.ContentTree)`) are acquired by the service performing the operation. When services call each other within an ambient scope, locks are already held by the parent.
|
||||
|
||||
**Rule**: Acquire locks at the highest level that creates the scope.
|
||||
|
||||
## File Structure
|
||||
|
||||
```
|
||||
src/Umbraco.Core/Services/
|
||||
├── IContentCrudService.cs
|
||||
├── IContentPublishingService.cs
|
||||
├── IContentPublishOperationService.cs # Renamed to avoid collision
|
||||
├── IContentMoveService.cs
|
||||
├── IContentQueryService.cs # Promoted to public
|
||||
├── IContentVersionService.cs # Promoted to public
|
||||
└── ContentService.cs (facade - existing file, refactored)
|
||||
|
||||
src/Umbraco.Infrastructure/Services/
|
||||
├── ContentCrudService.cs
|
||||
├── ContentPublishingService.cs
|
||||
├── ContentPublishOperationService.cs # Renamed to avoid collision
|
||||
├── ContentMoveService.cs
|
||||
├── ContentQueryService.cs # Promoted to public
|
||||
├── ContentVersionService.cs # Promoted to public
|
||||
├── ContentServiceBase.cs (shared infrastructure)
|
||||
└── Helpers/
|
||||
├── ContentVersioningHelper.cs
|
||||
├── ContentQueryHelper.cs
|
||||
├── ContentPermissionHelper.cs
|
||||
└── ContentBlueprintHelper.cs
|
||||
└── Content/
|
||||
├── ContentPermissionManager.cs # Renamed from Helper
|
||||
└── ContentBlueprintManager.cs # Renamed from Helper
|
||||
```
|
||||
|
||||
## Detailed Interface Designs
|
||||
@@ -99,10 +221,12 @@ public interface IContentCrudService
|
||||
}
|
||||
```
|
||||
|
||||
### IContentPublishingService
|
||||
### IContentPublishOperationService
|
||||
|
||||
*Renamed from `IContentPublishingService` to avoid collision with existing API-layer service.*
|
||||
|
||||
```csharp
|
||||
public interface IContentPublishingService
|
||||
public interface IContentPublishOperationService
|
||||
{
|
||||
// Publishing
|
||||
PublishResult Publish(IContent content, string[] cultures, int userId = Constants.Security.SuperUserId);
|
||||
@@ -125,6 +249,9 @@ public interface IContentPublishingService
|
||||
// Path/Status Checks
|
||||
bool IsPathPublishable(IContent content);
|
||||
bool IsPathPublished(IContent? content);
|
||||
|
||||
// Send to Publication (workflow)
|
||||
bool SendToPublication(IContent? content, int userId = Constants.Security.SuperUserId);
|
||||
}
|
||||
```
|
||||
|
||||
@@ -153,45 +280,57 @@ public interface IContentMoveService
|
||||
}
|
||||
```
|
||||
|
||||
### Internal Helpers
|
||||
### IContentVersionService (Promoted to Public)
|
||||
|
||||
```csharp
|
||||
internal class ContentVersioningHelper
|
||||
public interface IContentVersionService
|
||||
{
|
||||
IContent? GetVersion(int versionId);
|
||||
IEnumerable<IContent> GetVersions(int id);
|
||||
IEnumerable<IContent> GetVersionsSlim(int id, int skip, int take);
|
||||
IEnumerable<int> GetVersionIds(int id, int maxRows);
|
||||
OperationResult Rollback(int id, int versionId, string culture, int userId);
|
||||
void DeleteVersions(int id, DateTime versionDate, int userId);
|
||||
void DeleteVersion(int id, int versionId, bool deletePriorVersions, int userId);
|
||||
OperationResult Rollback(int id, int versionId, string culture = "*", int userId = Constants.Security.SuperUserId);
|
||||
void DeleteVersions(int id, DateTime versionDate, int userId = Constants.Security.SuperUserId);
|
||||
void DeleteVersion(int id, int versionId, bool deletePriorVersions, int userId = Constants.Security.SuperUserId);
|
||||
}
|
||||
```
|
||||
|
||||
internal class ContentQueryHelper
|
||||
### IContentQueryService (Promoted to Public)
|
||||
|
||||
```csharp
|
||||
public interface IContentQueryService
|
||||
{
|
||||
// Counting
|
||||
int Count(string? contentTypeAlias = null);
|
||||
int CountPublished(string? contentTypeAlias = null);
|
||||
int CountChildren(int parentId, string? contentTypeAlias = null);
|
||||
int CountDescendants(int parentId, string? contentTypeAlias = null);
|
||||
bool HasChildren(int id);
|
||||
|
||||
// Hierarchy
|
||||
IEnumerable<IContent> GetByLevel(int level);
|
||||
IEnumerable<IContent> GetAncestors(int id);
|
||||
IEnumerable<IContent> GetAncestors(IContent content);
|
||||
IEnumerable<IContent> GetPublishedChildren(int id);
|
||||
bool HasChildren(int id);
|
||||
IEnumerable<IContent> GetPagedChildren(...);
|
||||
IEnumerable<IContent> GetPagedDescendants(...);
|
||||
IEnumerable<IContent> GetPagedOfType(...);
|
||||
IEnumerable<IContent> GetPagedOfTypes(...);
|
||||
}
|
||||
|
||||
internal class ContentPermissionHelper
|
||||
// Paged Queries
|
||||
IEnumerable<IContent> GetPagedChildren(int id, long pageIndex, int pageSize, out long totalRecords, IQuery<IContent>? filter = null, Ordering? ordering = null);
|
||||
IEnumerable<IContent> GetPagedDescendants(int id, long pageIndex, int pageSize, out long totalRecords, IQuery<IContent>? filter = null, Ordering? ordering = null);
|
||||
IEnumerable<IContent> GetPagedOfType(int contentTypeId, long pageIndex, int pageSize, out long totalRecords, IQuery<IContent> filter, Ordering? ordering = null);
|
||||
IEnumerable<IContent> GetPagedOfTypes(int[] contentTypeIds, long pageIndex, int pageSize, out long totalRecords, IQuery<IContent>? filter, Ordering? ordering = null);
|
||||
}
|
||||
```
|
||||
|
||||
### Internal Managers
|
||||
|
||||
```csharp
|
||||
internal class ContentPermissionManager
|
||||
{
|
||||
void SetPermissions(EntityPermissionSet permissionSet);
|
||||
void SetPermission(IContent entity, string permission, IEnumerable<int> groupIds);
|
||||
EntityPermissionCollection GetPermissions(IContent content);
|
||||
}
|
||||
|
||||
internal class ContentBlueprintHelper
|
||||
internal class ContentBlueprintManager
|
||||
{
|
||||
IContent? GetBlueprintById(int id);
|
||||
IContent? GetBlueprintById(Guid id);
|
||||
@@ -199,6 +338,7 @@ internal class ContentBlueprintHelper
|
||||
void DeleteBlueprint(IContent content, int userId);
|
||||
IContent CreateBlueprintFromContent(IContent blueprint, string name, int userId);
|
||||
IEnumerable<IContent> GetBlueprintsForContentTypes(params int[] contentTypeId);
|
||||
void DeleteBlueprintsOfType(int contentTypeId, int userId);
|
||||
void DeleteBlueprintsOfTypes(IEnumerable<int> contentTypeIds, int userId);
|
||||
}
|
||||
```
|
||||
@@ -227,14 +367,14 @@ public class ContentServicesComposer : IComposer
|
||||
{
|
||||
// Public services
|
||||
builder.Services.AddScoped<IContentCrudService, ContentCrudService>();
|
||||
builder.Services.AddScoped<IContentPublishingService, ContentPublishingService>();
|
||||
builder.Services.AddScoped<IContentPublishOperationService, ContentPublishOperationService>();
|
||||
builder.Services.AddScoped<IContentMoveService, ContentMoveService>();
|
||||
builder.Services.AddScoped<IContentQueryService, ContentQueryService>();
|
||||
builder.Services.AddScoped<IContentVersionService, ContentVersionService>();
|
||||
|
||||
// Internal helpers
|
||||
builder.Services.AddScoped<ContentVersioningHelper>();
|
||||
builder.Services.AddScoped<ContentQueryHelper>();
|
||||
builder.Services.AddScoped<ContentPermissionHelper>();
|
||||
builder.Services.AddScoped<ContentBlueprintHelper>();
|
||||
// Internal managers
|
||||
builder.Services.AddScoped<ContentPermissionManager>();
|
||||
builder.Services.AddScoped<ContentBlueprintManager>();
|
||||
|
||||
// Facade (backward compatible)
|
||||
builder.Services.AddScoped<IContentService, ContentService>();
|
||||
@@ -244,35 +384,633 @@ public class ContentServicesComposer : IComposer
|
||||
|
||||
## Implementation Order
|
||||
|
||||
1. **Phase 1: CRUD Service** - Establish patterns
|
||||
2. **Phase 2: Publishing Service** - Most complex, tackle early
|
||||
3. **Phase 3: Move Service** - Depends on CRUD/Publishing
|
||||
4. **Phase 4: Versioning Helper** - Depends on CRUD
|
||||
5. **Phase 5: Query Helper** - Utility operations
|
||||
6. **Phase 6: Permission Helper** - Small extraction
|
||||
7. **Phase 7: Blueprint Helper** - Final cleanup
|
||||
8. **Phase 8: Facade** - Wire everything together
|
||||
Each phase MUST run tests before and after to verify no regressions.
|
||||
|
||||
| Phase | Service | Tests to Run | Gate |
|
||||
|-------|---------|--------------|------|
|
||||
| 0 | Write tests | `ContentServiceRefactoringTests` | All 15 pass |
|
||||
| 1 | CRUD Service | All ContentService*Tests | All pass |
|
||||
| 2 | Query Service | All ContentService*Tests | All pass |
|
||||
| 3 | Version Service | All ContentService*Tests | All pass |
|
||||
| 4 | Move Service | All ContentService*Tests + Sort/MoveToRecycleBin tests | All pass |
|
||||
| 5 | Publish Operation Service | All ContentService*Tests + Notification ordering tests | All pass |
|
||||
| 6 | Permission Manager | All ContentService*Tests + Permission tests | All pass |
|
||||
| 7 | Blueprint Manager | All ContentService*Tests | All pass |
|
||||
| 8 | Facade | **Full test suite** | All pass |
|
||||
|
||||
### Phase Details
|
||||
|
||||
1. **Phase 0: Write Tests** - Create `ContentServiceRefactoringTests.cs` with all 15 tests
|
||||
2. **Phase 1: CRUD Service** - Establish patterns, base class
|
||||
3. **Phase 2: Query Service** - Read-only operations, low risk
|
||||
4. **Phase 3: Version Service** - Straightforward extraction
|
||||
5. **Phase 4: Move Service** - Depends on CRUD; Sort and MoveToRecycleBin tests critical
|
||||
6. **Phase 5: Publish Operation Service** - Most complex; notification ordering tests critical
|
||||
7. **Phase 6: Permission Manager** - Small extraction; permission tests critical
|
||||
8. **Phase 7: Blueprint Manager** - Final cleanup
|
||||
9. **Phase 8: Facade** - Wire everything together, add async methods
|
||||
|
||||
### Test Execution Commands
|
||||
|
||||
```bash
|
||||
# Run refactoring-specific tests (fast feedback)
|
||||
dotnet test tests/Umbraco.Tests.Integration --filter "FullyQualifiedName~ContentServiceRefactoringTests"
|
||||
|
||||
# Run all ContentService tests (phase gate)
|
||||
dotnet test tests/Umbraco.Tests.Integration --filter "FullyQualifiedName~ContentService"
|
||||
|
||||
# Run full integration test suite (final gate)
|
||||
dotnet test tests/Umbraco.Tests.Integration
|
||||
```
|
||||
|
||||
### Regression Protocol
|
||||
|
||||
If any test fails during a phase:
|
||||
1. **STOP** - Do not proceed to next phase
|
||||
2. **DIAGNOSE** - Identify which behavior changed
|
||||
3. **FIX** - Restore expected behavior
|
||||
4. **VERIFY** - Re-run all tests for current phase
|
||||
5. **CONTINUE** - Only after all tests pass
|
||||
|
||||
## Complete Method Mapping
|
||||
|
||||
This table maps ALL methods from `IContentService` (and its base `IContentServiceBase<IContent>`) to the proposed services/managers.
|
||||
|
||||
### IContentCrudService Methods
|
||||
|
||||
| Current Method | Target | Notes |
|
||||
|----------------|--------|-------|
|
||||
| `Create(string name, Guid parentId, string docTypeAlias, int userId)` | IContentCrudService | |
|
||||
| `Create(string name, int parentId, string docTypeAlias, int userId)` | IContentCrudService | |
|
||||
| `Create(string name, int parentId, IContentType contentType, int userId)` | IContentCrudService | |
|
||||
| `Create(string name, IContent? parent, string docTypeAlias, int userId)` | IContentCrudService | |
|
||||
| `CreateAndSave(string name, int parentId, string contentTypeAlias, int userId)` | IContentCrudService | Calls Save internally |
|
||||
| `CreateAndSave(string name, IContent parent, string contentTypeAlias, int userId)` | IContentCrudService | Calls Save internally |
|
||||
| `GetById(int id)` | IContentCrudService | |
|
||||
| `GetById(Guid key)` | IContentCrudService | |
|
||||
| `GetByIds(IEnumerable<int> ids)` | IContentCrudService | |
|
||||
| `GetByIds(IEnumerable<Guid> ids)` | IContentCrudService | |
|
||||
| `GetRootContent()` | IContentCrudService | |
|
||||
| `GetParent(int id)` | IContentCrudService | |
|
||||
| `GetParent(IContent content)` | IContentCrudService | |
|
||||
| `Save(IContent content, int? userId, ContentScheduleCollection? schedule)` | IContentCrudService | |
|
||||
| `Save(IEnumerable<IContent> contents, int userId)` | IContentCrudService | |
|
||||
| `Delete(IContent content, int userId)` | IContentCrudService | |
|
||||
| `DeleteOfType(int documentTypeId, int userId)` | IContentCrudService | *Orchestration: moves descendants to bin first* |
|
||||
| `DeleteOfTypes(IEnumerable<int> contentTypeIds, int userId)` | IContentCrudService | *Orchestration: moves descendants to bin first* |
|
||||
|
||||
### IContentPublishOperationService Methods
|
||||
|
||||
| Current Method | Target | Notes |
|
||||
|----------------|--------|-------|
|
||||
| `Publish(IContent content, string[] cultures, int userId)` | IContentPublishOperationService | |
|
||||
| `PublishBranch(IContent content, PublishBranchFilter filter, string[] cultures, int userId)` | IContentPublishOperationService | Complex: ~200 lines |
|
||||
| `Unpublish(IContent content, string? culture, int userId)` | IContentPublishOperationService | |
|
||||
| `IsPathPublishable(IContent content)` | IContentPublishOperationService | Read-only |
|
||||
| `IsPathPublished(IContent content)` | IContentPublishOperationService | Read-only |
|
||||
| `SendToPublication(IContent? content, int userId)` | IContentPublishOperationService | Workflow trigger |
|
||||
| `PerformScheduledPublish(DateTime date)` | IContentPublishOperationService | Scheduled job entry point |
|
||||
| `GetContentForExpiration(DateTime date)` | IContentPublishOperationService | Schedule query |
|
||||
| `GetContentForRelease(DateTime date)` | IContentPublishOperationService | Schedule query |
|
||||
| `GetContentScheduleByContentId(int contentId)` | IContentPublishOperationService | |
|
||||
| `GetContentScheduleByContentId(Guid contentId)` | IContentPublishOperationService | |
|
||||
| `PersistContentSchedule(IContent content, ContentScheduleCollection schedule)` | IContentPublishOperationService | |
|
||||
| `GetContentSchedulesByIds(Guid[] keys)` | IContentPublishOperationService | Bulk query |
|
||||
|
||||
### IContentMoveService Methods
|
||||
|
||||
| Current Method | Target | Notes |
|
||||
|----------------|--------|-------|
|
||||
| `Move(IContent content, int parentId, int userId)` | IContentMoveService | |
|
||||
| `MoveToRecycleBin(IContent content, int userId)` | **Facade** | *Orchestration: may unpublish first* |
|
||||
| `EmptyRecycleBin(int userId)` | IContentMoveService | |
|
||||
| `EmptyRecycleBinAsync(Guid userId)` | IContentMoveService | Async variant |
|
||||
| `RecycleBinSmells()` | IContentMoveService | |
|
||||
| `GetPagedContentInRecycleBin(...)` | IContentMoveService | |
|
||||
| `Copy(IContent content, int parentId, bool relateToOriginal, int userId)` | IContentMoveService | |
|
||||
| `Copy(IContent content, int parentId, bool relateToOriginal, bool recursive, int userId)` | IContentMoveService | |
|
||||
| `Sort(IEnumerable<IContent> items, int userId)` | IContentMoveService | |
|
||||
| `Sort(IEnumerable<int>? ids, int userId)` | IContentMoveService | |
|
||||
|
||||
### IContentQueryService Methods
|
||||
|
||||
| Current Method | Target | Notes |
|
||||
|----------------|--------|-------|
|
||||
| `Count(string? documentTypeAlias)` | IContentQueryService | |
|
||||
| `CountPublished(string? documentTypeAlias)` | IContentQueryService | |
|
||||
| `CountChildren(int parentId, string? documentTypeAlias)` | IContentQueryService | |
|
||||
| `CountDescendants(int parentId, string? documentTypeAlias)` | IContentQueryService | |
|
||||
| `HasChildren(int id)` | IContentQueryService | |
|
||||
| `GetByLevel(int level)` | IContentQueryService | |
|
||||
| `GetAncestors(int id)` | IContentQueryService | |
|
||||
| `GetAncestors(IContent content)` | IContentQueryService | |
|
||||
| `GetPagedChildren(int id, ...)` | IContentQueryService | |
|
||||
| `GetPagedDescendants(int id, ...)` | IContentQueryService | |
|
||||
| `GetPagedOfType(int contentTypeId, ...)` | IContentQueryService | |
|
||||
| `GetPagedOfTypes(int[] contentTypeIds, ...)` | IContentQueryService | |
|
||||
|
||||
### IContentVersionService Methods
|
||||
|
||||
| Current Method | Target | Notes |
|
||||
|----------------|--------|-------|
|
||||
| `GetVersion(int versionId)` | IContentVersionService | |
|
||||
| `GetVersions(int id)` | IContentVersionService | |
|
||||
| `GetVersionsSlim(int id, int skip, int take)` | IContentVersionService | |
|
||||
| `GetVersionIds(int id, int topRows)` | IContentVersionService | |
|
||||
| `Rollback(int id, int versionId, string culture, int userId)` | IContentVersionService | |
|
||||
| `DeleteVersions(int id, DateTime date, int userId)` | IContentVersionService | |
|
||||
| `DeleteVersion(int id, int versionId, bool deletePriorVersions, int userId)` | IContentVersionService | |
|
||||
|
||||
### ContentPermissionManager Methods (Internal)
|
||||
|
||||
| Current Method | Target | Notes |
|
||||
|----------------|--------|-------|
|
||||
| `GetPermissions(IContent content)` | ContentPermissionManager | |
|
||||
| `SetPermissions(EntityPermissionSet permissionSet)` | ContentPermissionManager | |
|
||||
| `SetPermission(IContent entity, string permission, IEnumerable<int> groupIds)` | ContentPermissionManager | |
|
||||
|
||||
### ContentBlueprintManager Methods (Internal)
|
||||
|
||||
| Current Method | Target | Notes |
|
||||
|----------------|--------|-------|
|
||||
| `GetBlueprintById(int id)` | ContentBlueprintManager | |
|
||||
| `GetBlueprintById(Guid id)` | ContentBlueprintManager | |
|
||||
| `GetBlueprintsForContentTypes(params int[] documentTypeId)` | ContentBlueprintManager | |
|
||||
| `SaveBlueprint(IContent content, int userId)` | ContentBlueprintManager | Obsolete overload |
|
||||
| `SaveBlueprint(IContent content, IContent? createdFromContent, int userId)` | ContentBlueprintManager | |
|
||||
| `DeleteBlueprint(IContent content, int userId)` | ContentBlueprintManager | |
|
||||
| `CreateBlueprintFromContent(IContent blueprint, string name, int userId)` | ContentBlueprintManager | |
|
||||
| `CreateContentFromBlueprint(IContent blueprint, string name, int userId)` | ContentBlueprintManager | Obsolete |
|
||||
| `DeleteBlueprintsOfType(int contentTypeId, int userId)` | ContentBlueprintManager | |
|
||||
| `DeleteBlueprintsOfTypes(IEnumerable<int> contentTypeIds, int userId)` | ContentBlueprintManager | |
|
||||
|
||||
### Facade Orchestration Methods
|
||||
|
||||
These methods stay in the facade because they coordinate multiple services:
|
||||
|
||||
| Method | Why in Facade |
|
||||
|--------|---------------|
|
||||
| `MoveToRecycleBin` | Unpublishes content (IContentPublishOperationService) then moves (IContentMoveService) |
|
||||
| `DeleteOfType` / `DeleteOfTypes` | Moves descendants to bin first, then deletes type content |
|
||||
|
||||
## Notification Responsibility Matrix
|
||||
|
||||
Each service fires only its own notifications. State is preserved via `WithStateFrom()` for notification continuity.
|
||||
|
||||
| Notification | Firing Service | Notes |
|
||||
|--------------|----------------|-------|
|
||||
| `ContentSavingNotification` | IContentCrudService | Cancellable |
|
||||
| `ContentSavedNotification` | IContentCrudService | |
|
||||
| `ContentDeletingNotification` | IContentCrudService | Cancellable |
|
||||
| `ContentDeletedNotification` | IContentCrudService | |
|
||||
| `ContentPublishingNotification` | IContentPublishOperationService | Cancellable |
|
||||
| `ContentPublishedNotification` | IContentPublishOperationService | |
|
||||
| `ContentUnpublishingNotification` | IContentPublishOperationService | Cancellable |
|
||||
| `ContentUnpublishedNotification` | IContentPublishOperationService | |
|
||||
| `ContentMovingNotification` | IContentMoveService | Cancellable |
|
||||
| `ContentMovedNotification` | IContentMoveService | |
|
||||
| `ContentMovingToRecycleBinNotification` | **Facade** | Orchestrated operation |
|
||||
| `ContentMovedToRecycleBinNotification` | **Facade** | Orchestrated operation |
|
||||
| `ContentCopyingNotification` | IContentMoveService | Cancellable |
|
||||
| `ContentCopiedNotification` | IContentMoveService | |
|
||||
| `ContentSortingNotification` | IContentMoveService | Cancellable |
|
||||
| `ContentSortedNotification` | IContentMoveService | |
|
||||
| `ContentEmptyingRecycleBinNotification` | IContentMoveService | Cancellable |
|
||||
| `ContentEmptiedRecycleBinNotification` | IContentMoveService | |
|
||||
| `ContentRollingBackNotification` | IContentVersionService | Cancellable |
|
||||
| `ContentRolledBackNotification` | IContentVersionService | |
|
||||
| `ContentSendingToPublishNotification` | IContentPublishOperationService | Cancellable |
|
||||
| `ContentSentToPublishNotification` | IContentPublishOperationService | |
|
||||
| `ContentSavingBlueprintNotification` | ContentBlueprintManager | Via IEventAggregator |
|
||||
| `ContentSavedBlueprintNotification` | ContentBlueprintManager | Via IEventAggregator |
|
||||
| `ContentDeletedBlueprintNotification` | ContentBlueprintManager | Via IEventAggregator |
|
||||
|
||||
**State Preservation Rule**: When a method chains operations (e.g., Save → Publish), the second notification carries state from the first using `WithStateFrom()`.
|
||||
|
||||
## Risk Mitigation
|
||||
|
||||
| Risk | Mitigation |
|
||||
|------|------------|
|
||||
| Circular dependencies | Inject repository directly where needed |
|
||||
| Transaction boundaries | Ensure scopes work across service calls |
|
||||
| Notification consistency | Events fire from correct service |
|
||||
| Circular dependencies | Unidirectional: Publish/Move → CRUD only; no reverse |
|
||||
| Transaction boundaries | Ambient scope pattern (see Transaction/Scope Ownership) |
|
||||
| Notification consistency | Notification responsibility matrix per service |
|
||||
| Breaking changes | Extensive test coverage before refactor |
|
||||
| Naming collision | Renamed to IContentPublishOperationService |
|
||||
| Cross-service calls | Facade orchestrates complex multi-service operations |
|
||||
|
||||
## Async Considerations
|
||||
|
||||
Methods with async variants should define async-first interfaces where database I/O occurs:
|
||||
|
||||
| Current Method | Async Variant Needed |
|
||||
|----------------|---------------------|
|
||||
| `EmptyRecycleBin` | ✅ Already exists: `EmptyRecycleBinAsync` |
|
||||
| `Delete` (bulk) | Consider for large deletions |
|
||||
| `PublishBranch` | Consider for large trees |
|
||||
|
||||
**Decision**: New services should provide async overloads for operations that can process large datasets.
|
||||
|
||||
## Clarifications (from Architectural Review)
|
||||
|
||||
### Q1: Existing IContentPublishingService handling?
|
||||
**A**: The existing `IContentPublishingService` (API-layer) remains unchanged. It continues to orchestrate via `IContentService`. After refactoring, it will indirectly use the new `IContentPublishOperationService` through the facade.
|
||||
|
||||
### Q2: PublishBranch ownership?
|
||||
**A**: `PublishBranch` belongs entirely to `IContentPublishOperationService`. Tree traversal is part of publishing logic in this context.
|
||||
|
||||
### Q3: Locking strategy?
|
||||
**A**: Locks are acquired by the scope creator (highest-level caller). Nested service calls within an ambient scope inherit the existing locks.
|
||||
|
||||
### Q4: Culture-variant complexity (CommitDocumentChangesInternal)?
|
||||
**A**: The full complexity (~330 lines) stays in `ContentPublishOperationService`. No further splitting at this time; revisit if the class exceeds 1200 lines.
|
||||
|
||||
### Q5: RepositoryService base class?
|
||||
**A**: New services inherit from `ContentServiceBase` (see Shared Infrastructure), which wraps scope/repository access similarly to `RepositoryService` but is specific to content operations.
|
||||
|
||||
## Performance Optimizations
|
||||
|
||||
Performance improvements to implement during the refactoring, organized into four areas with incremental steps.
|
||||
|
||||
### 1. Database Query Efficiency
|
||||
|
||||
**Approach**: Batch lookups to eliminate N+1 queries.
|
||||
|
||||
| Step | Change |
|
||||
|------|--------|
|
||||
| 1 | Add `IIdKeyMap.GetIdsForKeys(Guid[] keys, UmbracoObjectTypes type)` for batch key-to-id resolution |
|
||||
| 2 | Add `GetSchedulesByContentIds(int[] ids)` to batch schedule lookups |
|
||||
| 3 | Add `ArePathsPublished(int[] contentIds)` for batch path validation |
|
||||
| 4 | Add `GetParents(int[] contentIds)` for batch ancestor lookups |
|
||||
|
||||
**Key locations to fix**:
|
||||
- `GetContentSchedulesByIds` (line 1025-1049): N+1 in `_idKeyMap.GetIdForKey` calls
|
||||
- `CommitDocumentChangesInternal` (line 1461+): Multiple repository calls in same scope
|
||||
- `IsPathPublishable` (line 1070), `GetAncestors` (line 792): Repeated single-item lookups
|
||||
|
||||
### 2. Memory Allocation Patterns
|
||||
|
||||
**Approach**: Aggressive optimization, implemented incrementally.
|
||||
|
||||
| Step | Focus | Changes |
|
||||
|------|-------|---------|
|
||||
| 1 | Remove unnecessary materializations | Delete `.ToArray()`/`.ToList()` before `foreach` loops |
|
||||
| 2 | Fix string allocations | Replace `+` concat in loops with `StringBuilder` |
|
||||
| 3 | Add StringBuilder pooling | Use `StringBuilderCache` or `ObjectPool<StringBuilder>` |
|
||||
| 4 | Eliminate closure allocations | Hoist lambdas, use static lambdas where possible |
|
||||
| 5 | Span for string operations | Replace substrings with `ReadOnlySpan<char>` |
|
||||
| 6 | ArrayPool for temp arrays | Pool arrays in batch operations |
|
||||
| 7 | Pooled collections | Use `ArrayPoolList<T>` or similar for hot paths |
|
||||
|
||||
**Key locations to fix**:
|
||||
- Lines 1170, 814, 2650: ToArray/ToList before iteration
|
||||
- Lines 1201, 2596-2598: String concat in loops
|
||||
- Lines 1125-1127: Lambda/closure on every save
|
||||
- Lines 555-556: Dictionary recreation
|
||||
|
||||
### 3. Concurrency & Locking
|
||||
|
||||
**Approach**: Moderate optimization focused on lock duration and correctness.
|
||||
|
||||
| Step | Focus | Changes |
|
||||
|------|-------|---------|
|
||||
| 1 | Remove unnecessary read locks | Skip `ReadLock` when data is retrieved from cache |
|
||||
| 2 | Lock before notification | Move `WriteLock` acquisition before `*SavingNotification` |
|
||||
| 3 | Batch descendant operations | In `PerformMoveLocked`, collect changes then batch-save |
|
||||
| 4 | Document lock contracts | Each new service documents its lock expectations |
|
||||
|
||||
**Current issues**:
|
||||
- Single coarse-grained lock (`ContentTree`) for all 60+ lock calls
|
||||
- Long lock hold in `PerformMoveLocked` (line 2600+) during descendant iteration
|
||||
- Lock acquired after notification (line 1114) creates race window
|
||||
|
||||
### 4. Caching Strategies
|
||||
|
||||
**Approach**: Moderate optimization with clear invalidation contracts.
|
||||
|
||||
| Step | Focus | Changes |
|
||||
|------|-------|---------|
|
||||
| 1 | Audit RefreshBranch usage | Switch to `RefreshNode` where only single node affected |
|
||||
| 2 | Parallelize cache clears | Run Memory, Navigation, Routing, Published status clears concurrently |
|
||||
| 3 | Fix N+1 in HandleNavigation | Batch descendant lookups instead of per-item queries |
|
||||
| 4 | Document invalidation contracts | Each service specifies which cache events it fires |
|
||||
|
||||
**Cache invalidation flow**:
|
||||
```
|
||||
ContentService operation
|
||||
→ ContentTreeChangeNotification
|
||||
→ ContentTreeChangeDistributedCacheNotificationHandler
|
||||
→ ContentCacheRefresher.Refresh()
|
||||
→ Repository cache, Elements cache, Navigation, URLs, Published status
|
||||
```
|
||||
|
||||
**Known limitations (out of scope)**:
|
||||
- Elements cache full clear: Requires architectural change (elements as entities, not JSON blobs)
|
||||
- Sync-over-async in cache refresh: Cross-cutting concern for separate initiative
|
||||
|
||||
## Testing Strategy
|
||||
|
||||
### Overview
|
||||
|
||||
1. Ensure existing unit tests pass throughout refactor
|
||||
2. Add new unit tests for each extracted service
|
||||
2. Add new integration tests for identified coverage gaps (15 tests)
|
||||
3. Integration tests validate end-to-end behavior unchanged
|
||||
4. Benchmark critical paths before/after
|
||||
|
||||
### Test File Organization
|
||||
|
||||
Create a single new test file for refactoring-specific coverage:
|
||||
|
||||
```
|
||||
tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/
|
||||
ContentServiceRefactoringTests.cs
|
||||
```
|
||||
|
||||
**Rationale**: These tests exist specifically to validate the refactoring. Grouping them together:
|
||||
- Makes clear which tests are refactoring-specific safety nets
|
||||
- Can be retired or merged once refactoring is stable
|
||||
- Avoids polluting existing test files
|
||||
|
||||
### Test Coverage Summary
|
||||
|
||||
| Gap Area | Test Count | Purpose |
|
||||
|----------|------------|---------|
|
||||
| Notification ordering | 2 | MoveToRecycleBin unpublish→move sequence |
|
||||
| Sort operation | 3 | Sort(IContent), Sort(ids), notification firing |
|
||||
| DeleteOfType | 3 | Descendants handling, mixed types, multiple types |
|
||||
| Permissions | 4 | SetPermission, SetPermissions, GetPermissions |
|
||||
| Transaction boundaries | 3 | Ambient scope across chained operations |
|
||||
| **Total** | **15** | |
|
||||
|
||||
### Notification Handler Extension
|
||||
|
||||
The existing `ContentNotificationHandler` pattern needs extension for new notifications:
|
||||
|
||||
```csharp
|
||||
internal sealed class ContentNotificationHandler :
|
||||
INotificationHandler<ContentSavingNotification>,
|
||||
INotificationHandler<ContentSavedNotification>,
|
||||
INotificationHandler<ContentPublishingNotification>,
|
||||
INotificationHandler<ContentPublishedNotification>,
|
||||
INotificationHandler<ContentUnpublishingNotification>,
|
||||
INotificationHandler<ContentUnpublishedNotification>,
|
||||
INotificationHandler<ContentMovingToRecycleBinNotification>, // NEW
|
||||
INotificationHandler<ContentMovedToRecycleBinNotification>, // NEW
|
||||
INotificationHandler<ContentSortingNotification>, // NEW
|
||||
INotificationHandler<ContentSortedNotification>, // NEW
|
||||
INotificationHandler<ContentTreeChangeNotification>
|
||||
{
|
||||
// Existing delegates...
|
||||
|
||||
// NEW delegates
|
||||
public static Action<ContentMovingToRecycleBinNotification> MovingToRecycleBin { get; set; }
|
||||
public static Action<ContentMovedToRecycleBinNotification> MovedToRecycleBin { get; set; }
|
||||
public static Action<ContentSortingNotification> Sorting { get; set; }
|
||||
public static Action<ContentSortedNotification> Sorted { get; set; }
|
||||
|
||||
// NEW handlers
|
||||
public void Handle(ContentMovingToRecycleBinNotification notification)
|
||||
=> MovingToRecycleBin?.Invoke(notification);
|
||||
public void Handle(ContentMovedToRecycleBinNotification notification)
|
||||
=> MovedToRecycleBin?.Invoke(notification);
|
||||
public void Handle(ContentSortingNotification notification)
|
||||
=> Sorting?.Invoke(notification);
|
||||
public void Handle(ContentSortedNotification notification)
|
||||
=> Sorted?.Invoke(notification);
|
||||
}
|
||||
```
|
||||
|
||||
### Detailed Test Specifications
|
||||
|
||||
#### 1. Notification Ordering Tests (2 tests)
|
||||
|
||||
**Test 1: MoveToRecycleBin_PublishedContent_UnpublishesBeforeMoving**
|
||||
|
||||
Verifies that for published content, `MoveToRecycleBin` fires notifications in order:
|
||||
1. `ContentUnpublishingNotification`
|
||||
2. `ContentUnpublishedNotification`
|
||||
3. `ContentMovingToRecycleBinNotification`
|
||||
4. `ContentMovedToRecycleBinNotification`
|
||||
|
||||
**Test 2: MoveToRecycleBin_UnpublishedContent_OnlyFiresMoveNotifications**
|
||||
|
||||
Verifies that for unpublished content, only move notifications fire (no unpublish notifications).
|
||||
|
||||
#### 2. Sort Operation Tests (3 tests)
|
||||
|
||||
**Test 3: Sort_WithContentItems_ChangesSortOrder**
|
||||
|
||||
Verifies `Sort(IEnumerable<IContent>)` correctly reorders children.
|
||||
|
||||
**Test 4: Sort_WithIds_ChangesSortOrder**
|
||||
|
||||
Verifies `Sort(IEnumerable<int>)` correctly reorders children by ID.
|
||||
|
||||
**Test 5: Sort_FiresSortingAndSortedNotifications**
|
||||
|
||||
Verifies `ContentSortingNotification` and `ContentSortedNotification` fire in sequence with correct entities.
|
||||
|
||||
#### 3. DeleteOfType Tests (3 tests)
|
||||
|
||||
**Test 6: DeleteOfType_MovesDescendantsToRecycleBinFirst**
|
||||
|
||||
Verifies hierarchical content (parent → child → grandchild) is fully deleted and recycle bin is empty afterward.
|
||||
|
||||
**Test 7: DeleteOfType_WithMixedTypes_OnlyDeletesSpecifiedType**
|
||||
|
||||
Verifies only content of the specified type is deleted; other types remain.
|
||||
|
||||
**Test 8: DeleteOfTypes_DeletesMultipleTypesAtOnce**
|
||||
|
||||
Verifies `DeleteOfTypes(IEnumerable<int>)` deletes multiple content types in single operation.
|
||||
|
||||
#### 4. Permission Tests (4 tests)
|
||||
|
||||
**Test 9: SetPermission_AssignsPermissionToUserGroup**
|
||||
|
||||
Verifies `SetPermission` assigns a permission and `GetPermissions` retrieves it.
|
||||
|
||||
**Test 10: SetPermission_MultiplePermissionsForSameGroup**
|
||||
|
||||
Verifies multiple `SetPermission` calls accumulate permissions for a user group.
|
||||
|
||||
**Test 11: SetPermissions_AssignsPermissionSet**
|
||||
|
||||
Verifies `SetPermissions(EntityPermissionSet)` assigns a complete permission set.
|
||||
|
||||
**Test 12: SetPermission_AssignsToMultipleUserGroups**
|
||||
|
||||
Verifies single `SetPermission` call can assign to multiple user groups simultaneously.
|
||||
|
||||
#### 5. Transaction Boundary Tests (3 tests)
|
||||
|
||||
**Test 13: AmbientScope_NestedOperationsShareTransaction**
|
||||
|
||||
Verifies that multiple operations within an uncompleted scope all roll back together.
|
||||
|
||||
**Test 14: AmbientScope_CompletedScopeCommitsAllOperations**
|
||||
|
||||
Verifies that multiple operations within a completed scope all commit together.
|
||||
|
||||
**Test 15: AmbientScope_MoveToRecycleBinRollsBackCompletely**
|
||||
|
||||
Verifies that `MoveToRecycleBin` (which internally unpublishes then moves) rolls back both operations when scope is not completed.
|
||||
|
||||
## Success Criteria
|
||||
|
||||
- [ ] All existing tests pass
|
||||
- [ ] No public API breaking changes
|
||||
- [ ] ContentService reduced to ~200 lines
|
||||
- [ ] Each new service independently testable
|
||||
- [ ] No performance regression
|
||||
- [ ] Notification ordering matches current behavior
|
||||
- [ ] All 80+ IContentService methods mapped to new services
|
||||
|
||||
### Test Coverage Criteria
|
||||
|
||||
- [ ] 15 new integration tests written and passing (ContentServiceRefactoringTests.cs)
|
||||
- [ ] Notification ordering tests verify MoveToRecycleBin sequence
|
||||
- [ ] Sort operation has full test coverage
|
||||
- [ ] DeleteOfType/DeleteOfTypes descendant handling verified
|
||||
- [ ] Permission operations (SetPermission, SetPermissions) tested
|
||||
- [ ] Transaction boundary tests confirm ambient scope behavior
|
||||
|
||||
### Performance Criteria
|
||||
|
||||
- [ ] N+1 queries eliminated (batch lookups implemented)
|
||||
- [ ] No unnecessary ToArray/ToList materializations in hot paths
|
||||
- [ ] Lock duration reduced for bulk operations
|
||||
- [ ] Each service documents its lock and cache invalidation contracts
|
||||
- [ ] Benchmarks show no regression (target: 10%+ improvement on batch operations)
|
||||
|
||||
## Performance Benchmarks
|
||||
|
||||
Dedicated benchmark suite for capturing baseline metrics and comparing before/after refactoring.
|
||||
|
||||
### Benchmark File
|
||||
|
||||
```
|
||||
tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/
|
||||
ContentServiceRefactoringBenchmarks.cs
|
||||
```
|
||||
|
||||
### Benchmark Infrastructure
|
||||
|
||||
**BenchmarkResult model:**
|
||||
```csharp
|
||||
public record BenchmarkResult(
|
||||
string Name,
|
||||
long ElapsedMs,
|
||||
int ItemCount,
|
||||
double MsPerItem,
|
||||
DateTime Timestamp);
|
||||
```
|
||||
|
||||
**Output format** (JSON for comparison):
|
||||
```json
|
||||
{
|
||||
"runDate": "2025-12-20T10:30:00Z",
|
||||
"branch": "refactor/ContentService",
|
||||
"commit": "f4a01ed",
|
||||
"results": {
|
||||
"Save_BatchOf100": { "elapsedMs": 245, "itemCount": 100, "msPerItem": 2.45 },
|
||||
"GetAncestors_DeepHierarchy": { "elapsedMs": 89, "itemCount": 10, "msPerItem": 8.9 }
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
### CRUD Operation Benchmarks (7 tests)
|
||||
|
||||
| Test Name | Operation | Data Size | What It Measures |
|
||||
|-----------|-----------|-----------|------------------|
|
||||
| `Save_SingleItem` | `Save(IContent)` | 1 | Single save latency |
|
||||
| `Save_BatchOf100` | `Save(IEnumerable<IContent>)` | 100 | Batch save throughput |
|
||||
| `Save_BatchOf1000` | `Save(IEnumerable<IContent>)` | 1000 | Large batch scalability |
|
||||
| `GetById_Single` | `GetById(int)` | 1 | Single retrieval latency |
|
||||
| `GetByIds_BatchOf100` | `GetByIds(IEnumerable<int>)` | 100 | Batch retrieval (N+1 detection) |
|
||||
| `Delete_SingleItem` | `Delete(IContent)` | 1 | Single delete latency |
|
||||
| `Delete_WithDescendants` | `Delete(IContent)` | Parent + 50 children | Cascade delete performance |
|
||||
|
||||
### Query Operation Benchmarks (6 tests)
|
||||
|
||||
| Test Name | Operation | Data Size | What It Measures |
|
||||
|-----------|-----------|-----------|------------------|
|
||||
| `GetPagedChildren_100Items` | `GetPagedChildren()` | 100 children | Paged query performance |
|
||||
| `GetPagedDescendants_DeepTree` | `GetPagedDescendants()` | 3 levels, 300 total | Tree traversal efficiency |
|
||||
| `GetAncestors_DeepHierarchy` | `GetAncestors()` | 10-level deep item | Ancestor lookup (N+1 prone) |
|
||||
| `Count_ByContentType` | `Count(alias)` | 500 items of type | Count query optimization |
|
||||
| `CountDescendants_LargeTree` | `CountDescendants()` | 500 descendants | Descendant counting |
|
||||
| `HasChildren_100Nodes` | `HasChildren()` × 100 | 100 parent nodes | Repeated single lookups |
|
||||
|
||||
### Publish Operation Benchmarks (7 tests)
|
||||
|
||||
| Test Name | Operation | Data Size | What It Measures |
|
||||
|-----------|-----------|-----------|------------------|
|
||||
| `Publish_SingleItem` | `Publish()` | 1 | Single publish latency |
|
||||
| `Publish_BatchOf50` | `Publish()` × 50 | 50 items | Repeated publish overhead |
|
||||
| `PublishBranch_ShallowTree` | `PublishBranch()` | 1 parent + 20 children | Branch publish (small) |
|
||||
| `PublishBranch_DeepTree` | `PublishBranch()` | 5 levels, 100 items | Branch publish scalability |
|
||||
| `Unpublish_SingleItem` | `Unpublish()` | 1 | Single unpublish latency |
|
||||
| `PerformScheduledPublish` | `PerformScheduledPublish()` | 50 scheduled items | Scheduled job performance |
|
||||
| `GetContentSchedulesByIds_100Items` | `GetContentSchedulesByIds()` | 100 items | N+1 issue (line 1025-1049) |
|
||||
|
||||
### Move Operation Benchmarks (8 tests)
|
||||
|
||||
| Test Name | Operation | Data Size | What It Measures |
|
||||
|-----------|-----------|-----------|------------------|
|
||||
| `Move_SingleItem` | `Move()` | 1 | Single move latency |
|
||||
| `Move_WithDescendants` | `Move()` | 1 parent + 50 children | Descendant path updates |
|
||||
| `MoveToRecycleBin_Published` | `MoveToRecycleBin()` | 1 published item | Unpublish + move orchestration |
|
||||
| `MoveToRecycleBin_LargeTree` | `MoveToRecycleBin()` | 100 descendants | Lock duration (line 2600+) |
|
||||
| `Copy_SingleItem` | `Copy()` | 1 | Single copy latency |
|
||||
| `Copy_Recursive_50Items` | `Copy(..., recursive: true)` | 50 descendants | Recursive copy overhead |
|
||||
| `Sort_100Children` | `Sort(IEnumerable<IContent>)` | 100 siblings | Sort operation efficiency |
|
||||
| `EmptyRecycleBin_100Items` | `EmptyRecycleBin()` | 100 items in bin | Bulk delete performance |
|
||||
|
||||
### Version Operation Benchmarks (4 tests)
|
||||
|
||||
| Test Name | Operation | Data Size | What It Measures |
|
||||
|-----------|-----------|-----------|------------------|
|
||||
| `GetVersions_ItemWith50Versions` | `GetVersions()` | 50 versions | Version history retrieval |
|
||||
| `GetVersionsSlim_Paged` | `GetVersionsSlim(skip, take)` | 50 versions, page of 10 | Paged version query |
|
||||
| `Rollback_ToVersion` | `Rollback()` | 1 item with 10 versions | Rollback latency |
|
||||
| `DeleteVersions_ByDate` | `DeleteVersions()` | 50 versions | Version cleanup performance |
|
||||
|
||||
### Baseline Comparison (1 test)
|
||||
|
||||
| Test Name | Purpose |
|
||||
|-----------|---------|
|
||||
| `CompareAgainstBaseline` | Compares current run against saved baseline, fails if >20% regression |
|
||||
|
||||
### Benchmark Summary
|
||||
|
||||
| Category | Test Count |
|
||||
|----------|------------|
|
||||
| CRUD Operations | 7 |
|
||||
| Query Operations | 6 |
|
||||
| Publish Operations | 7 |
|
||||
| Move Operations | 8 |
|
||||
| Version Operations | 4 |
|
||||
| Comparison | 1 |
|
||||
| **Total** | **33** |
|
||||
|
||||
### Benchmark Execution Commands
|
||||
|
||||
```bash
|
||||
# Run all benchmarks and save as baseline (before refactoring)
|
||||
dotnet test tests/Umbraco.Tests.Integration \
|
||||
--filter "Category=Benchmark&FullyQualifiedName~ContentServiceRefactoringBenchmarks" \
|
||||
-- TestRunParameters.Parameter(name="SaveAsBaseline", value="true")
|
||||
|
||||
# Run benchmarks and compare against baseline (after each phase)
|
||||
dotnet test tests/Umbraco.Tests.Integration \
|
||||
--filter "Category=Benchmark&FullyQualifiedName~ContentServiceRefactoringBenchmarks"
|
||||
```
|
||||
|
||||
### Sample Comparison Output
|
||||
|
||||
```
|
||||
| Operation | Baseline | Current | Change |
|
||||
|------------------------------|----------|---------|---------|
|
||||
| Save_BatchOf100 | 245ms | 198ms | -19.2% |
|
||||
| GetContentSchedulesByIds | 892ms | 156ms | -82.5% | ✓ N+1 fixed
|
||||
| MoveToRecycleBin_LargeTree | 1240ms | 1180ms | -4.8% |
|
||||
```
|
||||
|
||||
Reference in New Issue
Block a user