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

@@ -55,13 +55,8 @@ class OptimizedImage < ActiveRecord::Base
 
     if original_path.blank?
       # download is protected with a DistributedMutex
-      external_copy =
+      external_copy = Discourse.store.download_safe(upload)
-        begin
+      original_path = external_copy&.path
-          Discourse.store.download(upload)
-        rescue StandardError
-          nil
-        end
-      original_path = external_copy.try(:path)
     end
 
     lock(upload.id, width, height) do

app/models/theme_field.rb

@@ -189,13 +189,8 @@ class ThemeField < ActiveRecord::Base
       end
 
     if Discourse.store.external?
-      external_copy =
+      external_copy = Discourse.store.download_safe(upload)
-        begin
+      path = external_copy&.path
-          Discourse.store.download(upload)
-        rescue StandardError
-          nil
-        end
-      path = external_copy.try(:path)
     else
       path = Discourse.store.path_for(upload)
     end

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 =
+        external_copy = Discourse.store.download_safe(self)
-          begin
+        original_path = external_copy&.path
-            Discourse.store.download(self)
-          rescue StandardError
-            nil
-          end
-        original_path = external_copy.try(:path)
       end
 
       image_info =
@@ -361,13 +356,7 @@ class Upload < ActiveRecord::Base
         if local?
           Discourse.store.path_for(self)
         else
-          begin
+          Discourse.store.download_safe(self)&.path
-            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
         end
 
       if local_path.nil?

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(

lib/tasks/uploads.rake

@@ -1200,13 +1200,7 @@ task "uploads:downsize" => :environment do
       if upload.local?
         Discourse.store.path_for(upload)
       else
-        (
+        Discourse.store.download_safe(upload, max_file_size_kb: 100.megabytes)&.path
-          begin
-            Discourse.store.download(upload, max_file_size_kb: 100.megabytes)
-          rescue StandardError
-            nil
-          end
-        )&.path
       end
 
     unless path

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