DEV: Add both safe and unsafe Discourse.store.download methods (stable) (#21499)

### Background

Several call sites use `FileStore#download` (through `Discourse.store.download`). In some cases the author seems aware that the method can raise an error if the download fails, and in some cases not. Because of this we're seeing some of these exceptions bubble all the way up and getting logged in production. Although they are not really actionable at that point. Rather each call site needs to be considered to figure out how to handle them.

### What is this change?

This change accomplishes primarily two things.

Firstly it separates the method into a safe version which will handle errors by returning `nil`, and an unsafe version which will re-package upstream errors in a new `FileStore::DownloadError` class.

Secondly it updates the call sites which have been doing error handling downstream to use the new safe version.

For backwards compatibility, there's an interim situation and a desired end state.

**Interim:**

```
FileStore#download      → Old unsafe version. Will raise any error and show a deprecation warning.
FileStore#download!     → New unsafe version. Will raise FileStore::DownloadError.
FileStore#download_safe → New safe version.   Will return nil.
```

**Desired end-state:**

```
FileStore#download  → New safe version.   Will return nil.
FileStore#download! → New unsafe version. Will raise FileStore::DownloadError.
```

### What's next?

We need to do a quick audit of the call sites that are using the old unsafe version without any error handling, as well as check for call sites in plugins other repos. Follow-up PRs incoming.

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

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

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?

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
-        (
-          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

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

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