diff --git a/CHANGELOG.md b/CHANGELOG.md index 93df1e8e9443..812385d7b31b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -70,6 +70,7 @@ Tracks can be exported/imported to/from Datumaro and Sly Pointcloud formats () - \[Server API\] Various errors in the generated schema () - SiamMask and TransT serverless functions () +- Сreating a project or task with the same labels () - \[Server API\] Ability to rename label to an existing name () ### Security diff --git a/cvat/apps/engine/migrations/0064_delete_or_rename_wrong_labels.py b/cvat/apps/engine/migrations/0064_delete_or_rename_wrong_labels.py new file mode 100644 index 000000000000..63c167381529 --- /dev/null +++ b/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 + ), + ] diff --git a/cvat/apps/engine/migrations/0065_auto_20230221_0931.py b/cvat/apps/engine/migrations/0065_auto_20230221_0931.py new file mode 100644 index 000000000000..f1b09c4ca237 --- /dev/null +++ b/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'), + ), + ] diff --git a/cvat/apps/engine/models.py b/cvat/apps/engine/models.py index 8313148a6c4c..db97e6dfa133 100644 --- a/cvat/apps/engine/models.py +++ b/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) diff --git a/cvat/apps/engine/serializers.py b/cvat/apps/engine/serializers.py index edcdb27fbfd5..15b0f5c787f0 100644 --- a/cvat/apps/engine/serializers.py +++ b/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) diff --git a/tests/python/rest_api/test_labels.py b/tests/python/rest_api/test_labels.py index ba1985d3886a..6ba36d132c71 100644 --- a/tests/python/rest_api/test_labels.py +++ b/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"]) diff --git a/tests/python/rest_api/test_projects.py b/tests/python/rest_api/test_projects.py index a5453b086e4c..09c7c6bfb688 100644 --- a/tests/python/rest_api/test_projects.py +++ b/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] diff --git a/tests/python/rest_api/test_tasks.py b/tests/python/rest_api/test_tasks.py index e496cfa5251e..a4d63ff05ede 100644 --- a/tests/python/rest_api/test_tasks.py +++ b/tests/python/rest_api/test_tasks.py @@ -27,7 +27,14 @@ import shared.utils.s3 as s3 from shared.fixtures.init import get_server_image_tag -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 shared.utils.helpers import generate_image_files from .utils import CollectionSimpleFilterTestBase, export_dataset @@ -929,6 +936,30 @@ def test_can_specify_file_job_mapping(self): start_frame = stop_frame + 1 + def test_cannot_create_task_with_same_labels(self): + task_spec = { + "name": "test cannot create task with same labels", + "labels": [{"name": "l1"}, {"name": "l1"}], + } + response = post_method(self._USERNAME, "/tasks", task_spec) + assert response.status_code == HTTPStatus.BAD_REQUEST + + response = get_method(self._USERNAME, "/tasks") + assert response.status_code == HTTPStatus.OK + + def test_cannot_create_task_with_same_skeleton_sublabels(self): + task_spec = { + "name": "test cannot create task with same skeleton sublabels", + "labels": [ + {"name": "s1", "type": "skeleton", "sublabels": [{"name": "1"}, {"name": "1"}]} + ], + } + response = post_method(self._USERNAME, "/tasks", task_spec) + assert response.status_code == HTTPStatus.BAD_REQUEST + + response = get_method(self._USERNAME, "/tasks") + assert response.status_code == HTTPStatus.OK + @pytest.mark.usefixtures("restore_db_per_function") class TestPatchTaskLabel: @@ -991,7 +1022,7 @@ def test_cannot_rename_label_to_duplicate_name(self, tasks, labels, admin_user): response = patch_method(admin_user, f'/tasks/{task["id"]}', {"labels": [label_payload]}) assert response.status_code == HTTPStatus.BAD_REQUEST - assert f"Label '{task_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, tasks, labels, admin_user): task = list(tasks)[0]