Configure services that reference other services

* Move service configuration from the Engine to Service
* Delegate configuration mechanics to internal Service::Configurator
* Delegate service building to the concrete Service classes, allowing
  them to configure composed services.
* Implement for the Mirror service.

10 files changed, 89 insertions, 43 deletions

diff --git a/lib/active_storage/engine.rb b/lib/active_storage/engine.rb
index d35d3c16db..d066a689eb 100644
--- a/lib/active_storage/engine.rb
+++ b/lib/active_storage/engine.rb
@@ -25,31 +25,28 @@ module ActiveStorage
     end
 
     config.after_initialize do |app|
-      config_choice = app.config.active_storage.service
-      config_file   = Pathname.new(Rails.root.join("config/storage_services.yml"))
-
-      if config_choice
+      if config_choice = app.config.active_storage.service
+        config_file = Pathname.new(Rails.root.join("config/storage_services.yml"))
         raise("Couldn't find Active Storage configuration in #{config_file}") unless config_file.exist?
 
-        begin
-          require "yaml"
-          require "erb"
-          configs = YAML.load(ERB.new(config_file.read).result) || {}
+        require "yaml"
+        require "erb"
 
-          if service_configuration = configs[config_choice.to_s].symbolize_keys
-            service_name = service_configuration.delete(:service)
+        configs =
+          begin
+            YAML.load(ERB.new(config_file.read).result) || {}
+          rescue Psych::SyntaxError => e
+            raise "YAML syntax error occurred while parsing #{config_file}. " \
+                  "Please note that YAML must be consistently indented using spaces. Tabs are not allowed. " \
+                  "Error: #{e.message}"
+          end
 
-            ActiveStorage::Blob.service = ActiveStorage::Service.configure(service_name, service_configuration)
-          else
-            raise "Couldn't configure Active Storage as #{config_choice} was not found in #{config_file}"
+        ActiveStorage::Blob.service =
+          begin
+            ActiveStorage::Service.configure config_choice, configs
+          rescue => e
+            raise e, "Cannot load `Rails.config.active_storage.service`:\n#{e.message}", e.backtrace
           end
-        rescue Psych::SyntaxError => e
-          raise "YAML syntax error occurred while parsing #{config_file}. " \
-                "Please note that YAML must be consistently indented using spaces. Tabs are not allowed. " \
-                "Error: #{e.message}"
-        rescue => e
-          raise e, "Cannot load `Rails.config.active_storage.service`:\n#{e.message}", e.backtrace
-        end
       end
     end
   end
diff --git a/lib/active_storage/service.rb b/lib/active_storage/service.rb
index f15958fda9..1a6a55739f 100644
--- a/lib/active_storage/service.rb
+++ b/lib/active_storage/service.rb
@@ -32,13 +32,15 @@
 class ActiveStorage::Service
   class ActiveStorage::IntegrityError < StandardError; end
 
-  def self.configure(service, **options)
-    begin
-      require "active_storage/service/#{service.to_s.downcase}_service"
-      ActiveStorage::Service.const_get(:"#{service}Service").new(**options)
-    rescue LoadError => e
-      puts "Couldn't configure service: #{service} (#{e.message})"
-    end
+  def self.configure(service_name, configurations)
+    require 'active_storage/service/configurator'
+    Configurator.new(service_name, configurations).build
+  end
+
+  # Override in subclasses that stitch together multiple services and hence
+  # need to do additional lookups from configurations. See MirrorService.
+  def self.build(config, configurations) #:nodoc:
+    new(config)
   end
 
   def upload(key, io, checksum: nil)
diff --git a/lib/active_storage/service/configurator.rb b/lib/active_storage/service/configurator.rb
new file mode 100644
index 0000000000..5054e07ec7
--- /dev/null
+++ b/lib/active_storage/service/configurator.rb
@@ -0,0 +1,31 @@
+class ActiveStorage::Service::Configurator #:nodoc:
+  def initialize(service_name, configurations)
+    @service_name, @configurations = service_name.to_sym, configurations.symbolize_keys
+  end
+
+  def build
+    service_class.build(service_config.except(:service), @configurations)
+  end
+
+  private
+    def service_class
+      resolve service_class_name
+    end
+
+    def service_class_name
+      service_config.fetch :service do
+        raise "Missing Active Storage `service: …` configuration for #{service_config.inspect}"
+      end
+    end
+
+    def service_config
+      @configurations.fetch @service_name do
+        raise "Missing configuration for the #{@service_name.inspect} Active Storage service. Configurations available for #{@configurations.keys.inspect}"
+      end
+    end
+
+    def resolve(service_class_name)
+      require "active_storage/service/#{service_class_name.to_s.downcase}_service"
+      ActiveStorage::Service.const_get(:"#{service_class_name}Service")
+    end
+end
diff --git a/lib/active_storage/service/mirror_service.rb b/lib/active_storage/service/mirror_service.rb
index 7ec166aace..eec1f2af65 100644
--- a/lib/active_storage/service/mirror_service.rb
+++ b/lib/active_storage/service/mirror_service.rb
@@ -5,6 +5,17 @@ class ActiveStorage::Service::MirrorService < ActiveStorage::Service
 
   delegate :download, :exist?, :url, to: :primary
 
+  # Stitch together from named configuration.
+  def self.build(mirror_config, all_configurations) #:nodoc:
+    primary = ActiveStorage::Service.configure(mirror_config.fetch(:primary), all_configurations)
+
+    mirrors = mirror_config.fetch(:mirrors).collect do |service_name|
+      ActiveStorage::Service.configure(service_name.to_sym, all_configurations)
+    end
+
+    new primary: primary, mirrors: mirrors
+  end
+
   def initialize(primary:, mirrors:)
     @primary, @mirrors = primary, mirrors
   end
diff --git a/lib/active_storage/storage_services.yml b/lib/active_storage/storage_services.yml
index d3f001a27b..a93304d88f 100644
--- a/lib/active_storage/storage_services.yml
+++ b/lib/active_storage/storage_services.yml
@@ -24,4 +24,4 @@ google:
 mirror:
   service: Mirror
   primary: local
-  secondaries: [ amazon, google ]
+  mirrors: [ amazon, google ]
diff --git a/test/service/disk_service_test.rb b/test/service/disk_service_test.rb
index 5dd7cff303..bd2e68277e 100644
--- a/test/service/disk_service_test.rb
+++ b/test/service/disk_service_test.rb
@@ -2,7 +2,7 @@ require "tmpdir"
 require "service/shared_service_tests"
 
 class ActiveStorage::Service::DiskServiceTest < ActiveSupport::TestCase
-  SERVICE = ActiveStorage::Service.configure(:Disk, root: File.join(Dir.tmpdir, "active_storage"))
+  SERVICE = ActiveStorage::Service.configure(:test, test: { service: "Disk", root: File.join(Dir.tmpdir, "active_storage") })
 
   include ActiveStorage::Service::SharedServiceTests
 end
diff --git a/test/service/gcs_service_test.rb b/test/service/gcs_service_test.rb
index 42f9cd3061..7d4700498b 100644
--- a/test/service/gcs_service_test.rb
+++ b/test/service/gcs_service_test.rb
@@ -2,7 +2,7 @@ require "service/shared_service_tests"
 
 if SERVICE_CONFIGURATIONS[:gcs]
   class ActiveStorage::Service::GCSServiceTest < ActiveSupport::TestCase
-    SERVICE = ActiveStorage::Service.configure(:GCS, SERVICE_CONFIGURATIONS[:gcs])
+    SERVICE = ActiveStorage::Service.configure(:gcs, SERVICE_CONFIGURATIONS)
 
     include ActiveStorage::Service::SharedServiceTests
 
diff --git a/test/service/mirror_service_test.rb b/test/service/mirror_service_test.rb
index 10af41c0a8..6fee3cadd2 100644
--- a/test/service/mirror_service_test.rb
+++ b/test/service/mirror_service_test.rb
@@ -2,12 +2,17 @@ require "tmpdir"
 require "service/shared_service_tests"
 
 class ActiveStorage::Service::MirrorServiceTest < ActiveSupport::TestCase
-  PRIMARY_DISK_SERVICE   = ActiveStorage::Service.configure(:Disk, root: File.join(Dir.tmpdir, "active_storage"))
-  MIRROR_SERVICES = (1..3).map do |i|
-    ActiveStorage::Service.configure(:Disk, root: File.join(Dir.tmpdir, "active_storage_mirror_#{i}"))
-  end
+  mirror_config = (1..3).map do |i|
+    [ "mirror_#{i}",
+      service: "Disk",
+      root: File.join(Dir.tmpdir, "active_storage_mirror_#{i}") ]
+  end.to_h
+
+  config = mirror_config.merge \
+    mirror:   { service: "Mirror", primary: 'primary', mirrors: mirror_config.keys },
+    primary:  { service: "Disk", root: File.join(Dir.tmpdir, "active_storage") }
 
-  SERVICE = ActiveStorage::Service.configure :Mirror, primary: PRIMARY_DISK_SERVICE, mirrors: MIRROR_SERVICES
+  SERVICE = ActiveStorage::Service.configure :mirror, config
 
   include ActiveStorage::Service::SharedServiceTests
 
@@ -16,8 +21,8 @@ class ActiveStorage::Service::MirrorServiceTest < ActiveSupport::TestCase
       data = "Something else entirely!"
       key  = upload(data, to: @service)
 
-      assert_equal data, PRIMARY_DISK_SERVICE.download(key)
-      MIRROR_SERVICES.each do |mirror|
+      assert_equal data, SERVICE.primary.download(key)
+      SERVICE.mirrors.each do |mirror|
         assert_equal data, mirror.download(key)
       end
     ensure
@@ -27,22 +32,22 @@ class ActiveStorage::Service::MirrorServiceTest < ActiveSupport::TestCase
 
   test "downloading from primary service" do
     data = "Something else entirely!"
-    key  = upload(data, to: PRIMARY_DISK_SERVICE)
+    key  = upload(data, to: SERVICE.primary)
 
     assert_equal data, @service.download(key)
   end
 
   test "deleting from all services" do
     @service.delete FIXTURE_KEY
-    assert_not PRIMARY_DISK_SERVICE.exist?(FIXTURE_KEY)
-    MIRROR_SERVICES.each do |mirror|
+    assert_not SERVICE.primary.exist?(FIXTURE_KEY)
+    SERVICE.mirrors.each do |mirror|
       assert_not mirror.exist?(FIXTURE_KEY)
     end
   end
 
   test "URL generation in primary service" do
     travel_to Time.now do
-      assert_equal PRIMARY_DISK_SERVICE.url(FIXTURE_KEY, expires_in: 2.minutes, disposition: :inline, filename: "test.txt"),
+      assert_equal SERVICE.primary.url(FIXTURE_KEY, expires_in: 2.minutes, disposition: :inline, filename: "test.txt"),
         @service.url(FIXTURE_KEY, expires_in: 2.minutes, disposition: :inline, filename: "test.txt")
     end
   end
diff --git a/test/service/s3_service_test.rb b/test/service/s3_service_test.rb
index 604dfd6c60..e8cc4cb5f4 100644
--- a/test/service/s3_service_test.rb
+++ b/test/service/s3_service_test.rb
@@ -2,7 +2,7 @@ require "service/shared_service_tests"
 
 if SERVICE_CONFIGURATIONS[:s3]
   class ActiveStorage::Service::S3ServiceTest < ActiveSupport::TestCase
-    SERVICE = ActiveStorage::Service.configure(:S3, SERVICE_CONFIGURATIONS[:s3])
+    SERVICE = ActiveStorage::Service.configure(:s3, SERVICE_CONFIGURATIONS)
 
     include ActiveStorage::Service::SharedServiceTests
   end
diff --git a/test/test_helper.rb b/test/test_helper.rb
index dcabe33c18..b374a777dd 100644
--- a/test/test_helper.rb
+++ b/test/test_helper.rb
@@ -7,7 +7,7 @@ require "byebug"
 require "active_storage"
 
 require "active_storage/service"
-ActiveStorage::Blob.service = ActiveStorage::Service.configure(:Disk, root: File.join(Dir.tmpdir, "active_storage"))
+ActiveStorage::Blob.service = ActiveStorage::Service.configure(:test, test: { service: 'Disk', root: File.join(Dir.tmpdir, "active_storage") })
 
 require "active_storage/verified_key_with_expiration"
 ActiveStorage::VerifiedKeyWithExpiration.verifier = ActiveSupport::MessageVerifier.new("Testing")