From 7e6db46051135c68f03763a52e39621999670772 Mon Sep 17 00:00:00 2001 From: Graeme Porteous Date: Tue, 24 Oct 2023 20:41:33 +0100 Subject: [PATCH] Update "show as HTML" attachment controller action This updates `AttachmentController#show_as_html` to do attachment masking in the background. Our "view attachments in the browser" feature, or "view as HTML", is doing a lot of processing inline in the web processes and this sometimes causes memory issues on the server and results in oom-kill running. Uses the same logic as the `#show` action to check an attachment is masked before proceeding with the action. This means it will time outs after 5 seconds and then redirect the `AttachmentMaskController` to wait for the job to complete before rendering a view saying the attachment is now viewable. Fixes #7977 --- .../attachment_masks_controller.rb | 6 ++ app/controllers/attachments_controller.rb | 2 +- .../attachment_masks_controller_spec.rb | 10 +++ .../attachments_controller_spec.rb | 63 +++++++++++++++++++ spec/integration/incoming_mail_spec.rb | 14 ++++- 5 files changed, 92 insertions(+), 3 deletions(-) diff --git a/app/controllers/attachment_masks_controller.rb b/app/controllers/attachment_masks_controller.rb index f5e349fa56..95801ceedf 100644 --- a/app/controllers/attachment_masks_controller.rb +++ b/app/controllers/attachment_masks_controller.rb @@ -9,6 +9,8 @@ class AttachmentMasksController < ApplicationController def wait if @attachment.masked? + redirect_to @referer and return if refered_from_show_as_html? + redirect_to done_attachment_mask_path( id: @attachment.to_signed_global_id, referer: verifier.generate(@referer) @@ -54,4 +56,8 @@ def ensure_attachment def verifier Rails.application.message_verifier('AttachmentsController') end + + def refered_from_show_as_html? + @referer =~ %r(/request/\d+/response/\d+/attach/html/) + end end diff --git a/app/controllers/attachments_controller.rb b/app/controllers/attachments_controller.rb index 79c3be0f85..a414aae17d 100644 --- a/app/controllers/attachments_controller.rb +++ b/app/controllers/attachments_controller.rb @@ -16,7 +16,7 @@ class AttachmentsController < ApplicationController before_action :authenticate_attachment before_action :authenticate_attachment_as_html, only: :show_as_html - around_action :ensure_masked, only: :show + around_action :ensure_masked around_action :cache_attachments, only: :show_as_html def show diff --git a/spec/controllers/attachment_masks_controller_spec.rb b/spec/controllers/attachment_masks_controller_spec.rb index 297e7997bb..5d6b759671 100644 --- a/spec/controllers/attachment_masks_controller_spec.rb +++ b/spec/controllers/attachment_masks_controller_spec.rb @@ -30,6 +30,16 @@ def wait end end + context 'when attachment is masked and refered from show as HTML action' do + it 'redirects to referer' do + allow(controller).to receive(:refered_from_show_as_html?). + and_return(true) + allow(attachment).to receive(:to_signed_global_id).and_return('ABC') + wait + expect(response).to redirect_to('/referer') + end + end + context 'when attachment is unmasked' do let(:attachment) { FactoryBot.build(:body_text, :unmasked, id: 1) } diff --git a/spec/controllers/attachments_controller_spec.rb b/spec/controllers/attachments_controller_spec.rb index cc6cad6093..d64ef2c5ce 100644 --- a/spec/controllers/attachments_controller_spec.rb +++ b/spec/controllers/attachments_controller_spec.rb @@ -450,6 +450,69 @@ def show_as_html(params = {}) .to raise_error(ActiveRecord::RecordNotFound) end + context 'when attachment has not been masked' do + let(:attachment) do + FactoryBot.create( + :body_text, :unmasked, + incoming_message: message, + prominence: attachment_prominence + ) + end + + context 'when masked attachment is available before timing out' do + before do + allow(IncomingMessage).to receive( + :get_attachment_by_url_part_number_and_filename! + ).and_return(attachment) + allow(attachment).to receive(:masked?).and_return(false, true) + end + + it 'queues FoiAttachmentMaskJob' do + expect(FoiAttachmentMaskJob).to receive(:perform_later). + with(attachment) + show_as_html + end + + it 'redirects to show action' do + show_as_html + expect(response).to redirect_to(request.fullpath) + end + end + + context 'when response times out waiting for masked attachment' do + before do + allow(Timeout).to receive(:timeout).and_raise(Timeout::Error) + end + + it 'queues FoiAttachmentMaskJob' do + expect(FoiAttachmentMaskJob).to receive(:perform_later). + with(attachment) + show_as_html + end + + it 'redirects to wait for attachment mask route' do + allow_any_instance_of(FoiAttachment).to receive(:to_signed_global_id). + and_return('ABC') + + verifier = double('ActiveSupport::MessageVerifier') + allow(controller).to receive(:verifier).and_return(verifier) + allow(verifier).to receive(:generate).with( + get_attachment_as_html_path( + incoming_message_id: attachment.incoming_message_id, + id: info_request.id, + part: attachment.url_part_number, + file_name: attachment.filename + ) + ).and_return('DEF') + + show_as_html + expect(response).to redirect_to( + wait_for_attachment_mask_path('ABC', referer: 'DEF') + ) + end + end + end + context 'when request is embargoed' do let(:info_request) { FactoryBot.create(:embargoed_request) } diff --git a/spec/integration/incoming_mail_spec.rb b/spec/integration/incoming_mail_spec.rb index 9ec7fa9d82..2635af2729 100644 --- a/spec/integration/incoming_mail_spec.rb +++ b/spec/integration/incoming_mail_spec.rb @@ -64,12 +64,17 @@ it "generates a valid HTML verson of plain text attachments" do receive_incoming_mail('incoming-request-two-same-name.email', email_to: info_request.incoming_email) - visit get_attachment_as_html_path( + attachment_path = get_attachment_as_html_path( incoming_message_id: info_request.incoming_messages.first.id, id: info_request.id, part: 2, file_name: 'hello world.txt.html', skip_cache: 1) + + visit attachment_path + perform_enqueued_jobs + + visit attachment_path expect(page.response_headers['Content-Type']).to eq("text/html; charset=utf-8") expect(page).to have_content "Second hello" end @@ -77,12 +82,17 @@ it "generates a valid HTML verson of PDF attachments" do receive_incoming_mail('incoming-request-pdf-attachment.email', email_to: info_request.incoming_email) - visit get_attachment_as_html_path( + attachment_path = get_attachment_as_html_path( incoming_message_id: info_request.incoming_messages.first.id, id: info_request.id, part: 2, file_name: 'fs 50379341.pdf.html', skip_cache: 1) + + visit attachment_path + perform_enqueued_jobs + + visit attachment_path expect(page.response_headers['Content-Type']).to eq("text/html; charset=utf-8") expect(page).to have_content "Walberswick Parish Council" end