Fixed the bug where files were not being deleted except after assignment.

Added S3 support.


git-svn-id: https://svn.thoughtbot.com/plugins/paperclip/trunk@442 7bbfaf0e-4d1d-0410-9690-a8bb5f8ef2aa

lib/paperclip.rb

@@ -30,6 +30,7 @@ require 'paperclip/upfile'
 require 'paperclip/iostream'
 require 'paperclip/geometry'
 require 'paperclip/thumbnail'
+require 'paperclip/storage'
 require 'paperclip/attachment'
 
 # The base module that gets included in ActiveRecord::Base.
@@ -181,8 +182,7 @@ module Paperclip
 
     def save_attached_files
       each_attachment do |name, attachment|
-        attachment.send(:flush_writes)
-        attachment.send(:flush_deletes)
+        attachment.send(:save)
       end
     end
 

lib/paperclip/attachment.rb

@@ -10,7 +10,8 @@ module Paperclip
         :styles        => {},
         :default_url   => "/:attachment/:style/missing.png",
         :default_style => :original,
-        :validations   => []
+        :validations   => [],
+        :storage       => :filesystem
       }
     end
 
@@ -31,15 +32,22 @@ module Paperclip
       @default_url       = options[:default_url]
       @validations       = options[:validations]
       @default_style     = options[:default_style]
+      @storage           = options[:storage]
+      @options           = options
       @queued_for_delete = []
       @processed_files   = {}
       @errors            = []
+      @file              = nil
       @validation_errors = nil
       @dirty             = false
 
       normalize_style_definition
+      initialize_storage
 
-      @file              = File.new(path) if original_filename && File.exists?(path)
+      if original_filename
+        @processed_files = locate_files
+        @file            = @processed_files[@default_style]
+      end
     end
 
     # What gets called when you call instance.attachment = File. It clears errors,
@@ -101,6 +109,8 @@ module Paperclip
       if valid?
         flush_deletes
         flush_writes
+        @dirty = false
+        @file = @processed_files[default_style]
         true
       else
         flush_errors
@@ -108,17 +118,14 @@ module Paperclip
       end
     end
 
-    # Returns an +IO+ representing the data of the file assigned to the given
-    # style. Useful for streaming with +send_file+.
-    def to_io style = nil
-      begin
-        style ||= default_style
-        @processed_files[style] || File.new(path(style))
-      rescue Errno::ENOENT
-        nil
-      end
+    # Returns representation of the data of the file assigned to the given
+    # style, in the format most representative of the current storage.
+    def to_file style = nil
+      @processed_files[style || default_style]
     end
 
+    alias_method :to_io, :to_file
+
     # Returns the name of the file as originally assigned, and as lives in the
     # <attachment>_file_name attribute of the model.
     def original_filename
@@ -173,6 +180,11 @@ module Paperclip
       end
     end
 
+    def initialize_storage
+      @storage_module = Paperclip::Storage.const_get(@storage.to_s.capitalize)
+      self.extend(@storage_module)
+    end
+
     def post_process #:nodoc:
       return nil if @file.nil?
       @styles.each do |name, args|
@@ -183,7 +195,7 @@ module Paperclip
                                                   format, 
                                                   @whiny_thumnails)
         rescue Errno::ENOENT  => e
-          @errors << "could not be processed because the filedoes not exist."
+          @errors << "could not be processed because the file does not exist."
         rescue PaperclipError => e
           @errors << e.message
         end
@@ -199,7 +211,7 @@ module Paperclip
           l.call( self, style )
         end
       end
-      pattern.gsub(%r{/+}, "/")
+      pattern
     end
 
     def path style = nil #:nodoc:
@@ -221,24 +233,6 @@ module Paperclip
       end
     end
 
-    def flush_writes #:nodoc:
-      @processed_files.each do |style, file|
-        FileUtils.mkdir_p( File.dirname(path(style)) )
-        @processed_files[style] = file.stream_to(path(style)) unless file.path == path(style)
-      end
-      @file = @processed_files[default_style]
-    end
-
-    def flush_deletes #:nodoc:
-      @queued_for_delete.compact.each do |file|
-        begin
-          FileUtils.rm(file.path)
-        rescue Errno::ENOENT => e
-          # ignore them
-        end
-      end
-      @queued_for_delete = []
-    end
   end
 end
 

lib/paperclip/storage.rb

+module Paperclip
+  module Storage
+
+    # With the Filesystem module installed, @file and @processed_files are just File instances.
+    module Filesystem
+      def self.extended base
+      end
+
+      def locate_files
+        [:original, *@styles.keys].uniq.inject({}) do |files, style|
+          files[style] = File.new(path(style), "rb") if File.exist?(path(style))
+          files
+        end
+      end
+
+      def flush_writes #:nodoc:
+        @processed_files.each do |style, file|
+          FileUtils.mkdir_p( File.dirname(path(style)) )
+          @processed_files[style] = file.stream_to(path(style)) unless file.path == path(style)
+        end
+      end
+
+      def flush_deletes #:nodoc:
+        @queued_for_delete.compact.each do |file|
+          begin
+            FileUtils.rm(file.path)
+          rescue Errno::ENOENT => e
+            # ignore them
+          end
+        end
+        @queued_for_delete = []
+      end
+    end
+
+    # With the S3 module included, @file and the @processed_files will be
+    # RightAws::S3::Key instances.
+    module S3
+      def self.extended base
+        require 'right_aws'
+        base.instance_eval do
+          @bucket             = @options[:bucket]
+          @s3_credentials     = parse_credentials(@options[:s3_credentials])
+          @s3_options         = {:creds => "public-read"}.merge(@options[:s3_options] || {})
+          @s3_permissions = @s3_options.delete(:creds)
+
+          @s3                 = RightAws::S3.new(@s3_credentials['access_key_id'],
+                                                 @s3_credentials['secret_access_key'],
+                                                 @s3_options)
+          @s3_bucket          = @s3.bucket(@bucket, true, @s3_permissions)
+          @url                = ":s3_url"
+        end
+        base.class.interpolations[:s3_url] = lambda do |attachment, style|
+          attachment.to_io(style).public_link
+        end
+      end
+
+      def parse_credentials creds
+        case creds
+        when File:
+          YAML.load_file(creds.path)
+        when String:
+          YAML.load_file(creds)
+        when Hash:
+          creds
+        else
+          raise ArgumentError, "Credentials are not a path, file, or hash."
+        end
+      end
+
+      def locate_files
+        [:original, *@styles.keys].uniq.inject({}) do |files, style|
+          files[style] = @s3_bucket.key(path(style))
+          files
+        end
+      end
+
+      def flush_writes #:nodoc:
+        return if not dirty?
+        @processed_files.each do |style, key|
+          begin
+            unless key.is_a? RightAws::S3::Key
+              saved_data = key
+              key = @processed_files[style] = @s3_bucket.key(path(style))
+              key.data = saved_data
+            end
+            key.put(nil, @s3_permissions)
+          rescue RightAws::AwsError => e
+            @processed_files[style] = nil
+            raise
+          end
+        end
+      end
+
+      def flush_deletes #:nodoc:
+        @queued_for_delete.compact.each do |file|
+          begin
+            file.delete
+          rescue RightAws::AwsError
+            # Ignore this.
+          end
+        end
+        @queued_for_delete = []
+      end
+
+    end
+  end
+end

test/test_attachment.rb

@@ -8,7 +8,7 @@ class AttachmentTest < Test::Unit::TestCase
   context "Attachment default_options" do
     setup do
       rebuild_model
-      @old_default_options = Paperclip::Attachment.default_options
+      @old_default_options = Paperclip::Attachment.default_options.dup
       @new_default_options = @old_default_options.merge({
         :path => "argle/bargle",
         :url => "fooferon",
@@ -16,6 +16,10 @@ class AttachmentTest < Test::Unit::TestCase
       })
     end
 
+    teardown do
+      Paperclip::Attachment.default_options.merge! @old_default_options
+    end
+
     should "be overrideable" do
       Paperclip::Attachment.default_options.merge!(@new_default_options)
       @new_default_options.keys.each do |key|
@@ -146,7 +150,7 @@ class AttachmentTest < Test::Unit::TestCase
           end
 
           should "have #file be equal #to_io(:original)" do
-            assert @attachment.file == @attachment.to_io(:original)
+            assert_equal @attachment.file, @attachment.to_io(:original)
           end
 
           should "still have its #file attribute not be nil" do
@@ -155,5 +159,15 @@ class AttachmentTest < Test::Unit::TestCase
         end
       end
     end
+
+    context "when trying a nonexistant storage type" do
+      setup do
+        rebuild_model :storage => :not_here
+      end
+
+      should "not be able to find the module" do
+        assert_raise(NameError){ Dummy.new.avatar }
+      end
+    end
   end
 end

test/test_integration.rb

 require 'test/helper.rb'
 
-class PaperclipTest < Test::Unit::TestCase
-  context "A model with an attachment" do
+class IntegrationTest < Test::Unit::TestCase
+  context "A model with a filesystem attachment" do
     setup do
       rebuild_model :styles => { :large => "300x300>",
                                  :medium => "100x100",
@@ -9,9 +9,6 @@ class PaperclipTest < Test::Unit::TestCase
                     :default_style => :medium,
                     :url => "/:attachment/:class/:style/:id/:basename.:extension",
                     :path => ":rails_root/tmp/:attachment/:class/:style/:id/:basename.:extension"
-    end
-
-    should "integrate" do
       @dummy     = Dummy.new
       @file      = File.new(File.join(FIXTURES_DIR, "5k.png"))
       @bad_file  = File.new(File.join(FIXTURES_DIR, "bad.png"))
@@ -19,24 +16,26 @@ class PaperclipTest < Test::Unit::TestCase
       assert @dummy.avatar = @file
       assert @dummy.valid?
       assert @dummy.save
+    end
 
+    should "write and delete its files" do
       [["100x15", nil],
        ["434x66", :original],
        ["300x46", :large],
        ["100x15", :medium],
        ["32x32", :thumb]].each do |geo, style|
-        cmd = %Q[identify -format "%wx%h" #{@dummy.avatar.to_io(style).path}]
+        cmd = %Q[identify -format "%wx%h" #{@dummy.avatar.to_file(style).path}]
         assert_equal geo, `#{cmd}`.chomp, cmd
       end
 
-      saved_paths = [:thumb, :medium, :large, :original].collect{|s| @dummy.avatar.to_io(s).path }
+      saved_paths = [:thumb, :medium, :large, :original].collect{|s| @dummy.avatar.to_file(s).path }
 
       @d2 = Dummy.find(@dummy.id)
-      assert_equal "100x15", `identify -format "%wx%h" #{@dummy.avatar.to_io.path}`.chomp
-      assert_equal "434x66", `identify -format "%wx%h" #{@dummy.avatar.to_io(:original).path}`.chomp
-      assert_equal "300x46", `identify -format "%wx%h" #{@d2.avatar.to_io(:large).path}`.chomp
-      assert_equal "100x15", `identify -format "%wx%h" #{@d2.avatar.to_io(:medium).path}`.chomp
-      assert_equal "32x32",  `identify -format "%wx%h" #{@d2.avatar.to_io(:thumb).path}`.chomp
+      assert_equal "100x15", `identify -format "%wx%h" #{@d2.avatar.to_file.path}`.chomp
+      assert_equal "434x66", `identify -format "%wx%h" #{@d2.avatar.to_file(:original).path}`.chomp
+      assert_equal "300x46", `identify -format "%wx%h" #{@d2.avatar.to_file(:large).path}`.chomp
+      assert_equal "100x15", `identify -format "%wx%h" #{@d2.avatar.to_file(:medium).path}`.chomp
+      assert_equal "32x32",  `identify -format "%wx%h" #{@d2.avatar.to_file(:thumb).path}`.chomp
 
       @dummy.avatar = "not a valid file but not nil"
       assert_equal File.basename(@file.path), @dummy.avatar_file_name
@@ -58,21 +57,48 @@ class PaperclipTest < Test::Unit::TestCase
 
       @d2 = Dummy.find(@dummy.id)
       assert_nil @d2.avatar_file_name
+    end
+
+    should "work exactly the same when new as when reloaded" do
+      @d2 = Dummy.find(@dummy.id)
+
+      assert_equal @dummy.avatar_file_name, @d2.avatar_file_name
+      [:thumb, :medium, :large, :original].each do |style|
+        assert_equal @dummy.avatar.to_file(style).path, @d2.avatar.to_file(style).path
+      end
+
+      saved_paths = [:thumb, :medium, :large, :original].collect{|s| @dummy.avatar.to_file(s).path }
 
-      @d2.avatar = @bad_file
-      assert ! @d2.valid?
       @d2.avatar = nil
-      assert @d2.valid?
+      assert @d2.save
+
+      saved_paths.each do |p|
+        assert ! File.exists?(p)
+      end
+    end
+
+    should "know the difference between good files, bad files, not files, and nil" do
+      expected = @dummy.avatar.file
+      @dummy.avatar = "not a file"
+      assert @dummy.valid?
+      assert_equal expected.path, @dummy.avatar.file.path
+
+      @dummy.avatar = @bad_file
+      assert ! @dummy.valid?
+      @dummy.avatar = nil
+      assert @dummy.valid?
 
       Dummy.validates_attachment_presence :avatar
-      @d3 = Dummy.find(@d2.id)
-      @d3.avatar = @file
-      assert   @d3.valid?
-      @d3.avatar = @bad_file
-      assert ! @d3.valid?
-      @d3.avatar = nil
-      assert ! @d3.valid?
+      @d2 = Dummy.find(@dummy.id)
+      @d2.avatar = @file
+      assert   @d2.valid?
+      @d2.avatar = @bad_file
+      assert ! @d2.valid?
+      @d2.avatar = nil
+      assert ! @d2.valid?
+    end
 
+    should "be able to reload without saving an not have the file disappear" do
       @dummy.avatar = @file
       assert @dummy.save
       @dummy.avatar = nil
@@ -81,5 +107,132 @@ class PaperclipTest < Test::Unit::TestCase
       assert_equal "5k.png", @dummy.avatar_file_name
     end
   end
+
+  if ENV['S3_TEST_BUCKET']
+    def s3_files_for attachment
+      [:thumb, :medium, :large, :original].inject({}) do |files, style|
+        data = `curl '#{attachment.url(style)}' 2>/dev/null`.chomp
+        t = Tempfile.new("paperclip-test")
+        t.write(data)
+        t.rewind
+        files[style] = t
+        files
+      end
+    end
+
+    context "A model with an S3 attachment" do
+      setup do
+        rebuild_model :styles => { :large => "300x300>",
+                                   :medium => "100x100",
+                                   :thumb => ["32x32#", :gif] },
+                      :storage => :s3,
+                      # :s3_options => {:logger => Logger.new(StringIO.new)},
+                      :s3_credentials => File.new(File.join(File.dirname(__FILE__), "s3.yml")),
+                      :default_style => :medium,
+                      :bucket => ENV['S3_TEST_BUCKET'],
+                      :path => ":class/:attachment/:id/:style/:basename.:extension"
+        @dummy     = Dummy.new
+        @file      = File.new(File.join(FIXTURES_DIR, "5k.png"))
+        @bad_file  = File.new(File.join(FIXTURES_DIR, "bad.png"))
+
+        assert @dummy.avatar = @file
+        assert @dummy.valid?
+        assert @dummy.save
+
+        @files_on_s3 = s3_files_for @dummy.avatar
+      end
+
+      should "write and delete its files" do
+        [["434x66", :original],
+         ["300x46", :large],
+         ["100x15", :medium],
+         ["32x32", :thumb]].each do |geo, style|
+          cmd = %Q[identify -format "%wx%h" #{@files_on_s3[style].path}]
+          assert_equal geo, `#{cmd}`.chomp, cmd
+        end
+
+        @d2 = Dummy.find(@dummy.id)
+        @d2_files = s3_files_for @d2.avatar
+        [["434x66", :original],
+         ["300x46", :large],
+         ["100x15", :medium],
+         ["32x32", :thumb]].each do |geo, style|
+          cmd = %Q[identify -format "%wx%h" #{@d2_files[style].path}]
+          assert_equal geo, `#{cmd}`.chomp, cmd
+        end
+
+        @dummy.avatar = "not a valid file but not nil"
+        assert_equal File.basename(@file.path), @dummy.avatar_file_name
+        assert @dummy.valid?
+        assert @dummy.save
+
+        saved_keys = [:thumb, :medium, :large, :original].collect{|s| @dummy.avatar.to_file(s) }
+
+        saved_keys.each do |key|
+          assert key.exists?
+        end
+
+        @dummy.avatar = nil
+        assert_nil @dummy.avatar_file_name
+        assert @dummy.valid?
+        assert @dummy.save
+
+        saved_keys.each do |key|
+          assert ! key.exists?
+        end
+
+        @d2 = Dummy.find(@dummy.id)
+        assert_nil @d2.avatar_file_name
+      end
+
+      should "work exactly the same when new as when reloaded" do
+        @d2 = Dummy.find(@dummy.id)
+
+        assert_equal @dummy.avatar_file_name, @d2.avatar_file_name
+        [:thumb, :medium, :large, :original].each do |style|
+          assert_equal @dummy.avatar.to_file(style).to_s, @d2.avatar.to_file(style).to_s
+        end
+
+        saved_keys = [:thumb, :medium, :large, :original].collect{|s| @dummy.avatar.to_file(s) }
+
+        @d2.avatar = nil
+        assert @d2.save
+
+        saved_keys.each do |key|
+          assert ! key.exists?
+        end
+      end
+
+      should "know the difference between good files, bad files, not files, and nil" do
+        expected = @dummy.avatar.file
+        @dummy.avatar = "not a file"
+        assert @dummy.valid?
+        assert_equal expected, @dummy.avatar.file
+
+        @dummy.avatar = @bad_file
+        assert ! @dummy.valid?
+        @dummy.avatar = nil
+        assert @dummy.valid?
+
+        Dummy.validates_attachment_presence :avatar
+        @d2 = Dummy.find(@dummy.id)
+        @d2.avatar = @file
+        assert   @d2.valid?
+        @d2.avatar = @bad_file
+        assert ! @d2.valid?
+        @d2.avatar = nil
+        assert ! @d2.valid?
+      end
+
+      should "be able to reload without saving an not have the file disappear" do
+        @dummy.avatar = @file
+        assert @dummy.save
+        @dummy.avatar = nil
+        assert_nil @dummy.avatar_file_name
+        @dummy.reload
+        assert_equal "5k.png", @dummy.avatar_file_name
+      end
+    end
+  end
 end
 

test/test_storage.rb

+require 'rubygems'
+require 'test/unit'
+require 'shoulda'
+require 'right_aws'
+
+require File.join(File.dirname(__FILE__), '..', 'lib', 'paperclip', 'geometry.rb')
+
+class S3Test < Test::Unit::TestCase
+  context "An attachment with S3 storage" do
+    setup do
+      rebuild_model :storage => :s3,
+                    :bucket => "testing",
+                    :s3_credentials => {
+                      'access_key_id' => "12345",
+                      'secret_access_key' => "54321"
+                    }
+
+      @s3_mock     = stub
+      @bucket_mock = stub
+      RightAws::S3.expects(:new).
+        with("12345", "54321", {}).
+        returns(@s3_mock)
+      @s3_mock.expects(:bucket).with("testing", true, "public-read").returns(@bucket_mock)
+    end
+
+    should "be extended by the S3 module" do
+      assert Dummy.new.avatar.is_a?(Paperclip::Storage::S3)
+    end
+
+    should "not be extended by the Filesystem module" do
+      assert ! Dummy.new.avatar.is_a?(Paperclip::Storage::Filesystem)
+    end
+
+    context "when assigned" do
+      setup do
+        @file = File.new(File.join(File.dirname(__FILE__), 'fixtures', '5k.png'))
+        @dummy = Dummy.new
+        @dummy.avatar = @file
+      end
+
+      should "still return a Tempfile when sent #to_io" do
+        assert_equal Tempfile, @dummy.avatar.to_io.class
+      end
+
+      context "and saved" do
+        setup do
+          @key_mock = stub
+          @bucket_mock.expects(:key).returns(@key_mock)
+          @key_mock.expects(:data=)
+          @key_mock.expects(:put)
+          @dummy.save
+        end
+
+        should "succeed" do
+          assert true
+        end
+      end
+    end
+  end
+
+  unless ENV["S3_TEST_BUCKET"].blank?
+    context "Using S3 for real, an attachment with S3 storage" do
+      setup do
+        rebuild_model :styles => { :thumb => "100x100", :square => "32x32#" },
+                      :storage => :s3,
+                      :bucket => ENV["S3_TEST_BUCKET"],
+                      :path => ":class/:attachment/:id/:style.:extension",
+                      :s3_credentials => File.new(File.join(File.dirname(__FILE__), "s3.yml"))
+
+        Dummy.delete_all
+        @dummy = Dummy.new
+      end
+
+      should "be extended by the S3 module" do
+        assert Dummy.new.avatar.is_a?(Paperclip::Storage::S3)
+      end
+
+      context "when assigned" do
+        setup do
+          @file = File.new(File.join(File.dirname(__FILE__), 'fixtures', '5k.png'))
+          @dummy.avatar = @file
+        end
+
+        should "still return a Tempfile when sent #to_io" do
+          assert_equal Tempfile, @dummy.avatar.to_io.class
+        end
+
+        context "and saved" do
+          setup do
+            @dummy.save
+          end
+
+          should "be on S3" do
+            assert true
+          end
+        end
+      end
+    end
+  end
+end