Skip to content

Commit

Permalink
Update "show as HTML" attachment controller action
Browse files Browse the repository at this point in the history
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
  • Loading branch information
gbp committed Oct 25, 2023
1 parent 68d5dec commit 7e6db46
Show file tree
Hide file tree
Showing 5 changed files with 92 additions and 3 deletions.
6 changes: 6 additions & 0 deletions app/controllers/attachment_masks_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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
2 changes: 1 addition & 1 deletion app/controllers/attachments_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
10 changes: 10 additions & 0 deletions spec/controllers/attachment_masks_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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) }

Expand Down
63 changes: 63 additions & 0 deletions spec/controllers/attachments_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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) }

Expand Down
14 changes: 12 additions & 2 deletions spec/integration/incoming_mail_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -64,25 +64,35 @@
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

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
Expand Down

0 comments on commit 7e6db46

Please sign in to comment.