Merge pull request #2941 from bacongobbler/2512-return-json-errors

fix(controller): return proper json messages on error

Author: Matthew Fisher

controller/api/__init__.py

@@ -2,4 +2,4 @@
 The **api** Django app presents a RESTful web API for interacting with the **deis** system.
 """
 
-__version__ = '1.1.1'
+__version__ = '1.1.2'

controller/api/tests/test_app.py

@@ -247,7 +247,8 @@ def test_run_without_auth(self):
         response = self.client.post(url, json.dumps(body), content_type='application/json',
                                     HTTP_AUTHORIZATION='token {}'.format(self.token))
         self.assertEquals(response.status_code, 400)
-        self.assertEquals(response.data, 'Support for admin commands is not configured')
+        self.assertEquals(response.data, {'detail': 'Support for admin commands '
+                                                    'is not configured'})
 
     def test_run_without_release_should_error(self):
         """
@@ -264,8 +265,8 @@ def test_run_without_release_should_error(self):
         response = self.client.post(url, json.dumps(body), content_type='application/json',
                                     HTTP_AUTHORIZATION='token {}'.format(self.token))
         self.assertEqual(response.status_code, 400)
-        self.assertEqual(response.data, "No build associated with this release "
-                                        "to run this command")
+        self.assertEqual(response.data, {'detail': 'No build associated with this '
+                                                   'release to run this command'})
 
     def test_unauthorized_user_cannot_see_app(self):
         """

controller/api/tests/test_auth.py

@@ -133,6 +133,8 @@ def test_passwd(self):
         response = self.client.post(url, json.dumps(submit), content_type='application/json',
                                     HTTP_AUTHORIZATION='token {}'.format(token))
         self.assertEqual(response.status_code, 400)
+        self.assertEqual(response.data, {'detail': 'Current password does not match'})
+        self.assertEqual(response.get('content-type'), 'application/json')
         submit = {
             'password': password,
             'new_password': 'password2',

controller/api/tests/test_container.py

@@ -303,7 +303,8 @@ def test_container_errors(self):
         body = {'web': 'not_an_int'}
         response = self.client.post(url, json.dumps(body), content_type='application/json',
                                     HTTP_AUTHORIZATION='token {}'.format(self.token))
-        self.assertContains(response, 'Invalid scaling format', status_code=400)
+        self.assertEqual(response.status_code, 400)
+        self.assertEqual(response.data, {'detail': 'Invalid scaling format'})
         body = {'invalid': 1}
         response = self.client.post(url, json.dumps(body), content_type='application/json',
                                     HTTP_AUTHORIZATION='token {}'.format(self.token))
@@ -446,7 +447,7 @@ def test_scale_without_build_should_error(self):
         response = self.client.post(url, json.dumps(body), content_type='application/json',
                                     HTTP_AUTHORIZATION='token {}'.format(self.token))
         self.assertEqual(response.status_code, 400)
-        self.assertEqual(response.data, "No build associated with this release")
+        self.assertEqual(response.data, {'detail': 'No build associated with this release'})
 
     def test_command_good(self):
         """Test the default command for each container workflow"""

controller/api/tests/test_release.py

@@ -147,6 +147,8 @@ def test_release_rollback(self):
         response = self.client.post(url, content_type='application/json',
                                     HTTP_AUTHORIZATION='token {}'.format(self.token))
         self.assertEqual(response.status_code, 400)
+        self.assertEqual(response.data, {'detail': 'version cannot be below 0'})
+        self.assertEqual(response.get('content-type'), 'application/json')
         # update config to roll a new release
         url = '/v1/apps/{app_id}/config'.format(**locals())
         body = {'values': json.dumps({'NEW_URL1': 'http://localhost:8080/'})}

controller/api/tests/test_scheduler.py

@@ -69,6 +69,8 @@ def test_create_chaos(self):
         response = self.client.post(url, json.dumps(body), content_type='application/json',
                                     HTTP_AUTHORIZATION='token {}'.format(self.token))
         self.assertEqual(response.status_code, 503)
+        self.assertEqual(response.data, {'detail': 'aborting, failed to create some containers'})
+        self.assertEqual(response.get('content-type'), 'application/json')
         # inspect broken containers
         url = "/v1/apps/{app_id}/containers".format(**locals())
         response = self.client.get(url, HTTP_AUTHORIZATION='token {}'.format(self.token))
@@ -151,6 +153,8 @@ def test_destroy_chaos(self):
         response = self.client.post(url, json.dumps(body), content_type='application/json',
                                     HTTP_AUTHORIZATION='token {}'.format(self.token))
         self.assertEqual(response.status_code, 503)
+        self.assertEqual(response.data, {'detail': 'aborting, failed to destroy some containers'})
+        self.assertEqual(response.get('content-type'), 'application/json')
         # inspect broken containers
         url = "/v1/apps/{app_id}/containers".format(**locals())
         response = self.client.get(url, HTTP_AUTHORIZATION='token {}'.format(self.token))
@@ -167,7 +171,10 @@ def test_destroy_chaos(self):
             # break if we destroyed successfully
             if response.status_code == 204:
                 break
-            self.assertEquals(response.status_code, 503)
+            self.assertEqual(response.status_code, 503)
+            self.assertEqual(response.data, {'detail': 'aborting, failed to '
+                                                       'destroy some containers'})
+            self.assertEqual(response.get('content-type'), 'application/json')
             # inspect broken containers
             url = "/v1/apps/{app_id}/containers".format(**locals())
             response = self.client.get(url, HTTP_AUTHORIZATION='token {}'.format(self.token))
@@ -216,6 +223,8 @@ def test_build_chaos(self):
         response = self.client.post(url, json.dumps(body), content_type='application/json',
                                     HTTP_AUTHORIZATION='token {}'.format(self.token))
         self.assertEqual(response.status_code, 503)
+        self.assertEqual(response.data, {'detail': 'aborting, failed to create some containers'})
+        self.assertEqual(response.get('content-type'), 'application/json')
         # inspect releases
         url = "/v1/apps/{app_id}/releases".format(**locals())
         response = self.client.get(url, HTTP_AUTHORIZATION='token {}'.format(self.token))
@@ -267,6 +276,8 @@ def test_config_chaos(self):
         response = self.client.post(url, json.dumps(body), content_type='application/json',
                                     HTTP_AUTHORIZATION='token {}'.format(self.token))
         self.assertEqual(response.status_code, 503)
+        self.assertEqual(response.data, {'detail': 'aborting, failed to create some containers'})
+        self.assertEqual(response.get('content-type'), 'application/json')
         # inspect releases
         url = "/v1/apps/{app_id}/releases".format(**locals())
         response = self.client.get(url, HTTP_AUTHORIZATION='token {}'.format(self.token))
@@ -315,3 +326,5 @@ def test_run_chaos(self):
         response = self.client.post(url, json.dumps(body), content_type='application/json',
                                     HTTP_AUTHORIZATION='token {}'.format(self.token))
         self.assertEqual(response.status_code, 503)
+        self.assertEqual(response.data, {'detail': 'exit code 1'})
+        self.assertEqual(response.get('content-type'), 'application/json')

controller/api/views.py

@@ -38,7 +38,8 @@ def get_object(self):
     def passwd(self, request, **kwargs):
         obj = self.get_object()
         if not obj.check_password(request.DATA['password']):
-            return Response("Current password did not match", status=status.HTTP_400_BAD_REQUEST)
+            return Response({'detail': 'Current password does not match'},
+                            status=status.HTTP_400_BAD_REQUEST)
         obj.set_password(request.DATA['new_password'])
         obj.save()
         return Response({'status': 'password set'})
@@ -60,7 +61,7 @@ def create(self, request, *args, **kwargs):
             return super(BaseDeisViewSet, self).create(request, *args, **kwargs)
         # If the scheduler oopsie'd
         except RuntimeError as e:
-            return Response(str(e), status=status.HTTP_503_SERVICE_UNAVAILABLE)
+            return Response({'detail': str(e)}, status=status.HTTP_503_SERVICE_UNAVAILABLE)
 
 
 class AppResourceViewSet(BaseDeisViewSet):
@@ -131,22 +132,20 @@ def post_save(self, app):
 
     def scale(self, request, **kwargs):
         new_structure = {}
+        app = self.get_object()
         try:
             for target, count in request.DATA.items():
                 new_structure[target] = int(count)
-        except (TypeError, ValueError):
-            return Response('Invalid scaling format',
-                            status=status.HTTP_400_BAD_REQUEST)
-        app = self.get_object()
-        try:
             models.validate_app_structure(new_structure)
             app.scale(request.user, new_structure)
-        except (EnvironmentError, ValidationError) as e:
-            return Response(str(e), status=status.HTTP_400_BAD_REQUEST)
+        except (TypeError, ValueError):
+            return Response({'detail': 'Invalid scaling format'},
+                            status=status.HTTP_400_BAD_REQUEST)
+        except (ValidationError, EnvironmentError) as e:
+            return Response({'detail': str(e)}, status=status.HTTP_400_BAD_REQUEST)
         except RuntimeError as e:
-            return Response(str(e), status=status.HTTP_503_SERVICE_UNAVAILABLE)
-        return Response(status=status.HTTP_204_NO_CONTENT,
-                        content_type='application/json')
+            return Response({'detail': str(e)}, status=status.HTTP_503_SERVICE_UNAVAILABLE)
+        return Response(status=status.HTTP_204_NO_CONTENT)
 
     def logs(self, request, **kwargs):
         app = self.get_object()
@@ -165,9 +164,9 @@ def run(self, request, **kwargs):
         try:
             output_and_rc = app.run(self.request.user, command)
         except EnvironmentError as e:
-            return Response(str(e), status=status.HTTP_400_BAD_REQUEST)
+            return Response({'detail': str(e)}, status=status.HTTP_400_BAD_REQUEST)
         except RuntimeError as e:
-            return Response(str(e), status=status.HTTP_503_SERVICE_UNAVAILABLE)
+            return Response({'detail': str(e)}, status=status.HTTP_503_SERVICE_UNAVAILABLE)
         return Response(output_and_rc, status=status.HTTP_200_OK,
                         content_type='text/plain')
 
@@ -252,7 +251,7 @@ def rollback(self, request, **kwargs):
             response = {'version': new_release.version}
             return Response(response, status=status.HTTP_201_CREATED)
         except EnvironmentError as e:
-            return Response(str(e), status=status.HTTP_400_BAD_REQUEST)
+            return Response({'detail': str(e)}, status=status.HTTP_400_BAD_REQUEST)
         except RuntimeError:
             new_release.delete()
             raise

controller/scheduler/chaos.py

@@ -50,7 +50,7 @@ def run(self, name, image, entrypoint, command):
         Run a one-off command
         """
         if random.random() < CREATE_ERROR_RATE:
-            raise RuntimeError
+            raise RuntimeError('exit code 1')
         return 0, ''
 
 SchedulerClient = ChaosSchedulerClient

docs/reference/api-v1.1.rst

@@ -18,6 +18,8 @@ What's New
 
 **New!** Users see a 404 response when modifying applications they are unauthorized to see.
 
+**New!** Responses returned from the scheduler are properly formatted JSON responses.
+
 
 Authentication
 --------------