Merge pull request #4029 from chef/ssd/search-pagination-fix

Fix search result pagination

2 files changed, 35 insertions, 3 deletions

diff --git a/lib/chef/search/query.rb b/lib/chef/search/query.rb
index 6469a18c49..658af8779c 100644
--- a/lib/chef/search/query.rb
+++ b/lib/chef/search/query.rb
@@ -88,8 +88,21 @@ WARNDEP
 
         if block
           response["rows"].each { |row| block.call(row) if row }
-          unless (response["start"] + response["rows"].length) >= response["total"]
-            args_h[:start] = response["start"] + response["rows"].length
+          #
+          # args_h[:rows] and args_h[:start] are the page size and
+          # start position requested of the search index backing the
+          # search API.
+          #
+          # The response may contain fewer rows than arg_h[:rows] if
+          # the page of index results included deleted nodes which
+          # have been filtered from the returned data. In this case,
+          # we still want to start the next page at start +
+          # 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)
+          unless next_start >= response["total"]
+            args_h[:start] = next_start
             search(type, query, args_h, &block)
           end
           true
@@ -99,6 +112,7 @@ WARNDEP
       end
 
       private
+
       def validate_type(t)
         unless t.kind_of?(String) || t.kind_of?(Symbol)
           msg = "Invalid search object type #{t.inspect} (#{t.class}), must be a String or Symbol." +
diff --git a/spec/unit/search/query_spec.rb b/spec/unit/search/query_spec.rb
index 59ac80f228..f85b1760d4 100644
--- a/spec/unit/search/query_spec.rb
+++ b/spec/unit/search/query_spec.rb
@@ -83,6 +83,8 @@ describe Chef::Search::Query do
   describe "search" do
     let(:query_string) { "search/node?q=platform:rhel&sort=X_CHEF_id_CHEF_X%20asc&start=0" }
     let(:query_string_continue) { "search/node?q=platform:rhel&sort=X_CHEF_id_CHEF_X%20asc&start=4" }
+    let(:query_string_with_rows) { "search/node?q=platform:rhel&sort=X_CHEF_id_CHEF_X%20asc&start=0&rows=4" }
+    let(:query_string_continue_with_rows) { "search/node?q=platform:rhel&sort=X_CHEF_id_CHEF_X%20asc&start=4&rows=4" }
 
     let(:response) { {
       "rows" => [
@@ -149,6 +151,14 @@ describe Chef::Search::Query do
       r
     }
 
+    let(:big_response_empty) {
+      {
+        "start" => 0,
+        "total" => 8,
+        "rows" => []
+      }
+    }
+
     let(:big_response_end) {
       r = response.dup
       r["start"] = 4
@@ -208,7 +218,7 @@ describe Chef::Search::Query do
     it "pages through the responses" do
       @call_me = double("blocky")
       response["rows"].each { |r| expect(@call_me).to receive(:do).with(r) }
-      query.search(:node, "*:*", sort: nil, start: 0, rows: 1) { |r| @call_me.do(r) }
+      query.search(:node, "*:*", sort: nil, start: 0, rows: 4) { |r| @call_me.do(r) }
     end
 
     it "sends multiple API requests when the server indicates there is more data" do
@@ -219,6 +229,14 @@ describe Chef::Search::Query do
       end
     end
 
+    it "paginates correctly in the face of filtered nodes" do
+      expect(rest).to receive(:get_rest).with(query_string_with_rows).and_return(big_response_empty)
+      expect(rest).to receive(:get_rest).with(query_string_continue_with_rows).and_return(big_response_end)
+      query.search(:node, "platform:rhel", rows: 4) do |r|
+        nil
+      end
+    end
+
     context "when :filter_result is provided as a result" do
       include_context "filtered search" do
         let(:filter_key) { :filter_result }