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

Background processing #2261

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
34 changes: 33 additions & 1 deletion lib/paperclip/attachment.rb
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ def self.default_options
:hash_digest => "SHA1",
:interpolator => Paperclip::Interpolations,
:only_process => [],
:process_in_background => [],

Choose a reason for hiding this comment

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

Use the new Ruby 1.9 hash syntax.

:path => ":rails_root/public:url",
:preserve_files => false,
:processors => [:thumbnail],
Expand Down Expand Up @@ -50,8 +51,10 @@ def self.default_options
# +url+ - a relative URL of the attachment. This is interpolated using +interpolator+
# +path+ - where on the filesystem to store the attachment. This is interpolated using +interpolator+
# +styles+ - a hash of options for processing the attachment. See +has_attached_file+ for the details
# +only_process+ - style args to be run through the post-processor. This defaults to the empty list (which is
# +only_process+ - style args to be run through the post-processor. This defaults to the empty list (which is

Choose a reason for hiding this comment

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

Line is too long. [113/80]

# a special case that indicates all styles should be processed)
# +process_in_background+ - an array of styles to be processed in background
# (requires "active_job")
# +default_url+ - a URL for the missing image
# +default_style+ - the style to use when an argument is not specified e.g. #url, #path
# +storage+ - the storage mechanism. Defaults to :filesystem
Expand Down Expand Up @@ -309,6 +312,12 @@ def updated_at
time && time.to_f.to_i
end

# Returns true when record is enqueued for background processing
# and job did not finish yet.
def processing_in_background
instance_read(:processing_in_background)
end

# The time zone to use for timestamp interpolation. Using the default
# time zone ensures that results are consistent across all threads.
def time_zone
Expand Down Expand Up @@ -383,6 +392,21 @@ def instance_read(attr)
end
end

def generate_style_files(*styles)
@file = Paperclip.io_adapters.for(self)
@queued_for_write[:original] = @file
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems to be little hacky, but without this other styles won't process

post_process(*styles)
flush_writes
end

def clear_enqueue_background_processing
@enqueue_background_processing = false
end

def enqueue_background_processing?
@enqueue_background_processing
end

private

def path_option
Expand Down Expand Up @@ -430,6 +454,7 @@ def assign_attributes
assign_file_information
assign_fingerprint { @file.fingerprint }
assign_timestamps
assign_background_processing
end

def assign_file_information
Expand All @@ -452,6 +477,13 @@ def assign_timestamps
instance_write(:updated_at, Time.now)
end

def assign_background_processing
if @options[:process_in_background].any?
instance_write(:processing_in_background, true)
@enqueue_background_processing = true
end
end

def post_process_file
dirty!

Expand Down
15 changes: 15 additions & 0 deletions lib/paperclip/has_attached_file.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ def define
define_setter
define_query
register_new_attachment
add_background_processing if @options[:process_in_background]
add_active_record_callbacks
add_paperclip_callbacks
add_required_validations
Expand Down Expand Up @@ -100,6 +101,20 @@ def add_active_record_callbacks
end
end

def add_background_processing
require "paperclip/process_job"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added require in this place, to not force active_job presence for people that won't use this feature

name = @name
if @klass.respond_to?(:after_commit)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I care for case when there is no after_commit and add after_save then?

@klass.send(:after_commit, on: [:create, :update]) do
attachment = send(name)
if attachment.enqueue_background_processing?
ProcessJob.perform_later(self, name.to_s)
attachment.clear_enqueue_background_processing
end
end
end
end

def add_paperclip_callbacks
@klass.send(
:define_paperclip_callbacks,
Expand Down
21 changes: 21 additions & 0 deletions lib/paperclip/process_job.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
begin
require "active_job"
rescue LoadError
raise LoadError,
"To use background processing you have to include 'active_job' in load path"

Choose a reason for hiding this comment

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

Align the parameters of a method call if they span more than one line.

end

module Paperclip
class ProcessJob < ActiveJob::Base
def perform(instance, attachment_name)
attachment = instance.send(attachment_name)
styles = attachment.options[:process_in_background]
attachment.generate_style_files(*styles)

if attachment.instance_respond_to?(:processing_in_background)
attachment.instance_write(:processing_in_background, false)
instance.save!
end
end
end
end
1 change: 1 addition & 0 deletions paperclip.gemspec
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ Gem::Specification.new do |s|
s.add_dependency('mimemagic', '~> 0.3.0')

s.add_development_dependency('activerecord', '>= 4.2.0')
s.add_development_dependency('activejob', '>= 4.2.0')
s.add_development_dependency('shoulda')
s.add_development_dependency('rspec', '~> 3.0')
s.add_development_dependency('appraisal')
Expand Down
63 changes: 62 additions & 1 deletion spec/paperclip/attachment_spec.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
# encoding: utf-8
require 'spec_helper'
require "spec_helper"
require "active_job"

describe Paperclip::Attachment do

Expand Down Expand Up @@ -1477,6 +1478,66 @@ def call(filename)
end
end

context "an attachment with process_in_background set" do
before do
ActiveJob::Base.queue_adapter = :test
ActiveJob::Base.logger = nil

rebuild_model styles: { thumb: "100x100" },
only_process: [:none],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a hack to skip processing in real time. We need to think about better UI, so user can decide which styles he wants to be processed in real time/background

process_in_background: [:thumb]
@file = File.new(fixture_file("5k.png"), "rb")
GlobalID.app = "paperclip-test"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I couldn't make it work without it. In normal Rails app, this is set up by railties

Dummy.send(:include, GlobalID::Identification)
@dummy = Dummy.new
end

after do
ActiveJob::Base.queue_adapter.enqueued_jobs.clear
end

it "enqueues job to process given styles in background" do
@dummy.avatar = @file
@dummy.save!
jobs = ActiveJob::Base.queue_adapter.enqueued_jobs

assert_equal 1, jobs.count

job = jobs[0]
assert_equal Paperclip::ProcessJob, job[:job]
assert_equal @dummy, GlobalID::Locator.locate(job[:args][0].values[0])
assert_equal "avatar", job[:args][1]
assert_equal "default", job[:queue]
end

it "does not process given styles in real time" do
attachment = @dummy.avatar
attachment.expects(:post_process).with(:none)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm little confused when to use rspec and when minitest - which one is preferred?

Copy link
Contributor

Choose a reason for hiding this comment

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

RSpec, see message of 1f3a746

attachment.expects(:post_process).with(:thumb).never
attachment.assign(@file)
end

it "does not enqueue job when no attachment assigned" do
@dummy.save!
jobs = ActiveJob::Base.queue_adapter.enqueued_jobs

assert_equal 0, jobs.count
end

it "populates avatar_processing_in_background column" do
ActiveRecord::Base.connection.add_column :dummies, :avatar_processing_in_background, :boolean

Choose a reason for hiding this comment

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

Line is too long. [99/80]

rebuild_class styles: { thumb: "100x100" },
only_process: [:none],
process_in_background: [:thumb]
GlobalID.app = "paperclip-test"
Dummy.send(:include, GlobalID::Identification)
@dummy = Dummy.new
@dummy.avatar = @file

assert_equal true, @dummy.avatar_processing_in_background
end
end

context "An attached file" do
before do
rebuild_model
Expand Down
42 changes: 42 additions & 0 deletions spec/paperclip/process_job_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
require "spec_helper"
require "paperclip/process_job"

RSpec.describe Paperclip::ProcessJob do
before do
ActiveJob::Base.queue_adapter = :test
ActiveJob::Base.logger = nil

@file = File.new(fixture_file("5k.png"), "rb")
@thumb_path = "tmp/public/system/dummies/avatars/000/000/001/thumb/5k.png"
File.delete(@thumb_path) if File.exist?(@thumb_path)

FileUtils.cp(@file, "tmp/public/system/dummies/avatars/000/000/001/original/5k.png")

Choose a reason for hiding this comment

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

Line is too long. [88/80]

rebuild_model styles: { thumb: "100x100" },
only_process: [:none],
process_in_background: [:thumb]
end

after do
ActiveJob::Base.queue_adapter.enqueued_jobs.clear
end

it "processes styles marked for background processing" do
dummy = Dummy.create!(avatar_file_name: "5k.png")

assert_file_not_exists(@thumb_path)
Paperclip::ProcessJob.perform_now(dummy, "avatar")
assert_file_exists(@thumb_path)
end

it "updates avatar_processing_in_background to false when finished" do
ActiveRecord::Base.connection.add_column :dummies, :avatar_processing_in_background, :boolean

Choose a reason for hiding this comment

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

Line is too long. [97/80]

rebuild_class styles: { thumb: "100x100" },
only_process: [:none],
process_in_background: [:thumb]

dummy = Dummy.create!(avatar_file_name: "5k.png", avatar_processing_in_background: true)

Choose a reason for hiding this comment

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

Line is too long. [92/80]

Paperclip::ProcessJob.perform_now(dummy, "avatar")

assert_equal false, dummy.reload.avatar_processing_in_background
end
end