Skip to content

Fix larger video files not being transcoded#14306

Merged
Gargron merged 1 commit intomastodon:masterfrom
ClearlyClaire:fixes/file-upload-type
Jul 14, 2020
Merged

Fix larger video files not being transcoded#14306
Gargron merged 1 commit intomastodon:masterfrom
ClearlyClaire:fixes/file-upload-type

Conversation

@ClearlyClaire
Copy link
Copy Markdown
Contributor

Since #14145, the set_type_and_extension has been moved from
before_post_process to before_file_post_process, but while the former
runs before all validations performed by Paperclip, the latter is dependent
on the order validations and hooks are defined.

In our case, this meant video files could be checked against the generic 10MB
limit, causing validation failures, which, internally, make Paperclip skip
post-processing, and thus, transcoding of the video file.

The actual validation would then happen after the type is correctly set, so
the large file would pass validation, but without being transcoded first.

This commit moves the hook definition so that it is run before checking for
the file size.

Since mastodon#14145, the `set_type_and_extension` has been moved from
`before_post_process` to `before_file_post_process`, but while the former
runs before all validations performed by Paperclip, the latter is dependent
on the order validations and hooks are defined.

In our case, this meant video files could be checked against the generic 10MB
limit, causing validation failures, which, internally, make Paperclip skip
post-processing, and thus, transcoding of the video file.

The actual validation would then happen after the type is correctly set, so
the large file would pass validation, but without being transcoded first.

This commit moves the hook definition so that it is run before checking for
the file size.
@ClearlyClaire
Copy link
Copy Markdown
Contributor Author

It's the second time we've been bitten by Paperclip silently skipping some processing because of internal validation error and causing very confusing issues down the line.

This is caused by Paperclip::Attachment#post_process that is defined as such:

    def post_process(*style_args) #:nodoc:
      return if @queued_for_write[:original].nil?

      instance.run_paperclip_callbacks(:post_process) do
        instance.run_paperclip_callbacks(:"#{name}_post_process") do
          if !@options[:check_validity_before_processing] || !instance.errors.any?
            post_process_styles(*style_args)
          end
        end
      end
    end

I guess we may want to monkey-patch it to raise an error whenever instance.errors.any? instead of skipping the post-processing…

@Gargron Gargron merged commit a8e84a1 into mastodon:master Jul 14, 2020
shouo1987 pushed a commit to CrossGate-Pawoo/mastodon that referenced this pull request Dec 7, 2022
Since mastodon#14145, the `set_type_and_extension` has been moved from
`before_post_process` to `before_file_post_process`, but while the former
runs before all validations performed by Paperclip, the latter is dependent
on the order validations and hooks are defined.

In our case, this meant video files could be checked against the generic 10MB
limit, causing validation failures, which, internally, make Paperclip skip
post-processing, and thus, transcoding of the video file.

The actual validation would then happen after the type is correctly set, so
the large file would pass validation, but without being transcoded first.

This commit moves the hook definition so that it is run before checking for
the file size.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants