Merge pull request #75 from JeffreyMFarley/improve-error-handling

Improve error handling

complaint_search/decorators.py

@@ -1,18 +1,31 @@
+import logging
 from rest_framework import status
 from rest_framework.response import Response
 from elasticsearch import TransportError
 
+log = logging.getLogger(__name__)
+
+
 def catch_es_error(function):
     def wrap(request, *args, **kwargs):
         try:
             return function(request, *args, **kwargs)
-        except TransportError as e:
-            status_code = e.status_code if isinstance(e.status_code, int) \
-                else status.HTTP_400_BAD_REQUEST
+        except TransportError as te:
+            log.error(te)
+
+            status_code = 424  # HTTP_424_FAILED_DEPENDENCY
+            res = {
+                "error": 'There was an error calling Elasticsearch'
+            }
+            return Response(res, status=status_code)
+        except Exception as e:
+            log.error(e)
+
+            status_code = status.HTTP_500_INTERNAL_SERVER_ERROR
             res = {
-                "error": 'Elasticsearch error: ' + e.error
+                "error": 'There was a problem retrieving your request'
             }
             return Response(res, status=status_code)
     wrap.__doc__ = function.__doc__
     wrap.__name__ = function.__name__
-    return wrap
+    return wrap

complaint_search/es_interface.py

@@ -162,7 +162,7 @@ def search(agg_exclude=None, **kwargs):
     )
 
     # format
-    res = None
+    res = {}
     format = params.get("format")
     if format == "default":
         if not params.get("no_aggs"):

complaint_search/tests/test_es_interface.py

@@ -375,7 +375,7 @@ def test_search_with_field_all__valid(self, mock_scroll, mock_count, mock_search
     def test_search_with_format__invalid(self, mock_rget, mock_search):
         mock_search.return_value = 'OK'
         res = search(format="pdf")
-        self.assertIsNone(res)
+        self.assertEqual(res, {})
         mock_search.assert_not_called()
         mock_rget.assert_not_called()
 

complaint_search/tests/test_view_suggest_company.py

@@ -63,27 +63,15 @@ def test_suggest_cors_headers(self, mock_essuggest):
         self.assertTrue(response.has_header('Access-Control-Allow-Origin'))
 
     @mock.patch('complaint_search.es_interface.filter_suggest')
-    def test_suggest__transport_error_with_status_code(self, mock_essuggest):
-        mock_essuggest.side_effect = TransportError(
-            status.HTTP_404_NOT_FOUND, "Error"
-        )
-        url = reverse('complaint_search:suggest_company')
-        param = {"text": "test"}
-        response = self.client.get(url, param)
-        self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND)
-        self.assertDictEqual(
-            {"error": "Elasticsearch error: Error"}, response.data
-        )
-
-    @mock.patch('complaint_search.es_interface.filter_suggest')
-    def test_suggest__transport_error_without_status_code(
+    def test_suggest__transport_error(
         self, mock_essuggest
     ):
         mock_essuggest.side_effect = TransportError('N/A', "Error")
         url = reverse('complaint_search:suggest_company')
         param = {"text": "test"}
         response = self.client.get(url, param)
-        self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST)
+        self.assertEqual(response.status_code, 424)
         self.assertDictEqual(
-            {"error": "Elasticsearch error: Error"}, response.data
+            {"error": "There was an error calling Elasticsearch"},
+            response.data
         )

complaint_search/tests/test_views_document.py

@@ -11,6 +11,7 @@
     _CCDB_UI_URL,
 )
 
+
 class DocumentTests(APITestCase):
 
     def setUp(self):
@@ -34,7 +35,6 @@ def test_document__valid(self, mock_esdocument):
         mock_esdocument.assert_called_once_with("123456")
         self.assertEqual('OK', response.data)
 
-
     @mock.patch('complaint_search.es_interface.document')
     def test_document_with_document_anon_rate_throttle(self, mock_esdocument):
         url = reverse('complaint_search:complaint', kwargs={"id": "123456"})
@@ -47,7 +47,9 @@ def test_document_with_document_anon_rate_throttle(self, mock_esdocument):
             self.assertEqual('OK', response.data)
 
         response = self.client.get(url)
-        self.assertEqual(response.status_code, status.HTTP_429_TOO_MANY_REQUESTS)
+        self.assertEqual(
+            response.status_code, status.HTTP_429_TOO_MANY_REQUESTS
+        )
         self.assertIsNotNone(response.data.get('detail'))
         self.assertIn("Request was throttled", response.data.get('detail'))
         self.assertEqual(limit, mock_esdocument.call_count)
@@ -72,19 +74,12 @@ def test_document_with_document_ui_rate_throttle(self, mock_esdocument):
         self.assertEqual(5, limit)
 
     @mock.patch('complaint_search.es_interface.document')
-    def test_document__transport_error_with_status_code(self, mock_esdocument):
-        mock_esdocument.side_effect = TransportError(status.HTTP_404_NOT_FOUND, "Error")
-        url = reverse('complaint_search:complaint', kwargs={"id": "123456"})
-        response = self.client.get(url)
-        self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND)
-        self.assertDictEqual({"error": "Elasticsearch error: Error"}, response.data)
-
-    @mock.patch('complaint_search.es_interface.document')
-    def test_document__transport_error_without_status_code(self, mock_esdocument):
+    def test_document__transport_error(self, mock_esdocument):
         mock_esdocument.side_effect = TransportError('N/A', "Error")
         url = reverse('complaint_search:complaint', kwargs={"id": "123456"})
         response = self.client.get(url)
-        self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST)
-        self.assertDictEqual({"error": "Elasticsearch error: Error"}, response.data)
-
-
+        self.assertEqual(response.status_code, 424)
+        self.assertDictEqual(
+            {"error": "There was an error calling Elasticsearch"},
+            response.data
+        )