Skip to content
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.

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

Bugfix - Enumerating collection with documents expiring causes early return #422

Merged
merged 2 commits into from
Nov 27, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -364,6 +364,7 @@ We'd love your contributions! If you want to contribute please read our [Contrib
* [@imansafari1991](https://github.com/imansafari1991)
* [@AndersenGans](https://github.com/AndersenGans)
* [@mdrakib](https://github.com/mdrakib)
* [@jrpavoncello](https://github.com/jrpavoncello)

<!-- Logo -->
[Logo]: images/logo.svg
Expand Down
6 changes: 3 additions & 3 deletions src/Redis.OM/Redis.OM.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,9 @@
<RootNamespace>Redis.OM</RootNamespace>
<Nullable>enable</Nullable>
<AutoGenerateBindingRedirects>true</AutoGenerateBindingRedirects>
<PackageVersion>0.5.4</PackageVersion>
<Version>0.5.4</Version>
<PackageReleaseNotes>https://github.com/redis/redis-om-dotnet/releases/tag/v0.5.4</PackageReleaseNotes>
<PackageVersion>0.5.5</PackageVersion>
<Version>0.5.5</Version>
<PackageReleaseNotes>https://github.com/redis/redis-om-dotnet/releases/tag/v0.5.5</PackageReleaseNotes>
<Description>Object Mapping and More for Redis</Description>
<Title>Redis OM</Title>
<Authors>Steve Lorello</Authors>
Expand Down
4 changes: 2 additions & 2 deletions src/Redis.OM/Searching/RedisCollectionEnumerator.cs
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ public bool MoveNext()
switch (_started)
{
case true when _limited:
case true when _records.Documents.Count < _query!.Limit!.Number:
case true when _records.Documents.Count < _query!.Limit!.Number && _records.DocumentsSkippedCount == 0:
return false;
default:
return GetNextChunk();
Expand All @@ -113,7 +113,7 @@ public async ValueTask<bool> MoveNextAsync()
switch (_started)
{
case true when _limited:
case true when _records.Documents.Count < _query!.Limit!.Number:
case true when _records.Documents.Count < _query!.Limit!.Number && _records.DocumentsSkippedCount == 0:
return false;
default:
return await GetNextChunkAsync();
Expand Down
11 changes: 11 additions & 0 deletions src/Redis.OM/Searching/SearchResponse.cs
Original file line number Diff line number Diff line change
Expand Up @@ -124,13 +124,18 @@ public SearchResponse(RedisReply val)
var obj = RedisObjectHandler.FromHashSet<T>(documentHash);
Documents.Add(docId, obj);
}
else
{
DocumentsSkippedCount++; // needed when a key expired while it was being enumerated by Redis.
}
}
}
}

private SearchResponse()
{
DocumentCount = 0;
DocumentsSkippedCount = 0;
Documents = new Dictionary<string, T>();
}

Expand All @@ -139,6 +144,12 @@ private SearchResponse()
/// </summary>
public long DocumentCount { get; set; }

/// <summary>
/// Gets the number of documents skipped while enumerating the search result set.
/// This can be indicative of documents that have expired during enumeration.
/// </summary>
public int DocumentsSkippedCount { get; private set; }

/// <summary>
/// Gets the documents.
/// </summary>
Expand Down
175 changes: 175 additions & 0 deletions test/Redis.OM.Unit.Tests/RediSearchTests/SearchTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3474,5 +3474,180 @@ public void TestConstantExpressionContains()
_ = collection.Where(lambada).ToList();
_substitute.Received().Execute("FT.SEARCH", "person-idx", "(@TagField:{James|Bond})", "LIMIT", "0", "100");
}

[Fact]
public async Task EnumerateAllWhenKeyExpires()
{
RedisReply firstReply = new RedisReply[]
{
new(2),
new("Redis.OM.Unit.Tests.RediSearchTests.Person:E912BED67BD64386B4FDC7322D"),
new(new RedisReply[]
{
"$",
"{\"Name\":\"Steve\",\"Age\":32,\"Height\":71.0, \"Id\":\"E912BED67BD64386B4FDC7322D\"}"
}),
new("Redis.OM.Unit.Tests.RediSearchTests.Person:01FVN836BNQGYMT80V7RCVY73N"),
// Key expired while executing the search
new(Array.Empty<RedisReply>())
};
RedisReply secondReply = new RedisReply[]
{
new(2),
new("Redis.OM.Unit.Tests.RediSearchTests.Person:4F6AE0A9BAE044E4B2D2186044"),
new(new RedisReply[]
{
"$",
"{\"Name\":\"Josh\",\"Age\":30,\"Height\":12.0, \"Id\":\"4F6AE0A9BAE044E4B2D2186044\"}"
})
};
RedisReply finalEmptyResult = new RedisReply[]
{
new(0),
};

_substitute.ClearSubstitute();
_substitute.ExecuteAsync(
"FT.SEARCH",
"person-idx",
"*",
"LIMIT",
"0",
"2").Returns(firstReply);
_substitute.ExecuteAsync(
"FT.SEARCH",
"person-idx",
"*",
"LIMIT",
"2",
"2").Returns(secondReply);
_substitute.ExecuteAsync(
"FT.SEARCH",
"person-idx",
"*",
"LIMIT",
"4",
"2").Returns(finalEmptyResult);

var people = new List<Person>();
// Chunk size 2 induces the iterator to call FT.SEARCH 3 times
await foreach (var person in new RedisCollection<Person>(_substitute, 2))
{
people.Add(person);
}

Assert.Equal(2, people.Count);

Assert.Equal("Steve", people[0].Name);
Assert.Equal("Josh", people[1].Name);
}

[Fact]
public async Task EnumerateAllWhenKeyExpiresAtEnd()
{
RedisReply firstReply = new RedisReply[]
{
new(2),
new("Redis.OM.Unit.Tests.RediSearchTests.Person:E912BED67BD64386B4FDC7322D"),
new(new RedisReply[]
{
"$",
"{\"Name\":\"Steve\",\"Age\":32,\"Height\":71.0, \"Id\":\"E912BED67BD64386B4FDC7322D\"}"
}),
new("Redis.OM.Unit.Tests.RediSearchTests.Person:4F6AE0A9BAE044E4B2D2186044"),
new(new RedisReply[]
{
"$",
"{\"Name\":\"Josh\",\"Age\":30,\"Height\":12.0, \"Id\":\"4F6AE0A9BAE044E4B2D2186044\"}"
})
};
RedisReply secondReply = new RedisReply[]
{
new(1),
new("Redis.OM.Unit.Tests.RediSearchTests.Person:01FVN836BNQGYMT80V7RCVY73N"),
// Key expired while executing the search
new(Array.Empty<RedisReply>())
};
RedisReply finalEmptyResult = new RedisReply[]
{
new(0),
};

_substitute.ClearSubstitute();
_substitute.ExecuteAsync(
"FT.SEARCH",
"person-idx",
"*",
"LIMIT",
"0",
"2").Returns(firstReply);
_substitute.ExecuteAsync(
"FT.SEARCH",
"person-idx",
"*",
"LIMIT",
"2",
"2").Returns(secondReply);
_substitute.ExecuteAsync(
"FT.SEARCH",
"person-idx",
"*",
"LIMIT",
"4",
"2").Returns(finalEmptyResult);

var people = new List<Person>();
// Chunk size 2 induces the iterator to call FT.SEARCH 3 times
await foreach (var person in new RedisCollection<Person>(_substitute, 2))
{
people.Add(person);
}

Assert.Equal(2, people.Count);

Assert.Equal("Steve", people[0].Name);
Assert.Equal("Josh", people[1].Name);
}

[Fact]
public async Task EnumerateAllButAllExpired()
{
RedisReply firstReply = new RedisReply[]
{
new(1),
new("Redis.OM.Unit.Tests.RediSearchTests.Person:01FVN836BNQGYMT80V7RCVY73N"),
// Key expired while executing the search
new(Array.Empty<RedisReply>())
};
RedisReply finalEmptyResult = new RedisReply[]
{
new(0),
};

_substitute.ClearSubstitute();
_substitute.ExecuteAsync(
"FT.SEARCH",
"person-idx",
"*",
"LIMIT",
"0",
"2").Returns(firstReply);
_substitute.ExecuteAsync(
"FT.SEARCH",
"person-idx",
"*",
"LIMIT",
"4",
"2").Returns(finalEmptyResult);

var people = new List<Person>();
// Chunk size 2 induces the iterator to call FT.SEARCH twice
await foreach (var person in new RedisCollection<Person>(_substitute, 2))
{
people.Add(person);
}

Assert.Empty(people);
}
}
}
Loading