DRAFT toward fixing after_post_process is run even if validations fail

[jrochkind]

@tute, re #2178

This is not a solution, it may or may not be even the road to the solution, but sharing what I've figured out and where I've gotten to.

Context

Paperclip lets you set before_ and after_ callbacks, both attribute-specific (eg after_avatar_post_process), and that apply to any attribute (after_post_process).

Paperclip README says:

The callbacks are intended to be as close to normal ActiveRecord callbacks as possible, so if you return false (specifically - returning nil is not the same) in a before_filter, the post processing step will halt. Returning false in an after_filter will not halt anything, but you can access the model and the attachment if necessary."

And indeed, the callbacks are implemented using ActiveSupport::Callbacks

The README also says, regarding validations:

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.

The latter appears not to be true, see #2204. I am not sure how much of the former is true or not, I have not tested it extensively.

skip_after_callbacks_if_terminated

AS::Callbacks has a skip_after_callbacks_if_terminated: "Determines if after callbacks should be terminated by the :terminator option. By default after callbacks executed no matter if callback chain was terminated or not. Option makes sense only when :terminator option is specified."

Paperclip does not supply this option (it does supply the terminator option).

Paperclip validations are implemented as before callbacks. And the problem seemed to be after callbacks (after_post_process) running if even validations failed, so after spelunking through the paperclip code and seeing this, my first thought was to try setting skip_after_callbacks_if_terminated: true.

Wrong specifications in attachment_spec.rb.

That change made some specs fail. On further reflection, I think these specs were failing becuase they were wrong, they were ensuring precisely the wrong behavior.

They were ensuring that even if a before action cancelled, the after actions still would run. They should have been ensuring the opposite.

Maybe? Clearly they should have been for validations, but is that true for other types of callback actions? The intended behavior is not entirely clear. Should a terminated before callback stop after processing in general, even if it's not validation-related?

I would suggest probably so -- the docs say it's intended to be as much like ActiveRecord callbacks as possible, and I think that's how ActiveRecord callbacks work. The docs say "if you return false (specifically - returning nil is not the same) in a before_filter, the post processing step will halt."

So I think the pre-existing specs were wrong, and skip_after_callbacks_if_terminated: true should be added. BUT, that's not enough...

The two callback chains

For every attribute, there are two callback chains. The attribute-specific one, and the general one. skip_after_callbacks_if_terminated will make one of the callbacks halt, but the other one is uneffected.

So after I changed all specs to look like the right requirements (in this PR).... there were still one or two failing even after this change.

Again, it's not entirely clear what the expected behavior is with the callbacks in general, before we get to validations.

If a before_avater_post_process returns false, should a (generic) after_post_process callback be cancelled? And vice versa? I think so, but that's the thing that doesn't work with just skip_after_callbacks_if_terminated. For validations (implemented as before callbacks), it's more clear that it should do this. For callbacks in general... not sure.

If it's going to be done for callbacks in general, some more architecture is required. Maybe something involving the little-seen ruby throw/catch, like @tute pointed out Rails5 has done? A throw that gets caught outside the specific callbacks entirely, unlike Rails5? I experimented a bit with this, but didn't get anywhere, might try more. (Rails5 new func is implemented pretty much entirely in the default_terminator, paperclip [supplies it's own terminator], so can do whatever it wants possibly inspired by but independent from Rails5).

Or the fix could be specific to validations, instead of just treating at as a bug in callbacks in general. Especially if it's determined that callbacks in general are working as desired, it's just validations that need fixing. (I don't think so though).

The relevant paperclip code is here: https://github.com/thoughtbot/paperclip/blob/master/lib/paperclip/attachment.rb#L503-L508

Validation-specific

In addition to messing with specs as above in attachment_spec.rb for general callback related behavior, I tried to add some content-type-validation specific specs related to this behavior. However, I was not successful at doing so -- the tests I tried to add are 'false negatives', they pass no matter what, even without any other changes.

Not sure why i was not successful at adding broken tests for the current reproducibly broken behavior. Advice welcome.

Do other parts of the documented callbacks work as advertised?

The README suggests to me that a before callback returning false should cancel everything. We've talked about it cancelling after callbacks -- but I think it should probably cancel the meat of the thing itself, the README says not just that after callbacks won't happen, but that "the post processing step will halt"

I think that may be broken too. There is some code specific to cancelling the post_process_styles step for validation failures specifically, but my reading of the docs is that it should be called for any before callback termination. I don't think that is happening, bug I haven't tested it. I think it should be.

It's possible that a solution to the after callbacks truly being cancelled (for both attribute-specific and general chains) might also easily flow to post_process_styles not happening either in general not just for validation failures. I think that would be a correction.

Assistance welcome

So, yeah, this starts to get more confusing and complicated the more I look at it, with part of the issue being lack of clarity about what should happen. Assistance welcome. I hope this long narrative helped clarify rather than confuse.

I'm ignoring the annoying Hound warnings, because we have way bigger challenges then that here,
although they are annoying. Someone else is welcome to fix them -- oops, except I guess you can't commit to my fork. I'll give @tute et al rights to commit to my fork if that will help with some collaboration.

[houndci-bot]

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

[tute]

Debugging in my computer the false positive in master.

[jrochkind]

@tute lengthy narrative explanation now added above, with what I found and why I did what I did and where it's at.

[tute]

This patch always fails, regardless of the options:

From 10a0cc6141d9d825d0fa0c19b99e5bd109a6447f Mon Sep 17 00:00:00 2001
From: Tute Costa <tutecosta@gmail.com>
Date: Tue, 10 May 2016 16:53:20 -0400
Subject: [PATCH] triaging

---
 lib/paperclip/callbacks.rb                   |  7 ++++++-
 paperclip.gemspec                            |  1 -
 spec/paperclip/attachment_processing_spec.rb | 27 +++++++++++++++++++++++++++
 3 files changed, 33 insertions(+), 2 deletions(-)

diff --git a/lib/paperclip/callbacks.rb b/lib/paperclip/callbacks.rb
index 53a4368..60bc1ea 100644
--- a/lib/paperclip/callbacks.rb
+++ b/lib/paperclip/callbacks.rb
@@ -7,7 +7,12 @@ module Paperclip

     module Defining
       def define_paperclip_callbacks(*callbacks)
-        define_callbacks(*[callbacks, { terminator: hasta_la_vista_baby }].flatten)
+        options = {
+          terminator: hasta_la_vista_baby,
+          skip_after_callbacks_if_terminated: true
+        }
+        define_callbacks(*[callbacks, options].flatten)
+
         callbacks.each do |callback|
           eval <<-end_callbacks
             def before_#{callback}(*args, &blk)
diff --git a/paperclip.gemspec b/paperclip.gemspec
index 80164aa..4587fba 100644
--- a/paperclip.gemspec
+++ b/paperclip.gemspec
@@ -40,7 +40,6 @@ Gem::Specification.new do |s|
   s.add_development_dependency('cucumber', '~> 1.3.18')
   s.add_development_dependency('aruba', '~> 0.9.0')
   s.add_development_dependency('nokogiri')
-  # Ruby version < 1.9.3 can't install capybara > 2.0.3.
   s.add_development_dependency('capybara')
   s.add_development_dependency('bundler')
   s.add_development_dependency('fog-aws')
diff --git a/spec/paperclip/attachment_processing_spec.rb b/spec/paperclip/attachment_processing_spec.rb
index 8243036..7e4764f 100644
--- a/spec/paperclip/attachment_processing_spec.rb
+++ b/spec/paperclip/attachment_processing_spec.rb
@@ -44,6 +44,33 @@ describe 'Attachment Processing' do

       attachment.assign(invalid_assignment)
     end
+
+    context "with custom post-processing" do
+      before do
+        Dummy.class_eval do
+          validates_attachment_content_type :avatar, content_type: "image/png"
+          before_avatar_post_process :do_before_avatar
+          after_avatar_post_process :do_after_avatar
+          before_post_process :do_before_all
+          after_post_process :do_after_all
+          def do_before_avatar; end
+          def do_after_avatar; end
+          def do_before_all; end
+          def do_after_all; end
+        end
+      end
+
+      it 'does not run custom post-processing if validation fails' do
+        file = File.new(fixture_file("5k.png"))
+        instance = Dummy.new
+        instance.expects(:do_before_avatar).once
+        instance.expects(:do_before_all).once
+        instance.expects(:do_after_avatar).never
+        instance.expects(:do_after_all).never
+
+        instance.avatar.assign(file)
+      end
+    end
   end

   context 'using validates_attachment' do
-- 
2.8.1

[tute]

The terminator's result is [:avatar] for these tests. There is something off.

[tute]

@jrochkind, this is the patch with the proper regression spec:

From 633f547797bb850f24a2c639855c9668f2440d1f Mon Sep 17 00:00:00 2001
From: Tute Costa <tutecosta@gmail.com>
Date: Tue, 10 May 2016 16:53:20 -0400
Subject: [PATCH] triaging

---
 lib/paperclip/callbacks.rb                   |  7 +++++-
 paperclip.gemspec                            |  1 -
 spec/paperclip/attachment_processing_spec.rb | 32 ++++++++++++++++++++++++++++
 3 files changed, 38 insertions(+), 2 deletions(-)

diff --git a/lib/paperclip/callbacks.rb b/lib/paperclip/callbacks.rb
index 53a4368..60bc1ea 100644
--- a/lib/paperclip/callbacks.rb
+++ b/lib/paperclip/callbacks.rb
@@ -7,7 +7,12 @@ module Paperclip

     module Defining
       def define_paperclip_callbacks(*callbacks)
-        define_callbacks(*[callbacks, { terminator: hasta_la_vista_baby }].flatten)
+        options = {
+          terminator: hasta_la_vista_baby,
+          skip_after_callbacks_if_terminated: true
+        }
+        define_callbacks(*[callbacks, options].flatten)
+
         callbacks.each do |callback|
           eval <<-end_callbacks
             def before_#{callback}(*args, &blk)
diff --git a/paperclip.gemspec b/paperclip.gemspec
index 80164aa..4587fba 100644
--- a/paperclip.gemspec
+++ b/paperclip.gemspec
@@ -40,7 +40,6 @@ Gem::Specification.new do |s|
   s.add_development_dependency('cucumber', '~> 1.3.18')
   s.add_development_dependency('aruba', '~> 0.9.0')
   s.add_development_dependency('nokogiri')
-  # Ruby version < 1.9.3 can't install capybara > 2.0.3.
   s.add_development_dependency('capybara')
   s.add_development_dependency('bundler')
   s.add_development_dependency('fog-aws')
diff --git a/spec/paperclip/attachment_processing_spec.rb b/spec/paperclip/attachment_processing_spec.rb
index 8243036..fcc6cef 100644
--- a/spec/paperclip/attachment_processing_spec.rb
+++ b/spec/paperclip/attachment_processing_spec.rb
@@ -44,6 +44,38 @@ describe 'Attachment Processing' do

       attachment.assign(invalid_assignment)
     end
+
+    context "with custom post-processing" do
+      before do
+        Dummy.class_eval do
+          validates_attachment_content_type :avatar, content_type: "image/png"
+          before_avatar_post_process :do_before_avatar
+          after_avatar_post_process :do_after_avatar
+          before_post_process :do_before_all
+          after_post_process :do_after_all
+          def do_before_avatar
+            false
+          end
+          def do_before_all
+            false
+          end
+
+          def do_after_avatar
+          end
+          def do_after_all
+          end
+        end
+      end
+
+      it 'does not run custom post-processing if validation fails' do
+        file = File.new(fixture_file("5k.png"))
+        instance = Dummy.new
+        instance.expects(:do_after_avatar).never
+        instance.expects(:do_after_all).never
+
+        instance.avatar.assign(file)
+      end
+    end
   end

   context 'using validates_attachment' do
-- 
2.8.1

The question now is if it takes into account the save => false result, or only the before_* callback results.

[tute]

These callbacks are not affected by the validity of the model, only by what before_* callbacks might return: https://github.com/thoughtbot/paperclip/blob/master/lib/paperclip/attachment.rb#L99-L117.

Patch in #2204 (comment) is easy to write, but asserting on the validity of the model doesn't change the calls. before_* returning target.valid? do, but that will be client code. And might be the workaround for now, until (and if we do) we find the proper fix.

[tute]

This "works" (with client code defining before callbacks result as valid?):

diff --git a/spec/paperclip/attachment_processing_spec.rb b/spec/paperclip/attachment_processing_spec.rb
index fcc6cef..ed6d2af 100644
--- a/spec/paperclip/attachment_processing_spec.rb
+++ b/spec/paperclip/attachment_processing_spec.rb
@@ -48,16 +48,16 @@ describe 'Attachment Processing' do
     context "with custom post-processing" do
       before do
         Dummy.class_eval do
-          validates_attachment_content_type :avatar, content_type: "image/png"
+          validates_attachment :avatar, content_type: { not: "image/png" }
           before_avatar_post_process :do_before_avatar
           after_avatar_post_process :do_after_avatar
           before_post_process :do_before_all
           after_post_process :do_after_all
           def do_before_avatar
-            false
+            valid?
           end
           def do_before_all
-            false
+            valid?
           end

           def do_after_avatar

[tute]

This patch changes behavior of the terminator, and makes one assertion work, but not the general after_all:

From 90d3f8d460d81401779be912671c10492ec8b505 Mon Sep 17 00:00:00 2001
From: Tute Costa <tutecosta@gmail.com>
Date: Tue, 10 May 2016 16:53:20 -0400
Subject: [PATCH] triaging

---
 lib/paperclip/callbacks.rb                   | 15 ++++++++-------
 spec/paperclip/attachment_processing_spec.rb | 24 ++++++++++++++++++++++++
 2 files changed, 32 insertions(+), 7 deletions(-)

diff --git a/lib/paperclip/callbacks.rb b/lib/paperclip/callbacks.rb
index 53a4368..fd4097b 100644
--- a/lib/paperclip/callbacks.rb
+++ b/lib/paperclip/callbacks.rb
@@ -7,7 +7,12 @@ module Paperclip

     module Defining
       def define_paperclip_callbacks(*callbacks)
-        define_callbacks(*[callbacks, { terminator: hasta_la_vista_baby }].flatten)
+        options = {
+          terminator: hasta_la_vista_baby,
+          skip_after_callbacks_if_terminated: true
+        }
+        define_callbacks(*[callbacks, options].flatten)
+
         callbacks.each do |callback|
           eval <<-end_callbacks
             def before_#{callback}(*args, &blk)
@@ -23,12 +28,8 @@ module Paperclip
       private

       def hasta_la_vista_baby
-        lambda do |_, result|
-          if result.respond_to?(:call)
-            result.call == false
-          else
-            result == false
-          end
+        lambda do |target, result|
+          !target.valid?
         end
       end
     end
diff --git a/spec/paperclip/attachment_processing_spec.rb b/spec/paperclip/attachment_processing_spec.rb
index 8243036..8085f39 100644
--- a/spec/paperclip/attachment_processing_spec.rb
+++ b/spec/paperclip/attachment_processing_spec.rb
@@ -44,6 +44,30 @@ describe 'Attachment Processing' do

       attachment.assign(invalid_assignment)
     end
+
+    context "with custom post-processing" do
+      before do
+        Dummy.class_eval do
+          validates_attachment :avatar, content_type: { not: "image/png" }
+          after_avatar_post_process :do_after_avatar
+          after_post_process :do_after_all
+
+          def do_after_avatar
+          end
+          def do_after_all
+          end
+        end
+      end
+
+      it 'does not run custom post-processing if validation fails' do
+        file = File.new(fixture_file("5k.png"))
+        instance = Dummy.new
+        instance.expects(:do_after_avatar).never
+        # instance.expects(:do_after_all).never
+
+        instance.avatar.assign(file)
+      end
+    end
   end

   context 'using validates_attachment' do
-- 
2.8.1

[jrochkind]

These callbacks are not affected by the validity of the model, only by what before_* callbacks might return: https://github.com/thoughtbot/paperclip/blob/master/lib/paperclip/attachment.rb#L99-L117.

The Paperclip validations are written as before_* callbacks however, is my thinking. So that should work.

[jrochkind]

Re: The "proper regression spec" at #2204 (comment)

I don't think that's testing what we want to test. You have before_ callbacks returning false. That might stop after callbacks from being called -- but that's already tested over in attachment_spec.rb. What we want to test is that the content-type validation failure alone prevents after callbacks from being called.

My problem attempting to write the regression spec, is I couldn't get the after callbacks to be called in my spec even without the validation failure! So my regression spec was useless.

[tute]

I couldn't get the after callbacks to be called in my spec even without the validation failure

And I get unexpected calls. This is fun!

[jrochkind]

I've got some more ideas (although not about the failing regression yet).

I wish, from inside the callback block body, you could tell the callback "Okay, I cancel you, don't run after callbacks". That would be the solution to wanting to cancel the double-callback-chain thing (assuming that is desired behavior, which I think it is). But I don't think that feature is in AS::Callbacks; I could PR it, but that's little help to paperclip now, which needs to work with existing and older Rails.

[jrochkind]

Ugh, I realize it gets more complicated.

Let's say you have two paperclip attachments.

You have a generic all-attachment after_processing callback defined.

Ordinarily, would that be called once, or twice? I think twice?

And I think if one of the attachments fails validation... then the general after callback should still be called for the other one? Gets really confusing.

The AS::Callbacks source code is sure confusing too.

[tute]

I know I can't get to say this at the library level, but the moral is: use composed objects instead of callbacks. 😄

[houndci-bot]

Line is too long. [84/80]

[jrochkind]

Okay, I think I just got closer with some crazy throw/catch code (thanks for the hint that Rails5 uses throw/catch, although I'm using it totally different, I wouldn't have remembered that ruby feature even existed).

Not there yet, but maybe closer.

[houndci-bot]

Put a comma after the last item of a multiline hash.

[jrochkind]

Okay, I'm also going to note: The if !@options[:check_validity_before_processing] || !instance.errors.any? guard, at https://github.com/thoughtbot/paperclip/blob/master/lib/paperclip/attachment.rb#L505

Right? It's totally unneccesary. Maybe only now after these other fixes? If you remove it, all tests pass. It's not neccesary to catch paperclip's own validations, because these are implemented as paperclip post_process callbacks, and if the post_process callbacks are correctly stopped as they should be (or as I think they should be; the actual API spec is not entirely clear as I've mentioned), then these cause things to stop anyway.

Unless it's meant to prevent post-processing if other non-paperclip related validations fail. But there are no specs for that (again all specs keep passing in this branch if the line is removed), and I don't think it's working right to do that anyhow.

[jrochkind]

Also, hound here is totally being awful, insisting I put a comma after the last line of a multi-line hash literal, really?

[tute]

Unless it's meant to prevent post-processing if other non-paperclip related validations fail. But there are no specs for that (again all specs keep passing in this branch if the line is removed), and I don't think it's working right to do that anyhow.

I think this is the intended behavior according to other issues, and what we should do is add specs of whatever is current behavior to verify (and not change). I like this PR, and discussion, again, thank you for your help.

Also, hound here is totally being awful, insisting I put a comma after the last line of a multi-line hash literal, really?

Yeah. The reasoning being that adding and removing items leave a clean git diff.

[jrochkind]

Okay, I think I actually have something that basically works, now in this PR.

If there's anyone around that has a sense of the history and scope and uses of Paperclip, they should probably review this.

This actually fixes/changes two things, although I didn't set out to:

1. Paperclip validations. If any paperclip validations fail, no after callbacks will be run. Hooray!
   • all before callbacks are still run. I started out trying to make it so a failed callback cancelled all subsequent callbacks (even later before callbacks), and made that happen -- but it made other specs fail, that expected all paperclip validations in #errors after assignment (not rails validation), which my new behavior broke. So I didn't do it. All before callbacks still run, but no after callbacks.
     • I have no idea if people expect subsequent before callbacks to be cancelled by a validation
       failure too, but since it would break existing specs to do that, I didn't.
   • Other non-paperclip Rails model validation probably does not effect paperclip after callbacks. I have no idea if that's desirable, and I'm not sure it's really possible even if it is. I didn't test this at all.
2. Callbacks in general. Now, after this PR, a before callback returning false will keep all after callbacks from being run (both after_post_processing and attribute-specific eg after_avatar_post_processing).
   • I thought that was related to the validation problem is why I dived down that hole. Turns out, it wasn't, exaclty. While paperclip validations are run as paperclip before callbacks, they never return false to cancel callback chain.
   • I still think it is the "correct" behavior documented by the README.
   • It did require changing a few specs that specifically specified the opposite behave though, that after callbacks were still run even if a before callback cancelled. But I think it's what was suggested by the README "The callbacks are intended to be as close to normal ActiveRecord callbacks as possible, so if you return false (specifically - returning nil is not the same) in a before_filter, the post processing step will halt. " (and not currently working that way).

So that's that. A bit hacky with the try/catch. I don't know if this is really the right approach. Digging into paperclip I wanted to make even huger changes, things are kind of tangled in there, and some huger changes might make possible a different approach here... but I didn't, and it would have invariably created more backwards incompat.

I also ran into huge trouble trying to add specs to an existing spec file, wound up with spec setup in that file messing me up in ways I didn't understand, so I just created a brand new test file.

[tute]

Thanks for all your work, @jrochkind.

Callbacks in general. Now, after this PR, a before callback returning false will keep all after callbacks from being run

Keep in mind that this is default behavior until Rails 4.2. In Rails 5 we have to throw to halt the callback chain.

[tute]

We are probably running against the clock to make deeper changes, but remember is the best time, as we didn't release v5 yet.

I agree that the mocks and stubs are confusing, and also stub the system under test, which is in general an anti-pattern. I'd love to move away from this type of assertion.

[tute]

The new spec attachment_callbacks_spec makes me very happy. ❤️

What do you think of addressing hound comments in new code? Feel free to ignore if you disagree, but I favor them.

[jrochkind]

Keep in mind that this is default behavior until Rails 4.2. In Rails 5 we have to throw to halt the callback chain.

My understanding is this is not quite true.

• Paperclip callbacks should keep working exactly the same in Rails5. Looking at the code Rails5, implements the new default throw :abort behavior with a default AS::Callbacks terminator. However, paperclip uses it's own AS::Callbacks terminator, so the default wont' be used, and we won't get (optional or required) throw :abort behavior automatically, and old behavior will keep working unchanged.
• Additionally, at least the code at time of this writing, even in Rails5 the return false behavior will still be supported with a deprecation message, along with the throw :abort behavior. There is a config variable you can use to turn it off, and it will be presumably be removed entirely in a future Rails (5.x?) version.

So paperclip will keep working exaclty the same in Rails5 -- you will still be able to return false to halt (without deprecation), you will not be able to throw :abort to halt in a paperclip callback.

So the paperclip callbacks will no longer have behavior aligned with Rails5 controller/ActiveModel/ActiveRecord callbacks, as originally intended, at least with regard to termination. But all existing code will keep working exaclty the same without deprecation. If you'd like to align paperclip callbacks with Rails5 default callbacks, you just need to change paperclip's AS::Callbacks terminator.

That's my understanding after looking at the Rails5 code.

Re: Hound: In my opinion, addressing the current hound flags will make the code less readable rather than more, as well as less consistent with other parts of the codebase. But you can feel free to if you like. I can give you commit access to my forked branch if you want, or you can of course manually make changes instead of just using GitHub GUI to do the merge (I think github gives you instructions by the 'merge' button). I think I'm done with this for now myself.

[jrochkind]

Note well also:

This code will not stop subsequent before actions from running after a failed validation.

This means that a spoofed or invalid content_type file can still potenially make it to imagemagick. Imagine a content-type validation followed by an image height/width validation. The content type validation might fail if someone uploads a wrong content type, but the image height/width validation will run anyway. Which could mean sending a banned content_type to imagemagick, if the height/width validation were done with imagemagick. It could also mean simply getting an imagemagick exception when you ask imagemagick (or any other implementation of a height/width validation) to get height/width of a TXT file or something.

I originally intended to make a failed validation halt the callback chain entirely, causing subsequent before actions also not to to be run. (Actually I originally originally assumed this was how it worked, then tried to make it so when I discovered it wasn't!).

I did succesfully implement code that did this, made a failed validation halt the callback chain entirely. However, it caused other paperclip specs to fail, that were basically specifically spec'ing that failed validaitons did not halt the callback chain. So I didn't know what this change would break, so I gave up and called it a day.

[joshuaclayton]

thoughts on moving the conditional to wrap the throw?

[jrochkind]

sure, if you think that's better I can do it. You just mean:

 unless inner_result
   throw :cancel_all_after_callbacks
 end

okay!

[joshuaclayton]

@jrochkind yep, exactly - I might even go so far as to suggest switching to a if !inner_result. The trailing conditional is really what we want to avoid though.

[tute]

I'll give @tute et al rights to commit to my fork if that will help with some collaboration.

I can work on your branch/PR if you'd like, @jrochkind.

[houndci-bot]

Line is too long. [82/80]

[jrochkind]

@tute you now have commit rights on https://github.com/jrochkind/paperclip/tree/failed_validation_cancel_after_post_process now, if you want to try to make hound happy.

Any pushes to that branch end up in this PR.

[jrochkind]

Can I help this move forward?

If you want me to fix all the hound warnings, I can, although I think it's going to make the code crazy to read.

In particular, Hound hates this kind of thing:

def do_before_avatar ; end

Which appears all over the existing codebase, and I followed the style in existing codebase.

And hound wants to make lines shorter I frankly see no way to make shorter and still sensible.

But if that's what you need to move forward, I'll do it!

[tute]

Thank you, @jrochkind. I keep this thread in my inbox to address it. I hope tomorrow Friday to give it a shot.

Also, I let myself sleep on it to take decisions around the general after_post_process callback. I've been wondering if we should keep that in paperclip v5, and if there is any reason to do it more than backwards compatibility.

Thanks for your work and keeping this alive!

[jrochkind]

I encountered the bug with after_post_process and failed validations, because the solidus gem (a Rails ecommerce engine) uses it:

https://github.com/solidusio/solidus/blob/60ee1dffc7c3932cd7652cc2b4566f08a12f74b8/core/app/models/spree/image.rb#L18-L32

It's using it to calculate and save in the db the height and the width of the image.

That seems like a useful thing to do. Is there a better way to do it in current paperclip?

I am also wondering if you are using paperclip to upload non-image files. Say audio files, and you want to do some transcodes on them after uploading (say, re-encode at variable bitrate, or encode in a variety of different audio formats). That obviously is a reasonable thing to do. Is there currently a better way to do that then an after_post_process hook? We could imagine a different API for doing that (is there already one in paperclip?), but I suspect it would have similar issues.

Or, say, if you want to gzip the uploaded thing for some reason. Seems like a reasonable thing to want to do, after_post_process hook seems an appropriate place for it.

In general, I think it is important to have some kind of custom hook for "after paperclip has decided the file is good and is done with it, let me do things to it." It wouldn't need to be an API that looks just like the current one, but if it's an API that allows you to supply custom code of some kind, that gets run only after paperclip is done with things and doesn't get run if paperclip validations fail -- it's going to need similar logic one way or another.

Plus, I think backwards compatibility does matter, a lot, unless there's a very clear reason to break it, your users will really appreciate backwards compatibility. Semver says you have to advertise backwards incompat with a major version change -- but that doesn't mean backwards incompat isn't still painful for your downstream users.

[jrochkind]

PPS: As long as we're looking at that solidus file, you can see here that solidus is having to work around paperclip adding kind of useless validation messages (to the ActiveRecord model itself) in some error conditions.

I haven't looked into exactly what's going on there, but if I were going to vote for things to prioritizing fixing in a next release, that'd be high on the list!

[jrochkind]

Hi, I think this is actually kind of a severe bug. Unfortunately, there seems to be no very simple low-impact-to-code fix for it that I could find, this PR is the best I could do. But I think it's a pretty severe bug that needs fixing. I spent some significant time on it, encouraged by @tute saying:

I can't fix it myself in the foreseeable future, but I'll swiftly merge a PR that addresses this problem and publish releases.

So I tried to help. What are your current thoughts on this? Should I figure there isn't going to be a paperclip release fixing this issue anytime soon?

Would you instead like a PR to README and docs making it clear that after_filters are always run regardless of validation, counter to the current README and docs text? I would rather fix the actual problem, because I think current README describes the 'correct' desirable behavior. But in the absence of a fix, the README and docs should probably describe the actual behavior.

[tute]

I'm sorry I could not get to it on time as I expected. I do keep this in mind, and I intend to go through this. I won't do any releases without addressing this PR, and taking a decision either in code or in docs. Thank you very much for your help triaging this bug and proposing fixes.

[tute]

Would you instead like a PR to README and docs making it clear that after_filters are always run regardless of validation, counter to the current README and docs text?

We just released new paperclip versions. This suggestion is a good workaround until we get to actually tackle the root issue. Would you send that PR fixing the docs? Thank you so much for your work and for your patience.

[mike-burns]

Closing in favor of #2594. Thank you so much for the research and work you had done here!

[ssinghi]

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