Make search with filtering match partial_search.

* Updated search to use argument parameters.

2 files changed, 82 insertions, 120 deletions

diff --git a/lib/chef/search/query.rb b/lib/chef/search/query.rb
index cc43efe1b1..531ad53990 100644
--- a/lib/chef/search/query.rb
+++ b/lib/chef/search/query.rb
@@ -16,14 +16,12 @@
 # limitations under the License.
 #
 
+
 require 'chef/config'
-require 'uri'
-require 'chef/rest'
-require 'chef/node'
-require 'chef/role'
-require 'chef/data_bag'
-require 'chef/data_bag_item'
 require 'chef/exceptions'
+require 'chef/rest'
+
+require 'uri'
 
 class Chef
   class Search
@@ -32,37 +30,45 @@ class Chef
       attr_accessor :rest
 
       def initialize(url=nil)
-        @rest = Chef::REST.new(url ||Chef::Config[:chef_server_url])
+        @rest = Chef::REST.new(url || Chef::Config[:chef_server_url])
       end
 
-
-      # This search is only kept for backwards compatibility, since the results of the
-      # new filtered search method will be in a slightly different format
-      def partial_search(type, query='*:*', *args, &block)
-        Chef::Log.warn("DEPRECATED: The 'partial_search' api is deprecated, please use the search api with 'filter_result'")
-        # accept both types of args
-        if args.length == 1 && args[0].is_a?(Hash)
-          args_hash = args[0].dup
-          # partial_search implemented in the partial search cookbook uses the
-          # arg hash :keys instead of :filter_result to filter returned data
-          args_hash[:filter_result] = args_hash[:keys]
+      # Backwards compatability for cookbooks.
+      # This can be removed in Chef > 12.
+      def partial_search(type, query="*:*", *args, &block)
+        Chef::Log.warn(<<-WARNDEP)
+DEPRECATED: The 'partial_search' API is deprecated and will be removed in
+future releases. Please use 'search' with a :filter_result argument to get
+partial search data.
+WARNDEP
+
+        # Users can pass either a hash or a list of arguments.
+        if args.length == 1 && args.first.is_a?(Hash)
+          args_h = args.first
+          rows = args_h[:rows]
+          start = args_h[:start]
+          sort = args_h[:sort]
+          keys = args_h[:keys]
         else
-          args_hash = {}
-          args_hash[:sort] = args[0] if args.length >= 1
-          args_hash[:start] = args[1] if args.length >= 2
-          args_hash[:rows] = args[2] if args.length >= 3
+          rows = args[0]
+          start = args[1]
+          sort = args[2]
+          keys = args[3]
         end
 
-        unless block.nil?
-          raw_results = search(type,query,args_hash)
+        # Set defaults. Otherwise, search may receive nil arguments.
+        # We could pass nil arguments along to search, assuming that default values will be
+        # filled in later. However, since this is a deprecated method, it will be easier to
+        # do a little more work here than to change search in the future.
+        rows ||= 1000
+        start ||= 0
+        sort ||= 'X_CHEF_id_CHEF_X asc'
+
+        unless block.nil? #@TODO: IS THIS CORRECT? THIS DOESN'T SEEM CORRECT. WHY DO WE EVEN NEED IT?
+          search(type, query, filter_result: keys, rows: rows, start: start, sort: sort)
         else
-          raw_results = search(type,query,args_hash,&block)
-        end
-        results = Array.new
-        raw_results[0].each do |r|
-          results << r["data"]
+          search(type, query, filter_result: keys.dup, rows: rows, start: start, sort: sort, &block)
         end
-        return results
       end
 
       #
@@ -85,89 +91,57 @@ class Chef
       # an example of the returned json may be:
       # {"ip_address":"127.0.0.1", "ruby_version": "1.9.3"}
       #
-      def search(type, query='*:*', *args, &block)
+      def search(type, query="*:*", filter_result:nil, rows:1000, start:0, sort:'X_CHEF_id_CHEF_X asc', &block)
         validate_type(type)
-        validate_args(args)
 
-        scrubbed_args = Hash.new
+        query_string = create_query_string(type, query, rows, start, sort)
+        response = call_rest_service(query_string, filter_result)
 
-        # argify everything
-        if args[0].kind_of?(Hash)
-          scrubbed_args = args[0]
+        if block
+          response["rows"].each { |row| block.call(row) if row }
+          if response["start"] + response["rows"].size < response["total"]
+            start = response["start"] + rows
+            search(type, query, filter_result: filter_result, rows: rows, start: start, sort: sort, &block)
+          end
+          true
         else
-          # This api will be deprecated in a future release
-          scrubbed_args = { :sort => args[0], :start => args[1], :rows => args[2] }
+          [
+            response["rows"],
+            response["start"],
+            response["total"]
+          ]
         end
-
-        # set defaults, if they haven't been set yet.
-        scrubbed_args[:sort] ||= 'X_CHEF_id_CHEF_X asc'
-        scrubbed_args[:start] ||= 0
-        scrubbed_args[:rows] ||= 1000
-
-        do_search(type, query, scrubbed_args, &block)
-      end
-
-      def list_indexes
-        @rest.get_rest("search")
       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." +
-            "Useage: search(:node, QUERY, [OPTIONAL_ARGS])" +
+            "Useage: search(:node, QUERY[, OPTIONAL_ARGS])" +
             "        `knife search environment QUERY (options)`"
           raise Chef::Exceptions::InvalidSearchQuery, msg
         end
       end
 
-      def validate_args(a)
-        max_args = 3
-        raise Chef::Exceptions::InvalidSearchQuery, "Too many arguments! (#{a.size} for <= #{max_args})" if a.size > max_args
+      def create_query_string(type, query, rows, start, sort)
+        "search/#{type}?q=#{escape(query)}&sort=#{escape(sort)}&start=#{escape(start)}&rows=#{escape(rows)}"
       end
 
       def escape(s)
         s && URI.escape(s.to_s)
       end
 
-      # new search api that allows for a cleaner implementation of things like return filters
-      # (formerly known as 'partial search').
-      # Also args should never be nil, but that is required for Ruby 1.8 compatibility
-      def do_search(type, query="*:*", args=nil, &block)
-        query_string = create_query_string(type, query, args)
-        response = call_rest_service(query_string, args)
-        unless block.nil?
-          response["rows"].each { |rowset| block.call(rowset) unless rowset.nil?}
-          unless (response["start"] + response["rows"].length) >= response["total"]
-            args[:start] = response["start"] + args[:rows]
-            do_search(type, query, args, &block)
-          end
-          true
+      def call_rest_service(query_string, filter_result)
+        if filter_result
+          response = rest.post_rest(query_string, filter_result)
+          response['rows'].map! { |row| row['data'] }
         else
-          [ response["rows"], response["start"], response["total"] ]
+          response = rest.get_rest(query_string)
         end
-      end
-
-      # create the full rest url string
-      def create_query_string(type, query, args)
-        # create some default variables just so we don't break backwards compatibility
-        sort = args[:sort]
-        start = args[:start]
-        rows = args[:rows]
 
-        return "search/#{type}?q=#{escape(query)}&sort=#{escape(sort)}&start=#{escape(start)}&rows=#{escape(rows)}"
+        response
       end
 
-      def call_rest_service(query_string, args)
-        if args.key?(:filter_result)
-          response = @rest.post_rest(query_string, args[:filter_result])
-          response_rows = response['rows'].map { |row| row['data'] }
-        else
-          response = @rest.get_rest(query_string)
-          response_rows = response['rows']
-        end
-        return response
-      end
     end
   end
 end
diff --git a/spec/unit/search/query_spec.rb b/spec/unit/search/query_spec.rb
index d4ff9e4367..2b8e8ac21b 100644
--- a/spec/unit/search/query_spec.rb
+++ b/spec/unit/search/query_spec.rb
@@ -17,7 +17,7 @@
 #
 
 require 'spec_helper'
-require 'chef/search/query'
+require 'chef/search/_query'
 
 describe Chef::Search::Query do
   let(:rest) { double("Chef::REST") }
@@ -65,6 +65,14 @@ describe Chef::Search::Query do
         "total" => 4
       }
     }
+    let(:response_rows) {
+      [
+        { "env" => "elysium", "ruby_plat" => "nudibranch" },
+        { "env" => "hades", "ruby_plat" => "i386-mingw32"},
+        { "env" => "elysium", "ruby_plat" => "centos"},
+        { "env" => "moon", "ruby_plat" => "solaris2"}
+      ]
+    }
   end
 
   before(:each) do
@@ -150,22 +158,22 @@ describe Chef::Search::Query do
 
     it "should let you set a sort order" do
       expect(rest).to receive(:get_rest).with("search/node?q=platform:rhel&sort=id%20desc&start=0&rows=1000").and_return(response)
-      query.search(:node, "platform:rhel", "id desc")
+      query.search(:node, "platform:rhel", sort: "id desc")
     end
 
     it "should let you set a starting object" do
-      expect(rest).to receive(:get_rest).with("search/node?q=platform:rhel&sort=id%20desc&start=2&rows=1000").and_return(response)
-      query.search(:node, "platform:rhel", "id desc", 2)
+      expect(rest).to receive(:get_rest).with("search/node?q=platform:rhel&sort=X_CHEF_id_CHEF_X%20asc&start=2&rows=1000").and_return(response)
+      query.search(:node, "platform:rhel", start: 2)
     end
 
     it "should let you set how many rows to return" do
-      expect(rest).to receive(:get_rest).with("search/node?q=platform:rhel&sort=id%20desc&start=2&rows=40").and_return(response)
-      query.search(:node, "platform:rhel", "id desc", 2, 40)
+      expect(rest).to receive(:get_rest).with("search/node?q=platform:rhel&sort=X_CHEF_id_CHEF_X%20asc&start=0&rows=40").and_return(response)
+      query.search(:node, "platform:rhel", rows: 40)
     end
 
-    it "should throw an exception if you pass to many options" do
-      expect { query.search(:node, "platform:rhel", "id desc", 2, 40, "wrong") }
-        .to raise_error(Chef::Exceptions::InvalidSearchQuery, "Too many arguments! (4 for <= 3)")
+    it "should throw an exception if you pass an incorrect option" do
+      expect { query.search(:node, "platform:rhel", total: 10) }
+        .to raise_error(ArgumentError, /unknown keyword: total/)
     end
 
     it "should return the raw rows, start, and total if no block is passed" do
@@ -184,7 +192,7 @@ describe Chef::Search::Query do
     it "should page through the responses" do
       @call_me = double("blocky")
       response["rows"].each { |r| expect(@call_me).to receive(:do).with(r) }
-      query.search(:node, "*:*", nil, 0, 1) { |r| @call_me.do(r) }
+      query.search(:node, "*:*", sort: nil, start: 0, rows: 1) { |r| @call_me.do(r) }
     end
 
     context "when :filter_result is provided as a result" do
@@ -206,20 +214,8 @@ describe Chef::Search::Query do
         end
 
         it "should return rows with the filter applied" do
-          results = query.search(:node, "platform:rhel", args)[0]
-
-          results.each_with_index do |result, idx|
-            expected = response["rows"][idx]
-
-            expect(result).to have_key("url")
-            expect(result["url"]).to eq(expected["url"])
-
-            expect(result).to have_key("data")
-            filter_hash.keys.each do |filter_key|
-              expect(result["data"]).to have_key(filter_key)
-              expect(result["data"][filter_key]).to eq(expected["data"][filter_key])
-            end
-          end
+          filtered_rows = query.search(:node, "platform:rhel", args)[0]
+          expect(filtered_rows).to match_array(response_rows)
         end
 
       end
@@ -233,22 +229,14 @@ describe Chef::Search::Query do
       it "should emit a deprecation warning" do
         # partial_search calls search, so we'll stub search to return empty
         allow(query).to receive(:search).and_return( [ [], 0, 0 ] )
-        expect(Chef::Log).to receive(:warn).with("DEPRECATED: The 'partial_search' api is deprecated, please use the search api with 'filter_result'")
+        expect(Chef::Log).to receive(:warn).with(/DEPRECATED: The 'partial_search' API is deprecated/)
         query.partial_search(:node, "platform:rhel", args)
       end
 
       it "should return an array of filtered hashes" do
         expect(rest).to receive(:post_rest).with(query_string, args[filter_key]).and_return(response)
         results = query.partial_search(:node, "platform:rhel", args)
-
-        results.each_with_index do |result, idx|
-          expected = response["rows"][idx]
-
-          filter_hash.keys.each do |filter_key|
-            expect(result).to have_key(filter_key)
-            expect(result[filter_key]).to eq(expected["data"][filter_key])
-          end
-        end
+        expect(results[0]).to match_array(response_rows)
       end
     end
   end