Fix pagination in Content Delivery API Index Helper (#19606)

* Refactor descendant enumeration in DeliveryApiContentIndexHelper

Improved loop condition to allow for processing of more than 10.000 descendants for indexing.

* Add failing test for original issue.

* Renamed variable for clarity.

---------

Co-authored-by: Brynjar Þorsteinsson <[email protected]>
Co-authored-by: Andy Butland <[email protected]>

Author: 3 people

src/Umbraco.Infrastructure/Examine/DeliveryApiContentIndexHelper.cs

@@ -1,4 +1,4 @@
-using Microsoft.Extensions.Options;
+using Microsoft.Extensions.Options;
 using Umbraco.Cms.Core.Configuration.Models;
 using Umbraco.Cms.Core.Models;
 using Umbraco.Cms.Core.Persistence.Querying;
@@ -28,21 +28,28 @@ public DeliveryApiContentIndexHelper(
     public void EnumerateApplicableDescendantsForContentIndex(int rootContentId, Action<IContent[]> actionToPerform)
     {
         const int pageSize = 10000;
-        var pageIndex = 0;
+        EnumerateApplicableDescendantsForContentIndex(rootContentId, actionToPerform, pageSize);
+    }
+
+    internal void EnumerateApplicableDescendantsForContentIndex(int rootContentId, Action<IContent[]> actionToPerform, int pageSize)
+    {
+        var itemIndex = 0;
+        long total;
 
-        IContent[] descendants;
         IQuery<IContent> query = _umbracoDatabaseFactory.SqlContext.Query<IContent>().Where(content => content.Trashed == false);
+
+        IContent[] descendants;
         do
         {
             descendants = _contentService
-                .GetPagedDescendants(rootContentId, pageIndex, pageSize, out _, query, Ordering.By("Path"))
+                .GetPagedDescendants(rootContentId, itemIndex / pageSize, pageSize, out total, query, Ordering.By("Path"))
                 .Where(descendant => _deliveryApiSettings.IsAllowedContentType(descendant.ContentType.Alias))
                 .ToArray();
 
-            actionToPerform(descendants.ToArray());
+            actionToPerform(descendants);
 
-            pageIndex++;
+            itemIndex += pageSize;
         }
-        while (descendants.Length == pageSize);
+        while (descendants.Length > 0 && itemIndex < total);
     }
 }

tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Examine/DeliveryApiContentIndexHelperTests.cs

@@ -0,0 +1,120 @@
+using Microsoft.Extensions.Options;
+using Moq;
+using NUnit.Framework;
+using Umbraco.Cms.Core.Configuration.Models;
+using Umbraco.Cms.Core.Models;
+using Umbraco.Cms.Core.Services;
+using Umbraco.Cms.Infrastructure.Examine;
+using Umbraco.Cms.Infrastructure.Persistence;
+using Umbraco.Cms.Tests.Common.Builders;
+using Umbraco.Cms.Tests.Common.Testing;
+using Umbraco.Cms.Tests.Integration.Testing;
+
+namespace Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Examine;
+
+[UmbracoTest(Database = UmbracoTestOptions.Database.NewSchemaPerTest)]
+[TestFixture]
+public class DeliveryApiContentIndexHelperTests : UmbracoIntegrationTestWithContent
+{
+    public override void CreateTestData()
+    {
+        base.CreateTestData();
+
+        // Save an extra, published content item of a different type to those created via the base class,
+        // that we'll use to test filtering out disallowed content types.
+        var template = TemplateBuilder.CreateTextPageTemplate("textPage2");
+        FileService.SaveTemplate(template);
+
+        var contentType = ContentTypeBuilder.CreateSimpleContentType("umbTextpage2", "Textpage2", defaultTemplateId: template.Id);
+        contentType.Key = Guid.NewGuid();
+        ContentTypeService.Save(contentType);
+
+        ContentType.AllowedContentTypes =
+        [
+            new ContentTypeSort(ContentType.Key, 0, "umbTextpage"),
+            new ContentTypeSort(contentType.Key, 1, "umbTextpage2"),
+        ];
+        ContentTypeService.Save(ContentType);
+
+        var subpage = ContentBuilder.CreateSimpleContent(contentType, "Alternate Text Page 4", Textpage.Id);
+        ContentService.Save(subpage);
+
+        // And then add some more of the first type, so the one we'll filter out in tests isn't in the last page.
+        for (int i = 0; i < 5; i++)
+        {
+            subpage = ContentBuilder.CreateSimpleContent(ContentType, $"Text Page {5 + i}", Textpage.Id);
+            ContentService.Save(subpage);
+        }
+    }
+
+    [Test]
+    public void Can_Enumerate_Descendants_For_Content_Index()
+    {
+        var sut = CreateDeliveryApiContentIndexHelper();
+
+        var expectedNumberOfContentItems = GetExpectedNumberOfContentItems();
+
+        var contentEnumerated = 0;
+        Action<IContent[]> actionToPerform = content =>
+        {
+            contentEnumerated += content.Length;
+        };
+
+        const int pageSize = 3;
+        sut.EnumerateApplicableDescendantsForContentIndex(
+            Cms.Core.Constants.System.Root,
+            actionToPerform,
+            pageSize);
+
+        Assert.AreEqual(expectedNumberOfContentItems, contentEnumerated);
+    }
+
+    [Test]
+    public void Can_Enumerate_Descendants_For_Content_Index_With_Disallowed_Content_Type()
+    {
+        var sut = CreateDeliveryApiContentIndexHelper(["umbTextPage2"]);
+
+        var expectedNumberOfContentItems = GetExpectedNumberOfContentItems();
+
+        var contentEnumerated = 0;
+        Action<IContent[]> actionToPerform = content =>
+        {
+            contentEnumerated += content.Length;
+        };
+
+        const int pageSize = 3;
+        sut.EnumerateApplicableDescendantsForContentIndex(
+            Cms.Core.Constants.System.Root,
+            actionToPerform,
+            pageSize);
+
+        Assert.AreEqual(expectedNumberOfContentItems - 1, contentEnumerated);
+    }
+
+    private DeliveryApiContentIndexHelper CreateDeliveryApiContentIndexHelper(HashSet<string>? disallowedContentTypeAliases = null)
+    {
+        return new DeliveryApiContentIndexHelper(
+            ContentService,
+            GetRequiredService<IUmbracoDatabaseFactory>(),
+            GetDeliveryApiSettings(disallowedContentTypeAliases ?? []));
+    }
+
+    private IOptionsMonitor<DeliveryApiSettings> GetDeliveryApiSettings(HashSet<string> disallowedContentTypeAliases)
+    {
+        var deliveryApiSettings = new DeliveryApiSettings
+        {
+            DisallowedContentTypeAliases = disallowedContentTypeAliases,
+        };
+
+        var optionsMonitorMock = new Mock<IOptionsMonitor<DeliveryApiSettings>>();
+        optionsMonitorMock.Setup(o => o.CurrentValue).Returns(deliveryApiSettings);
+        return optionsMonitorMock.Object;
+    }
+
+    private int GetExpectedNumberOfContentItems()
+    {
+        var result = ContentService.GetAllPublished().Count();
+        Assert.AreEqual(10, result);
+        return result;
+    }
+}