Avoid calling URI.escape in #url method, and use Paperclip::InterpolatedString

Paperclip::InterpolatedString will ensure that we're not double escape the string, which was the cause of the escaped filename problem in #path method.

lib/paperclip/attachment.rb

@@ -131,7 +131,9 @@ module Paperclip
     def url(style_name = default_style, use_timestamp = @options.use_timestamp)
       default_url = @options.default_url.is_a?(Proc) ? @options.default_url.call(self) : @options.default_url
       url = original_filename.nil? ? interpolate(default_url, style_name) : interpolate(@options.url, style_name)
-      URI.escape(use_timestamp && updated_at ? [url, updated_at].compact.join(url.include?("?") ? "&" : "?") : url)
+
+      url << (url.include?("?") ? "&" : "?") + updated_at.to_s if use_timestamp && updated_at
+      url.respond_to?(:escape) ? url.escape : URI.escape(url)
     end
 
     # Returns the path of the attachment as defined by the :path option. If the
@@ -139,7 +141,8 @@ module Paperclip
     # on disk. If the file is stored in S3, the path is the "key" part of the
     # URL, and the :bucket option refers to the S3 bucket.
     def path(style_name = default_style)
-      original_filename.nil? ? nil : interpolate(@options.path, style_name)
+      path = original_filename.nil? ? nil : interpolate(@options.path, style_name)
+      path.respond_to?(:unescape) ? path.unescape : path
     end
 
     # Alias to +url+

lib/paperclip/interpolations.rb

+require 'paperclip/interpolated_string'
+
 module Paperclip
   # This module contains all the methods that are available for interpolation
   # in paths and urls. To add your own (or override an existing one), you
@@ -29,11 +31,13 @@ module Paperclip
     # an interpolation pattern for Paperclip to use.
     def self.interpolate pattern, *args
       pattern = args.first.instance.send(pattern) if pattern.kind_of? Symbol
-      all.reverse.inject( pattern.dup ) do |result, tag|
+      interpolated_string = all.reverse.inject(InterpolatedString.new(pattern)) do |result, tag|
         result.gsub(/:#{tag}/) do |match|
           send( tag, *args )
         end
       end
+      interpolated_string.force_escape if pattern =~ /:url/
+      interpolated_string
     end
 
     # Returns the filename, the same way as ":basename.:extension" would.

test/storage/filesystem_test.rb

@@ -44,6 +44,10 @@ class FileSystemTest < Test::Unit::TestCase
         assert File.exists?(@dummy.avatar.path)
       end
 
+      should "store the path unescaped" do
+        assert_match /\/spaced file\.png/, @dummy.avatar.path
+      end
+
       should "return an escaped version of URL" do
         assert_match /\/spaced%20file\.png/, @dummy.avatar.url
       end

test/storage/s3_live_test.rb

@@ -25,7 +25,10 @@ unless ENV["S3_TEST_BUCKET"].blank?
           @dummy.avatar = @file
         end
 
-        teardown { @file.close }
+        teardown do
+          @file.close
+          @dummy.destroy
+        end
 
         should "still return a Tempfile when sent #to_file" do
           assert_equal Paperclip::Tempfile, @dummy.avatar.to_file.class
@@ -47,5 +50,39 @@ unless ENV["S3_TEST_BUCKET"].blank?
         end
       end
     end
+
+    context "An attachment that uses S3 for storage and has spaces in file name" do
+      setup do
+        rebuild_model :styles => { :thumb => "100x100", :square => "32x32#" },
+                      :storage => :s3,
+                      :bucket => ENV["S3_TEST_BUCKET"],
+                      :s3_credentials => File.new(File.join(File.dirname(__FILE__), "..", "s3.yml"))
+
+        Dummy.delete_all
+        @dummy = Dummy.new
+        @dummy.avatar = File.new(File.join(File.dirname(__FILE__), '..', 'fixtures', 'spaced file.png'), 'rb')
+        @dummy.save
+      end
+
+      teardown { @dummy.destroy }
+
+      should "return an unescaped version for path" do
+        assert_match /.+\/spaced file\.png/, @dummy.avatar.path
+      end
+
+      should "return an escaped version for url" do
+        assert_match /.+\/spaced%20file\.png/, @dummy.avatar.url
+      end
+
+      should "be accessible" do
+        assert_match /200 OK/, `curl -I #{@dummy.avatar.url}`
+      end
+
+      should "be destoryable" do
+        url = @dummy.avatar.url
+        @dummy.destroy
+        assert_match /404 Not Found/, `curl -I #{url}`
+      end
+    end
   end
 end

test/storage/s3_test.rb

@@ -116,7 +116,6 @@ class S3Test < Test::Unit::TestCase
       rebuild_model :styles  => { :large => ['500x500#', :jpg] },
                     :storage => :s3,
                     :bucket  => "bucket",
-                    :path => ":attachment/:basename.:extension",
                     :s3_credentials => {
                       'access_key_id' => "12345",
                       'secret_access_key' => "54321"
@@ -126,7 +125,11 @@ class S3Test < Test::Unit::TestCase
       @dummy.avatar = File.new(File.join(File.dirname(__FILE__), '..', 'fixtures', 'spaced file.png'), 'rb')
     end
 
-    should "return an escaped version of url" do
+    should "return an unescaped version for path" do
+      assert_match /.+\/spaced file\.png/, @dummy.avatar.path
+    end
+
+    should "return an escaped version for url" do
       assert_match /.+\/spaced%20file\.png/, @dummy.avatar.url
     end
   end