Merge pull request #33666 from cbothner/fail-gracefully-from-activestorage-file-not-found

Fail more gracefully from ActiveStorage missing file exceptions

9 files changed, 118 insertions, 18 deletions

diff --git a/activestorage/CHANGELOG.md b/activestorage/CHANGELOG.md
index 8bfda4799e..b592f79ca6 100644
--- a/activestorage/CHANGELOG.md
+++ b/activestorage/CHANGELOG.md
@@ -1,3 +1,18 @@
+*   `ActiveStorage::DiskController#show` generates a 404 Not Found response when
+    the requested file is missing from the disk service. It previously raised
+    `Errno::ENOENT`.
+
+    *Cameron Bothner*
+
+*   `ActiveStorage::Blob#download` and `ActiveStorage::Blob#open` raise
+    `ActiveStorage::FileNotFoundError` when the corresponding file is missing
+    from the storage service. Services translate service-specific missing object
+    exceptions (e.g. `Google::Cloud::NotFoundError` for the GCS service and
+    `Errno::ENOENT` for the disk service) into
+    `ActiveStorage::FileNotFoundError`.
+
+    *Cameron Bothner*
+
 *   Added the `ActiveStorage::SetCurrent` concern for custom Active Storage
     controllers that can't inherit from `ActiveStorage::BaseController`.
 
diff --git a/activestorage/app/controllers/active_storage/disk_controller.rb b/activestorage/app/controllers/active_storage/disk_controller.rb
index 75cc11d6ff..7bd641ab9a 100644
--- a/activestorage/app/controllers/active_storage/disk_controller.rb
+++ b/activestorage/app/controllers/active_storage/disk_controller.rb
@@ -13,6 +13,8 @@ class ActiveStorage::DiskController < ActiveStorage::BaseController
     else
       head :not_found
     end
+  rescue Errno::ENOENT
+    head :not_found
   end
 
   def update
diff --git a/activestorage/lib/active_storage/errors.rb b/activestorage/lib/active_storage/errors.rb
index f4bf66a615..6475c1d076 100644
--- a/activestorage/lib/active_storage/errors.rb
+++ b/activestorage/lib/active_storage/errors.rb
@@ -19,4 +19,8 @@ module ActiveStorage
   # Raised when uploaded or downloaded data does not match a precomputed checksum.
   # Indicates that a network error or a software bug caused data corruption.
   class IntegrityError < Error; end
+
+  # Raised when ActiveStorage::Blob#download is called on a blob where the
+  # backing file is no longer present in its service.
+  class FileNotFoundError < Error; end
 end
diff --git a/activestorage/lib/active_storage/service/azure_storage_service.rb b/activestorage/lib/active_storage/service/azure_storage_service.rb
index b26234c722..66aabc1f9f 100644
--- a/activestorage/lib/active_storage/service/azure_storage_service.rb
+++ b/activestorage/lib/active_storage/service/azure_storage_service.rb
@@ -34,16 +34,20 @@ module ActiveStorage
         end
       else
         instrument :download, key: key do
-          _, io = blobs.get_blob(container, key)
-          io.force_encoding(Encoding::BINARY)
+          handle_errors do
+            _, io = blobs.get_blob(container, key)
+            io.force_encoding(Encoding::BINARY)
+          end
         end
       end
     end
 
     def download_chunk(key, range)
       instrument :download_chunk, key: key, range: range do
-        _, io = blobs.get_blob(container, key, start_range: range.begin, end_range: range.exclude_end? ? range.end - 1 : range.end)
-        io.force_encoding(Encoding::BINARY)
+        handle_errors do
+          _, io = blobs.get_blob(container, key, start_range: range.begin, end_range: range.exclude_end? ? range.end - 1 : range.end)
+          io.force_encoding(Encoding::BINARY)
+        end
       end
     end
 
@@ -139,11 +143,23 @@ module ActiveStorage
         chunk_size = 5.megabytes
         offset = 0
 
+        raise ActiveStorage::FileNotFoundError unless blob.present?
+
         while offset < blob.properties[:content_length]
           _, chunk = blobs.get_blob(container, key, start_range: offset, end_range: offset + chunk_size - 1)
           yield chunk.force_encoding(Encoding::BINARY)
           offset += chunk_size
         end
       end
+
+      def handle_errors
+        yield
+      rescue Azure::Core::Http::HTTPError => e
+        if e.type == "BlobNotFound"
+          raise ActiveStorage::FileNotFoundError
+        else
+          raise
+        end
+      end
   end
 end
diff --git a/activestorage/lib/active_storage/service/disk_service.rb b/activestorage/lib/active_storage/service/disk_service.rb
index 9f304b7e01..52f3a3df16 100644
--- a/activestorage/lib/active_storage/service/disk_service.rb
+++ b/activestorage/lib/active_storage/service/disk_service.rb
@@ -22,27 +22,31 @@ module ActiveStorage
       end
     end
 
-    def download(key)
+    def download(key, &block)
       if block_given?
         instrument :streaming_download, key: key do
-          File.open(path_for(key), "rb") do |file|
-            while data = file.read(5.megabytes)
-              yield data
-            end
-          end
+          stream key, &block
         end
       else
         instrument :download, key: key do
-          File.binread path_for(key)
+          begin
+            File.binread path_for(key)
+          rescue Errno::ENOENT
+            raise ActiveStorage::FileNotFoundError
+          end
         end
       end
     end
 
     def download_chunk(key, range)
       instrument :download_chunk, key: key, range: range do
-        File.open(path_for(key), "rb") do |file|
-          file.seek range.begin
-          file.read range.size
+        begin
+          File.open(path_for(key), "rb") do |file|
+            file.seek range.begin
+            file.read range.size
+          end
+        rescue Errno::ENOENT
+          raise ActiveStorage::FileNotFoundError
         end
       end
     end
@@ -122,6 +126,16 @@ module ActiveStorage
     end
 
     private
+      def stream(key)
+        File.open(path_for(key), "rb") do |file|
+          while data = file.read(5.megabytes)
+            yield data
+          end
+        end
+      rescue Errno::ENOENT
+        raise ActiveStorage::FileNotFoundError
+      end
+
       def folder_for(key)
         [ key[0..1], key[2..3] ].join("/")
       end
diff --git a/activestorage/lib/active_storage/service/gcs_service.rb b/activestorage/lib/active_storage/service/gcs_service.rb
index eb46973509..18c0f14cfc 100644
--- a/activestorage/lib/active_storage/service/gcs_service.rb
+++ b/activestorage/lib/active_storage/service/gcs_service.rb
@@ -34,14 +34,22 @@ module ActiveStorage
         end
       else
         instrument :download, key: key do
-          file_for(key).download.string
+          begin
+            file_for(key).download.string
+          rescue Google::Cloud::NotFoundError
+            raise ActiveStorage::FileNotFoundError
+          end
         end
       end
     end
 
     def download_chunk(key, range)
       instrument :download_chunk, key: key, range: range do
-        file_for(key).download(range: range).string
+        begin
+          file_for(key).download(range: range).string
+        rescue Google::Cloud::NotFoundError
+          raise ActiveStorage::FileNotFoundError
+        end
       end
     end
 
@@ -116,6 +124,8 @@ module ActiveStorage
         chunk_size = 5.megabytes
         offset = 0
 
+        raise ActiveStorage::FileNotFoundError unless file.present?
+
         while offset < file.size
           yield file.download(range: offset..(offset + chunk_size - 1)).string
           offset += chunk_size
diff --git a/activestorage/lib/active_storage/service/s3_service.rb b/activestorage/lib/active_storage/service/s3_service.rb
index 0286e7ff21..89a9e54158 100644
--- a/activestorage/lib/active_storage/service/s3_service.rb
+++ b/activestorage/lib/active_storage/service/s3_service.rb
@@ -33,14 +33,22 @@ module ActiveStorage
         end
       else
         instrument :download, key: key do
-          object_for(key).get.body.string.force_encoding(Encoding::BINARY)
+          begin
+            object_for(key).get.body.string.force_encoding(Encoding::BINARY)
+          rescue Aws::S3::Errors::NoSuchKey
+            raise ActiveStorage::FileNotFoundError
+          end
         end
       end
     end
 
     def download_chunk(key, range)
       instrument :download_chunk, key: key, range: range do
-        object_for(key).get(range: "bytes=#{range.begin}-#{range.exclude_end? ? range.end - 1 : range.end}").body.read.force_encoding(Encoding::BINARY)
+        begin
+          object_for(key).get(range: "bytes=#{range.begin}-#{range.exclude_end? ? range.end - 1 : range.end}").body.read.force_encoding(Encoding::BINARY)
+        rescue Aws::S3::Errors::NoSuchKey
+          raise ActiveStorage::FileNotFoundError
+        end
       end
     end
 
@@ -103,6 +111,8 @@ module ActiveStorage
         chunk_size = 5.megabytes
         offset = 0
 
+        raise ActiveStorage::FileNotFoundError unless object.exists?
+
         while offset < object.content_length
           yield object.get(range: "bytes=#{offset}-#{offset + chunk_size - 1}").body.read.force_encoding(Encoding::BINARY)
           offset += chunk_size
diff --git a/activestorage/test/controllers/disk_controller_test.rb b/activestorage/test/controllers/disk_controller_test.rb
index c053052f6f..4bc61d13f3 100644
--- a/activestorage/test/controllers/disk_controller_test.rb
+++ b/activestorage/test/controllers/disk_controller_test.rb
@@ -31,6 +31,14 @@ class ActiveStorage::DiskControllerTest < ActionDispatch::IntegrationTest
     assert_equal " worl", response.body
   end
 
+  test "showing blob that does not exist" do
+    blob = create_blob
+    blob.delete
+
+    get blob.service_url
+    assert_response :not_found
+  end
+
 
   test "directly uploading blob with integrity" do
     data = "Something else entirely!"
diff --git a/activestorage/test/service/shared_service_tests.rb b/activestorage/test/service/shared_service_tests.rb
index 30cfca4e36..58f189af2b 100644
--- a/activestorage/test/service/shared_service_tests.rb
+++ b/activestorage/test/service/shared_service_tests.rb
@@ -50,6 +50,13 @@ module ActiveStorage::Service::SharedServiceTests
       assert_equal FIXTURE_DATA, @service.download(@key)
     end
 
+    test "downloading a nonexistent file" do
+      assert_raises(ActiveStorage::FileNotFoundError) do
+        @service.download(SecureRandom.base58(24))
+      end
+    end
+
+
     test "downloading in chunks" do
       key = SecureRandom.base58(24)
       expected_chunks = [ "a" * 5.megabytes, "b" ]
@@ -68,11 +75,25 @@ module ActiveStorage::Service::SharedServiceTests
       end
     end
 
+    test "downloading a nonexistent file in chunks" do
+      assert_raises(ActiveStorage::FileNotFoundError) do
+        @service.download(SecureRandom.base58(24)) {}
+      end
+    end
+
+
     test "downloading partially" do
       assert_equal "\x10\x00\x00", @service.download_chunk(@key, 19..21)
       assert_equal "\x10\x00\x00", @service.download_chunk(@key, 19...22)
     end
 
+    test "partially downloading a nonexistent file" do
+      assert_raises(ActiveStorage::FileNotFoundError) do
+        @service.download_chunk(SecureRandom.base58(24), 19..21)
+      end
+    end
+
+
     test "existing" do
       assert @service.exist?(@key)
       assert_not @service.exist?(@key + "nonsense")