From 0b6d82c63278ac2b822bc90e734cf0b3fa2cbf8a Mon Sep 17 00:00:00 2001 From: ulferts Date: Thu, 5 Mar 2020 13:35:25 +0100 Subject: [PATCH] cache non avatar attachments for a year --- lib/api/helpers/attachment_renderer.rb | 17 +++++++++++++---- lib/api/v3/attachments/attachments_api.rb | 2 +- .../lib/api/v3/users/user_avatar_api.rb | 15 ++++----------- .../bim/bcf/api/v2_1/viewpoints/api.rb | 4 +++- .../api/bcf/v2_1/viewpoints_api_spec.rb | 10 +++++++++- .../attachment_resource_shared_examples.rb | 18 +++++++++++++++++- 6 files changed, 47 insertions(+), 19 deletions(-) diff --git a/lib/api/helpers/attachment_renderer.rb b/lib/api/helpers/attachment_renderer.rb index e224926e2a2..76359ce4925 100644 --- a/lib/api/helpers/attachment_renderer.rb +++ b/lib/api/helpers/attachment_renderer.rb @@ -40,11 +40,15 @@ module API # or by directly rendering the file # # @param attachment [Attachment] Attachment to be responded with. - # @param external_link_expires_in [ActiveSupport::Duration] Time after which link expires. Default is 5 minutes. - # Only applicable in case of external storage. - def respond_with_attachment(attachment, external_link_expires_in: nil) + # @param cache_seconds [integer] Time in seconds the cache headers signal the browser to cache the attachment. + # Defaults to no cache headers. + def respond_with_attachment(attachment, cache_seconds: nil) + if cache_seconds + set_cache_headers!(cache_seconds) + end + if attachment.external_storage? - redirect attachment.external_url(expires_in: external_link_expires_in).to_s + redirect attachment.external_url(expires_in: cache_seconds).to_s else content_type attachment.content_type header['Content-Disposition'] = "#{attachment.content_disposition}; filename=#{attachment.filename}" @@ -53,6 +57,11 @@ module API end end + def set_cache_headers!(seconds) + header "Cache-Control", "public, max-age=#{seconds}" + header "Expires", CGI.rfc1123_date(Time.now.utc + seconds) + end + def avatar_link_expires_in seconds = avatar_link_expiry_seconds diff --git a/lib/api/v3/attachments/attachments_api.rb b/lib/api/v3/attachments/attachments_api.rb index 3e0fbf7add6..dcefd835ce5 100644 --- a/lib/api/v3/attachments/attachments_api.rb +++ b/lib/api/v3/attachments/attachments_api.rb @@ -65,7 +65,7 @@ module API helpers ::API::Helpers::AttachmentRenderer get do - respond_with_attachment @attachment + respond_with_attachment @attachment, cache_seconds: 1.year.to_i end end end diff --git a/modules/avatars/lib/api/v3/users/user_avatar_api.rb b/modules/avatars/lib/api/v3/users/user_avatar_api.rb index bb525295fc3..0184b7756a5 100644 --- a/modules/avatars/lib/api/v3/users/user_avatar_api.rb +++ b/modules/avatars/lib/api/v3/users/user_avatar_api.rb @@ -34,21 +34,14 @@ module API helpers ::AvatarHelper helpers ::API::Helpers::AttachmentRenderer - helpers do - def set_cache_headers! - return if @user == current_user - - header "Cache-Control", "public, max-age=#{avatar_link_expiry_seconds}" - header "Expires", CGI.rfc1123_date(Time.now.utc + avatar_link_expiry_seconds) - end - end - get '/avatar' do - set_cache_headers! + cache_seconds = @user == current_user ? nil : avatar_link_expires_in if (local_avatar = local_avatar?(@user)) - respond_with_attachment(local_avatar, external_link_expires_in: avatar_link_expires_in) + respond_with_attachment(local_avatar, cache_seconds: cache_seconds) elsif avatar_manager.gravatar_enabled? + set_cache_headers!(cache_seconds) + redirect build_gravatar_image_url(@user) else status 404 diff --git a/modules/bim/app/controllers/bim/bcf/api/v2_1/viewpoints/api.rb b/modules/bim/app/controllers/bim/bcf/api/v2_1/viewpoints/api.rb index 16f503f5fe3..f03deee6fb5 100644 --- a/modules/bim/app/controllers/bim/bcf/api/v2_1/viewpoints/api.rb +++ b/modules/bim/app/controllers/bim/bcf/api/v2_1/viewpoints/api.rb @@ -28,7 +28,9 @@ # See docs/COPYRIGHT.rdoc for more details. #++ +# rubocop:disable Naming/ClassAndModuleCamelCase module Bim::Bcf::API::V2_1 + # rubocop:enable Naming/ClassAndModuleCamelCase module Viewpoints class API < ::API::OpenProjectAPI # Avoid oj parsing numbers into BigDecimal @@ -79,7 +81,7 @@ module Bim::Bcf::API::V2_1 get do viewpoint = @issue.viewpoints.find_by!(uuid: params[:viewpoint_uuid]) if snapshot = viewpoint.snapshot - respond_with_attachment snapshot + respond_with_attachment snapshot, cache_seconds: 1.year.to_i else raise ActiveRecord::RecordNotFound end diff --git a/modules/bim/spec/requests/api/bcf/v2_1/viewpoints_api_spec.rb b/modules/bim/spec/requests/api/bcf/v2_1/viewpoints_api_spec.rb index 09f0b1dd621..ba4aa13a86f 100644 --- a/modules/bim/spec/requests/api/bcf/v2_1/viewpoints_api_spec.rb +++ b/modules/bim/spec/requests/api/bcf/v2_1/viewpoints_api_spec.rb @@ -200,9 +200,17 @@ describe 'BCF 2.1 viewpoints resource', type: :request, content_type: :json, wit get path end - it 'responds with the attachment' do + it 'responds with the attachment with the appropriate content type and cache headers' do expect(subject.status).to eq 200 expect(subject.headers['Content-Type']).to eq 'image/jpeg' + + expect(subject.headers["Cache-Control"]).to eq "public, max-age=#{1.year.to_i}" + expect(subject.headers["Expires"]).to be_present + + expires_time = Time.parse response.headers["Expires"] + + expect(expires_time < Time.now.utc + 1.year.to_i).to be_truthy + expect(expires_time > Time.now.utc + 1.year.to_i - 60).to be_truthy end end diff --git a/spec/requests/api/v3/attachments/attachment_resource_shared_examples.rb b/spec/requests/api/v3/attachments/attachment_resource_shared_examples.rb index b691885b1d0..2827cb1794c 100644 --- a/spec/requests/api/v3/attachments/attachment_resource_shared_examples.rb +++ b/spec/requests/api/v3/attachments/attachment_resource_shared_examples.rb @@ -270,12 +270,20 @@ shared_examples 'an APIv3 attachment resource', type: :request, content_type: :j expect(subject.status).to eq 200 end - it 'has the necessary headers' do + it 'has the necessary headers for content and caching' do expect(subject.headers['Content-Disposition']) .to eql content_disposition expect(subject.headers['Content-Type']) .to eql mock_file.content_type + + expect(subject.headers["Cache-Control"]).to eq "public, max-age=#{1.year.to_i}" + expect(subject.headers["Expires"]).to be_present + + expires_time = Time.parse response.headers["Expires"] + + expect(expires_time < Time.now.utc + 1.year.to_i).to be_truthy + expect(expires_time > Time.now.utc + 1.year.to_i - 60).to be_truthy end it 'sends the file in binary' do @@ -323,6 +331,14 @@ shared_examples 'an APIv3 attachment resource', type: :request, content_type: :j expect(subject.status).to eq 302 expect(subject.headers['Location']) .to eql external_url + + expect(subject.headers["Cache-Control"]).to eq "public, max-age=#{1.year.to_i}" + expect(subject.headers["Expires"]).to be_present + + expires_time = Time.parse response.headers["Expires"] + + expect(expires_time < Time.now.utc + 1.year.to_i).to be_truthy + expect(expires_time > Time.now.utc + 1.year.to_i - 60).to be_truthy end end end