handle document download errors properly

if doc download returns a 403, that's a screw-up on our side. it's not
helpful to a notify user for that to be passed on. the only thing they
should care about is if it's a 400, because they uploaded a filetype we
don't allow.

Everything else should return 500 internal server error.

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']

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"}'