Raise Exception if letter PDF not in S3

Previously, the function would just return a presumed filename. Now that
it actually checks s3, if the file doesn't exist it'll raise an
exception. By default that's a StopIteration at the end of the bucket
iterator, which isn't ideal as this will get supressed if the function
is called within a generator loop further up or anything.

There are a couple of places where we expect the file may not exist, so
we define a custom exception to rescue specifically here. I did consider
subclassing boto's ClientError, but this wasn't straightforward as the
constructor expects to know the operation that failed, which for me is a
signal that it's not an appropriate (re-)use of the class.

app/celery/letters_pdf_tasks.py

@@ -26,6 +26,7 @@ from app.dao.templates_dao import dao_get_template_by_id
 from app.errors import VirusScanError
 from app.exceptions import NotificationTechnicalFailureException
 from app.letters.utils import (
+    LetterPDFNotFound,
     ScanErrorType,
     find_letter_pdf_filename,
     generate_letter_pdf_filename,
@@ -243,7 +244,7 @@ def get_key_and_size_of_letters_to_be_sent_to_print(print_run_deadline, postage)
                 "Size": letter_head['ContentLength'],
                 "ServiceId": str(letter.service_id)
             }
-        except BotoClientError as e:
+        except (BotoClientError, LetterPDFNotFound) as e:
             current_app.logger.exception(
                 f"Error getting letter from bucket for notification: {letter.id} with reference: {letter.reference}", e)
 

app/dao/notifications_dao.py

@@ -27,7 +27,7 @@ from app.clients.sms.firetext import (
     get_message_status_and_reason_from_firetext_code,
 )
 from app.dao.dao_utils import transactional
-from app.letters.utils import find_letter_pdf_filename
+from app.letters.utils import LetterPDFNotFound, find_letter_pdf_filename
 from app.models import (
     EMAIL_TYPE,
     KEY_TYPE_NORMAL,
@@ -453,7 +453,13 @@ def _delete_letters_from_s3(
         Notification.status.in_(NOTIFICATION_STATUS_TYPES_COMPLETED)
     ).limit(query_limit).all()
     for letter in letters_to_delete_from_s3:
-        prefix = find_letter_pdf_filename(letter)
+        try:
+            prefix = find_letter_pdf_filename(letter)
+        except LetterPDFNotFound:
+            current_app.logger.exception(
+                "Could not delete S3 object for letter: {}".format(letter.id))
+            continue
+
         s3_objects = get_s3_bucket_objects(bucket_name=bucket_name, subfolder=prefix)
         for s3_object in s3_objects:
             try:

app/letters/utils.py

@@ -37,6 +37,10 @@ def get_folder_name(created_at):
     return '{}/'.format(print_datetime.date())
 
 
+class LetterPDFNotFound(Exception):
+    pass
+
+
 def find_letter_pdf_filename(notification):
     """
     Retrieve the filename of a letter from s3 by searching for it based on a prefix.
@@ -47,7 +51,10 @@ def find_letter_pdf_filename(notification):
 
     s3 = boto3.resource('s3')
     bucket = s3.Bucket(bucket_name)
-    item = next(x for x in bucket.objects.filter(Prefix=prefix))
+    try:
+        item = next(x for x in bucket.objects.filter(Prefix=prefix))
+    except StopIteration:
+        raise LetterPDFNotFound(f'File not found in bucket {bucket_name} with prefix {prefix}', )
     return item.key
 
 

tests/app/letters/test_letter_utils.py

@@ -8,6 +8,7 @@ from freezegun import freeze_time
 from moto import mock_s3
 
 from app.letters.utils import (
+    LetterPDFNotFound,
     ScanErrorType,
     find_letter_pdf_filename,
     generate_letter_pdf_filename,
@@ -62,6 +63,19 @@ def test_find_letter_pdf_filename_returns_filename(sample_notification):
     assert find_letter_pdf_filename(sample_notification) == f'{prefix}-and-then-some'
 
 
+@mock_s3
+def test_find_letter_pdf_filename_raises_if_not_found(sample_notification):
+    bucket_name = current_app.config['LETTERS_PDF_BUCKET_NAME']
+    s3 = boto3.client('s3', region_name='eu-west-1')
+    s3.create_bucket(
+        Bucket=bucket_name,
+        CreateBucketConfiguration={'LocationConstraint': 'eu-west-1'}
+    )
+
+    with pytest.raises(LetterPDFNotFound):
+        find_letter_pdf_filename(sample_notification)
+
+
 @pytest.mark.parametrize('created_at,folder', [
     (datetime(2017, 1, 1, 17, 29), '2017-01-01'),
     (datetime(2017, 1, 1, 17, 31), '2017-01-02'),