Merge branch '7934-fix-unsaved-attachments' into develop

app/jobs/foi_attachment_mask_job.rb

@@ -16,7 +16,23 @@ class FoiAttachmentMaskJob < ApplicationJob
 
   def perform(attachment)
     @attachment = attachment
+    mask
 
+  rescue FoiAttachment::MissingAttachment
+    incoming_message.parse_raw_email!(true)
+
+    begin
+      attachment.reload
+    rescue ActiveRecord::RecordNotFound
+      @attachment = attachment.load_attachment_from_incoming_message
+    end
+
+    mask
+  end
+
+  private
+
+  def mask
     body = AlaveteliTextMasker.apply_masks(
       attachment.unmasked_body,
       attachment.content_type,
@@ -31,13 +47,8 @@ def perform(attachment)
     end
 
     attachment.update(body: body, masked_at: Time.zone.now)
-
-  rescue FoiAttachment::MissingAttachment
-    incoming_message.parse_raw_email!(true)
   end
 
-  private
-
   def masks
     {
       censor_rules: info_request.applicable_censor_rules,

app/models/foi_attachment.rb

@@ -34,7 +34,6 @@ class FoiAttachment < ApplicationRecord
   include MessageProminence
 
   MissingAttachment = Class.new(StandardError)
-  RebuiltAttachment = Class.new(StandardError)
 
   belongs_to :incoming_message,
              inverse_of: :foi_attachments
@@ -82,33 +81,6 @@ class FoiAttachment < ApplicationRecord
   }.freeze
   # rubocop:enable Style/LineLength
 
-  # Helper method which can wrap calls to #body/#body_as_text/#default_body to
-  # ensure the `RebuiltAttachment` exception is caught. This is useful for when
-  # the body is required inline eg when the search index is being built or the
-  # text of the main attachment part is being cached in the database.
-  #
-  # Need to call this so the attachment is loaded within the block, EG, this
-  # would work:
-  #   protect_against_rebuilt_attachments do
-  #     incoming_message.foi_attachment.last.body
-  #   end
-  #
-  # but this would fail:
-  #   attachment = incoming_message.foi_attachment.last
-  #   protect_against_rebuilt_attachments do
-  #     attachment.body
-  #   end
-  def self.protect_against_rebuilt_attachments(&block)
-    errored = false
-    begin
-      block.call if block_given?
-    rescue RebuiltAttachment => ex
-      raise ex if errored
-      errored = true
-      retry
-    end
-  end
-
   def delete_cached_file!
     @cached_body = nil
     file.purge if file.attached?
@@ -142,7 +114,7 @@ def body
     end
 
   rescue ActiveRecord::RecordNotFound
-    raise RebuiltAttachment, "attachment no longer present in DB (ID=#{id})"
+    load_attachment_from_incoming_message.body
   end
 
   # body as UTF-8 text, with scrubbing of invalid chars if needed
@@ -349,6 +321,14 @@ def cached_urls
     ]
   end
 
+  def load_attachment_from_incoming_message
+    IncomingMessage.get_attachment_by_url_part_number_and_filename!(
+      incoming_message.get_attachments_for_display,
+      url_part_number,
+      display_filename
+    )
+  end
+
   private
 
   def text_type?

app/models/incoming_message.rb

@@ -345,10 +345,8 @@ def get_main_body_text_unfolded
   # Returns body text from main text part of email, converted to UTF-8
   def get_main_body_text_internal
     parse_raw_email!
-    FoiAttachment.protect_against_rebuilt_attachments do
-      main_part = get_main_body_text_part
-      _convert_part_body_to_text(main_part)
-    end
+    main_part = get_main_body_text_part
+    _convert_part_body_to_text(main_part)
   end
 
   # Given a main text part, converts it to text
@@ -547,9 +545,7 @@ def get_body_for_quoting
 
   # Returns text version of attachment text
   def get_attachment_text_full
-    text = FoiAttachment.protect_against_rebuilt_attachments do
-      _get_attachment_text_internal
-    end
+    text = _get_attachment_text_internal
     text = apply_masks(text, 'text/html')
 
     # This can be useful for memory debugging

spec/jobs/foi_attachment_mask_job_spec.rb

@@ -91,11 +91,52 @@ def perform
     expect(attachment.body).to include 'Horse'
   end
 
-  it 'reparses raw email after rescuing FoiAttachment::MissingAttachment' do
-    allow(attachment).to receive(:unmasked_body).and_raise(
-      FoiAttachment::MissingAttachment
-    )
-    expect(incoming_message).to receive(:parse_raw_email!).with(true)
-    perform
+  context 'after rescuing from FoiAttachment::MissingAttachment' do
+    before do
+      # first call to #unmasked_body should raise MissingAttachment exception
+      # subsequent calls should call the original method.
+      @raised = false
+      allow(attachment).to receive(:unmasked_body).
+        and_wrap_original do |original_method, *args, &block|
+          unless @raised
+            @raised = true
+            raise FoiAttachment::MissingAttachment
+          end
+          original_method.call(*args, &block)
+        end
+    end
+
+    it 'parses raw email again' do
+      expect(incoming_message).to receive(:parse_raw_email!).with(true)
+      perform
+    end
+
+    it 'masks the body' do
+      CensorRule.create!(
+        text: 'dull', replacement: 'Orange',
+        last_edit_editor: 'unknown', last_edit_comment: 'none'
+      )
+      perform
+      expect(attachment.body).to include 'Orange'
+    end
+
+    it 'rebuilds the attachment and masks if the hexdigest does not match' do
+      CensorRule.create!(
+        text: 'dull', replacement: 'Banana',
+        last_edit_editor: 'unknown', last_edit_comment: 'none'
+      )
+
+      attachment.update(hexdigest: '123')
+      perform
+
+      new_attachment = IncomingMessage.
+        get_attachment_by_url_part_number_and_filename!(
+          incoming_message.get_attachments_for_display,
+          attachment.url_part_number,
+          attachment.display_filename
+        )
+      expect(new_attachment.unmasked_body).to include 'dull'
+      expect(new_attachment.body).to include 'Banana'
+    end
   end
 end

spec/models/foi_attachment_spec.rb

@@ -165,30 +165,13 @@
           })
       end
 
-      it 'raises rebuilt attachment exception' do
-        expect { foi_attachment.body }.to raise_error(
-          FoiAttachment::RebuiltAttachment
+      it 'returns load_attachment_from_incoming_message.body' do
+        allow(foi_attachment).to(
+          receive(:load_attachment_from_incoming_message).and_return(
+            double(body: 'thisisthenewtext')
+          )
         )
-      end
-
-      context 'and called within protect_against_rebuilt_attachments block' do
-        def body
-          FoiAttachment.protect_against_rebuilt_attachments do
-            # Note, we're not using the `foi_attachment` "let" variable as the
-            # `FoiAttachment#body` code will call `parse_raw_email!(true)` and
-            # recreate the attachments, so the body returned, will belong to a
-            # new `FoiAttachment` instance
-            incoming_message.foi_attachments.last.body
-          end
-        end
-
-        it 'does not raise rebuilt attachment exception' do
-          expect { body }.to_not raise_error
-        end
-
-        it 'returns rebuilt body' do
-          expect(body).to eq('hereisthetext')
-        end
+        expect(foi_attachment.body).to eq('thisisthenewtext')
       end
     end