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.

Sign up for GitHub

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 routing issues #17572

Merged
merged 4 commits into from
Nov 26, 2024
Merged

Fixes routing issues #17572

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5ba2103
Child of second root should also hide root path and backoffice needs …
bergmania Nov 18, 2024
ba7fb5c
Fixes routing issues based on findings https://github.com/umbraco/Umb…
bergmania Nov 20, 2024
7a98d44
Revert "Fixes routing issues based on findings https://github.com/umb…
bergmania Nov 20, 2024
a86e8b0
Fix urls of descendants of non-first roots do not show the root urlse…
bergmania Nov 20, 2024
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
191 changes: 137 additions & 54 deletions src/Umbraco.Core/Services/DocumentUrlService.cs
View file
Open in desktop
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 (v15/dev)

ℹ Getting worse: Overall Code Complexity 

The mean cyclomatic complexity increases from 4.47 to 4.56, 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.

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 (v15/dev)

ℹ Getting worse: Primitive Obsession 

The ratio of primitive types in function arguments increases from 55.07% to 56.76%, threshold = 30.0%. The functions in this file have too many primitive types (e.g. int, double, float) in their function argument lists. Using many primitive types lead to the code smell Primitive Obsession. Avoid adding more primitive arguments.
using System.Globalization;
using System.Runtime.CompilerServices;
using Microsoft.Extensions.Logging;
Expand Down Expand Up @@ -393,7 +393,19 @@
return GetTopMostRootKey(isDraft, culture);
}

// Otherwise we have to find the root items (or child of the first root when hideTopLevelNodeFromPath is true) and follow the url segments in them to get to correct document key
// Special case for all top level nodes except the first (that will have /)
if (runnerKey is null && urlSegments.Length == 1 && hideTopLevelNodeFromPath is true)
{
IEnumerable<Guid> rootKeys = GetKeysInRoot(false, isDraft, culture);
Guid? rootKeyWithUrlSegment = GetChildWithUrlSegment(rootKeys, urlSegments.First(), culture, isDraft);

if (rootKeyWithUrlSegment is not null)
{
return rootKeyWithUrlSegment;
}
}

// Otherwise we have to find the root items (or child of the roots when hideTopLevelNodeFromPath is true) and follow the url segments in them to get to correct document key

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

View check run for this annotation

CodeScene Delta Analysis / CodeScene Cloud Delta Analysis (v15/dev)

❌ Getting worse: Complex Method 

GetDocumentKeyByRoute increases in cyclomatic complexity from 19 to 23, 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.

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

View check run for this annotation

CodeScene Delta Analysis / CodeScene Cloud Delta Analysis (v15/dev)

❌ Getting worse: Complex Conditional 

GetDocumentKeyByRoute increases from 1 complex conditionals with 2 branches to 2 complex conditionals with 4 branches, threshold = 2. A complex conditional is an expression inside a branch (e.g. if, for, while) which consists of multiple, logical operators such as AND/OR. The more logical operators in an expression, the more severe the code smell.

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

View check run for this annotation

CodeScene Delta Analysis / CodeScene Cloud Delta Analysis (v15/dev)

❌ Getting worse: Bumpy Road Ahead 

GetDocumentKeyByRoute increases from 4 to 5 logical blocks with deeply nested code, threshold is one single block per function. The Bumpy Road code smell is a function that contains multiple chunks of nested conditional logic. The deeper the nesting and the more bumps, the lower the code health.
for (var index = 0; index < urlSegments.Length; index++)
{
var urlSegment = urlSegments[index];
Expand Down Expand Up @@ -447,32 +459,30 @@
var cultureOrDefault = string.IsNullOrWhiteSpace(culture) is false ? culture : _languageService.GetDefaultIsoCodeAsync().GetAwaiter().GetResult();

Guid[] ancestorsOrSelfKeysArray = ancestorsOrSelfKeys as Guid[] ?? ancestorsOrSelfKeys.ToArray();
IDictionary<Guid, Domain?> ancestorOrSelfKeyToDomains = ancestorsOrSelfKeysArray.ToDictionary(x => x, ancestorKey =>
ILookup<Guid, Domain?> ancestorOrSelfKeyToDomains = ancestorsOrSelfKeysArray.ToLookup(x => x, ancestorKey =>
{
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>();

Domain? foundDomain = null;

foreach (Guid ancestorOrSelfKey in ancestorsOrSelfKeysArray)
{
if (ancestorOrSelfKeyToDomains.TryGetValue(ancestorOrSelfKey, out Domain? domain))
var domains = ancestorOrSelfKeyToDomains[ancestorOrSelfKey].WhereNotNull();
if (domains.Any())
{
if (domain is not null)
{
foundDomain = domain;
break;
}
foundDomain = domains.First();// What todo here that is better?
break;

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

View check run for this annotation

CodeScene Delta Analysis / CodeScene Cloud Delta Analysis (v15/dev)

✅ Getting better: Complex Method 

GetLegacyRouteFormat decreases in cyclomatic complexity from 17 to 16, 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.
}

if (_cache.TryGetValue(CreateCacheKey(ancestorOrSelfKey, cultureOrDefault, isDraft), out PublishedDocumentUrlSegment? publishedDocumentUrlSegment))
Expand Down Expand Up @@ -528,79 +538,128 @@
var cultures = languages.ToDictionary(x=>x.IsoCode);

Guid[] ancestorsOrSelfKeysArray = ancestorsOrSelfKeys as Guid[] ?? ancestorsOrSelfKeys.ToArray();
Dictionary<Guid, Task<Dictionary<string, Domain>>> ancestorOrSelfKeyToDomains = ancestorsOrSelfKeysArray.ToDictionary(x => x, async ancestorKey =>
Dictionary<Guid, Task<ILookup<string, Domain>>> ancestorOrSelfKeyToDomains = ancestorsOrSelfKeysArray.ToDictionary(x => x, async ancestorKey =>
{
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!);
return domains.ToLookup(x => x.Culture!);
})!;

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

var hasUrlInCulture = true;
foreach (Guid ancestorOrSelfKey in ancestorsOrSelfKeysArray)
{
if (ancestorOrSelfKeyToDomains.TryGetValue(ancestorOrSelfKey, out Task<Dictionary<string, Domain>>? domainDictionaryTask))
var hasUrlInCulture = true;
foreach (Guid ancestorOrSelfKey in ancestorsOrSelfKeysArray)
{
var domainLookup = await ancestorOrSelfKeyToDomains[ancestorOrSelfKey];
if (domainLookup.Any())
{
Dictionary<string, Domain> domainDictionary = await domainDictionaryTask;
if (domainDictionary.TryGetValue(culture, out Domain? domain))
var domains = domainLookup[culture];
foreach (Domain domain in domains)
{
Attempt<Guid> domainKeyAttempt = _idKeyMap.GetKeyForId(domain.ContentId, UmbracoObjectTypes.Document);
Attempt<Guid> domainKeyAttempt =
_idKeyMap.GetKeyForId(domain.ContentId, UmbracoObjectTypes.Document);
if (domainKeyAttempt.Success)
{
if (_publishStatusQueryService.IsDocumentPublished(domainKeyAttempt.Result, culture))
{
foundDomain = domain;
break;
foundDomains.Add(domain);
}
}
}

if (foundDomains.Any())
{
break;
}
}

if (_cache.TryGetValue(CreateCacheKey(ancestorOrSelfKey, culture, false), out PublishedDocumentUrlSegment? publishedDocumentUrlSegment))
if (_cache.TryGetValue(
CreateCacheKey(ancestorOrSelfKey, culture, false),
out PublishedDocumentUrlSegment? publishedDocumentUrlSegment))
{
urlSegments.Add(publishedDocumentUrlSegment.UrlSegment);
}
else
{
hasUrlInCulture = false;
}
}
}

//If we did not find a domain and this is not the default language, then the content is not routable
if (foundDomains.Any() is false && language.IsDefault is false)
{
continue;
}

//If we did not find a domain and this is not the default language, then the content is not routable
if (foundDomain is null && language.IsDefault is false)
{
continue;
}

var isRootFirstItem = GetTopMostRootKey(false, culture) == ancestorsOrSelfKeysArray.Last();
var isRootFirstItem = GetTopMostRootKey(false, culture) == ancestorsOrSelfKeysArray.Last();

var leftToRight = _globalSettings.ForceCombineUrlPathLeftToRight
|| CultureInfo.GetCultureInfo(culture).TextInfo.IsRightToLeft is false;
if (leftToRight)
{
urlSegments.Reverse();
}

// If no domain was found, we need to add a null domain to the list to make sure we check for no domains.
if (foundDomains.Any() is false)
{
foundDomains.Add(null);
}

foreach (Domain? foundDomain in foundDomains)
{
var foundUrl = GetFullUrl(isRootFirstItem, urlSegments, foundDomain, leftToRight);

if (foundDomain is not null)
{
// if we found a domain, it should be safe to show url
result.Add(new UrlInfo(
text: foundUrl,
isUrl: hasUrlInCulture,
culture: culture
));
}
else
{
// otherwise we need to ensure that no other page has the same url
// e.g. a site with two roots that both have a child with the same name
Guid? documentKeyByRoute = GetDocumentKeyByRoute(foundUrl, culture, foundDomain?.ContentId, false);
if (contentKey.Equals(documentKeyByRoute))
{
result.Add(new UrlInfo(
text: foundUrl,
isUrl: hasUrlInCulture,
culture: culture
));
}
else
{
result.Add(new UrlInfo(
text: "Conflict: Other page has the same url",
isUrl: false,
culture: culture
));
}
}

var leftToRight = _globalSettings.ForceCombineUrlPathLeftToRight
|| CultureInfo.GetCultureInfo(culture).TextInfo.IsRightToLeft is false;
if (leftToRight)
{
urlSegments.Reverse();
}

result.Add(new UrlInfo(
text: GetFullUrl(isRootFirstItem, urlSegments, foundDomain, leftToRight),
isUrl: hasUrlInCulture,
culture: culture
));

}

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

View check run for this annotation

CodeScene Delta Analysis / CodeScene Cloud Delta Analysis (v15/dev)

❌ Getting worse: Complex Method 

ListUrlsAsync increases in cyclomatic complexity from 16 to 21, 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.

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

View check run for this annotation

CodeScene Delta Analysis / CodeScene Cloud Delta Analysis (v15/dev)

❌ New issue: Bumpy Road Ahead 

ListUrlsAsync has 3 blocks with nested conditional logic. Any nesting of 2 or deeper is considered. Threshold is one single, nested block per function. The Bumpy Road code smell is a function that contains multiple chunks of nested conditional logic. The deeper the nesting and the more bumps, the lower the code health.
}

return result;
}


private string GetFullUrl(bool isRootFirstItem, List<string> segments, Domain? foundDomain, bool leftToRight)
{
var urlSegments = new List<string>(segments);
Expand All @@ -610,7 +669,7 @@
return foundDomain.Name.EnsureEndsWith("/") + string.Join('/', urlSegments);
}

var hideTopLevel = _globalSettings.HideTopLevelNodeFromPath && isRootFirstItem;
var hideTopLevel = HideTopLevel(_globalSettings.HideTopLevelNodeFromPath, isRootFirstItem, urlSegments);
if (leftToRight)
{
return '/' + string.Join('/', urlSegments.Skip(hideTopLevel ? 1 : 0));
Expand All @@ -624,6 +683,21 @@
return '/' + string.Join('/', urlSegments);
}

private bool HideTopLevel(bool hideTopLevelNodeFromPath, bool isRootFirstItem, List<string> urlSegments)
{
if (hideTopLevelNodeFromPath is false)
{
return false;
}

if(isRootFirstItem is false && urlSegments.Count == 1)
{
return false;
}

return true;
}

public async Task CreateOrUpdateUrlSegmentsWithDescendantsAsync(Guid key)
{
var id = _idKeyMap.GetIdForKey(key, UmbracoObjectTypes.Document).Result;
Expand All @@ -646,37 +720,41 @@
}
}

private IEnumerable<Guid> GetKeysInRoot(bool addFirstLevelChildren, bool isDraft, string culture)
private IEnumerable<Guid> GetKeysInRoot(bool considerFirstLevelAsRoot, bool isDraft, string culture)
{
if (_documentNavigationQueryService.TryGetRootKeys(out IEnumerable<Guid> rootKeysEnumerable) is false)
{
yield break;
}

IEnumerable<Guid> rootKeys = rootKeysEnumerable as Guid[] ?? rootKeysEnumerable.ToArray();

foreach (Guid rootKey in rootKeys)
if (considerFirstLevelAsRoot)
{
yield return rootKey;
}
yield return rootKeys.First();

if (addFirstLevelChildren)
{
foreach (Guid rootKey in rootKeys)
{
if (isDraft is false && IsContentPublished(rootKey, culture) is false)
{
continue;
}

IEnumerable<Guid> childKeys = GetChildKeys(rootKey);

foreach (Guid childKey in childKeys)
{
yield return childKey;
}
}
}
else
{
foreach (Guid rootKey in rootKeys)
{
yield return rootKey;
}
}

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

View check run for this annotation

CodeScene Delta Analysis / CodeScene Cloud Delta Analysis (v15/dev)

❌ New issue: Bumpy Road Ahead 

GetKeysInRoot has 2 blocks with nested conditional logic. Any nesting of 2 or deeper is considered. Threshold is one single, nested block per function. The Bumpy Road code smell is a function that contains multiple chunks of nested conditional logic. The deeper the nesting and the more bumps, the lower the code health.

}

Expand Down Expand Up @@ -710,23 +788,28 @@
return Enumerable.Empty<Guid>();
}

/// <summary>
/// Gets the top most root key.
/// </summary>
/// <returns>The top most root key.</returns>
private Guid? GetTopMostRootKey(bool isDraft, string culture)
private IEnumerable<Guid> GetRootKeys(bool isDraft, string culture)
{
if (_documentNavigationQueryService.TryGetRootKeys(out IEnumerable<Guid> rootKeys))
{
foreach (Guid rootKey in rootKeys)
{
if (isDraft || IsContentPublished(rootKey, culture))
{
return rootKey;
yield return rootKey;
}
}
}
return null;
}


/// <summary>
/// Gets the top most root key.
/// </summary>
/// <returns>The top most root key.</returns>
private Guid? GetTopMostRootKey(bool isDraft, string culture)
{
return GetRootKeys(isDraft, culture).Cast<Guid?>().FirstOrDefault();
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
Expand Down
29 changes: 17 additions & 12 deletions ...es/documents/documents/workspace/views/info/document-workspace-view-info-links.element.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ export class UmbDocumentWorkspaceViewInfoLinksElement extends UmbLitElement {
private _variantOptions?: Array<UmbDocumentVariantOptionModel>;

@state()
private _lookup: Record<string, string> = {};
private _lookup: Record<string, string[]> = {};

constructor() {
super();
Expand Down Expand Up @@ -55,7 +55,10 @@ export class UmbDocumentWorkspaceViewInfoLinksElement extends UmbLitElement {
if (data?.length) {
data[0].urls.forEach((item) => {
if (item.culture && item.url) {
this._lookup[item.culture] = item.url;
if(this._lookup[item.culture] == null){
this._lookup[item.culture] = [];
}
this._lookup[item.culture].push(item.url);
}
});
this.requestUpdate('_lookup');
Expand Down Expand Up @@ -107,18 +110,20 @@ export class UmbDocumentWorkspaceViewInfoLinksElement extends UmbLitElement {
#renderUrl(variantOption: UmbDocumentVariantOptionModel) {
const varies = !!variantOption.culture;
const culture = varies ? variantOption.culture! : variantOption.language.unique;
const url = this._lookup[culture];
const urls = this._lookup[culture];
return when(
url,
urls && urls.length >= 1,
() => html`
<a class="link-item" href=${url} target="_blank">
<span>
<span class="culture">${varies ? culture : nothing}</span>
<span>${url}</span>
</span>
<uui-icon name="icon-out"></uui-icon>
</a>
`,
${urls.map((url) =>
html`
<a class="link-item" href=${url} target="_blank">
<span>
<span class="culture">${varies ? culture : nothing}</span>
<span>${url}</span>
</span>
<uui-icon name="icon-out"></uui-icon>
</a>`
)}`,
() => html`
<div class="link-item">
<span>
Expand Down
Loading
Loading