Improve deletion logic in UmbracoContentIndex (#21091)

* Improve deletion logic in UmbracoContentIndex

When re-indexing a node it will first delete it, however in many cases the query to delete doesn't return anything, however a delete command is still executed with an empty integer collection. We can avoid allocating and iterating lists and avoid the deletion call all together if the collection is empty.

* Apply suggestions from code review

Co-authored-by: Copilot <[email protected]>

* Clean-up and warning resolution whilst we are modifying class.

---------

Co-authored-by: Andy Butland <[email protected]>
Co-authored-by: Copilot <[email protected]>

Author: 3 people

src/Umbraco.Examine.Lucene/UmbracoContentIndex.cs

@@ -12,13 +12,16 @@
 namespace Umbraco.Cms.Infrastructure.Examine;
 
 /// <summary>
-///     An indexer for Umbraco content and media
+///     An indexer for Umbraco content and media.
 /// </summary>
 public class UmbracoContentIndex : UmbracoExamineIndex, IUmbracoContentIndex
 {
     private readonly ISet<string> _idOnlyFieldSet = new HashSet<string> { "id" };
     private readonly ILogger<UmbracoContentIndex> _logger;
 
+    /// <summary>
+    /// Initializes a new instance of the <see cref="UmbracoContentIndex"/> class.
+    /// </summary>
     public UmbracoContentIndex(
         ILoggerFactory loggerFactory,
         string name,
@@ -31,12 +34,8 @@ public UmbracoContentIndex(
         LanguageService = languageService;
         _logger = loggerFactory.CreateLogger<UmbracoContentIndex>();
 
-        LuceneDirectoryIndexOptions namedOptions = indexOptions.Get(name);
-        if (namedOptions == null)
-        {
-            throw new InvalidOperationException(
-                $"No named {typeof(LuceneDirectoryIndexOptions)} options with name {name}");
-        }
+        LuceneDirectoryIndexOptions namedOptions = indexOptions.Get(name)
+            ?? throw new InvalidOperationException($"No named {typeof(LuceneDirectoryIndexOptions)} options with name {name}");
 
         if (namedOptions.Validator is IContentValueSetValidator contentValueSetValidator)
         {
@@ -45,25 +44,25 @@ public UmbracoContentIndex(
         }
     }
 
+    /// <summary>
+    /// Gets the <see cref="ILocalizationService"/>.
+    /// </summary>
     protected ILocalizationService? LanguageService { get; }
 
     /// <summary>
     ///     Explicitly override because we need to do validation differently than the underlying logic
     /// </summary>
-    /// <param name="values"></param>
     void IIndex.IndexItems(IEnumerable<ValueSet> values) => PerformIndexItems(values, OnIndexOperationComplete);
 
     /// <summary>
-    ///     Special check for invalid paths
+    ///     Special check for invalid paths.
     /// </summary>
-    /// <param name="values"></param>
-    /// <param name="onComplete"></param>
     protected override void PerformIndexItems(IEnumerable<ValueSet> values, Action<IndexOperationEventArgs> onComplete)
     {
         // We don't want to re-enumerate this list, but we need to split it into 2x enumerables: invalid and valid items.
         // The Invalid items will be deleted, these are items that have invalid paths (i.e. moved to the recycle bin, etc...)
         // Then we'll index the Value group all together.
-        var invalidOrValid = values.GroupBy(v =>
+        IGrouping<ValueSetValidationStatus, ValueSet>[] invalidOrValid = values.GroupBy(v =>
         {
             if (!v.Values.TryGetValue("path", out IReadOnlyList<object>? paths) || paths.Count <= 0 || paths[0] == null)
             {
@@ -82,24 +81,24 @@ protected override void PerformIndexItems(IEnumerable<ValueSet> values, Action<I
         var hasDeletes = false;
         var hasUpdates = false;
 
-        // ordering by descending so that Filtered/Failed processes first
+        // Ordering by descending so that Filtered/Failed processes first.
         foreach (IGrouping<ValueSetValidationStatus, ValueSet> group in invalidOrValid.OrderByDescending(x => x.Key))
         {
             switch (group.Key)
             {
                 case ValueSetValidationStatus.Valid:
                     hasUpdates = true;
 
-                    //these are the valid ones, so just index them all at once
+                    // These are the valid ones, so just index them all at once.
                     base.PerformIndexItems(group.ToArray(), onComplete);
                     break;
                 case ValueSetValidationStatus.Failed:
-                    // don't index anything that is invalid
+                    // Don't index anything that is invalid.
                     break;
                 case ValueSetValidationStatus.Filtered:
                     hasDeletes = true;
 
-                    // these are the invalid/filtered items so we'll delete them
+                    // These are the invalid/filtered items so we'll delete them
                     // since the path is not valid we need to delete this item in
                     // case it exists in the index already and has now
                     // been moved to an invalid parent.
@@ -110,7 +109,7 @@ protected override void PerformIndexItems(IEnumerable<ValueSet> values, Action<I
 
         if ((hasDeletes && !hasUpdates) || (!hasDeletes && !hasUpdates))
         {
-            //we need to manually call the completed method
+            // We need to manually call the completed method.
             onComplete(new IndexOperationEventArgs(this, 0));
         }
     }
@@ -133,26 +132,44 @@ protected override void PerformDeleteFromIndex(IEnumerable<string> itemIds, Acti
         {
             var nodeId = idsAsList[i];
 
-            //find all descendants based on path
+            // Find all descendants based on path.
             var descendantPath = $@"\-1\,*{nodeId}\,*";
             var rawQuery = $"{UmbracoExamineFieldNames.IndexPathFieldName}:{descendantPath}";
             IQuery? c = Searcher.CreateQuery();
             IBooleanOperation? filtered = c.NativeQuery(rawQuery);
             IOrdering? selectedFields = filtered.SelectFields(_idOnlyFieldSet);
             ISearchResults? results = selectedFields.Execute();
-            if (_logger.IsEnabled(Microsoft.Extensions.Logging.LogLevel.Debug))
+            if (_logger.IsEnabled(LogLevel.Debug))
             {
                 _logger.LogDebug("DeleteFromIndex with query: {Query} (found {TotalItems} results)", rawQuery, results.TotalItemCount);
             }
 
+            // Avoid unnecessary operations when we have no items to handle. This is necessary for ExamineX's Elastic implementation
+            // which doesn't support making requests with an empty collection.
+            if (results.TotalItemCount == 0)
+            {
+                continue;
+            }
+
             var toRemove = results.Select(x => x.Id).ToList();
-            // delete those descendants (ensure base. is used here so we aren't calling ourselves!)
+
+            // Delete those descendants (ensure base. is used here so we aren't calling ourselves!).
             base.PerformDeleteFromIndex(toRemove, null);
 
-            // remove any ids from our list that were part of the descendants
+            // Remove any ids from our list that were part of the descendants.
             idsAsList.RemoveAll(x => toRemove.Contains(x));
         }
 
-        base.PerformDeleteFromIndex(idsAsList, onComplete);
+        // Avoid unnecessary operations when we have no items to handle. This is necessary for ExamineX's Elastic implementation
+        // which doesn't support making requests with an empty collection.
+        if (idsAsList.Count > 0)
+        {
+            base.PerformDeleteFromIndex(idsAsList, onComplete);
+        }
+        else
+        {
+            // Manually invoke the complete callback if provided, so we maintain consistency when there is or isn't anything to delete.
+            onComplete?.Invoke(new IndexOperationEventArgs(this, 0));
+        }
     }
 }