Page the "Register content move redirects" job so it survives large tables (6.x backport of #188)

CHANGELOG.md

@@ -2,6 +2,17 @@
 
 All notable changes to this project will be documented in this file.
 
+## [Unreleased]
+
+### Added
+
+- `OptimizelyNotFoundHandlerOptions.MovedContentBatchSize` (default 1000) and `NotFoundHandlerOptions.CommandTimeout` so the "Register content move redirects" job pages through large `NotFoundHandler.ContentUrlHistory` tables instead of loading every row in one unbounded query that exceeded the command timeout.
+- `IContentUrlHistoryLoader.GetAllMoved(int skip, int take)` for paged access to moved content. Ships as a default interface implementation (pages in-memory over `GetAllMoved()`), so existing implementers keep compiling.
+
+### Changed
+
+- `SqlDataExecutor.ExecuteQuery` now propagates SQL exceptions (e.g. command timeouts) instead of swallowing them and returning an empty result, which previously surfaced as a misleading "Cannot find table 0" error.
+
 ## [1.1.0]
 
 - Refctoring

README.md

@@ -152,6 +152,8 @@ If the `bufferSize` is set to `0`, the `threshold` value will be ignored, and ev
 
 **LogWithHostname**: Set to `true` to include hostname in the log. Useful in a multisite environment with several hostnames/domains. Default is `false`
 
+**CommandTimeout**: Command timeout, in seconds, applied to SQL queries issued by the NotFound handler. Raise it past the SqlClient default of 30 if you have very large NotFound handler tables. Default is 30
+
 **ActiveStatusCodes**: A integerlist with the status codes that NotFoundHandler will be active on. (Ex. ```options.ActiveStatusCodes = new int[] { StatusCodes.Status404NotFound, StatusCodes.Status410Gone };```)
 
 ### Specifying ignored resources
@@ -320,7 +322,13 @@ An Optimizely scheduled job was added - <code>[Geta NotFoundHandler] Suggestions
 
 Additionally, there are two optimizely scheduled jobs responsible for:
 - *[Geta NotFoundHandler] Index content URLs* - as mentioned before, this job indexes URLs of content. Usually, it is required to run this job only once. All new content is automatically indexed. But if for some reason content publish events are not firing when creating new content (for example, during the import), then you should set this job to run frequently.
-- *[Geta NotFoundHandler] Register content move redirects* - this job creates redirects based on registered moved content. Normally, this job is not required at all, but there might be situations when content move is registered but redirect creation is not completed. This could happen during deployments. In this case, you can manually run this job or schedule it to run time to time to fix such issues.
+- *[Geta NotFoundHandler] Register content move redirects* - this job creates redirects based on registered moved content. Normally, this job is not required at all, but there might be situations when content move is registered but redirect creation is not completed. This could happen during deployments. In this case, you can manually run this job or schedule it to run time to time to fix such issues. The job processes the `NotFoundHandler.ContentUrlHistory` table page-by-page so it scales to large tables. You can tune the page size via `MovedContentBatchSize` (default 1000):
+```
+services.AddOptimizelyNotFoundHandler(o =>
+{
+    o.MovedContentBatchSize = 1000;
+});
+```
 
 
 # Troubleshooting

src/Geta.NotFoundHandler.Optimizely/Core/AutomaticRedirects/IContentUrlHistoryLoader.cs

@@ -2,13 +2,29 @@
 // Licensed under Apache-2.0. See the LICENSE file in the project root for more information
 
 using System.Collections.Generic;
+using System.Linq;
 
 namespace Geta.NotFoundHandler.Optimizely.Core.AutomaticRedirects
 {
     public interface IContentUrlHistoryLoader
     {
         bool IsRegistered(ContentUrlHistory entity);
         IEnumerable<(string contentKey, IReadOnlyCollection<ContentUrlHistory> histories)> GetAllMoved();
+
+        /// <summary>
+        /// Returns a single page of moved content (keys with more than one URL-history entry),
+        /// ordered by content key. Lets callers stream large tables instead of loading every row
+        /// in one unbounded query.
+        /// </summary>
+        /// <param name="skip">Number of moved content keys to skip.</param>
+        /// <param name="take">Maximum number of moved content keys to return.</param>
+        /// <remarks>
+        /// Default implementation pages in-memory over <see cref="GetAllMoved()"/> so existing
+        /// implementers keep compiling; data-backed stores should override it with a server-side paged query.
+        /// </remarks>
+        IEnumerable<(string contentKey, IReadOnlyCollection<ContentUrlHistory> histories)> GetAllMoved(int skip, int take)
+            => GetAllMoved().Skip(skip).Take(take);
+
         IReadOnlyCollection<ContentUrlHistory> GetMoved(string contentKey);
     }
 }

src/Geta.NotFoundHandler.Optimizely/Core/AutomaticRedirects/RegisterMovedContentRedirectsJob.cs

@@ -6,6 +6,8 @@
 using EPiServer.PlugIn;
 using EPiServer.Scheduler;
 using Geta.NotFoundHandler.Optimizely.Infrastructure;
+using Geta.NotFoundHandler.Optimizely.Infrastructure.Configuration;
+using Microsoft.Extensions.Options;
 
 namespace Geta.NotFoundHandler.Optimizely.Core.AutomaticRedirects
 {
@@ -17,60 +19,78 @@ public class RegisterMovedContentRedirectsJob : ScheduledJobBase
         private readonly IContentUrlHistoryLoader _contentUrlHistoryLoader;
         private readonly JobStatusLogger _jobStatusLogger;
         private readonly IAutomaticRedirectsService _automaticRedirectsService;
+        private readonly int _batchSize;
         private bool _stopped;
 
         public RegisterMovedContentRedirectsJob(
             IAutomaticRedirectsService automaticRedirectsService,
-            IContentUrlHistoryLoader contentUrlHistoryLoader)
+            IContentUrlHistoryLoader contentUrlHistoryLoader,
+            IOptions<OptimizelyNotFoundHandlerOptions> options)
         {
             _automaticRedirectsService = automaticRedirectsService;
             _contentUrlHistoryLoader = contentUrlHistoryLoader;
+            // Clamp to >= 1: a misconfigured 0/negative would page with FETCH NEXT 0 ROWS (invalid SQL) and fail the job.
+            _batchSize = Math.Max(1, options.Value.MovedContentBatchSize);
             _jobStatusLogger = new JobStatusLogger(OnStatusChanged);
 
             IsStoppable = true;
         }
 
         public override string Execute()
         {
-            var movedContent = _contentUrlHistoryLoader.GetAllMoved().ToList();
-            var totalCount = movedContent.Count;
             var successCount = 0;
             var failedCount = 0;
             var currentCount = 0;
+            var skip = 0;
 
-            _jobStatusLogger.LogWithStatus($"In total will process moved content: {totalCount}");
+            _jobStatusLogger.LogWithStatus($"Processing moved content in batches of {_batchSize}");
 
-            foreach (var content in movedContent)
+            // Page through the moved content rather than materialising the whole table up front.
+            // CreateRedirects only touches the redirects store, not ContentUrlHistory, so the moved
+            // set is stable for the duration of the run and skip-based paging never skips a key.
+            while (true)
             {
-                if (_stopped)
+                var batch = _contentUrlHistoryLoader.GetAllMoved(skip, _batchSize).ToList();
+
+                foreach (var content in batch)
                 {
-                    _jobStatusLogger.Log(
-                        $"Job was stopped, successful content handled before stopped: {successCount} out of total {totalCount} content");
-                    return _jobStatusLogger.ToString();
-                }
+                    if (_stopped)
+                    {
+                        _jobStatusLogger.Log(
+                            $"Job was stopped, successful content handled before stopped: {successCount} out of {currentCount} processed");
+                        return _jobStatusLogger.ToString();
+                    }
 
-                currentCount++;
+                    currentCount++;
 
-                try
-                {
-                    _automaticRedirectsService.CreateRedirects(content.histories);
-                    successCount++;
-                }
-                catch (Exception ex)
-                {
-                    _jobStatusLogger.Log($"Processing [{content.contentKey}] failed, exception: {ex}");
-                    failedCount++;
+                    try
+                    {
+                        _automaticRedirectsService.CreateRedirects(content.histories);
+                        successCount++;
+                    }
+                    catch (Exception ex)
+                    {
+                        _jobStatusLogger.Log($"Processing [{content.contentKey}] failed, exception: {ex}");
+                        failedCount++;
+                    }
+
+                    if (currentCount % 500 == 0)
+                    {
+                        _jobStatusLogger.Status(
+                            $"Processed {currentCount}, of whom successful {successCount}; failed: {failedCount}");
+                    }
                 }
 
-                if (currentCount % 500 == 0)
+                if (batch.Count < _batchSize)
                 {
-                    _jobStatusLogger.Status(
-                        $"Processed {currentCount} of whom successful {successCount} out of total {totalCount} content; failed: {failedCount}");
+                    break;
                 }
+
+                skip += _batchSize;
             }
 
             _jobStatusLogger.Log(
-                $"Processed {currentCount} of whom successful {successCount} out of total {totalCount} content; failed: {failedCount}");
+                $"Processed {currentCount}, of whom successful {successCount}; failed: {failedCount}");
 
             return _jobStatusLogger.ToString();
         }

src/Geta.NotFoundHandler.Optimizely/Data/SqlContentUrlHistoryRepository.cs

@@ -18,6 +18,7 @@ public class SqlContentUrlHistoryRepository : IRepository<ContentUrlHistory>, IC
     {
         private const string ContentUrlHistoryTable = "[dbo].[NotFoundHandler.ContentUrlHistory]";
         private const string AllFields = "Id, ContentKey, Urls, CreatedUtc, md5_ContentKey";
+        private const int DefaultPageSize = 1000;
         private readonly IDataExecutor _dataExecutor;
 
         public SqlContentUrlHistoryRepository(IDataExecutor dataExecutor)
@@ -69,17 +70,56 @@ public bool IsRegistered(ContentUrlHistory entity)
 
         public IEnumerable<(string contentKey, IReadOnlyCollection<ContentUrlHistory> histories)> GetAllMoved()
         {
+            // Stream the table one page at a time rather than issuing a single unbounded query that
+            // grows with the table and can exceed the command timeout on large sites.
+            var skip = 0;
+            while (true)
+            {
+                var page = GetAllMoved(skip, DefaultPageSize).ToList();
+
+                foreach (var moved in page)
+                {
+                    yield return moved;
+                }
+
+                if (page.Count < DefaultPageSize)
+                {
+                    yield break;
+                }
+
+                skip += DefaultPageSize;
+            }
+        }
+
+        public IEnumerable<(string contentKey, IReadOnlyCollection<ContentUrlHistory> histories)> GetAllMoved(int skip, int take)
+        {
+            // Guard the inputs: "FETCH NEXT 0 ROWS ONLY" (or a negative count) is invalid SQL, and a
+            // negative offset would fail too, so a bad caller shouldn't reach the database.
+            if (take <= 0)
+            {
+                return Enumerable.Empty<(string, IReadOnlyCollection<ContentUrlHistory>)>();
+            }
+
+            skip = Math.Max(0, skip);
+
+            // Page over the moved content keys (md5_ContentKey is the hash of ContentKey, so each key
+            // maps to a single group and is never split across pages) and return their histories.
             var sqlCommand = $@"SELECT h.Id, h.ContentKey, h.Urls, h.CreatedUtc, h.md5_ContentKey
                                 FROM {ContentUrlHistoryTable} h
-                                INNER JOIN 
+                                INNER JOIN
                                     (SELECT ContentKey, md5_ContentKey
                                     FROM {ContentUrlHistoryTable}
                                     GROUP BY ContentKey, md5_ContentKey
-                                    HAVING COUNT(*) > 1) k
+                                    HAVING COUNT(*) > 1
+                                    ORDER BY ContentKey, md5_ContentKey
+                                    OFFSET @skip ROWS FETCH NEXT @take ROWS ONLY) k
                                 ON h.ContentKey = k.ContentKey AND h.md5_ContentKey = k.md5_ContentKey
                                 ORDER BY h.ContentKey, h.CreatedUtc DESC";
 
-            var dataTable = _dataExecutor.ExecuteQuery(sqlCommand);
+            var dataTable = _dataExecutor.ExecuteQuery(
+                sqlCommand,
+                _dataExecutor.CreateIntParameter("skip", skip),
+                _dataExecutor.CreateIntParameter("take", take));
 
             var histories = ToContentUrlHistory(dataTable);
 

src/Geta.NotFoundHandler.Optimizely/Infrastructure/Configuration/OptimizelyNotFoundHandlerOptions.cs

@@ -16,6 +16,12 @@ public class OptimizelyNotFoundHandlerOptions
         public bool AutomaticRedirectsEnabled { get; set; }
         public RedirectType AutomaticRedirectType { get; set; } = RedirectType.Temporary;
 
+        /// <summary>
+        /// Number of moved content keys the "Register content move redirects" job loads per page.
+        /// The job processes the table page-by-page so it scales to large NotFoundHandler tables.
+        /// </summary>
+        public int MovedContentBatchSize { get; set; } = 1000;
+
         private readonly List<Type> _contentKeyProviders = new();
         public IEnumerable<Type> ContentKeyProviders => _contentKeyProviders;
 

src/Geta.NotFoundHandler/Data/SqlDataExecutor.cs

@@ -16,12 +16,15 @@ public class SqlDataExecutor : IDataExecutor
     {
         private readonly ILogger<SqlDataExecutor> _logger;
         private readonly string _connectionString;
+        private readonly int _commandTimeout;
 
         public SqlDataExecutor(
             IOptions<NotFoundHandlerOptions> options,
             ILogger<SqlDataExecutor> logger)
         {
             _connectionString = options.Value.ConnectionString;
+            // SqlCommand.CommandTimeout throws on a negative value; clamp so a misconfiguration can't break every query. (0 = no timeout.)
+            _commandTimeout = Math.Max(0, options.Value.CommandTimeout);
             _logger = logger;
         }
 
@@ -42,6 +45,12 @@ public DataTable ExecuteQuery(string sqlCommand, params IDbDataParameter[] param
                 _logger.LogError(ex,
                                  "An error occurred in the ExecuteSQL method with the following sql: {SqlCommand}",
                                  sqlCommand);
+
+                // Previously the exception was swallowed here and execution fell through to
+                // 'return ds.Tables[0]'. On a failed Fill the DataSet has no tables, so that line
+                // threw IndexOutOfRangeException ("Cannot find table 0"), masking the real cause
+                // (e.g. a command timeout). Rethrow so callers see the actual failure.
+                throw;
             }
 
             return ds.Tables[0];
@@ -199,10 +208,11 @@ private static SqlParameter CreateReturnParameter()
             return parameter;
         }
 
-        private static SqlCommand CreateCommand(SqlConnection connection, string sqlCommand, params IDbDataParameter[] parameters)
+        private SqlCommand CreateCommand(SqlConnection connection, string sqlCommand, params IDbDataParameter[] parameters)
         {
             var command = connection.CreateCommand();
             command.CommandText = sqlCommand;
+            command.CommandTimeout = _commandTimeout;
 
             if (parameters != null)
             {

src/Geta.NotFoundHandler/Infrastructure/Configuration/NotFoundHandlerOptions.cs

@@ -17,6 +17,13 @@ public class NotFoundHandlerOptions
 
         public int BufferSize { get; set; } = 30;
         public int ThreshHold { get; set; } = 5;
+
+        /// <summary>
+        /// Command timeout, in seconds, applied to SQL queries issued by <see cref="Data.SqlDataExecutor"/>.
+        /// Exposed so sites with large NotFoundHandler tables can raise it past the SqlClient default of 30s.
+        /// Set to 0 for no timeout; negative values are treated as 0.
+        /// </summary>
+        public int CommandTimeout { get; set; } = 30;
         public SuggestionsCleanupOptions SuggestionsCleanupOptions { get; set; } = new();
         public bool UseInternalScheduler { get; set; }
         public string InternalSchedulerCronInterval { get; set; } = "0 0 * * *";