Validate the attachment before we attach without calling valid?

Using the ActiveRecord validations is smart in order to lessen load on
the project to develop our own validators, but it's problematic in that
calling `valid?` on the record fires off all the other validations --
including those on fields which may not be set yet because of
mass-assignment. This commit will "pre-validate" the attachment's fields
so that it doesn't process an invalid attachment, but it does so by
running its validations manually on assignment.

lib/paperclip/attachment.rb

@@ -28,7 +28,8 @@ module Paperclip
         :url_generator         => Paperclip::UrlGenerator,
         :use_default_time_zone => true,
         :use_timestamp         => true,
-        :whiny                 => Paperclip.options[:whiny] || Paperclip.options[:whiny_thumbnails]
+        :whiny                 => Paperclip.options[:whiny] || Paperclip.options[:whiny_thumbnails],
+        :check_validity_before_processing => true
       }
     end
 
@@ -105,7 +106,7 @@ module Paperclip
 
       @dirty = true
 
-      post_process(*only_process) if post_processing && valid_assignment?
+      post_process(*only_process) if post_processing
 
       # Reset the file size if the original file was reprocessed.
       instance_write(:file_size,   @queued_for_write[:original].size)
@@ -376,16 +377,6 @@ module Paperclip
       Paperclip.log(message)
     end
 
-    def valid_assignment? #:nodoc:
-      if instance.valid?
-        true
-      else
-        instance.errors.none? do |attr, message|
-          attr.to_s.start_with?(@name.to_s)
-        end
-      end
-    end
-
     def initialize_storage #:nodoc:
       storage_class_name = @options[:storage].to_s.downcase.camelize
       begin
@@ -418,7 +409,9 @@ module Paperclip
 
       instance.run_paperclip_callbacks(:post_process) do
         instance.run_paperclip_callbacks(:"#{name}_post_process") do
-          post_process_styles(*style_args)
+          unless @options[:check_validity_before_processing] && instance.errors.any?
+            post_process_styles(*style_args)
+          end
         end
       end
     end

lib/paperclip/validators.rb

@@ -34,13 +34,32 @@ module Paperclip
             validator_kind = $1.underscore.to_sym
 
             if options.has_key?(validator_kind)
-              options[:"attachment_#{validator_kind}"] = options.delete(validator_kind)
+              validator_options = options.delete(validator_kind)
+              validator_options = {} if validator_options == true
+              local_options = attributes + [validator_options]
+              send(:"validates_attachment_#{validator_kind}", *local_options)
             end
           end
         end
+      end
+
+      def validate_before_processing(validator_class, options)
+        options = options.dup
+        attributes = options.delete(:attributes)
+        attributes.each do |attribute|
+          options[:attributes] = [attribute]
+          create_validating_before_filter(attribute, validator_class, options)
+        end
+      end
 
-        validates(*attributes + [options])
+      def create_validating_before_filter(attribute, validator_class, options)
+        if_clause = options.delete(:if)
+        unless_clause = options.delete(:unless)
+        send(:"before_#{attribute}_post_process", :if => if_clause, :unless => unless_clause) do |*args|
+          validator_class.new(options.dup).validate(self)
+        end
       end
+
     end
   end
 end

lib/paperclip/validators/attachment_content_type_validator.rb

@@ -73,7 +73,9 @@ module Paperclip
       # You'll still need to have a virtual attribute (created by +attr_accessor+)
       # name +[attachment]_content_type+ to be able to use this validator.
       def validates_attachment_content_type(*attr_names)
-        validates_with AttachmentContentTypeValidator, _merge_attributes(attr_names)
+        options = _merge_attributes(attr_names)
+        validates_with AttachmentContentTypeValidator, options.dup
+        validate_before_processing AttachmentContentTypeValidator, options.dup
       end
     end
   end

lib/paperclip/validators/attachment_presence_validator.rb

@@ -2,12 +2,10 @@ require 'active_model/validations/presence'
 
 module Paperclip
   module Validators
-    class AttachmentPresenceValidator < ActiveModel::Validations::PresenceValidator
-      def validate(record)
-        [attributes].flatten.map do |attribute|
-          if record.send(:read_attribute_for_validation, "#{attribute}_file_name").blank?
-            record.errors.add(attribute, :blank, options)
-          end
+    class AttachmentPresenceValidator < ActiveModel::EachValidator
+      def validate_each(record, attribute, value)
+        if record.send("#{attribute}_file_name").blank?
+          record.errors.add(attribute, :blank, options)
         end
       end
     end
@@ -19,7 +17,9 @@ module Paperclip
       #   be run if this lambda or method returns true.
       # * +unless+: Same as +if+ but validates if lambda or method returns false.
       def validates_attachment_presence(*attr_names)
-        validates_with AttachmentPresenceValidator, _merge_attributes(attr_names)
+        options = _merge_attributes(attr_names)
+        validates_with AttachmentPresenceValidator, options.dup
+        validate_before_processing AttachmentPresenceValidator, options.dup
       end
     end
   end

lib/paperclip/validators/attachment_size_validator.rb

@@ -98,7 +98,9 @@ module Paperclip
       #   be run if this lambda or method returns true.
       # * +unless+: Same as +if+ but validates if lambda or method returns false.
       def validates_attachment_size(*attr_names)
-        validates_with AttachmentSizeValidator, _merge_attributes(attr_names)
+        options = _merge_attributes(attr_names)
+        validates_with AttachmentSizeValidator, options.dup
+        validate_before_processing AttachmentSizeValidator, options.dup
       end
     end
   end

paperclip.gemspec

@@ -45,6 +45,7 @@ Gem::Specification.new do |s|
   s.add_development_dependency('bundler')
   s.add_development_dependency('fog', '>= 1.4.0', "< 1.7.0")
   s.add_development_dependency('pry')
+  s.add_development_dependency('pry-debugger')
   s.add_development_dependency('launchy')
   s.add_development_dependency('rake')
   s.add_development_dependency('fakeweb')

test/attachment_processing_test.rb

@@ -7,23 +7,77 @@ class AttachmentProcessingTest < Test::Unit::TestCase
     rebuild_model
   end
 
-  should 'process attachments given a valid assignment' do
-    file = File.new(fixture_file("5k.png"))
-    Dummy.validates_attachment_content_type :avatar, :content_type => "image/png"
-    instance = Dummy.new
-    attachment = instance.avatar
-    attachment.expects(:post_process)
-
-    attachment.assign(file)
+  context 'using validates_attachment_content_type' do
+    should 'process attachments given a valid assignment' do
+      file = File.new(fixture_file("5k.png"))
+      Dummy.validates_attachment_content_type :avatar, :content_type => "image/png"
+      instance = Dummy.new
+      attachment = instance.avatar
+      attachment.expects(:post_process_styles)
+
+      attachment.assign(file)
+    end
+
+    should 'not process attachments given an invalid assignment with :not' do
+      file = File.new(fixture_file("5k.png"))
+      Dummy.validates_attachment_content_type :avatar, :not => "image/png"
+      instance = Dummy.new
+      attachment = instance.avatar
+      attachment.expects(:post_process_styles).never
+
+      attachment.assign(file)
+    end
+
+    should 'not process attachments given an invalid assignment with :content_type' do
+      file = File.new(fixture_file("5k.png"))
+      Dummy.validates_attachment_content_type :avatar, :content_type => "image/tiff"
+      instance = Dummy.new
+      attachment = instance.avatar
+      attachment.expects(:post_process_styles).never
+
+      attachment.assign(file)
+    end
+
+    should 'when validation :if clause returns false, allow what would be an invalid assignment' do
+      invalid_assignment = File.new(fixture_file("5k.png"))
+      Dummy.validates_attachment_content_type :avatar, :content_type => "image/tiff", :if => lambda{false}
+      instance = Dummy.new
+      attachment = instance.avatar
+      attachment.expects(:post_process_styles)
+
+      attachment.assign(invalid_assignment)
+    end
   end
 
-  should 'not process attachments if the assignment does not pass validation' do
-    file = File.new(fixture_file("5k.png"))
-    Dummy.validates_attachment_content_type :avatar, :content_type => "image/tiff"
-    instance = Dummy.new
-    attachment = instance.avatar
-    attachment.expects(:post_process).never
+  context 'using validates_attachment' do
+    should 'process attachments given a valid assignment' do
+      file = File.new(fixture_file("5k.png"))
+      Dummy.validates_attachment :avatar, :content_type => {:content_type => "image/png"}
+      instance = Dummy.new
+      attachment = instance.avatar
+      attachment.expects(:post_process_styles)
+
+      attachment.assign(file)
+    end
+
+    should 'not process attachments given an invalid assignment with :not' do
+      file = File.new(fixture_file("5k.png"))
+      Dummy.validates_attachment :avatar, :content_type => {:not => "image/png"}
+      instance = Dummy.new
+      attachment = instance.avatar
+      attachment.expects(:post_process_styles).never
+
+      attachment.assign(file)
+    end
+
+    should 'not process attachments given an invalid assignment with :content_type' do
+      file = File.new(fixture_file("5k.png"))
+      Dummy.validates_attachment :avatar, :content_type => {:content_type => "image/tiff"}
+      instance = Dummy.new
+      attachment = instance.avatar
+      attachment.expects(:post_process_styles).never
 
-    attachment.assign(file)
+      attachment.assign(file)
+    end
   end
 end

test/integration_test.rb

@@ -348,7 +348,7 @@ class IntegrationTest < Test::Unit::TestCase
       Dummy.validates_attachment_presence :avatar
       @d2 = Dummy.find(@dummy.id)
       @d2.avatar = @file
-      assert   @d2.valid?, @d2.errors.full_messages.inspect
+      assert  @d2.valid?, @d2.errors.full_messages.inspect
       @d2.avatar = @bad_file
       assert ! @d2.valid?
     end

test/validators_test.rb

@@ -7,7 +7,7 @@ class ValidatorsTest < Test::Unit::TestCase
 
   context "using the helper" do
     setup do
-      Dummy.validates_attachment :avatar, :presence => true, :content_type => { :content_type => "image/jpg" }, :size => { :in => 0..10.kilobytes }
+      Dummy.validates_attachment :avatar, :presence => true, :content_type => { :content_type => "image/jpeg" }, :size => { :in => 0..10.kilobytes }
     end
 
     should "add the attachment_presence validator to the class" do
@@ -21,5 +21,12 @@ class ValidatorsTest < Test::Unit::TestCase
     should "add the attachment_size validator to the class" do
       assert Dummy.validators_on(:avatar).any?{ |validator| validator.kind == :attachment_size }
     end
+
+    should 'prevent you from attaching a file that violates that validation' do
+      Dummy.class_eval{ validate(:name) { raise "DO NOT RUN THIS" } }
+      dummy = Dummy.new(:avatar => File.new(fixture_file("12k.png")))
+      assert_equal [:avatar_content_type, :avatar, :avatar_file_size], dummy.errors.keys
+      assert_raise(RuntimeError){ dummy.valid? }
+    end
   end
 end