From 41adcc752a0f098ddef1355023797b76a07d8d41 Mon Sep 17 00:00:00 2001 From: Jonathan Rochkind Date: Tue, 10 May 2016 16:21:07 -0400 Subject: [PATCH 1/8] work toward fixing after_post_process is run even if validations fail --- lib/paperclip/callbacks.rb | 2 +- spec/paperclip/attachment_processing_spec.rb | 31 ++++++++++++++++++++ spec/paperclip/attachment_spec.rb | 6 ++-- 3 files changed, 35 insertions(+), 4 deletions(-) diff --git a/lib/paperclip/callbacks.rb b/lib/paperclip/callbacks.rb index 53a436857..fac815ebd 100644 --- a/lib/paperclip/callbacks.rb +++ b/lib/paperclip/callbacks.rb @@ -7,7 +7,7 @@ def self.included(base) module Defining def define_paperclip_callbacks(*callbacks) - define_callbacks(*[callbacks, { terminator: hasta_la_vista_baby }].flatten) + define_callbacks(*[callbacks, { terminator: hasta_la_vista_baby, skip_after_callbacks_if_terminated: true }].flatten) callbacks.each do |callback| eval <<-end_callbacks def before_#{callback}(*args, &blk) diff --git a/spec/paperclip/attachment_processing_spec.rb b/spec/paperclip/attachment_processing_spec.rb index 8243036c9..cb06a287f 100644 --- a/spec/paperclip/attachment_processing_spec.rb +++ b/spec/paperclip/attachment_processing_spec.rb @@ -44,6 +44,37 @@ 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 + ## FALSE POSITIVE: This passes even before our change in callbacks.rb, + # even though we know it had a problem with this. This passes + # even if we don't trigger a validation error, we haven't succesfully + # set up our callbacks at all somehow. + it 'does not run custom post-processing if validation fails' do + file = File.new(fixture_file("5k.png")) + + instance = Dummy.new + attachment = instance.avatar + + attachment.expects(:do_after_avatar).never + attachment.expects(:do_after_all).never + + attachment.assign(file) + end + end end context 'using validates_attachment' do diff --git a/spec/paperclip/attachment_spec.rb b/spec/paperclip/attachment_spec.rb index c95cd5a5a..9c8b9bf2f 100644 --- a/spec/paperclip/attachment_spec.rb +++ b/spec/paperclip/attachment_spec.rb @@ -847,16 +847,16 @@ def do_after_all; end @dummy.expects(:do_before_avatar).never @dummy.expects(:do_after_avatar).never @dummy.expects(:do_before_all).with().returns(false) - @dummy.expects(:do_after_all) + @dummy.expects(:do_after_all).never Paperclip::Thumbnail.expects(:make).never @dummy.avatar = @file end it "cancels the processing if a before_avatar_post_process returns false" do @dummy.expects(:do_before_avatar).with().returns(false) - @dummy.expects(:do_after_avatar) + @dummy.expects(:do_after_avatar).never @dummy.expects(:do_before_all).with().returns(true) - @dummy.expects(:do_after_all) + @dummy.expects(:do_after_all).never Paperclip::Thumbnail.expects(:make).never @dummy.avatar = @file end From dc63b13e7496ab6618386024c2988ebd031222c7 Mon Sep 17 00:00:00 2001 From: Jonathan Rochkind Date: Tue, 10 May 2016 18:52:03 -0400 Subject: [PATCH 2/8] Cancelled attribute-specific callbacks will prevent general callbacks from being called --- lib/paperclip/attachment.rb | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/lib/paperclip/attachment.rb b/lib/paperclip/attachment.rb index 773ad24aa..6f3299882 100644 --- a/lib/paperclip/attachment.rb +++ b/lib/paperclip/attachment.rb @@ -499,12 +499,15 @@ def process_options(options_type, style) #:nodoc: def post_process(*style_args) #:nodoc: return if @queued_for_write[:original].nil? - - instance.run_paperclip_callbacks(:post_process) do - instance.run_paperclip_callbacks(:"#{name}_post_process") do - if !@options[:check_validity_before_processing] || !instance.errors.any? - post_process_styles(*style_args) + catch(:cancel_outer_callbacks) do + instance.run_paperclip_callbacks(:post_process) do + inner_result = instance.run_paperclip_callbacks(:"#{name}_post_process") do + if !@options[:check_validity_before_processing] || !instance.errors.any? + post_process_styles(*style_args) + end + true end + throw :cancel_outer_callbacks unless inner_result end end end From cac2921d40c6c666d278e5274ab70cfe8c30c84e Mon Sep 17 00:00:00 2001 From: Jonathan Rochkind Date: Tue, 10 May 2016 18:55:15 -0400 Subject: [PATCH 3/8] shut up hound --- lib/paperclip/callbacks.rb | 6 +++++- spec/paperclip/attachment_processing_spec.rb | 5 ++++- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/lib/paperclip/callbacks.rb b/lib/paperclip/callbacks.rb index fac815ebd..89aa15eea 100644 --- a/lib/paperclip/callbacks.rb +++ b/lib/paperclip/callbacks.rb @@ -7,7 +7,11 @@ def self.included(base) module Defining def define_paperclip_callbacks(*callbacks) - define_callbacks(*[callbacks, { terminator: hasta_la_vista_baby, skip_after_callbacks_if_terminated: true }].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/spec/paperclip/attachment_processing_spec.rb b/spec/paperclip/attachment_processing_spec.rb index cb06a287f..78f701723 100644 --- a/spec/paperclip/attachment_processing_spec.rb +++ b/spec/paperclip/attachment_processing_spec.rb @@ -54,8 +54,11 @@ 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 @@ -63,7 +66,7 @@ def do_after_all; end # even though we know it had a problem with this. This passes # even if we don't trigger a validation error, we haven't succesfully # set up our callbacks at all somehow. - it 'does not run custom post-processing if validation fails' do + it "does not run custom post-processing if validation fails" do file = File.new(fixture_file("5k.png")) instance = Dummy.new From f8b849cc543406dd9c924e3a653ef91dc2e1371d Mon Sep 17 00:00:00 2001 From: Jonathan Rochkind Date: Tue, 10 May 2016 19:00:33 -0400 Subject: [PATCH 4/8] really, hound? --- lib/paperclip/callbacks.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/paperclip/callbacks.rb b/lib/paperclip/callbacks.rb index 89aa15eea..6c55eab5b 100644 --- a/lib/paperclip/callbacks.rb +++ b/lib/paperclip/callbacks.rb @@ -9,7 +9,7 @@ module Defining def define_paperclip_callbacks(*callbacks) options = { terminator: hasta_la_vista_baby, - skip_after_callbacks_if_terminated: true + skip_after_callbacks_if_terminated: true, } define_callbacks(*[callbacks, options].flatten) callbacks.each do |callback| From a99fb94b0d3ff942d5bd7d5cf63f876d6db55ac3 Mon Sep 17 00:00:00 2001 From: Jonathan Rochkind Date: Wed, 11 May 2016 14:25:46 -0400 Subject: [PATCH 5/8] good spec for after callbacks not called on validation fail --- spec/paperclip/attachment_callbacks_spec.rb | 56 +++++++++++++++++++++ 1 file changed, 56 insertions(+) create mode 100644 spec/paperclip/attachment_callbacks_spec.rb diff --git a/spec/paperclip/attachment_callbacks_spec.rb b/spec/paperclip/attachment_callbacks_spec.rb new file mode 100644 index 000000000..2fc3e2949 --- /dev/null +++ b/spec/paperclip/attachment_callbacks_spec.rb @@ -0,0 +1,56 @@ +require 'spec_helper' + +describe Paperclip::Attachment do + context "callbacks" do + context "with content_type validation" do + def rebuild(required_content_type) + rebuild_class styles: { something: "100x100#" } + Dummy.class_eval do + validates_attachment_content_type :avatar, content_type: [required_content_type] + 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 + @dummy = Dummy.new + end + + context "that passes" do + let!(:required_content_type) { "image/png" } + before { rebuild(required_content_type) } + let(:fake_file) { StringIO.new(".").tap { |s| s.stubs(:to_tempfile).returns(s) } } + + it "calls all callbacks when assigned" do + @dummy.expects(:do_before_avatar).with() + @dummy.expects(:do_after_avatar).with() + @dummy.expects(:do_before_all).with() + @dummy.expects(:do_after_all).with() + Paperclip::Thumbnail.expects(:make).returns(fake_file) + @dummy.avatar = File.new(fixture_file("5k.png")) + end + end + + context "that fails" do + let!(:required_content_type) { "image/jpeg" } + before { rebuild(required_content_type) } + + it "does not call after callbacks when assigned" do + # before callbacks ARE still called at present + @dummy.expects(:do_before_all).with() + @dummy.expects(:do_before_avatar).with() + + # But after_* are not + @dummy.expects(:do_after_avatar).never + @dummy.expects(:do_after_all).never + Paperclip::Thumbnail.expects(:make).never + + @dummy.avatar = File.new(fixture_file("5k.png")) + end + end + end + end +end From e6fa5bafd061f27d79af48d01d9382a59963057a Mon Sep 17 00:00:00 2001 From: Jonathan Rochkind Date: Wed, 11 May 2016 14:26:10 -0400 Subject: [PATCH 6/8] make after callbacks actually not called on paperclip validation error --- lib/paperclip/attachment.rb | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/lib/paperclip/attachment.rb b/lib/paperclip/attachment.rb index 6f3299882..5063061af 100644 --- a/lib/paperclip/attachment.rb +++ b/lib/paperclip/attachment.rb @@ -50,7 +50,7 @@ def self.default_options # +url+ - a relative URL of the attachment. This is interpolated using +interpolator+ # +path+ - where on the filesystem to store the attachment. This is interpolated using +interpolator+ # +styles+ - a hash of options for processing the attachment. See +has_attached_file+ for the details - # +only_process+ - style args to be run through the post-processor. This defaults to the empty list (which is + # +only_process+ - style args to be run through the post-processor. This defaults to the empty list (which is # a special case that indicates all styles should be processed) # +default_url+ - a URL for the missing image # +default_style+ - the style to use when an argument is not specified e.g. #url, #path @@ -499,15 +499,17 @@ def process_options(options_type, style) #:nodoc: def post_process(*style_args) #:nodoc: return if @queued_for_write[:original].nil? - catch(:cancel_outer_callbacks) do + catch(:cancel_all_after_callbacks) do instance.run_paperclip_callbacks(:post_process) do inner_result = instance.run_paperclip_callbacks(:"#{name}_post_process") do - if !@options[:check_validity_before_processing] || !instance.errors.any? - post_process_styles(*style_args) + # paperclip validations set instance.errors at this point... + unless !@options[:check_validity_before_processing] || !instance.errors.any? + throw :cancel_all_after_callbacks end + post_process_styles(*style_args) true end - throw :cancel_outer_callbacks unless inner_result + throw :cancel_all_after_callbacks unless inner_result end end end From 23d2ecb4d3b7691cfb6153b3d436eb36c33702e5 Mon Sep 17 00:00:00 2001 From: Jonathan Rochkind Date: Wed, 11 May 2016 14:35:28 -0400 Subject: [PATCH 7/8] remove tentative specs that never worked, replaced with separate file --- spec/paperclip/attachment_processing_spec.rb | 34 -------------------- 1 file changed, 34 deletions(-) diff --git a/spec/paperclip/attachment_processing_spec.rb b/spec/paperclip/attachment_processing_spec.rb index 78f701723..8243036c9 100644 --- a/spec/paperclip/attachment_processing_spec.rb +++ b/spec/paperclip/attachment_processing_spec.rb @@ -44,40 +44,6 @@ 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 - ## FALSE POSITIVE: This passes even before our change in callbacks.rb, - # even though we know it had a problem with this. This passes - # even if we don't trigger a validation error, we haven't succesfully - # set up our callbacks at all somehow. - it "does not run custom post-processing if validation fails" do - file = File.new(fixture_file("5k.png")) - - instance = Dummy.new - attachment = instance.avatar - - attachment.expects(:do_after_avatar).never - attachment.expects(:do_after_all).never - - attachment.assign(file) - end - end end context 'using validates_attachment' do From dc602454c101b2413f3347748d57c978b3bfa451 Mon Sep 17 00:00:00 2001 From: Jonathan Rochkind Date: Thu, 12 May 2016 15:03:02 -0400 Subject: [PATCH 8/8] clean up throw conditions --- lib/paperclip/attachment.rb | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/lib/paperclip/attachment.rb b/lib/paperclip/attachment.rb index 5063061af..e4d35c713 100644 --- a/lib/paperclip/attachment.rb +++ b/lib/paperclip/attachment.rb @@ -503,13 +503,15 @@ def post_process(*style_args) #:nodoc: instance.run_paperclip_callbacks(:post_process) do inner_result = instance.run_paperclip_callbacks(:"#{name}_post_process") do # paperclip validations set instance.errors at this point... - unless !@options[:check_validity_before_processing] || !instance.errors.any? + if @options[:check_validity_before_processing] && instance.errors.any? throw :cancel_all_after_callbacks end post_process_styles(*style_args) true end - throw :cancel_all_after_callbacks unless inner_result + unless inner_result + throw :cancel_all_after_callbacks + end end end end