Skip to content

Commit 947afdb

Browse files
kjacAndyButland
kjac
and
AndyButland
authored
Amend root content routing and ensure trailing slashes as configured (#18958)
* Amend root content routing and ensure trailing slashes as configured * Fix false positives at root + add more tests * Awaited async method and resolved warning around readonly. --------- Co-authored-by: Andy Butland <[email protected]>
1 parent 134c800 commit 947afdb

16 files changed

+1341
-40
lines changed

‎src/Umbraco.Core/DeliveryApi/ApiContentRouteBuilder.cs

Lines changed: 34 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
using Umbraco.Cms.Core.Models.DeliveryApi;
66
using Umbraco.Cms.Core.Models.PublishedContent;
77
using Umbraco.Cms.Core.PublishedCache;
8+
using Umbraco.Cms.Core.Services;
89
using Umbraco.Cms.Core.Services.Navigation;
910
using Umbraco.Extensions;
1011

@@ -19,6 +20,7 @@ public sealed class ApiContentRouteBuilder : IApiContentRouteBuilder
1920
private readonly IPublishedContentCache _contentCache;
2021
private readonly IDocumentNavigationQueryService _navigationQueryService;
2122
private readonly IPublishStatusQueryService _publishStatusQueryService;
23+
private readonly IDocumentUrlService _documentUrlService;
2224
private RequestHandlerSettings _requestSettings;
2325

2426
public ApiContentRouteBuilder(
@@ -29,19 +31,44 @@ public ApiContentRouteBuilder(
2931
IOptionsMonitor<RequestHandlerSettings> requestSettings,
3032
IPublishedContentCache contentCache,
3133
IDocumentNavigationQueryService navigationQueryService,
32-
IPublishStatusQueryService publishStatusQueryService)
34+
IPublishStatusQueryService publishStatusQueryService,
35+
IDocumentUrlService documentUrlService)
3336
{
3437
_apiContentPathProvider = apiContentPathProvider;
3538
_variationContextAccessor = variationContextAccessor;
3639
_requestPreviewService = requestPreviewService;
3740
_contentCache = contentCache;
3841
_navigationQueryService = navigationQueryService;
3942
_publishStatusQueryService = publishStatusQueryService;
43+
_documentUrlService = documentUrlService;
4044
_globalSettings = globalSettings.Value;
4145
_requestSettings = requestSettings.CurrentValue;
4246
requestSettings.OnChange(settings => _requestSettings = settings);
4347
}
4448

49+
[Obsolete("Use the non-obsolete constructor, scheduled for removal in v17")]
50+
public ApiContentRouteBuilder(
51+
IApiContentPathProvider apiContentPathProvider,
52+
IOptions<GlobalSettings> globalSettings,
53+
IVariationContextAccessor variationContextAccessor,
54+
IRequestPreviewService requestPreviewService,
55+
IOptionsMonitor<RequestHandlerSettings> requestSettings,
56+
IPublishedContentCache contentCache,
57+
IDocumentNavigationQueryService navigationQueryService,
58+
IPublishStatusQueryService publishStatusQueryService)
59+
: this(
60+
apiContentPathProvider,
61+
globalSettings,
62+
variationContextAccessor,
63+
requestPreviewService,
64+
requestSettings,
65+
contentCache,
66+
navigationQueryService,
67+
publishStatusQueryService,
68+
StaticServiceProvider.Instance.GetRequiredService<IDocumentUrlService>())
69+
{
70+
}
71+
4572
[Obsolete("Use the non-obsolete constructor, scheduled for removal in v17")]
4673
public ApiContentRouteBuilder(
4774
IApiContentPathProvider apiContentPathProvider,
@@ -59,7 +86,8 @@ public ApiContentRouteBuilder(
5986
requestSettings,
6087
contentCache,
6188
navigationQueryService,
62-
StaticServiceProvider.Instance.GetRequiredService<IPublishStatusQueryService>())
89+
StaticServiceProvider.Instance.GetRequiredService<IPublishStatusQueryService>(),
90+
StaticServiceProvider.Instance.GetRequiredService<IDocumentUrlService>())
6391
{
6492
}
6593

@@ -113,7 +141,7 @@ public ApiContentRouteBuilder(
113141
// we can perform fallback to the content route.
114142
if (IsInvalidContentPath(contentPath))
115143
{
116-
contentPath = _contentCache.GetRouteById(content.Id, culture) ?? contentPath;
144+
contentPath = _documentUrlService.GetLegacyRouteFormat(content.Key, culture ?? _variationContextAccessor.VariationContext?.Culture, isPreview);
117145
}
118146

119147
// if the content path has still not been resolved as a valid path, the content is un-routable in this culture
@@ -125,7 +153,9 @@ public ApiContentRouteBuilder(
125153
: null;
126154
}
127155

128-
return contentPath;
156+
return _requestSettings.AddTrailingSlash
157+
? contentPath?.EnsureEndsWith('/')
158+
: contentPath?.TrimEnd('/');
129159
}
130160

131161
private string ContentPreviewPath(IPublishedContent content) => $"{Constants.DeliveryApi.Routing.PreviewContentPathPrefix}{content.Key:D}{(_requestSettings.AddTrailingSlash ? "/" : string.Empty)}";

‎src/Umbraco.Core/DeliveryApi/ApiPublishedContentCache.cs

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -86,10 +86,35 @@ public ApiPublishedContentCache(
8686
_variationContextAccessor.VariationContext?.Culture,
8787
_requestPreviewService.IsPreview());
8888

89+
// in multi-root settings, we've historically resolved all but the first root by their ID + URL segment,
90+
// e.g. "1234/second-root-url-segment". in V15+, IDocumentUrlService won't resolve this anymore; it will
91+
// however resolve "1234/" correctly, so to remain backwards compatible, we need to perform this extra step.
92+
var verifyUrlSegment = false;
93+
if (documentKey is null && route.TrimEnd('/').CountOccurrences("/") is 1)
94+
{
95+
documentKey = _apiDocumentUrlService.GetDocumentKeyByRoute(
96+
route[..(route.IndexOf('/') + 1)],
97+
_variationContextAccessor.VariationContext?.Culture,
98+
_requestPreviewService.IsPreview());
99+
verifyUrlSegment = true;
100+
}
101+
89102
IPublishedContent? content = documentKey.HasValue
90103
? _publishedContentCache.GetById(isPreviewMode, documentKey.Value)
91104
: null;
92105

106+
// the additional look-up above can result in false positives; if attempting to request a non-existing child to
107+
// the currently contextualized request root (either by start item or by domain), the root content key might
108+
// get resolved. to counter for this, we compare the requested URL segment with the resolved content URL segment.
109+
if (content is not null && verifyUrlSegment)
110+
{
111+
var expectedUrlSegment = route[(route.IndexOf('/') + 1)..];
112+
if (content.UrlSegment != expectedUrlSegment)
113+
{
114+
content = null;
115+
}
116+
}
117+
93118
return ContentOrNullIfDisallowed(content);
94119
}
95120

‎tests/Umbraco.Tests.Integration/Umbraco.Core/DeliveryApi/Request/ApiContentPathResolverInvariantTests.cs

Lines changed: 258 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,258 @@
1+
using Microsoft.AspNetCore.Http;
2+
using Microsoft.Extensions.DependencyInjection;
3+
using NUnit.Framework;
4+
using Umbraco.Cms.Core;
5+
using Umbraco.Cms.Core.Cache;
6+
using Umbraco.Cms.Core.Configuration.Models;
7+
using Umbraco.Cms.Core.Models;
8+
using Umbraco.Cms.Core.Services.Changes;
9+
using Umbraco.Cms.Tests.Common.Builders;
10+
using Umbraco.Cms.Tests.Common.Builders.Extensions;
11+
using Umbraco.Cms.Tests.Integration.Attributes;
12+
13+
namespace Umbraco.Cms.Tests.Integration.Umbraco.Core.DeliveryApi.Request;
14+
15+
public class ApiContentPathResolverInvariantTests : ApiContentPathResolverTestBase
16+
{
17+
private Dictionary<string, IContent> _contentByName = new ();
18+
19+
public static void ConfigureIncludeTopLevelNodeInPath(IUmbracoBuilder builder)
20+
=> builder.Services.Configure<GlobalSettings>(config => config.HideTopLevelNodeFromPath = false);
21+
22+
[SetUp]
23+
public async Task SetUpTest()
24+
{
25+
UmbracoContextFactory.EnsureUmbracoContext();
26+
SetRequestHost("localhost");
27+
28+
if (_contentByName.Any())
29+
{
30+
// these tests all run on the same DB to make them run faster, so we need to get the cache in a
31+
// predictable state with each test run.
32+
RefreshContentCache();
33+
return;
34+
}
35+
36+
await DocumentUrlService.InitAsync(true, CancellationToken.None);
37+
38+
var contentType = new ContentTypeBuilder()
39+
.WithAlias("theContentType")
40+
.Build();
41+
contentType.AllowedAsRoot = true;
42+
await ContentTypeService.CreateAsync(contentType, Constants.Security.SuperUserKey);
43+
contentType.AllowedContentTypes = [new() { Alias = contentType.Alias, Key = contentType.Key }];
44+
await ContentTypeService.UpdateAsync(contentType, Constants.Security.SuperUserKey);
45+
foreach (var rootNumber in Enumerable.Range(1, 3))
46+
{
47+
var root = new ContentBuilder()
48+
.WithContentType(contentType)
49+
.WithName($"Root {rootNumber}")
50+
.Build();
51+
ContentService.Save(root);
52+
ContentService.Publish(root, ["*"]);
53+
_contentByName[root.Name!] = root;
54+
55+
foreach (var childNumber in Enumerable.Range(1, 3))
56+
{
57+
var child = new ContentBuilder()
58+
.WithContentType(contentType)
59+
.WithParent(root)
60+
.WithName($"Child {childNumber}")
61+
.Build();
62+
ContentService.Save(child);
63+
ContentService.Publish(child, ["*"]);
64+
_contentByName[$"{root.Name!}/{child.Name!}"] = child;
65+
66+
foreach (var grandchildNumber in Enumerable.Range(1, 3))
67+
{
68+
var grandchild = new ContentBuilder()
69+
.WithContentType(contentType)
70+
.WithParent(child)
71+
.WithName($"Grandchild {grandchildNumber}")
72+
.Build();
73+
ContentService.Save(grandchild);
74+
ContentService.Publish(grandchild, ["*"]);
75+
_contentByName[$"{root.Name!}/{child.Name!}/{grandchild.Name!}"] = grandchild;
76+
}
77+
}
78+
}
79+
}
80+
81+
[Test]
82+
public void First_Root_Without_StartItem()
83+
{
84+
Assert.IsEmpty(GetRequiredService<IHttpContextAccessor>().HttpContext!.Request.Headers["Start-Item"].ToString());
85+
86+
var content = ApiContentPathResolver.ResolveContentPath("/");
87+
Assert.IsNotNull(content);
88+
Assert.AreEqual(_contentByName["Root 1"].Key, content.Key);
89+
}
90+
91+
[Test]
92+
[ConfigureBuilder(ActionName = nameof(ConfigureIncludeTopLevelNodeInPath))]
93+
public void First_Root_Without_StartItem_With_Top_Level_Node_Included()
94+
{
95+
Assert.IsEmpty(GetRequiredService<IHttpContextAccessor>().HttpContext!.Request.Headers["Start-Item"].ToString());
96+
97+
var content = ApiContentPathResolver.ResolveContentPath("/");
98+
Assert.IsNotNull(content);
99+
Assert.AreEqual(_contentByName["Root 1"].Key, content.Key);
100+
}
101+
102+
[TestCase(1)]
103+
[TestCase(2)]
104+
[TestCase(3)]
105+
public void First_Root_Child_Without_StartItem(int child)
106+
{
107+
Assert.IsEmpty(GetRequiredService<IHttpContextAccessor>().HttpContext!.Request.Headers["Start-Item"].ToString());
108+
109+
var content = ApiContentPathResolver.ResolveContentPath($"/child-{child}");
110+
Assert.IsNotNull(content);
111+
Assert.AreEqual(_contentByName[$"Root 1/Child {child}"].Key, content.Key);
112+
}
113+
114+
[TestCase(1, 1)]
115+
[TestCase(2, 2)]
116+
[TestCase(3, 3)]
117+
[TestCase(1, 2)]
118+
[TestCase(2, 3)]
119+
[TestCase(3, 1)]
120+
public void First_Root_Grandchild_Without_StartItem(int child, int grandchild)
121+
{
122+
Assert.IsEmpty(GetRequiredService<IHttpContextAccessor>().HttpContext!.Request.Headers["Start-Item"].ToString());
123+
124+
var content = ApiContentPathResolver.ResolveContentPath($"/child-{child}/grandchild-{grandchild}");
125+
Assert.IsNotNull(content);
126+
Assert.AreEqual(_contentByName[$"Root 1/Child {child}/Grandchild {grandchild}"].Key, content.Key);
127+
}
128+
129+
[TestCase(1)]
130+
[TestCase(2)]
131+
[TestCase(3)]
132+
public void Root_With_StartItem(int root)
133+
{
134+
SetRequestStartItem($"root-{root}");
135+
136+
var content = ApiContentPathResolver.ResolveContentPath("/");
137+
Assert.IsNotNull(content);
138+
Assert.AreEqual(_contentByName[$"Root {root}"].Key, content.Key);
139+
}
140+
141+
[TestCase(1)]
142+
[TestCase(2)]
143+
[TestCase(3)]
144+
[ConfigureBuilder(ActionName = nameof(ConfigureIncludeTopLevelNodeInPath))]
145+
public void Root_With_StartItem_With_Top_Level_Node_Included(int root)
146+
{
147+
SetRequestStartItem($"root-{root}");
148+
149+
var content = ApiContentPathResolver.ResolveContentPath("/");
150+
Assert.IsNotNull(content);
151+
Assert.AreEqual(_contentByName[$"Root {root}"].Key, content.Key);
152+
}
153+
154+
[TestCase(1, 1)]
155+
[TestCase(2, 2)]
156+
[TestCase(3, 3)]
157+
[TestCase(1, 2)]
158+
[TestCase(2, 3)]
159+
[TestCase(3, 1)]
160+
public void Child_With_StartItem(int root, int child)
161+
{
162+
SetRequestStartItem($"root-{root}");
163+
164+
var content = ApiContentPathResolver.ResolveContentPath($"/child-{child}");
165+
Assert.IsNotNull(content);
166+
Assert.AreEqual(_contentByName[$"Root {root}/Child {child}"].Key, content.Key);
167+
}
168+
169+
[TestCase(1, 1)]
170+
[TestCase(2, 2)]
171+
[TestCase(3, 3)]
172+
[TestCase(1, 2)]
173+
[TestCase(2, 3)]
174+
[TestCase(3, 1)]
175+
[ConfigureBuilder(ActionName = nameof(ConfigureIncludeTopLevelNodeInPath))]
176+
public void Child_With_StartItem_With_Top_Level_Node_Included(int root, int child)
177+
{
178+
SetRequestStartItem($"root-{root}");
179+
180+
var content = ApiContentPathResolver.ResolveContentPath($"/child-{child}");
181+
Assert.IsNotNull(content);
182+
Assert.AreEqual(_contentByName[$"Root {root}/Child {child}"].Key, content.Key);
183+
}
184+
185+
[TestCase(1, 1, 1)]
186+
[TestCase(2, 2, 2)]
187+
[TestCase(3, 3, 3)]
188+
[TestCase(1, 2, 3)]
189+
[TestCase(2, 3, 1)]
190+
[TestCase(3, 1, 2)]
191+
public void Grandchild_With_StartItem(int root, int child, int grandchild)
192+
{
193+
SetRequestStartItem($"root-{root}");
194+
195+
var content = ApiContentPathResolver.ResolveContentPath($"/child-{child}/grandchild-{grandchild}");
196+
Assert.IsNotNull(content);
197+
Assert.AreEqual(_contentByName[$"Root {root}/Child {child}/Grandchild {grandchild}"].Key, content.Key);
198+
}
199+
200+
[TestCase("/", 1)]
201+
[TestCase("/root-2", 2)]
202+
[TestCase("/root-3", 3)]
203+
public void Root_By_Path_With_StartItem(string path, int root)
204+
{
205+
SetRequestStartItem($"root-{root}");
206+
207+
var content = ApiContentPathResolver.ResolveContentPath(path);
208+
Assert.IsNotNull(content);
209+
Assert.AreEqual(_contentByName[$"Root {root}"].Key, content.Key);
210+
}
211+
212+
[TestCase("/", 1)]
213+
[TestCase("/root-2", 2)]
214+
[TestCase("/root-3", 3)]
215+
public void Root_By_Path_Without_StartItem(string path, int root)
216+
{
217+
var content = ApiContentPathResolver.ResolveContentPath(path);
218+
Assert.IsNotNull(content);
219+
Assert.AreEqual(_contentByName[$"Root {root}"].Key, content.Key);
220+
}
221+
222+
[TestCase(1)]
223+
[TestCase(2)]
224+
[TestCase(3)]
225+
public async Task Root_With_Domain_Bindings(int root)
226+
{
227+
await SetContentHost(_contentByName[$"Root {root}"], "some.host", "en-US");
228+
SetRequestHost("some.host");
229+
230+
var content = ApiContentPathResolver.ResolveContentPath("/");
231+
Assert.IsNotNull(content);
232+
Assert.AreEqual(_contentByName[$"Root {root}"].Key, content.Key);
233+
}
234+
235+
[TestCase("/a", 1)]
236+
[TestCase("/123", 2)]
237+
[TestCase("/no-such-child", 3)]
238+
[TestCase("/a/b", 1)]
239+
[TestCase("/123/456", 2)]
240+
[TestCase("/no-such-child/no-such-grandchild", 3)]
241+
public void Non_Existant_Descendant_By_Path_With_StartItem(string path, int root)
242+
{
243+
SetRequestStartItem($"root-{root}");
244+
245+
var content = ApiContentPathResolver.ResolveContentPath(path);
246+
Assert.IsNull(content);
247+
}
248+
249+
[TestCase("/a")]
250+
[TestCase("/123")]
251+
[TestCase("/a/b")]
252+
[TestCase("/123/456")]
253+
public void Non_Existant_Descendant_By_Path_Without_StartItem(string path)
254+
{
255+
var content = ApiContentPathResolver.ResolveContentPath(path);
256+
Assert.IsNull(content);
257+
}
258+
}

0 commit comments

Comments
 (0)