Updated based on awesome feedback

3 files changed, 97 insertions, 67 deletions

diff --git a/lib/chef/knife/search.rb b/lib/chef/knife/search.rb
index 1bb182a61b..cf2ca7b683 100644
--- a/lib/chef/knife/search.rb
+++ b/lib/chef/knife/search.rb
@@ -92,8 +92,6 @@ class Chef
         result_items = []
         result_count = 0
 
-        # rows = config[:rows]
-        # start = config[:start]
         search_args = Hash.new
         search_args[:sort] = config[:sort]
         search_args[:start] = config[:start]
@@ -163,17 +161,24 @@ class Chef
         end
       end
 
+      # This method turns a set of key value pairs in a string into the appropriate data structure that the
+      # chef-server search api is expecting.
+      # expected input is in the form of:
+      # -f "return_var1=path.to.attribute, return_var2=shorter.path"
+      #
+      # a more concrete example might be:
+      # -f "env=chef_environment, ruby_platform=languages.ruby.platform"
+      #
+      # The end result is a hash where the key is a symbol in the hash (the return variable)
+      # and the path is an array with the path elements as strings (in order)
+      # See lib/chef/search/query.rb for more examples of this.
       def create_result_filter(filter_string)
         final_filter = Hash.new
-
-        # this probably needs review. I betcha there's a better way!!!
         filter_string.gsub!(" ", "")
         filters = filter_string.split(",")
         filters.each do |f|
-          fsplit = f.split("=")
-          return_id = fsplit[0]
-          attr_path = fsplit[1].split(".")
-          final_filter[return_id.to_sym] = attr_path
+          return_id, attr_path = f.split("=")
+          final_filter[return_id.to_sym] = attr_path.split(".")
         end
         return final_filter
       end
diff --git a/lib/chef/search/query.rb b/lib/chef/search/query.rb
index bf3518e695..de64d64ed0 100644
--- a/lib/chef/search/query.rb
+++ b/lib/chef/search/query.rb
@@ -34,40 +34,35 @@ class Chef
         @rest = Chef::REST.new(url ||Chef::Config[:chef_server_url])
       end
 
-      # new search api that allows for a cleaner implementation of things like return filters
-      # (formerly known as 'partial search'). A passthrough to either the old style ("full search")
-      # or the new 'filtered' search
-      def search_new(type, query="*:*", args=nil, &block)
-        raise ArgumentError, "Type must be a string or a symbol!" unless (type.kind_of?(String) || type.kind_of?(Symbol))
-
-        # if args is nil, we need to set some defaults, for backwards compatibility
-        if args.nil?
-          args = Hash.new
-          args[:sort] = 'X_CHEF_id_CHEF_X asc'
-          args[:start] = 0
-          args[:rows] = 1000
-        end
-
-        query_string = create_query_string(type, query, args)
-        response = call_rest_service(query_string, args)
-        if block
-          response["rows"].each { |o| block.call(o) unless o.nil?}
-          unless (response["start"] + response["rows"].length) >= response["total"]
-            args[:start] = response["start"] + args[:rows]
-            search_new(type, query, args, &block)
-          end
-          true
-        else
-          [ response["rows"], response["start"], response["total"] ]
-        end
-
-      end
-
+      #
+      # New search input, designed to be backwards compatible with the old method signature
+      # 'type' and 'query' are the same as before, args now will accept either a Hash of 
+      # search arguments with symbols as the keys (ie :sort, :start, :rows) and a :filter_result
+      # option.
+      #
+      # :filter_result should be in the format of another Hash with the structure of:
+      # {
+      #   :returned_name1 => ["path", "to", "variable"],
+      #   :returned_name2 => ["shorter", "path"]
+      # } 
+      # a real world example might be something like:
+      # {
+      #   :ip_address => ["ipaddress"],
+      #   :ruby_version => ["languages", "ruby", "version"]
+      # }
+      #  this will bring back 2 variables 'ip_address' and 'ruby_version' with whatever value was found
+      # 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)
         raise ArgumentError, "Type must be a string or a symbol!" unless (type.kind_of?(String) || type.kind_of?(Symbol))
         raise ArgumentError, "Invalid number of arguments!" if (args.size > 3)
         if args.size == 1 && args[0].is_a?(Hash)
-          args_hash = args[0]
+          args_hash = args[0] 
+          # just in case the hash doesn't have the correct defaults, we'll set them
+          args_hash[:sort] = args_hash.key?(:sort) ? args_hash[:sort] : 'X_CHEF_id_CHEF_X asc'
+          args_hash[:start] = args_hash.key?(:start) ? args_hash[:start] : 0
+          args_hash[:rows] = args_hash.key?(:rows) ? args_hash[:rows] : 1000
           search_new(type, query, args_hash, &block)
         else
           sort = args.length >= 1 ? args[0] : 'X_CHEF_id_CHEF_X asc'
@@ -77,21 +72,6 @@ class Chef
         end
       end
 
-
-      # Search Solr for objects of a given type, for a given query. If you give
-      # it a block, it will handle the paging for you dynamically.
-      def search_old(type, query="*:*", sort='X_CHEF_id_CHEF_X asc', start=0, rows=1000, &block)
-        raise ArgumentError, "Type must be a string or a symbol!" unless (type.kind_of?(String) || type.kind_of?(Symbol))
-
-        # argify things
-        args = Hash.new
-        args[:sort] = sort
-        args[:start] = start
-        args[:rows] = rows
-
-        search_new(type, query, args, &block)
-      end
-
       def list_indexes
         @rest.get_rest("search")
       end
@@ -100,13 +80,50 @@ class Chef
         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'). A passthrough to either the old style ("full search")
+        # or the new 'filtered' search
+        def search_new(type, query="*:*", args=nil, &block)
+          raise ArgumentError, "Type must be a string or a symbol!" unless (type.kind_of?(String) || type.kind_of?(Symbol))
+
+          # if args is nil, we need to set some defaults, for backwards compatibility
+          if args.nil?
+            args = Hash.new
+            args = args || { :sort => 'X_CHEF_id_CHEF_X asc', :start => 0, :rows => 1000 }
+          end
+
+          query_string = create_query_string(type, query, args)
+          response = call_rest_service(query_string, args)
+          if block
+            response["rows"].each { |o| block.call(o) unless o.nil?}
+            unless (response["start"] + response["rows"].length) >= response["total"]
+              args[:start] = response["start"] + args[:rows]
+              search_new(type, query, args, &block)
+            end
+            true
+          else
+            [ response["rows"], response["start"], response["total"] ]
+          end
+        end
         
+        # Search Solr for objects of a given type, for a given query. If you give
+        # it a block, it will handle the paging for you dynamically.
+        def search_old(type, query="*:*", sort='X_CHEF_id_CHEF_X asc', start=0, rows=1000, &block)
+          raise ArgumentError, "Type must be a string or a symbol!" unless (type.kind_of?(String) || type.kind_of?(Symbol))
+
+          # argify things
+          args = Hash.new
+          args = { :sort => sort, :start => start, :rows => rows }
+          search_new(type, query, args, &block)
+        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.key?(:sort) ? args[:sort] : 'X_CHEF_id_CHEF_X asc'
-          start = args.key?(:start) ? args[:start] : 0
-          rows = args.key?(:rows) ? args[:rows] : 1000
+          sort = args[:sort]
+          start = args[:start]
+          rows = args[:rows]
 
           return "search/#{type}?q=#{escape(query)}&sort=#{escape(sort)}&start=#{escape(start)}&rows=#{escape(rows)}"
         end
diff --git a/spec/unit/search/query_spec.rb b/spec/unit/search/query_spec.rb
index 714806ce29..8ab653b6f7 100644
--- a/spec/unit/search/query_spec.rb
+++ b/spec/unit/search/query_spec.rb
@@ -145,7 +145,7 @@ describe Chef::Search::Query do
     end
   end
 
-  # copypasta existing functionality for new searhc, because new search should at the very least do the same stuff!
+  # copypasta existing functionality for new search, because new search should at the very least do the same stuff!
   describe "new search" do
     before(:each) do
       @response = {
@@ -239,8 +239,10 @@ describe Chef::Search::Query do
       @rest.should_receive(:get_rest).with("search/node?q=platform:rhel&sort=id%20desc&start=2&rows=1000").and_return(@response)
       @query = Chef::Search::Query.new
       args = Hash.new
-      args[:sort] = "id desc"
-      args[:start] = 2
+      args = {
+        :sort => "id desc",
+        :start => 2
+      }
       @query.search(:node, "platform:rhel", args)
     end
 
@@ -248,9 +250,11 @@ describe Chef::Search::Query do
       @rest.should_receive(:get_rest).with("search/node?q=platform:rhel&sort=id%20desc&start=2&rows=40").and_return(@response)
       @query = Chef::Search::Query.new
       args = Hash.new
-      args[:sort] = "id desc"
-      args[:start] = 2
-      args[:rows] = 40
+      args = {
+        :sort => "id desc",
+        :start => 2,
+        :rows => 40
+      }
       @query.search(:node, "platform:rhel", args)
     end
 
@@ -271,9 +275,11 @@ describe Chef::Search::Query do
       @call_me = double("blocky")
       @response["rows"].each { |r| @call_me.should_receive(:do).with(r) }
       args = Hash.new
-      args[:sort] = nil
-      args[:start] = 0
-      args[:rows] = 1
+      args = {
+        :sort => nil,
+        :start => 0,
+        :rows => 1
+      }
       @query.search(:node, "*:*", args) { |r| @call_me.do(r) }
     end
   end
@@ -316,8 +322,10 @@ describe Chef::Search::Query do
 
     it "should allow you to filter search results" do
       filter_args = Hash.new
-      filter_args[:env] = ["chef_environment"]
-      filter_args[:ruby_plat] = ["languages", "ruby", "platform"]
+      filter_args = {
+        :env => [ "chef_environment" ],
+        :ruby_plat => [ "languages", "ruby", "platform" ]
+      }
       @rest.should_receive(:post_rest).with("search/node?q=platform:rhel&sort=X_CHEF_id_CHEF_X%20asc&start=0&rows=1000", filter_args).and_return(@response)
       @query = Chef::Search::Query.new
       args = Hash.new