validate_attachment_type_content_matcher: Short-curcuiting && means side effects do not happen; confusion around where errors are added

[cofiem]

Similar, but not exactly the same, as #792 #1158 #1334 #161

Short-curcuiting && means side-effects of methods are not in effect

The return in matches? cannot be done as it is on line 38. This is because && short-curcuits when a return value is false.

A failure will call failure_message. Due to failure_message calling accepted_types_and_failures and rejected_types_and_failures, @missing_rejected_types and @missing_allowed_types must have values (any? is called on them). These two instance variables are set in the allowed_types_allowed? and rejected_types_rejected? methods.

The short-curcuit will mean that one or both of allowed_types_allowed? and rejected_types_rejected? are not called. The end result of all of this is the error discussed in #1334: undefined methodany?' for nil:NilClass`

One possible fix is to replace line 37 and 38 with:

rejected_t = rejected_types_rejected?
allowed_t = allowed_types_allowed?
@allowed_types && @rejected_types && allowed_t && rejected_t

Confusion around where errors are added

Due to #1253, there is confusion about where to look for errors on @subject in the type_allowed? method.

Currently line 85 only checks @subject.errors[:"#{@attachment_name}_content_type"]. It should also check @subject.errors[:"#{@attachment_name}"].

A possible fix is to replace line 85 with:

content_type_any = @subject.errors[:"#{@attachment_name}_content_type"].any? { |item| item == 'is invalid' }
type_any = @subject.errors[:"#{@attachment_name}"].any? { |item| item == 'is invalid' }
!content_type_any && !type_any

[cofiem]

Repro in progress using paperclip_demo app.

The second issue - confusion around where errors are added - causes validate_attachment_content_type to always fail, as the content types to reject are never rejected.

[cofiem]

using paperclip-4.1.1

[cofiem]

failing test for short-circuiting && problem: https://github.com/cofiem/paperclip_demo/compare/thoughtbot:master...master

[jyurek]

This is interesting. After I apply the suggested fix, I still get a few errors in the demo app. Is that intended to highlight things that should be failing?

[cofiem]

Yes, there are still some errors. I don't think they're all part of what I'm talking about here. I'll check, then comment more here.

[cofiem]

Ok, that was a little confusing, sorry about that. I've added comments to the tests I added, and commented out irrelevant tests. Have a look at this commit: https://github.com/cofiem/paperclip_demo/commit/767bff943c544835000df7b436b3f2c7c85e584c

Unfortunately I haven't been able to reproduce the second problem I mentioned in the issue description: 'Confusion around where errors are added'.

I have found something else, however: two of the tests have errors I do not expect (see comments above tests in commit). I believe the problem is on line 43 or 45 of paperclip / lib / paperclip / matchers / validate_attachment_content_type_matcher.rb. One of either accepted_types_and_failures or rejected_types_and_failures may return nil, which cannot be converted into a string (can't convert nil into String).

All in, all, it seems to me there are quite a few assumptions of non-empty arrays and assumptions of ordering of method calls.

[maclover7]

Hi @cofiem ! Is this still an issue for you in Paperclip; I know this issue is from approximately 1 year ago. If it is still an issue, can you please provide the code that's causing you the error? Thanks!

[cofiem]

@maclover7 I haven't looked at this in quite some time. The code this issue refers to has most likely changed in the year since. Please refer to the tests in the fork I linked to in earlier comments.

[tute]

I don't think the error message is a problem, as they are always present at that location or the suggested addition one, too.
We'll merge the PR fixing the other bug.
Thanks!