From a912f587e76508462f85d82e67e7d1f0ca182a9a Mon Sep 17 00:00:00 2001
From: Erwan Rouchet <rouchet@teklia.com>
Date: Thu, 13 Apr 2023 15:36:33 +0200
Subject: [PATCH] Verify WorkerConfiguration name uniqueness on updates
---
arkindex/process/serializers/workers.py | 37 +++++------
.../tests/test_worker_configurations.py | 61 ++++++++++++++++---
2 files changed, 73 insertions(+), 25 deletions(-)
diff --git a/arkindex/process/serializers/workers.py b/arkindex/process/serializers/workers.py
index 3e74f28b71..bc6911790a 100644
--- a/arkindex/process/serializers/workers.py
+++ b/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.")
diff --git a/arkindex/process/tests/test_worker_configurations.py b/arkindex/process/tests/test_worker_configurations.py
index 11921ec841..a43e80f17d 100644
--- a/arkindex/process/tests/test_worker_configurations.py
+++ b/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.'],
+ })
--
GitLab