diff --git a/changelog.d/20231117_123912_maria_fix_cs_deletion_handling.md b/changelog.d/20231117_123912_maria_fix_cs_deletion_handling.md new file mode 100644 index 000000000000..bb2ff7fb33c7 --- /dev/null +++ b/changelog.d/20231117_123912_maria_fix_cs_deletion_handling.md @@ -0,0 +1,4 @@ +### Fixed + +- Reset source/target storage if related cloud storage has been deleted + () diff --git a/cvat/apps/engine/location.py b/cvat/apps/engine/location.py index d2dc669f86ef..6eb5dc764418 100644 --- a/cvat/apps/engine/location.py +++ b/cvat/apps/engine/location.py @@ -3,9 +3,9 @@ # SPDX-License-Identifier: MIT from enum import Enum -from typing import Any, Dict +from typing import Any, Dict, Union -from cvat.apps.engine.models import Location, Job +from cvat.apps.engine.models import Location, Project, Task, Job class StorageType(str, Enum): TARGET = 'target_storage' @@ -14,7 +14,11 @@ class StorageType(str, Enum): def __str__(self): return self.value -def get_location_configuration(obj, field_name: str, use_settings: bool = False) -> Dict[str, Any]: +def get_location_configuration( + obj: Union[Project, Task, Job, Dict], + field_name: str, + use_settings: bool = False, +) -> Dict[str, Any]: location_conf = { "is_default": use_settings } @@ -25,20 +29,12 @@ def get_location_configuration(obj, field_name: str, use_settings: bool = False) location_conf['location'] = Location.LOCAL else: location_conf['location'] = storage.location - sid = storage.cloud_storage_id - if sid: - location_conf['storage_id'] = sid + if cloud_storage_id := storage.cloud_storage_id: + location_conf['storage_id'] = cloud_storage_id else: # obj is query_params - # FIXME when ui part will be done location_conf['location'] = obj.get('location', Location.LOCAL) - # try: - # location_conf['location'] = obj['location'] - # except KeyError: - # raise ValidationError("Custom settings were selected but no location was specified") - - sid = obj.get('cloud_storage_id') - if sid: - location_conf['storage_id'] = int(sid) + if cloud_storage_id := obj.get('cloud_storage_id'): + location_conf['storage_id'] = int(cloud_storage_id) return location_conf diff --git a/cvat/apps/engine/migrations/0076_remove_storages_that_refer_to_deleted_cloud_storages.py b/cvat/apps/engine/migrations/0076_remove_storages_that_refer_to_deleted_cloud_storages.py new file mode 100644 index 000000000000..50c1461319a7 --- /dev/null +++ b/cvat/apps/engine/migrations/0076_remove_storages_that_refer_to_deleted_cloud_storages.py @@ -0,0 +1,26 @@ +# Generated by Django 4.2.6 on 2023-11-17 10:10 + +from django.db import migrations, models +from cvat.apps.engine.models import Location + + +def manually_remove_outdated_relations(apps, schema_editor): + Storage = apps.get_model("engine", "Storage") + CloudStorage = apps.get_model("engine", "CloudStorage") + Storage.objects.filter(location=Location.LOCAL, cloud_storage_id__isnull=False).update(cloud_storage_id=None) + Storage.objects.filter( + ~models.Exists( + CloudStorage.objects.filter(pk=models.OuterRef("cloud_storage_id")) + ), + location=Location.CLOUD_STORAGE, + ).delete() + + +class Migration(migrations.Migration): + dependencies = [ + ("engine", "0075_annotationguide_is_public"), + ] + + operations = [ + migrations.RunPython(manually_remove_outdated_relations), + ] diff --git a/cvat/apps/engine/migrations/0077_auto_20231121_1952.py b/cvat/apps/engine/migrations/0077_auto_20231121_1952.py new file mode 100644 index 000000000000..8b5c3648e068 --- /dev/null +++ b/cvat/apps/engine/migrations/0077_auto_20231121_1952.py @@ -0,0 +1,28 @@ +# Generated by Django 4.2.6 on 2023-11-21 19:52 + +from django.db import migrations, models +import django.db.models.deletion + + +class Migration(migrations.Migration): + dependencies = [ + ("engine", "0076_remove_storages_that_refer_to_deleted_cloud_storages"), + ] + + operations = [ + migrations.AlterField( + model_name="storage", + name="cloud_storage_id", + field=models.ForeignKey( + null=True, + on_delete=django.db.models.deletion.CASCADE, + related_name="+", + to="engine.cloudstorage", + ), + ), + migrations.RenameField( + model_name="storage", + old_name="cloud_storage_id", + new_name="cloud_storage", + ), + ] diff --git a/cvat/apps/engine/models.py b/cvat/apps/engine/models.py index df76256e6e9f..351d0f80264b 100644 --- a/cvat/apps/engine/models.py +++ b/cvat/apps/engine/models.py @@ -1114,7 +1114,12 @@ def has_at_least_one_manifest(self) -> bool: class Storage(models.Model): location = models.CharField(max_length=16, choices=Location.choices(), default=Location.LOCAL) - cloud_storage_id = models.IntegerField(null=True, blank=True, default=None) + cloud_storage = models.ForeignKey( + CloudStorage, + on_delete=models.CASCADE, + null=True, + related_name='+', + ) class Meta: default_permissions = () diff --git a/cvat/apps/engine/serializers.py b/cvat/apps/engine/serializers.py index ba8a8f1bb82f..ea1c5dc7199f 100644 --- a/cvat/apps/engine/serializers.py +++ b/cvat/apps/engine/serializers.py @@ -1030,6 +1030,8 @@ def _create_files(self, instance, files): ) class StorageSerializer(serializers.ModelSerializer): + cloud_storage_id = serializers.IntegerField(required=False, allow_null=True) + class Meta: model = models.Storage fields = ('id', 'location', 'cloud_storage_id') @@ -1980,63 +1982,81 @@ class Meta: read_only_fields = ('path',) -def _update_related_storages(instance, validated_data): - for storage in ('source_storage', 'target_storage'): - new_conf = validated_data.pop(storage, None) +def _update_related_storages( + instance: Union[models.Project, models.Task], + validated_data: Dict[str, Any], +) -> None: + for storage_type in ('source_storage', 'target_storage'): + new_conf = validated_data.pop(storage_type, None) if not new_conf: continue - cloud_storage_id = new_conf.get('cloud_storage_id') - if cloud_storage_id: - _validate_existence_of_cloud_storage(cloud_storage_id) + new_cloud_storage_id = new_conf.get('cloud_storage_id') + new_location = new_conf.get('location') # storage_instance maybe None - storage_instance = getattr(instance, storage) + storage_instance = getattr(instance, storage_type) + + if new_cloud_storage_id: + if new_location and new_location != models.Location.CLOUD_STORAGE: + raise serializers.ValidationError( + f"It is not allowed to specify '{new_location}' location together with cloud storage id" + ) + elif ( + not new_location + and getattr(storage_instance, "location", None) != models.Location.CLOUD_STORAGE + ): + raise serializers.ValidationError( + f"The configuration of {storage_type} is not full" + ) + + if not models.CloudStorage.objects.filter(id=new_cloud_storage_id).exists(): + raise serializers.ValidationError( + f"The specified cloud storage {new_cloud_storage_id} does not exist." + ) + else: + if new_location == models.Location.CLOUD_STORAGE: + raise serializers.ValidationError( + "Cloud storage was selected as location but its id was not specified" + ) + elif ( + not new_location + and getattr(storage_instance, "location", None) == models.Location.CLOUD_STORAGE + and "cloud_storage_id" in new_conf + ): + raise serializers.ValidationError( + "It is not allowed to reset a cloud storage id without explicitly resetting a location" + ) + if not storage_instance: storage_instance = models.Storage(**new_conf) storage_instance.save() - setattr(instance, storage, storage_instance) + setattr(instance, storage_type, storage_instance) continue - new_location = new_conf.get('location') storage_instance.location = new_location or storage_instance.location - storage_instance.cloud_storage_id = new_conf.get('cloud_storage_id', \ - storage_instance.cloud_storage_id if not new_location else None) - - cloud_storage_id = storage_instance.cloud_storage_id - if cloud_storage_id: - try: - _ = models.CloudStorage.objects.get(id=cloud_storage_id) - except models.CloudStorage.DoesNotExist: - raise serializers.ValidationError(f'The specified cloud storage {cloud_storage_id} does not exist.') - + storage_instance.cloud_storage_id = new_cloud_storage_id storage_instance.save() -def _configure_related_storages(validated_data): - +def _configure_related_storages(validated_data: Dict[str, Any]) -> Dict[str, Optional[models.Storage]]: storages = { 'source_storage': None, 'target_storage': None, } for i in storages: - storage_conf = validated_data.get(i) - if storage_conf: - cloud_storage_id = storage_conf.get('cloud_storage_id') - if cloud_storage_id: - _validate_existence_of_cloud_storage(cloud_storage_id) + if storage_conf := validated_data.get(i): + if ( + (cloud_storage_id := storage_conf.get('cloud_storage_id')) and + not models.CloudStorage.objects.filter(id=cloud_storage_id).exists() + ): + raise serializers.ValidationError(f'The specified cloud storage {cloud_storage_id} does not exist.') storage_instance = models.Storage(**storage_conf) storage_instance.save() storages[i] = storage_instance return storages -def _validate_existence_of_cloud_storage(cloud_storage_id): - try: - _ = models.CloudStorage.objects.get(id=cloud_storage_id) - except models.CloudStorage.DoesNotExist: - raise serializers.ValidationError(f'The specified cloud storage {cloud_storage_id} does not exist.') - class AssetReadSerializer(WriteOnceMixin, serializers.ModelSerializer): filename = serializers.CharField(required=True, max_length=1024) owner = BasicUserSerializer(required=False) diff --git a/cvat/schema.yml b/cvat/schema.yml index b5804a210fe0..e548c069cb4f 100644 --- a/cvat/schema.yml +++ b/cvat/schema.yml @@ -9385,8 +9385,6 @@ components: $ref: '#/components/schemas/LocationEnum' cloud_storage_id: type: integer - maximum: 2147483647 - minimum: -2147483648 nullable: true StorageMethod: enum: @@ -9403,8 +9401,6 @@ components: $ref: '#/components/schemas/LocationEnum' cloud_storage_id: type: integer - maximum: 2147483647 - minimum: -2147483648 nullable: true StorageType: enum: