Repository Caches: Fix GUID read repository cache key collision causing GetAll failures (closes #21756)

[AndyButland]

Description

Fixes #21756 — DataTypeService.GetAllAsync() (and equivalent calls for Document, Media, and Template) throws InvalidOperationException on the second invocation when the cache is warm.

Root Cause

GUID-keyed read repositories (e.g. DataTypeByGuidReadRepository) and their parent int-keyed repositories both resolve to the same IAppPolicyCache via IsolatedCaches.GetOrCreate<TEntity>(), and both used the same cache key prefix ("uRepo_{TypeName}_"). When DefaultRepositoryCachePolicy.GetAll runs its count validation, it finds both int-keyed and GUID-keyed entries under the same prefix, doubling the expected count. This causes a count mismatch, which falls through to PerformGetAll → GetBaseQuery(isCount: true) — a method that throws InvalidOperationException("This method won't be implemented.") on the inner GUID read repositories.

As well as throwing this exception, investigating and fixing this bug also revealed that the int-keyed GetAll path suffered a performance issue: GUID entries inflated the prefix-based cache count, causing count validation to always fail even after a successful GetAll populated the cache. This forced a full database re-query on every subsequent GetAll call.

Finally, I also found that we were missing some specific invalidations of the GUID caches.

Fixes

Introduced GuidReadRepositoryCachePolicy<TEntity> with a distinct cache key prefix ("uRepoGuid_{TypeName}_") so GUID-keyed entries never interfere with the int-keyed repository's prefix search and count validation.

With the separate prefix, count validation now passes correctly and data is served from cache (only a single SELECT COUNT(*) validation query is executed).

The Template repository fix used a different approach, which was simply to remove the GUID read repository. The TemplateByGuidReadRepository was introduced in #21280 to mirror the GUID caching pattern used by Document and Media repositories. However, unlike those repositories which use DefaultRepositoryCachePolicy (caching individual entities bykey), TemplateRepository uses FullDataSetRepositoryCachePolicy which already caches all templates as a single collection. So this update was unnecessary as the GUID lookup methods could simply filter the already-cached full dataset via GetMany(). Removing it both fixes the cache collision bug and simplifies the code to match what the architecture already provides.

The missing invalidations of the GUID caches where added.

Refactoring

This PR also includes a minor refactor in moving all the TimeSpan.FromMinutes(5) into a constant where the time can be managed in one place.

Testing

New integration tests have been added to verify:

• The exception is no longer thrown.
• Items are returned from the cache when we expect them to do so.

For manual testing the following surface controller can be used. Access via /umbraco/surface/DataTypeCacheBug/Test:

using Microsoft.AspNetCore.Mvc;
using Umbraco.Cms.Core.Cache;
using Umbraco.Cms.Core.Logging;
using Umbraco.Cms.Core.Routing;
using Umbraco.Cms.Core.Services;
using Umbraco.Cms.Core.Web;
using Umbraco.Cms.Infrastructure.Persistence;
using Umbraco.Cms.Web.Website.Controllers;

namespace Umbraco.Cms.Web.UI.Controllers;

public class DataTypeCacheBugController : SurfaceController
{
    private readonly IDataTypeService _dataTypeService;

    public DataTypeCacheBugController(
        IUmbracoContextAccessor umbracoContextAccessor,
        IUmbracoDatabaseFactory databaseFactory,
        ServiceContext services,
        AppCaches appCaches,
        IProfilingLogger profilingLogger,
        IPublishedUrlProvider publishedUrlProvider,
        IDataTypeService dataTypeService)
        : base(umbracoContextAccessor, databaseFactory, services, appCaches, profilingLogger, publishedUrlProvider)
    {
        _dataTypeService = dataTypeService;
    }

    [HttpGet]
    public async Task<IActionResult> Test()
    {
        try
        {
            var first = await _dataTypeService.GetAllAsync();
            var firstCount = first.Count();

            var second = await _dataTypeService.GetAllAsync();
            var secondCount = second.Count();

            return Content(
                $"First call: {firstCount} data types\n" +
                $"Second call: {secondCount} data types\n\n" +
                "SUCCESS: Both calls completed without error. The fix is working.",
                "text/plain");
        }
        catch (InvalidOperationException ex)
        {
            return Content(
                $"FAILURE: InvalidOperationException on second call.\n" +
                $"Message: {ex.Message}\n" +
                $"Stack: {ex.StackTrace}\n\n" +
                "This indicates the bug is still present — the GUID read repository's\n" +
                "cache policy is calling GetBaseQuery(isCount: true) which is not implemented.",
                "text/plain");
        }
    }
}

Before the fix, this will show the exception being thrown. After the fix, it displays the expected result.

[Copilot]

Pull request overview

Fixes a warm-cache failure where GUID-keyed read repositories and their parent int-keyed repositories shared the same isolated cache key prefix, causing GetAll/GetMany cache count validation to misbehave and (for GUID repos) throw InvalidOperationException.

Changes:

• Added a GUID-specific repository cache key prefix (uRepoGuid_{TypeName}_) via RepositoryCacheKeys.GetGuidKey<T>().
• Introduced GuidReadRepositoryCachePolicy<TEntity> and applied it to DataType/Document/Media GUID read repositories to prevent cache key collisions.
• Updated Template GUID read repository to use NoCacheRepositoryCachePolicy and added integration tests covering the warm-cache GUID GetMany() scenario and DataType GetAll caching behavior.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
src/Umbraco.Core/Persistence/Repositories/RepositoryCacheKeys.cs	Adds GUID-specific cache key prefix generation to avoid cross-repo prefix collisions.
src/Umbraco.Infrastructure/Cache/GuidReadRepositoryCachePolicy.cs	New cache policy for GUID-keyed read repositories using a distinct prefix.
src/Umbraco.Infrastructure/Persistence/Repositories/Implement/DataTypeRepository.cs	GUID inner repo now uses GuidReadRepositoryCachePolicy + GUID-prefixed cache keys.
src/Umbraco.Infrastructure/Persistence/Repositories/Implement/DocumentRepository.cs	Same as above for document GUID inner repo.
src/Umbraco.Infrastructure/Persistence/Repositories/Implement/MediaRepository.cs	Same as above for media GUID inner repo.
src/Umbraco.Infrastructure/Persistence/Repositories/Implement/TemplateRepository.cs	Removes GUID-key cache population/clearing and uses no-cache policy for GUID inner repo to avoid stale GUID entries.
tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/Repositories/DataTypeRepositoryTest.cs	Adds warm-cache regression tests for GUID GetMany() and cached GetMany()/GetAll behavior.
tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/Repositories/DocumentRepositoryTest.cs	Adds warm-cache regression test for GUID GetMany().
tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/Repositories/MediaRepositoryTest.cs	Adds warm-cache regression test for GUID GetMany().
tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/Repositories/TemplateRepositoryTest.cs	Adds warm-cache regression test for GUID GetMany().
tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/DataTypeServiceTests.cs	Updates tests to use async create APIs for templates/content types and adjusts obsolete API warning suppression.

[nikolajlauridsen]

Looks good, only have one minor concern 😄