Verify WorkerConfiguration name uniqueness on updates

arkindex/process/serializers/workers.py

@@ -337,14 +337,19 @@ class WorkerConfigurationListSerializer(serializers.ModelSerializer):
         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:
+
+        # No further validation required if we only sent `archived`
+        if not name and not configuration:
             return data
-        data['worker_id'] = worker.id
+
+        if 'worker' in self.context:
+            # CreateWorkerConfiguration provides the worker through the context
+            worker = self.context['worker']
+            data['worker_id'] = worker.id
+        elif self.instance is not None:
+            # Use the already set worker in (Partial)UpdateConfiguration to revalidate
+            worker = self.instance.worker
+
         existing_configurations = list(worker.configurations.filter(Q(configuration_hash=configuration_hash) | Q(name=name)))
         errors = defaultdict(list)
         for config in existing_configurations:
@@ -353,6 +358,7 @@ class WorkerConfigurationListSerializer(serializers.ModelSerializer):
                 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:
@@ -363,8 +369,15 @@ class WorkerConfigurationListSerializer(serializers.ModelSerializer):
             # exist; but checking the length of existing_configurations wouldn't differentiate between this case
             # and an actual duplicated worker configuration.
             raise WorkerConfiguration.MultipleObjectsReturned
+
+        # Archived configurations cannot be renamed, but un-archiving a configuration and renaming it at the same time is
+        # possible. It's also possible to archive and rename a configuration simultaneously.
+        if self.instance is not None and self.instance.archived and data.get('archived') is not False and name:
+            errors['name'].append('Archived configurations cannot be renamed.')
+
         if errors:
             raise ValidationError(errors)
+
         return data
 
 
@@ -375,16 +388,6 @@ class WorkerConfigurationSerializer(WorkerConfigurationListSerializer):
         # The configuration cannot be updated
         read_only_fields = ('id', 'configuration')
 
-    def validate(self, data):
-        name = data.get('name')
-        data_archived = data.get('archived')
-        instance_archived = self.instance.archived
-        # Archived configurations cannot be renamed, but un-archiving a configuration and renaming it at the same time is
-        # possible. It's also possible to archive and rename a configuration simultaneously.
-        if instance_archived and data_archived is not False and name:
-            raise ValidationError({'name': 'Archived configurations cannot be renamed.'})
-        return data
-
 
 class WorkerConfigurationExistsErrorSerializer(serializers.Serializer):
     id = serializers.UUIDField(required=False, help_text="UUID of an existing worker configuration, if the error comes from a duplicate configuration.")

arkindex/process/tests/test_worker_configurations.py

@@ -444,7 +444,7 @@ class TestWorkerConfigurations(FixtureAPITestCase):
 
         self.assertFalse(self.worker_config.archived)
 
-        with self.assertNumQueries(7):
+        with self.assertNumQueries(8):
             response = self.client.put(
                 reverse('api:configuration-retrieve', kwargs={'pk': str(self.worker_config.id)}),
                 data={'archived': True, 'name': 'new name'}
@@ -467,7 +467,7 @@ class TestWorkerConfigurations(FixtureAPITestCase):
 
         self.assertFalse(self.worker_config.archived)
 
-        with self.assertNumQueries(6):
+        with self.assertNumQueries(7):
             response = self.client.put(
                 reverse('api:configuration-retrieve', kwargs={'pk': str(self.worker_config.id)}),
                 data={'archived': True, 'name': 'new name'}
@@ -489,7 +489,7 @@ class TestWorkerConfigurations(FixtureAPITestCase):
 
         self.assertFalse(self.worker_config.archived)
 
-        with self.assertNumQueries(4):
+        with self.assertNumQueries(5):
             response = self.client.put(
                 reverse('api:configuration-retrieve', kwargs={'pk': str(self.worker_config.id)}),
                 data={'archived': True, 'name': 'a new name'}
@@ -513,7 +513,7 @@ class TestWorkerConfigurations(FixtureAPITestCase):
 
         self.assertFalse(self.worker_config.archived)
 
-        with self.assertNumQueries(4):
+        with self.assertNumQueries(5):
             response = self.client.put(
                 reverse('api:configuration-retrieve', kwargs={'pk': str(self.worker_config.id)}),
                 data={
@@ -540,7 +540,7 @@ class TestWorkerConfigurations(FixtureAPITestCase):
         self.worker_config.save()
         self.assertTrue(self.worker_config.archived)
 
-        with self.assertNumQueries(8):
+        with self.assertNumQueries(9):
             response = self.client.put(
                 reverse('api:configuration-retrieve', kwargs={'pk': str(self.worker_config.id)}),
                 data={
@@ -568,7 +568,7 @@ class TestWorkerConfigurations(FixtureAPITestCase):
         self.worker_config.save()
         self.assertTrue(self.worker_config.archived)
 
-        with self.assertNumQueries(9):
+        with self.assertNumQueries(10):
             response = self.client.put(
                 reverse('api:configuration-retrieve', kwargs={'pk': str(self.worker_config.id)}),
                 data={
@@ -583,6 +583,29 @@ class TestWorkerConfigurations(FixtureAPITestCase):
         self.assertEqual(self.worker_config.name, 'new name')
         self.assertFalse(self.worker_config.archived)
 
+    def test_update_name_exists(self):
+        """
+        Updating the name of a configuration to the name of another existing configuration on the same worker
+        should fail as this would break the unique constraint
+        """
+        self.worker_1.configurations.create(name='new name', configuration={'key': 'value', 'key2': 'value2'})
+        self.client.force_login(self.user)
+
+        with self.assertNumQueries(9):
+            response = self.client.put(
+                reverse('api:configuration-retrieve', kwargs={'pk': str(self.worker_config.id)}),
+                data={
+                    'name': 'new name',
+                    'archived': False,
+                },
+                format='json',
+            )
+            self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST)
+
+        self.assertEqual(response.json(), {
+            'name': ['A worker configuration with this name already exists for this worker.'],
+        })
+
     def test_partial_update_requires_login(self):
         with self.assertNumQueries(0):
             response = self.client.patch(
@@ -721,7 +744,7 @@ class TestWorkerConfigurations(FixtureAPITestCase):
 
         self.assertFalse(self.worker_config.archived)
 
-        with self.assertNumQueries(4):
+        with self.assertNumQueries(5):
             response = self.client.patch(
                 reverse('api:configuration-retrieve', kwargs={'pk': str(self.worker_config.id)}),
                 data={
@@ -748,7 +771,7 @@ class TestWorkerConfigurations(FixtureAPITestCase):
         self.worker_config.save()
         self.assertTrue(self.worker_config.archived)
 
-        with self.assertNumQueries(8):
+        with self.assertNumQueries(9):
             response = self.client.patch(
                 reverse('api:configuration-retrieve', kwargs={'pk': str(self.worker_config.id)}),
                 data={
@@ -765,3 +788,25 @@ class TestWorkerConfigurations(FixtureAPITestCase):
         self.worker_config.refresh_from_db()
         self.assertEqual(self.worker_config.name, 'config time')
         self.assertTrue(self.worker_config.archived)
+
+    def test_partial_update_name_exists(self):
+        """
+        Updating the name of a configuration to the name of another existing configuration on the same worker
+        should fail as this would break the unique constraint
+        """
+        self.worker_1.configurations.create(name='new name', configuration={'key': 'value', 'key2': 'value2'})
+        self.client.force_login(self.user)
+
+        with self.assertNumQueries(9):
+            response = self.client.patch(
+                reverse('api:configuration-retrieve', kwargs={'pk': str(self.worker_config.id)}),
+                data={
+                    'name': 'new name',
+                },
+                format='json',
+            )
+            self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST)
+
+        self.assertEqual(response.json(), {
+            'name': ['A worker configuration with this name already exists for this worker.'],
+        })