From f55dd193f9a62623054dba1537d01bd7f5cd32f3 Mon Sep 17 00:00:00 2001 From: ThibG Date: Wed, 22 Jul 2020 11:44:02 +0200 Subject: [PATCH] Fix RSS feeds not being cachable (#14368) * Add tests for some cachable responses This only covers responses that we should have managed to make cachable so far. It's not the case of all responses that should be cachable in the end. * Fix RSS feeds not being cachable --- app/controllers/application_controller.rb | 2 +- spec/controllers/accounts_controller_spec.rb | 31 ++++++++++++------- .../collections_controller_spec.rb | 23 ++++++++++---- .../activitypub/outboxes_controller_spec.rb | 23 ++++++++++---- .../activitypub/replies_controller_spec.rb | 23 ++++++++++---- spec/controllers/statuses_controller_spec.rb | 23 ++++++++++---- 6 files changed, 88 insertions(+), 37 deletions(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 973db6aca9765..2201e463e68d0 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -55,7 +55,7 @@ class ApplicationController < ActionController::Base end def store_current_location - store_location_for(:user, request.url) unless request.format == :json + store_location_for(:user, request.url) unless [:json, :rss].include?(request.format&.to_sym) end def require_admin! diff --git a/spec/controllers/accounts_controller_spec.rb b/spec/controllers/accounts_controller_spec.rb index bd36f5494074c..93bf2c83f4076 100644 --- a/spec/controllers/accounts_controller_spec.rb +++ b/spec/controllers/accounts_controller_spec.rb @@ -5,6 +5,21 @@ RSpec.describe AccountsController, type: :controller do let(:account) { Fabricate(:user).account } + shared_examples 'cachable response' do + it 'does not set cookies' do + expect(response.cookies).to be_empty + expect(response.headers['Set-Cookies']).to be nil + end + + it 'does not set sessions' do + expect(session).to be_empty + end + + it 'returns public Cache-Control header' do + expect(response.headers['Cache-Control']).to include 'public' + end + end + describe 'GET #show' do let(:format) { 'html' } @@ -323,9 +338,7 @@ RSpec.describe AccountsController, type: :controller do expect(response.content_type).to eq 'application/activity+json' end - it 'returns public Cache-Control header' do - expect(response.headers['Cache-Control']).to include 'public' - end + it_behaves_like 'cachable response' it 'renders account' do json = body_as_json @@ -343,9 +356,7 @@ RSpec.describe AccountsController, type: :controller do expect(response.content_type).to eq 'application/activity+json' end - it 'returns public Cache-Control header' do - expect(response.headers['Cache-Control']).to include 'public' - end + it_behaves_like 'cachable response' it 'returns Vary header with Signature' do expect(response.headers['Vary']).to include 'Signature' @@ -401,9 +412,7 @@ RSpec.describe AccountsController, type: :controller do expect(response.content_type).to eq 'application/activity+json' end - it 'returns public Cache-Control header' do - expect(response.headers['Cache-Control']).to include 'public' - end + it_behaves_like 'cachable response' it 'renders account' do json = body_as_json @@ -447,9 +456,7 @@ RSpec.describe AccountsController, type: :controller do expect(response).to have_http_status(200) end - it 'returns public Cache-Control header' do - expect(response.headers['Cache-Control']).to include 'public' - end + it_behaves_like 'cachable response' end context do diff --git a/spec/controllers/activitypub/collections_controller_spec.rb b/spec/controllers/activitypub/collections_controller_spec.rb index 56be49be3d7c7..89939d1d25835 100644 --- a/spec/controllers/activitypub/collections_controller_spec.rb +++ b/spec/controllers/activitypub/collections_controller_spec.rb @@ -6,6 +6,21 @@ RSpec.describe ActivityPub::CollectionsController, type: :controller do let!(:account) { Fabricate(:account) } let(:remote_account) { nil } + shared_examples 'cachable response' do + it 'does not set cookies' do + expect(response.cookies).to be_empty + expect(response.headers['Set-Cookies']).to be nil + end + + it 'does not set sessions' do + expect(session).to be_empty + end + + it 'returns public Cache-Control header' do + expect(response.headers['Cache-Control']).to include 'public' + end + end + before do allow(controller).to receive(:signed_request_account).and_return(remote_account) @@ -31,9 +46,7 @@ RSpec.describe ActivityPub::CollectionsController, type: :controller do expect(response.content_type).to eq 'application/activity+json' end - it 'returns public Cache-Control header' do - expect(response.headers['Cache-Control']).to include 'public' - end + it_behaves_like 'cachable response' it 'returns orderedItems with pinned statuses' do json = body_as_json @@ -58,9 +71,7 @@ RSpec.describe ActivityPub::CollectionsController, type: :controller do expect(response.content_type).to eq 'application/activity+json' end - it 'returns public Cache-Control header' do - expect(response.headers['Cache-Control']).to include 'public' - end + it_behaves_like 'cachable response' it 'returns orderedItems with pinned statuses' do json = body_as_json diff --git a/spec/controllers/activitypub/outboxes_controller_spec.rb b/spec/controllers/activitypub/outboxes_controller_spec.rb index 03490533dd592..1baf5a62300b3 100644 --- a/spec/controllers/activitypub/outboxes_controller_spec.rb +++ b/spec/controllers/activitypub/outboxes_controller_spec.rb @@ -3,6 +3,21 @@ require 'rails_helper' RSpec.describe ActivityPub::OutboxesController, type: :controller do let!(:account) { Fabricate(:account) } + shared_examples 'cachable response' do + it 'does not set cookies' do + expect(response.cookies).to be_empty + expect(response.headers['Set-Cookies']).to be nil + end + + it 'does not set sessions' do + expect(session).to be_empty + end + + it 'returns public Cache-Control header' do + expect(response.headers['Cache-Control']).to include 'public' + end + end + before do Fabricate(:status, account: account, visibility: :public) Fabricate(:status, account: account, visibility: :unlisted) @@ -39,9 +54,7 @@ RSpec.describe ActivityPub::OutboxesController, type: :controller do expect(json[:totalItems]).to eq 4 end - it 'returns public Cache-Control header' do - expect(response.headers['Cache-Control']).to include 'public' - end + it_behaves_like 'cachable response' end context 'with page requested' do @@ -62,9 +75,7 @@ RSpec.describe ActivityPub::OutboxesController, type: :controller do expect(json[:orderedItems].all? { |item| item[:to].include?(ActivityPub::TagManager::COLLECTIONS[:public]) || item[:cc].include?(ActivityPub::TagManager::COLLECTIONS[:public]) }).to be true end - it 'returns public Cache-Control header' do - expect(response.headers['Cache-Control']).to include 'public' - end + it_behaves_like 'cachable response' end end diff --git a/spec/controllers/activitypub/replies_controller_spec.rb b/spec/controllers/activitypub/replies_controller_spec.rb index d956e1b35f0e0..ed383864dd1fe 100644 --- a/spec/controllers/activitypub/replies_controller_spec.rb +++ b/spec/controllers/activitypub/replies_controller_spec.rb @@ -7,6 +7,21 @@ RSpec.describe ActivityPub::RepliesController, type: :controller do let(:remote_reply_id) { nil } let(:remote_account) { nil } + shared_examples 'cachable response' do + it 'does not set cookies' do + expect(response.cookies).to be_empty + expect(response.headers['Set-Cookies']).to be nil + end + + it 'does not set sessions' do + expect(session).to be_empty + end + + it 'returns public Cache-Control header' do + expect(response.headers['Cache-Control']).to include 'public' + end + end + before do allow(controller).to receive(:signed_request_account).and_return(remote_account) @@ -36,9 +51,7 @@ RSpec.describe ActivityPub::RepliesController, type: :controller do expect(response.content_type).to eq 'application/activity+json' end - it 'returns public Cache-Control header' do - expect(response.headers['Cache-Control']).to include 'public' - end + it_behaves_like 'cachable response' it 'returns items with account\'s own replies' do json = body_as_json @@ -87,9 +100,7 @@ RSpec.describe ActivityPub::RepliesController, type: :controller do expect(response.content_type).to eq 'application/activity+json' end - it 'returns public Cache-Control header' do - expect(response.headers['Cache-Control']).to include 'public' - end + it_behaves_like 'cachable response' context 'without only_other_accounts' do it 'returns items with account\'s own replies' do diff --git a/spec/controllers/statuses_controller_spec.rb b/spec/controllers/statuses_controller_spec.rb index ba1f1370a631e..cd6e1e6074b3d 100644 --- a/spec/controllers/statuses_controller_spec.rb +++ b/spec/controllers/statuses_controller_spec.rb @@ -5,6 +5,21 @@ require 'rails_helper' describe StatusesController do render_views + shared_examples 'cachable response' do + it 'does not set cookies' do + expect(response.cookies).to be_empty + expect(response.headers['Set-Cookies']).to be nil + end + + it 'does not set sessions' do + expect(session).to be_empty + end + + it 'returns public Cache-Control header' do + expect(response.headers['Cache-Control']).to include 'public' + end + end + describe 'GET #show' do let(:account) { Fabricate(:account) } let(:status) { Fabricate(:status, account: account) } @@ -80,9 +95,7 @@ describe StatusesController do expect(response.headers['Vary']).to eq 'Accept' end - it 'returns public Cache-Control header' do - expect(response.headers['Cache-Control']).to include 'public' - end + it_behaves_like 'cachable response' it 'returns Content-Type header' do expect(response.headers['Content-Type']).to include 'application/activity+json' @@ -470,9 +483,7 @@ describe StatusesController do expect(response.headers['Vary']).to eq 'Accept' end - it 'returns public Cache-Control header' do - expect(response.headers['Cache-Control']).to include 'public' - end + it_behaves_like 'cachable response' it 'returns Content-Type header' do expect(response.headers['Content-Type']).to include 'application/activity+json'