Skip to content
This repository has been archived by the owner on Jul 13, 2023. It is now read-only.

Commit

Permalink
@wip Fixed bug where processing occurs on invalid objects
Browse files Browse the repository at this point in the history 
- 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 #2462, Closes #2321, Closes
  #2236, Closes #2178,
  Closes #1960, Closes #2204
  • Loading branch information
@saghaulor
saghaulor committed Aug 19, 2018
1 parent acbb03f commit eaed2af
Show file tree
Hide file tree
Showing 3 changed files with 107 additions and 17 deletions.
4 changes: 2 additions & 2 deletions lib/paperclip/attachment.rb
View file
Expand Up @@ -109,8 +109,6 @@ def assign(uploaded_file)
nil
else
assign_attributes
post_process_file
reset_file_if_original_reprocessed
end
else
nil
Expand Down Expand Up @@ -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)
Expand Down
51 changes: 39 additions & 12 deletions spec/paperclip/attachment_processing_spec.rb
View file
Expand Up @@ -4,49 +4,63 @@
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
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.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
Expand All @@ -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
69 changes: 66 additions & 3 deletions spec/paperclip/attachment_spec.rb
View file
Expand Up @@ -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)
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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

Expand Down

0 comments on commit eaed2af

Please sign in to comment.