Translate service-specific missing object exceptions into a generic one

`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`.

activestorage/CHANGELOG.md

@@ -1,3 +1,12 @@
+*   `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`.
 

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

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

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

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

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

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")