Refactor update creation api

This merge request does the following:

• Raise DRF errors rather than returning response to take advantage of the DRF exception handling mechanism
• Create serialisers for PUT request to simplify the code
• Improve validation checks in several places
• Fixes several issues in DRF infrastructure uses
• Fixes JSON fields handling
• Remove routes that do no fully qualify objects
• Implement a custom DRF exception handler that will allow to log errors
• Improve testing
• Improve permission management

beat/web/common/api.py

@@ -29,27 +29,33 @@ from django.shortcuts import get_object_or_404
 
 from rest_framework import status
 from rest_framework import generics
-from rest_framework import permissions
+from rest_framework import permissions as drf_permissions
+from rest_framework import exceptions as drf_exceptions
 from rest_framework.response import Response
 from rest_framework.reverse import reverse
 
-from .responses import BadRequestResponse, ForbiddenResponse
 from .models import Contribution, Versionable
-from .permissions import IsAuthor
 from .exceptions import ShareError, BaseCreationError
-from .serializers import SharingSerializer, ContributionSerializer, CheckNameSerializer, DiffSerializer
-from .mixins import CommonContextMixin, SerializerFieldsMixin, IsAuthorOrReadOnlyMixin
-
+from .serializers import (
+    SharingSerializer,
+    ContributionSerializer,
+    CheckNameSerializer,
+    DiffSerializer,
+)
+from .mixins import CommonContextMixin, SerializerFieldsMixin
+from .utils import py3_cmp
+
+from . import permissions as beat_permissions
 from . import is_true
 
 
 class CheckContributionNameView(CommonContextMixin, generics.CreateAPIView):
     serializer_class = CheckNameSerializer
-    permission_classes = [permissions.IsAuthenticated]
+    permission_classes = [drf_permissions.IsAuthenticated]
 
     def get_serializer_context(self):
         context = super(CheckContributionNameView, self).get_serializer_context()
-        context['model'] = self.model
+        context["model"] = self.model
         return context
 
     def post(self, request):
@@ -59,28 +65,28 @@ class CheckContributionNameView(CommonContextMixin, generics.CreateAPIView):
 
 
 class ShareView(CommonContextMixin, generics.CreateAPIView):
-    permission_classes = [permissions.IsAuthenticated, IsAuthor]
+    permission_classes = [beat_permissions.IsAuthor]
     serializer_class = SharingSerializer
 
     def get_queryset(self):
-        author_name = self.kwargs.get('author_name')
-        object_name = self.kwargs.get('object_name')
-        version = self.kwargs.get('version')
-        return get_object_or_404(self.model,
-                                 author__username__iexact=author_name,
-                                 name__iexact=object_name,
-                                 version=version)
+        author_name = self.kwargs.get("author_name")
+        object_name = self.kwargs.get("object_name")
+        version = self.kwargs.get("version")
+        return get_object_or_404(
+            self.model,
+            author__username__iexact=author_name,
+            name__iexact=object_name,
+            version=version,
+        )
 
     def do_share(self, obj, data):
-        users = data.get('users', None)
-        teams = data.get('teams', None)
+        users = data.get("users", None)
+        teams = data.get("teams", None)
         obj.share(users=users, teams=teams)
 
     def post(self, request, *args, **kwargs):
         serializer = self.get_serializer(data=request.data)
-        if not serializer.is_valid():
-            return BadRequestResponse(serializer.errors)
-
+        serializer.is_valid(raise_exception=True)
         data = serializer.data
 
         object_db = self.get_queryset()
@@ -88,48 +94,61 @@ class ShareView(CommonContextMixin, generics.CreateAPIView):
         try:
             self.do_share(object_db, data)
         except ShareError as e:
-            return BadRequestResponse(e.errors)
+            # ShareError happens when someone does not have access to
+            #  all dependencies that should be shared.
+            raise drf_exceptions.PermissionDenied(e.errors)
 
         return Response(object_db.sharing_preferences())
 
 
-class ListContributionView(CommonContextMixin, SerializerFieldsMixin, generics.ListAPIView):
+class ListContributionView(
+    CommonContextMixin, SerializerFieldsMixin, generics.ListAPIView
+):
     model = Contribution
     serializer_class = ContributionSerializer
-    permission_classes = [permissions.AllowAny]
+    permission_classes = [drf_permissions.AllowAny]
 
     def get_queryset(self):
         return self.model.objects.for_user(self.request.user, True)
 
     def get(self, request, *args, **kwargs):
         fields_to_return = self.get_serializer_fields(request)
-        limit_to_latest_versions = is_true(request.query_params.get('latest_versions', False))
+        limit_to_latest_versions = is_true(
+            request.query_params.get("latest_versions", False)
+        )
 
         all_contributions = self.get_queryset().select_related()
-        if hasattr(self.model, 'author'):
-            all_contributions = all_contributions.order_by('author__username', 'name', '-version')
+        if hasattr(self.model, "author"):
+            all_contributions = all_contributions.order_by(
+                "author__username", "name", "-version"
+            )
         else:
-            all_contributions = all_contributions.order_by('name', '-version')
+            all_contributions = all_contributions.order_by("name", "-version")
 
         if limit_to_latest_versions:
             all_contributions = self.model.filter_latest_versions(all_contributions)
             # Sort the data formats and sends the response
-            all_contributions.sort(lambda x, y: cmp(x.fullname(), y.fullname()))
+            all_contributions.sort(lambda x, y: py3_cmp(x.fullname(), y.fullname()))
 
-        serializer = self.get_serializer(all_contributions, many=True, fields=fields_to_return)
+        serializer = self.get_serializer(
+            all_contributions, many=True, fields=fields_to_return
+        )
         return Response(serializer.data)
 
 
-class ListCreateBaseView(CommonContextMixin, SerializerFieldsMixin, generics.ListCreateAPIView):
-
+class ListCreateBaseView(
+    CommonContextMixin, SerializerFieldsMixin, generics.ListCreateAPIView
+):
     def get_serializer(self, *args, **kwargs):
-        if self.request.method == 'POST':
+        if self.request.method == "POST":
             self.serializer_class = self.writing_serializer_class
         return super(ListCreateBaseView, self).get_serializer(*args, **kwargs)
 
     def get(self, request, *args, **kwargs):
         fields_to_return = self.get_serializer_fields(request)
-        limit_to_latest_versions = is_true(request.query_params.get('latest_versions', False))
+        limit_to_latest_versions = is_true(
+            request.query_params.get("latest_versions", False)
+        )
 
         objects = self.get_queryset().select_related()
 
@@ -141,38 +160,38 @@ class ListCreateBaseView(CommonContextMixin, SerializerFieldsMixin, generics.Lis
 
     def post(self, request, *args, **kwargs):
         serializer = self.get_serializer(data=request.data)
-        if serializer.is_valid():
-            try:
-                if hasattr(self.model, 'author'):
-                    db_object = serializer.save(author=request.user)
-                else:
-                    db_object = serializer.save()
-            except BaseCreationError as e:
-                return BadRequestResponse(e.errors)
-        else:
-            return BadRequestResponse(serializer.errors)
-
-        html_view_args = [db_object.name, db_object.version]
-        if hasattr(db_object, "author"):
-            html_view_args.insert(0, db_object.author.username)
+        serializer.is_valid(raise_exception=True)
+        try:
+            if hasattr(self.model, "author"):
+                db_object = serializer.save(author=request.user)
+            else:
+                db_object = serializer.save()
+        except BaseCreationError as e:
+            raise drf_exceptions.APIException(e.errors)
 
         result = {
-            'name': db_object.name,
-            'full_name': db_object.fullname(),
-            'url': reverse('{}:all'.format(self.namespace)) + db_object.fullname() + '/',
-            'object_view': reverse('{}:view'.format(self.namespace.split('_')[1]), args=html_view_args),
+            "name": db_object.name,
+            "full_name": db_object.fullname(),
+            "url": reverse("{}:all".format(self.namespace))
+            + db_object.fullname()
+            + "/",
+            "object_view": reverse(
+                "{}:view".format(self.namespace.split("_")[1]),
+                args=db_object.fullname().split("/"),
+            ),
         }
 
         response = Response(result, status=201)
-        response['Location'] = result['url']
+        response["Location"] = result["url"]
         return response
 
 
-class ListCreateContributionView(IsAuthorOrReadOnlyMixin, ListCreateBaseView):
+class ListCreateContributionView(ListCreateBaseView):
+    permission_classes = [beat_permissions.IsAuthorOrReadOnly]
 
     def get_queryset(self):
         user = self.request.user
-        author_name = self.kwargs.get('author_name')
+        author_name = self.kwargs.get("author_name")
         return self.model.objects.from_author_and_public(user, author_name)
 
 
@@ -182,96 +201,83 @@ class DiffView(generics.RetrieveAPIView):
 
     def get(self, request, author1, name1, version1, author2, name2, version2):
         # Retrieve the objects
-        try:
-            object1 = self.model.objects.get(author__username__iexact=author1,
-                                             name__iexact=name1,
-                                             version=int(version1))
-        except:
-            return Response('%s/%s/%s' % (author1, name1, version1), status=404)
-
-        try:
-            object2 = self.model.objects.get(author__username__iexact=author2,
-                                             name__iexact=name2,
-                                             version=int(version2))
-        except:
-            return Response('%s/%s/%s' % (author2, name2, version2), status=404)
-
+        object1 = get_object_or_404(
+            self.model,
+            author__username__iexact=author1,
+            name__iexact=name1,
+            version=int(version1),
+        )
+
+        object2 = get_object_or_404(
+            self.model,
+            author__username__iexact=author2,
+            name__iexact=name2,
+            version=int(version2),
+        )
 
         # Check that the user can access them
         accessibility = object1.accessibility_for(request.user)
         if not accessibility[0]:
-            return ForbiddenResponse(object1.fullname())
+            raise drf_exceptions.PermissionDenied(object1.fullname())
 
         accessibility = object2.accessibility_for(request.user)
         if not accessibility[0]:
-            return ForbiddenResponse(object2.fullname())
+            raise drf_exceptions.PermissionDenied(object2.fullname())
 
         # Compute the diff
-        serializer = self.get_serializer({'object1': object1,
-                                          'object2': object2})
+        serializer = self.get_serializer({"object1": object1, "object2": object2})
         return Response(serializer.data)
 
 
-class RetrieveUpdateDestroyContributionView(CommonContextMixin, SerializerFieldsMixin, IsAuthorOrReadOnlyMixin, generics.RetrieveUpdateDestroyAPIView):
+class RetrieveUpdateDestroyContributionView(
+    CommonContextMixin, SerializerFieldsMixin, generics.RetrieveUpdateDestroyAPIView
+):
     model = Contribution
+    permission_classes = [
+        beat_permissions.IsAuthorOrReadOnly,
+        beat_permissions.IsModifiableOrRead,
+    ]
 
-    def get_queryset(self):
-        version = self.kwargs.get('version', None)
-        author_name = self.kwargs.get('author_name')
-        object_name = self.kwargs.get('object_name')
-        user = self.request.user
-        if version is not None:
-            queryset = self.model.objects.for_user(user, True).filter(author__username__iexact=author_name,
-                                                                      name__iexact=object_name,
-                                                                      version__gte=version)\
-                .order_by('version')
-        else:
-            queryset = self.model.objects.for_user(user, True).filter(author__username__iexact=author_name,
-                                                                      name__iexact=object_name).order_by('-version')
+    def get_serializer(self, *args, **kwargs):
+        if self.request.method == "PUT":
+            self.serializer_class = self.writing_serializer_class
+        return super().get_serializer(*args, **kwargs)
 
-        return queryset
+    def get_object(self):
+        version = self.kwargs["version"]
+        author_name = self.kwargs["author_name"]
+        object_name = self.kwargs["object_name"]
+        user = self.request.user
+        try:
+            obj = self.model.objects.for_user(user, True).get(
+                author__username__iexact=author_name,
+                name__iexact=object_name,
+                version=version,
+            )
+        except self.model.DoesNotExist:
+            raise drf_exceptions.NotFound()
+        return obj
 
     def get(self, request, *args, **kwargs):
-        db_objects = self.get_queryset()
-
-        if db_objects.count() == 0:
-            return Response(status=404)
-
-
-        db_object = db_objects[0]
-        version = int(self.kwargs.get('version', -1))
-
-        if version != -1 and db_object.version != version:
-            return Response(status=404)
+        db_object = self.get_object()
+        self.check_object_permissions(request, db_object)
 
         # Process the query string
-        allow_sharing = hasattr(db_object, 'author') and (request.user == db_object.author)
+        allow_sharing = request.user == db_object.author
 
-        fields_to_return = self.get_serializer_fields(request, allow_sharing=allow_sharing)
+        fields_to_return = self.get_serializer_fields(
+            request, allow_sharing=allow_sharing
+        )
 
         serializer = self.get_serializer(db_object, fields=fields_to_return)
         return Response(serializer.data)
 
-
-    def delete(self, request, *args, **kwargs):
-        author_name = self.kwargs.get('author_name')
-        object_name = self.kwargs.get('object_name')
-        version = self.kwargs.get('version', None)
-
-        # Retrieve the object
-        if version is None:
-            return BadRequestResponse('A version number must be provided')
-
-        db_object = get_object_or_404(self.model,
-                                      author__username__iexact=author_name,
-                                      name__iexact=object_name,
-                                      version=version)
-
+    def perform_destroy(self, instance):
         # Check that the object can be deleted
-        if not(db_object.deletable()):
-            return ForbiddenResponse("The {} can't be deleted anymore (needed by an attestation, an algorithm or another data format)".format(db_object.model_name()))
-
-
-        # Deletion of the object
-        db_object.delete()
-        return Response(status=204)
+        if not (instance.deletable()):
+            raise drf_exceptions.MethodNotAllowed(
+                "The {} can't be deleted anymore (needed by an attestation, an algorithm or another data format)".format(
+                    instance.model_name()
+                )
+            )
+        return super().perform_destroy(instance)