Skip to content

Commit 0d5256d

Browse files
authored
fix: make index ID configurable in NodeSearchService (#1106)
## Summary Fixes known issue #4: Index ID was hardcoded as `"fts-main"` instead of being configurable. ## Changes - **`src/Core/Search/SearchConstants.cs`**: Added `DefaultFtsIndexId` constant - **`src/Core/Search/NodeSearchService.cs`**: Added `indexId` constructor parameter with default value for backward compatibility - **`tests/Core.Tests/Search/NodeSearchServiceIndexIdTests.cs`**: Added 4 unit tests - **`KNOWN-ISSUES.md`**: Removed resolved issue #4 ## Test plan - [x] All 524 tests pass (310 Core + 214 Main) - [x] Zero skipped tests - [x] Code coverage at 83.85% (above 80% threshold) - [x] `build.sh` passes with 0 warnings - [x] `format.sh` passes - [x] `coverage.sh` passes ## Backward Compatibility Fully backward compatible - the constructor parameter has a default value matching the previous hardcoded behavior.
1 parent 2ebdbe4 commit 0d5256d

File tree

4 files changed

+249
-17
lines changed

4 files changed

+249
-17
lines changed

KNOWN-ISSUES.md

Lines changed: 1 addition & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -86,21 +86,7 @@ km search 'content:"user:password"'
8686

8787
## Configuration Limitations
8888

89-
### 4. Index ID Hardcoded
90-
91-
**Status:** Known limitation
92-
93-
**Issue:** Index ID is hardcoded as `"fts-main"` instead of being loaded from configuration.
94-
95-
**Location:** `src/Core/Search/NodeSearchService.cs:77`
96-
97-
**Impact:** Cannot configure multiple indexes per node.
98-
99-
**Fix Required:** Load index configuration from node settings.
100-
101-
---
102-
103-
### 5. Index Weights Not Configurable
89+
### 4. Index Weights Not Configurable
10490

10591
**Status:** Known limitation
10692

src/Core/Search/NodeSearchService.cs

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ namespace KernelMemory.Core.Search;
1414
public sealed class NodeSearchService
1515
{
1616
private readonly string _nodeId;
17+
private readonly string _indexId;
1718
private readonly IFtsIndex _ftsIndex;
1819
private readonly IContentStorage _contentStorage;
1920

@@ -23,9 +24,15 @@ public sealed class NodeSearchService
2324
/// <param name="nodeId">The node ID this service operates on.</param>
2425
/// <param name="ftsIndex">The FTS index for this node.</param>
2526
/// <param name="contentStorage">The content storage for loading full records.</param>
26-
public NodeSearchService(string nodeId, IFtsIndex ftsIndex, IContentStorage contentStorage)
27+
/// <param name="indexId">Optional index ID for this FTS index. Defaults to SearchConstants.DefaultFtsIndexId.</param>
28+
public NodeSearchService(
29+
string nodeId,
30+
IFtsIndex ftsIndex,
31+
IContentStorage contentStorage,
32+
string indexId = SearchConstants.DefaultFtsIndexId)
2733
{
2834
this._nodeId = nodeId;
35+
this._indexId = indexId;
2936
this._ftsIndex = ftsIndex;
3037
this._contentStorage = contentStorage;
3138
}
@@ -74,7 +81,7 @@ public NodeSearchService(string nodeId, IFtsIndex ftsIndex, IContentStorage cont
7481
{
7582
RecordId = content.Id,
7683
NodeId = this._nodeId,
77-
IndexId = "fts-main", // TODO: Get from index config
84+
IndexId = this._indexId,
7885
ChunkId = null,
7986
BaseRelevance = (float)match.Score,
8087
Title = content.Title,

src/Core/Search/SearchConstants.cs

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -118,4 +118,10 @@ public static class SearchConstants
118118
/// Minimum relevance score.
119119
/// </summary>
120120
public const float MinRelevanceScore = 0.0f;
121+
122+
/// <summary>
123+
/// Default FTS index ID used when not specified in configuration.
124+
/// This is the identifier assigned to search results from the full-text search index.
125+
/// </summary>
126+
public const string DefaultFtsIndexId = "fts-main";
121127
}
Lines changed: 233 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,233 @@
1+
// Copyright (c) Microsoft. All rights reserved.
2+
3+
using KernelMemory.Core.Search;
4+
using KernelMemory.Core.Search.Models;
5+
using KernelMemory.Core.Search.Query.Ast;
6+
using KernelMemory.Core.Storage;
7+
using Microsoft.EntityFrameworkCore;
8+
using Microsoft.Extensions.Logging;
9+
using Moq;
10+
11+
namespace KernelMemory.Core.Tests.Search;
12+
13+
/// <summary>
14+
/// Unit tests for NodeSearchService index ID configuration.
15+
/// Tests that index ID is properly configurable and not hardcoded.
16+
/// Issue: Known Issue #4 - Index ID was hardcoded as "fts-main".
17+
/// </summary>
18+
public sealed class NodeSearchServiceIndexIdTests : IDisposable
19+
{
20+
private readonly string _tempDir;
21+
private readonly Mock<ILogger<SqliteFtsIndex>> _mockFtsLogger;
22+
private readonly Mock<ILogger<ContentStorageService>> _mockStorageLogger;
23+
private readonly List<ContentStorageDbContext> _contexts = [];
24+
private readonly List<SqliteFtsIndex> _ftsIndexes = [];
25+
26+
public NodeSearchServiceIndexIdTests()
27+
{
28+
this._tempDir = Path.Combine(Path.GetTempPath(), $"km-index-id-test-{Guid.NewGuid():N}");
29+
Directory.CreateDirectory(this._tempDir);
30+
this._mockFtsLogger = new Mock<ILogger<SqliteFtsIndex>>();
31+
this._mockStorageLogger = new Mock<ILogger<ContentStorageService>>();
32+
}
33+
34+
public void Dispose()
35+
{
36+
// Dispose all tracked resources
37+
foreach (var ftsIndex in this._ftsIndexes)
38+
{
39+
ftsIndex.Dispose();
40+
}
41+
42+
foreach (var context in this._contexts)
43+
{
44+
context.Dispose();
45+
}
46+
47+
try
48+
{
49+
if (Directory.Exists(this._tempDir))
50+
{
51+
Directory.Delete(this._tempDir, true);
52+
}
53+
}
54+
catch (IOException)
55+
{
56+
// Ignore cleanup errors
57+
}
58+
}
59+
60+
/// <summary>
61+
/// Tests that NodeSearchService uses the provided index ID in search results.
62+
/// The index ID should be configurable, not hardcoded.
63+
/// </summary>
64+
[Fact]
65+
public async Task SearchAsync_WithCustomIndexId_ResultsContainThatIndexId()
66+
{
67+
// Arrange
68+
const string customIndexId = "custom-fts-index";
69+
const string nodeId = "test-node";
70+
71+
var (ftsIndex, storage) = this.CreateIndexAndStorage("custom_index");
72+
73+
// Insert content
74+
await storage.UpsertAsync(new KernelMemory.Core.Storage.Models.UpsertRequest
75+
{
76+
Content = "Test content for custom index ID verification",
77+
MimeType = "text/plain"
78+
}, CancellationToken.None).ConfigureAwait(false);
79+
80+
// Create NodeSearchService with custom index ID
81+
var nodeService = new NodeSearchService(nodeId, ftsIndex, storage, customIndexId);
82+
83+
var request = new SearchRequest
84+
{
85+
Query = "content",
86+
Limit = 10,
87+
MinRelevance = 0.0f
88+
};
89+
90+
// Act - SearchIndexResult is returned by NodeSearchService.SearchAsync
91+
var queryNode = new TextSearchNode { SearchText = "content" };
92+
var (results, _) = await nodeService.SearchAsync(
93+
queryNode,
94+
request,
95+
CancellationToken.None).ConfigureAwait(false);
96+
97+
// Assert - SearchIndexResult has IndexId property
98+
Assert.NotEmpty(results);
99+
Assert.All(results, r => Assert.Equal(customIndexId, r.IndexId));
100+
}
101+
102+
/// <summary>
103+
/// Tests that the default index ID constant is used when not specified.
104+
/// Ensures backward compatibility.
105+
/// </summary>
106+
[Fact]
107+
public async Task SearchAsync_WithDefaultIndexId_UsesSearchConstantsDefault()
108+
{
109+
// Arrange
110+
const string nodeId = "test-node";
111+
112+
var (ftsIndex, storage) = this.CreateIndexAndStorage("default_index");
113+
114+
// Insert content
115+
await storage.UpsertAsync(new KernelMemory.Core.Storage.Models.UpsertRequest
116+
{
117+
Content = "Test content for default index ID verification",
118+
MimeType = "text/plain"
119+
}, CancellationToken.None).ConfigureAwait(false);
120+
121+
// Create NodeSearchService WITHOUT specifying index ID (uses default)
122+
var nodeService = new NodeSearchService(nodeId, ftsIndex, storage);
123+
124+
var request = new SearchRequest
125+
{
126+
Query = "content",
127+
Limit = 10,
128+
MinRelevance = 0.0f
129+
};
130+
131+
// Act
132+
var queryNode = new TextSearchNode { SearchText = "content" };
133+
var (results, _) = await nodeService.SearchAsync(
134+
queryNode,
135+
request,
136+
CancellationToken.None).ConfigureAwait(false);
137+
138+
// Assert
139+
Assert.NotEmpty(results);
140+
Assert.All(results, r => Assert.Equal(SearchConstants.DefaultFtsIndexId, r.IndexId));
141+
}
142+
143+
/// <summary>
144+
/// Tests that different nodes can have different index IDs.
145+
/// Validates multi-node, multi-index scenarios.
146+
/// </summary>
147+
[Fact]
148+
public async Task SearchAsync_MultipleNodesWithDifferentIndexIds_EachHasCorrectIndexId()
149+
{
150+
// Arrange
151+
const string node1Id = "node1";
152+
const string node1IndexId = "node1-fts-index";
153+
const string node2Id = "node2";
154+
const string node2IndexId = "node2-fts-index";
155+
156+
var (ftsIndex1, storage1) = this.CreateIndexAndStorage("node1_db");
157+
var (ftsIndex2, storage2) = this.CreateIndexAndStorage("node2_db");
158+
159+
// Insert content into both nodes
160+
await storage1.UpsertAsync(new KernelMemory.Core.Storage.Models.UpsertRequest
161+
{
162+
Content = "Content in first node with custom index",
163+
MimeType = "text/plain"
164+
}, CancellationToken.None).ConfigureAwait(false);
165+
166+
await storage2.UpsertAsync(new KernelMemory.Core.Storage.Models.UpsertRequest
167+
{
168+
Content = "Content in second node with different index",
169+
MimeType = "text/plain"
170+
}, CancellationToken.None).ConfigureAwait(false);
171+
172+
// Create NodeSearchServices with different index IDs
173+
var node1Service = new NodeSearchService(node1Id, ftsIndex1, storage1, node1IndexId);
174+
var node2Service = new NodeSearchService(node2Id, ftsIndex2, storage2, node2IndexId);
175+
176+
var queryNode = new TextSearchNode { SearchText = "content" };
177+
var request = new SearchRequest
178+
{
179+
Query = "content",
180+
Limit = 10,
181+
MinRelevance = 0.0f
182+
};
183+
184+
// Act - Test each node service directly
185+
var (results1, _) = await node1Service.SearchAsync(queryNode, request, CancellationToken.None).ConfigureAwait(false);
186+
var (results2, _) = await node2Service.SearchAsync(queryNode, request, CancellationToken.None).ConfigureAwait(false);
187+
188+
// Assert - Each node returns results with its own index ID
189+
Assert.NotEmpty(results1);
190+
Assert.NotEmpty(results2);
191+
Assert.All(results1, r => Assert.Equal(node1IndexId, r.IndexId));
192+
Assert.All(results2, r => Assert.Equal(node2IndexId, r.IndexId));
193+
}
194+
195+
/// <summary>
196+
/// Tests that SearchConstants.DefaultFtsIndexId constant has the expected value.
197+
/// Validates the constant is properly defined.
198+
/// </summary>
199+
[Fact]
200+
public void DefaultFtsIndexId_HasExpectedValue()
201+
{
202+
// Assert
203+
Assert.Equal("fts-main", SearchConstants.DefaultFtsIndexId);
204+
}
205+
206+
/// <summary>
207+
/// Helper method to create FTS index and storage for testing.
208+
/// Tracks created resources for proper disposal.
209+
/// </summary>
210+
/// <param name="dbPrefix">Prefix for database file names to ensure uniqueness.</param>
211+
/// <returns>Tuple of FTS index and storage service.</returns>
212+
private (SqliteFtsIndex ftsIndex, ContentStorageService storage) CreateIndexAndStorage(string dbPrefix)
213+
{
214+
var ftsDbPath = Path.Combine(this._tempDir, $"{dbPrefix}_fts.db");
215+
var contentDbPath = Path.Combine(this._tempDir, $"{dbPrefix}_content.db");
216+
217+
var ftsIndex = new SqliteFtsIndex(ftsDbPath, enableStemming: true, this._mockFtsLogger.Object);
218+
this._ftsIndexes.Add(ftsIndex);
219+
220+
var options = new DbContextOptionsBuilder<ContentStorageDbContext>()
221+
.UseSqlite($"Data Source={contentDbPath}")
222+
.Options;
223+
var context = new ContentStorageDbContext(options);
224+
this._contexts.Add(context);
225+
context.Database.EnsureCreated();
226+
227+
var cuidGenerator = new CuidGenerator();
228+
var searchIndexes = new Dictionary<string, ISearchIndex> { ["fts"] = ftsIndex };
229+
var storage = new ContentStorageService(context, cuidGenerator, this._mockStorageLogger.Object, searchIndexes);
230+
231+
return (ftsIndex, storage);
232+
}
233+
}

0 commit comments

Comments
 (0)