From bf5f41d948b6f3f27db7fdc2b70897aec991065f Mon Sep 17 00:00:00 2001
From: George Claghorn <george.claghorn@gmail.com>
Date: Tue, 1 May 2018 23:20:56 -0400
Subject: Support streaming downloads from Google Cloud Storage

---
 Gemfile                                            |  2 +-
 Gemfile.lock                                       | 12 +++---
 activestorage/CHANGELOG.md                         |  5 +++
 .../lib/active_storage/service/gcs_service.rb      | 44 ++++++++++++----------
 activestorage/test/service/shared_service_tests.rb | 18 ++++++---
 5 files changed, 50 insertions(+), 31 deletions(-)

diff --git a/Gemfile b/Gemfile
index fc7680c9bf..f0c489f642 100644
--- a/Gemfile
+++ b/Gemfile
@@ -85,7 +85,7 @@ end
 # Active Storage
 group :storage do
   gem "aws-sdk-s3", require: false
-  gem "google-cloud-storage", "~> 1.8", require: false
+  gem "google-cloud-storage", "~> 1.11", require: false
   gem "azure-storage", require: false
 
   gem "image_processing", "~> 1.2"
diff --git a/Gemfile.lock b/Gemfile.lock
index d5e3bafc53..8d3c3e70b7 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -240,21 +240,21 @@ GEM
     ffi (1.9.21-x86-mingw32)
     globalid (0.4.1)
       activesupport (>= 4.2.0)
-    google-api-client (0.17.3)
+    google-api-client (0.19.8)
       addressable (~> 2.5, >= 2.5.1)
       googleauth (>= 0.5, < 0.7.0)
       httpclient (>= 2.8.1, < 3.0)
       mime-types (~> 3.0)
       representable (~> 3.0)
       retriable (>= 2.0, < 4.0)
-    google-cloud-core (1.1.0)
+    google-cloud-core (1.2.0)
       google-cloud-env (~> 1.0)
     google-cloud-env (1.0.1)
       faraday (~> 0.11)
-    google-cloud-storage (1.9.0)
+    google-cloud-storage (1.11.0)
       digest-crc (~> 0.4)
-      google-api-client (~> 0.17.0)
-      google-cloud-core (~> 1.1)
+      google-api-client (~> 0.19.0)
+      google-cloud-core (~> 1.2)
       googleauth (~> 0.6.2)
     googleauth (0.6.2)
       faraday (~> 0.12)
@@ -523,7 +523,7 @@ DEPENDENCIES
   delayed_job
   delayed_job_active_record
   ffi (<= 1.9.21)
-  google-cloud-storage (~> 1.8)
+  google-cloud-storage (~> 1.11)
   hiredis
   image_processing (~> 1.2)
   json (>= 2.0.0)
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
-- 
cgit v1.2.3