Failing validations don't prevent post_processing callbacks

[danielfone]

Steps to Reproduce:

1. Have a record with a validation on the attachment (e.g. content type)
2. Have an after_post_process callback
3. Create a record with an attachment that fails the validation

class Widget < ActiveRecord::Base
  include Paperclip::Glue

  has_attached_file :image

  validates_attachment_content_type :image, :content_type => %r{image/.*}

  after_image_post_process :save_image_dimensions

  def save_image_dimensions
    puts "We should not get here if the image is invalid!"
  end
end

Widget.create! image: File.open('dummy_file.txt')

Expected:

As per the documentation in the README:

NOTE: Post processing will not even start if the attachment is not valid according to the validations. Your callbacks and processors will only be called with valid attachments.

We would not expect the after_post_process callback to be run.

Actual:

The code in attachment.rb will guard the post-processing against invalid attachments, but the callbacks will be run anyway.

I've setup a runnable, reproducible case here to demonstrate the issue in full.

Suggestion:

Either the README can be updated to reflect this behaviour, or the Attachment#post_process method could be rewritten not to run the callbacks when the validations fail. I'm more than happy to raise a PR depending on the preferred solution.

[tute]

This sounds like a bug that should be addressed in a PR. @jyurek?

[jyurek]

Agreed, this is definitely a bug. @danielfone, thanks a huge amount for the runnable verification!

[jyurek]

Well, that's dumb. OK, on digging a little, I recalled that the validators are defined as before_post_process callbacks. So we can't simply ignore them. That being the case, what would be the expectation, here. Is simply documenting that after_ callbacks will be run enough? before_ callbacks will be run regardless given that the validations might not even have run yet, so it would make sense, wouldn't it?

[danielfone]

From my limited point of view, here's 3 options:

1. Just update the documentation. This is easiest I guess, but puts the burden back on the user to guard the callbacks. At the moment, I'm using a simple return if errors.any? inside the callback — perhaps this suggestion could be made in the documentation.
2. Move the validations outside of the callbacks, and don't start the callback sequence if the validations fail. I feel like this is probably the best behaviour, but also requires the most internal changes.
3. Run the before_* callbacks — including the validations — but if the validations don't pass, break out with a throw/catch so the after_* callbacks aren't run. This is closest to how ActiveRecord behaves from I user's point-of-view I think. Conceptually, this could look like this gist.

IMO running the post_processing callbacks on an attachment that is obviously invalid is surprising behaviour. The fact that the validations are being run as before_* callbacks is probably not something average user is aware of, and hence they're not likely to expect the after_* callbacks to run either. But if the documentation is clear enough I guess that would mitigate it.

[tute]

Related with: #2178

[mike-burns]

Thank you for reporting this. Unfortunately, we will be deprecating Paperclip and therefore will not have the bandwidth to address this issue.

[ssinghi]

This has been fixed at https://github.com/kreeti/paperclip/commits/master.