always re-raise on argument exception, refactor into smaller methods

2 files changed, 87 insertions, 60 deletions

diff --git a/lib/chef/provider/remote_file.rb b/lib/chef/provider/remote_file.rb
index 81dfb77000..d87715fece 100644
--- a/lib/chef/provider/remote_file.rb
+++ b/lib/chef/provider/remote_file.rb
@@ -40,54 +40,23 @@ class Chef
           Chef::Log.debug("#{@new_resource} checksum matches target checksum (#{@new_resource.checksum}) - not updating")
         else
           sources = @new_resource.source
-          source = sources.shift
-          begin
-            uri = URI.parse(source)
-            if URI::HTTP === uri
-              #HTTP or HTTPS
-              raw_file = RestClient::Request.execute(:method => :get, :url => source, :raw_response => true).file
-            elsif URI::FTP === uri
-              #FTP
-              raw_file = FTP::fetch(uri, @new_resource.ftp_active_mode)
-            elsif uri.scheme == "file"
-              #local/network file
-              raw_file = ::File.new(uri.path, "r")
-            else
-              raise ArgumentError, "Invalid uri. Only http(s), ftp, and file are currently supported"
-            end
-          rescue => e
-            Chef::Log.debug("#{@new_resource} cannot be downloaded from #{source}")
-            if source = sources.shift
-              Chef::Log.debug("#{@new_resource} trying to download from another mirror")
-              retry
-            else
-              raise e
-            end
-          end
+          raw_file = try_multiple_sources(sources)
           if matches_current_checksum?(raw_file)
             Chef::Log.debug "#{@new_resource} target and source checksums are the same - not updating"
           else
             description = [] 
-            description << "copy file downloaded from #{source} into #{@new_resource.path}"
+            description << "copy file downloaded from #{@new_resource.source} into #{@new_resource.path}"
             description << diff_current(raw_file.path)
             converge_by(description) do
               backup_new_resource
               FileUtils.cp raw_file.path, @new_resource.path
               Chef::Log.info "#{@new_resource} updated"
-              if raw_file.is_a? Tempfile
-                raw_file.close!
-              else
-                raw_file.close
-              end
+              raw_file.close!
             end
             # whyrun mode cleanup - the temp file will never be used,
             # so close/unlink it here. 
             if whyrun_mode?
-              if raw_file.is_a? Tempfile
-                raw_file.close!
-              else
-                raw_file.close
-              end
+              raw_file.close!
             end
           end
         end
@@ -120,6 +89,53 @@ class Chef
           backup @new_resource.path
         end
       end
+
+      private
+
+      # Given an array of source uris, iterate through them until one does not fail
+      def try_multiple_sources(sources)
+        source = sources.shift
+        begin
+          uri = URI.parse(source)
+          raw_file = grab_file_from_uri(uri)
+        rescue ArgumentError => e
+          raise e
+        rescue => e
+          Chef::Log.debug("#{@new_resource} cannot be downloaded from #{source}")
+          if source = sources.shift
+            Chef::Log.debug("#{@new_resource} trying to download from another mirror")
+            retry
+          else
+            raise e
+          end
+        end
+        if uri.userinfo
+          uri.password = "********"
+        end
+        @new_resource.source uri.to_s
+        raw_file
+      end
+
+      # Given a source uri, return a Tempfile, or a File that acts like a Tempfile (close! method)
+      def grab_file_from_uri(uri)
+        if URI::HTTP === uri
+          #HTTP or HTTPS
+          raw_file = RestClient::Request.execute(:method => :get, :url => uri.to_s, :raw_response => true).file
+        elsif URI::FTP === uri
+          #FTP
+          raw_file = FTP::fetch(uri, @new_resource.ftp_active_mode)
+        elsif uri.scheme == "file"
+          #local/network file
+          raw_file = ::File.new(uri.path, "r")
+          def raw_file.close!
+            self.close
+          end
+        else
+          raise ArgumentError, "Invalid uri. Only http(s), ftp, and file are currently supported"
+        end
+        raw_file
+      end
+
     end
   end
 end
diff --git a/lib/chef/provider/remote_file/ftp.rb b/lib/chef/provider/remote_file/ftp.rb
index 995fb0ba72..d2ebbc5faa 100644
--- a/lib/chef/provider/remote_file/ftp.rb
+++ b/lib/chef/provider/remote_file/ftp.rb
@@ -27,50 +27,61 @@ class Chef
       class FTP
 
         # Fetches the file at uri using Net::FTP, returning a Tempfile
-        # Taken from open-uri
         def self.fetch(uri, ftp_active_mode)
-          path = uri.path
-          path = path.sub(%r{\A/}, '%2F') # re-encode the beginning slash because uri library decodes it.
-          directories = path.split(%r{/}, -1)
-          directories.each {|d|
+          self.new(uri, ftp_active_mode).fetch()
+        end
+
+        # Parse the uri into instance variables
+        def initialize(uri, ftp_active_mode)
+          @path = uri.path.sub(%r{\A/}, '%2F') # re-encode the beginning slash because uri library decodes it.
+          @directories = @path.split(%r{/}, -1)
+          @directories.each {|d|
             d.gsub!(/%([0-9A-Fa-f][0-9A-Fa-f])/) { [$1].pack("H2") }
           }
-          unless filename = directories.pop
+          unless @filename = @directories.pop
             raise ArgumentError, "no filename: #{uri.inspect}"
           end
-          if filename.length == 0 || filename.end_with?( "/" )
+          if @filename.length == 0 || @filename.end_with?( "/" )
             raise ArgumentError, "no filename: #{uri.inspect}"
           end
-          typecode = uri.typecode
+          @typecode = uri.typecode
           # Only support ascii and binary types
-          if typecode && /\A[ai]\z/ !~ typecode
-            raise ArgumentError, "invalid typecode: #{typecode.inspect}"
+          if @typecode && /\A[ai]\z/ !~ @typecode
+            raise ArgumentError, "invalid typecode: #{@typecode.inspect}"
           end
+          @ftp_active_mode = ftp_active_mode
+          @hostname = uri.hostname
+          @port = uri.port
+          if uri.userinfo
+            @user = URI.unescape(uri.user)
+            @pass = URI.unescape(uri.password)
+          else
+            @user = 'anonymous'
+            @pass = nil
+          end
+        end
 
-          tempfile = Tempfile.new(filename)
+        # Fetches using Net::FTP, returns a Tempfile with the content
+        def fetch()
+          tempfile = Tempfile.new(@filename)
 
           # The access sequence is defined by RFC 1738
           ftp = Net::FTP.new
-          ftp.connect(uri.hostname, uri.port)
-          ftp.passive = !ftp_active_mode
-          user = 'anonymous'
-          passwd = nil
-          if uri.userinfo
-            user = URI.unescape(uri.user)
-            passwd = URI.unescape(uri.password)
-          end
-          ftp.login(user, passwd)
-          directories.each {|cwd|
+          ftp.connect(@hostname, @port)
+          ftp.passive = !@ftp_active_mode
+          ftp.login(@user, @pass)
+          @directories.each do |cwd|
             ftp.voidcmd("CWD #{cwd}")
-          }
-          if typecode
-            ftp.voidcmd("TYPE #{typecode.upcase}")
           end
-          ftp.getbinaryfile(filename, tempfile.path)
+          if @typecode
+            ftp.voidcmd("TYPE #{@typecode.upcase}")
+          end
+          ftp.getbinaryfile(@filename, tempfile.path)
           ftp.close
 
           tempfile
         end
+
       end
     end
   end