Clearing an attachment from a new record throws an error upon save #62

Closed
qrush opened this Issue Jul 16, 2009 · 3 comments

Comments

Projects
None yet
5 participants
Contributor

qrush commented Jul 16, 2009

Reported by Adam Grant

My Paperclip::VERSION == "2.2.9.2"

If you create a new ActiveRecord object, assign an attachment to it, clear out that attachment, and then try and save the record, the Storage#flush_writes method balks because queued_for_write was never cleared.

Example:

class Asset < ActiveRecord::Base
   has_attached_file :data
end
asset = Asset.new
asset.data = File.new("some/path", "r")
asset.data.clear
asset.data.save!
TypeError: can't convert nil into String
    from ../vendor/plugins/paperclip/lib/paperclip/storage.rb:41:in `dirname'
    from ../vendor/plugins/paperclip/lib/paperclip/storage.rb:41:in `flush_writes'
    from ../vendor/plugins/paperclip/lib/paperclip/storage.rb:39:in `each'
    from ../vendor/plugins/paperclip/lib/paperclip/storage.rb:39:in `flush_writes'
    from ../vendor/plugins/paperclip/lib/paperclip/attachment.rb:142:in `save'
    from ../vendor/plugins/paperclip/lib/paperclip/attachment.rb:165:in `destroy'
==========================================

My solution is this: modify the clear method to reset the @queued_for_write variable:

Index: lib/paperclip/attachment.rb
=================================

--- lib/paperclip/attachment.rb (revision 1679)
+++ lib/paperclip/attachment.rb (working copy)
@@ -153,6 +153,7 @@
     # use #destroy.
     def clear
       queue_existing_for_delete
+      @queued_for_write  = {}
       @errors            = {}
       @validation_errors = nil
     end

I have a test here as well:

Index: test/attachment_test.rb
===================================================================
--- test/attachment_test.rb (revision 1679)
+++ test/attachment_test.rb (working copy)
@@ -514,6 +514,16 @@
       assert_equal nil, @attachment.path(:blah)
     end

+    context "with a file assigned but not saved yet" do
+      should "clear out any attached files" do
+        @attachment.assign(@file)
+        assert @attachment.valid?
+        assert !@attachment.queued_for_write.blank?
+        @attachment.clear
+        assert @attachment.queued_for_write.blank?
+      end
+    end    
+
     context "with a file assigned in the database" do
       setup do
         @attachment.stubs(:instance_read).with(:file_name).returns("5k.png")
===================================

Sorry I can't get it into any better patch format (using git and Github and forking) than that. My machine I'm working from is quite limited from the git perspective. It's a simple fix though.

See any problems with this?

I'd like this change too.

I have a "delete picture" checkbox and http://gist.github.com/102379 would do the job. The thing is I want the checkbox do clear the picture even if you do select a file in the input. I modified the module a bit to accomplish this, but ran into this issue.

As for now a quick hack does the job for me:
picture.clear
picture.instance_variable_set("@queued_for_write", {} )

Thanks, qrush

jdrowell commented Dec 1, 2009

So I ran into this problem in a different way. My database seeds (which include images) didn't work on another machine (probably due to a different version of ImageMagick) and trying to .clear the attachment after finding out it was not .valid? triggered the same error. My fork of paperclip has basically the same fix as above but adds different tests. Please merge either one.

My fork: http://github.com/jdrowell/paperclip/tree/master

arkan commented Apr 1, 2010

Thanks for your hack wojciech :)

@ghost ghost assigned sikachu Jun 24, 2011

@sikachu sikachu closed this in 054a807 Jun 30, 2011

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment