diff --git a/lib/mongo/collection/view/iterable.rb b/lib/mongo/collection/view/iterable.rb index cb19e39c4f..38a85fe906 100644 --- a/lib/mongo/collection/view/iterable.rb +++ b/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? diff --git a/lib/mongo/query_cache.rb b/lib/mongo/query_cache.rb index dbfe2ac0f5..22ba0b3fa3 100644 --- a/lib/mongo/query_cache.rb +++ b/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) diff --git a/spec/integration/query_cache_spec.rb b/spec/integration/query_cache_spec.rb index 7552e1fcf5..923445eb1c 100644 --- a/spec/integration/query_cache_spec.rb +++ b/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) diff --git a/spec/mongo/query_cache_spec.rb b/spec/mongo/query_cache_spec.rb index 1455449ae4..a0f8f033e7 100644 --- a/spec/mongo/query_cache_spec.rb +++ b/spec/mongo/query_cache_spec.rb @@ -192,6 +192,14 @@ end end + context 'when the query has a limit but negative' do + let(:limit) { -5 } + + it 'returns the caching cursor' do + expect(Mongo::QueryCache.get(**query_options)).to eq(caching_cursor) + end + end + context 'when the query has no limit' do let(:limit) { nil } @@ -199,6 +207,65 @@ expect(Mongo::QueryCache.get(**query_options)).to eq(caching_cursor) end end + + context 'when the query has a 0 limit' do + let(:limit) { 0 } + + it 'returns the caching cursor' do + expect(Mongo::QueryCache.get(**query_options)).to eq(caching_cursor) + end + end + end + + context 'when that entry has a 0 limit' do + let(:caching_cursor_options) do + { + namespace: 'db.coll', + selector: { field: 'value' }, + limit: 0, + } + end + + let(:query_options) do + caching_cursor_options.merge(limit: limit) + end + + before do + allow(view).to receive(:limit) { 0 } + end + + context 'when the query has a limit' do + let(:limit) { 5 } + + it 'returns the caching cursor' do + expect(Mongo::QueryCache.get(**query_options)).to eq(caching_cursor) + end + end + + context 'when the query has a limit but negative' do + let(:limit) { -5 } + + it 'returns the caching cursor' do + expect(Mongo::QueryCache.get(**query_options)).to eq(caching_cursor) + end + end + + + context 'when the query has no limit' do + let(:limit) { nil } + + it 'returns the caching cursor' do + expect(Mongo::QueryCache.get(**query_options)).to eq(caching_cursor) + end + end + + context 'when the query has a 0 limit' do + let(:limit) { 0 } + + it 'returns the caching cursor' do + expect(Mongo::QueryCache.get(**query_options)).to eq(caching_cursor) + end + end end context 'when that entry has a limit' do @@ -226,6 +293,14 @@ end end + context 'and the new query has a smaller limit but negative' do + let(:limit) { -4 } + + it 'returns the caching cursor' do + expect(Mongo::QueryCache.get(**query_options)).to eq(caching_cursor) + end + end + context 'and the new query has a larger limit' do let(:limit) { 6 } @@ -234,6 +309,14 @@ end end + context 'and the new query has a larger limit but negative' do + let(:limit) { -6 } + + it 'returns nil' do + expect(Mongo::QueryCache.get(**query_options)).to be_nil + end + end + context 'and the new query has the same limit' do let(:limit) { 5 } @@ -242,6 +325,80 @@ end end + context 'and the new query has the same limit but negative' do + let(:limit) { -5 } + + it 'returns the caching cursor' do + expect(Mongo::QueryCache.get(**query_options)).to eq(caching_cursor) + end + end + + context 'and the new query has no limit' do + let(:limit) { nil } + + it 'returns nil' do + expect(Mongo::QueryCache.get(**query_options)).to be_nil + end + end + + context 'and the new query has a 0 limit' do + let(:limit) { 0 } + + it 'returns nil' do + expect(Mongo::QueryCache.get(**query_options)).to be_nil + end + end + end + + context 'when that entry has a negative limit' do + let(:caching_cursor_options) do + { + namespace: 'db.coll', + selector: { field: 'value' }, + limit: -5, + } + end + + let(:query_options) do + caching_cursor_options.merge(limit: limit) + end + + before do + allow(view).to receive(:limit) { -5 } + end + + context 'and the new query has a smaller limit' do + let(:limit) { 4 } + + it 'returns the caching cursor' do + expect(Mongo::QueryCache.get(**query_options)).to eq(caching_cursor) + end + end + + context 'and the new query has a larger limit' do + let(:limit) { 6 } + + it 'returns nil' do + expect(Mongo::QueryCache.get(**query_options)).to be_nil + end + end + + context 'and the new query has the same negative limit' do + let(:limit) { -5 } + + it 'returns the caching cursor' do + expect(Mongo::QueryCache.get(**query_options)).to eq(caching_cursor) + end + end + + context 'and the new query has the same positive limit' do + let(:limit) { 5 } + + it 'returns the caching cursor' do + expect(Mongo::QueryCache.get(**query_options)).to eq(caching_cursor) + end + end + context 'and the new query has no limit' do let(:limit) { nil } @@ -249,6 +406,14 @@ expect(Mongo::QueryCache.get(**query_options)).to be_nil end end + + context 'and the new query has a 0 limit' do + let(:limit) { 0 } + + it 'returns nil' do + expect(Mongo::QueryCache.get(**query_options)).to be_nil + end + end end end end