RUBY-2961 Treat 0 limit as no limit and negative limit as positive limit in query caching (#2452)

Author: mikebaldry

lib/mongo/collection/view/iterable.rb

@@ -70,7 +70,7 @@ def each
             # If a query with a limit is performed, the query cache will
             # re-use results from an earlier query with the same or larger
             # limit, and then impose the lower limit during iteration.
-            limit_for_cached_query = respond_to?(:limit) ? limit : nil
+            limit_for_cached_query = respond_to?(:limit) ? QueryCache.normalized_limit(limit) : nil
           end
 
           if block_given?

lib/mongo/query_cache.rb

@@ -179,7 +179,8 @@ def set(cursor, **opts)
       #
       # @api private
       def get(**opts)
-        limit = opts[:limit]
+        limit = normalized_limit(opts[:limit])
+
         _namespace_key = namespace_key(**opts)
         _cache_key = cache_key(**opts)
 
@@ -189,7 +190,7 @@ def get(**opts)
         caching_cursor = namespace_hash[_cache_key]
         return nil unless caching_cursor
 
-        caching_cursor_limit = caching_cursor.view.limit
+        caching_cursor_limit = normalized_limit(caching_cursor.view.limit)
 
         # There are two scenarios in which a caching cursor could fulfill the
         # query:
@@ -199,6 +200,7 @@ def get(**opts)
         #
         # Otherwise, return nil because the stored cursor will not satisfy
         # the query.
+
         if limit && (caching_cursor_limit.nil? || caching_cursor_limit >= limit)
           caching_cursor
         elsif limit.nil? && caching_cursor_limit.nil?
@@ -208,6 +210,14 @@ def get(**opts)
         end
       end
 
+      def normalized_limit(limit)
+        return nil unless limit
+        # For the purposes of caching, a limit of 0 means no limit, as mongo treats it as such.
+        return nil if limit == 0
+        # For the purposes of caching, a negative limit is the same as as a positive limit.
+        limit.abs
+      end
+
       private
 
       def cache_key(**opts)

spec/integration/query_cache_spec.rb

@@ -345,18 +345,69 @@
 
         it 'uses the cache' do
           results_limit_5 = authorized_collection.find.limit(5).to_a
+          results_limit_negative_5 = authorized_collection.find.limit(-5).to_a
           results_limit_3 = authorized_collection.find.limit(3).to_a
+          results_limit_negative_3 = authorized_collection.find.limit(-3).to_a
           results_no_limit = authorized_collection.find.to_a
+          results_limit_0 = authorized_collection.find.limit(0).to_a
+
+
+          expect(results_limit_5.length).to eq(5)
+          expect(results_limit_5.map { |r| r["test"] }).to eq([0, 1, 2, 3, 4])
+
+          expect(results_limit_negative_5.length).to eq(5)
+          expect(results_limit_negative_5.map { |r| r["test"] }).to eq([0, 1, 2, 3, 4])
+
+          expect(results_limit_3.length).to eq(3)
+          expect(results_limit_3.map { |r| r["test"] }).to eq([0, 1, 2])
+
+          expect(results_limit_negative_3.length).to eq(3)
+          expect(results_limit_negative_3.map { |r| r["test"] }).to eq([0, 1, 2])
+
+          expect(results_no_limit.length).to eq(10)
+          expect(results_no_limit.map { |r| r["test"] }).to eq([0, 1, 2, 3, 4, 5, 6, 7, 8, 9])
+
+          expect(results_limit_0.length).to eq(10)
+          expect(results_limit_0.map { |r| r["test"] }).to eq([0, 1, 2, 3, 4, 5, 6, 7, 8, 9])
+
+          expect(events.length).to eq(1)
+        end
+      end
+
+      context 'when the first query has a 0 limit' do
+        before do
+          authorized_collection.find.limit(0).to_a
+        end
+
+        it 'uses the cache' do
+          results_limit_5 = authorized_collection.find.limit(5).to_a
+          results_limit_negative_5 = authorized_collection.find.limit(-5).to_a
+          results_limit_3 = authorized_collection.find.limit(3).to_a
+          results_limit_negative_3 = authorized_collection.find.limit(-3).to_a
+          results_no_limit = authorized_collection.find.to_a
+          results_limit_0 = authorized_collection.find.limit(0).to_a
 
           expect(results_limit_5.length).to eq(5)
           expect(results_limit_5.map { |r| r["test"] }).to eq([0, 1, 2, 3, 4])
 
+          expect(results_limit_negative_5.length).to eq(5)
+          expect(results_limit_negative_5.map { |r| r["test"] }).to eq([0, 1, 2, 3, 4])
+
+
           expect(results_limit_3.length).to eq(3)
           expect(results_limit_3.map { |r| r["test"] }).to eq([0, 1, 2])
 
+          expect(results_limit_negative_3.length).to eq(3)
+          expect(results_limit_negative_3.map { |r| r["test"] }).to eq([0, 1, 2])
+
+
           expect(results_no_limit.length).to eq(10)
           expect(results_no_limit.map { |r| r["test"] }).to eq([0, 1, 2, 3, 4, 5, 6, 7, 8, 9])
 
+
+          expect(results_limit_0.length).to eq(10)
+          expect(results_limit_0.map { |r| r["test"] }).to eq([0, 1, 2, 3, 4, 5, 6, 7, 8, 9])
+
           expect(events.length).to eq(1)
         end
       end
@@ -391,6 +442,21 @@
           end
         end
 
+        context 'and two queries are performed with a larger negative limit' do
+          it 'uses the query cache for the third query' do
+            results1 = authorized_collection.find.limit(-3).to_a
+            results2 = authorized_collection.find.limit(-3).to_a
+
+            expect(results1.length).to eq(3)
+            expect(results1.map { |r| r["test"] }).to eq([0, 1, 2])
+
+            expect(results2.length).to eq(3)
+            expect(results2.map { |r| r["test"] }).to eq([0, 1, 2])
+
+            expect(events.length).to eq(2)
+          end
+        end
+
         context 'and the second query has a smaller limit' do
           let(:results) { authorized_collection.find.limit(1).to_a }
 
@@ -401,6 +467,99 @@
           end
         end
 
+        context 'and the second query has a smaller negative limit' do
+          let(:results) { authorized_collection.find.limit(-1).to_a }
+
+          it 'uses the cached query' do
+            expect(results.count).to eq(1)
+            expect(results.first["test"]).to eq(0)
+            expect(events.length).to eq(1)
+          end
+        end
+
+        context 'and the second query has no limit' do
+          it 'queries again' do
+            expect(authorized_collection.find.to_a.count).to eq(10)
+            expect(events.length).to eq(2)
+          end
+        end
+      end
+
+      context 'when the first query has a negative limit' do
+        before do
+          authorized_collection.find.limit(-2).to_a
+        end
+
+        context 'and the second query has a larger limit' do
+          let(:results) { authorized_collection.find.limit(3).to_a }
+
+          it 'queries again' do
+            expect(results.length).to eq(3)
+            expect(results.map { |result| result["test"] }).to eq([0, 1, 2])
+            expect(events.length).to eq(2)
+          end
+        end
+
+        context 'and the second query has a larger negative limit' do
+          let(:results) { authorized_collection.find.limit(-3).to_a }
+
+          it 'queries again' do
+            expect(results.length).to eq(3)
+            expect(results.map { |result| result["test"] }).to eq([0, 1, 2])
+            expect(events.length).to eq(2)
+          end
+        end
+
+        context 'and two queries are performed with a larger limit' do
+          it 'uses the query cache for the third query' do
+            results1 = authorized_collection.find.limit(3).to_a
+            results2 = authorized_collection.find.limit(3).to_a
+
+            expect(results1.length).to eq(3)
+            expect(results1.map { |r| r["test"] }).to eq([0, 1, 2])
+
+            expect(results2.length).to eq(3)
+            expect(results2.map { |r| r["test"] }).to eq([0, 1, 2])
+
+            expect(events.length).to eq(2)
+          end
+        end
+
+        context 'and two queries are performed with a larger negative limit' do
+          it 'uses the query cache for the third query' do
+            results1 = authorized_collection.find.limit(-3).to_a
+            results2 = authorized_collection.find.limit(-3).to_a
+
+            expect(results1.length).to eq(3)
+            expect(results1.map { |r| r["test"] }).to eq([0, 1, 2])
+
+            expect(results2.length).to eq(3)
+            expect(results2.map { |r| r["test"] }).to eq([0, 1, 2])
+
+            expect(events.length).to eq(2)
+          end
+        end
+
+        context 'and the second query has a smaller limit' do
+          let(:results) { authorized_collection.find.limit(1).to_a }
+
+          it 'uses the cached query' do
+            expect(results.count).to eq(1)
+            expect(results.first["test"]).to eq(0)
+            expect(events.length).to eq(1)
+          end
+        end
+
+        context 'and the second query has a smaller negative limit' do
+          let(:results) { authorized_collection.find.limit(-1).to_a }
+
+          it 'uses the cached query' do
+            expect(results.count).to eq(1)
+            expect(results.first["test"]).to eq(0)
+            expect(events.length).to eq(1)
+          end
+        end
+
         context 'and the second query has no limit' do
           it 'queries again' do
             expect(authorized_collection.find.to_a.count).to eq(10)