fix(index): fill model-declared indexes from existing documents (#171)

src/BLite.Core/Collections/DocumentCollection.cs

@@ -721,7 +721,7 @@ public async Task VacuumAsync(VacuumOptions? options = null, CancellationToken c
             {
                 foreach (var index in _indexManager.GetAllIndexes())
                 {
-                    await RebuildIndexFromScratchAsync(index.Name, ct).ConfigureAwait(false);
+                    await RebuildIndexFromScratchAsync(index, ct).ConfigureAwait(false);
                 }
             }
 
@@ -867,10 +867,12 @@ internal DocumentCollection(StorageEngine storage, ITransactionHolder transactio
 
     /// <summary>
     /// Decimal properties used to be indexed as <c>(double)value</c>. An index whose persisted
-    /// <see cref="IndexMetadata.KeyFormat"/> predates the exact decimal encoding is replaced by an
-    /// empty tree, repopulated from the documents and then marked with the current format, the first
-    /// time the collection is opened. The format is recorded only after the rebuild committed, so a
-    /// crash in between just repeats the migration on the next open. No-op for every other index.
+    /// <see cref="IndexMetadata.KeyFormat"/> predates the exact decimal encoding is rebuilt from the
+    /// documents into a copy that queries don't see, the first time the collection is opened. Only
+    /// after the copy committed is it swapped in, with the current format, in one metadata save, and
+    /// the old tree freed. If the rebuild fails or is interrupted, the copy's pages are freed and the
+    /// old index stays registered in its old format, so the next open repeats the migration.
+    /// No-op for every other index.
     /// </summary>
     private void MigrateLegacyDecimalIndexes()
     {
@@ -881,12 +883,13 @@ private void MigrateLegacyDecimalIndexes()
             var name = index.Name;
             try
             {
-                var fresh = _indexManager.RecreateIndex(name);
+                var fresh = _indexManager.CreateUnregistered(index.Definition);
+                fresh.KeyFormat = IndexMetadata.CurrentKeyFormat;
                 // Run on the thread pool so a captured SynchronizationContext cannot deadlock the wait.
                 // A document that cannot be loaded fails the migration rather than silently leaving
                 // the rebuilt index incomplete.
-                Task.Run(() => RebuildIndexAsync(fresh, skipUnreadableDocuments: false)).GetAwaiter().GetResult();
-                _indexManager.SetKeyFormat(name, IndexMetadata.CurrentKeyFormat);
+                Task.Run(() => FillUnregisteredIndexAsync(fresh, CancellationToken.None)).GetAwaiter().GetResult();
+                PublishFilledIndex(fresh, copy => _indexManager.ReplaceIndex(name, copy));
             }
             catch (Exception ex)
             {
@@ -1072,28 +1075,29 @@ public IEnumerable<CollectionIndexInfo> GetIndexes()
     [RequiresDynamicCode("Index creation compiles key selector expressions using Expression.Compile() which requires dynamic code generation.")]
     internal void ApplyIndexBuilder(Metadata.IndexBuilder<T> builder)
     {
-        // Use the IndexManager directly to ensure the index exists
-        // We need to convert the LambdaExpression to a typed expression if possible, 
-        // or add an untyped CreateIndexAsync to IndexManager.
+        var name = CollectionIndexManager<TId, T>.ResolveIndexName(builder.KeySelector, builder.Name);
+        if (_indexManager.GetIndex(name) != null)
+            return;
 
-        // For now, let's use a dynamic approach or cast if we know it's Func<T, object>
-        if (builder.Type == IndexType.Vector)
-        {
-            _indexManager.CreateVectorIndexUntyped(builder.KeySelector, builder.Dimensions, builder.Metric, builder.Name);
-        }
-        else if (builder.Type == IndexType.Spatial)
-        {
-            _indexManager.CreateSpatialIndexUntyped(builder.KeySelector, builder.Name);
-        }
-        else if (builder.KeySelector is System.Linq.Expressions.Expression<Func<T, object>> selector)
+        // The collection may already hold documents, e.g. when a new app version adds the index.
+        // Fill the index before registering it, so neither a crash nor a document that cannot be
+        // indexed leaves an empty index persisted that later opens would trust.
+        var definition = CollectionIndexManager<TId, T>.BuildDefinition(
+            builder.KeySelector, name, builder.IsUnique, builder.Type, builder.Dimensions, builder.Metric);
+        var index = _indexManager.CreateUnregistered(definition);
+        try
         {
-            _indexManager.EnsureIndex(selector, builder.Name, builder.IsUnique);
+            // Run on the thread pool so a captured SynchronizationContext cannot deadlock the wait.
+            Task.Run(() => FillUnregisteredIndexAsync(index, CancellationToken.None)).GetAwaiter().GetResult();
         }
-        else
+        catch (Exception ex)
         {
-            // Try to rebuild the expression or use untyped version
-            _indexManager.EnsureIndexUntyped(builder.KeySelector, builder.Name, builder.IsUnique);
+            throw new InvalidOperationException(
+                $"Cannot build index '{name}' of collection '{_collectionName}' from the documents already in it: {ex.Message.TrimEnd('.')}. " +
+                "Nothing was saved, but every open fails the same way until those documents can be indexed or the index " +
+                "is removed from the model (OnModelCreating).", ex);
         }
+        PublishFilledIndex(index, _indexManager.Register);
     }
 
     /// <summary>
@@ -1597,63 +1601,83 @@ public async IAsyncEnumerable<T> QueryIndexAsync(
     }
 
     /// <summary>
-    /// Populates an empty copy of index <paramref name="name"/> from the documents and swaps it in
+    /// Populates an empty copy of <paramref name="current"/> from the documents and swaps it in
     /// only after the rebuild committed; the old tree is then freed. If any document cannot be
     /// loaded or indexed, the rebuild fails and the existing index stays in place untouched,
     /// instead of being replaced by an incomplete one.
     /// </summary>
-    private async Task RebuildIndexFromScratchAsync(string name, CancellationToken ct)
+    private async Task RebuildIndexFromScratchAsync(CollectionSecondaryIndex<TId, T> current, CancellationToken ct)
+    {
+        var fresh = _indexManager.CreateUnregistered(current.Definition);
+        await FillUnregisteredIndexAsync(fresh, ct).ConfigureAwait(false);
+        PublishFilledIndex(fresh, copy => _indexManager.ReplaceIndex(current.Name, copy));
+    }
+
+    /// <summary>
+    /// Fills an index that is not registered yet (see <see cref="CollectionIndexManager{TId,T}.CreateUnregistered"/>)
+    /// from every document, in its own transaction, and commits it. If a document cannot be loaded
+    /// or indexed, or the commit fails, every page of the index is freed, including the pages its
+    /// splits added, the index is disposed and the exception propagates.
+    /// </summary>
+    private async Task FillUnregisteredIndexAsync(CollectionSecondaryIndex<TId, T> index, CancellationToken ct)
     {
-        var fresh = _indexManager.CreateUnregisteredCopy(name);
         var transaction = _storage.BeginTransaction(IsolationLevel.ReadCommitted);
         try
         {
-            await FillIndexAsync(fresh, transaction, skipUnreadableDocuments: false, ct).ConfigureAwait(false);
+            await FillIndexAsync(index, transaction, skipUnreadableDocuments: false, ct).ConfigureAwait(false);
             await transaction.CommitAsync(ct);
         }
         catch
         {
-            // Collect the copy's pages through the transaction before rolling it back: pages added
+            // Collect the index's pages through the transaction before rolling it back: pages added
             // by splits are allocated outside the transaction but linked only from nodes written
             // in it, so after the rollback they could not be found any more.
             IReadOnlyList<uint> pages = Array.Empty<uint>();
-            try { pages = fresh.CollectAllPages(transaction.TransactionId); }
+            try { pages = index.CollectAllPages(transaction.TransactionId); }
             catch { /* best-effort: never mask the original failure */ }
 
             try { await transaction.RollbackAsync(); }
             catch { /* best-effort: never mask the original failure */ }
 
-            FreeUnregisteredCopy(fresh, pages);
+            FreeUnregisteredIndex(index, pages);
             throw;
         }
+    }
 
+    /// <summary>
+    /// Hands an index filled by <see cref="FillUnregisteredIndexAsync"/> to <paramref name="register"/>
+    /// (<see cref="CollectionIndexManager{TId,T}.Register"/> or <see cref="CollectionIndexManager{TId,T}.ReplaceIndex"/>).
+    /// If that fails and the index did not get registered, nothing refers to it any more, so its
+    /// pages are freed and it is disposed.
+    /// </summary>
+    private void PublishFilledIndex(CollectionSecondaryIndex<TId, T> index, Action<CollectionSecondaryIndex<TId, T>> register)
+    {
         try
         {
-            _indexManager.ReplaceIndex(name, fresh);
+            register(index);
         }
         catch
         {
-            // The copy is committed but, unless the swap itself went through, nothing refers to it.
-            if (!ReferenceEquals(_indexManager.GetIndex(name), fresh))
+            if (!ReferenceEquals(_indexManager.GetIndex(index.Name), index))
             {
                 IReadOnlyList<uint> pages = Array.Empty<uint>();
-                try { pages = fresh.CollectAllPages(0); }
+                try { pages = index.CollectAllPages(0); }
                 catch { /* best-effort: never mask the original failure */ }
-                FreeUnregisteredCopy(fresh, pages);
+                FreeUnregisteredIndex(index, pages);
             }
             throw;
         }
     }
 
-    private void FreeUnregisteredCopy(CollectionSecondaryIndex<TId, T> copy, IReadOnlyList<uint> pages)
+    private void FreeUnregisteredIndex(CollectionSecondaryIndex<TId, T> index, IReadOnlyList<uint> pages)
     {
         try
         {
             foreach (var pageId in pages)
                 _storage.FreePage(pageId);
         }
         catch { /* best-effort: never mask the original failure */ }
-        copy.Dispose();
+        index.Dispose();
     }
 
     private async Task RebuildIndexAsync(CollectionSecondaryIndex<TId, T> index, CancellationToken ct = default, bool skipUnreadableDocuments = true)

src/BLite.Core/Indexing/CollectionIndexManager.cs

@@ -128,6 +128,66 @@ public CollectionSecondaryIndex<TId, T> CreateIndex(CollectionIndexDefinition<T>
         }
     }
 
+    /// <summary>The name an index on <paramref name="keySelector"/> gets when none is given.</summary>
+    internal static string ResolveIndexName(LambdaExpression keySelector, string? name)
+        => name ?? GenerateIndexName(ExpressionAnalyzer.ExtractPropertyPaths(keySelector));
+
+    /// <summary>
+    /// Builds an index definition from an untyped key selector, as declared in the model.
+    /// </summary>
+    [RequiresDynamicCode("Index creation compiles key selector expressions using Expression.Compile() which requires dynamic code generation.")]
+    internal static CollectionIndexDefinition<T> BuildDefinition(
+        LambdaExpression keySelector,
+        string? name,
+        bool unique,
+        IndexType type,
+        int dimensions,
+        VectorMetric metric)
+    {
+        var propertyPaths = ExpressionAnalyzer.ExtractPropertyPaths(keySelector);
+        var body = keySelector.Body.Type == typeof(object)
+            ? keySelector.Body
+            : Expression.Convert(keySelector.Body, typeof(object));
+        var lambda = Expression.Lambda<Func<T, object>>(body, keySelector.Parameters);
+
+        return new CollectionIndexDefinition<T>(
+            name ?? GenerateIndexName(propertyPaths), propertyPaths, lambda, unique, type, false, dimensions, metric);
+    }
+
+    /// <summary>
+    /// Creates an index for <paramref name="definition"/> without registering or persisting it,
+    /// so it can be filled before queries or the collection metadata see it. The index owns its
+    /// root page: hand it to <see cref="Register"/> or <see cref="ReplaceIndex"/>, or release its
+    /// pages.
+    /// </summary>
+    internal CollectionSecondaryIndex<TId, T> CreateUnregistered(CollectionIndexDefinition<T> definition)
+    {
+        lock (_lock)
+        {
+            if (_disposed)
+                throw new ObjectDisposedException(nameof(CollectionIndexManager<TId, T>));
+
+            return new CollectionSecondaryIndex<TId, T>(definition, _storage, _mapper);
+        }
+    }
+
+    /// <summary>
+    /// Registers an index built by <see cref="CreateUnregistered"/> and persists it. If the
+    /// metadata cannot be saved, the index is not registered and remains the caller's to release.
+    /// </summary>
+    internal void Register(CollectionSecondaryIndex<TId, T> index)
+    {
+        lock (_lock)
+        {
+            if (_disposed)
+                throw new ObjectDisposedException(nameof(CollectionIndexManager<TId, T>));
+            if (_indexes.ContainsKey(index.Name))
+                throw new InvalidOperationException($"Index '{index.Name}' already exists");
+
+            PublishLocked(index.Name, index, previous: null);
+        }
+    }
+
     // ... methods ...
 
     /// <summary>
@@ -206,24 +266,7 @@ public CollectionSecondaryIndex<TId, T> EnsureIndex(
         }
     }
 
-    [RequiresDynamicCode("Index creation compiles key selector expressions using Expression.Compile() which requires dynamic code generation.")]
-    internal CollectionSecondaryIndex<TId, T> EnsureIndexUntyped(
-        LambdaExpression keySelector,
-        string? name = null,
-        bool unique = false)
-    {
-        // Convert LambdaExpression to Expression<Func<T, object>> properly by sharing parameters
-        var body = keySelector.Body;
-        if (body.Type != typeof(object))
-        {
-            body = Expression.Convert(body, typeof(object));
-        }
-
-        var lambda = Expression.Lambda<Func<T, object>>(body, keySelector.Parameters);
-
-        return EnsureIndex(lambda, name, unique);
-    }
-
+    [Obsolete("Use model HasVectorIndex/HasSpatialIndex or CreateIndex(definition); will be removed in 6.0")]
     [RequiresDynamicCode("Index creation compiles key selector expressions using Expression.Compile() which requires dynamic code generation.")]
     public CollectionSecondaryIndex<TId, T> CreateVectorIndexUntyped(
         LambdaExpression keySelector,
@@ -252,6 +295,7 @@ public CollectionSecondaryIndex<TId, T> CreateVectorIndexUntyped(
         }
     }
 
+    [Obsolete("Use model HasVectorIndex/HasSpatialIndex or CreateIndex(definition); will be removed in 6.0")]
     [RequiresDynamicCode("Index creation compiles key selector expressions using Expression.Compile() which requires dynamic code generation.")]
     public CollectionSecondaryIndex<TId, T> CreateSpatialIndexUntyped(
         LambdaExpression keySelector,
@@ -307,45 +351,6 @@ public bool DropIndex(string name)
         }
     }
 
-    /// <summary>
-    /// Replaces an index with an empty one of the same definition, freeing the old tree's pages.
-    /// The new index keeps the old <see cref="CollectionSecondaryIndex{TId,T}.KeyFormat"/>: the
-    /// caller repopulates it and then records the new format with <see cref="SetKeyFormat"/>, so an
-    /// interrupted rebuild is simply redone on the next open.
-    /// </summary>
-    internal CollectionSecondaryIndex<TId, T> RecreateIndex(string name)
-    {
-        lock (_lock)
-        {
-            if (!_indexes.TryGetValue(name, out var old))
-                throw new InvalidOperationException($"Index '{name}' does not exist");
-
-            var fresh = CreateUnregisteredCopy(name);
-            fresh.KeyFormat = old.KeyFormat;
-            ReplaceIndex(name, fresh);
-            return fresh;
-        }
-    }
-
-    /// <summary>
-    /// Creates an empty index with the same definition as <paramref name="name"/> without
-    /// registering it, so queries keep using the existing index while the copy is populated.
-    /// The copy owns its root page: hand it to <see cref="ReplaceIndex"/> or release it with
-    /// <see cref="CollectionSecondaryIndex{TId,T}.FreeAllPages"/>.
-    /// </summary>
-    internal CollectionSecondaryIndex<TId, T> CreateUnregisteredCopy(string name)
-    {
-        lock (_lock)
-        {
-            if (_disposed)
-                throw new ObjectDisposedException(nameof(CollectionIndexManager<TId, T>));
-            if (!_indexes.TryGetValue(name, out var old))
-                throw new InvalidOperationException($"Index '{name}' does not exist");
-
-            return new CollectionSecondaryIndex<TId, T>(old.Definition, _storage, _mapper);
-        }
-    }
-
     /// <summary>
     /// Registers <paramref name="fresh"/> under <paramref name="name"/>, persists its root page
     /// and frees the pages of the index it replaces. If the metadata cannot be saved, the old
@@ -360,29 +365,43 @@ internal void ReplaceIndex(string name, CollectionSecondaryIndex<TId, T> fresh)
             if (!_indexes.TryGetValue(name, out var old))
                 throw new InvalidOperationException($"Index '{name}' does not exist");
 
-            _indexes[name] = fresh;
-            _cachedIndexInfo = null; // invalidate cache
-            RefreshHasUniqueBTreeIndex();
-
             // Persist the new root before freeing the old tree: a crash in between leaks the old
             // pages, whereas the reverse order would let the next open free them a second time.
-            try
-            {
-                SaveMetadata();
-            }
-            catch
-            {
-                // Keep the old index registered, matching the metadata on disk; the caller still
-                // owns the copy.
-                _indexes[name] = old;
-                UpdateMetadata();
-                throw;
-            }
+            PublishLocked(name, fresh, old);
             old.FreeAllPages(_storage);
             old.Dispose();
         }
     }
 
+    /// <summary>
+    /// Registers <paramref name="index"/> under <paramref name="name"/> in place of
+    /// <paramref name="previous"/> (null: none) and saves the metadata. If the save fails, the
+    /// previous registration is restored, so memory keeps matching the metadata on disk, and the
+    /// exception propagates. Must be called under <see cref="_lock"/>.
+    /// </summary>
+    private void PublishLocked(string name, CollectionSecondaryIndex<TId, T> index, CollectionSecondaryIndex<TId, T>? previous)
+    {
+        _indexes[name] = index;
+        _cachedIndexInfo = null; // invalidate cache
+        RefreshHasUniqueBTreeIndex();
+
+        try
+        {
+            SaveMetadata();
+        }
+        catch
+        {
+            if (previous != null)
+                _indexes[name] = previous;
+            else
+                _indexes.Remove(name);
+            _cachedIndexInfo = null;
+            RefreshHasUniqueBTreeIndex();
+            UpdateMetadata();
+            throw;
+        }
+    }
+
     /// <summary>Records the key format an index's pages are written with and persists it.</summary>
     internal void SetKeyFormat(string name, byte keyFormat)
     {