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

@WIP Fixed bug where processing occurs on invalid objects #2594

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+107 −17
Open

@WIP Fixed bug where processing occurs on invalid objects #2594

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions lib/paperclip/attachment.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
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
Open in desktop
Original file line number Diff line number Diff line change
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
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.
Metrics/LineLength: Line is too long. [83/80]

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
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

file = File.new(fixture_file("5k.png"))
Dummy.validates_attachment :avatar, content_type: {content_type: "image/png"}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Layout/SpaceInsideHashLiteralBraces: Space inside { missing.
Metrics/LineLength: Line is too long. [83/80]
Layout/SpaceInsideHashLiteralBraces: Space inside } missing.

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
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.
Metrics/LineLength: Line is too long. [83/80]

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.
Metrics/LineLength: Line is too long. [83/80]

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
Open in desktop
Original file line number Diff line number Diff line change
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
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Metrics/LineLength: Line is too long. [84/80]

expected_params = @style_params[:once].merge({
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/BracesAroundHashParameters: Redundant curly braces around a hash parameter.

style: :once,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Layout/IndentHash: Use 2 spaces for indentation in a hash, relative to the first position after the preceding left parenthesis.

processors: [:thumbnail, :test],
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/SymbolArray: Use %i or %I for an array of symbols.

whiny: true,
convert_options: "",
source_file_options: ""
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/TrailingCommaInHashLiteral: Put a comma after the last item of a multiline hash.

})
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Layout/IndentHash: Indent the right brace the same as the first position after the preceding left parenthesis.


@dummy.avatar = @file

expect(Paperclip::Thumbnail).not_to have_received(:make).with(anything, expected_params, anything)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Metrics/LineLength: Line is too long. [106/80]

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)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Metrics/LineLength: Line is too long. [99/80]

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
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Metrics/LineLength: Line is too long. [99/80]

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