Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

regard :from_version option on calling #recreate_versions! with args #1879

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
56 changes: 44 additions & 12 deletions lib/carrierwave/uploader/versions.rb
Expand Up @@ -213,23 +213,60 @@ def url(*args)
# Recreate versions and reprocess them. This can be used to recreate
# versions if their parameters somehow have changed.
#
def recreate_versions!(*versions)
def recreate_versions!(*names)
# Some files could possibly not be stored on the local disk. This
# doesn't play nicely with processing. Make sure that we're only
# processing a cached file
#
# The call to store! will trigger the necessary callbacks to both
# process this version and all sub-versions
if versions.any?
file = sanitized_file if !cached?
store_versions!(file, versions)

if names.any?
file = sanitized_file
set_versions_to_cache_and_store(names)
cache!(file)
store!(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.

Now we avoid duplication and possible errors by using similar approach to cache/store in both branches of a condition.

Copy link
Contributor

@rogerhu rogerhu Oct 16, 2020

Choose a reason for hiding this comment

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

We noticed this change causes an AWS S3-backed store to trigger callbacks and send DELETES. Is this expected behavior?

/Users/rogerh/.rvm/gems/ruby-2.6.6/gems/fog-xml-0.1.3/lib/fog/xml/connection.rb:9:in `request'
/Users/rogerh/.rvm/gems/ruby-2.6.6/gems/fog-aws-3.6.7/lib/fog/aws/storage.rb:628:in `_request'
/Users/rogerh/.rvm/gems/ruby-2.6.6/gems/fog-aws-3.6.7/lib/fog/aws/storage.rb:623:in `request'
/Users/rogerh/.rvm/gems/ruby-2.6.6/gems/fog-aws-3.6.7/lib/fog/aws/requests/storage/delete_object.rb:23:in `delete_object'
/Users/rogerh/.rvm/gems/ruby-2.6.6/gems/fog-aws-3.6.7/lib/fog/aws/models/storage/file.rb:118:in `destroy'
/Users/rogerh/.rvm/gems/ruby-2.6.6/gems/carrierwave-2.1.0/lib/carrierwave/storage/fog.rb:255:in `delete'
/Users/rogerh/.rvm/gems/ruby-2.6.6/gems/carrierwave-2.1.0/lib/carrierwave/uploader/store.rb:68:in `block in store!'
/Users/rogerh/.rvm/gems/ruby-2.6.6/gems/carrierwave-2.1.0/lib/carrierwave/uploader/callbacks.rb:15:in `with_callbacks'
/Users/rogerh/.rvm/gems/ruby-2.6.6/gems/carrierwave-2.1.0/lib/carrierwave/uploader/store.rb:65:in `store!'
/Users/rogerh/.rvm/gems/ruby-2.6.6/gems/carrierwave-2.1.0/lib/carrierwave/uploader/versions.rb:233:in `recreate_versions!'

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems related to #2385

Copy link
Contributor

Choose a reason for hiding this comment

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

Found the reason -- this change caused the cache storage to match the uploader:

629afec

reset_versions_to_cache_and_store
else
cache! if !cached?
store!
end
end

private

def set_versions_to_cache_and_store(names)
@versions_to_cache = source_versions_of(names)
Copy link
Member

Choose a reason for hiding this comment

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

Those instance variables being set here makes me think that the recreate_versions could be in its own class. As of now, I guess it's good to avoid adding more code here. Do you mind giving it a try @hedgesky ?

@versions_to_store = active_versions_with_names_in(@versions_to_cache + names)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Basically, it's still an instance variable. And now even two of them instead of one 😄 . But introducing them helps to avoid conditions in cache_versions! and store_versions! methods. Also, assigning these variables is hidden in methods with readable names. So, I think this change decreases overall complexity.

end

def reset_versions_to_cache_and_store
@versions_to_cache, @versions_to_store = nil, nil
end

def versions_to_cache
@versions_to_cache || dependent_versions
end

def versions_to_store
@versions_to_store || active_versions
end

def source_versions_of(requested_names)
versions.inject([]) do |sources, (name, uploader)|
next sources unless requested_names.include?(name)
next sources unless source_name = uploader.class.version_options[:from_version]

sources << [source_name, versions[source_name]]
end.uniq
end

def active_versions_with_names_in(names)
active_versions.select do |pretendent_name, uploader|
names.include?(pretendent_name)
end
end

def assign_parent_cache_id(file)
active_versions.each do |name, uploader|
uploader.parent_cache_id = @cache_id
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now versions to store is part of object's state, so we don't need extra argument here.

Expand Down Expand Up @@ -263,19 +300,14 @@ def full_original_filename
end

def cache_versions!(new_file)
dependent_versions.each do |name, v|
versions_to_cache.each do |name, v|
v.send(:cache_id=, @cache_id)
v.cache!(new_file)
end
end

def store_versions!(new_file, versions=nil)
if versions
active = Hash[active_versions]
versions.each { |v| active[v].try(:store!, new_file) } unless active.empty?
else
active_versions.each { |name, v| v.store!(new_file) }
end
def store_versions!(new_file)
versions_to_store.each { |name, v| v.store!(new_file) }
end

def remove_versions!
Expand Down
54 changes: 54 additions & 0 deletions spec/uploader/versions_spec.rb
Expand Up @@ -629,5 +629,59 @@ def upcase
expect(File.read(public_path(@uploader.thumb.to_s))).to eq(File.read(public_path(@uploader.small_thumb.to_s)))
end
end

describe "#recreate_versions!" do
let(:bork_file) { File.open(file_path('bork.txt')) }
let(:original_contents) { File.read(public_path(@uploader.to_s)) }
let(:thumb_contents) { File.read(public_path(@uploader.thumb.to_s)) }
let(:small_thumb_contents) { File.read(public_path(@uploader.small_thumb.to_s)) }

context "when no versions are given" do
it "should process file based on the version" do
@uploader.store!(bork_file)
@uploader.recreate_versions!
expect(thumb_contents).to eq(small_thumb_contents)
end
end

context "when version is given" do
it "should process file based on the version" do
@uploader.store!(bork_file)
FileUtils.rm([@uploader.small_thumb.path, @uploader.thumb.path])
@uploader.recreate_versions!(:small_thumb)
expect(File.exists?(public_path(@uploader.thumb.to_s))).to be true
expect(small_thumb_contents).to eq(thumb_contents)
expect(small_thumb_contents).not_to eq(original_contents)
end

it "reprocess parent version, too" do
@uploader.store!(bork_file)
FileUtils.rm(@uploader.thumb.path)
@uploader.recreate_versions!(:small_thumb)
end

it "works fine when recreating both dependent and parent versions" do
@uploader.store!(bork_file)
FileUtils.rm([@uploader.small_thumb.path, @uploader.thumb.path])
@uploader.recreate_versions!(:small_thumb, :thumb)
expect(File.exists?(public_path(@uploader.small_thumb.to_s))).to be true
expect(File.exists?(public_path(@uploader.thumb.to_s))).to be true

# doesn't depend on arguments order
FileUtils.rm([@uploader.small_thumb.path, @uploader.thumb.path])
@uploader.recreate_versions!(:thumb, :small_thumb)
expect(File.exists?(public_path(@uploader.small_thumb.to_s))).to be true
expect(File.exists?(public_path(@uploader.thumb.to_s))).to be true
end

it "doesn't touch other versions" do
@uploader_class.version(:another)
@uploader.store!(bork_file)
FileUtils.rm(@uploader.another.path)
@uploader.recreate_versions!(:small_thumb)
expect(File.exists?(public_path(@uploader.another.to_s))).to be false
end
end
end
end
end