Fixes #1472

app/Actions/Album/Delete.php

@@ -14,6 +14,7 @@
 use App\Models\TagAlbum;
 use App\Policies\UserPolicy;
 use App\SmartAlbums\UnsortedAlbum;
+use Illuminate\Database\Eloquent\Collection;
 use Illuminate\Database\Eloquent\ModelNotFoundException;
 use Illuminate\Database\Query\Builder as BaseBuilder;
 use Illuminate\Support\Facades\Auth;
@@ -46,7 +47,7 @@
 class Delete extends Action
 {
 	/**
-	 * Deletes the designated albums from the DB.
+	 * Deletes the designated albums (tag albums and regular albums) from the DB.
 	 *
 	 * The method only deletes the records for albums, photos, their size
 	 * variants and potentially associated symbolic links from the DB.
@@ -57,7 +58,7 @@ class Delete extends Action
 	 * This object can (and must) be used to eventually delete the files,
 	 * however doing so can be deferred.
 	 *
-	 * @param string[] $albumIDs the album IDs
+	 * @param string[] $albumIDs the album IDs (contains IDs of regular _and_ tag albums)
 	 *
 	 * @return FileDeleter contains the collected files which became obsolete
 	 *
@@ -69,7 +70,6 @@ public function do(array $albumIDs): FileDeleter
 	{
 		try {
 			$unsortedPhotoIDs = [];
-			$recursiveAlbumIDs = $albumIDs;
 
 			// Among the smart albums, the unsorted album is special,
 			// because it provides deletion of photos
@@ -85,11 +85,13 @@ public function do(array $albumIDs): FileDeleter
 			// find all photos in those and their descendants
 			// Only load necessary attributes for tree; in particular avoid
 			// loading expensive `min_taken_at` and `max_taken_at`.
+			/** @var Collection<Album> $albums */
 			$albums = Album::query()
 				->without(['cover', 'thumb'])
 				->select(['id', 'parent_id', '_lft', '_rgt', 'track_short_path'])
 				->findMany($albumIDs);
 
+			$recursiveAlbumIDs = $albums->pluck('id')->all(); // only IDs which refer to regular albums are incubators for recursive IDs
 			$recursiveAlbumTracks = $albums->pluck('track_short_path');
 
 			/** @var Album $album */
@@ -105,54 +107,8 @@ public function do(array $albumIDs): FileDeleter
 			$fileDeleter = (new PhotoDelete())->do($unsortedPhotoIDs, $recursiveAlbumIDs);
 			$fileDeleter->addRegularFiles($recursiveAlbumTracks);
 
-			// Remove descendants of each album which is going to be deleted
-			// This is ugly as hell and copy & pasted from
-			// \Kalnoy\Nestedset\NodeTrait
-			// I really liked the code of master@0199212 ways better, but it was
-			// simply too inefficient
-
-			// This code also fixes a bug when more than one album with
-			// sub-albums is deleted, i.e. if we delete a "sub-forest".
-			// The original code (of the nested set model) updates the
-			// (lft,rgt)-indices on the DB level for every single deletion.
-			// However, this way deletion of the second albums fails, if the
-			// second album has already been hydrated earlier, because the
-			// indices of the already hydrated models and the indices in the
-			// DB are out-of-sync.
-			// Either all remaining models needs to be re-hydrated aka
-			// "refreshed" from the (already updated) DB after every single
-			// deletion or the update of the DB needs to be postponed until
-			// all models have been deleted.
-			// The latter is more efficient, because we do not reload models
-			// from the DB.
-
-			/** @var array<array{lft: int, rgt:int}> $pendingGapsToMake */
-			$pendingGapsToMake = [];
-			$deleteQuery = Album::query();
-			// First collect all albums to delete in a single query and
-			// memorize which indices need to be updated later.
-			foreach ($albums as $album) {
-				$pendingGapsToMake[] = [
-					'lft' => $album->getLft(),
-					'rgt' => $album->getRgt(),
-				];
-				$deleteQuery->whereDescendantOf($album, 'or', false, true);
-			}
-			// For MySQL deletion must be done in correct order otherwise the
-			// foreign key constraint to `parent_id` fails.
-			$deleteQuery->orderBy('_lft', 'desc')->delete();
-			// _After all_ albums have been deleted, remove the gaps which
-			// have been created by the removed albums.
-			// Note, the gaps must be removed beginning with the highest
-			// values first otherwise the later indices won't be correct.
-			// To save some DB queries, we could implement a "makeMultiGap".
-			usort($pendingGapsToMake, fn ($a, $b) => $b['lft'] <=> $a['lft']);
-			foreach ($pendingGapsToMake as $pendingGap) {
-				$height = $pendingGap['rgt'] - $pendingGap['lft'] + 1;
-				(new Album())->newNestedSetQuery()->makeGap($pendingGap['rgt'] + 1, -$height);
-				Album::$actionsPerformed++;
-			}
-
+			// Remove the sub-forest spanned by the regular albums
+			$this->deleteSubForest($albums);
 			TagAlbum::query()->whereIn('id', $albumIDs)->delete();
 
 			// Note, we may need to delete more base albums than those whose
@@ -185,4 +141,69 @@ public function do(array $albumIDs): FileDeleter
 			throw LycheeAssertionError::createFromUnexpectedException($e);
 		}
 	}
+
+	/**
+	 * Deletes the given set of regular albums incl. their descendants from DB.
+	 *
+	 * This is ugly as hell and is mostly copy & pasted from
+	 * {@link \Kalnoy\Nestedset\NodeTrait} with adoptions.
+	 * I really liked the code of master@0199212 ways better, but it was
+	 * simply too inefficient
+	 *
+	 * This code also fixes a bug when more than one album with
+	 * sub-albums is deleted, i.e. if we delete a "sub-forest".
+	 * The original code (of the nested set model) updates the
+	 * (lft,rgt)-indices on the DB level for every single deletion.
+	 * However, this way deletion of the second albums fails, if the
+	 * second album has already been hydrated earlier, because the
+	 * indices of the already hydrated models and the indices in the
+	 * DB are out-of-sync.
+	 * Either all remaining models needs to be re-hydrated aka
+	 * "refreshed" from the (already updated) DB after every single
+	 * deletion or the update of the DB needs to be postponed until
+	 * all models have been deleted.
+	 * The latter is more efficient, because we do not reload models
+	 * from the DB.
+	 *
+	 * @param Collection<Album> $albums
+	 *
+	 * @return void
+	 *
+	 * @throws ModelNotFoundException
+	 * @throws QueryBuilderException
+	 */
+	private function deleteSubForest(Collection $albums): void
+	{
+		if ($albums->isEmpty()) {
+			return;
+		}
+
+		/** @var array<array{lft: int, rgt:int}> $pendingGapsToMake */
+		$pendingGapsToMake = [];
+		$deleteQuery = Album::query();
+		// First collect all albums to delete in a single query and
+		// memorize which indices need to be updated later.
+		/** @var Album $album */
+		foreach ($albums as $album) {
+			$pendingGapsToMake[] = [
+				'lft' => $album->getLft(),
+				'rgt' => $album->getRgt(),
+			];
+			$deleteQuery->whereDescendantOf($album, 'or', false, true);
+		}
+		// For MySQL deletion must be done in correct order otherwise the
+		// foreign key constraint to `parent_id` fails.
+		$deleteQuery->orderBy('_lft', 'desc')->delete();
+		// _After all_ albums have been deleted, remove the gaps which
+		// have been created by the removed albums.
+		// Note, the gaps must be removed beginning with the highest
+		// values first otherwise the later indices won't be correct.
+		// To save some DB queries, we could implement a "makeMultiGap".
+		usort($pendingGapsToMake, fn ($a, $b) => $b['lft'] <=> $a['lft']);
+		foreach ($pendingGapsToMake as $pendingGap) {
+			$height = $pendingGap['rgt'] - $pendingGap['lft'] + 1;
+			(new Album())->newNestedSetQuery()->makeGap($pendingGap['rgt'] + 1, -$height);
+			Album::$actionsPerformed++;
+		}
+	}
 }

phpstan.neon

@@ -25,8 +25,6 @@ parameters:
 		- '#Dynamic call to static method (Illuminate\\Database\\Query\\Builder|Illuminate\\Database\\Eloquent\\(Builder|Relations\\.*)|App\\Models\\Extensions\\FixedQueryBuilder)(<.*>)?::update\(\).#'
 		- '#Call to an undefined method Illuminate\\Database\\Eloquent\\.*::update\(\)#'
 		- '#Call to an undefined method Illuminate\\Database\\Eloquent\\.*::with(Only)?\(\)#'
-		- '#Call to an undefined method Illuminate\\Database\\Eloquent\\.*::getLft\(\).#'
-		- '#Call to an undefined method Illuminate\\Database\\Eloquent\\.*::getRgt\(\).#'
 		- '#Call to private method latest\(\) of parent class Illuminate\\Database\\Eloquent\\Relations\\HasMany<Illuminate\\Database\\Eloquent\\Model>#'
 		- '#Call to private method where(Not)?(Null|In|Exists|Column)?\(\) of parent class Illuminate\\Database\\Eloquent\\Builder<Illuminate\\Database\\Eloquent\\Model>.#'
 		- '#Call to protected method asDateTime\(\) of class Illuminate\\Database\\Eloquent\\Model.#'

tests/Feature/AlbumTest.php

@@ -21,11 +21,13 @@
 use Illuminate\Support\Facades\DB;
 use Illuminate\Support\Facades\Session;
 use Tests\Feature\Lib\AlbumsUnitTest;
+use Tests\Feature\Lib\PhotosUnitTest;
 use Tests\Feature\Lib\RootAlbumUnitTest;
 use Tests\Feature\Lib\SharingUnitTest;
 use Tests\Feature\Lib\UsersUnitTest;
 use Tests\Feature\Traits\InteractWithSmartAlbums;
 use Tests\Feature\Traits\RequiresEmptyAlbums;
+use Tests\Feature\Traits\RequiresEmptyPhotos;
 use Tests\Feature\Traits\RequiresEmptyUsers;
 use Tests\TestCase;
 
@@ -34,25 +36,30 @@ class AlbumTest extends TestCase
 	use InteractWithSmartAlbums;
 	use RequiresEmptyAlbums;
 	use RequiresEmptyUsers;
+	use RequiresEmptyPhotos;
 
 	protected AlbumsUnitTest $albums_tests;
 	protected RootAlbumUnitTest $root_album_tests;
 	protected UsersUnitTest $users_tests;
 	protected SharingUnitTest $sharing_tests;
+	protected PhotosUnitTest $photos_tests;
 
 	public function setUp(): void
 	{
 		parent::setUp();
 		$this->setUpRequiresEmptyUsers();
 		$this->setUpRequiresEmptyAlbums();
+		$this->setUpRequiresEmptyPhotos();
 		$this->albums_tests = new AlbumsUnitTest($this);
 		$this->root_album_tests = new RootAlbumUnitTest($this);
 		$this->users_tests = new UsersUnitTest($this);
 		$this->sharing_tests = new SharingUnitTest($this);
+		$this->photos_tests = new PhotosUnitTest($this);
 	}
 
 	public function tearDown(): void
 	{
+		$this->tearDownRequiresEmptyPhotos();
 		$this->tearDownRequiresEmptyAlbums();
 		$this->tearDownRequiresEmptyUsers();
 		parent::tearDown();
@@ -482,4 +489,41 @@ public function testDeleteMultipleAlbumsByOwner(): void
 		$albumID2 = $this->albums_tests->add(null, 'Test Album 2')->offsetGet('id');
 		$this->albums_tests->delete([$albumID1, $albumID2]);
 	}
+
+	/**
+	 * Creates a (regular) album, put some photos in it, tags some of them,
+	 * creates a corresponding tag album and deletes the tag album again.
+	 *
+	 * This test ensures that only and ONLY the tag album is deleted.
+	 *
+	 * In particular, the test assures:
+	 *  - deleting the tag album does not delete the photos inside it
+	 *  - deleting the tah album does not delete the regular album which
+	 *    contains the tagged photos
+	 *
+	 * Test for issue
+	 * [LycheeOrg/Lychee#1472](https://github.com/LycheeOrg/Lychee/issues/1472).
+	 *
+	 * @return void
+	 */
+	public function testDeleteNonEmptyTagAlbumWithPhotosFromRegularAlbum(): void
+	{
+		Auth::loginUsingId(0);
+		$regularAlbumID = $this->albums_tests->add(null, 'Regular Album for Delete Test')->offsetGet('id');
+		$photoID = $this->photos_tests->upload(
+			self::createUploadedFile(self::SAMPLE_FILE_MONGOLIA_IMAGE), $regularAlbumID
+		)->offsetGet('id');
+		$this->photos_tests->set_tag([$photoID], ['tag-for-delete-test']);
+		$tagAlbumID = $this->albums_tests->addByTags('Tag Album for Delete Test', ['tag-for-delete-test'])->offsetGet('id');
+
+		// Ensure that the photo is actually part of the tag album and that
+		// we are testing what we want to test
+		$this->albums_tests->get($tagAlbumID)->assertJson([]);
+
+		$this->albums_tests->delete([$tagAlbumID]);
+
+		// Ensure that the regular album and the photo are still there
+		$this->albums_tests->get($regularAlbumID);
+		$this->photos_tests->get($photoID);
+	}
 }