Move task error handling to function

* Replaces large amounts of repeated code on the task view with a
  _handle_task_error() function
* try and unify error handling to always return a list 'errors' or
  a dict when specific field name errors are known.

Change-Id: I9d140db0af204524eadc4ba3a6d3eb7299b239b4
This commit is contained in:
Amelia Cordwell 2018-01-15 10:04:23 +13:00 committed by Adrian Turjak
parent 16be61428f
commit dab3938ab2
5 changed files with 156 additions and 221 deletions

View File

@ -281,7 +281,7 @@ class UserRoles(tasks.TaskView):
if errors:
self.logger.info("(%s) - Validation errors with registration." %
timezone.now())
return Response(errors, status=status)
return Response({'errors': errors}, status=status)
response_dict = {'notes': processed.get('notes')}
@ -493,7 +493,7 @@ class UpdateProjectQuotas(tasks.TaskView):
if errors:
self.logger.info("(%s) - Validation errors with task." %
timezone.now())
return Response(errors, status=status)
return Response({'errors': errors}, status=status)
if processed.get('auto_approved', False):
response_dict = {'notes': processed['notes']}

View File

@ -163,19 +163,12 @@ class TaskView(APIViewWithLogger):
# Instantiate Task
ip_address = request.META['REMOTE_ADDR']
keystone_user = request.keystone_user
try:
task = Task.objects.create(
ip_address=ip_address,
keystone_user=keystone_user,
project_id=keystone_user['project_id'],
task_type=self.task_type,
hash_key=hash_key)
except KeyError:
task = Task.objects.create(
ip_address=ip_address,
keystone_user=keystone_user,
task_type=self.task_type,
hash_key=hash_key)
task = Task.objects.create(
ip_address=ip_address,
keystone_user=keystone_user,
project_id=keystone_user.get('project_id'),
task_type=self.task_type,
hash_key=hash_key)
task.save()
# Instantiate actions with serializers
@ -195,24 +188,8 @@ class TaskView(APIViewWithLogger):
try:
action_instance.pre_approve()
except Exception as e:
import traceback
trace = traceback.format_exc()
self.logger.critical((
"(%s) - Exception escaped! %s\nTrace: \n%s") % (
timezone.now(), e, trace))
notes = {
'errors':
[("Error: '%s' while setting up task. " +
"See task itself for details.") % e]
}
create_notification(task, notes, error=True)
response_dict = {
'errors':
["Error: Something went wrong on the server. " +
"It will be looked into shortly."]
}
return response_dict, 500
return self._handle_task_error(
e, task, error_text='while setting up task')
# send initial confirmation email:
email_conf = class_conf.get('emails', {}).get('initial', None)
@ -255,25 +232,8 @@ class TaskView(APIViewWithLogger):
send_stage_email(task, email_conf, token)
return {'notes': ['created token']}, 200
except KeyError as e:
import traceback
trace = traceback.format_exc()
self.logger.critical((
"(%s) - Exception escaped! %s\nTrace: \n%s") % (
timezone.now(), e, trace))
notes = {
'errors':
[("Error: '%s' while sending " +
"token. See task " +
"itself for details.") % e]
}
create_notification(task, notes, error=True)
response_dict = {
'errors':
["Error: Something went wrong on the " +
"server. It will be looked into shortly."]
}
return response_dict, 500
return self._handle_task_error(
e, task, error_text='while sending token')
def approve(self, request, task):
"""
@ -297,30 +257,15 @@ class TaskView(APIViewWithLogger):
valid = all([act.valid for act in actions])
if not valid:
return {'errors': ['actions invalid']}, 400
# TODO(amelia): get action invalidation reasons
# post_approve all actions
for action in actions:
try:
action.post_approve()
except Exception as e:
import traceback
trace = traceback.format_exc()
self.logger.critical((
"(%s) - Exception escaped! %s\nTrace: \n%s") % (
timezone.now(), e, trace))
notes = {
'errors':
[("Error: '%s' while approving task. " +
"See task itself for details.") % e]
}
create_notification(task, notes, error=True)
response_dict = {
'errors':
["Error: Something went wrong on the server. " +
"It will be looked into shortly."]
}
return response_dict, 500
return self._handle_task_error(
e, task, error_text='while approving task')
valid = all([act.valid for act in actions])
if not valid:
@ -335,25 +280,8 @@ class TaskView(APIViewWithLogger):
try:
action.submit({})
except Exception as e:
import traceback
trace = traceback.format_exc()
self.logger.critical((
"(%s) - Exception escaped! %s\nTrace: \n%s") % (
timezone.now(), e, trace))
notes = {
'errors':
[("Error: '%s' while submitting " +
"task. See task " +
"itself for details.") % e]
}
create_notification(task, notes, error=True)
response_dict = {
'errors':
["Error: Something went wrong on the " +
"server. It will be looked into shortly."]
}
return response_dict, 500
self._handle_task_error(
e, task, error_text='while submitting task')
task.completed = True
task.completed_on = timezone.now()
@ -403,7 +331,7 @@ class CreateProject(TaskView):
if errors:
self.logger.info("(%s) - Validation errors with task." %
timezone.now())
return Response(errors, status=status)
return Response({'errors': errors}, status=status)
notes = {
'notes':
@ -452,8 +380,6 @@ class InviteUser(TaskView):
self.logger.info("(%s) - Validation errors with task." %
timezone.now())
if isinstance(errors, dict):
return Response(errors, status=status)
return Response({'errors': errors}, status=status)
response_dict = {'notes': processed['notes']}
@ -499,7 +425,7 @@ class ResetPassword(TaskView):
if errors:
self.logger.info("(%s) - Validation errors with task." %
timezone.now())
return Response(errors, status=status)
return Response({'errors': errors}, status=status)
task = processed['task']
self.logger.info("(%s) - AutoApproving Resetuser request."
@ -581,7 +507,7 @@ class EditUser(TaskView):
if errors:
self.logger.info("(%s) - Validation errors with task." %
timezone.now())
return Response(errors, status=status)
return Response({'errors': errors}, status=status)
response_dict = {'notes': processed.get('notes')}
add_task_id_for_roles(request, processed, response_dict, ['admin'])
@ -609,7 +535,7 @@ class UpdateEmail(TaskView):
if errors:
self.logger.info("(%s) - Validation errors with task." %
timezone.now())
return Response(errors, status=status)
return Response({'errors': errors}, status=status)
response_dict = {'notes': processed['notes']}
return Response(response_dict, status=status)

View File

@ -1511,3 +1511,53 @@ class AdminAPITests(APITestCase):
response.json()['notes'],
['If user with email exists, reset token will be issued.'])
self.assertEqual(0, Token.objects.count())
@mock.patch('adjutant.common.tests.fake_clients.FakeManager.find_project')
def test_apiview_error_handler(self, mocked_find):
"""
Ensure the _handle_task_error function works as expected for APIViews.
"""
setup_identity_cache()
url = "/v1/actions/CreateProject"
data = {'project_name': "test_project", 'email': "test@example.com"}
response = self.client.post(url, data, format='json')
self.assertEqual(
response.status_code, status.HTTP_200_OK)
mocked_find.side_effect = KeyError("Forced key error.")
new_task = Task.objects.all()[0]
url = "/v1/tasks/" + new_task.uuid
data = {
'project_name': "test_project", 'email': "test@example.com",
'region': 'test'
}
headers = {
'project_name': "test_project",
'project_id': "test_project_id",
'roles': "admin,_member_",
'username': "test@example.com",
'user_id': "test_user_id",
'authenticated': True
}
response = self.client.put(url, data, format='json', headers=headers)
self.assertEqual(
response.status_code, status.HTTP_500_INTERNAL_SERVER_ERROR)
self.assertEqual(
response.json()['errors'],
["Error: Something went wrong on the server. " +
"It will be looked into shortly."])
new_task = Task.objects.all()[0]
new_notification = Notification.objects.all()[1]
self.assertTrue(new_notification.error)
self.assertEqual(
new_notification.notes,
{'errors': [
"Error: KeyError('Forced key error.') while updating task. " +
"See task itself for details."]})
self.assertEqual(new_notification.task, new_task)

View File

@ -20,7 +20,7 @@ from django.core import mail
from rest_framework import status
from adjutant.api.models import Task, Token
from adjutant.api.models import Task, Token, Notification
from adjutant.api.v1.tasks import CreateProject
from adjutant.common.tests.fake_clients import (
FakeManager, setup_identity_cache)
@ -61,17 +61,19 @@ class TaskViewTests(AdjutantAPITestCase):
'project_id': project.id}
response = self.client.post(url, data, format='json', headers=headers)
self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST)
self.assertEqual(response.json(),
{'email': ['This field is required.']})
self.assertEqual(
response.json(),
{'errors': {'email': ['This field is required.']}})
data = {'email': "not_a_valid_email", 'roles': ["not_a_valid_role"],
'project_id': project.id}
response = self.client.post(url, data, format='json', headers=headers)
self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST)
self.assertEqual(
response.json(), {
response.json(),
{'errors': {
'email': ['Enter a valid email address.'],
'roles': ['"not_a_valid_role" is not a valid choice.']})
'roles': ['"not_a_valid_role" is not a valid choice.']}})
def test_new_user(self):
"""
@ -837,8 +839,9 @@ class TaskViewTests(AdjutantAPITestCase):
response = self.client.post(url, data, format='json', headers=headers)
self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST)
self.assertEqual(response.json(),
{'new_email': [u'Enter a valid email address.']})
self.assertEqual(
response.json(),
{'errors': {'new_email': [u'Enter a valid email address.']}})
@override_settings(USERNAME_IS_EMAIL=True)
def test_update_email_pre_existing_user_with_email(self):
@ -867,7 +870,7 @@ class TaskViewTests(AdjutantAPITestCase):
response = self.client.post(url, data, format='json', headers=headers)
self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST)
self.assertEqual(response.json(), ['actions invalid'])
self.assertEqual(response.json(), {'errors': ['actions invalid']})
self.assertEqual(len(Token.objects.all()), 0)
self.assertEqual(len(mail.outbox), 0)
@ -1398,3 +1401,35 @@ class TaskViewTests(AdjutantAPITestCase):
actions = new_task.actions
observed_action_names = [a.action_name for a in actions]
self.assertEqual(observed_action_names, expected_action_names)
@mock.patch('adjutant.common.tests.fake_clients.FakeManager.find_project')
def test_task_error_handler(self, mocked_find):
"""
Ensure the _handle_task_error function works as expected.
"""
setup_identity_cache()
mocked_find.side_effect = KeyError("Forced key error.")
url = "/v1/actions/CreateProject"
data = {'project_name': "test_project", 'email': "test@example.com"}
response = self.client.post(url, data, format='json')
self.assertEqual(
response.status_code, status.HTTP_500_INTERNAL_SERVER_ERROR)
self.assertEqual(
response.json(),
{'errors': ["Error: Something went wrong on the server. " +
"It will be looked into shortly."]})
new_task = Task.objects.all()[0]
new_notification = Notification.objects.all()[0]
self.assertTrue(new_notification.error)
self.assertEqual(
new_notification.notes,
{'errors': [
"Error: KeyError('Forced key error.') while setting up " +
"task. See task itself for details."]})
self.assertEqual(new_notification.task, new_task)

View File

@ -41,6 +41,29 @@ class APIViewWithLogger(APIView):
super(APIViewWithLogger, self).__init__(*args, **kwargs)
self.logger = getLogger('adjutant')
def _handle_task_error(self, e, task, error_text="while running task",
return_response=False):
import traceback
trace = traceback.format_exc()
self.logger.critical((
"(%s) - Exception escaped! %s\nTrace: \n%s") % (
timezone.now(), e, trace))
notes = {
'errors':
[("Error: %s(%s) %s. See task " +
"itself for details.") % (type(e).__name__, e, error_text)]
}
create_notification(task, notes, error=True)
response_dict = {
'errors':
["Error: Something went wrong on the " +
"server. It will be looked into shortly."]
}
if return_response:
return Response(response_dict, status=500)
return response_dict, 500
class StatusView(APIViewWithLogger):
@ -101,9 +124,9 @@ class NotificationList(APIViewWithLogger):
try:
notifications = paginator.page(page)
except EmptyPage:
return Response({'error': 'Empty page'}, status=400)
return Response({'errors': ['Empty page']}, status=400)
except PageNotAnInteger:
return Response({'error': 'Page not an integer'},
return Response({'errors': ['Page not an integer']},
status=400)
note_list = []
@ -211,9 +234,9 @@ class TaskList(APIViewWithLogger):
try:
tasks = paginator.page(page)
except EmptyPage:
return Response({'error': 'Empty page'}, status=400)
return Response({'errors': ['Empty page']}, status=400)
except PageNotAnInteger:
return Response({'error': 'Page not an integer'},
return Response({'errors': ['Page not an integer']},
status=400)
task_list = []
for task in tasks:
@ -315,26 +338,8 @@ class TaskDetail(APIViewWithLogger):
try:
act['action'].get_action().pre_approve()
except Exception as e:
notes = {
'errors':
[("Error: '%s' while updating task. " +
"See task itself for details.") % e],
'task': task.uuid
}
create_notification(task, notes, error=True)
import traceback
trace = traceback.format_exc()
self.logger.critical(("(%s) - Exception escaped! %s\n" +
"Trace: \n%s") %
(timezone.now(), e, trace))
response_dict = {
'errors':
["Error: Something went wrong on the server. " +
"It will be looked into shortly."]
}
return Response(response_dict, status=500)
return self._handle_task_error(
e, task, "while updating task", return_response=True)
return Response(
{'notes': ["Task successfully updated."]},
@ -419,21 +424,8 @@ class TaskDetail(APIViewWithLogger):
try:
act_model.post_approve()
except Exception as e:
notes = {
'errors':
[("Error: '%s' while approving task. " +
"See task itself for details.") % e],
'task': task.uuid
}
create_notification(task, notes, error=True)
import traceback
trace = traceback.format_exc()
self.logger.critical(("(%s) - Exception escaped! %s\n" +
"Trace: \n%s") %
(timezone.now(), e, trace))
return Response(notes, status=500)
return self._handle_task_error(
e, task, "while approving task", return_response=True)
if not action.valid:
valid = False
@ -454,48 +446,16 @@ class TaskDetail(APIViewWithLogger):
return Response({'notes': ['created token']},
status=200)
except KeyError as e:
notes = {
'errors':
[("Error: '%s' while sending " +
"token. See task " +
"itself for details.") % e],
'task': task.uuid
}
create_notification(task, notes, error=True)
import traceback
trace = traceback.format_exc()
self.logger.critical(("(%s) - Exception escaped!" +
" %s\n Trace: \n%s") %
(timezone.now(), e, trace))
response_dict = {
'errors':
["Error: Something went wrong on the " +
"server. It will be looked into shortly."]
}
return Response(response_dict, status=500)
return self._handle_task_error(
e, task, "while sending token", return_response=True)
else:
for action in actions:
try:
action.submit({})
except Exception as e:
notes = {
'errors':
[("Error: '%s' while submitting " +
"task. See task " +
"itself for details.") % e],
'task': task.uuid
}
create_notification(task, notes, error=True)
import traceback
trace = traceback.format_exc()
self.logger.critical(("(%s) - Exception escaped!" +
" %s\n Trace: \n%s") %
(timezone.now(), e, trace))
return Response(notes, status=500)
return self._handle_task_error(
e, task, "while submitting task",
return_response=True)
task.completed = True
task.completed_on = timezone.now()
@ -582,7 +542,7 @@ class TokenList(APIViewWithLogger):
uuid = request.data.get('task', None)
if uuid is None:
return Response(
{'task': ["This field is required.", ]},
{'errors': {'task': ["This field is required.", ]}},
status=400)
try:
if 'admin' in request.keystone_user['roles']:
@ -625,27 +585,8 @@ class TokenList(APIViewWithLogger):
email_conf = class_conf['emails']['token']
send_stage_email(task, email_conf, token)
except KeyError as e:
notes = {
'errors': [
("Error: '%(error)s' while sending token. " +
"See registration itself for details.") % {'error': e}
],
'task': task.uuid
}
create_notification(task, notes, error=True)
import traceback
trace = traceback.format_exc()
self.logger.critical(("(%s) - Exception escaped!" +
" %s\n Trace: \n%s") %
(timezone.now(), e, trace))
response_dict = {
'errors':
["Error: Something went wrong on the " +
"server. It will be looked into shortly."]
}
return Response(response_dict, status=500)
return self._handle_task_error(
e, task, "while sending token", return_response=True)
return Response(
{'notes': ['Token reissued.']}, status=200)
@ -764,26 +705,9 @@ class TokenDetail(APIViewWithLogger):
valid = False
except Exception as e:
notes = {
'errors':
[("Error: '%s' while submitting task. " +
"See task itself for details.") % e],
'task': token.task.uuid
}
create_notification(token.task, notes, error=True)
import traceback
trace = traceback.format_exc()
self.logger.critical(("(%s) - Exception escaped! %s\n" +
"Trace: \n%s") %
(timezone.now(), e, trace))
response_dict = {
'errors':
["Error: Something went wrong on the server. " +
"It will be looked into shortly."]
}
return Response(response_dict, status=500)
return self._handle_task_error(
e, token.task, "while submiting task",
return_response=True)
if not valid:
return Response({"errors": ["Actions invalid"]}, status=400)