From 206972ee022618eb4a5739ad86d571a7e5c14e55 Mon Sep 17 00:00:00 2001 From: Michael Baldry Date: Thu, 14 Apr 2022 11:27:37 +0100 Subject: [PATCH 1/2] Treat 0 limit as no limit in query caching --- lib/mongo/collection/view/iterable.rb | 3 ++ lib/mongo/query_cache.rb | 6 +++ spec/integration/query_cache_spec.rb | 31 ++++++++++++++ spec/mongo/query_cache_spec.rb | 58 +++++++++++++++++++++++++++ 4 files changed, 98 insertions(+) diff --git a/lib/mongo/collection/view/iterable.rb b/lib/mongo/collection/view/iterable.rb index cb19e39c4f..35fe722e6b 100644 --- a/lib/mongo/collection/view/iterable.rb +++ b/lib/mongo/collection/view/iterable.rb @@ -71,6 +71,9 @@ def each # 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 + + # For the purposes of caching, a limit of 0 means no limit, as mongo treats it as such. + limit_for_cached_query = nil if limit_for_cached_query == 0 end if block_given? diff --git a/lib/mongo/query_cache.rb b/lib/mongo/query_cache.rb index dbfe2ac0f5..8f031b8c5c 100644 --- a/lib/mongo/query_cache.rb +++ b/lib/mongo/query_cache.rb @@ -180,6 +180,9 @@ def set(cursor, **opts) # @api private def get(**opts) limit = opts[:limit] + # For the purposes of caching, a limit of 0 means no limit, as mongo treats it as such. + limit = nil if limit == 0 + _namespace_key = namespace_key(**opts) _cache_key = cache_key(**opts) @@ -190,6 +193,8 @@ def get(**opts) return nil unless caching_cursor caching_cursor_limit = caching_cursor.view.limit + # For the purposes of caching, a limit of 0 means no limit, as mongo treats it as such. + caching_cursor_limit = nil if caching_cursor_limit == 0 # There are two scenarios in which a caching cursor could fulfill the # query: @@ -199,6 +204,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? diff --git a/spec/integration/query_cache_spec.rb b/spec/integration/query_cache_spec.rb index 7552e1fcf5..6ddb12e507 100644 --- a/spec/integration/query_cache_spec.rb +++ b/spec/integration/query_cache_spec.rb @@ -347,6 +347,7 @@ results_limit_5 = authorized_collection.find.limit(5).to_a results_limit_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]) @@ -357,6 +358,36 @@ 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_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_3.length).to eq(3) + expect(results_limit_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 diff --git a/spec/mongo/query_cache_spec.rb b/spec/mongo/query_cache_spec.rb index 1455449ae4..738c1f238a 100644 --- a/spec/mongo/query_cache_spec.rb +++ b/spec/mongo/query_cache_spec.rb @@ -199,6 +199,56 @@ 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 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 @@ -249,6 +299,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 From e5dce9a9b2215c16e84554c3b9dacda1fa525da1 Mon Sep 17 00:00:00 2001 From: Michael Baldry Date: Thu, 14 Apr 2022 14:20:05 +0100 Subject: [PATCH 2/2] Treat negative limit as a positive limit in query caching --- lib/mongo/collection/view/iterable.rb | 5 +- lib/mongo/query_cache.rb | 16 ++-- spec/integration/query_cache_spec.rb | 128 ++++++++++++++++++++++++++ spec/mongo/query_cache_spec.rb | 107 +++++++++++++++++++++ 4 files changed, 246 insertions(+), 10 deletions(-) diff --git a/lib/mongo/collection/view/iterable.rb b/lib/mongo/collection/view/iterable.rb index 35fe722e6b..38a85fe906 100644 --- a/lib/mongo/collection/view/iterable.rb +++ b/lib/mongo/collection/view/iterable.rb @@ -70,10 +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 - - # For the purposes of caching, a limit of 0 means no limit, as mongo treats it as such. - limit_for_cached_query = nil if limit_for_cached_query == 0 + 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 8f031b8c5c..22ba0b3fa3 100644 --- a/lib/mongo/query_cache.rb +++ b/lib/mongo/query_cache.rb @@ -179,9 +179,7 @@ def set(cursor, **opts) # # @api private def get(**opts) - limit = opts[:limit] - # For the purposes of caching, a limit of 0 means no limit, as mongo treats it as such. - limit = nil if limit == 0 + limit = normalized_limit(opts[:limit]) _namespace_key = namespace_key(**opts) _cache_key = cache_key(**opts) @@ -192,9 +190,7 @@ def get(**opts) caching_cursor = namespace_hash[_cache_key] return nil unless caching_cursor - caching_cursor_limit = caching_cursor.view.limit - # For the purposes of caching, a limit of 0 means no limit, as mongo treats it as such. - caching_cursor_limit = nil if caching_cursor_limit == 0 + caching_cursor_limit = normalized_limit(caching_cursor.view.limit) # There are two scenarios in which a caching cursor could fulfill the # query: @@ -214,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 6ddb12e507..923445eb1c 100644 --- a/spec/integration/query_cache_spec.rb +++ b/spec/integration/query_cache_spec.rb @@ -345,16 +345,25 @@ 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]) @@ -372,19 +381,30 @@ 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]) @@ -422,6 +442,104 @@ 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) + 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 } @@ -432,6 +550,16 @@ 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 738c1f238a..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 } @@ -234,6 +242,15 @@ 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 } @@ -276,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 } @@ -284,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 } @@ -292,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 }