From 5b4f08e1c6fc83a12b896b3949676ceff6436f52 Mon Sep 17 00:00:00 2001 From: Steven Danna Date: Mon, 31 Jul 2017 01:13:38 +0100 Subject: Set explicit page size for every search request The previous code would advance the search start parameter by either the user-provided rows parameter or (as a fallback) the number of rows returned in that page. Most code that calls the search() function does not set rows, so the fallback is used often. Since the search index may have stale results, it is possible to get responses with a completely empty page. For example: { "start": 0, "total": 1, "rows": [] } In this case, if no rows parameter was passed, the search function would infinitely recurse (eventually running out of stack). To avoid this, we set rows to 1000 if no user-provided value was set. This is the default page size of Chef Server so for most users it will not be changing the default behavior. Signed-off-by: Steven Danna --- lib/chef/search/query.rb | 7 ++++++- spec/functional/knife/ssh_spec.rb | 2 +- spec/unit/search/query_spec.rb | 31 +++++++++++++------------------ 3 files changed, 20 insertions(+), 20 deletions(-) diff --git a/lib/chef/search/query.rb b/lib/chef/search/query.rb index 6f494819ba..a2663d111d 100644 --- a/lib/chef/search/query.rb +++ b/lib/chef/search/query.rb @@ -71,6 +71,11 @@ class Chef args_h = args_h.reject { |k, v| k == :fuzz } end + # Set default rows parameter to 1000. This is the default in + # Chef Server, but we set it explicitly here so that we can + # confidently advance our start parameter. + args_h[:rows] ||= 1000 + response = call_rest_service(type, query: query, **args_h) if block @@ -87,7 +92,7 @@ class Chef # args_h[:rows] to avoid asking the search backend for # overlapping pages (which could result in duplicates). # - next_start = response["start"] + (args_h[:rows] || response["rows"].length) + next_start = response["start"] + args_h[:rows] unless next_start >= response["total"] args_h[:start] = next_start search(type, query, args_h, &block) diff --git a/spec/functional/knife/ssh_spec.rb b/spec/functional/knife/ssh_spec.rb index fba344649f..9d6fd3ae10 100644 --- a/spec/functional/knife/ssh_spec.rb +++ b/spec/functional/knife/ssh_spec.rb @@ -304,7 +304,7 @@ describe Chef::Knife::Ssh do Chef::Config[:client_key] = nil Chef::Config[:chef_server_url] = "http://localhost:9000" - @api.post("/search/node?q=*:*&start=0", 200) do + @api.post("/search/node?q=*:*&start=0&rows=1000", 200) do %({"total":1, "start":0, "rows":[{"data": {"fqdn":"the.fqdn", "config": "the_public_hostname", "knife_config": "the_public_hostname" }}]}) end end diff --git a/spec/unit/search/query_spec.rb b/spec/unit/search/query_spec.rb index 95221870d5..151fcb51b3 100644 --- a/spec/unit/search/query_spec.rb +++ b/spec/unit/search/query_spec.rb @@ -22,9 +22,10 @@ require "chef/search/query" describe Chef::Search::Query do let(:rest) { double("Chef::ServerAPI") } let(:query) { Chef::Search::Query.new } + let(:default_rows) { 1000 } shared_context "filtered search" do - let(:query_string) { "search/node?q=platform:rhel&start=0" } + let(:query_string) { "search/node?q=platform:rhel&start=0&rows=#{default_rows}" } let(:server_url) { "https://api.opscode.com/organizations/opscode/nodes" } let(:args) { { filter_key => filter_hash } } let(:filter_hash) do @@ -81,8 +82,8 @@ describe Chef::Search::Query do end describe "search" do - let(:query_string) { "search/node?q=platform:rhel&start=0" } - let(:query_string_continue) { "search/node?q=platform:rhel&start=4" } + let(:query_string) { "search/node?q=platform:rhel&start=0&rows=#{default_rows}" } + let(:query_string_continue) { "search/node?q=platform:rhel&start=4&rows=#{default_rows}" } let(:query_string_with_rows) { "search/node?q=platform:rhel&start=0&rows=4" } let(:query_string_continue_with_rows) { "search/node?q=platform:rhel&start=4&rows=4" } @@ -150,12 +151,6 @@ describe Chef::Search::Query do "total" => 4, } end - let(:big_response) do - r = response.dup - r["total"] = 8 - r - end - let(:big_response_empty) do { "start" => 0, @@ -178,17 +173,17 @@ describe Chef::Search::Query do end it "queries for every object of a type by default" do - expect(rest).to receive(:get).with("search/node?q=*:*&start=0").and_return(response) + expect(rest).to receive(:get).with("search/node?q=*:*&start=0&rows=#{default_rows}").and_return(response) query.search(:node) end it "allows a custom query" do - expect(rest).to receive(:get).with("search/node?q=platform:rhel&start=0").and_return(response) + expect(rest).to receive(:get).with("search/node?q=platform:rhel&start=0&rows=#{default_rows}").and_return(response) query.search(:node, "platform:rhel") end it "lets you set a starting object" do - expect(rest).to receive(:get).with("search/node?q=platform:rhel&start=2").and_return(response) + expect(rest).to receive(:get).with("search/node?q=platform:rhel&start=2&rows=#{default_rows}").and_return(response) query.search(:node, "platform:rhel", start: 2) end @@ -221,9 +216,9 @@ describe Chef::Search::Query do query.search(:node, "*:*", start: 0, rows: 4) { |r| @call_me.do(r) } end - it "sends multiple API requests when the server indicates there is more data" do - expect(rest).to receive(:get).with(query_string).and_return(big_response) - expect(rest).to receive(:get).with(query_string_continue).and_return(big_response_end) + # This test would loop infinitely if pagination didn't advance + it "paginates correctly in the face of filtered nodes without explicit rows" do + allow(rest).to receive(:get).with(query_string).and_return(big_response_empty) query.search(:node, "platform:rhel") do |r| nil end @@ -239,21 +234,21 @@ describe Chef::Search::Query do it "fuzzifies node searches when fuzz is set" do expect(rest).to receive(:get).with( - "search/node?q=tags:*free.messi*%20OR%20roles:*free.messi*%20OR%20fqdn:*free.messi*%20OR%20addresses:*free.messi*%20OR%20policy_name:*free.messi*%20OR%20policy_group:*free.messi*&start=0" + "search/node?q=tags:*free.messi*%20OR%20roles:*free.messi*%20OR%20fqdn:*free.messi*%20OR%20addresses:*free.messi*%20OR%20policy_name:*free.messi*%20OR%20policy_group:*free.messi*&start=0&rows=#{default_rows}" ).and_return(response) query.search(:node, "free.messi", fuzz: true) end it "does not fuzzify node searches when fuzz is not set" do expect(rest).to receive(:get).with( - "search/node?q=free.messi&start=0" + "search/node?q=free.messi&start=0&rows=#{default_rows}" ).and_return(response) query.search(:node, "free.messi") end it "does not fuzzify client searches" do expect(rest).to receive(:get).with( - "search/client?q=messi&start=0" + "search/client?q=messi&start=0&rows=#{default_rows}" ).and_return(response) query.search(:client, "messi", fuzz: true) end -- cgit v1.2.1