Merge pull request #109 from alphagov/add-cache-control-header-to-s3-…

…object

Add Cache-Control header to S3 object

app/controllers/application_controller.rb

@@ -27,6 +27,8 @@ def error(code, message)
   end
 
   def set_expiry(duration)
-    expires_in duration, public: true unless Rails.env.development?
+    unless Rails.env.development?
+      expires_in duration, **AssetManager.cache_control.options
+    end
   end
 end

app/controllers/media_controller.rb

@@ -15,7 +15,7 @@ def download
 
     respond_to do |format|
       format.any do
-        set_expiry(24.hours)
+        set_expiry(AssetManager.cache_control.max_age)
         if stream_from_s3?
           body = Services.cloud_storage.load(asset)
           send_data(body.read, filename: File.basename(asset.file.path), disposition: 'inline')
@@ -29,8 +29,7 @@ def download
 protected
 
   def stream_from_s3?
-    config = AssetManager::Application.config
-    config.stream_all_assets_from_s3 || params[:stream_from_s3].present?
+    AssetManager.stream_all_assets_from_s3 || params[:stream_from_s3].present?
   end
 
   def filename_current?

app/models/asset.rb

@@ -71,7 +71,7 @@ def accessible_by?(user)
   end
 
   def save_to_cloud_storage
-    Services.cloud_storage.save(self)
+    Services.cloud_storage.save(self, cache_control: AssetManager.cache_control.header)
   rescue => e
     Airbrake.notify_or_ignore(e, params: { id: self.id, filename: self.filename })
     raise

config/application.rb

@@ -33,4 +33,8 @@ class Application < Rails::Application
 
     config.action_dispatch.x_sendfile_header = "X-Accel-Redirect"
   end
+
+  mattr_accessor :aws_s3_bucket_name
+  mattr_accessor :stream_all_assets_from_s3
+  mattr_accessor :cache_control
 end

config/initializers/aws.rb

@@ -1,5 +1,5 @@
-AssetManager::Application.config.aws_s3_bucket_name = ENV['AWS_S3_BUCKET_NAME']
-AssetManager::Application.config.stream_all_assets_from_s3 = ENV['STREAM_ALL_ASSETS_FROM_S3'].present?
+AssetManager.aws_s3_bucket_name = ENV['AWS_S3_BUCKET_NAME']
+AssetManager.stream_all_assets_from_s3 = ENV['STREAM_ALL_ASSETS_FROM_S3'].present?
 
 Aws.config.update(
   logger: Rails.logger

config/initializers/cache_control.rb

@@ -0,0 +1,6 @@
+require 'cache_control_configuration'
+
+AssetManager.cache_control = CacheControlConfiguration.new(
+  max_age: 24.hours,
+  public: true
+)

lib/cache_control_configuration.rb

@@ -0,0 +1,22 @@
+class CacheControlConfiguration
+  def initialize(attributes = {})
+    @attributes = attributes
+  end
+
+  def max_age
+    @attributes[:max_age]
+  end
+
+  def options
+    @attributes.except(:max_age)
+  end
+
+  def header
+    response = ActionDispatch::Response.new
+    @attributes.each do |key, value|
+      response.cache_control[key] = value
+    end
+    response.prepare!
+    response['Cache-Control']
+  end
+end

lib/s3_storage.rb

@@ -19,8 +19,8 @@ def initialize(bucket_name)
     @bucket_name = bucket_name
   end
 
-  def save(asset)
-    object_for(asset).upload_file(asset.file.path)
+  def save(asset, options = {})
+    object_for(asset).upload_file(asset.file.path, options)
   end
 
   def load(asset)

lib/services.rb

@@ -2,7 +2,6 @@
 
 module Services
   def self.cloud_storage
-    aws_s3_bucket_name = AssetManager::Application.config.aws_s3_bucket_name
-    @cloud_storage ||= S3Storage.build(aws_s3_bucket_name)
+    @cloud_storage ||= S3Storage.build(AssetManager.aws_s3_bucket_name)
   end
 end

spec/controllers/media_controller_spec.rb

@@ -76,7 +76,7 @@ def do_get(extra_params = {})
         before do
           allow(Services).to receive(:cloud_storage).and_return(cloud_storage)
           allow(cloud_storage).to receive(:load).with(asset).and_return(io)
-          allow(AssetManager::Application.config).to receive(:stream_all_assets_from_s3).and_return(true)
+          allow(AssetManager).to receive(:stream_all_assets_from_s3).and_return(true)
         end
 
         it "should be successful" do

spec/lib/cache_control_configuration_spec.rb

@@ -0,0 +1,39 @@
+require 'rails_helper'
+
+RSpec.describe CacheControlConfiguration do
+  subject { described_class.new(attributes) }
+
+  describe '#max_age' do
+    let(:attributes) { { max_age: 1.hour } }
+
+    it 'returns max_age attribute value' do
+      expect(subject.max_age).to eq(1.hour)
+    end
+  end
+
+  describe '#options' do
+    let(:attributes) { { max_age: 1.hour, public: true } }
+
+    it 'returns all attributes and their values except for max_age' do
+      expect(subject.options).to eq(public: true)
+    end
+  end
+
+  describe '#header' do
+    context 'when attributes are supplied to constructor' do
+      let(:attributes) { { max_age: 1.hour, public: true, must_revalidate: true } }
+
+      it 'returns Cache-Control header value c.f. expires_in' do
+        expect(subject.header).to eq('max-age=3600, public, must-revalidate')
+      end
+    end
+
+    context 'when only max_age attribute is supplied to constructor' do
+      let(:attributes) { { max_age: 1.hour } }
+
+      it "returns Cache-Control header value with default visibility" do
+        expect(subject.header).to include('private')
+      end
+    end
+  end
+end

spec/lib/s3_storage_spec.rb

@@ -15,11 +15,17 @@
 
   describe '#save' do
     it 'uploads file to S3 bucket' do
-      expect(s3_object).to receive(:upload_file).with(asset.file.path)
+      expect(s3_object).to receive(:upload_file).with(asset.file.path, anything)
 
       subject.save(asset)
     end
 
+    it 'passes options to Aws::S3::Object#upload_file' do
+      expect(s3_object).to receive(:upload_file).with(anything, cache_control: 'cache-control-header')
+
+      subject.save(asset, cache_control: 'cache-control-header')
+    end
+
     context 'when bucket name is blank' do
       let(:bucket_name) { '' }
 

spec/models/asset_spec.rb

@@ -120,13 +120,22 @@
   describe "#save_to_cloud_storage" do
     let(:asset) { FactoryGirl.create(:clean_asset) }
     let(:cloud_storage) { double(:cloud_storage) }
+    let(:cache_control) { instance_double(CacheControlConfiguration) }
 
     before do
       allow(Services).to receive(:cloud_storage).and_return(cloud_storage)
+      allow(AssetManager).to receive(:cache_control).and_return(cache_control)
+      allow(cache_control).to receive(:header).and_return('cache-control-header')
     end
 
     it 'saves the asset to cloud storage' do
-      expect(cloud_storage).to receive(:save).with(asset)
+      expect(cloud_storage).to receive(:save).with(asset, anything)
+
+      asset.save_to_cloud_storage
+    end
+
+    it 'sets the Cache-Control header on the asset stored in the cloud' do
+      expect(cloud_storage).to receive(:save).with(anything, cache_control: 'cache-control-header')
 
       asset.save_to_cloud_storage
     end

spec/requests/media_requests_spec.rb

@@ -13,7 +13,7 @@
       let(:asset) { FactoryGirl.create(:clean_asset) }
 
       before do
-        allow(AssetManager::Application.config).to receive(:aws_s3_bucket_name).and_return(nil)
+        allow(AssetManager).to receive(:aws_s3_bucket_name).and_return(nil)
       end
 
       it "should respond with internal server error status" do