diff --git a/lib/carrierwave/mount.rb b/lib/carrierwave/mount.rb index 718ca3adb..7c47ad3a5 100644 --- a/lib/carrierwave/mount.rb +++ b/lib/carrierwave/mount.rb @@ -185,6 +185,18 @@ def #{column}_identifier _mounter(:#{column}).read_identifiers[0] end + def #{column}_integrity_error + #{column}_integrity_errors.last + end + + def #{column}_processing_error + #{column}_processing_errors.last + end + + def #{column}_download_error + #{column}_download_errors.last + end + def store_previous_changes_for_#{column} attribute_changes = ::ActiveRecord.version.to_s.to_f >= 5.1 ? saved_changes : changes @_previous_changes_for_#{column} = attribute_changes[_mounter(:#{column}).serialization_column] @@ -240,9 +252,9 @@ def remove_previously_stored_#{column} # [store_images!] Stores all files that have been assigned with +images=+ # [remove_images!] Removes the uploaded file from the filesystem. # - # [images_integrity_error] Returns an error object if the last files to be assigned caused an integrity error - # [images_processing_error] Returns an error object if the last files to be assigned caused a processing error - # [images_download_error] Returns an error object if the last files to be remotely assigned caused a download error + # [image_integrity_errors] Returns error objects of files which failed to pass integrity check + # [image_processing_errors] Returns error objects of files which failed to be processed + # [image_download_errors] Returns error objects of files which failed to be downloaded # # [image_identifiers] Reads out the identifiers of the files # @@ -395,16 +407,16 @@ def store_#{column}! _mounter(:#{column}).store! end - def #{column}_integrity_error - _mounter(:#{column}).integrity_error + def #{column}_integrity_errors + _mounter(:#{column}).integrity_errors end - def #{column}_processing_error - _mounter(:#{column}).processing_error + def #{column}_processing_errors + _mounter(:#{column}).processing_errors end - def #{column}_download_error - _mounter(:#{column}).download_error + def #{column}_download_errors + _mounter(:#{column}).download_errors end def mark_remove_#{column}_false diff --git a/lib/carrierwave/mounter.rb b/lib/carrierwave/mounter.rb index 95ed6148a..6b18fdccf 100644 --- a/lib/carrierwave/mounter.rb +++ b/lib/carrierwave/mounter.rb @@ -3,14 +3,17 @@ module CarrierWave # this is an internal class, used by CarrierWave::Mount so that # we don't pollute the model with a lot of methods. class Mounter #:nodoc: - attr_reader :column, :record, :remote_urls, :integrity_error, - :processing_error, :download_error + attr_reader :column, :record, :remote_urls, :integrity_errors, + :processing_errors, :download_errors attr_accessor :remove, :remote_request_headers def initialize(record, column, options={}) @record = record @column = column @options = record.class.uploader_options[column] + @download_errors = [] + @processing_errors = [] + @integrity_errors = [] end def uploader_class @@ -41,25 +44,18 @@ def cache(new_files) return if new_files.blank? old_uploaders = uploaders @uploaders = new_files.map do |new_file| - if new_file.is_a?(String) - uploader = old_uploaders.detect { |uploader| uploader.identifier == new_file } - uploader.staged = true if uploader - uploader - else - uploader = blank_uploader - uploader.cache!(new_file) - uploader + handle_error do + if new_file.is_a?(String) + uploader = old_uploaders.detect { |uploader| uploader.identifier == new_file } + uploader.staged = true if uploader + uploader + else + uploader = blank_uploader + uploader.cache!(new_file) + uploader + end end end.compact - - @integrity_error = nil - @processing_error = nil - rescue CarrierWave::IntegrityError => e - @integrity_error = e - raise e unless option(:ignore_integrity_errors) - rescue CarrierWave::ProcessingError => e - @processing_error = e - raise e unless option(:ignore_processing_errors) end def cache_names @@ -70,36 +66,29 @@ def cache_names=(cache_names) return if cache_names.blank? clear_unstaged cache_names.map do |cache_name| - uploader = blank_uploader - uploader.retrieve_from_cache!(cache_name) - @uploaders << uploader + begin + uploader = blank_uploader + uploader.retrieve_from_cache!(cache_name) + @uploaders << uploader + rescue CarrierWave::InvalidParameter + # ignore + end end - rescue CarrierWave::InvalidParameter end def remote_urls=(urls) return if urls.blank? || urls.all?(&:blank?) @remote_urls = urls - @download_error = nil - @integrity_error = nil clear_unstaged urls.zip(remote_request_headers || []).map do |url, header| - uploader = blank_uploader - uploader.download!(url, header || {}) - @uploaders << uploader + handle_error do + uploader = blank_uploader + uploader.download!(url, header || {}) + @uploaders << uploader + end end - - rescue CarrierWave::DownloadError => e - @download_error = e - raise e unless option(:ignore_download_errors) - rescue CarrierWave::ProcessingError => e - @processing_error = e - raise e unless option(:ignore_processing_errors) - rescue CarrierWave::IntegrityError => e - @integrity_error = e - raise e unless option(:ignore_integrity_errors) end def store! @@ -172,5 +161,18 @@ def clear_unstaged @uploaders ||= [] @uploaders.keep_if(&:staged) end + + def handle_error + yield + rescue CarrierWave::DownloadError => e + @download_errors << e + raise e unless option(:ignore_download_errors) + rescue CarrierWave::ProcessingError => e + @processing_errors << e + raise e unless option(:ignore_processing_errors) + rescue CarrierWave::IntegrityError => e + @integrity_errors << e + raise e unless option(:ignore_integrity_errors) + end end # Mounter end # CarrierWave diff --git a/lib/carrierwave/validations/active_model.rb b/lib/carrierwave/validations/active_model.rb index 27dad42aa..6be0a3e70 100644 --- a/lib/carrierwave/validations/active_model.rb +++ b/lib/carrierwave/validations/active_model.rb @@ -11,7 +11,7 @@ module ActiveModel class ProcessingValidator < ::ActiveModel::EachValidator def validate_each(record, attribute, value) - if e = record.__send__("#{attribute}_processing_error") + record.__send__("#{attribute}_processing_errors").each do |e| message = (e.message == e.class.to_s) ? :carrierwave_processing_error : e.message record.errors.add(attribute, message) end @@ -21,7 +21,7 @@ def validate_each(record, attribute, value) class IntegrityValidator < ::ActiveModel::EachValidator def validate_each(record, attribute, value) - if e = record.__send__("#{attribute}_integrity_error") + record.__send__("#{attribute}_integrity_errors").each do |e| message = (e.message == e.class.to_s) ? :carrierwave_integrity_error : e.message record.errors.add(attribute, message) end @@ -31,7 +31,7 @@ def validate_each(record, attribute, value) class DownloadValidator < ::ActiveModel::EachValidator def validate_each(record, attribute, value) - if e = record.__send__("#{attribute}_download_error") + record.__send__("#{attribute}_download_errors").each do |e| message = (e.message == e.class.to_s) ? :carrierwave_download_error : e.message record.errors.add(attribute, message) end diff --git a/spec/mount_multiple_spec.rb b/spec/mount_multiple_spec.rb index bfbca2d4f..eea5435fd 100644 --- a/spec/mount_multiple_spec.rb +++ b/spec/mount_multiple_spec.rb @@ -199,47 +199,62 @@ def rotate instance.images = [test_file_stub] end - context "fails silently if the images fails a white list integrity check" do + context "if the images fails a white list integrity check" do before do uploader.class_eval do def extension_whitelist %w(txt) end end + end - instance.images = [text_file_stub, test_file_stub] + it "fails silently" do + expect { instance.images = [test_file_stub] }.not_to raise_error end - it { expect(instance.images).to be_empty } + it "keeps files which passed the check" do + instance.images = [test_file_stub, text_file_stub] + expect(instance.images.map(&:identifier)).to eq ['bork.txt'] + end end - describe "fails silently if the images fails a black list integrity check" do + describe "if the images fails a black list integrity check" do before do uploader.class_eval do def extension_blacklist %w(jpg) end end + end - instance.images = [text_file_stub, test_file_stub] + it "fails silently" do + expect { instance.images = [test_file_stub] }.not_to raise_error end - it { expect(instance.images).to be_empty } + it "keeps files which passed the check" do + instance.images = [test_file_stub, text_file_stub] + expect(instance.images.map(&:identifier)).to eq ['bork.txt'] + end end - describe "fails silently if the images fails to be processed" do + describe "if the images fails to be processed" do before do uploader.class_eval do process :monkey def monkey - raise CarrierWave::ProcessingError, "Ohh noez!" + raise CarrierWave::ProcessingError, "Ohh noez!" if file.path =~ /test\.jpg/ end end + end - instance.images = [test_file_stub] + it "fails silently" do + expect { instance.images = [test_file_stub] }.not_to raise_error end - it { expect(instance.images).to be_empty } + it "keeps files which was processed successfully" do + instance.images = [test_file_stub, text_file_stub] + expect(instance.images.map(&:identifier)).to eq ['bork.txt'] + end end describe "with stored files" do @@ -441,6 +456,14 @@ def monkey it { expect(instance.images[0].current_path).to match(/test.jpg$/) } end + + context "when valid and invalid cache names are assigned" do + before { instance.images_cache = ['1369894322-123-0123-1234/test.jpg', 'invalid'].to_json } + + it "retrieves valid file only from cache" do + expect(instance.images.map(&:cache_name)).to eq(['1369894322-123-0123-1234/test.jpg']) + end + end end describe "#remote_images_urls" do @@ -464,6 +487,7 @@ def monkey before do stub_request(:get, "www.example.com/#{test_file_name}").to_return(body: File.read(test_file_stub)) + stub_request(:get, "http://www.example.com/test.txt").to_return(status: 404) instance.remote_images_urls = remote_images_url end @@ -517,6 +541,14 @@ def monkey it { is_expected.to match(/portrait.jpg$/) } end + + context "if a file fails to be downloaded" do + let(:remote_images_url) { ["http://www.example.com/test.txt", "http://www.example.com/test.jpg"] } + + it "keeps files which was downloaded successfully" do + expect(instance.images.map(&:identifier)).to eq ['test.jpg'] + end + end end describe '#store_images!' do @@ -631,17 +663,17 @@ def monkey end end - describe '#images_integrity_error' do - subject(:images_integrity_error) { instance.images_integrity_error } + describe '#images_integrity_errors' do + subject(:images_integrity_errors) { instance.images_integrity_errors } describe "default behaviour" do - it { is_expected.to be_nil } + it { is_expected.to be_empty } end context "when a file is cached" do before { instance.images = test_file_stub } - it { is_expected.to be_nil } + it { is_expected.to be_empty } end describe "when an integrity check fails" do @@ -656,10 +688,10 @@ def extension_whitelist context "when file is cached" do before { instance.images = [test_file_stub] } - it { is_expected.to be_an_instance_of(CarrierWave::IntegrityError) } + it { is_expected.to include(a_kind_of(CarrierWave::IntegrityError)) } it "has an error message" do - expect(images_integrity_error.message.lines.grep(/^You are not allowed to upload/)).to be_truthy + expect(images_integrity_errors[0].message.lines.grep(/^You are not allowed to upload/)).to be_truthy end end @@ -669,10 +701,10 @@ def extension_whitelist instance.remote_images_urls = ["http://www.example.com/#{test_file_name}"] end - it { is_expected.to be_an_instance_of(CarrierWave::IntegrityError) } + it { is_expected.to include(a_kind_of(CarrierWave::IntegrityError)) } it "has an error message" do - expect(images_integrity_error.message.lines.grep(/^You are not allowed to upload/)).to be_truthy + expect(images_integrity_errors[0].message.lines.grep(/^You are not allowed to upload/)).to be_truthy end end @@ -682,26 +714,26 @@ def extension_whitelist instance.remote_images_urls = "" end - it { is_expected.to be_an_instance_of(CarrierWave::IntegrityError) } + it { is_expected.to include(a_kind_of(CarrierWave::IntegrityError)) } it "has an error message" do - expect(images_integrity_error.message.lines.grep(/^You are not allowed to upload/)).to be_truthy + expect(images_integrity_errors[0].message.lines.grep(/^You are not allowed to upload/)).to be_truthy end end end end - describe '#images_processing_error' do - subject(:images_processing_error) { instance.images_processing_error } + describe '#images_processing_errors' do + subject(:images_processing_errors) { instance.images_processing_errors } describe "default behavior" do - it { is_expected.to be_nil } + it { is_expected.to be_empty } end context "when file is cached" do before { instance.images = [test_file_stub] } - it { is_expected.to be_nil } + it { is_expected.to be_empty } end describe "when an processing error occurs" do @@ -717,7 +749,7 @@ def monkey context "when file is cached" do before { instance.images = [test_file_stub] } - it { is_expected.to be_an_instance_of(CarrierWave::ProcessingError) } + it { is_expected.to include(a_kind_of(CarrierWave::ProcessingError)) } end context "when file was downloaded" do @@ -726,13 +758,13 @@ def monkey instance.remote_images_urls = ["http://www.example.com/#{test_file_name}"] end - it { is_expected.to be_an_instance_of(CarrierWave::ProcessingError) } + it { is_expected.to include(a_kind_of(CarrierWave::ProcessingError)) } end end end - describe '#images_download_error' do - subject(:images_download_error) { instance.images_download_error } + describe '#images_download_errors' do + subject(:images_download_errors) { instance.images_download_errors } before do stub_request(:get, "www.example.com/#{test_file_name}").to_return(body: File.read(test_file_stub)) @@ -740,19 +772,19 @@ def monkey end describe "default behaviour" do - it { expect(instance.images_download_error).to be_nil } + it { expect(instance.images_download_errors).to be_empty } end context "when file download was successful" do before { instance.remote_images_urls = ["http://www.example.com/#{test_file_name}"] } - it { is_expected.to be_nil } + it { is_expected.to be_empty } end context "when file couldn't be found" do before { instance.remote_images_urls = ["http://www.example.com/missing.jpg"] } - it { is_expected.to be_an_instance_of(CarrierWave::DownloadError) } + it { is_expected.to include(a_kind_of(CarrierWave::DownloadError)) } end end