diff --git a/app/models/optimized_image.rb b/app/models/optimized_image.rb index dbfa6d10c66..e5a8b4fdb3f 100644 --- a/app/models/optimized_image.rb +++ b/app/models/optimized_image.rb @@ -55,13 +55,8 @@ class OptimizedImage < ActiveRecord::Base if original_path.blank? # download is protected with a DistributedMutex - external_copy = - begin - Discourse.store.download(upload) - rescue StandardError - nil - end - original_path = external_copy.try(:path) + external_copy = Discourse.store.download_safe(upload) + original_path = external_copy&.path end lock(upload.id, width, height) do diff --git a/app/models/theme_field.rb b/app/models/theme_field.rb index 780789c42bf..3489f8e729b 100644 --- a/app/models/theme_field.rb +++ b/app/models/theme_field.rb @@ -184,13 +184,8 @@ class ThemeField < ActiveRecord::Base end if Discourse.store.external? - external_copy = - begin - Discourse.store.download(upload) - rescue StandardError - nil - end - path = external_copy.try(:path) + external_copy = Discourse.store.download_safe(upload) + path = external_copy&.path else path = Discourse.store.path_for(upload) end diff --git a/app/models/upload.rb b/app/models/upload.rb index 964ef52d1dd..f4bbfbac7ae 100644 --- a/app/models/upload.rb +++ b/app/models/upload.rb @@ -156,13 +156,8 @@ class Upload < ActiveRecord::Base # this is relatively cheap once cached original_path = Discourse.store.path_for(self) if original_path.blank? - external_copy = - begin - Discourse.store.download(self) - rescue StandardError - nil - end - original_path = external_copy.try(:path) + external_copy = Discourse.store.download_safe(self) + original_path = external_copy&.path end image_info = @@ -357,13 +352,7 @@ class Upload < ActiveRecord::Base if local? Discourse.store.path_for(self) else - begin - Discourse.store.download(self)&.path - rescue OpenURI::HTTPError => e - # Some issue with downloading the image from a remote store. - # Assume the upload is broken and save an empty string to prevent re-evaluation - nil - end + Discourse.store.download_safe(self)&.path end if local_path.nil? diff --git a/lib/file_store/base_store.rb b/lib/file_store/base_store.rb index f73114ca897..9645513ab1f 100644 --- a/lib/file_store/base_store.rb +++ b/lib/file_store/base_store.rb @@ -1,6 +1,9 @@ # frozen_string_literal: true module FileStore + class DownloadError < StandardError + end + class BaseStore UPLOAD_PATH_REGEX ||= %r{/(original/\d+X/.*)} OPTIMIZED_IMAGE_PATH_REGEX ||= %r{/(optimized/\d+X/.*)} @@ -88,7 +91,26 @@ module FileStore not_implemented end - def download(object, max_file_size_kb: nil) + # TODO: Remove when #download becomes the canonical safe version. + def download_safe(*, **) + download(*, **, print_deprecation: false) + rescue StandardError + nil + end + + def download!(*, **) + download(*, **, print_deprecation: false) + rescue StandardError + raise DownloadError + end + + def download(object, max_file_size_kb: nil, print_deprecation: true) + Discourse.deprecate(<<~MESSAGE, output_in_test: true) if print_deprecation + In a future version `FileStore#download` will no longer raise an error when the + download fails, and will instead return `nil`. If you need a method that raises + an error, use `FileStore#download!`, which raises a `FileStore::DownloadError`. + MESSAGE + DistributedMutex.synchronize("download_#{object.sha1}", validity: 3.minutes) do extension = File.extname( diff --git a/lib/tasks/uploads.rake b/lib/tasks/uploads.rake index 6314efff696..4c0dc03de3b 100644 --- a/lib/tasks/uploads.rake +++ b/lib/tasks/uploads.rake @@ -1200,13 +1200,7 @@ task "uploads:downsize" => :environment do if upload.local? Discourse.store.path_for(upload) else - ( - begin - Discourse.store.download(upload, max_file_size_kb: 100.megabytes) - rescue StandardError - nil - end - )&.path + Discourse.store.download_safe(upload, max_file_size_kb: 100.megabytes)&.path end unless path diff --git a/spec/lib/file_store/base_store_spec.rb b/spec/lib/file_store/base_store_spec.rb index 0214ebdbc63..78fe40f8c0d 100644 --- a/spec/lib/file_store/base_store_spec.rb +++ b/spec/lib/file_store/base_store_spec.rb @@ -189,16 +189,16 @@ RSpec.describe FileStore::BaseStore do # Net::HTTP always returns binary ASCII-8BIT encoding. File.read auto-detects the encoding # Make sure we File.read after downloading a file for consistency - first_encoding = store.download(upload_s3).read.encoding + first_encoding = store.download(upload_s3, print_deprecation: false).read.encoding - second_encoding = store.download(upload_s3).read.encoding + second_encoding = store.download(upload_s3, print_deprecation: false).read.encoding expect(first_encoding).to eq(Encoding::UTF_8) expect(second_encoding).to eq(Encoding::UTF_8) end it "should return the file" do - file = store.download(upload_s3) + file = store.download(upload_s3, print_deprecation: false) expect(file.class).to eq(File) end @@ -210,7 +210,7 @@ RSpec.describe FileStore::BaseStore do body: "Hello world", ) - file = store.download(upload_s3) + file = store.download(upload_s3, print_deprecation: true) expect(file.class).to eq(File) end @@ -223,9 +223,41 @@ RSpec.describe FileStore::BaseStore do signed_url = Discourse.store.signed_url_for_path(upload_s3.url) stub_request(:get, signed_url).to_return(status: 200, body: "Hello world") - file = store.download(upload_s3) + file = store.download(upload_s3, print_deprecation: false) expect(file.class).to eq(File) end end + + describe "#download!" do + before do + setup_s3 + stub_request(:get, upload_s3.url).to_return(status: 200, body: "Hello world") + end + + let(:upload_s3) { Fabricate(:upload_s3) } + let(:store) { FileStore::BaseStore.new } + + it "does not raise an error when download fails" do + FileHelper.stubs(:download).raises(OpenURI::HTTPError.new("400 error", anything)) + + expect { store.download!(upload_s3) }.to raise_error(FileStore::DownloadError) + end + end + + describe "#download_safe" do + before do + setup_s3 + stub_request(:get, upload_s3.url).to_return(status: 200, body: "Hello world") + end + + let(:upload_s3) { Fabricate(:upload_s3) } + let(:store) { FileStore::BaseStore.new } + + it "does not raise an error when download fails" do + FileHelper.stubs(:download).raises(OpenURI::HTTPError.new("400 error", anything)) + + expect(store.download_safe(upload_s3)).to eq(nil) + end + end end diff --git a/spec/lib/stylesheet/manager_spec.rb b/spec/lib/stylesheet/manager_spec.rb index dc74ab3a098..07fe97df699 100644 --- a/spec/lib/stylesheet/manager_spec.rb +++ b/spec/lib/stylesheet/manager_spec.rb @@ -878,20 +878,10 @@ RSpec.describe Stylesheet::Manager do end describe ".precompile css" do - before do - class << STDERR - alias_method :orig_write, :write - def write(x) - end - end - end + before { STDERR.stubs(:write) } after do - class << STDERR - def write(x) - orig_write(x) - end - end + STDERR.unstub(:write) FileUtils.rm_rf("tmp/stylesheet-cache") end