Merge pull request #2696 from alphagov/doc-dl-errors

handle document download errors properly

app/clients/document_download.py

@@ -12,13 +12,8 @@ class DocumentDownloadError(Exception):
 
     @classmethod
     def from_exception(cls, e):
-        try:
+        message = e.response.json()['error']
-            message = e.response.json()['error']
+        status_code = e.response.status_code
-            status_code = e.response.status_code
-        except (TypeError, ValueError, AttributeError, KeyError):
-            message = 'connection error'
-            status_code = 503
-
         return cls(message, status_code)
 
 
@@ -45,11 +40,16 @@ class DocumentDownloadClient:
 
             response.raise_for_status()
         except requests.RequestException as e:
-            error = DocumentDownloadError.from_exception(e)
+            # if doc dl responds with a non-400, (eg 403) it's referring to credentials that the API and Doc DL use.
-            current_app.logger.warning(
+            # we don't want to tell users about that, so anything that isn't a 400 (virus scan failed or file type
-                'Document download request failed with error: {}'.format(error.message)
+            # unrecognised) should be raised as a 500 internal server error here.
-            )
+            if e.response.status_code == 400:
-
+                error = DocumentDownloadError.from_exception(e)
-            raise error
+                current_app.logger.info(
+                    'Document download request failed with error: {}'.format(error.message)
+                )
+                raise error
+            else:
+                raise Exception(f'Unhandled document download error: {e.response.text}')
 
         return response.json()['document']['url']

app/v2/notifications/get_notifications.py

@@ -39,7 +39,7 @@ def get_pdf_for_notification(notification_id):
         raise BadRequestError(message="Notification is not a letter")
 
     if notification.status == NOTIFICATION_VIRUS_SCAN_FAILED:
-        raise BadRequestError(message='Document did not pass the virus scan')
+        raise BadRequestError(message='File did not pass the virus scan')
 
     if notification.status == NOTIFICATION_TECHNICAL_FAILURE:
         raise BadRequestError(message='PDF not available for letters in status {}'.format(notification.status))

tests/app/clients/test_document_download.py

@@ -1,4 +1,3 @@
-import requests
 import requests_mock
 import pytest
 
@@ -33,26 +32,27 @@ def test_upload_document(document_download):
     assert resp == 'https://document-download/services/service-id/documents/uploaded-url'
 
 
-def test_should_raise_for_status(document_download):
+def test_should_raise_400s_as_DocumentDownloadErrors(document_download):
     with pytest.raises(DocumentDownloadError) as excinfo, requests_mock.Mocker() as request_mock:
         request_mock.post('https://document-download/services/service-id/documents', json={
-            'error': 'Invalid encoding'
+            'error': 'Invalid mime type'
-        }, status_code=403)
+        }, status_code=400)
 
         document_download.upload_document('service-id', 'abababab')
 
-    assert excinfo.value.message == 'Invalid encoding'
+    assert excinfo.value.message == 'Invalid mime type'
-    assert excinfo.value.status_code == 403
+    assert excinfo.value.status_code == 400
 
 
-def test_should_raise_for_connection_errors(document_download):
+def test_should_raise_non_400_statuses_as_exceptions(document_download):
-    with pytest.raises(DocumentDownloadError) as excinfo, requests_mock.Mocker() as request_mock:
+    with pytest.raises(Exception) as excinfo, requests_mock.Mocker() as request_mock:
         request_mock.post(
             'https://document-download/services/service-id/documents',
-            exc=requests.exceptions.ConnectTimeout
+            json={'error': 'Auth Error Of Some Kind'},
+            status_code=403
         )
 
         document_download.upload_document('service-id', 'abababab')
 
-    assert excinfo.value.message == 'connection error'
+    assert type(excinfo.value) == Exception  # make sure it's a base exception, so will be handled as a 500 by v2 api
-    assert excinfo.value.status_code == 503
+    assert str(excinfo.value) == 'Unhandled document download error: {"error": "Auth Error Of Some Kind"}'

tests/app/db.py

@@ -250,7 +250,7 @@ def create_notification(
         updated_at = updated_at or datetime.utcnow()
 
     if not one_off and (job is None and api_key is None):
-        # we didn't specify in test - lets create it
+        # we did not specify in test - lets create it
         api_key = ApiKey.query.filter(ApiKey.service == template.service, ApiKey.key_type == key_type).first()
         if not api_key:
             api_key = create_api_key(template.service, key_type=key_type)

tests/app/v2/notifications/test_get_notifications.py

@@ -703,7 +703,7 @@ def test_get_pdf_for_notification_returns_400_if_pdf_not_found(
 
 
 @pytest.mark.parametrize('status, expected_message', [
-    ('virus-scan-failed', 'Document did not pass the virus scan'),
+    ('virus-scan-failed', 'File did not pass the virus scan'),
     ('technical-failure', 'PDF not available for letters in status technical-failure'),
 ])
 def test_get_pdf_for_notification_only_returns_pdf_content_if_right_status(