Add exception handling for create/destroy methods

We added exception handling for certain create/destroy methods which
involve multiple object creation (log messages and/or tags) and which
may commonly raise errors, like tag creation with invalid characters.
This was done within the superevent viewsets and in the events api.

gracedb/events/api/views.py

@@ -7,6 +7,7 @@ from django.urls import reverse as django_reverse
 from django.conf import settings
 from django.utils.functional import wraps
 from django.db import IntegrityError
+from django.core.exceptions import ValidationError
 
 from django.contrib.auth.models import User, Permission
 from django.contrib.auth.models import Group as AuthGroup
@@ -851,15 +852,17 @@ class EventLogList(APIView):
                 retval = tmp.put(request, event.graceid(), n, tagname) 
                 # XXX This seems like a bizarre way of getting an error message out.
                 if retval.status_code != 201:
-                    tw_dict = {'tagWarning': 'Error creating tag %s.' % tagname }
-                    #response['tagWarning'] = 'Error creating tag %s.' % tagname
+                    return Response(('Log message created, but error creating '
+                        'tag: {0}').format(retval.data),
+                        status=retval.status_code)
+                    #tw_dict = {'tagWarning': 'Error creating tag %s.' % tagname }
                     
         # Serialize the event log object *after* adding tags!
         rv = eventLogToDict(logentry, request=request)
         response = Response(rv, status=status.HTTP_201_CREATED)
         response['Location'] = rv['self']
-        if 'tagWarning' in tw_dict.keys():
-            response['tagWarning'] = tw_dict['tagWarning']
+        #if 'tagWarning' in tw_dict.keys():
+        #    response['tagWarning'] = tw_dict['tagWarning']
 
         # Issue alert.
         description = "LOG: "
@@ -1234,7 +1237,12 @@ class EventLogTagDetail(APIView):
                     return HttpResponseForbidden(msg)            
 
             # Look for the tag.  If it doesn't already exist, create it.
-            tag, created = Tag.objects.get_or_create(name=tagname)
+            #tag, created = Tag.objects.get_or_create(name=tagname)
+            try:
+                tag, created = Tag.objects.get_or_create(name=tagname)
+            except ValidationError as e:
+                return Response(e.__str__(),
+                    status=status.HTTP_400_BAD_REQUEST)
             displayName = request.data.get('displayName')
             if created:
                 tag.displayName = displayName
@@ -1843,21 +1851,23 @@ class VOEventList(APIView):
         # Create LogEntry to document the new VOEvent.
         logentry = EventLog(event=event,
                              issuer=request.user,
-                             comment='',
+                             comment='New VOEvent',
                              filename=filename,
                              file_version=file_version)
         try:
             logentry.save()
-        except:
+        except Exception as e:
             rv['warnings'] = 'Problem saving log entry for VOEvent %s of %s' % (voevent.N, 
                 event.graceid()) 
 
-        # Tag log entry as 'sky_loc'
+        # Tag log entry as 'em_follow')
         tmp = EventLogTagDetail()
-        retval = tmp.put(request, event.graceid(), logentry.N, 'em_follow') 
+        retval = tmp.put(request, event.graceid(), logentry.N, 'em_follow')
         # XXX This seems like a bizarre way of getting an error message out.
         if retval.status_code != 201:
-            rv['tagWarning'] = 'Error tagging VOEvent log message as em_follow.'
+            return Response(('VOEvent log message created, but error tagging '
+                'message: {0}').format(retval.data), status=retval.status_code)
+            #rv['tagWarning'] = 'Error tagging VOEvent log message as em_follow.'
 
         # Issue alert.
         description = "VOEVENT: %s" % filename

gracedb/superevents/api/mixins.py

 from django.shortcuts import get_object_or_404
+from django.core.exceptions import ValidationError as DjangoValidationError
+from rest_framework.exceptions import ValidationError as \
+    RestFrameworkValidationError
+from rest_framework import status, mixins
+from rest_framework.response import Response
 
 from ..models import Superevent
 from .settings import SUPEREVENT_LOOKUP_FIELD, SUPEREVENT_LOOKUP_REGEX
 
+import logging
+logger = logging.getLogger(__name__)
 
 class GetParentMixin(object):
     parent_lookup_field = None
@@ -43,3 +50,47 @@ class BaseGetObjectMixin(object):
         filter_kwargs = {self.query_field: query_value}
         obj = get_object_or_404(queryset, **filter_kwargs)
         return obj
+
+
+class SafeDestroyMixin(mixins.DestroyModelMixin):
+    """
+    Copy of rest_framework's DestroyModelMixin which wraps
+    the call to perform_destroy() in a try-except block for
+    proper response handling.
+    """
+    destroy_error_classes = (Exception,)
+    destroy_error_response_status = status.HTTP_500_INTERNAL_SERVER_ERROR
+    destroy_error_message = None
+
+    def destroy(self, request, *args, **kwargs):
+        instance = self.get_object()
+        try:
+            self.perform_destroy(instance)
+        except self.destroy_error_classes as e:
+            err_msg = self.destroy_error_message or e.__str__()
+            return Response(err_msg, status=self.destroy_error_response_status)
+        return Response(status=status.HTTP_204_NO_CONTENT)
+
+
+class SafeCreateMixin(mixins.CreateModelMixin):
+    """
+    Copy of rest_framework's CreateModelMixin which wraps
+    the call to perform_destroy() in a try-except block for
+    proper response handling.
+    """
+    create_error_classes = \
+        (DjangoValidationError, RestFrameworkValidationError)
+    create_error_response_status = status.HTTP_400_BAD_REQUEST
+    create_error_message = None
+
+    def create(self, request, *args, **kwargs):
+        serializer = self.get_serializer(data=request.data)
+        serializer.is_valid(raise_exception=True)
+        try:
+            self.perform_create(serializer)
+        except self.create_error_classes as e:
+            err_msg = self.create_error_message or e.__str__()
+            return Response(err_msg, status=self.create_error_response_status)
+        headers = self.get_success_headers(serializer.data)
+        return Response(serializer.data, status=status.HTTP_201_CREATED, headers=headers)
+

gracedb/superevents/api/views.py

@@ -26,7 +26,8 @@ from events.view_utils import reverse as gracedb_reverse
 from events.api.backends import LigoAuthentication
 
 from .filters import SupereventSearchFilter, SupereventOrderingFilter
-from .mixins import GetParentSupereventMixin, BaseGetObjectMixin
+from .mixins import GetParentSupereventMixin, BaseGetObjectMixin, \
+    SafeDestroyMixin, SafeCreateMixin
 from .paginators import BasePaginationFactory, CustomLabelPagination, \
     CustomLogTagPagination, CustomSupereventPagination
 from .serializers import SupereventSerializer, SupereventUpdateSerializer, \
@@ -102,13 +103,15 @@ class SupereventViewSet(viewsets.ModelViewSet):
 class SupereventEventViewSet(mixins.ListModelMixin,
                              mixins.CreateModelMixin,
                              mixins.RetrieveModelMixin,
-                             mixins.DestroyModelMixin,
+                             SafeDestroyMixin,
                              GetParentSupereventMixin,
                              viewsets.GenericViewSet):
     """View for events attached to a superevent"""
     serializer_class = SupereventEventSerializer
     pagination_class = BasePaginationFactory(results_name='events')
     lookup_field = 'graceid'
+    destroy_error_classes = (Superevent.PreferredEventRemovalError,)
+    destroy_error_response_status = status.HTTP_400_BAD_REQUEST
 
     def get_queryset(self):
         superevent = self.get_parent()
@@ -127,20 +130,11 @@ class SupereventEventViewSet(mixins.ListModelMixin,
 
         return obj
 
-    def destroy(self, request, *args, **kwargs):
-        instance = self.get_object()
-        try:
-            self.perform_destroy(instance)
-        except Superevent.PreferredEventRemovalError as e:
-            return Response(e.__str__(), status=status.HTTP_400_BAD_REQUEST)
-
-        return Response(status=status.HTTP_204_NO_CONTENT)
-
     def perform_destroy(self, instance):
-       remove_event_from_superevent(instance.superevent, instance,
-           self.request.user, add_superevent_log=True,
-           add_event_log=True, issue_superevent_alert=True,
-           issue_event_alert=True)
+        remove_event_from_superevent(instance.superevent, instance,
+            self.request.user, add_superevent_log=True,
+            add_event_log=True, issue_superevent_alert=True,
+            issue_event_alert=True)
 
 
 class SupereventLabelViewSet(GetParentSupereventMixin,
@@ -158,16 +152,6 @@ class SupereventLabelViewSet(GetParentSupereventMixin,
         queryset = superevent.labelling_set.all()
         return queryset
 
-    def destroy(self, request, *args, **kwargs):
-        instance = self.get_object()
-        try:
-            self.perform_destroy(instance)
-        except Exception as e:
-            return Response(e.__str__(),
-                status=status.HTTP_500_INTERNAL_SERVER_ERROR)
-
-        return Response(status=status.HTTP_204_NO_CONTENT)
-
     def perform_destroy(self, instance):
         remove_label_from_superevent(instance, self.request.user,
             add_log_message=True, issue_alert=True)
@@ -175,7 +159,7 @@ class SupereventLabelViewSet(GetParentSupereventMixin,
 
 class SupereventLogViewSet(mixins.ListModelMixin,
                            mixins.RetrieveModelMixin,
-                           mixins.CreateModelMixin,
+                           SafeCreateMixin,
                            GetParentSupereventMixin,
                            BaseGetObjectMixin,
                            viewsets.GenericViewSet):
@@ -197,7 +181,8 @@ class SupereventLogViewSet(mixins.ListModelMixin,
 
 class SupereventLogTagViewSet(GetParentSupereventMixin,
                               BaseGetObjectMixin,
-                              viewsets.ModelViewSet):
+                              viewsets.ModelViewSet,
+                              SafeCreateMixin):
     """
     View for tags attached to a log message which is attached to a superevent.
     """
@@ -216,16 +201,6 @@ class SupereventLogTagViewSet(GetParentSupereventMixin,
         parent_log = self.get_parent_log()
         return parent_log.tags.all()
 
-    def destroy(self, request, *args, **kwargs):
-        instance = self.get_object()
-        try:
-            self.perform_destroy(instance)
-        except Exception as e:
-            return Response(e.__str__(),
-                status=status.HTTP_500_INTERNAL_SERVER_ERROR)
-
-        return Response(status=status.HTTP_204_NO_CONTENT)
-
     def perform_destroy(self, instance):
         parent_log = self.get_parent_log()
         remove_tag_from_log(parent_log, instance, self.request.user,