Move worker configuration validation from api to serializer

arkindex/dataimport/api.py

@@ -86,7 +86,6 @@ from arkindex.dataimport.serializers.workers import (
     WorkerVersionEditSerializer,
     WorkerVersionSerializer,
 )
-from arkindex.dataimport.utils import hash_object
 from arkindex.documents.models import Corpus, Element
 from arkindex.project.aws import get_ingest_resource
 from arkindex.project.fields import ArrayRemove
@@ -1074,7 +1073,7 @@ class WorkerRetrieve(WorkerACLMixin, RetrieveAPIView):
             status_codes=['400'],
             response_only=True,
             name="config-exists",
-            value={'id': "01fd00d9-1020-4d84-85f2-d0785e2c5b4a", 'configuration': ["A worker configuration with this configuration already exists for this worker."]},
+            value={'id': ["01fd00d9-1020-4d84-85f2-d0785e2c5b4a"], 'configuration': ["A worker configuration with this configuration already exists for this worker."]},
             description="An error where a worker configuration with this configuration already exists, including the existing worker configuration's UUID."
         )]
     ),
@@ -1084,19 +1083,26 @@ class WorkerConfigurationList(WorkerACLMixin, ListCreateAPIView):
     serializer_class = WorkerConfigurationListSerializer
     queryset = WorkerConfiguration.objects.none()
 
-    def get_object(self):
-        return get_object_or_404(
+    @cached_property
+    def worker(self):
+        worker = get_object_or_404(
             Worker.objects.select_related('repository'),
             pk=self.kwargs['pk']
         )
-
-    def get_queryset(self):
-        worker = self.get_object()
-
-        if not self.has_read_access(worker):
+        if self.request.method in permissions.SAFE_METHODS and not self.has_read_access(worker):
             raise PermissionDenied(detail='You do not have a guest access to this worker.')
+        if self.request.method not in permissions.SAFE_METHODS and not self.has_execution_access(worker):
+            raise PermissionDenied(detail='You do not have contributor access to this worker.')
+        return worker
+
+    def get_serializer_context(self):
+        context = super().get_serializer_context()
+        if 'pk' in self.kwargs:
+            context['worker'] = self.worker
+        return context
 
-        return worker.configurations.order_by('name')
+    def get_queryset(self):
+        return self.worker.configurations.order_by('name')
 
     def filter_queryset(self, queryset):
         # Consider any value that does not look like a false, including the empty string (`?archived` alone),
@@ -1104,52 +1110,6 @@ class WorkerConfigurationList(WorkerACLMixin, ListCreateAPIView):
         archived = self.request.query_params.get('archived', 'false').lower() not in ('false', '0')
         return queryset.filter(archived=archived)
 
-    def create(self, request, *args, **kwargs):
-        worker = self.get_object()
-
-        if not self.has_execution_access(worker):
-            raise PermissionDenied(detail='You do not have contributor access to this worker.')
-
-        serializer = self.get_serializer(data=request.data)
-        serializer.is_valid(raise_exception=True)
-
-        # Check unique constraints
-        # TODO: Move all of this into the serializer itself; overriding CreateAPIView.create is a bad practice
-        name = serializer.validated_data['name']
-        configuration = serializer.validated_data['configuration']
-        configuration_hash = hash_object(configuration)
-        existing_configurations = list(worker.configurations.filter(Q(configuration_hash=configuration_hash) | Q(name=name)))
-        errors = defaultdict(list)
-        for config in existing_configurations:
-            if configuration_hash == config.configuration_hash:
-                errors['configuration'].append("A worker configuration with this configuration already exists for this worker.")
-                errors['id'] = config.id
-            if name == config.name:
-                errors['name'].append("A worker configuration with this name already exists for this worker.")
-        # Due to the defaultdict, writing e.g. len(errors['configuration']) > 1 would add {'configuration': []} to
-        # the errors dict if the key is not already in the dict.
-        if len(errors.get('name', [])) > 1 or len(errors.get('configuration', [])) > 1:
-            # If there is more than one configuration with the same name and/or configuration as the one the user
-            # is trying to create, raise a MultipleObjectsReturned exception because this shouldn't be possible
-            # due to the unicity constraints. There can however be more than one worker configurations in
-            # existing_configurations, if both a worker configuration with the same name and the same configuration
-            # exist; but checking the length of existing_configurations wouldn't differentiate between this case
-            # and an actual duplicated worker configuration.
-            raise WorkerConfiguration.MultipleObjectsReturned
-        if errors:
-            raise ValidationError(errors)
-
-        archived = serializer.validated_data.get('archived', False)
-
-        worker_configuration = WorkerConfiguration.objects.create(
-            worker=worker,
-            name=name,
-            configuration=configuration,
-            archived=archived,
-        )
-
-        return Response(WorkerConfigurationSerializer(worker_configuration).data, status=status.HTTP_201_CREATED)
-
 
 @extend_schema_view(
     get=extend_schema(description=dedent(

arkindex/dataimport/serializers/workers.py

@@ -2,6 +2,7 @@ import urllib
 from collections import defaultdict
 from enum import Enum
 
+from django.db.models import Q
 from drf_spectacular.utils import extend_schema_field
 from rest_framework import serializers
 from rest_framework.exceptions import ValidationError
@@ -20,6 +21,7 @@ from arkindex.dataimport.models import (
     WorkerVersionState,
 )
 from arkindex.dataimport.serializers.git import RevisionWithRefsSerializer
+from arkindex.dataimport.utils import hash_object
 from arkindex.project.serializer_fields import EnumField
 
 
@@ -312,9 +314,44 @@ class WorkerConfigurationListSerializer(serializers.ModelSerializer):
             'id',
             'name',
             'configuration',
-            'archived',
+            'archived'
         )
 
+    def validate(self, data):
+        # Check unique constraints
+        name = data.get('name')
+        configuration = data.get('configuration')
+        configuration_hash = hash_object(configuration)
+        data['archived'] = data.get('archived', False)
+        worker = self.context.get('worker')
+        # Since WorkerConfigurationSerializer is built from this serializer, this validation function can be triggered by a call
+        # to the WorkerConfigurationRetrieve API endpoint, updating an existing configuration. In this case, there is no worker
+        # cached property and this validation is not necessary anyway because the name and configuration fields are read_only.
+        if not worker:
+            return data
+        data['worker_id'] = worker.id
+        existing_configurations = list(worker.configurations.filter(Q(configuration_hash=configuration_hash) | Q(name=name)))
+        errors = defaultdict(list)
+        for config in existing_configurations:
+            if configuration_hash == config.configuration_hash:
+                errors['configuration'].append("A worker configuration with this configuration already exists for this worker.")
+                errors['id'] = config.id
+            if name == config.name:
+                errors['name'].append("A worker configuration with this name already exists for this worker.")
+        # Due to the defaultdict, writing e.g. len(errors['configuration']) > 1 would add {'configuration': []} to
+        # the errors dict if the key is not already in the dict.
+        if len(errors.get('name', [])) > 1 or len(errors.get('configuration', [])) > 1:
+            # If there is more than one configuration with the same name and/or configuration as the one the user
+            # is trying to create, raise a MultipleObjectsReturned exception because this shouldn't be possible
+            # due to the unicity constraints. There can however be more than one worker configurations in
+            # existing_configurations, if both a worker configuration with the same name and the same configuration
+            # exist; but checking the length of existing_configurations wouldn't differentiate between this case
+            # and an actual duplicated worker configuration.
+            raise WorkerConfiguration.MultipleObjectsReturned
+        if errors:
+            raise ValidationError(errors)
+        return data
+
 
 class WorkerConfigurationSerializer(WorkerConfigurationListSerializer):
     configuration = serializers.DictField(allow_empty=False, read_only=True)

arkindex/dataimport/tests/test_worker_configurations.py

@@ -210,7 +210,7 @@ class TestWorkerConfigurations(FixtureAPITestCase):
             response.json(),
             {
                 'configuration': ['A worker configuration with this configuration already exists for this worker.'],
-                'id': str(test_config.id),
+                'id': [str(test_config.id)],
             },
         )
 
@@ -230,7 +230,7 @@ class TestWorkerConfigurations(FixtureAPITestCase):
             response.json(),
             {
                 'configuration': ['A worker configuration with this configuration already exists for this worker.'],
-                'id': str(test_config.id),
+                'id': [str(test_config.id)],
                 'name': ['A worker configuration with this name already exists for this worker.']
             }
         )
@@ -253,7 +253,7 @@ class TestWorkerConfigurations(FixtureAPITestCase):
             response.json(),
             {
                 'configuration': ['A worker configuration with this configuration already exists for this worker.'],
-                'id': str(test_config_2.id),
+                'id': [str(test_config_2.id)],
                 'name': ['A worker configuration with this name already exists for this worker.']
             }
         )