Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes slow legacy routes by using a domain cache #17371

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
using System.Diagnostics;
using Asp.Versioning;
using Microsoft.AspNetCore.Http;
using Microsoft.AspNetCore.Mvc;
Expand Down Expand Up @@ -46,19 +47,17 @@ public async Task<IActionResult> ByIdV20(Guid id)

private async Task<IActionResult> HandleRequest(Guid id)
{
IPublishedContent? contentItem = ApiPublishedContentCache.GetById(id);
IPublishedContent? contentItem = await ApiPublishedContentCache.GetByIdAsync(id).ConfigureAwait(false);

if (contentItem is null)
{
return NotFound();
}

IActionResult? deniedAccessResult = await HandleMemberAccessAsync(contentItem, _requestMemberAccessService);
IActionResult? deniedAccessResult = await HandleMemberAccessAsync(contentItem, _requestMemberAccessService).ConfigureAwait(false);
if (deniedAccessResult is not null)
{
return deniedAccessResult;
}

IApiContentResponse? apiContentResponse = ApiContentResponseBuilder.Build(contentItem);
if (apiContentResponse is null)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ public async Task<IActionResult> ItemsV20([FromQuery(Name = "id")] HashSet<Guid>

private async Task<IActionResult> HandleRequest(HashSet<Guid> ids)
{
IPublishedContent[] contentItems = ApiPublishedContentCache.GetByIds(ids).ToArray();
IPublishedContent[] contentItems = (await ApiPublishedContentCache.GetByIdsAsync(ids).ConfigureAwait(false)).ToArray();

IActionResult? deniedAccessResult = await HandleMemberAccessAsync(contentItems, _requestMemberAccessService);
if (deniedAccessResult is not null)
Expand Down
52 changes: 51 additions & 1 deletion src/Umbraco.Core/DeliveryApi/ApiPublishedContentCache.cs
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,35 @@
deliveryApiSettings.OnChange(settings => _deliveryApiSettings = settings);
}

public async Task<IPublishedContent?> GetByRouteAsync(string route)
{
var isPreviewMode = _requestPreviewService.IsPreview();

// Handle the nasty logic with domain document ids in front of paths.
int? documentStartNodeId = null;
if (route.StartsWith("/") is false)
{
var index = route.IndexOf('/');

if (index > -1 && int.TryParse(route.Substring(0, index), out var nodeId))
{
documentStartNodeId = nodeId;
route = route.Substring(index);
}
}

Guid? documentKey = _documentUrlService.GetDocumentKeyByRoute(
route,
_requestCultureService.GetRequestedCulture(),
documentStartNodeId,
_requestPreviewService.IsPreview()
);
IPublishedContent? content = documentKey.HasValue
? await _publishedContentCache.GetByIdAsync(documentKey.Value, isPreviewMode)
: null;

return ContentOrNullIfDisallowed(content);
}

Check warning on line 63 in src/Umbraco.Core/DeliveryApi/ApiPublishedContentCache.cs

View check run for this annotation

CodeScene Delta Analysis / CodeScene Cloud Delta Analysis (release/15.0)

❌ New issue: Code Duplication

The module contains 2 functions with similar structure: GetByRoute,GetByRouteAsync. Avoid duplicated, aka copy-pasted, code inside the module. More duplication lowers the code health.

public IPublishedContent? GetByRoute(string route)
{
Expand Down Expand Up @@ -64,16 +93,37 @@
return ContentOrNullIfDisallowed(content);
}

public async Task<IPublishedContent?> GetByIdAsync(Guid contentId)
{
IPublishedContent? content = await _publishedContentCache.GetByIdAsync(contentId, _requestPreviewService.IsPreview()).ConfigureAwait(false);
return ContentOrNullIfDisallowed(content);
}

public IPublishedContent? GetById(Guid contentId)
{
IPublishedContent? content = _publishedContentCache.GetById(_requestPreviewService.IsPreview(), contentId);
return ContentOrNullIfDisallowed(content);
}
public async Task<IEnumerable<IPublishedContent>> GetByIdsAsync(IEnumerable<Guid> contentIds)
{
var isPreviewMode = _requestPreviewService.IsPreview();

IEnumerable<Task<IPublishedContent?>> tasks = contentIds
.Select(contentId => _publishedContentCache.GetByIdAsync(contentId, isPreviewMode));

IPublishedContent?[] allContent = await Task.WhenAll(tasks);

return allContent
.WhereNotNull()
.Where(IsAllowedContentType)
.ToArray();
}

public IEnumerable<IPublishedContent> GetByIds(IEnumerable<Guid> contentIds)
{
var isPreviewMode = _requestPreviewService.IsPreview();
return contentIds
.Select(contentId => _publishedContentCache.GetById(_requestPreviewService.IsPreview(), contentId))
.Select(contentId => _publishedContentCache.GetById(isPreviewMode, contentId))
.WhereNotNull()
.Where(IsAllowedContentType)
.ToArray();
Expand Down
4 changes: 4 additions & 0 deletions src/Umbraco.Core/DeliveryApi/IApiPublishedContentCache.cs
Original file line number Diff line number Diff line change
Expand Up @@ -9,4 +9,8 @@ public interface IApiPublishedContentCache
IPublishedContent? GetById(Guid contentId);

IEnumerable<IPublishedContent> GetByIds(IEnumerable<Guid> contentIds);

Task<IPublishedContent?> GetByIdAsync(Guid contentId) => Task.FromResult(GetById(contentId));
Task<IPublishedContent?> GetByRouteAsync(string route) => Task.FromResult(GetByRoute(route));
Task<IEnumerable<IPublishedContent>> GetByIdsAsync(IEnumerable<Guid> contentIds) => Task.FromResult(GetByIds(contentIds));
}
54 changes: 34 additions & 20 deletions src/Umbraco.Core/Services/DocumentUrlService.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
using System.Collections.Concurrent;

Check notice on line 1 in src/Umbraco.Core/Services/DocumentUrlService.cs

View check run for this annotation

CodeScene Delta Analysis / CodeScene Cloud Delta Analysis (release/15.0)

ℹ Getting worse: Overall Code Complexity

The mean cyclomatic complexity increases from 4.03 to 4.10, threshold = 4. This file has many conditional statements (e.g. if, for, while) across its implementation, leading to lower code health. Avoid adding more conditionals.
using System.Globalization;
using System.Runtime.CompilerServices;
using Microsoft.Extensions.Logging;
Expand All @@ -6,6 +6,7 @@
using Umbraco.Cms.Core.Configuration.Models;
using Umbraco.Cms.Core.Models;
using Umbraco.Cms.Core.Persistence.Repositories;
using Umbraco.Cms.Core.PublishedCache;
using Umbraco.Cms.Core.Routing;
using Umbraco.Cms.Core.Scoping;
using Umbraco.Cms.Core.Services.Navigation;
Expand All @@ -30,8 +31,8 @@
private readonly IKeyValueService _keyValueService;
private readonly IIdKeyMap _idKeyMap;
private readonly IDocumentNavigationQueryService _documentNavigationQueryService;
private readonly IDomainService _domainService;
private readonly IPublishStatusQueryService _publishStatusQueryService;
private readonly IDomainCacheService _domainCacheService;

private readonly ConcurrentDictionary<string, PublishedDocumentUrlSegment> _cache = new();
private bool _isInitialized;
Expand All @@ -49,8 +50,8 @@
IKeyValueService keyValueService,
IIdKeyMap idKeyMap,
IDocumentNavigationQueryService documentNavigationQueryService,
IDomainService domainService,
IPublishStatusQueryService publishStatusQueryService)
IPublishStatusQueryService publishStatusQueryService,
IDomainCacheService domainCacheService)
{
_logger = logger;
_documentUrlRepository = documentUrlRepository;
Expand All @@ -64,8 +65,8 @@
_keyValueService = keyValueService;
_idKeyMap = idKeyMap;
_documentNavigationQueryService = documentNavigationQueryService;
_domainService = domainService;
_publishStatusQueryService = publishStatusQueryService;
_domainCacheService = domainCacheService;
}

public async Task InitAsync(bool forceEmpty, CancellationToken cancellationToken)
Expand Down Expand Up @@ -443,42 +444,49 @@
var cultureOrDefault = string.IsNullOrWhiteSpace(culture) is false ? culture : _languageService.GetDefaultIsoCodeAsync().GetAwaiter().GetResult();

Guid[] ancestorsOrSelfKeysArray = ancestorsOrSelfKeys as Guid[] ?? ancestorsOrSelfKeys.ToArray();
IDictionary<Guid, IDomain?> ancestorOrSelfKeyToDomains = ancestorsOrSelfKeysArray.ToDictionary(x => x, ancestorKey =>
IDictionary<Guid, Domain?> ancestorOrSelfKeyToDomains = ancestorsOrSelfKeysArray.ToDictionary(x => x, ancestorKey =>
{
IEnumerable<IDomain> domains = _domainService.GetAssignedDomainsAsync(ancestorKey, false).GetAwaiter().GetResult();
return domains.FirstOrDefault(x=>x.LanguageIsoCode == cultureOrDefault);
Attempt<int> idAttempt = _idKeyMap.GetIdForKey(ancestorKey, UmbracoObjectTypes.Document);

if(idAttempt.Success is false)
{
return null;
}

IEnumerable<Domain> domains = _domainCacheService.GetAssigned(idAttempt.Result, false);
return domains.FirstOrDefault(x=>x.Culture == cultureOrDefault);
});

var urlSegments = new List<string>();

IDomain? foundDomain = null;
Domain? foundDomain = null;

foreach (Guid ancestorOrSelfKey in ancestorsOrSelfKeysArray)
{
if (ancestorOrSelfKeyToDomains.TryGetValue(ancestorOrSelfKey, out IDomain? domain))
if (ancestorOrSelfKeyToDomains.TryGetValue(ancestorOrSelfKey, out Domain? domain))
{
if (domain is not null)
{
foundDomain = domain;
break;
}
}

if (_cache.TryGetValue(CreateCacheKey(ancestorOrSelfKey, cultureOrDefault, isDraft), out PublishedDocumentUrlSegment? publishedDocumentUrlSegment))
{
urlSegments.Add(publishedDocumentUrlSegment.UrlSegment);
}

if (foundDomain is not null)
{
break;
}
}

if (foundDomain is not null)
{
//we found a domain, and not to construct the route in the funny legacy way
return foundDomain.RootContentId + "/" + string.Join("/", urlSegments);
return foundDomain.ContentId + "/" + string.Join("/", urlSegments);

Check warning on line 489 in src/Umbraco.Core/Services/DocumentUrlService.cs

View check run for this annotation

CodeScene Delta Analysis / CodeScene Cloud Delta Analysis (release/15.0)

❌ Getting worse: Complex Method

GetLegacyRouteFormat increases in cyclomatic complexity from 11 to 12, threshold = 9. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.
}

var isRootFirstItem = GetTopMostRootKey(isDraft, cultureOrDefault) == ancestorsOrSelfKeysArray.Last();
Expand Down Expand Up @@ -510,24 +518,30 @@
var cultures = languages.ToDictionary(x=>x.IsoCode);

Guid[] ancestorsOrSelfKeysArray = ancestorsOrSelfKeys as Guid[] ?? ancestorsOrSelfKeys.ToArray();
Dictionary<Guid, Task<Dictionary<string, IDomain>>> ancestorOrSelfKeyToDomains = ancestorsOrSelfKeysArray.ToDictionary(x => x, async ancestorKey =>
Dictionary<Guid, Task<Dictionary<string, Domain>>> ancestorOrSelfKeyToDomains = ancestorsOrSelfKeysArray.ToDictionary(x => x, async ancestorKey =>
{
IEnumerable<IDomain> domains = await _domainService.GetAssignedDomainsAsync(ancestorKey, false);
return domains.ToDictionary(x => x.LanguageIsoCode!);
});
Attempt<int> idAttempt = _idKeyMap.GetIdForKey(ancestorKey, UmbracoObjectTypes.Document);

if(idAttempt.Success is false)
{
return null;
}
IEnumerable<Domain> domains = _domainCacheService.GetAssigned(idAttempt.Result, false);
return domains.ToDictionary(x => x.Culture!);
})!;

foreach ((string culture, ILanguage language) in cultures)
{
var urlSegments = new List<string>();
IDomain? foundDomain = null;
Domain? foundDomain = null;

var hasUrlInCulture = true;
foreach (Guid ancestorOrSelfKey in ancestorsOrSelfKeysArray)
{
if (ancestorOrSelfKeyToDomains.TryGetValue(ancestorOrSelfKey, out Task<Dictionary<string, IDomain>>? domainDictionaryTask))
if (ancestorOrSelfKeyToDomains.TryGetValue(ancestorOrSelfKey, out Task<Dictionary<string, Domain>>? domainDictionaryTask))
{
Dictionary<string, IDomain> domainDictionary = await domainDictionaryTask;
if (domainDictionary.TryGetValue(culture, out IDomain? domain))
Dictionary<string, Domain> domainDictionary = await domainDictionaryTask;
if (domainDictionary.TryGetValue(culture, out Domain? domain))

Check warning on line 544 in src/Umbraco.Core/Services/DocumentUrlService.cs

View check run for this annotation

CodeScene Delta Analysis / CodeScene Cloud Delta Analysis (release/15.0)

❌ Getting worse: Complex Method

ListUrlsAsync increases in cyclomatic complexity from 11 to 12, threshold = 9. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.
{
foundDomain = domain;
break;
Expand Down Expand Up @@ -562,14 +576,14 @@
return result;
}

private string GetFullUrl(bool isRootFirstItem, List<string> reversedUrlSegments, IDomain? foundDomain)
private string GetFullUrl(bool isRootFirstItem, List<string> reversedUrlSegments, Domain? foundDomain)
{
var urlSegments = new List<string>(reversedUrlSegments);
urlSegments.Reverse();

if (foundDomain is not null)
{
return foundDomain.DomainName.EnsureEndsWith("/") + string.Join('/', urlSegments);
return foundDomain.Name.EnsureEndsWith("/") + string.Join('/', urlSegments);
}

return '/' + string.Join('/', urlSegments.Skip(_globalSettings.HideTopLevelNodeFromPath && isRootFirstItem ? 1 : 0));
Expand Down
Loading