Previously, the server side correctly pulled parameters out of
the query strings, but the REST terminus never passed them on. It does
now, at least for finding and searching. It appears that at least
WEBrick doesn't support parameters for anything other than forms
and GET.
I've also added the ability for the REST terminus to pull
host/port information from the request key, if it's a URI.
Signed-off-by: Luke Kanies <[EMAIL PROTECTED]>
---
lib/puppet/indirector/rest.rb | 39 +++++++++++++---
lib/puppet/network/http/handler.rb | 6 ++-
spec/integration/indirector/rest.rb | 22 +++++++++-
spec/unit/indirector/rest.rb | 74 ++++++++++++++++++++++++++++----
spec/unit/network/http/handler.rb | 14 ++++++-
spec/unit/network/http/mongrel/rest.rb | 5 ++
spec/unit/network/http/webrick/rest.rb | 5 ++
7 files changed, 145 insertions(+), 20 deletions(-)
diff --git a/lib/puppet/indirector/rest.rb b/lib/puppet/indirector/rest.rb
index a2767d0..4389dfb 100644
--- a/lib/puppet/indirector/rest.rb
+++ b/lib/puppet/indirector/rest.rb
@@ -32,31 +32,54 @@ class Puppet::Indirector::REST <
Puppet::Indirector::Terminus
{"Accept" => model.supported_formats.join(", ")}
end
- def network
- Puppet::Network::HttpPool.http_instance(Puppet[:server],
Puppet[:masterport].to_i)
+ def network(request)
+ if request.key =~ /^\w+:\/\// # it looks like a URI
+ begin
+ uri = URI.parse(URI.escape(request.key))
+ rescue => detail
+ raise ArgumentError, "Could not understand URL %s: %s" %
[source, detail.to_s]
+ end
+ server = uri.host || Puppet[:server]
+ port = uri.port.to_i == 0 ? Puppet[:masterport].to_i :
uri.port.to_i
+ else
+ server = Puppet[:server]
+ port = Puppet[:masterport].to_i
+ end
+
+ Puppet::Network::HttpPool.http_instance(server, port)
end
def find(request)
- deserialize network.get("/#{indirection.name}/#{request.key}", headers)
+ deserialize
network(request).get("/#{indirection.name}/#{request.key}#{query_string(request)}",
headers)
end
def search(request)
if request.key
- path = "/#{indirection.name}s/#{request.key}"
+ path =
"/#{indirection.name}s/#{request.key}#{query_string(request)}"
else
- path = "/#{indirection.name}s"
+ path = "/#{indirection.name}s#{query_string(request)}"
end
- unless result = deserialize(network.get(path, headers), true)
+ unless result = deserialize(network(request).get(path, headers), true)
return []
end
return result
end
def destroy(request)
- deserialize network.delete("/#{indirection.name}/#{request.key}",
headers)
+ raise ArgumentError, "DELETE does not accept options" unless
request.options.empty?
+ deserialize
network(request).delete("/#{indirection.name}/#{request.key}", headers)
end
def save(request)
- deserialize network.put("/#{indirection.name}/",
request.instance.render, headers)
+ raise ArgumentError, "PUT does not accept options" unless
request.options.empty?
+ deserialize network(request).put("/#{indirection.name}/",
request.instance.render, headers)
+ end
+
+ private
+
+ # Create the qurey string, if options are present.
+ def query_string(request)
+ return "" unless request.options and ! request.options.empty?
+ "?" + request.options.collect { |key, value| "%s=%s" % [key, value]
}.join("&")
end
end
diff --git a/lib/puppet/network/http/handler.rb
b/lib/puppet/network/http/handler.rb
index 291481a..6f5117b 100644
--- a/lib/puppet/network/http/handler.rb
+++ b/lib/puppet/network/http/handler.rb
@@ -81,7 +81,11 @@ module Puppet::Network::HTTP::Handler
# Execute our search.
def do_search(request, response)
args = params(request)
- result = model.search(args)
+ if key = request_key(request)
+ result = model.search(key, args)
+ else
+ result = model.search(args)
+ end
if result.nil? or (result.is_a?(Array) and result.empty?)
return do_exception(response, "Could not find instances in %s with
'%s'" % [model.name, args.inspect], 404)
end
diff --git a/spec/integration/indirector/rest.rb
b/spec/integration/indirector/rest.rb
index b307e3c..95c2759 100755
--- a/spec/integration/indirector/rest.rb
+++ b/spec/integration/indirector/rest.rb
@@ -91,6 +91,11 @@ describe Puppet::Indirector::REST do
it 'should return an instance of the model class' do
Puppet::TestIndirectedFoo.find('bar').class.should ==
Puppet::TestIndirectedFoo
end
+
+ it "should pass options all the way through" do
+ @mock_model.expects(:find).with { |key, args| args["one"]
== "two" and args["three"] == "four" }.returns @model_instance
+ Puppet::TestIndirectedFoo.find('bar', :one => "two",
:three => "four")
+ end
it 'should return the instance of the model class associated
with the provided lookup key' do
Puppet::TestIndirectedFoo.find('bar').value.should ==
@model_instance.value
@@ -151,6 +156,11 @@ describe Puppet::Indirector::REST do
it 'should return all matching results' do
Puppet::TestIndirectedFoo.search('bar').length.should ==
@model_instances.length
end
+
+ it "should pass options all the way through" do
+ @mock_model.expects(:search).with { |key, args|
args["one"] == "two" and args["three"] == "four" }.returns @model_instances
+ Puppet::TestIndirectedFoo.search("foo", :one => "two",
:three => "four")
+ end
it 'should return model instances' do
Puppet::TestIndirectedFoo.search('bar').each do |result|
@@ -315,6 +325,11 @@ describe Puppet::Indirector::REST do
it 'should return an instance of the model class' do
Puppet::TestIndirectedFoo.find('bar').class.should ==
Puppet::TestIndirectedFoo
end
+
+ it "should pass options all the way through" do
+ @mock_model.expects(:find).with { |key, args| args["one"]
== "two" and args["three"] == "four" }.returns @model_instance
+ Puppet::TestIndirectedFoo.find('bar', :one => "two",
:three => "four")
+ end
it 'should return the instance of the model class associated
with the provided lookup key' do
Puppet::TestIndirectedFoo.find('bar').value.should ==
@model_instance.value
@@ -372,6 +387,11 @@ describe Puppet::Indirector::REST do
it 'should return all matching results' do
Puppet::TestIndirectedFoo.search('bar').length.should ==
@model_instances.length
end
+
+ it "should pass options all the way through" do
+ @mock_model.expects(:search).with { |key, args|
args["one"] == "two" and args["three"] == "four" }.returns @model_instances
+ Puppet::TestIndirectedFoo.search('bar', :one => "two",
:three => "four")
+ end
it 'should return model instances' do
Puppet::TestIndirectedFoo.search('bar').each do |result|
@@ -465,7 +485,7 @@ describe Puppet::Indirector::REST do
it "should not fail" do
lambda { @instance.save }.should_not raise_error
end
-
+
it 'should return an instance of the model class' do
@instance.save.class.should == Puppet::TestIndirectedFoo
end
diff --git a/spec/unit/indirector/rest.rb b/spec/unit/indirector/rest.rb
index 28ceb69..3bd6335 100755
--- a/spec/unit/indirector/rest.rb
+++ b/spec/unit/indirector/rest.rb
@@ -91,11 +91,40 @@ describe Puppet::Indirector::REST do
Puppet.settings.stubs(:value).returns("rest_testing")
end
- it "should use the :server setting as the host and the :masterport
setting (as an Integer) as the port" do
- Puppet.settings.expects(:value).with(:server).returns "myserver"
- Puppet.settings.expects(:value).with(:masterport).returns "1234"
- Puppet::Network::HttpPool.expects(:http_instance).with("myserver",
1234).returns "myconn"
- @searcher.network.should == "myconn"
+ describe "and the request key is not a URL" do
+ it "should use the :server setting as the host and the :masterport
setting (as an Integer) as the port" do
+ @request = stub 'request', :key => "foo"
+ Puppet.settings.expects(:value).with(:server).returns
"myserver"
+ Puppet.settings.expects(:value).with(:masterport).returns
"1234"
+
Puppet::Network::HttpPool.expects(:http_instance).with("myserver",
1234).returns "myconn"
+ @searcher.network(@request).should == "myconn"
+ end
+ end
+
+ describe "and the request key is a URL" do
+ it "should use the :server setting and masterport setting, as an
Integer, as the host if no values are provided" do
+ @request = stub 'request', :key => "puppet:///key"
+
+ Puppet.settings.expects(:value).with(:server).returns
"myserver"
+ Puppet.settings.expects(:value).with(:masterport).returns
"1234"
+
Puppet::Network::HttpPool.expects(:http_instance).with("myserver",
1234).returns "myconn"
+ @searcher.network(@request).should == "myconn"
+ end
+
+ it "should use the server if one is provided" do
+ @request.stubs(:key).returns "puppet://host/key"
+
+ Puppet.settings.expects(:value).with(:masterport).returns
"1234"
+ Puppet::Network::HttpPool.expects(:http_instance).with("host",
1234).returns "myconn"
+ @searcher.network(@request).should == "myconn"
+ end
+
+ it "should use the server and port if they are provided" do
+ @request.stubs(:key).returns "puppet://host:123/key"
+
+ Puppet::Network::HttpPool.expects(:http_instance).with("host",
123).returns "myconn"
+ @searcher.network(@request).should == "myconn"
+ end
end
end
@@ -104,7 +133,7 @@ describe Puppet::Indirector::REST do
@connection = stub('mock http connection', :get => @response)
@searcher.stubs(:network).returns(@connection) # neuter the
network connection
- @request = stub 'request', :key => 'foo'
+ @request = stub 'request', :key => 'foo', :options => {}
end
it "should call the GET http method on a network connection" do
@@ -126,6 +155,13 @@ 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" % [EMAIL PROTECTED], "foo"]
+ @connection.expects(:get).with { |path, args| path =~
/\?one=two&three=four$/ }.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)
@@ -151,7 +187,7 @@ describe Puppet::Indirector::REST do
@model.stubs(:convert_from_multiple)
- @request = stub 'request', :key => 'foo'
+ @request = stub 'request', :key => 'foo', :options => {}
end
it "should call the GET http method on a network connection" do
@@ -180,6 +216,14 @@ describe Puppet::Indirector::REST do
@searcher.search(@request)
end
+ it "should include all options in the query string" do
+ @request.stubs(:options).returns(:one => "two", :three => "four")
+
+ should_path = "/%s/%s" % [EMAIL PROTECTED], "foo"]
+ @connection.expects(:get).with { |path, args| path =~
/\?one=two&three=four$/ }.returns(@response)
+ @searcher.search(@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)
@@ -204,7 +248,7 @@ describe Puppet::Indirector::REST do
@connection = stub('mock http connection', :delete => @response)
@searcher.stubs(:network).returns(@connection) # neuter the
network connection
- @request = stub 'request', :key => 'foo'
+ @request = stub 'request', :key => 'foo', :options => {}
end
it "should call the DELETE http method on a network connection" do
@@ -213,6 +257,12 @@ describe Puppet::Indirector::REST do
@searcher.destroy(@request)
end
+ it "should fail if any options are provided, since DELETE apparently
does not support query options" do
+ @request.stubs(:options).returns(:one => "two", :three => "four")
+
+ lambda { @searcher.destroy(@request) }.should
raise_error(ArgumentError)
+ end
+
it "should deserialize and return the http response" do
@connection.expects(:delete).returns @response
@searcher.expects(:deserialize).with(@response).returns "myobject"
@@ -250,7 +300,7 @@ describe Puppet::Indirector::REST do
@searcher.stubs(:network).returns(@connection) # neuter the
network connection
@instance = stub 'instance', :render => "mydata"
- @request = stub 'request', :instance => @instance
+ @request = stub 'request', :instance => @instance, :options => {}
end
it "should call the PUT http method on a network connection" do
@@ -259,6 +309,12 @@ describe Puppet::Indirector::REST do
@searcher.save(@request)
end
+ it "should fail if any options are provided, since DELETE apparently
does not support query options" do
+ @request.stubs(:options).returns(:one => "two", :three => "four")
+
+ lambda { @searcher.save(@request) }.should
raise_error(ArgumentError)
+ end
+
it "should use the indirection name as the path for the request" do
@connection.expects(:put).with { |path, data, args| path ==
"/[EMAIL PROTECTED]/" }.returns @response
diff --git a/spec/unit/network/http/handler.rb
b/spec/unit/network/http/handler.rb
index 816c0ea..6fc9320 100755
--- a/spec/unit/network/http/handler.rb
+++ b/spec/unit/network/http/handler.rb
@@ -302,12 +302,24 @@ describe Puppet::Network::HTTP::Handler do
it "should use a common method for determining the request
parameters" do
@handler.stubs(:params).returns(:foo => :baz, :bar => :xyzzy)
- @model_class.expects(:search).with do |args|
+ @model_class.expects(:search).with do |key, args|
args[:foo] == :baz and args[:bar] == :xyzzy
end.returns @result
@handler.do_search(@request, @response)
end
+ it "should use a request key if one is provided" do
+ @handler.expects(:request_key).with(@request).returns "foo"
+ @model_class.expects(:search).with { |key, args| key == "foo"
}.returns @result
+ @handler.do_search(@request, @response)
+ end
+
+ it "should work with no request key if none is provided" do
+ @handler.expects(:request_key).with(@request).returns nil
+ @model_class.expects(:search).with { |args| args.is_a?(Hash)
}.returns @result
+ @handler.do_search(@request, @response)
+ end
+
it "should use the default status when a model search call
succeeds" do
@model_class.stubs(:search).returns(@result)
@handler.do_search(@request, @response)
diff --git a/spec/unit/network/http/mongrel/rest.rb
b/spec/unit/network/http/mongrel/rest.rb
index 0d4ce36..3b248dc 100755
--- a/spec/unit/network/http/mongrel/rest.rb
+++ b/spec/unit/network/http/mongrel/rest.rb
@@ -58,6 +58,11 @@ describe "Puppet::Network::HTTP::MongrelREST" do
@handler.request_key(@request).should == "bar"
end
+ it "should return nil as the request key if no second field is
present" do
+
@params.expects(:[]).with(Mongrel::Const::REQUEST_PATH).returns "/foo"
+ @handler.request_key(@request).should be_nil
+ end
+
it "should return the request body as the body" do
@request.expects(:body).returns "mybody"
@handler.body(@request).should == "mybody"
diff --git a/spec/unit/network/http/webrick/rest.rb
b/spec/unit/network/http/webrick/rest.rb
index 4c72ec5..6202014 100755
--- a/spec/unit/network/http/webrick/rest.rb
+++ b/spec/unit/network/http/webrick/rest.rb
@@ -52,6 +52,11 @@ describe Puppet::Network::HTTP::WEBrickREST do
@handler.request_key(@request).should == "bar"
end
+ it "should return nil as the request key if there is no second
field" do
+ @request.expects(:path).returns "/foo"
+ @handler.request_key(@request).should be_nil
+ end
+
it "should return the request body as the body" do
@request.expects(:body).returns "my body"
@handler.body(@request).should == "my body"
--
1.5.3.7
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Puppet Developers" group.
To post to this group, send email to [email protected]
To unsubscribe from this group, send email to [EMAIL PROTECTED]
For more options, visit this group at
http://groups.google.com/group/puppet-dev?hl=en
-~----------~----~----~----~------~----~------~--~---
