From 02afcd3b58723536c9593b65f08b915d59372192 Mon Sep 17 00:00:00 2001 From: Matthias Nagel Date: Sun, 21 Aug 2022 11:50:07 +0200 Subject: [PATCH 1/2] Wrote test case to reproduce issue #1472 --- tests/Feature/AlbumTest.php | 44 +++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/tests/Feature/AlbumTest.php b/tests/Feature/AlbumTest.php index 832a357656..c3905b840d 100644 --- a/tests/Feature/AlbumTest.php +++ b/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); + } } From b48f74bc7d71cad664ba8363d73c23c839abadcd Mon Sep 17 00:00:00 2001 From: Matthias Nagel Date: Sun, 21 Aug 2022 12:26:04 +0200 Subject: [PATCH 2/2] Fix #1472 --- app/Actions/Album/Delete.php | 123 ++++++++++++++++++++--------------- phpstan.neon | 2 - 2 files changed, 72 insertions(+), 53 deletions(-) diff --git a/app/Actions/Album/Delete.php b/app/Actions/Album/Delete.php index 5f368b40e9..9bf634add6 100644 --- a/app/Actions/Album/Delete.php +++ b/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 $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 $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 $albums + * + * @return void + * + * @throws ModelNotFoundException + * @throws QueryBuilderException + */ + private function deleteSubForest(Collection $albums): void + { + if ($albums->isEmpty()) { + return; + } + + /** @var array $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++; + } + } } diff --git a/phpstan.neon b/phpstan.neon index ded157bab7..322b83373d 100644 --- a/phpstan.neon +++ b/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#' - '#Call to private method where(Not)?(Null|In|Exists|Column)?\(\) of parent class Illuminate\\Database\\Eloquent\\Builder.#' - '#Call to protected method asDateTime\(\) of class Illuminate\\Database\\Eloquent\\Model.#'