From b837459e1dc38a8598b20b7108cfc359ff863876 Mon Sep 17 00:00:00 2001 From: Ted Johansson Date: Thu, 11 May 2023 17:27:27 +0800 Subject: [PATCH] DEV: Add both safe and unsafe Discourse.store.download methods (#21498) * DEV: Add both safe and unsafe Discourse.store.download methods * DEV: Update call sites that can use the safe store download method --- app/models/optimized_image.rb | 9 ++---- app/models/theme_field.rb | 9 ++---- app/models/upload.rb | 17 ++--------- lib/file_store/base_store.rb | 24 ++++++++++++++- lib/tasks/uploads.rake | 8 +---- spec/lib/file_store/base_store_spec.rb | 42 +++++++++++++++++++++++--- 6 files changed, 68 insertions(+), 41 deletions(-) diff --git a/app/models/optimized_image.rb b/app/models/optimized_image.rb index 5b0b7312c53..ff3b5a6c089 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 415f1d5c97c..a9b22b2fb16 100644 --- a/app/models/theme_field.rb +++ b/app/models/theme_field.rb @@ -189,13 +189,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 8f069ec65d6..a3b62cedb3e 100644 --- a/app/models/upload.rb +++ b/app/models/upload.rb @@ -160,13 +160,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 = @@ -361,13 +356,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 f1a958fb8a5..e896bf5d8ae 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 1104a9f3a77..f9d9da7ebcd 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