Merge pull request #44 from Yelp/shawn_fix_jsondecode_500

Fix crash caused by a JSONDecodeError

Author: karmeleon

pyramid_hypernova/batch.py

@@ -7,6 +7,7 @@
 
 from pyramid_hypernova.rendering import render_blank_markup
 from pyramid_hypernova.rendering import RenderToken
+from pyramid_hypernova.request import ErrorData
 from pyramid_hypernova.request import HypernovaQuery
 from pyramid_hypernova.request import HypernovaQueryError
 from pyramid_hypernova.types import HypernovaError
@@ -141,7 +142,7 @@ def process_responses(self, query, jobs):
             __, __, exc_traceback = sys.exc_info()
             # We allow clients to send specific error data with their response that they may want to surface.
             # Check if any has been propagated, otherwise proceed with generic HypernovaError
-            if getattr(e, 'error_data', None) is not None:
+            if isinstance(getattr(e, 'error_data', None), ErrorData):
                 error = HypernovaError(
                     name=e.error_data.name,
                     message=e.error_data.message,

pyramid_hypernova/request.py

@@ -101,7 +101,9 @@ def json(self):
                     response_error_data = self.response.json().get('error', None)
                     error_data = format_response_error_data(response_error_data)
                 except JSONDecodeError:
-                    error_data = self.response.text or 'No response data'
+                    error_data = format_response_error_data({
+                        'message': self.response.text or 'SSRS did not return any content',
+                    })
 
                 raise HypernovaQueryError(e, error_data)
             except ConnectionError as e:

setup.py

@@ -4,7 +4,7 @@
 
 setup(
     name='pyramid-hypernova',
-    version='10.0.1',
+    version='10.0.2',
     author='Yelp, Inc.',
     author_email='[email protected]',
     license='MIT',

tests/batch_test.py

@@ -365,6 +365,84 @@ def test_batch_request_with_unhealthy_service(
             ),
         }
 
+    def test_batch_request_with_unhealthy_service_and_none_error_data(
+        self,
+        spy_plugin_controller,
+        test_data,
+        batch_request,
+        mock_hypernova_query,
+    ):
+        data = test_data[0]
+        job = Job(name='MyComponent.js', data=data[0], context={})
+        token = batch_request.render('MyComponent.js', data[0])
+
+        mock_hypernova_query.return_value.json.side_effect = HypernovaQueryError(
+            'oh no',
+            None,
+        )
+        response = batch_request.submit()
+
+        if batch_request.max_batch_size is None:
+            assert mock_hypernova_query.call_count == 1
+        else:
+            # Division (rounded-up) up to get total number of calls
+            jobs_count = len(batch_request.jobs)
+            max_batch_size = batch_request.max_batch_size
+            batch_count = (jobs_count + (max_batch_size - 1)) // max_batch_size
+            assert mock_hypernova_query.call_count == batch_count
+            mock_hypernova_query.assert_called_with(mock.ANY, mock.ANY, mock.ANY, batch_count == 1, {})
+
+        assert response == {
+            token.identifier: JobResult(
+                error=HypernovaError(
+                    name='HypernovaQueryError',
+                    message='oh no',
+                    stack=mock.ANY,
+                ),
+                html=render_blank_markup(token.identifier, job, True, batch_request.json_encoder),
+                job=job,
+            ),
+        }
+
+    def test_batch_request_with_unhealthy_service_and_other_type_error_data(
+        self,
+        spy_plugin_controller,
+        test_data,
+        batch_request,
+        mock_hypernova_query,
+    ):
+        data = test_data[0]
+        job = Job(name='MyComponent.js', data=data[0], context={})
+        token = batch_request.render('MyComponent.js', data[0])
+
+        mock_hypernova_query.return_value.json.side_effect = HypernovaQueryError(
+            'oh no',
+            'i should not be a string',
+        )
+        response = batch_request.submit()
+
+        if batch_request.max_batch_size is None:
+            assert mock_hypernova_query.call_count == 1
+        else:
+            # Division (rounded-up) up to get total number of calls
+            jobs_count = len(batch_request.jobs)
+            max_batch_size = batch_request.max_batch_size
+            batch_count = (jobs_count + (max_batch_size - 1)) // max_batch_size
+            assert mock_hypernova_query.call_count == batch_count
+            mock_hypernova_query.assert_called_with(mock.ANY, mock.ANY, mock.ANY, batch_count == 1, {})
+
+        assert response == {
+            token.identifier: JobResult(
+                error=HypernovaError(
+                    name='HypernovaQueryError',
+                    message='oh no',
+                    stack=mock.ANY,
+                ),
+                html=render_blank_markup(token.identifier, job, True, batch_request.json_encoder),
+                job=job,
+            ),
+        }
+
 
 class TestBatchRequestLifecycleMethods:
     """Test that BatchRequest calls plugin lifecycle methods at the

tests/request_test.py

@@ -126,7 +126,8 @@ def test_json_error_fallbacks_to_text(self, mock_fido_fetch, mock_requests_post)
             data=mock.ANY,
         )
         assert str(exc_info.value) == str(HypernovaQueryError(HTTPError('ayy lmao')))
-        assert exc_info.value.error_data == 'Non-JSON Error Body'
+        assert isinstance(exc_info.value.error_data, ErrorData)
+        assert exc_info.value.error_data.message == 'Non-JSON Error Body'
 
     def test_successful_send_asynchronous(self, mock_fido_fetch, mock_requests_post):
         mock_fido_fetch.return_value.wait.return_value.code = 200