REST: hide Request object

This change to the REST branch restores some sanity by explicitly
allowing a destination URL for indirector save() calls,
removing a hack that I was using to accomplish this.

17 files changed, 42 insertions, 52 deletions

diff --git a/lib/puppet/application/resource.rb b/lib/puppet/application/resource.rb
index 0046fc1d8..ae4349850 100644
--- a/lib/puppet/application/resource.rb
+++ b/lib/puppet/application/resource.rb
@@ -83,8 +83,7 @@ Puppet::Application.new(:resource) do
             if params.empty?
                 [ Puppet::Resource.find( key ) ]
             else
-                request = Puppet::Indirector::Request.new(:resource, :save, key) # Yuck.
-                [ Puppet::Resource.new( type, name, params ).save( request ) ]
+                [ Puppet::Resource.new( type, name, params ).save( key ) ]
             end
         else
             Puppet::Resource.search( key, {} )
diff --git a/lib/puppet/application/run.rb b/lib/puppet/application/run.rb
index 26ca362ff..46d1b191f 100644
--- a/lib/puppet/application/run.rb
+++ b/lib/puppet/application/run.rb
@@ -120,13 +120,12 @@ Puppet::Application.new(:run) do
 
         print "Triggering %s\n" % host
         begin
-            request = Puppet::Indirector::Request.new(:run, :save, url) # Yuck.
             run_options = {
                 :tags => @tags,
                 :background => ! options[:foreground],
                 :ignoreschedules => options[:ignoreschedules]
             }
-            run = Puppet::Run.new( run_options ).save( request )
+            run = Puppet::Run.new( run_options ).save( url )
             result = run.status
         rescue => detail
             puts detail.backtrace if Puppet[:trace]
diff --git a/lib/puppet/file_bucket/dipper.rb b/lib/puppet/file_bucket/dipper.rb
index c73d76345..b13e590a9 100644
--- a/lib/puppet/file_bucket/dipper.rb
+++ b/lib/puppet/file_bucket/dipper.rb
@@ -38,9 +38,7 @@ class Puppet::FileBucket::Dipper
             file_bucket_file = Puppet::FileBucket::File.new(contents, :bucket_path => @local_path, :path => file)
             dest_path = "#{@rest_path}#{file_bucket_file.name}"
 
-            request = Puppet::Indirector::Request.new(:file_bucket_file, :save, dest_path)
-
-            file_bucket_file.save(request)
+            file_bucket_file.save(dest_path)
             return file_bucket_file.checksum_data
         rescue => detail
             puts detail.backtrace if Puppet[:trace]
diff --git a/lib/puppet/indirector.rb b/lib/puppet/indirector.rb
index 61ef2db29..91c759331 100644
--- a/lib/puppet/indirector.rb
+++ b/lib/puppet/indirector.rb
@@ -61,8 +61,8 @@ module Puppet::Indirector
     end
 
     module InstanceMethods
-        def save(*args)
-            self.class.indirection.save self, *args
+        def save(key = nil)
+            self.class.indirection.save key, self
         end
     end
 end
diff --git a/lib/puppet/indirector/indirection.rb b/lib/puppet/indirector/indirection.rb
index 3c6414624..266758b84 100644
--- a/lib/puppet/indirector/indirection.rb
+++ b/lib/puppet/indirector/indirection.rb
@@ -116,14 +116,8 @@ class Puppet::Indirector::Indirection
     end
 
     # Set up our request object.
-    def request(method, instance_or_key, request_or_options = {})
-        if request_or_options.is_a? Puppet::Indirector::Request
-            request = request_or_options
-            request.instance = instance_or_key
-            request.method   = method
-            return request
-        end
-        Puppet::Indirector::Request.new(self.name, method, instance_or_key, request_or_options)
+    def request(*args)
+        Puppet::Indirector::Request.new(self.name, *args)
     end
 
     # Return the singleton terminus for this indirection.
@@ -254,8 +248,8 @@ class Puppet::Indirector::Indirection
 
     # Save the instance in the appropriate terminus.  This method is
     # normally an instance method on the indirected class.
-    def save(instance, request_or_options = nil)
-        request = request(:save, instance, request_or_options)
+    def save(key, instance = nil)
+        request = request(:save, key, instance)
         terminus = prepare(request)
 
         result = terminus.save(request)
diff --git a/lib/puppet/indirector/request.rb b/lib/puppet/indirector/request.rb
index 14608d0dc..cd354ac16 100644
--- a/lib/puppet/indirector/request.rb
+++ b/lib/puppet/indirector/request.rb
@@ -52,9 +52,14 @@ class Puppet::Indirector::Request
         ignore_terminus
     end
 
-    def initialize(indirection_name, method, key, options = {})
-        options ||= {}
-        raise ArgumentError, "Request options must be a hash, not %s" % options.class unless options.is_a?(Hash)
+    def initialize(indirection_name, method, key_or_instance, options_or_instance = {})
+        if options_or_instance.is_a? Hash
+            options = options_or_instance
+            @instance = nil
+        else
+            options  = {}
+            @instance = options_or_instance
+        end
 
         self.indirection_name = indirection_name
         self.method = method
@@ -63,7 +68,13 @@ class Puppet::Indirector::Request
 
         @options = options.inject({}) { |hash, ary| hash[ary[0].to_sym] = ary[1]; hash }
 
-        if key.is_a?(String) or key.is_a?(Symbol)
+        if key_or_instance.is_a?(String) || key_or_instance.is_a?(Symbol)
+            key = key_or_instance
+        else
+            @instance = key_or_instance if ! @instance
+        end
+
+        if key
             # If the request key is a URI, then we need to treat it specially,
             # because it rewrites the key.  We could otherwise strip server/port/etc
             # info out in the REST class, but it seemed bad design for the REST
@@ -73,10 +84,9 @@ class Puppet::Indirector::Request
             else
                 @key = key
             end
-        else
-            @instance = key
-            @key = @instance.name
         end
+
+        @key = @instance.name if ! @key and @instance
     end
 
     # Look up the indirection based on the name provided.
diff --git a/lib/puppet/network/http/handler.rb b/lib/puppet/network/http/handler.rb
index 01ca65023..ab853665c 100644
--- a/lib/puppet/network/http/handler.rb
+++ b/lib/puppet/network/http/handler.rb
@@ -165,7 +165,7 @@ module Puppet::Network::HTTP::Handler
     # LAK:NOTE This has to be here for testing; it's a stub-point so
     # we keep infinite recursion from happening.
     def save_object(ind_request, object)
-        object.save(ind_request)
+        object.save(ind_request.key)
     end
 
     def get?(request)
diff --git a/lib/puppet/node/facts.rb b/lib/puppet/node/facts.rb
index dca435c7d..ed7fe1253 100755
--- a/lib/puppet/node/facts.rb
+++ b/lib/puppet/node/facts.rb
@@ -10,7 +10,7 @@ class Puppet::Node::Facts
 
     # We want to expire any cached nodes if the facts are saved.
     module NodeExpirer
-        def save(instance, *args)
+        def save(key, instance)
             Puppet::Node.expire(instance.name)
             super
         end
diff --git a/spec/integration/configurer.rb b/spec/integration/configurer.rb
index 7bd351aa1..fce24a1b2 100755
--- a/spec/integration/configurer.rb
+++ b/spec/integration/configurer.rb
@@ -23,7 +23,7 @@ describe Puppet::Configurer do
 
             configurer = Puppet::Configurer.new
 
-            Puppet::Transaction::Report.indirection.expects(:save).with do |report|
+            Puppet::Transaction::Report.indirection.expects(:save).with do |x, report|
                 report.time.class == Time and report.logs.length > 0
             end
 
diff --git a/spec/unit/application/puppetrun.rb b/spec/unit/application/puppetrun.rb
index ce3af5a5b..f8ca48061 100755
--- a/spec/unit/application/puppetrun.rb
+++ b/spec/unit/application/puppetrun.rb
@@ -258,7 +258,7 @@ describe "run" do
                 end
 
                 it "should call run on a Puppet::Run for the given host" do
-                    @run.expects(:save).with{|req| req.uri == 'https://host:8139/production/run/host'}.returns(@run)
+                    @run.expects(:save).with('https://host:8139/production/run/host').returns(@run)
 
                     @run.run_for_host('host')
                 end
diff --git a/spec/unit/application/resource.rb b/spec/unit/application/resource.rb
index 9d47ba56e..ebb8c4f3e 100755
--- a/spec/unit/application/resource.rb
+++ b/spec/unit/application/resource.rb
@@ -202,7 +202,7 @@ describe "resource" do
                 push_args('type','name','param=temp')
 
                 res = stub "resource"
-                res.expects(:save).with{|x| x.uri == 'https://host:8139/production/resources/type/name'}.returns(res)
+                res.expects(:save).with('https://host:8139/production/resources/type/name').returns(res)
                 res.expects(:collect)
                 res.expects(:to_manifest)
                 Puppet::Resource.expects(:new).with('type', 'name', {'param' => 'temp'}).returns(res)
@@ -240,7 +240,7 @@ describe "resource" do
                 push_args('type','name','param=temp')
 
                 res = stub "resource"
-                res.expects(:save).with{|x| x.uri == nil}.returns(res)
+                res.expects(:save).with('type/name').returns(res)
                 res.expects(:collect)
                 res.expects(:to_manifest)
                 Puppet::Resource.expects(:new).with('type', 'name', {'param' => 'temp'}).returns(res)
diff --git a/spec/unit/file_bucket/dipper.rb b/spec/unit/file_bucket/dipper.rb
index ffa2d4576..5d6625103 100755
--- a/spec/unit/file_bucket/dipper.rb
+++ b/spec/unit/file_bucket/dipper.rb
@@ -26,11 +26,10 @@ describe Puppet::FileBucket::Dipper do
         File.stubs(:exist?).returns true
         File.stubs(:read).with("/my/file").returns "my contents"
 
-        req = stub "req"
         bucketfile = stub "bucketfile"
         bucketfile.stubs(:name).returns('md5/DIGEST123')
         bucketfile.stubs(:checksum_data).returns("DIGEST123")
-        bucketfile.expects(:save).with(req)
+        bucketfile.expects(:save).with('md5/DIGEST123')
 
         Puppet::FileBucket::File.stubs(:new).with(
             "my contents",
@@ -38,8 +37,6 @@ describe Puppet::FileBucket::Dipper do
             :path => '/my/file'
         ).returns(bucketfile)
 
-        Puppet::Indirector::Request.stubs(:new).with(:file_bucket_file, :save, 'md5/DIGEST123').returns(req)
-
         @dipper.backup("/my/file").should == "DIGEST123"
     end
 
@@ -70,11 +67,10 @@ describe Puppet::FileBucket::Dipper do
         File.stubs(:exist?).returns true
         File.stubs(:read).with("/my/file").returns "my contents"
 
-        req = stub "req"
         bucketfile = stub "bucketfile"
         bucketfile.stubs(:name).returns('md5/DIGEST123')
         bucketfile.stubs(:checksum_data).returns("DIGEST123")
-        bucketfile.expects(:save).with(req)
+        bucketfile.expects(:save).with('https://puppetmaster:31337/production/file_bucket_file/md5/DIGEST123')
 
         Puppet::FileBucket::File.stubs(:new).with(
             "my contents",
@@ -82,8 +78,6 @@ describe Puppet::FileBucket::Dipper do
             :path => '/my/file'
         ).returns(bucketfile)
 
-        Puppet::Indirector::Request.stubs(:new).with(:file_bucket_file, :save, 'https://puppetmaster:31337/production/file_bucket_file/md5/DIGEST123').returns(req)
-
         @dipper.backup("/my/file").should == "DIGEST123"
     end
 
diff --git a/spec/unit/indirector.rb b/spec/unit/indirector.rb
index 0b4c61695..cb1b04932 100755
--- a/spec/unit/indirector.rb
+++ b/spec/unit/indirector.rb
@@ -129,9 +129,9 @@ describe Puppet::Indirector, "when redirecting a model" do
             @instance.save
         end
 
-        it "should pass the instance and all arguments to the indirection's :save method" do
-            @indirection.expects(:save).with(@instance, :one => :two)
-            @instance.save :one => :two
+        it "should pass the instance and an optional key to the indirection's :save method" do
+            @indirection.expects(:save).with("key", @instance)
+            @instance.save "key"
         end
 
         it "should return the results of the delegation as its result" do
diff --git a/spec/unit/indirector/indirection.rb b/spec/unit/indirector/indirection.rb
index 02d04a3f7..0663fe55e 100755
--- a/spec/unit/indirector/indirection.rb
+++ b/spec/unit/indirector/indirection.rb
@@ -198,8 +198,8 @@ describe Puppet::Indirector::Indirection do
                 @indirection.request(:funtest, "yayness", :one => :two)
             end
 
-            it "should default to the arguments being empty" do
-                Puppet::Indirector::Request.expects(:new).with { |name, method, key, args| args == {} }
+            it "should not pass options if none are supplied" do
+                Puppet::Indirector::Request.expects(:new).with { |*args| args.length < 4 }
                 @indirection.request(:funtest, "yayness")
             end
 
diff --git a/spec/unit/indirector/request.rb b/spec/unit/indirector/request.rb
index b885779ed..58226397e 100755
--- a/spec/unit/indirector/request.rb
+++ b/spec/unit/indirector/request.rb
@@ -40,10 +40,6 @@ describe Puppet::Indirector::Request do
             lambda { Puppet::Indirector::Request.new(:ind, :method, :key) }.should_not raise_error(ArgumentError)
         end
 
-        it "should fail if options are specified as anything other than nil or a hash" do
-            lambda { Puppet::Indirector::Request.new(:ind, :method, :key, [:one, :two]) }.should raise_error(ArgumentError)
-        end
-
         it "should use an empty options hash if nil was provided" do
             Puppet::Indirector::Request.new(:ind, :method, :key, nil).options.should == {}
         end
diff --git a/spec/unit/network/http/handler.rb b/spec/unit/network/http/handler.rb
index 2218a0a78..e6dd88130 100755
--- a/spec/unit/network/http/handler.rb
+++ b/spec/unit/network/http/handler.rb
@@ -402,7 +402,7 @@ describe Puppet::Network::HTTP::Handler do
             end
 
             it "should use a common method for determining the request parameters" do
-                @model_instance.expects(:save).with(@irequest)
+                @model_instance.expects(:save).with('key').once
                 @handler.do_save(@irequest, @request, @response)
             end
 
diff --git a/spec/unit/ssl/certificate_request.rb b/spec/unit/ssl/certificate_request.rb
index a4eee92d6..63bc9f110 100755
--- a/spec/unit/ssl/certificate_request.rb
+++ b/spec/unit/ssl/certificate_request.rb
@@ -200,7 +200,7 @@ describe Puppet::SSL::CertificateRequest do
                 Puppet::SSL::CertificateAuthority.expects(:instance).returns ca
 
                 csr = Puppet::SSL::CertificateRequest.new("me")
-                Puppet::SSL::CertificateRequest.indirection.expects(:save).with(csr)
+                Puppet::SSL::CertificateRequest.indirection.expects(:save).with(nil, csr)
 
                 csr.save
             end
@@ -211,7 +211,7 @@ describe Puppet::SSL::CertificateRequest do
                 Puppet::SSL::CertificateAuthority.expects(:instance).returns nil
 
                 csr = Puppet::SSL::CertificateRequest.new("me")
-                Puppet::SSL::CertificateRequest.indirection.expects(:save).with(csr)
+                Puppet::SSL::CertificateRequest.indirection.expects(:save).with(nil, csr)
 
                 csr.save
             end