From cb4846c579e5d3d745b86a6e5939dda89b47bf39 Mon Sep 17 00:00:00 2001 From: Matthew Somerville Date: Thu, 7 Sep 2023 19:49:53 +0100 Subject: [PATCH] Add ability to ask external caches to invalidate. Add a cached_urls method to the Comment, FoiAttachment, InfoRequest, PublicBody and User models, containing the entries that those models wish to invalidate either when an event is logged, via log_event, or when the censor rules change, or when a body or user is updated. The entries are fixed paths to be purged, or regexes beginning with ^ to be banned. Adds a NotifyCacheJob that for each of the object's cached_urls, for each locale, for each host in VARNISH_HOSTS, makes BAN or PURGE HTTP requests to the host, to invalidate the relevant entries. --- app/jobs/notify_cache_job.rb | 68 +++++++++++ app/models/censor_rule.rb | 1 + app/models/comment.rb | 7 ++ app/models/foi_attachment.rb | 8 ++ app/models/info_request.rb | 29 +++++ app/models/info_request_event.rb | 16 +++ app/models/public_body.rb | 16 ++- app/models/user.rb | 16 ++- config/general.yml-example | 14 +++ lib/configuration.rb | 1 + .../admin/foi_attachments_controller_spec.rb | 1 + .../admin_incoming_message_controller_spec.rb | 3 + spec/factories/info_request_events.rb | 5 + spec/jobs/notify_cache_job_spec.rb | 111 ++++++++++++++++++ spec/models/censor_rule_spec.rb | 2 +- spec/models/comment_spec.rb | 10 ++ spec/models/foi_attachment_spec.rb | 10 ++ spec/models/info_request_event_spec.rb | 46 ++++++++ spec/models/info_request_spec.rb | 20 ++++ spec/models/public_body_spec.rb | 14 +++ spec/models/user_spec.rb | 12 ++ 21 files changed, 407 insertions(+), 3 deletions(-) create mode 100644 app/jobs/notify_cache_job.rb create mode 100644 spec/jobs/notify_cache_job_spec.rb diff --git a/app/jobs/notify_cache_job.rb b/app/jobs/notify_cache_job.rb new file mode 100644 index 0000000000..f5d77f173a --- /dev/null +++ b/app/jobs/notify_cache_job.rb @@ -0,0 +1,68 @@ +require 'net/http' + +class Net::HTTP::Purge < Net::HTTP::Get + METHOD = 'PURGE' +end + +class Net::HTTP::Ban < Net::HTTP::Get + METHOD = 'BAN' +end + +## +# Job to notify a cache of URLs to be purged or banned, given an object +# (that must have a cached_urls method). +# +# Examples: +# NotifyCacheJob.perform(InfoRequest.first) +# NotifyCacheJob.perform(FoiAttachment.first) +# NotifyCacheJob.perform(Comment.first) +# +class NotifyCacheJob < ApplicationJob + queue_as :default + + around_enqueue do |_, block| + block.call if AlaveteliConfiguration.varnish_hosts.present? + end + + def perform(object) + urls = object.cached_urls + locales = [''] + AlaveteliLocalization.available_locales.map { "/#{_1}" } + hosts = AlaveteliConfiguration.varnish_hosts + + urls.product(locales, hosts).each do |url, locale, host| + if url.start_with? '^' + request = Net::HTTP::Ban.new('/') + request['X-Invalidate-Pattern'] = '^' + locale + url[1..-1] + else + request = Net::HTTP::Purge.new(locale + url) + end + + response = connection_for_host(host).request(request) + log_result = "#{request.method} #{url} at #{host}: #{response.code}" + + case response + when Net::HTTPSuccess + Rails.logger.debug('NotifyCacheJob: ' + log_result) + else + Rails.logger.warn('NotifyCacheJob: Unable to ' + log_result) + end + end + + ensure + close_connections + end + + def connections + @connections ||= {} + end + + def connection_for_host(host) + connections[host] ||= Net::HTTP.start( + AlaveteliConfiguration.domain, 80, host, 6081 + ) + end + + def close_connections + connections.values.each { _1.finish if _1.started? } + end +end diff --git a/app/models/censor_rule.rb b/app/models/censor_rule.rb index a3704e9b6c..5a40589cc4 100644 --- a/app/models/censor_rule.rb +++ b/app/models/censor_rule.rb @@ -75,6 +75,7 @@ def is_global? def expire_requests if info_request InfoRequestExpireJob.perform_later(info_request) + NotifyCacheJob.perform_later(info_request) elsif user InfoRequestExpireJob.perform_later(user, :info_requests) elsif public_body diff --git a/app/models/comment.rb b/app/models/comment.rb index 07b295d271..747a66a7df 100644 --- a/app/models/comment.rb +++ b/app/models/comment.rb @@ -193,6 +193,13 @@ def hide(editor:) end end + def cached_urls + [ + request_path(info_request), + show_user_wall_path(url_name: user.url_name) + ] + end + private def check_body_has_content diff --git a/app/models/foi_attachment.rb b/app/models/foi_attachment.rb index 0ed0ae0229..104f21c5d3 100644 --- a/app/models/foi_attachment.rb +++ b/app/models/foi_attachment.rb @@ -29,6 +29,8 @@ require 'digest' class FoiAttachment < ApplicationRecord + include Rails.application.routes.url_helpers + include LinkToHelper include MessageProminence MissingAttachment = Class.new(StandardError) @@ -318,6 +320,12 @@ def body_as_html(dir, opts = {}) AttachmentToHTML.to_html(self, to_html_opts) end + def cached_urls + [ + request_path(incoming_message.info_request) + ] + end + private def text_type? diff --git a/app/models/info_request.rb b/app/models/info_request.rb index 73b3b1039e..2175f27dbe 100644 --- a/app/models/info_request.rb +++ b/app/models/info_request.rb @@ -52,6 +52,7 @@ class InfoRequest < ApplicationRecord include InfoRequest::TitleValidation include Taggable include Notable + include LinkToHelper admin_columns exclude: %i[title url_title], include: %i[rejected_incoming_count] @@ -1758,6 +1759,34 @@ def latest_refusals incoming_messages.select(&:refusals?).last&.refusals || [] end + def cached_urls + feed_request = TrackThing.new( + info_request: self, + track_type: 'request_updates' + ) + feed_body = TrackThing.new( + public_body: public_body, + track_type: 'public_body_updates' + ) + feed_user = TrackThing.new( + tracked_user: user, + track_type: 'user_updates' + ) + [ + '/', + public_body_path(public_body), + request_path(self), + request_details_path(self), + '^/list', + do_track_path(feed_request, feed = 'feed'), + '^/feed/list/', + do_track_path(feed_body, feed = 'feed'), + do_track_path(feed_user, feed = 'feed'), + user_path(user), + show_user_wall_path(url_name: user.url_name) + ] + end + private def self.add_conditions_from_extra_params(params, extra_params) diff --git a/app/models/info_request_event.rb b/app/models/info_request_event.rb index 31a56015d2..64a2cde8be 100644 --- a/app/models/info_request_event.rb +++ b/app/models/info_request_event.rb @@ -88,6 +88,7 @@ class InfoRequestEvent < ApplicationRecord self.event_type = "hide" end after_create :update_request, if: :response? + after_create :invalidate_cached_pages after_commit -> { info_request.create_or_update_request_summary }, on: [:create] @@ -355,6 +356,16 @@ def update_request info_request.update_last_public_response_at end + def invalidate_cached_pages + if comment + NotifyCacheJob.perform_later(comment) + elsif foi_attachment + NotifyCacheJob.perform_later(foi_attachment) + else + NotifyCacheJob.perform_later(info_request) + end + end + def same_email_as_previous_send? prev_addr = info_request.get_previous_email_sent_to(self) curr_addr = params[:email] @@ -406,6 +417,11 @@ def set_calculated_state!(state) end end + def foi_attachment + return unless params[:attachment_id] + @foi_attachment ||= FoiAttachment.find(params[:attachment_id]) + end + protected def variety diff --git a/app/models/public_body.rb b/app/models/public_body.rb index 65ef2c552c..e711b5265f 100644 --- a/app/models/public_body.rb +++ b/app/models/public_body.rb @@ -35,6 +35,8 @@ class PublicBody < ApplicationRecord include Taggable include Notable + include Rails.application.routes.url_helpers + include LinkToHelper class ImportCSVDryRun < StandardError; end @@ -114,7 +116,7 @@ def self.admin_title after_save :update_missing_email_tag - after_update :reindex_requested_from + after_update :reindex_requested_from, :invalidate_cached_pages # Every public body except for the internal admin one is visible scope :visible, -> { where("public_bodies.id <> #{ PublicBody.internal_admin_body.id }") } @@ -911,6 +913,14 @@ def questions PublicBodyQuestion.fetch(self) end + def cached_urls + [ + public_body_path(self), + list_public_bodies_path, + '^/body/list' + ] + end + private # If the url_name has changed, then all requested_from: queries will break @@ -919,6 +929,10 @@ def reindex_requested_from expire_requests if saved_change_to_attribute?(:url_name) end + def invalidate_cached_pages + NotifyCacheJob.perform_later(self) + end + # Read an attribute value (without using locale fallbacks if the # attribute is translated) def read_attribute_value(name, locale) diff --git a/app/models/user.rb b/app/models/user.rb index c33aae63d4..526b893cb4 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -47,6 +47,8 @@ class User < ApplicationRecord include User::OneTimePassword include User::Slug include User::Survey + include Rails.application.routes.url_helpers + include LinkToHelper DEFAULT_CONTENT_LIMITS = { info_requests: AlaveteliConfiguration.max_requests_per_user_per_day, @@ -179,7 +181,9 @@ class User < ApplicationRecord validate :email_and_name_are_valid after_initialize :set_defaults - after_update :reindex_referencing_models, :update_pro_account + after_update :reindex_referencing_models, + :update_pro_account, + :invalidate_cached_pages acts_as_xapian texts: [:name, :about_me], values: [ @@ -341,6 +345,10 @@ def expire_comments comments.find_each(&:reindex_request_events) end + def invalidate_cached_pages + NotifyCacheJob.perform_later(self) + end + def locale (super || AlaveteliLocalization.locale).to_s end @@ -673,6 +681,12 @@ def flipper_id "User;#{id}" end + def cached_urls + [ + user_path(self) + ] + end + private def set_defaults diff --git a/config/general.yml-example b/config/general.yml-example index f577b0c71c..89af7a927c 100644 --- a/config/general.yml-example +++ b/config/general.yml-example @@ -627,6 +627,20 @@ EXCEPTION_NOTIFICATIONS_TO: # --- MAX_REQUESTS_PER_USER_PER_DAY: 6 +# If you're running behind Varnish set this to work out where to send purge +# requests. Otherwise, don't set it. +# +# VARNISH_HOSTS - Array of Strings (default: nil) +# +# Examples: +# +# VARNISH_HOSTS: +# - host1 +# - host2 +# +# --- +VARNISH_HOSTS: null + # Adding a value here will enable Google Analytics on all non-admin pages for # non-admin users. # diff --git a/lib/configuration.rb b/lib/configuration.rb index ad1d2845db..926da9169a 100644 --- a/lib/configuration.rb +++ b/lib/configuration.rb @@ -131,6 +131,7 @@ module AlaveteliConfiguration USE_MAILCATCHER_IN_DEVELOPMENT: true, USER_SIGN_IN_ACTIVITY_RETENTION_DAYS: 0, UTILITY_SEARCH_PATH: ['/usr/bin', '/usr/local/bin'], + VARNISH_HOSTS: [], WORKING_OR_CALENDAR_DAYS: 'working' } # rubocop:enable Layout/LineLength diff --git a/spec/controllers/admin/foi_attachments_controller_spec.rb b/spec/controllers/admin/foi_attachments_controller_spec.rb index d37c2a5437..983c9f42ec 100644 --- a/spec/controllers/admin/foi_attachments_controller_spec.rb +++ b/spec/controllers/admin/foi_attachments_controller_spec.rb @@ -78,6 +78,7 @@ end it 'should log an "edit_attachment" event on the info_request' do + expect(NotifyCacheJob).to receive(:perform_later).with(attachment) allow(@controller).to receive(:admin_current_user). and_return("Admin user") diff --git a/spec/controllers/admin_incoming_message_controller_spec.rb b/spec/controllers/admin_incoming_message_controller_spec.rb index b2557d6663..d5452060f8 100644 --- a/spec/controllers/admin_incoming_message_controller_spec.rb +++ b/spec/controllers/admin_incoming_message_controller_spec.rb @@ -17,6 +17,7 @@ end it "destroys the ActiveStorage attachment record" do + expect(NotifyCacheJob).to receive(:perform_later).with(@im.info_request) file = @im.raw_email.file expect(file.attached?).to eq true post :destroy, params: { id: @im.id } @@ -217,6 +218,8 @@ def make_request(params=@default_params) it 'should log an "edit_incoming" event on the info_request' do allow(@controller).to receive(:admin_current_user).and_return("Admin user") + expect(NotifyCacheJob).to receive(:perform_later). + with(@incoming.info_request) make_request @incoming.reload last_event = @incoming.info_request_events.last diff --git a/spec/factories/info_request_events.rb b/spec/factories/info_request_events.rb index 3c8a2b839b..a03ee6ed4b 100644 --- a/spec/factories/info_request_events.rb +++ b/spec/factories/info_request_events.rb @@ -187,6 +187,11 @@ end end + factory :foi_attachment_event do + event_type { 'edit_attachment' } + params { { attachment_id: 1 } } + end + end end diff --git a/spec/jobs/notify_cache_job_spec.rb b/spec/jobs/notify_cache_job_spec.rb new file mode 100644 index 0000000000..bdad8dade0 --- /dev/null +++ b/spec/jobs/notify_cache_job_spec.rb @@ -0,0 +1,111 @@ +require 'spec_helper' + +RSpec.describe NotifyCacheJob, type: :job do + let(:args) { [] } + subject(:perform) { described_class.new.perform(*args) } + subject(:enqueue) { described_class.perform_later(*args) } + + let(:request) { FactoryBot.build(:info_request) } + let(:comment) { FactoryBot.build(:comment) } + let(:attachment) { FactoryBot.build(:pdf_attachment) } + let(:im) { FactoryBot.create(:plain_incoming_message) } + let(:body) { FactoryBot.create(:public_body) } + let(:user) { FactoryBot.create(:user) } + + before do + @old_include_default_locale_in_urls = + AlaveteliConfiguration.include_default_locale_in_urls + AlaveteliLocalization.set_default_locale_urls(false) + + allow(AlaveteliConfiguration).to receive(:varnish_hosts). + and_return(['varnish']) + + stub_request(:purge, /^http:\/\/test\.host(\/(en|es|fr|en_GB))?\/(|body|((feed\/)?body|(feed\/|details\/)?request|(feed\/)?user)\/[a-z0-9_]+|user\/[a-z0-9_]+\/wall)$/). + to_return(status: 200, body: "", headers: {}) + stub_request(:ban, 'http://test.host/'). + with(headers: + { + 'X-Invalidate-Pattern' => + /^\^(\/(en|es|fr|en_GB))?\/(list|feed\/list\/|body\/list)$/ + }). + to_return(status: 200, body: "", headers: {}) + end + + after do + AlaveteliLocalization.set_default_locale_urls( + @old_include_default_locale_in_urls + ) + end + + context 'when called with a request' do + let(:args) { [request] } + + it 'calls out to varnish correctly' do + expect(Rails.logger).to receive(:debug).exactly(55).times + perform + end + end + + context 'when called with a comment' do + let(:args) { [comment] } + + it 'calls out to varnish correctly' do + expect(Rails.logger).to receive(:debug).exactly(10).times + perform + end + end + + context 'when called with an attachment' do + let(:args) { [attachment] } + + it 'calls out to varnish correctly' do + attachment.incoming_message = im + expect(Rails.logger).to receive(:debug).exactly(5).times + perform + end + end + + context 'when called with a body' do + let(:args) { [body] } + + it 'calls out to varnish correctly' do + attachment.incoming_message = im + expect(Rails.logger).to receive(:debug).exactly(15).times + perform + end + end + + context 'when called with a user' do + let(:args) { [user] } + + it 'calls out to varnish correctly' do + attachment.incoming_message = im + expect(Rails.logger).to receive(:debug).exactly(5).times + perform + end + end + + context 'with varnish hosts configured' do + let(:args) { [request] } + + before do + allow(request).to receive(:id).and_return(1) + end + + it 'enqueues the job' do + expect(enqueue).to be_a(NotifyCacheJob) + end + end + + context 'without varnish hosts configured' do + let(:args) { [request] } + + before do + allow(AlaveteliConfiguration).to receive(:varnish_hosts).and_return(nil) + end + + it 'does not enqueues the job' do + expect(enqueue).to eq(false) + end + end +end diff --git a/spec/models/censor_rule_spec.rb b/spec/models/censor_rule_spec.rb index 7ca697f288..db0fed29cd 100644 --- a/spec/models/censor_rule_spec.rb +++ b/spec/models/censor_rule_spec.rb @@ -159,12 +159,12 @@ end describe '#expire_requests' do - it 'create expire job for the request if it is a request rule' do request = FactoryBot.create(:info_request) rule = FactoryBot.create(:info_request_censor_rule, info_request: request) expect(InfoRequestExpireJob).to receive(:perform_later).with(request) + expect(NotifyCacheJob).to receive(:perform_later).with(request) rule.expire_requests end diff --git a/spec/models/comment_spec.rb b/spec/models/comment_spec.rb index 608793c437..22ae7a8b96 100644 --- a/spec/models/comment_spec.rb +++ b/spec/models/comment_spec.rb @@ -188,6 +188,15 @@ end # rubocop:enable Layout/FirstArrayElementIndentation + describe '.cached_urls' do + it 'includes the correct paths' do + comment = FactoryBot.create(:comment) + request_path = "/request/" + comment.info_request.url_title + user_wall_path = "/user/" + comment.user.url_name + "/wall" + expect(comment.cached_urls).to eq([request_path, user_wall_path]) + end + end + describe '#prominence' do subject { comment.prominence } @@ -344,6 +353,7 @@ end it 'logs an event on the request' do + expect(NotifyCacheJob).to receive(:perform_later).with(comment) subject event = comment.info_request.last_event expect(event.event_type).to eq('hide_comment') diff --git a/spec/models/foi_attachment_spec.rb b/spec/models/foi_attachment_spec.rb index 14a21f5197..cf0f31dc8e 100644 --- a/spec/models/foi_attachment_spec.rb +++ b/spec/models/foi_attachment_spec.rb @@ -43,6 +43,16 @@ it { is_expected.to match_array(binary_attachments) } end + describe '.cached_urls' do + it 'includes the correct paths' do + att = FactoryBot.create(:jpeg_attachment) + im = FactoryBot.create(:plain_incoming_message) + att.incoming_message = im + request_path = "/request/" + att.incoming_message.info_request.url_title + expect(att.cached_urls).to eq([request_path]) + end + end + describe '#body=' do it "sets the body" do diff --git a/spec/models/info_request_event_spec.rb b/spec/models/info_request_event_spec.rb index 7bcb92bc14..ada3e63411 100644 --- a/spec/models/info_request_event_spec.rb +++ b/spec/models/info_request_event_spec.rb @@ -242,6 +242,39 @@ end + context "the event is for a comment" do + + it "enqueues NotifyCacheJob job for the comment" do + event = FactoryBot.build(:comment_event) + expect(NotifyCacheJob).to receive(:perform_later).with(event.comment) + event.save + end + + end + + context "the event is for an foi_attachment" do + + it "enqueues NotifyCacheJob job for the attachment" do + attachment = mock_model(FoiAttachment) + allow(FoiAttachment).to receive(:find).and_return(attachment) + event = FactoryBot.build(:foi_attachment_event) + expect(NotifyCacheJob).to receive(:perform_later).with(attachment) + event.save + end + + end + + context "the event is for a request" do + + it "enqueues NotifyCacheJob job for the request" do + event = FactoryBot.build(:info_request_event) + expect(NotifyCacheJob).to receive(:perform_later). + with(event.info_request) + event.save + end + + end + it "calls the request's create_or_update_request_summary on create" do info_request = FactoryBot.create(:info_request) event = FactoryBot.build(:info_request_event, info_request: info_request) @@ -879,4 +912,17 @@ end end + describe '#foi_attachment' do + it 'loads FoiAttachment from params as if it was an assoication' do + event = FactoryBot.build( + :info_request_event, params: { attachment_id: 1 } + ) + + attachment = mock_model(FoiAttachment) + allow(FoiAttachment).to receive(:find).with(1).and_return(attachment) + + expect(event.foi_attachment).to eq(attachment) + end + end + end diff --git a/spec/models/info_request_spec.rb b/spec/models/info_request_spec.rb index 5ae655fafd..b5ab265ac7 100644 --- a/spec/models/info_request_spec.rb +++ b/spec/models/info_request_spec.rb @@ -108,6 +108,26 @@ it { is_expected.to eq(4) } end + describe '.cached_urls' do + it 'includes the correct paths' do + request = FactoryBot.create(:info_request) + expect(request.cached_urls). + to eq([ + '/', + '/body/' + request.public_body.url_name, + '/request/' + request.url_title, + '/details/request/' + request.url_title, + '^/list', + '/feed/request/' + request.url_title, + '^/feed/list/', + '/feed/body/' + request.public_body.url_name, + '/feed/user/' + request.user.url_name, + '/user/' + request.user.url_name, + '/user/' + request.user.url_name + '/wall' + ]) + end + end + describe '#foi_attachments' do subject { info_request.foi_attachments } diff --git a/spec/models/public_body_spec.rb b/spec/models/public_body_spec.rb index c6bbf962d2..7a67850632 100644 --- a/spec/models/public_body_spec.rb +++ b/spec/models/public_body_spec.rb @@ -197,6 +197,14 @@ end + describe '.cached_urls' do + it 'includes the correct paths' do + body = FactoryBot.create(:public_body) + body_path = "/body/" + body.url_name + expect(body.cached_urls).to eq([body_path, '/body', '^/body/list']) + end + end + describe '#save' do subject { public_body.save } @@ -1182,6 +1190,12 @@ def set_default_attributes(public_body) expect(@public_body.versions.last.name).to eq('Test') end + it 'triggers cache invalidation when updated' do + body = FactoryBot.create(:public_body) + expect(NotifyCacheJob).to receive(:perform_later).with(body) + body.update!(url_name: 'baz-bar-foo') + end + it 'reindexes request events when url_name has changed' do body = FactoryBot.create(:public_body, name: 'foo-bar-baz') requests = diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index ec086c83f0..4015e312cb 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -376,6 +376,11 @@ def create_user(options = {}) let(:request) { FactoryBot.create(:info_request, user: user) } let(:request_event) { request.reload.last_event } + it 'triggers cache invalidation when updated' do + expect(NotifyCacheJob).to receive(:perform_later).with(user) + user.save! + end + it "should reindex events associated with that user's comments when URL changes" do user.url_name = 'updated_url_name' user.save! @@ -741,6 +746,13 @@ def create_user(options = {}) end end + describe '.cached_urls' do + it 'includes the correct paths' do + user = FactoryBot.create(:user) + expect(user.cached_urls).to eq(["/user/" + user.url_name]) + end + end + describe '#transactions' do it 'returns a TransactionCalculator with the default transaction set' do