Harden tests scenarios for deletion (#4003)

Author: ildyria

app/Actions/Album/Delete.php

@@ -8,11 +8,11 @@
 
 namespace App\Actions\Album;
 
-use App\Actions\Photo\Delete as PhotoDelete;
 use App\Actions\Shop\PurchasableService;
 use App\Constants\AccessPermissionConstants as APC;
 use App\Constants\PhotoAlbum as PA;
-use App\DTO\Delete\AlbumsToBeDeleteDTO;
+use App\DTO\Delete\AlbumsToBeDeletedDTO;
+use App\DTO\Delete\PhotosToBeDeletedDTO;
 use App\Enum\StorageDiskType;
 use App\Events\AlbumDeleted;
 use App\Exceptions\CorruptedTreeException;
@@ -87,11 +87,10 @@ public function do(array $album_ids): void
 
 		$this->jobs[] = new FileDeleterJob(StorageDiskType::LOCAL, $albums_to_delete->tracks->all());
 
-		$photos_to_delete = $this->findAllPhotosToDelete($albums_to_delete->album_ids);
+		$photos_to_be_deleted = $this->findAllPhotosToDelete($albums_to_delete->album_ids);
 
 		// Nuke the photos.
-		$photo_delete_action = resolve(PhotoDelete::class);
-		$jobs = $photo_delete_action->forceDelete($photos_to_delete);
+		$jobs = $photos_to_be_deleted->executeDelete();
 
 		// Nuke the albums.
 		$albums_to_delete->executeDelete();
@@ -134,9 +133,9 @@ private function deleteTagAlbums(array $tag_album_ids): void
 	 *
 	 * @param string[] $album_ids that are being deleted
 	 *
-	 * @return string[] of photo IDs that can be deleted fully
+	 * @return PhotosToBeDeletedDTO of photo IDs that can be deleted fully
 	 */
-	public function findAllPhotosToDelete(array $album_ids): array
+	public function findAllPhotosToDelete(array $album_ids): PhotosToBeDeletedDTO
 	{
 		// First collect which pictures needs to be potentially deleted.
 		// ! RISK OF MEMOY EXHAUSTION !
@@ -192,17 +191,21 @@ public function findAllPhotosToDelete(array $album_ids): array
 			$idx_total++;
 		}
 
-		return $photos_to_delete_fully;
+		return new PhotosToBeDeletedDTO(
+			force_delete_photo_ids: $photos_to_delete_fully,
+			soft_delete_photo_ids: $photos_to_detach,
+			album_ids: $album_ids,
+		);
 	}
 
 	/**
 	 * Find all albums we want to delete (sub trees).
 	 *
 	 * @param array $album_ids
 	 *
-	 * @return AlbumsToBeDeleteDTO
+	 * @return AlbumsToBeDeletedDTO
 	 */
-	private function findAllAlbumsToDelete(array $album_ids): AlbumsToBeDeleteDTO
+	private function findAllAlbumsToDelete(array $album_ids): AlbumsToBeDeletedDTO
 	{
 		// First gather the gaps that will be made:
 		$gaps = $this->getGaps($album_ids);
@@ -236,7 +239,7 @@ private function findAllAlbumsToDelete(array $album_ids): AlbumsToBeDeleteDTO
 		// prune the null values
 		$recursive_album_tracks = $recursive_album_tracks->filter(fn ($val) => $val !== null);
 
-		return new AlbumsToBeDeleteDTO(
+		return new AlbumsToBeDeletedDTO(
 			parent_ids: $parent_ids,
 			album_ids: $recursive_album_ids,
 			tracks: $recursive_album_tracks,

app/Actions/Photo/Delete.php

@@ -10,16 +10,10 @@
 
 use App\Actions\Shop\PurchasableService;
 use App\Constants\PhotoAlbum as PA;
-use App\Enum\SizeVariantType;
-use App\Enum\StorageDiskType;
+use App\DTO\Delete\PhotosToBeDeletedDTO;
 use App\Events\PhotoDeleted;
 use App\Exceptions\Internal\LycheeLogicException;
-use App\Exceptions\Internal\QueryBuilderException;
 use App\Exceptions\ModelDBException;
-use App\Jobs\FileDeleterJob;
-use App\Models\SizeVariant;
-use Illuminate\Database\Query\JoinClause;
-use Illuminate\Support\Collection;
 use Illuminate\Support\Facades\DB;
 
 /**
@@ -53,66 +47,6 @@ public function __construct(
 		$this->purchasable_service = resolve(PurchasableService::class);
 	}
 
-	/**
-	 * Delete the designated photos.
-	 * There is no check for album dependencies. This has already be done.
-	 * This is a force delete.
-	 *
-	 * @param array $photo_ids Photos to deleted
-	 *
-	 * @return array
-	 */
-	public function forceDelete(array $photo_ids): array
-	{
-		// We do not check the purchasable service as they have already been deleted by the caller.
-		if (count($photo_ids) === 0) {
-			return [];
-		}
-
-		// Reset headers and covers pointing to deleted photos
-		DB::table('albums')->whereIn('header_id', $photo_ids)->update(['header_id' => null]);
-		DB::table('albums')->whereIn('cover_id', $photo_ids)->update(['cover_id' => null]);
-
-		// Maybe consider doing multiple queries for the different storage types.
-		$exclude_size_variants_ids = DB::table('order_items')->select(['size_variant_id'])->pluck('size_variant_id')->all();
-
-		// Collect size variants to be deleted
-		// ! Risk of memory exhaustion if too many photos are deleted at once !
-		$size_variants_local = $this->collectSizeVariantPathsByPhotoID($photo_ids, StorageDiskType::LOCAL, $exclude_size_variants_ids);
-		$short_paths_local = $size_variants_local->pluck('short_path')->all();
-		$short_path_watermarked_local = $size_variants_local->pluck('short_path_watermarked')->filter()->all();
-
-		$size_variants_s3 = $this->collectSizeVariantPathsByPhotoID($photo_ids, StorageDiskType::S3, $exclude_size_variants_ids);
-		$short_paths_s3 = $size_variants_s3->pluck('short_path')->all();
-		$short_path_watermarked_s3 = $size_variants_s3->pluck('short_path_watermarked')->filter()->all();
-
-		$live_photo_short_paths_local = $this->collectLivePhotoPathsByPhotoID($photo_ids, StorageDiskType::LOCAL)->pluck('live_photo_short_path')->all();
-		$live_photo_short_paths_s3 = $this->collectLivePhotoPathsByPhotoID($photo_ids, StorageDiskType::S3)->pluck('live_photo_short_path')->all();
-
-		$delete_jobs = [];
-		$delete_jobs[] = new FileDeleterJob(StorageDiskType::LOCAL, $short_paths_local);
-		$delete_jobs[] = new FileDeleterJob(StorageDiskType::LOCAL, $short_path_watermarked_local);
-		$delete_jobs[] = new FileDeleterJob(StorageDiskType::LOCAL, $live_photo_short_paths_local);
-		$delete_jobs[] = new FileDeleterJob(StorageDiskType::S3, $short_paths_s3);
-		$delete_jobs[] = new FileDeleterJob(StorageDiskType::S3, $short_path_watermarked_s3);
-		$delete_jobs[] = new FileDeleterJob(StorageDiskType::S3, $live_photo_short_paths_s3);
-
-		// Now delete DB records
-		// ! If we are deleting more than a few 1000 photos at once, we may run into
-		// ! SQL query size limits. In that case, we need to chunk the deletion.
-
-		// Those we are keeping.
-		DB::table('size_variants')->whereIn('id', $exclude_size_variants_ids)->update(['photo_id' => null]);
-		// Those we delete
-		DB::table('size_variants')->whereIn('photo_id', $photo_ids)->delete();
-		DB::table('statistics')->whereIn('photo_id', $photo_ids)->delete();
-		DB::table('palettes')->whereIn('photo_id', $photo_ids)->delete();
-		DB::table('photo_album')->whereIn('photo_id', $photo_ids)->delete(); // Just to be sure.
-		DB::table('photos')->whereIn('id', $photo_ids)->delete();
-
-		return $delete_jobs;
-	}
-
 	/**
 	 * Deletes the designated photos from the DB.
 	 *
@@ -172,11 +106,14 @@ public function do(array $photo_ids, string|null $from_id): void
 		// Finally, add the unsorted photos to be deleted.
 		$delete_photo_ids = array_merge($delete_photo_ids, $unsorted_photo_ids);
 
-		// We delete the photos which are in the from and listed in albums.
-		DB::table(PA::PHOTO_ALBUM)->whereIn(PA::PHOTO_ID, $photo_ids)->where(PA::ALBUM_ID, '=', $from_id)->delete();
-
 		$this->purchasable_service->deleteMulitplePhotoPurchasables($photo_ids, [$from_id]);
-		$jobs = $this->forceDelete($delete_photo_ids);
+
+		$photos_to_be_deleted = new PhotosToBeDeletedDTO(
+			force_delete_photo_ids: $delete_photo_ids,
+			soft_delete_photo_ids: $photo_ids,
+			album_ids: [$from_id],
+		);
+		$jobs = $photos_to_be_deleted->executeDelete();
 
 		// Dispatch events for affected albums to trigger recomputation
 		PhotoDeleted::dispatch($from_id);
@@ -185,69 +122,4 @@ public function do(array $photo_ids, string|null $from_id): void
 			dispatch($job);
 		}
 	}
-
-	/**
-	 * Collects all short paths of size variants which shall be deleted from
-	 * disk.
-	 *
-	 * Size variants which belong to a photo which has a duplicate that is
-	 * not going to be deleted are skipped.
-	 *
-	 * @param array<int,string> $photo_ids                 the photo IDs
-	 * @param StorageDiskType   $storage_disk              the storage disk to filter for (null = all)
-	 * @param array<int,string> $exclude_size_variants_ids size variant IDs to be excluded
-	 *
-	 * @return Collection<int,SizeVariant> the size variants to be deleted
-	 *
-	 * @throws QueryBuilderException
-	 */
-	private function collectSizeVariantPathsByPhotoID(array $photo_ids, StorageDiskType $storage_disk, array $exclude_size_variants_ids): Collection
-	{
-		if (count($photo_ids) === 0) {
-			return collect([]);
-		}
-
-		return SizeVariant::query()
-			->from('size_variants as sv')
-			->select(['sv.short_path', 'sv.short_path_watermarked'])
-			->join('photos as p', 'p.id', '=', 'sv.photo_id')
-			->whereIn('p.id', $photo_ids)
-			->where('sv.storage_disk', '=', $storage_disk->value)
-			->whereNotIn('sv.id', $exclude_size_variants_ids)
-			->toBase()
-			->get();
-	}
-
-	/**
-	 * Collects all short paths of live photos which shall be deleted from
-	 * disk.
-	 *
-	 * Live photos which have a duplicate that is not going to be deleted are
-	 * skipped.
-	 *
-	 * @param array<int,string> $photo_ids    the photo IDs
-	 * @param StorageDiskType   $storage_disk the storage disk to filter for (null = all)
-	 *
-	 * @return Collection<int,object{live_photo_short_path:string}> the live photo short paths to be deleted
-	 *
-	 * @throws QueryBuilderException
-	 */
-	private function collectLivePhotoPathsByPhotoID(array $photo_ids, StorageDiskType $storage_disk): Collection
-	{
-		if (count($photo_ids) === 0) {
-			return collect([]);
-		}
-
-		return DB::table('photos', 'p')
-			->select(['p.live_photo_short_path'])
-			->join('size_variants as sv', function (JoinClause $join): void {
-				$join
-					->on('sv.photo_id', '=', 'p.id')
-					->where('sv.type', '=', SizeVariantType::ORIGINAL);
-			})
-			->whereIn('p.id', $photo_ids)
-			->whereNotNull('p.live_photo_short_path')
-			->where('sv.storage_disk', '=', $storage_disk->value)
-			->get();
-	}
 }

app/DTO/Delete/AlbumsToBeDeleteDTO.php app/DTO/Delete/AlbumsToBeDeletedDTO.phpapp/DTO/Delete/AlbumsToBeDeleteDTO.php renamed to app/DTO/Delete/AlbumsToBeDeletedDTO.php

@@ -9,11 +9,12 @@
 namespace App\DTO\Delete;
 
 use App\Actions\Shop\PurchasableService;
+use App\Exceptions\Internal\LycheeLogicException;
 use App\Models\Album;
 use Illuminate\Support\Collection;
 use Illuminate\Support\Facades\DB;
 
-final class AlbumsToBeDeleteDTO
+final class AlbumsToBeDeletedDTO
 {
 	/**
 	 * Container for all Albums and associated Tracks to be deleted.
@@ -41,6 +42,11 @@ public function __construct(
 	public function executeDelete()
 	{
 		DB::transaction(function (): void {
+			// Safety checks
+			if (DB::table('photo_album')->whereIn('album_id', $this->album_ids)->count() > 0) {
+				throw new LycheeLogicException('There are still photos linked to the albums to be deleted.');
+			}
+
 			$purchasable_service = resolve(PurchasableService::class);
 			$purchasable_service->deleteMultipleAlbumPurchasables($this->album_ids);
 			DB::table('live_metrics')->whereIn('album_id', $this->album_ids)->delete();