Bugfix - Enumerating collection with documents expiring causes early …

…return (#422) Fix bug where enumerating a collection while documents are actively expiring can result in an early return with less documents than expected in the result
redis · Nov 27, 2023 · c87f2da · c87f2da
1 parent fa80a84
commit c87f2da
Show file tree

Hide file tree

Showing 5 changed files with 192 additions and 5 deletions.
diff --git a/README.md b/README.md
@@ -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

diff --git a/src/Redis.OM/Redis.OM.csproj b/src/Redis.OM/Redis.OM.csproj
@@ -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>

diff --git a/src/Redis.OM/Searching/RedisCollectionEnumerator.cs b/src/Redis.OM/Searching/RedisCollectionEnumerator.cs
@@ -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();
@@ -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();

diff --git a/src/Redis.OM/Searching/SearchResponse.cs b/src/Redis.OM/Searching/SearchResponse.cs
@@ -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>();
         }
 
@@ -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>

diff --git a/test/Redis.OM.Unit.Tests/RediSearchTests/SearchTests.cs b/test/Redis.OM.Unit.Tests/RediSearchTests/SearchTests.cs
@@ -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);
+        }
     }
 }