Set explicit page size for every search requestssd/always-forward

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 <steve@chef.io>

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