Performance: Use request cache for ContentSourceDto retrieval

[AndyButland]

Description

It was noted that GetContentSourceAsync is called more than once during save and publish operations, and that we go to the database both for published and draft content even though the data returned is the same. This PR adds a request cache around this, ensuring we only request the underlying DTO once, and then create the ContentCacheNode as appropriate from that.

Not super-happy with the implementation here - it seemed I either needed to pass a parameter indicating whether to use the cache, or have the repository expose the ContentSourceDto so the service can decide whether to re-get it or not. Neither felt very nice, but I've gone with the former. Maybe reviewers have a better approach here.

Change Summary

• Adds request-level caching for ContentSourceDto lookups in DatabaseCacheRepository to avoid redundant database queries within the same HTTP request
• Refactors GetContentSourceAsync by extracting GetContentSourceDto (with caching) and CreateContentCacheNode methods
• Updates XML documentation to clarify the preview parameter

Testing

Via a breakpoint you can verify that during a save and publish operation, only the first request for a content cache node hits the database.

[Copilot]

Pull request overview

This PR adds request-level caching to the DatabaseCacheRepository to optimize redundant database queries for ContentSourceDto lookups within a single HTTP request. The implementation caches the DTO object (which contains both draft and published data) and reuses it for multiple calls with different preview parameters, since the preview flag only determines which data fields to use from the DTO.

Key changes:

• Extracts database lookup logic into a new private GetContentSourceDto method with request-level caching
• Extracts ContentCacheNode creation logic into a new private CreateContentCacheNode method for better separation of concerns
• Updates XML documentation to clarify that the preview parameter affects the returned data

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
src/Umbraco.PublishedCache.HybridCache/Persistence/IDatabaseCacheRepository.cs	Updates XML documentation to clarify that GetContentSourceAsync considers both document key and preview status
src/Umbraco.PublishedCache.HybridCache/Persistence/DatabaseCacheRepository.cs	Adds request-level caching for ContentSourceDto retrieval and refactors GetContentSourceAsync into three focused methods

[Zeegaan]

Just looking at this, it seems there is only one place where we call the GetContentSourceAsync, with useCache true.
When that is the case, I really think it's overkill to have the parameter in GetContentSourceAsync itself.
I'd rather have the DocumentCacheService handle the request cache too, this also makes sense in an architectural sense, as that is already what we're doing in this service, by deciding whether or not to fetch from the Memory Cache / Second layer / repository (this is inherently done by HybridCache, but still). Adding a request cache to the service in theory "just" adds ´1 to the already numerous caches we're handling in the service 🤣
So in my head, we could wrap DocumentCacheService line 189
ContentCacheNode? publishedNode = await _databaseCacheRepository.GetContentSourceAsync(key, false, true);
In the request cache instead 🤔
Hope my ramblings here makes sense 🤣

[AndyButland]

They do to an extent - however I don't think it's quite as simple as that, as the problem is we have the repository responsible for populating ContentCacheNode from the ContentSourceDto. The creation of this depends on the preview parameter, but the data retrieval doesn't. I.e. we want to cache ContentSourceDto, not ContentCacheNode.

So it seemed to me the only way to move the cache to the service - which I agree is the best place - is to have the repository return ContentSourceDto instead of ContentCacheNode. But that doesn't feel right either (we should return "entities" from the repositories, not the persistence DTOs).

That was the crux of my point here:

Not super-happy with the implementation here - it seemed I either needed to pass a parameter indicating whether to use the cache, or have the repository expose the ContentSourceDto so the service can decide whether to re-get it or not. Neither felt very nice, but I've gone with the former. Maybe reviewers have a better approach here.

If you can see a way to unpick that problem in a clean way, I'd appreciate the attempt or suggestion.

[Zeegaan]

Fair point, I don't have any other good ideas, but as you say it just doesn't feel right 🙈
I guess this will be changed, whenever we can store the entire IPublishedContent in the cache, when we remove the Children & Parent properties, so it will work as a temporary solution for now 😁

[AndyButland]

I haven't merged this as it was still nagging me that there should be a cleaner approach, which I think I've found in #21407.

So will close this and propose the other in preference.

[Zeegaan]

Agreed, much cleaner 😁