Skip to content

defensive code when parsing FT.SEARCH results #463

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
mgravell wants to merge 4 commits into
base: master
Choose a base branch
from
+106 −65
Open

defensive code when parsing FT.SEARCH results #463

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
957da18
defensive code in parsing FT.SEARCH results:
mgravell Feb 5, 2026
96461b2
validate query results in async path
mgravell Feb 5, 2026
0a35af1
skip test validation of score calculation, due to v7 vs v8
mgravell Feb 5, 2026
618b0a1
Merge branch 'master' into marc/search-args-check
mgravell Feb 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
100 changes: 48 additions & 52 deletions src/NRedisStack/Search/SearchResult.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using StackExchange.Redis;
using System.Diagnostics;
using StackExchange.Redis;

namespace NRedisStack.Search;

Expand All @@ -22,64 +23,59 @@ internal SearchResult(RedisResult[] resp, bool hasContent, bool hasScores, bool
{
// Calculate the step distance to walk over the results.
// The order of results is id, score (if withScore), payLoad (if hasPayloads), fields
int step = 1;
int scoreOffset = 0;
int contentOffset = 1;
int payloadOffset = 0;
if (hasScores)
{
step++;
scoreOffset = 1;
contentOffset++;
int stride = 1;
if (hasScores) stride++;
if (hasPayloads) stride++;
if (hasContent) stride++;

}
if (hasContent)
// the first element is always the number of results
if (resp is not { Length: > 0 })
{
step++;
if (hasPayloads)
{
payloadOffset = scoreOffset + 1;
step++;
contentOffset++;
}
// unexpected empty case
TotalResults = 0;
Documents = [];
Debug.Assert(false, "Empty result from FT.SEARCH"); // debug only, flag as a problem
}

// the first element is always the number of results
TotalResults = (long)resp[0];
var docs = new List<Document>((resp.Length - 1) / step);
Documents = docs;
for (int i = 1; i < resp.Length; i += step)
else
{
var id = resp[i].ToString();
double score = 1.0;
byte[]? payload = null;
RedisValue[]? fields = null;
string[]? scoreExplained = null;
if (hasScores)
TotalResults = (long)resp[0];
int count = checked((int)(resp.Length - 1) / stride);
var docs = Documents = new List<Document>(count);
int offset = 1; // skip the first element which is the number of results
for (int docIndex = 0; docIndex < count; docIndex++)
{
// if (shouldExplainScore)
// {
// var scoreResult = (RedisResult[])resp[i + scoreOffset];
// score = (double) scoreResult[0];
// var redisResultsScoreExplained = (RedisResult[]) scoreResult[1];
// scoreExplained = FlatRedisResultArray(redisResultsScoreExplained).ToArray();
// }
//else
//{
score = (double)resp[i + scoreOffset];
//}
}
if (hasPayloads)
{
payload = (byte[]?)resp[i + payloadOffset];
}
var id = resp[offset++].ToString();
double score = 1.0;
byte[]? payload = null;
RedisValue[]? fields = null;
string[]? scoreExplained = null;
if (hasScores)
{
// if (shouldExplainScore)
// {
// var scoreResult = (RedisResult[])resp[offset++];
// score = (double) scoreResult[0];
// var redisResultsScoreExplained = (RedisResult[]) scoreResult[1];
// scoreExplained = FlatRedisResultArray(redisResultsScoreExplained).ToArray();
// }
//else
//{
score = (double)resp[offset++];
//}
}

if (hasContent)
{
fields = (RedisValue[]?)resp[i + contentOffset];
}
if (hasPayloads) // match logic from setup
{
payload = (byte[]?)resp[offset++];
}

if (hasContent)
{
fields = (RedisValue[]?)resp[offset++];
}

docs.Add(Document.Load(id, score, payload, fields, scoreExplained));
docs.Add(Document.Load(id, score, payload, fields, scoreExplained));
}
}
}
}
71 changes: 58 additions & 13 deletions tests/NRedisStack.Tests/Search/SearchTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2732,54 +2732,99 @@
}

[SkippableTheory]
[MemberData(nameof(EndpointsFixture.Env.AllEnvironments), MemberType = typeof(EndpointsFixture.Env))]
public async Task TestQueryAddParam_DefaultDialectAsync(string endpointId)
[MemberData(nameof(TestQueryParamsWithParams_Args))]
public async Task TestQueryAddParam_DefaultDialectAsync(string endpointId, bool withScores, bool withPayloads, bool withContent)
{
SkipClusterPre8(endpointId);
IDatabase db = GetCleanDatabase(endpointId);
var ft = db.FT(2);

var sc = new Schema().AddNumericField("numval");
Assert.True(await ft.CreateAsync("idx", new(), sc));
var sc = new Schema().AddNumericField("numval").AddTextField("sval");
Assert.True(await ft.CreateAsync("idx", new FTCreateParams().PayloadField("sval"), sc));

db.HashSet("1", "numval", 1);
db.HashSet("2", "numval", 2);
db.HashSet("3", "numval", 3);
await db.HashSetAsync("1", [new("numval", 1), new("sval", "a")]);
await db.HashSetAsync("2", [new("numval", 2), new("sval", "b")]);
await db.HashSetAsync("3", [new("numval", 3), new("sval", "c")]);

await AssertIndexSizeAsync(ft, "idx", 3);
Query query = new Query("@numval:[$min $max]").AddParam("min", 1).AddParam("max", 2);
ConfigureQuery(withScores, withPayloads, withContent, query);
var res = await ft.SearchAsync("idx", query);
Assert.Equal(2, res.TotalResults);
CheckQueryResults(res, withScores, withPayloads, withContent);
}

public static IEnumerable<object[]> TestQueryParamsWithParams_Args()
{
// for each environment, test each permutation of scores, payloads and content
foreach (var outer in EndpointsFixture.Env.AllEnvironments())
{
var env = outer.Single();
for (int i = 0; i < 8; i++)
{
yield return [env, (i & 4) != 0, (i & 2) != 0, (i & 1) != 0];
}
}
}

[SkippableTheory]
[MemberData(nameof(EndpointsFixture.Env.AllEnvironments), MemberType = typeof(EndpointsFixture.Env))]
public void TestQueryParamsWithParams_DefaultDialect(string endpointId)
[MemberData(nameof(TestQueryParamsWithParams_Args))]
public void TestQueryParamsWithParams_DefaultDialect(string endpointId, bool withScores, bool withPayloads, bool withContent)
{
SkipClusterPre8(endpointId);
IDatabase db = GetCleanDatabase(endpointId);
var ft = db.FT(2);

var sc = new Schema().AddNumericField("numval");
Assert.True(ft.Create("idx", new(), sc));
Assert.True(ft.Create("idx", new FTCreateParams().PayloadField("numval"), sc));

db.HashSet("1", "numval", 1);
db.HashSet("2", "numval", 2);
db.HashSet("3", "numval", 3);
db.HashSet("1", [new("numval", 1), new("sval", "a")]);
db.HashSet("2", [new("numval", 2), new("sval", "b")]);
db.HashSet("3", [new("numval", 3), new("sval", "c")]);

AssertIndexSize(ft, "idx", 3);
Query query = new Query("@numval:[$min $max]").AddParam("min", 1).AddParam("max", 2);
ConfigureQuery(withScores, withPayloads, withContent, query);

var res = ft.Search("idx", query);
Assert.Equal(2, res.TotalResults);
CheckQueryResults(res, withScores, withPayloads, withContent);

var paramValue = new Dictionary<string, object>()
{
["min"] = 1,
["max"] = 2
};
query = new("@numval:[$min $max]");
ConfigureQuery(withScores, withPayloads, withContent, query);
res = ft.Search("idx", query.Params(paramValue));
Assert.Equal(2, res.TotalResults);
CheckQueryResults(res, withScores, withPayloads, withContent);
}

private static void ConfigureQuery(bool withScores, bool withPayloads, bool withContent, Query query)
{
if (withScores) query.SetWithScores();
if (withPayloads) query.SetWithPayloads();
if (!withContent) query.SetNoContent();
}

private void CheckQueryResults(SearchResult result, bool withScores, bool withPayloads, bool withContent)
{
Assert.NotNull(result);
Assert.NotNull(result.Documents);
Assert.Equal(result.TotalResults, result.Documents.Count);
foreach (var doc in result.Documents)
{
_ = withScores;
// (we can't validate scores properly, due to calculation differences between v7 and v8)

if (withPayloads) Assert.NotNull(doc.Payload);
else Assert.Null(doc.Payload);

if (withContent) Assert.NotEmpty(doc._properties);
else Assert.Empty(doc._properties);
}
}

[SkippableTheory]
Expand Down Expand Up @@ -3940,7 +3985,7 @@
await Task.WhenAny(checkTask, Task.Delay(1000));
var keyTtl = db.KeyTimeToLive("student:22222");
Assert.Equal(0, keyTtl.HasValue ? keyTtl.Value.Milliseconds : 0);
Assert.Equal(3, completed);

Check failure on line 3988 in tests/NRedisStack.Tests/Search/SearchTests.cs

View workflow job for this annotation

GitHub Actions / Redis 7.2; .NET 8.0;

NRedisStack.Tests.Search.SearchTests.TestDocumentLoadWithDB_Issue352(endpointId: "standalone") 

Assert.Equal() Failure: Values differ
Expected: 3
Actual:   1
} while (droppedDocument == null && numberOfAttempts++ < 5);
// we won't do an actual assert here since
// it is not guaranteed that window stays open wide enough to catch it.
Expand Down
Loading