Performance: Implement key-based caching for data type and template repositories#21280
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements GUID key caching for DataTypeRepository and TemplateRepository to improve lookup performance by ensuring entities are cached for both int ID and GUID lookups bidirectionally.
Key changes:
- Adds nested read repositories (
TemplateByGuidReadRepositoryandDataTypeByGuidReadRepository) following the existing pattern used inDocumentRepositoryandMediaRepository - Implements bidirectional cache population where retrieving by ID also caches by GUID and vice versa
- Adds comprehensive integration tests verifying the caching behavior for all three repository types (templates, data types, and content types)
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Umbraco.Infrastructure/Persistence/Repositories/Implement/TemplateRepository.cs | Adds nested TemplateByGuidReadRepository for GUID lookups, implements cache population in PerformGet, PerformGetAll, and Save, includes obsolete constructor for backward compatibility |
| src/Umbraco.Infrastructure/Persistence/Repositories/Implement/DataTypeRepository.cs | Adds nested DataTypeByGuidReadRepository for GUID lookups with database queries, implements bidirectional cache population methods, updates Get(Guid) to use new repository |
| src/Umbraco.Core/Persistence/Repositories/ITemplateRepository.cs | Adds Get(Guid key) method with default implementation throwing NotImplementedException |
| tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/Repositories/TemplateRepositoryTest.cs | Adds five new integration tests verifying caching behavior for ID and GUID lookups in various scenarios |
| tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/Repositories/DataTypeRepositoryTest.cs | New test file with five integration tests mirroring template repository tests for data types |
| tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/Repositories/ContentTypeRepositoryTest.cs | Adds four integration tests documenting existing caching behavior, adds helper dependencies, adds CreateRepository method |
| tests/Umbraco.Tests.Common/Builders/DataTypeBuilder.cs | Updates default creator ID to use constant Constants.Security.SuperUserId instead of hardcoded 1 |
src/Umbraco.Core/Persistence/Repositories/ITemplateRepository.cs
Outdated
Show resolved
Hide resolved
....Tests.Integration/Umbraco.Infrastructure/Persistence/Repositories/TemplateRepositoryTest.cs
Outdated
Show resolved
Hide resolved
|
@AndyButland I have pushed some implementation details - could you have a look at the latest commits? Also managed to trigger a null reference exception that we have seemingly failed to safeguard against in a long time... so that's fixed as part of the changes. |
|
Your updates look good to me @kjac. I've just triggered the build checks (they didn't run after your last update for some reason). But if you are OK with this now, please approve and I'll merge it in. |
kjac
left a comment
There was a problem hiding this comment.
Looks good and works for me 👍
Description
We currently have nested repositories for reading documents and media by GUID, that are aligned with the main repository to ensure that if a request is made by Id, it's cached by key, and vice versa (see #21124).
Whilst this may not be architecturally ideal, it does follow an existing and established pattern, so seems reasonable in advance of more significant repository refactoring.
In practice, this didn't show any obvious improvement in save/publish time, but nonetheless feels a sensible update to make to ensure these requests for data types and templates are cached however they are retrieved.
In working on this PR I verified that this is already in place for content types, as we have policy there of retrieving all and caching them, so by ID/key look-ups always come from this already cached collection.
Change Details
DataTypeRepositoryandTemplateRepositoryto improve lookup performanceTesting
Integration tests have been added to verify the behaviour: