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

saghaulor · 2018-04-26T18:16:39Z

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 Post processing callbacks are run on invalid attachments. #2462, Closes Conflict between README and Wiki on Post Processing. #2321, Closes
contradictory wiki / readme about preprocessing invalid too-large files #2236, Closes after_post_process is run even if validations fail #2178,
Closes Failing validations don't prevent post_processing callbacks #1960, Closes DRAFT toward fixing after_post_process is run even if validations fail #2204

houndci-bot · 2018-04-26T18:16:53Z

spec/paperclip/attachment_spec.rb

@@ -807,6 +856,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


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

houndci-bot · 2018-04-26T18:16:53Z

spec/paperclip/attachment_spec.rb

+      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)


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

houndci-bot · 2018-04-26T18:16:53Z

spec/paperclip/attachment_spec.rb

+
+        @dummy.avatar = @file
+
+        expect(Paperclip::Thumbnail).not_to have_received(:make).with(anything, expected_params, anything)


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

houndci-bot · 2018-04-26T18:16:54Z

spec/paperclip/attachment_spec.rb

+          whiny: true,
+          convert_options: "",
+          source_file_options: ""
+        })


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

houndci-bot · 2018-04-26T18:16:54Z

spec/paperclip/attachment_spec.rb

+          processors: [:thumbnail, :test],
+          whiny: true,
+          convert_options: "",
+          source_file_options: ""


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

houndci-bot · 2018-04-26T18:16:55Z

spec/paperclip/attachment_processing_spec.rb

-    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"}


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

houndci-bot · 2018-04-26T18:16:55Z

spec/paperclip/attachment_processing_spec.rb

@@ -57,24 +71,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


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

houndci-bot · 2018-04-26T18:16:55Z

spec/paperclip/attachment_processing_spec.rb

    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


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]

houndci-bot · 2018-04-26T18:16:55Z

spec/paperclip/attachment_processing_spec.rb

+      attachment.save
+    end
+
+    it 'does not process attachments given an invalid object with :not' do


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

houndci-bot · 2018-04-26T18:16:55Z

spec/paperclip/attachment_processing_spec.rb


      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


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

saghaulor · 2018-04-26T20:29:59Z

@jrochkind @tute Perhaps I'm mistaken, but I believe all these problems are because the processors were being called on assignment, rather then during saving. Perhaps you can chime in and let me know if I'm missing something.

I'm trying to fix tests that seem to assert incorrect behavior, and get this PR ready for merging (assuming my assumptions about the nature of the bug are correct).

jrochkind · 2018-04-26T20:34:23Z

Sorry, it's been around 12 months since I last used paperclip, and I no longer work on the project where I encountered the issue I filed... it turns out almost exactly two years ago to this day. (anniversary, woo).

saghaulor · 2018-04-26T20:51:34Z

@jrochkind no worries, thanks for all the work you previously did!

mike-burns · 2018-04-27T15:30:25Z

Hi @saghaulor thank you for tracking down the cause of those bugs!

We're looking to cut a few bugfix releases of Paperclip while people are transitioning to Rails 5.2's ActiveStore. My concern is that this bugfix will introduce unexpected behavior elsewhere.

Do you know what kind of impact this will have on existing Paperclip uses?

jrochkind · 2018-04-27T15:46:16Z

We're looking to cut a few bugfix releases of Paperclip while people are transitioning to Rails 5.2's ActiveStore.

Are you saying you expect to retire Paperclip in light of ActiveStorage? I hadn't heard that.

mike-burns · 2018-04-27T15:50:25Z

I hadn't heard that.

It's not official yet, just something we're working on.

jrochkind · 2018-04-27T16:15:18Z

Thanks. Providing instructions for those who want to migrate to AS does not necessarily imply that you are sunsetting Paperclip, but sounds like you are strongly considering it, ok.

saghaulor · 2018-04-30T00:12:26Z

@mike-burns I'm still trying to determine if there are any side effects. From cursory testing it seems that it will behave as one would expect. However, as I'm sure you're well aware, it's very difficult to account for the myriad of use cases users have concocted.

It's sad to hear that Paperclip may be deprecated. From what I saw, ActiveStorage looks promising but is not a sufficient replacement for Paperclip. Moreover, migrating may be quite an undertaking if users have numerous models with attachments.

Unless I'm mistaken, ActiveStorage doesn't offer the processor plugin framework nor the security mechanisms that Paperclip offer. Granted, some of these can be reimplemented as modules.

mike-burns · 2018-05-25T19:52:18Z

Talked with @sidraval IRL. Let's do this:

6.1 with everything that's in master now.
7.0 with this commit merged.
7.1 with a deprecation notice across all of Paperclip.

We have concerns about this PR being a breaking change for some people who have worked around this bug. But we also want to see this bug fixed.

Sound good, @saghaulor ?

mike-burns · 2018-07-27T20:20:34Z

6.1 is out. Time to merge this.

@saghaulor I'm getting some 61 test failures locally after rebasing with master. Do you have the bandwidth to get these passing, or at least get it to a <20 number of failures?

saghaulor · 2018-07-27T20:24:04Z

Sorry about the lack of movement. I've been traveling a lot recently. I'm hoping to work on this and hopefully have it ready in the next couple of weeks.

On Fri, Jul 27, 2018 at 4:20 PM Mike Burns ***@***.***> wrote: 6.1 is out. Time to merge this. @saghaulor <https://github.com/saghaulor> I'm getting some 61 test failures locally after rebasing with master. Do you have the bandwidth to get these passing, or at least get it to a <20 number of failures? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2594 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAQtxjNCvEKRRWYXRtY9Mv7yg9w8EJ8eks5uK3YZgaJpZM4TmlJH> .

-- Dutifully yours, Stephen Aghaulor

mike-burns · 2018-07-27T20:29:19Z

Thank you! And I totally understand.

- 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#2462, Closes thoughtbot#2321, Closes thoughtbot#2236, Closes thoughtbot#2178, Closes thoughtbot#1960, Closes thoughtbot#2204

houndci-bot · 2018-08-19T01:04:52Z

spec/paperclip/attachment_processing_spec.rb

    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


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]

houndci-bot · 2018-08-19T01:04:52Z

spec/paperclip/attachment_processing_spec.rb

+      attachment.save
+    end
+
+    it 'does not process attachments given an invalid object with :not' do


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

ssinghi · 2020-08-06T17:04:23Z

@saghaulor This bug has been fixed in https://github.com/kreeti/kt-paperclip.

houndci-bot reviewed Apr 26, 2018

View reviewed changes

mike-burns added the Merge before deprecating label Apr 27, 2018

mike-burns mentioned this pull request May 25, 2018

DRAFT toward fixing after_post_process is run even if validations fail #2204

Closed

saghaulor force-pushed the processor_validation_fix branch from 90b2060 to eaed2af Compare August 19, 2018 01:04

houndci-bot reviewed Aug 19, 2018

View reviewed changes

anupamcis approved these changes May 2, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

saghaulor commented Apr 26, 2018

houndci-bot Apr 26, 2018

houndci-bot Apr 26, 2018

houndci-bot Apr 26, 2018

houndci-bot Apr 26, 2018

houndci-bot Apr 26, 2018

houndci-bot Apr 26, 2018

houndci-bot Apr 26, 2018

houndci-bot Apr 26, 2018

houndci-bot Apr 26, 2018

houndci-bot Apr 26, 2018

saghaulor commented Apr 26, 2018

jrochkind commented Apr 26, 2018 •

edited

saghaulor commented Apr 26, 2018

mike-burns commented Apr 27, 2018

jrochkind commented Apr 27, 2018

mike-burns commented Apr 27, 2018

jrochkind commented Apr 27, 2018

saghaulor commented Apr 30, 2018

mike-burns commented May 25, 2018

mike-burns commented Jul 27, 2018

saghaulor commented Jul 27, 2018 via email

mike-burns commented Jul 27, 2018

houndci-bot Aug 19, 2018

houndci-bot Aug 19, 2018

ssinghi commented Aug 6, 2020


		@dummy.avatar = @file

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

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

Are you sure you want to change the base?

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

Conversation

saghaulor commented Apr 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saghaulor commented Apr 26, 2018

jrochkind commented Apr 26, 2018 • edited

saghaulor commented Apr 26, 2018

mike-burns commented Apr 27, 2018

jrochkind commented Apr 27, 2018

mike-burns commented Apr 27, 2018

jrochkind commented Apr 27, 2018

saghaulor commented Apr 30, 2018

mike-burns commented May 25, 2018

mike-burns commented Jul 27, 2018

saghaulor commented Jul 27, 2018 via email

mike-burns commented Jul 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssinghi commented Aug 6, 2020

jrochkind commented Apr 26, 2018 •

edited