From eaed2af4c78bbd6f40b77e62cfd1e3aa4d4454d9 Mon Sep 17 00:00:00 2001 From: Stephen Aghaulor Date: Thu, 26 Apr 2018 09:52:13 -0700 Subject: [PATCH] @WIP Fixed bug where processing occurs on invalid objects - Because the processors were called on assignment, instead of during saving, the validations could never work correctly. This is because the built in validations use the values in the db columns to operate. However, since these are populated on assignment, the validations cannot run before the processors run. Moreover, any other type of validation not dependent on the db columns also cannot run, because the processors are called on assignment. The processors should be called during save which allows for validations to occur. - Fixed tests that assert the incorrect behavior - Closes thoughtbot/paperclip#2462, Closes thoughtbot/paperclip#2321, Closes thoughtbot/paperclip#2236, Closes thoughtbot/paperclip#2178, Closes thoughtbot/paperclip#1960, Closes thoughtbot/paperclip#2204 --- lib/paperclip/attachment.rb | 4 +- spec/paperclip/attachment_processing_spec.rb | 51 +++++++++++---- spec/paperclip/attachment_spec.rb | 69 +++++++++++++++++++- 3 files changed, 107 insertions(+), 17 deletions(-) diff --git a/lib/paperclip/attachment.rb b/lib/paperclip/attachment.rb index 65c26ddc7..f2e7d672a 100644 --- a/lib/paperclip/attachment.rb +++ b/lib/paperclip/attachment.rb @@ -109,8 +109,6 @@ def assign(uploaded_file) nil else assign_attributes - post_process_file - reset_file_if_original_reprocessed end else nil @@ -238,6 +236,8 @@ def dirty? # Saves the file, if there are no errors. If there are, it flushes them to # the instance's errors and returns false, cancelling the save. def save + post_process_file + reset_file_if_original_reprocessed flush_deletes unless @options[:keep_old_files] process = only_process if process.any? && !process.include?(:original) diff --git a/spec/paperclip/attachment_processing_spec.rb b/spec/paperclip/attachment_processing_spec.rb index f0231440f..e90c0c0f3 100644 --- a/spec/paperclip/attachment_processing_spec.rb +++ b/spec/paperclip/attachment_processing_spec.rb @@ -4,34 +4,47 @@ before { rebuild_class } context 'using validates_attachment_content_type' do - it 'processes attachments given a valid assignment' do + it "doesn't process attachments on 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.expects(:post_process_styles).never attachment.assign(file) end - it 'does not process attachments given an invalid assignment with :not' do + it 'processes attachments given the object is valid' 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.assign(file) + attachment.expects(:post_process_styles) + + attachment.save + end + + it 'does not process attachments given an invalid object 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.assign(file) attachment.expects(:post_process_styles).never - attachment.assign(file) + attachment.save end - it 'does not process attachments given an invalid assignment with :content_type' do + it 'does not process attachments given an invalid object 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.assign(file) attachment.expects(:post_process_styles).never - attachment.assign(file) + attachment.save end it 'allows what would be an invalid assignment when validation :if clause returns false' do @@ -39,14 +52,15 @@ Dummy.validates_attachment_content_type :avatar, content_type: "image/tiff", if: lambda{false} instance = Dummy.new attachment = instance.avatar + attachment.assign(invalid_assignment) attachment.expects(:post_process_styles) - attachment.assign(invalid_assignment) + attachment.save end end context 'using validates_attachment' do - it 'processes attachments given a valid assignment' do + it "doesn't process attachments on assignment" do file = File.new(fixture_file("5k.png")) Dummy.validates_attachment :avatar, content_type: {content_type: "image/png"} instance = Dummy.new @@ -56,24 +70,37 @@ attachment.assign(file) end - it 'does not process attachments given an invalid assignment with :not' do + it 'processes attachments given a valid object' 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.assign(file) + attachment.expects(:post_process_styles) + + attachment.save + end + + it 'does not process attachments given an invalid object 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.assign(file) attachment.expects(:post_process_styles).never - attachment.assign(file) + attachment.save end - it 'does not process attachments given an invalid assignment with :content_type' do + it 'does not process attachments given an invalid object 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.assign(file) attachment.expects(:post_process_styles).never - attachment.assign(file) + attachment.save end end end diff --git a/spec/paperclip/attachment_spec.rb b/spec/paperclip/attachment_spec.rb index 76d7e9fb4..d766e7162 100644 --- a/spec/paperclip/attachment_spec.rb +++ b/spec/paperclip/attachment_spec.rb @@ -711,9 +711,10 @@ class Paperclip::Test < Paperclip::Processor; end Paperclip::Thumbnail.stubs(:make).returns(@file) end - context "when assigned" do + context "when saved" do it "calls #make on all specified processors" do @dummy.avatar = @file + @dummy.save expect(Paperclip::Thumbnail).to have_received(:make) expect(Paperclip::Test).to have_received(:make) @@ -729,12 +730,14 @@ class Paperclip::Test < Paperclip::Processor; end }) @dummy.avatar = @file + @dummy.save expect(Paperclip::Thumbnail).to have_received(:make).with(anything, expected_params, anything) end it "calls #make with attachment passed as third argument" do @dummy.avatar = @file + @dummy.save expect(Paperclip::Test).to have_received(:make).with(anything, anything, @dummy.avatar) end @@ -745,6 +748,42 @@ class Paperclip::Test < Paperclip::Processor; end @dummy.avatar = @file end end + + context "when assigned" do + it "doesn't call #make on all specified processors" do + @dummy.avatar = @file + + expect(Paperclip::Thumbnail).not_to have_received(:make) + expect(Paperclip::Test).not_to have_received(:make) + end + + it "doesn't call #make with the right parameters passed as second argument" do + expected_params = @style_params[:once].merge({ + style: :once, + processors: [:thumbnail, :test], + whiny: true, + convert_options: "", + source_file_options: "" + }) + + @dummy.avatar = @file + + expect(Paperclip::Thumbnail).not_to have_received(:make).with(anything, expected_params, anything) + end + + it "doesn't call #make with attachment passed as third argument" do + @dummy.avatar = @file + + expect(Paperclip::Test).not_to have_received(:make).with(anything, anything, @dummy.avatar) + end + + it "doesn't call #make and unlinks intermediary files afterward" do + # TODO: Fix this test? + @dummy.avatar.expects(:unlink_files).with([@file, @file]) + + @dummy.avatar = @file + end + end end context "An attachment with a processor that returns original file" do @@ -758,8 +797,18 @@ def make; @file; end @dummy = Dummy.new end + context "when saved" do + it "calls #make and doesn't unlink the original file" do + @dummy.avatar.expects(:unlink_files).with([]) + + @dummy.avatar = @file + @dummy.save + end + end + context "when assigned" do - it "#calls #make and doesn't unlink the original file" do + it "doesn't call #make and doesn't unlink the original file" do + # TODO: Fix this test? @dummy.avatar.expects(:unlink_files).with([]) @dummy.avatar = @file @@ -822,6 +871,7 @@ def make; @file; end end context "Assigning an attachment with post_process hooks" do + # NOTE: Investigate the before_post_process hooks, as they may be intended to run on assignment before do rebuild_class styles: { something: "100x100#" } Dummy.class_eval do @@ -841,13 +891,23 @@ def do_after_all; end @attachment = @dummy.avatar end - it "calls the defined callbacks when assigned" do + it "calls the defined callbacks when saved" 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(@file) @dummy.avatar = @file + @dummy.save + end + + it "doesn't call the defined callbacks when assigned" do + @dummy.expects(:do_before_avatar).never + @dummy.expects(:do_after_avatar).never + @dummy.expects(:do_before_all).never + @dummy.expects(:do_after_all).never + Paperclip::Thumbnail.expects(:make).never + @dummy.avatar = @file end it "does not cancel the processing if a before_post_process returns nil" do @@ -857,6 +917,7 @@ def do_after_all; end @dummy.expects(:do_after_all).with() Paperclip::Thumbnail.expects(:make).returns(@file) @dummy.avatar = @file + @dummy.save end it "cancels the processing if a before_post_process returns false" do @@ -866,6 +927,7 @@ def do_after_all; end @dummy.expects(:do_after_all) Paperclip::Thumbnail.expects(:make).never @dummy.avatar = @file + @dummy.save end it "cancels the processing if a before_avatar_post_process returns false" do @@ -875,6 +937,7 @@ def do_after_all; end @dummy.expects(:do_after_all) Paperclip::Thumbnail.expects(:make).never @dummy.avatar = @file + @dummy.save end end