Support streaming downloads from Google Cloud Storage

3 files changed, 43 insertions, 24 deletions

diff --git a/activestorage/CHANGELOG.md b/activestorage/CHANGELOG.md
index 60d7d19540..4c12adae56 100644
--- a/activestorage/CHANGELOG.md
+++ b/activestorage/CHANGELOG.md
@@ -1,3 +1,8 @@
+*   The Google Cloud Storage service properly supports streaming downloads.
+    It now requires version 1.11 or newer of the google-cloud-storage gem.
+
+    *George Claghorn*
+
 *   Use the [ImageProcessing](https://github.com/janko-m/image_processing) gem
     for Active Storage variants, and deprecate the MiniMagick backend.
 
diff --git a/activestorage/lib/active_storage/service/gcs_service.rb b/activestorage/lib/active_storage/service/gcs_service.rb
index 7a1839a1aa..38acef81f4 100644
--- a/activestorage/lib/active_storage/service/gcs_service.rb
+++ b/activestorage/lib/active_storage/service/gcs_service.rb
@@ -1,6 +1,6 @@
 # frozen_string_literal: true
 
-gem "google-cloud-storage", "~> 1.8"
+gem "google-cloud-storage", "~> 1.11"
 
 require "google/cloud/storage"
 require "net/http"
@@ -32,28 +32,21 @@ module ActiveStorage
       end
     end
 
-    # FIXME: Download in chunks when given a block.
-    def download(key)
-      instrument :download, key: key do
-        io = file_for(key).download
-        io.rewind
-
-        if block_given?
-          yield io.string
-        else
-          io.string
+    def download(key, &block)
+      if block_given?
+        instrument :streaming_download, key: key do
+          stream(key, &block)
+        end
+      else
+        instrument :download, key: key do
+          file_for(key).download.string
         end
       end
     end
 
     def download_chunk(key, range)
       instrument :download_chunk, key: key, range: range do
-        file = file_for(key)
-        uri  = URI(file.signed_url(expires: 30.seconds))
-
-        Net::HTTP.start(uri.host, uri.port, use_ssl: uri.scheme == "https") do |client|
-          client.get(uri, "Range" => "bytes=#{range.begin}-#{range.exclude_end? ? range.end - 1 : range.end}").body
-        end
+        file_for(key).download(range: range).string
       end
     end
 
@@ -111,8 +104,21 @@ module ActiveStorage
     private
       attr_reader :config
 
-      def file_for(key)
-        bucket.file(key, skip_lookup: true)
+      def file_for(key, skip_lookup: true)
+        bucket.file(key, skip_lookup: skip_lookup)
+      end
+
+      # Reads the file for the given key in chunks, yielding each to the block.
+      def stream(key)
+        file = file_for(key, skip_lookup: false)
+
+        chunk_size = 5.megabytes
+        offset = 0
+
+        while offset < file.size
+          yield file.download(range: offset..(offset + chunk_size - 1)).string
+          offset += chunk_size
+        end
       end
 
       def bucket
diff --git a/activestorage/test/service/shared_service_tests.rb b/activestorage/test/service/shared_service_tests.rb
index 24debe7f47..b9f352e460 100644
--- a/activestorage/test/service/shared_service_tests.rb
+++ b/activestorage/test/service/shared_service_tests.rb
@@ -51,13 +51,21 @@ module ActiveStorage::Service::SharedServiceTests
     end
 
     test "downloading in chunks" do
-      chunks = []
+      key = SecureRandom.base58(24)
+      expected_chunks = [ "a" * 5.megabytes, "b" ]
+      actual_chunks = []
 
-      @service.download(FIXTURE_KEY) do |chunk|
-        chunks << chunk
-      end
+      begin
+        @service.upload key, StringIO.new(expected_chunks.join)
+
+        @service.download key do |chunk|
+          actual_chunks << chunk
+        end
 
-      assert_equal [ FIXTURE_DATA ], chunks
+        assert_equal expected_chunks, actual_chunks, "Downloaded chunks did not match uploaded data"
+      ensure
+        @service.delete key
+      end
     end
 
     test "downloading partially" do