Move configuration classes into dedicated module

app/services/configuration/configuration_service.rb

@@ -0,0 +1,13 @@
+module Configuration
+  class ConfigurationService
+    attr_reader :s3_buckets, :redis_uris
+
+    def s3_config_present?
+      raise NotImplementedError
+    end
+
+    def redis_config_present?
+      raise NotImplementedError
+    end
+  end
+end

app/services/configuration/paas_configuration_service.rb

@@ -0,0 +1,63 @@
+module Configuration
+  class PaasConfigurationService < ConfigurationService
+    def initialize(logger = Rails.logger)
+      super()
+      @logger = logger
+      @paas_config = read_pass_config
+      @s3_buckets = read_s3_buckets
+      @redis_uris = read_redis_uris
+    end
+
+    def s3_config_present?
+      config_present? && @paas_config.key?(:"aws-s3-bucket")
+    end
+
+    def redis_config_present?
+      config_present? && @paas_config.key?(:redis)
+    end
+
+  private
+
+    def config_present?
+      !ENV["VCAP_SERVICES"].nil?
+    end
+
+    def read_pass_config
+      unless config_present?
+        @logger.warn("Could not find VCAP_SERVICES in the environment!")
+        return {}
+      end
+
+      begin
+        JSON.parse(ENV["VCAP_SERVICES"], { symbolize_names: true })
+      rescue StandardError
+        @logger.warn("Could not parse VCAP_SERVICES!")
+        {}
+      end
+    end
+
+    def read_s3_buckets
+      return {} unless s3_config_present?
+
+      s3_buckets = {}
+      @paas_config[:"aws-s3-bucket"].each do |bucket_config|
+        if bucket_config.key?(:instance_name)
+          s3_buckets[bucket_config[:instance_name].to_sym] = bucket_config
+        end
+      end
+      s3_buckets
+    end
+
+    def read_redis_uris
+      return {} unless redis_config_present?
+
+      redis_uris = {}
+      @paas_config[:redis].each do |redis_config|
+        if redis_config.key?(:instance_name)
+          redis_uris[redis_config[:instance_name].to_sym] = redis_config.dig(:credentials, :uri)
+        end
+      end
+      redis_uris
+    end
+  end
+end

app/services/paas_configuration_service.rb

@@ -1,62 +0,0 @@
-class PaasConfigurationService
-  attr_reader :s3_buckets, :redis_uris
-
-  def initialize(logger = Rails.logger)
-    @logger = logger
-    @paas_config = read_pass_config
-    @s3_buckets = read_s3_buckets
-    @redis_uris = read_redis_uris
-  end
-
-  def config_present?
-    !ENV["VCAP_SERVICES"].nil?
-  end
-
-  def s3_config_present?
-    config_present? && @paas_config.key?(:"aws-s3-bucket")
-  end
-
-  def redis_config_present?
-    config_present? && @paas_config.key?(:redis)
-  end
-
-private
-
-  def read_pass_config
-    unless config_present?
-      @logger.warn("Could not find VCAP_SERVICES in the environment!")
-      return {}
-    end
-
-    begin
-      JSON.parse(ENV["VCAP_SERVICES"], { symbolize_names: true })
-    rescue StandardError
-      @logger.warn("Could not parse VCAP_SERVICES!")
-      {}
-    end
-  end
-
-  def read_s3_buckets
-    return {} unless s3_config_present?
-
-    s3_buckets = {}
-    @paas_config[:"aws-s3-bucket"].each do |bucket_config|
-      if bucket_config.key?(:instance_name)
-        s3_buckets[bucket_config[:instance_name].to_sym] = bucket_config
-      end
-    end
-    s3_buckets
-  end
-
-  def read_redis_uris
-    return {} unless redis_config_present?
-
-    redis_uris = {}
-    @paas_config[:redis].each do |redis_config|
-      if redis_config.key?(:instance_name)
-        redis_uris[redis_config[:instance_name].to_sym] = redis_config.dig(:credentials, :uri)
-      end
-    end
-    redis_uris
-  end
-end

app/services/storage/s3_service.rb

@@ -36,8 +36,8 @@ module Storage
   private
 
     def create_configuration
-      unless @config_service.config_present?
+      unless @config_service.s3_config_present?
-        raise "No PaaS configuration present"
+        raise "No S3 bucket is present in the PaaS configuration"
       end
       unless @config_service.s3_buckets.key?(@instance_name)
         raise "#{@instance_name} instance name could not be found"

config/initializers/rack_attack.rb

@@ -1,10 +1,11 @@
-require "paas_configuration_service"
+require "configuration/configuration_service"
+require "configuration/paas_configuration_service"
 
 if Rails.env.development? || Rails.env.test?
   Rack::Attack.cache.store = ActiveSupport::Cache::MemoryStore.new
   Rack::Attack.enabled = false
 else
-  redis_url = PaasConfigurationService.new.redis_uris[:"dluhc-core-#{Rails.env}-redis-rate-limit"]
+  redis_url = Configuration::PaasConfigurationService.new.redis_uris[:"dluhc-core-#{Rails.env}-redis-rate-limit"]
   Rack::Attack.cache.store = ActiveSupport::Cache::RedisCacheStore.new(url: redis_url)
 end
 

lib/tasks/data_export.rake

@@ -4,7 +4,7 @@ namespace :core do
     format = args[:format]
     full_update = args[:full_update].present? && args[:full_update] == "true"
 
-    storage_service = Storage::S3Service.new(PaasConfigurationService.new, ENV["EXPORT_PAAS_INSTANCE"])
+    storage_service = Storage::S3Service.new(Configuration::PaasConfigurationService.new, ENV["EXPORT_PAAS_INSTANCE"])
     export_service = Exports::CaseLogExportService.new(storage_service)
 
     if format.present? && format == "CSV"

lib/tasks/data_import.rake

@@ -5,7 +5,7 @@ namespace :core do
     path = args[:path]
     raise "Usage: rake core:data_import['data_type', 'path/to/xml_files']" if path.blank? || type.blank?
 
-    storage_service = Storage::S3Service.new(PaasConfigurationService.new, ENV["IMPORT_PAAS_INSTANCE"])
+    storage_service = Storage::S3Service.new(Configuration::PaasConfigurationService.new, ENV["IMPORT_PAAS_INSTANCE"])
 
     case type
     when "organisation"

lib/tasks/data_import_field.rake

@@ -5,7 +5,7 @@ namespace :core do
     path = args[:path]
     raise "Usage: rake core:data_import_field['field','path/to/xml_files']" if path.blank? || field.blank?
 
-    storage_service = Storage::S3Service.new(PaasConfigurationService.new, ENV["IMPORT_PAAS_INSTANCE"])
+    storage_service = Storage::S3Service.new(Configuration::PaasConfigurationService.new, ENV["IMPORT_PAAS_INSTANCE"])
 
     # We only allow a reduced list of known fields to be updatable
     case field

lib/tasks/full_import.rake

@@ -6,7 +6,7 @@ namespace :core do
     archive_path = args[:archive_path]
     raise "Usage: rake core:full_import['path/to/archive']" if archive_path.blank?
 
-    s3_service = Storage::S3Service.new(PaasConfigurationService.new, ENV["IMPORT_PAAS_INSTANCE"])
+    s3_service = Storage::S3Service.new(Configuration::PaasConfigurationService.new, ENV["IMPORT_PAAS_INSTANCE"])
     archive_io = s3_service.get_file_io(archive_path)
     archive_service = Storage::ArchiveService.new(archive_io)
 

spec/lib/tasks/data_export_spec.rb

@@ -6,7 +6,7 @@ describe "rake core:data_export", type: task do
 
   let(:paas_instance) { "paas_export_instance" }
   let(:storage_service) { instance_double(Storage::S3Service) }
-  let(:paas_config_service) { instance_double(PaasConfigurationService) }
+  let(:paas_config_service) { instance_double(Configuration::PaasConfigurationService) }
   let(:export_service) { instance_double(Exports::CaseLogExportService) }
 
   before do
@@ -15,7 +15,7 @@ describe "rake core:data_export", type: task do
     task.reenable
 
     allow(Storage::S3Service).to receive(:new).and_return(storage_service)
-    allow(PaasConfigurationService).to receive(:new).and_return(paas_config_service)
+    allow(Configuration::PaasConfigurationService).to receive(:new).and_return(paas_config_service)
     allow(Exports::CaseLogExportService).to receive(:new).and_return(export_service)
     allow(ENV).to receive(:[])
     allow(ENV).to receive(:[]).with("EXPORT_PAAS_INSTANCE").and_return(paas_instance)

spec/lib/tasks/data_import_spec.rb

@@ -6,7 +6,7 @@ describe "rake core:data_import", type: :task do
 
   let(:instance_name) { "paas_import_instance" }
   let(:storage_service) { instance_double(Storage::S3Service) }
-  let(:paas_config_service) { instance_double(PaasConfigurationService) }
+  let(:paas_config_service) { instance_double(Configuration::PaasConfigurationService) }
 
   before do
     Rake.application.rake_require("tasks/data_import")
@@ -14,7 +14,7 @@ describe "rake core:data_import", type: :task do
     task.reenable
 
     allow(Storage::S3Service).to receive(:new).and_return(storage_service)
-    allow(PaasConfigurationService).to receive(:new).and_return(paas_config_service)
+    allow(Configuration::PaasConfigurationService).to receive(:new).and_return(paas_config_service)
     allow(ENV).to receive(:[])
     allow(ENV).to receive(:[]).with("IMPORT_PAAS_INSTANCE").and_return(instance_name)
   end

spec/lib/tasks/date_import_field_spec.rb

@@ -6,7 +6,7 @@ describe "rake core:data_import_field", type: :task do
 
   let(:instance_name) { "paas_import_instance" }
   let(:storage_service) { instance_double(Storage::S3Service) }
-  let(:paas_config_service) { instance_double(PaasConfigurationService) }
+  let(:paas_config_service) { instance_double(Configuration::PaasConfigurationService) }
 
   before do
     Rake.application.rake_require("tasks/data_import_field")
@@ -14,7 +14,7 @@ describe "rake core:data_import_field", type: :task do
     task.reenable
 
     allow(Storage::S3Service).to receive(:new).and_return(storage_service)
-    allow(PaasConfigurationService).to receive(:new).and_return(paas_config_service)
+    allow(Configuration::PaasConfigurationService).to receive(:new).and_return(paas_config_service)
     allow(ENV).to receive(:[])
     allow(ENV).to receive(:[]).with("IMPORT_PAAS_INSTANCE").and_return(instance_name)
   end

spec/lib/tasks/full_import_spec.rb

@@ -7,14 +7,14 @@ describe "rake core:full_import", type: :task do
 
   let(:s3_service) { instance_double(Storage::S3Service) }
   let(:archive_service) { instance_double(Storage::ArchiveService) }
-  let(:paas_config_service) { instance_double(PaasConfigurationService) }
+  let(:paas_config_service) { instance_double(Configuration::PaasConfigurationService) }
 
   before do
     Rake.application.rake_require("tasks/full_import")
     Rake::Task.define_task(:environment)
     task.reenable
 
-    allow(PaasConfigurationService).to receive(:new).and_return(paas_config_service)
+    allow(Configuration::PaasConfigurationService).to receive(:new).and_return(paas_config_service)
     allow(Storage::S3Service).to receive(:new).and_return(s3_service)
     allow(s3_service).to receive(:get_file_io)
     allow(Storage::ArchiveService).to receive(:new).and_return(archive_service)

spec/services/configuration/paas_configuration_service_spec.rb

@@ -1,6 +1,6 @@
 require "rails_helper"
 
-RSpec.describe PaasConfigurationService do
+RSpec.describe Configuration::PaasConfigurationService do
   subject(:config_service) { described_class.new(logger) }
 
   let(:logger) { instance_double(ActiveSupport::LogSubscriber) }
@@ -8,14 +8,38 @@ RSpec.describe PaasConfigurationService do
   context "when the paas configuration is unavailable" do
     before { allow(logger).to receive(:warn) }
 
-    it "returns the configuration as not present" do
+    it "returns the S3 configuration as not present" do
-      expect(config_service.config_present?).to be(false)
+      expect(config_service.s3_config_present?).to be(false)
+    end
+
+    it "returns the redis configuration as not present" do
+      expect(config_service.redis_config_present?).to be(false)
+    end
+
+    it "does not retrieve any S3 bucket configuration" do
+      expect(config_service.s3_buckets).to be_a(Hash)
+      expect(config_service.s3_buckets).to be_empty
+    end
+
+    it "does not retrieve any redis configuration" do
+      expect(config_service.redis_uris).to be_a(Hash)
+      expect(config_service.redis_uris).to be_empty
+    end
+  end
+
+  context "when the paas configuration is present but empty" do
+    before do
+      allow(ENV).to receive(:[]).with("VCAP_SERVICES").and_return("{}")
     end
 
     it "returns the S3 configuration as not present" do
       expect(config_service.s3_config_present?).to be(false)
     end
 
+    it "returns the redis configuration as not present" do
+      expect(config_service.redis_config_present?).to be(false)
+    end
+
     it "does not retrieve any S3 bucket configuration" do
       expect(config_service.s3_buckets).to be_a(Hash)
       expect(config_service.s3_buckets).to be_empty
@@ -41,7 +65,7 @@ RSpec.describe PaasConfigurationService do
     end
   end
 
-  context "when the paas configuration is present with S3 buckets" do
+  context "when the paas configuration is present with S3 configured" do
     let(:vcap_services) do
       <<-JSON
         {"aws-s3-bucket": [{"instance_name": "bucket_1"},{"instance_name": "bucket_2"}]}
@@ -52,14 +76,14 @@ RSpec.describe PaasConfigurationService do
       allow(ENV).to receive(:[]).with("VCAP_SERVICES").and_return(vcap_services)
     end
 
-    it "returns the configuration as present" do
-      expect(config_service.config_present?).to be(true)
-    end
-
     it "returns the S3 configuration as present" do
       expect(config_service.s3_config_present?).to be(true)
     end
 
+    it "returns the redis configuration as not present" do
+      expect(config_service.redis_config_present?).to be(false)
+    end
+
     it "does retrieve the S3 bucket configurations" do
       s3_buckets = config_service.s3_buckets
 
@@ -70,29 +94,42 @@ RSpec.describe PaasConfigurationService do
     end
   end
 
-  context "when the paas configuration is present without S3 buckets" do
+  context "when the paas configuration is present with redis configured" do
+    let(:vcap_services) do
+      <<-JSON
+        {"redis": [{"instance_name": "redis_1", "credentials": {"uri": "redis_uri" }}]}
+      JSON
+    end
+
     before do
-      allow(ENV).to receive(:[]).with("VCAP_SERVICES").and_return("{}")
+      allow(ENV).to receive(:[]).with("VCAP_SERVICES").and_return(vcap_services)
     end
 
-    it "returns the configuration as present" do
+    it "returns the redis configuration as present" do
-      expect(config_service.config_present?).to be(true)
+      expect(config_service.redis_config_present?).to be(true)
     end
 
     it "returns the S3 configuration as not present" do
       expect(config_service.s3_config_present?).to be(false)
     end
 
-    it "does not retrieve any S3 bucket configuration" do
+    it "does retrieve the redis configurations" do
-      expect(config_service.s3_buckets).to be_a(Hash)
+      redis_uris = config_service.redis_uris
-      expect(config_service.s3_buckets).to be_empty
+
+      expect(redis_uris).not_to be_empty
+      expect(redis_uris.count).to be(1)
+      expect(redis_uris).to have_key(:redis_1)
+      expect(redis_uris[:redis_1]).to eq("redis_uri")
     end
   end
 
-  context "when the paas configuration is present with redis configurations" do
+  context "when the paas configuration is present with both S3 and redis configured" do
     let(:vcap_services) do
       <<-JSON
-        {"redis": [{"instance_name": "redis_1", "credentials": {"uri": "redis_uri" }}]}
+        {
+          "aws-s3-bucket": [{"instance_name": "bucket_1"},{"instance_name": "bucket_2"}],
+          "redis": [{"instance_name": "redis_1", "credentials": {"uri": "redis_uri" }}]
+        }
       JSON
     end
 
@@ -100,8 +137,8 @@ RSpec.describe PaasConfigurationService do
       allow(ENV).to receive(:[]).with("VCAP_SERVICES").and_return(vcap_services)
     end
 
-    it "returns the configuration as present" do
+    it "returns the S3configuration as present" do
-      expect(config_service.config_present?).to be(true)
+      expect(config_service.s3_config_present?).to be(true)
     end
 
     it "returns the redis configuration as present" do
@@ -116,24 +153,14 @@ RSpec.describe PaasConfigurationService do
       expect(redis_uris).to have_key(:redis_1)
       expect(redis_uris[:redis_1]).to eq("redis_uri")
     end
-  end
-
-  context "when the paas configuration is present without redis configuration" do
-    before do
-      allow(ENV).to receive(:[]).with("VCAP_SERVICES").and_return("{}")
-    end
 
-    it "returns the configuration as present" do
+    it "does retrieve the S3 bucket configurations" do
-      expect(config_service.config_present?).to be(true)
+      s3_buckets = config_service.s3_buckets
-    end
-
-    it "returns the redis configuration as not present" do
-      expect(config_service.redis_config_present?).to be(false)
-    end
 
-    it "does not retrieve any redis uris from configuration" do
+      expect(s3_buckets).not_to be_empty
-      expect(config_service.redis_uris).to be_a(Hash)
+      expect(s3_buckets.count).to be(2)
-      expect(config_service.redis_uris).to be_empty
+      expect(s3_buckets).to have_key(:bucket_1)
+      expect(s3_buckets).to have_key(:bucket_2)
     end
   end
 end

spec/services/storage/s3_service_spec.rb

@@ -20,18 +20,24 @@ RSpec.describe Storage::S3Service do
   end
 
   context "when we create a storage service with no PaaS Configuration present" do
-    subject(:storage_service) { described_class.new(PaasConfigurationService.new, "random_instance") }
+    subject(:storage_service) { described_class.new(Configuration::PaasConfigurationService.new, "random_instance") }
 
     it "raises an exception" do
-      expect { storage_service }.to raise_error(RuntimeError, /No PaaS configuration present/)
+      expect { storage_service }.to raise_error(RuntimeError, "No S3 bucket is present in the PaaS configuration")
     end
   end
 
-  context "when we create a storage service with an unknown instance name" do
+  context "when we create a storage service and the S3 instance name is not found in the PaaS configuration" do
-    subject(:storage_service) { described_class.new(PaasConfigurationService.new, "random_instance") }
+    subject(:storage_service) { described_class.new(Configuration::PaasConfigurationService.new, "random_instance") }
+
+    let(:vcap_services) do
+      <<-JSON
+        {"aws-s3-bucket": []}
+      JSON
+    end
 
     before do
-      allow(ENV).to receive(:[]).with("VCAP_SERVICES").and_return("{}")
+      allow(ENV).to receive(:[]).with("VCAP_SERVICES").and_return(vcap_services)
     end
 
     it "raises an exception" do
@@ -40,7 +46,7 @@ RSpec.describe Storage::S3Service do
   end
 
   context "when we create a storage service with a valid instance name" do
-    subject(:storage_service) { described_class.new(PaasConfigurationService.new, instance_name) }
+    subject(:storage_service) { described_class.new(Configuration::PaasConfigurationService.new, instance_name) }
 
     before do
       allow(ENV).to receive(:[])
@@ -65,7 +71,7 @@ RSpec.describe Storage::S3Service do
   end
 
   context "when we create a storage service and write a stubbed object" do
-    subject(:storage_service) { described_class.new(PaasConfigurationService.new, instance_name) }
+    subject(:storage_service) { described_class.new(Configuration::PaasConfigurationService.new, instance_name) }
 
     let(:filename) { "my_file" }
     let(:content) { "content" }
@@ -100,7 +106,7 @@ RSpec.describe Storage::S3Service do
   end
 
   context "when we create a storage service" do
-    subject(:storage_service) { described_class.new(PaasConfigurationService.new, instance_name) }
+    subject(:storage_service) { described_class.new(Configuration::PaasConfigurationService.new, instance_name) }
 
     let(:s3_client_stub) { Aws::S3::Client.new(stub_responses: true) }