Fix /api/tasks leads to 500 server error (#5700)

- Fixed re-deleting of skeleton sublabels (attempt to remove already
removed sublabels)

![Screenshot from 2023-02-13
10-00-03](https://user-images.githubusercontent.com/49231913/218693868-0afcde6a-935a-4d3f-bfa4-c1793a367f90.png)

- Updated Label model (added `project` to `unique_together`)
- Added migration to remove or rename wrong labels

CHANGELOG.md

@@ -71,6 +71,7 @@ Tracks can be exported/imported to/from Datumaro and Sly Pointcloud formats (<ht
 - Submit button is locked while file is not selected when importing a dataset (<https://github.com/opencv/cvat/pull/5757>)
 - \[Server API\] Various errors in the generated schema (<https://github.com/opencv/cvat/pull/5575>)
 - SiamMask and TransT serverless functions (<https://github.com/opencv/cvat/pull/5658>)
+- Сreating a project or task with the same labels (<https://github.com/opencv/cvat/pull/5700>)
 - \[Server API\] Ability to rename label to an existing name (<https://github.com/opencv/cvat/pull/5662>)
 
 ### Security

cvat/apps/engine/migrations/0064_delete_or_rename_wrong_labels.py

@@ -0,0 +1,41 @@
+import os
+
+from django.db import migrations
+from cvat.apps.engine.log import get_migration_logger
+
+def delete_or_rename_wrong_labels(apps, schema_editor):
+    migration_name = os.path.splitext(os.path.basename(__file__))[0]
+    with get_migration_logger(migration_name) as log:
+        log.info('\nDeleting skeleton Labels without skeletons...')
+
+        Label = apps.get_model('engine', 'Label')
+        for label in Label.objects.all():
+            if label.type == "skeleton" and not hasattr(label, "skeleton"):
+                label.delete()
+
+        log.info('\nDeleting duplicate skeleton sublabels and renaming duplicate Labels...')
+        for name, parent, project, task in Label.objects.values_list("name", "parent", "project", "task").distinct():
+            duplicate_labels = Label.objects.filter(name=name, parent=parent, project=project)
+            if task is not None:
+                duplicate_labels = Label.objects.filter(name=name, parent=parent, task=task)
+
+            if len(duplicate_labels) > 1:
+                label = duplicate_labels[0]
+                if label.parent is not None:
+                    label.delete()
+                else:
+                    for i, label in enumerate(duplicate_labels[1:]):
+                        label.name = f"{label.name}_duplicate_{i + 1}"
+                        label.save()
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('engine', '0063_delete_jobcommit'),
+    ]
+
+    operations = [
+        migrations.RunPython(
+            code=delete_or_rename_wrong_labels
+        ),
+    ]

cvat/apps/engine/migrations/0065_auto_20230221_0931.py

@@ -0,0 +1,33 @@
+# Generated by Django 3.2.18 on 2023-02-21 09:31
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('engine', '0064_delete_or_rename_wrong_labels'),
+    ]
+
+    operations = [
+        migrations.AlterUniqueTogether(
+            name='label',
+            unique_together=set(),
+        ),
+        migrations.AddConstraint(
+            model_name='label',
+            constraint=models.UniqueConstraint(condition=models.Q(('parent__isnull', True), ('task__isnull', True)), fields=('project', 'name'), name='project_name_unique'),
+        ),
+        migrations.AddConstraint(
+            model_name='label',
+            constraint=models.UniqueConstraint(condition=models.Q(('parent__isnull', True), ('project__isnull', True)), fields=('task', 'name'), name='task_name_unique'),
+        ),
+        migrations.AddConstraint(
+            model_name='label',
+            constraint=models.UniqueConstraint(condition=models.Q(('task__isnull', True)), fields=('project', 'name', 'parent'), name='project_name_parent_unique'),
+        ),
+        migrations.AddConstraint(
+            model_name='label',
+            constraint=models.UniqueConstraint(condition=models.Q(('project__isnull', True)), fields=('task', 'name', 'parent'), name='task_name_parent_unique'),
+        ),
+    ]

cvat/apps/engine/models.py

@@ -12,13 +12,15 @@
 from django.conf import settings
 from django.contrib.auth.models import User
 from django.core.files.storage import FileSystemStorage
-from django.db import models
+from django.db import IntegrityError, models
 from django.db.models.fields import FloatField
 from drf_spectacular.types import OpenApiTypes
 from drf_spectacular.utils import extend_schema_field
+
 from cvat.apps.engine.utils import parse_specific_attributes
 from cvat.apps.organizations.models import Organization
 
+
 class SafeCharField(models.CharField):
     def get_prep_value(self, value):
         value = super().get_prep_value(value)
@@ -502,7 +504,6 @@ def get_labels(self):
     class Meta:
         default_permissions = ()
 
-
 class InvalidLabel(ValueError):
     pass
 
@@ -520,28 +521,43 @@ def __str__(self):
     def has_parent_label(self):
         return bool(self.parent)
 
-    def _check_save_constraints(self) -> None:
-        # NOTE: constraints don't work for some reason
-        # https://github.com/opencv/cvat/pull/5700#discussion_r1112276036
-        # This method is not 100% reliable because of possible race conditions
-        # but it should work in relevant cases.
+    def save(self, *args, **kwargs):
+        try:
+            super().save(*args, **kwargs)
+        except IntegrityError:
+            raise InvalidLabel("All label names must be unique")
 
-        parent_entity = self.project or self.task
-
-        # Check for possible labels name duplicates in case of saving the new label
-        existing_labels: models.QuerySet = parent_entity.get_labels()
-        if self.id:
-            existing_labels = existing_labels.exclude(id=self.id)
-        if existing_labels.filter(name=self.name).count():
-            raise InvalidLabel(f"Label '{self.name}' already exists")
-
-    def save(self, *args, **kwargs) -> None:
-        self._check_save_constraints()
-        return super().save(*args, **kwargs)
+    @classmethod
+    def create(cls, **kwargs):
+        try:
+            return cls.objects.create(**kwargs)
+        except IntegrityError:
+            raise InvalidLabel("All label names must be unique")
 
     class Meta:
         default_permissions = ()
-        unique_together = ('task', 'name', 'parent')
+        constraints = [
+            models.UniqueConstraint(
+                name='project_name_unique',
+                fields=('project', 'name'),
+                condition=models.Q(task__isnull=True, parent__isnull=True)
+            ),
+            models.UniqueConstraint(
+                name='task_name_unique',
+                fields=('task', 'name'),
+                condition=models.Q(project__isnull=True, parent__isnull=True)
+            ),
+            models.UniqueConstraint(
+                name='project_name_parent_unique',
+                fields=('project', 'name', 'parent'),
+                condition=models.Q(task__isnull=True)
+            ),
+            models.UniqueConstraint(
+                name='task_name_parent_unique',
+                fields=('task', 'name', 'parent'),
+                condition=models.Q(project__isnull=True)
+            )
+        ]
 
 class Skeleton(models.Model):
     root = models.OneToOneField(Label, on_delete=models.CASCADE)

cvat/apps/engine/serializers.py

@@ -250,6 +250,7 @@ def validate(self, attrs):
         return attrs
 
     @classmethod
+    @transaction.atomic
     def update_label(
         cls,
         validated_data: Dict[str, Any],
@@ -287,12 +288,15 @@ def update_label(
 
             logger.info("Label id {} ({}) was updated".format(db_label.id, db_label.name))
         else:
-            db_label = models.Label.objects.create(
-                name=validated_data.get('name'),
-                type=validated_data.get('type'),
-                parent=parent_label,
-                **parent_info
-            )
+            try:
+                db_label = models.Label.create(
+                    name=validated_data.get('name'),
+                    type=validated_data.get('type'),
+                    parent=parent_label,
+                    **parent_info
+                )
+            except models.InvalidLabel as exc:
+                raise exceptions.ValidationError(str(exc)) from exc
             logger.info("New {} label was created".format(db_label.name))
 
         if validated_data.get('deleted'):
@@ -358,7 +362,10 @@ def create_labels(cls,
 
             sublabels = label.pop('sublabels', [])
             svg = label.pop('svg', '')
-            db_label = models.Label.objects.create(**label, **parent_info, parent=parent_label)
+            try:
+                db_label = models.Label.create(**label, **parent_info, parent=parent_label)
+            except models.InvalidLabel as exc:
+                raise exceptions.ValidationError(str(exc)) from exc
             logger.info(
                 f'label:create Label id:{db_label.id} for spec:{label} '
                 f'with sublabels:{sublabels}, parent_label:{parent_label}'
@@ -836,6 +843,7 @@ def to_representation(self, instance):
         return serializer.data
 
     # pylint: disable=no-self-use
+    @transaction.atomic
     def create(self, validated_data):
         project_id = validated_data.get("project_id")
         if not (validated_data.get("label_set") or project_id):
@@ -878,6 +886,7 @@ def create(self, validated_data):
         return db_task
 
     # pylint: disable=no-self-use
+    @transaction.atomic
     def update(self, instance, validated_data):
         instance.name = validated_data.get('name', instance.name)
         instance.owner_id = validated_data.get('owner_id', instance.owner_id)
@@ -990,20 +999,9 @@ def validate(self, attrs):
             for label, sublabels in new_sublabel_names.items():
                 if sublabels != target_project_sublabel_names.get(label):
                     raise serializers.ValidationError('All task or project label names must be mapped to the target project')
-        else:
-            if 'label_set' in attrs.keys():
-                # FIXME: doesn't work for renaming just a single label
-                label_names = [
-                    label['name']
-                    for label in attrs.get('label_set')
-                    if not label.get('deleted')
-                ]
-                if len(label_names) != len(set(label_names)):
-                    raise serializers.ValidationError('All label names must be unique for the task')
 
         return attrs
 
-
 class ProjectReadSerializer(serializers.ModelSerializer):
     owner = BasicUserSerializer(required=False, read_only=True)
     assignee = BasicUserSerializer(allow_null=True, required=False, read_only=True)
@@ -1052,6 +1050,7 @@ def to_representation(self, instance):
         return serializer.data
 
     # pylint: disable=no-self-use
+    @transaction.atomic
     def create(self, validated_data):
         labels = validated_data.pop('label_set')
 
@@ -1075,6 +1074,7 @@ def create(self, validated_data):
         return db_project
 
     # pylint: disable=no-self-use
+    @transaction.atomic
     def update(self, instance, validated_data):
         instance.name = validated_data.get('name', instance.name)
         instance.owner_id = validated_data.get('owner_id', instance.owner_id)
@@ -1090,18 +1090,6 @@ def update(self, instance, validated_data):
         instance.save()
         return instance
 
-    def validate_labels(self, value):
-        if value:
-            # FIXME: doesn't work for renaming just a single label
-            label_names = [
-                label['name']
-                for label in value
-                if not label.get('deleted')
-            ]
-            if len(label_names) != len(set(label_names)):
-                raise serializers.ValidationError('All label names must be unique for the project')
-        return value
-
 class AboutSerializer(serializers.Serializer):
     name = serializers.CharField(max_length=128)
     description = serializers.CharField(max_length=2048)

tests/python/rest_api/test_labels.py

@@ -698,7 +698,7 @@ def test_cannot_rename_label_to_duplicate_name(self, source_type, user):
         response = self._test_update_denied(
             user, lid=labels[0]["id"], data=payload, expected_status=HTTPStatus.BAD_REQUEST
         )
-        assert f"Label '{payload['name']}' already exists" in response.data.decode()
+        assert "All label names must be unique" in response.data.decode()
 
     def test_admin_patch_sandbox_label(self, admin_sandbox_case):
         label, user = get_attrs(admin_sandbox_case, ["label", "user"])

tests/python/rest_api/test_projects.py

@@ -21,7 +21,14 @@
 from deepdiff import DeepDiff
 from PIL import Image
 
-from shared.utils.config import BASE_URL, USER_PASS, get_method, make_api_client, patch_method
+from shared.utils.config import (
+    BASE_URL,
+    USER_PASS,
+    get_method,
+    make_api_client,
+    patch_method,
+    post_method,
+)
 
 from .utils import CollectionSimpleFilterTestBase, export_dataset
 
@@ -390,6 +397,30 @@ def _create_org(cls, api_client: ApiClient, members: Optional[Dict[str, str]] =
 
         return org
 
+    def test_cannot_create_project_with_same_labels(self, admin_user):
+        project_spec = {
+            "name": "test cannot create project with same labels",
+            "labels": [{"name": "l1"}, {"name": "l1"}],
+        }
+        response = post_method(admin_user, "/projects", project_spec)
+        assert response.status_code == HTTPStatus.BAD_REQUEST
+
+        response = get_method(admin_user, "/projects")
+        assert response.status_code == HTTPStatus.OK
+
+    def test_cannot_create_project_with_same_skeleton_sublabels(self, admin_user):
+        project_spec = {
+            "name": "test cannot create project with same skeleton sublabels",
+            "labels": [
+                {"name": "s1", "type": "skeleton", "sublabels": [{"name": "1"}, {"name": "1"}]}
+            ],
+        }
+        response = post_method(admin_user, "/projects", project_spec)
+        assert response.status_code == HTTPStatus.BAD_REQUEST
+
+        response = get_method(admin_user, "/projects")
+        assert response.status_code == HTTPStatus.OK
+
 
 def _check_cvat_for_video_project_annotations_meta(content, values_to_be_checked):
     document = ET.fromstring(content)
@@ -689,7 +720,7 @@ def test_cannot_rename_label_to_duplicate_name(self, projects, labels, admin_use
             admin_user, f'/projects/{project["id"]}', {"labels": [label_payload]}
         )
         assert response.status_code == HTTPStatus.BAD_REQUEST
-        assert f"Label '{project_labels[0]['name']}' already exists" in response.text
+        assert "All label names must be unique" in response.text
 
     def test_cannot_add_foreign_label(self, projects, labels, admin_user):
         project = list(projects)[0]