Adding clarity to query string handling in REST calls

We previously only handled simple strings as values,
but we know handle true and false as booleans, we URI-escape
all strings, and we can yaml-encode and then escape arrays of
strings.

This could get abused a bit, in that we're just yaml-dumping anything
that's an array, but it should be pretty safe.  Mmmm, should.

Signed-off-by: Luke Kanies <luke@madstop.com>

3 files changed, 120 insertions, 15 deletions

diff --git a/spec/unit/indirector/rest.rb b/spec/unit/indirector/rest.rb
index e7afbb5b2..aa6d51b6a 100755
--- a/spec/unit/indirector/rest.rb
+++ b/spec/unit/indirector/rest.rb
@@ -146,6 +146,66 @@ describe Puppet::Indirector::REST do
         end
     end
 
+    describe "when building a query string from request options" do
+        it "should return an empty query string if there are no options" do
+            @request.stubs(:options).returns nil
+            @searcher.query_string(@request).should == ""
+        end
+
+        it "should return an empty query string if the options are empty" do
+            @request.stubs(:options).returns({})
+            @searcher.query_string(@request).should == ""
+        end
+
+        it "should prefix the query string with '?'" do
+            @request.stubs(:options).returns(:one => "two")
+            @searcher.query_string(@request).should =~ /^\?/
+        end
+
+        it "should include all options in the query string, separated by '&'" do
+            @request.stubs(:options).returns(:one => "two", :three => "four")
+            @searcher.query_string(@request).sub(/^\?/, '').split("&").sort.should == %w{one=two three=four}.sort
+        end
+
+        it "should ignore nil options" do
+            @request.stubs(:options).returns(:one => "two", :three => nil)
+            @searcher.query_string(@request).should_not be_include("three")
+        end
+
+        it "should convert 'true' option values into strings" do
+            @request.stubs(:options).returns(:one => true)
+            @searcher.query_string(@request).should == "?one=true"
+        end
+
+        it "should convert 'false' option values into strings" do
+            @request.stubs(:options).returns(:one => false)
+            @searcher.query_string(@request).should == "?one=false"
+        end
+
+        it "should URI-escape all option values that are strings" do
+            escaping = URI.escape("one two")
+            @request.stubs(:options).returns(:one => "one two")
+            @searcher.query_string(@request).should == "?one=#{escaping}"
+        end
+
+        it "should YAML-dump and URI-escape arrays" do
+            escaping = URI.escape(YAML.dump(%w{one two}))
+            @request.stubs(:options).returns(:one => %w{one two})
+            @searcher.query_string(@request).should == "?one=#{escaping}"
+        end
+
+        it "should convert to a string and URI-escape all option values that are symbols" do
+            escaping = URI.escape("sym bol")
+            @request.stubs(:options).returns(:one => :"sym bol")
+            @searcher.query_string(@request).should == "?one=#{escaping}"
+        end
+
+        it "should fail if options other than booleans or strings are provided" do
+            @request.stubs(:options).returns(:one => {:one => :two})
+            lambda { @searcher.query_string(@request) }.should raise_error(ArgumentError)
+        end
+    end
+
     describe "when doing a find" do
         before :each do
             @connection = stub('mock http connection', :get => @response)
@@ -173,13 +233,6 @@ describe Puppet::Indirector::REST do
             @searcher.find(@request)
         end
 
-        it "should include all options in the query string" do
-            @request.stubs(:options).returns(:one => "two", :three => "four")
-            should_path = "/%s/%s" % [@indirection.name.to_s, "foo"]
-            @connection.expects(:get).with { |path, args| path =~ /\?one=two&three=four$/ or path =~ /\?three=four&one=two$/ }.returns(@response)
-            @searcher.find(@request)
-        end
-
         it "should provide an Accept header containing the list of supported formats joined with commas" do
             @connection.expects(:get).with { |path, args| args["Accept"] == "supported, formats" }.returns(@response)
 
diff --git a/spec/unit/network/http/mongrel/rest.rb b/spec/unit/network/http/mongrel/rest.rb
index 9dbc1c9ab..e72d4f540 100755
--- a/spec/unit/network/http/mongrel/rest.rb
+++ b/spec/unit/network/http/mongrel/rest.rb
@@ -97,16 +97,42 @@ describe "Puppet::Network::HTTP::MongrelREST" do
             end
         end
 
-        describe "and determining the request parameters", :shared => true do
+        describe "and determining the request parameters" do
             before do
                 @request.stubs(:params).returns({})
             end
 
-            it "should include the HTTP request parameters" do
+            it "should include the HTTP request parameters, with the keys as symbols" do
                 @request.expects(:params).returns('QUERY_STRING' => 'foo=baz&bar=xyzzy')
                 result = @handler.params(@request)
-                result["foo"].should == "baz"
-                result["bar"].should == "xyzzy"
+                result[:foo].should == "baz"
+                result[:bar].should == "xyzzy"
+            end
+
+            it "should URI-decode the HTTP parameters" do
+                encoding = URI.escape("foo bar")
+                @request.expects(:params).returns('QUERY_STRING' => "foo=#{encoding}")
+                result = @handler.params(@request)
+                result[:foo].should == "foo bar"
+            end
+
+            it "should convert the string 'true' to the boolean" do
+                @request.expects(:params).returns('QUERY_STRING' => 'foo=true')
+                result = @handler.params(@request)
+                result[:foo].should be_true
+            end
+
+            it "should convert the string 'false' to the boolean" do
+                @request.expects(:params).returns('QUERY_STRING' => 'foo=false')
+                result = @handler.params(@request)
+                result[:foo].should be_false
+            end
+
+            it "should YAML-load and URI-decode values that are YAML-encoded" do
+                escaping = URI.escape(YAML.dump(%w{one two}))
+                @request.expects(:params).returns('QUERY_STRING' => "foo=#{escaping}")
+                result = @handler.params(@request)
+                result[:foo].should == %w{one two}
             end
 
             it "should pass the client's ip address to model find" do
diff --git a/spec/unit/network/http/webrick/rest.rb b/spec/unit/network/http/webrick/rest.rb
index 1f4ccbe29..b32cf49c5 100755
--- a/spec/unit/network/http/webrick/rest.rb
+++ b/spec/unit/network/http/webrick/rest.rb
@@ -83,11 +83,37 @@ describe Puppet::Network::HTTP::WEBrickREST do
         end
 
         describe "and determining the request parameters" do
-            it "should include the HTTP request parameters" do
-                @request.stubs(:query).returns(:foo => :baz, :bar => :xyzzy)
+            it "should include the HTTP request parameters, with the keys as symbols" do
+                @request.stubs(:query).returns("foo" => "baz", "bar" => "xyzzy")
                 result = @handler.params(@request)
-                result[:foo].should == :baz
-                result[:bar].should == :xyzzy
+                result[:foo].should == "baz"
+                result[:bar].should == "xyzzy"
+            end
+
+            it "should URI-decode the HTTP parameters" do
+                encoding = URI.escape("foo bar")
+                @request.expects(:query).returns('foo' => encoding)
+                result = @handler.params(@request)
+                result[:foo].should == "foo bar"
+            end
+
+            it "should convert the string 'true' to the boolean" do
+                @request.expects(:query).returns('foo' => "true")
+                result = @handler.params(@request)
+                result[:foo].should be_true
+            end
+
+            it "should convert the string 'false' to the boolean" do
+                @request.expects(:query).returns('foo' => "false")
+                result = @handler.params(@request)
+                result[:foo].should be_false
+            end
+
+            it "should YAML-load and URI-decode values that are YAML-encoded" do
+                escaping = URI.escape(YAML.dump(%w{one two}))
+                @request.expects(:query).returns('foo' => escaping)
+                result = @handler.params(@request)
+                result[:foo].should == %w{one two}
             end
 
             it "should pass the client's ip address to model find" do