diff --git a/app/Actions/Album/Delete.php b/app/Actions/Album/Delete.php index b75c8413e1..7ad327a5ac 100644 --- a/app/Actions/Album/Delete.php +++ b/app/Actions/Album/Delete.php @@ -2,29 +2,129 @@ namespace App\Actions\Album; -use App\Contracts\AbstractAlbum; -use App\Models\Extensions\BaseAlbum; +use App\Actions\Photo\Delete as PhotoDelete; +use App\Facades\AccessControl; +use App\Image\FileDeleter; +use App\Models\Album; +use App\Models\BaseAlbumImpl; +use App\Models\TagAlbum; use App\SmartAlbums\UnsortedAlbum; +use Illuminate\Database\Query\Builder as BaseBuilder; +/** + * Deletes the albums with the designated IDs **efficiently**. + * + * This class deliberately violates the principle of separations of concerns. + * In an ideal world, the method would simply call `->delete()` on every + * `Album` model, the `Album` model would take care of deleting its + * sub-albums and every album in turn would take care of deleting its photos. + * But this is extremely inefficient due to Laravel's architecture: + * + * - Models are heavyweight god classes such that every instance also carries + * the whole code for serialization/deserialization + * - Models are active records (and don't use the unit-of-work pattern), i.e. + * every deletion of a model directly triggers a DB operation; they are + * not deferred into a batch operation + * + * Moreover, while removing the records for albums and photos from the + * DB can be implemented rather efficiently, the actual file operations may + * take some time. + * Especially, if the files are not stored locally but on a remote file system. + * Hence, this method collects all files which need to be removed. + * The caller can then decide to delete them asynchronously. + */ class Delete extends Action { /** - * @param array $albumIDs + * Deletes the designated albums from the DB. * - * @return bool + * The method only deletes the records for albums, photos, their size + * variants and potentially associated symbolic links from the DB. + * The method does not delete the associated files from the physical + * storage. + * Instead, the method returns an object in which all these files have + * been collected. + * This object can (and must) be used to eventually delete the files, + * however doing so can be deferred. + * + * @param string[] $albumIDs the album IDs + * + * @return FileDeleter contains the collected files which became obsolete */ - public function do(array $albumIDs): bool + public function do(array $albumIDs): FileDeleter { - $albums = $this->albumFactory->findWhereIDsIn($albumIDs); - $success = true; + try { + $unsortedPhotoIDs = []; + $recursiveAlbumIDs = $albumIDs; - /** @var AbstractAlbum $album */ - foreach ($albums as $album) { - if ($album instanceof BaseAlbum || $album instanceof UnsortedAlbum) { - $success &= $album->delete(); + // Among the smart albums, the unsorted album is special, + // because it provides deletion of photos + if (in_array(UnsortedAlbum::ID, $albumIDs, true)) { + $query = UnsortedAlbum::getInstance()->photos(); + if (!AccessControl::is_admin()) { + $query->where('owner_id', '=', AccessControl::id()); + } + $unsortedPhotoIDs = $query->pluck('id')->all(); + } + + // Only regular albums are owners of photos, so we only need to + // 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`. + $albums = Album::query() + ->without(['cover', 'thumb']) + ->select(['id', 'parent_id', '_lft', '_rgt']) + ->findMany($albumIDs); + + /** @var Album $album */ + foreach ($albums as $album) { + // Collect the IDs of all (aka recursive) sub-albums in each album + $recursiveAlbumIDs = array_merge($recursiveAlbumIDs, $album->descendants()->pluck('id')->all()); } - } - return $success; + // Delete the photos from DB and obtain the list of files which need + // to be deleted later + $fileDeleter = (new PhotoDelete())->do($unsortedPhotoIDs, $recursiveAlbumIDs); + + // 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 + foreach ($albums as $album) { + $lft = $album->getLft(); + $rgt = $album->getRgt(); + $album + ->descendants() + ->orderBy($album->getLftName(), 'desc') + ->delete(); + $height = $rgt - $lft + 1; + $album->newNestedSetQuery()->makeGap($rgt + 1, -$height); + Album::$actionsPerformed++; + } + Album::query()->whereIn('id', $albumIDs)->delete(); + TagAlbum::query()->whereIn('id', $albumIDs)->delete(); + + // Note, we may need to delete more base albums than those whose + // ID is in `$albumIDs`. + // As we might have deleted more regular albums as part of a subtree + // we simply delete all base albums who neither have an associated + // (regular) album or tag album. + BaseAlbumImpl::query()->whereNotExists(function (BaseBuilder $baseBuilder) { + $baseBuilder->from('albums')->whereColumn('albums.id', '=', 'base_albums.id'); + })->whereNotExists(function (BaseBuilder $baseBuilder) { + $baseBuilder->from('tag_albums')->whereColumn('tag_albums.id', '=', 'base_albums.id'); + })->delete(); + + return $fileDeleter; + } catch (\Exception $e) { + try { + // if anything goes wrong, don't leave the tree in an inconsistent state + Album::query()->fixTree(); + } catch (\Throwable) { + // Sic! We cannot do anything about the inner exception + } + throw $e; + } } } diff --git a/app/Actions/Album/Merge.php b/app/Actions/Album/Merge.php index 100dc4d4a4..2b28680f4a 100644 --- a/app/Actions/Album/Merge.php +++ b/app/Actions/Album/Merge.php @@ -41,12 +41,11 @@ public function do(string $albumID, array $sourceAlbumIDs): void } // Now we delete the source albums - // ! we have to do it via Model::delete() in order to not break the tree - $albums = Album::query()->whereIn('id', $sourceAlbumIDs)->get(); - /** @var Album $album */ - foreach ($albums as $album) { - $album->delete(); - } + // We must use the special `Delete` action in order to not break the + // tree. + // The returned `FileDeleter` can be ignored as all photos have been + // moved to the new location. + (new Delete())->do($sourceAlbumIDs); $targetAlbum->fixOwnershipOfChildren(); } diff --git a/app/Actions/Photo/Delete.php b/app/Actions/Photo/Delete.php index f471f54122..25d789e945 100644 --- a/app/Actions/Photo/Delete.php +++ b/app/Actions/Photo/Delete.php @@ -2,23 +2,307 @@ namespace App\Actions\Photo; +use App\Image\FileDeleter; use App\Models\Photo; +use App\Models\SizeVariant; +use App\Models\SymLink; +use Illuminate\Database\Query\Builder as BaseBuilder; +use Illuminate\Database\Query\JoinClause; +/** + * Deletes the photos with the designated IDs **efficiently**. + * + * This class deliberately violates the principle of separations of concerns. + * In an ideal world, the method would simply call `->delete()` on every + * `Photo` model and the `Photo` model would take care of deleting its + * associated size variants including the media files. + * But this is extremely inefficient due to Laravel's architecture: + * + * - Models are heavyweight god classes such that every instance also carries + * the whole code for serialization/deserialization + * - Models are active records (and don't use the unit-of-work pattern), i.e. + * every deletion of a model directly triggers a DB operation; they are + * not deferred into a batch operation + * + * Moreover, while removing the records for photos and size variants from the + * DB can be implemented rather efficiently, the actual file operations may + * take some time. + * Especially, if the files are not stored locally but on a remote file system. + * Hence, this method collects all files which need to be removed. + * The caller can then decide to delete them asynchronously. + */ class Delete { - public function do(array $photoIds): void - { - $photos = Photo::query() - ->with(['size_variants', 'size_variants.sym_links']) - ->whereIn('id', $photoIds) - ->get(); - $success = true; - /** @var Photo $photo */ - foreach ($photos as $photo) { - // we must call delete on the model and not on the database - // in order to remove the files, too - $success &= $photo->delete(); - } - abort_if(!$success, 500, 'could not delete photo(s)'); + protected FileDeleter $fileDeleter; + + public function __construct() + { + $this->fileDeleter = new FileDeleter(); + } + + /** + * Deletes the designated photos from the DB. + * + * The method only deletes the records for photos, their size variants + * and potentially associated symbolic links from the DB. + * The method does not delete the associated files from physical storage. + * Instead, the method returns an object in which all these files have + * been collected. + * This object can (and must) be used to eventually delete the files, + * however doing so can be deferred. + * + * The method allows deleting individual photos designated by + * `$photoIDs` or photos of entire albums designated by `$albumIDs`. + * The latter is more efficient, if albums shall be deleted, because + * it results in more succinct SQL queries. + * Both parameters can be used simultaneously and result in a merged + * deletion of the joined set of photos. + * + * @param string[] $photoIDs the photo IDs + * @param string[] $albumIDs the album IDs + * + * @return FileDeleter contains the collected files which became obsolete + */ + public function do(array $photoIDs, array $albumIDs = []): FileDeleter + { + $this->collectSizeVariantPathsByPhotoID($photoIDs); + $this->collectSizeVariantPathsByAlbumID($albumIDs); + $this->collectLivePhotoPathsByPhotoID($photoIDs); + $this->collectLivePhotoPathsByAlbumID($albumIDs); + $this->collectSymLinksByPhotoID($photoIDs); + $this->collectSymLinksByAlbumID($albumIDs); + $this->deleteDBRecords($photoIDs, $albumIDs); + + return $this->fileDeleter; + } + + /** + * 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 $photoIDs the photo IDs + * + * @return void + */ + private function collectSizeVariantPathsByPhotoID(array $photoIDs): void + { + if (empty($photoIDs)) { + return; + } + + $svShortPaths = SizeVariant::query() + ->from('size_variants as sv') + ->select(['sv.short_path']) + ->join('photos as p', 'p.id', '=', 'sv.photo_id') + ->leftJoin('photos as dup', function (JoinClause $join) use ($photoIDs) { + $join + ->on('dup.checksum', '=', 'p.checksum') + ->whereNotIn('dup.id', $photoIDs); + }) + ->whereIn('p.id', $photoIDs) + ->whereNull('dup.id') + ->pluck('sv.short_path'); + $this->fileDeleter->addRegularFilesOrSymbolicLinks($svShortPaths); + } + + /** + * 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 $albumIDs the album IDs + * + * @return void + */ + private function collectSizeVariantPathsByAlbumID(array $albumIDs): void + { + if (empty($albumIDs)) { + return; + } + + $svShortPaths = SizeVariant::query() + ->from('size_variants as sv') + ->select(['sv.short_path']) + ->join('photos as p', 'p.id', '=', 'sv.photo_id') + ->leftJoin('photos as dup', function (JoinClause $join) use ($albumIDs) { + $join + ->on('dup.checksum', '=', 'p.checksum') + ->whereNotIn('dup.album_id', $albumIDs); + }) + ->whereIn('p.album_id', $albumIDs) + ->whereNull('dup.id') + ->pluck('sv.short_path'); + $this->fileDeleter->addRegularFilesOrSymbolicLinks($svShortPaths); + } + + /** + * 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 $photoIDs the photo IDs + * + * @return void + */ + private function collectLivePhotoPathsByPhotoID(array $photoIDs) + { + if (empty($photoIDs)) { + return; + } + + $livePhotoShortPaths = Photo::query() + ->from('photos as p') + ->select(['p.live_photo_short_path']) + ->leftJoin('photos as dup', function (JoinClause $join) use ($photoIDs) { + $join + ->on('dup.live_photo_checksum', '=', 'p.live_photo_checksum') + ->whereNotIn('dup.id', $photoIDs); + }) + ->whereIn('p.id', $photoIDs) + ->whereNull('dup.id') + ->whereNotNull('p.live_photo_short_path') + ->pluck('p.live_photo_short_path'); + $this->fileDeleter->addRegularFilesOrSymbolicLinks($livePhotoShortPaths); + } + + /** + * 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 $albumIDs the album IDs + * + * @return void + */ + private function collectLivePhotoPathsByAlbumID(array $albumIDs) + { + if (empty($albumIDs)) { + return; + } + + $livePhotoShortPaths = Photo::query() + ->from('photos as p') + ->select(['p.live_photo_short_path']) + ->leftJoin('photos as dup', function (JoinClause $join) use ($albumIDs) { + $join + ->on('dup.live_photo_checksum', '=', 'p.live_photo_checksum') + ->whereNotIn('dup.album_id', $albumIDs); + }) + ->whereIn('p.album_id', $albumIDs) + ->whereNull('dup.id') + ->whereNotNull('p.live_photo_short_path') + ->pluck('p.live_photo_short_path'); + $this->fileDeleter->addRegularFilesOrSymbolicLinks($livePhotoShortPaths); + } + + /** + * Collects all symbolic links which shall be deleted from disk. + * + * @param array $photoIDs the photo IDs + * + * @return void + */ + private function collectSymLinksByPhotoID(array $photoIDs): void + { + if (empty($photoIDs)) { + return; + } + + $symLinkPaths = SymLink::query() + ->from('sym_links', 'sl') + ->select(['sl.short_path']) + ->join('size_variants as sv', 'sv.id', '=', 'sl.size_variant_id') + ->whereIn('sv.photo_id', $photoIDs) + ->pluck('sl.short_path'); + $this->fileDeleter->addSymbolicLinks($symLinkPaths); + } + + /** + * Collects all symbolic links which shall be deleted from disk. + * + * @param array $albumIDs the album IDs + * + * @return void + */ + private function collectSymLinksByAlbumID(array $albumIDs): void + { + if (empty($albumIDs)) { + return; + } + + $symLinkPaths = SymLink::query() + ->from('sym_links', 'sl') + ->select(['sl.short_path']) + ->join('size_variants as sv', 'sv.id', '=', 'sl.size_variant_id') + ->join('photos as p', 'p.id', '=', 'sv.photo_id') + ->whereIn('p.album_id', $albumIDs) + ->pluck('sl.short_path'); + $this->fileDeleter->addSymbolicLinks($symLinkPaths); + } + + /** + * Deletes the records from DB. + * + * The records are deleted in such an order that foreign keys are not + * broken. + * + * @param array $photoIDs the photo IDs + * @param array $albumIDs the album IDs + * + * @return void + */ + private function deleteDBRecords(array $photoIDs, array $albumIDs): void + { + if (!empty($photoIDs)) { + SymLink::query() + ->whereExists(function (BaseBuilder $query) use ($photoIDs) { + $query + ->from('size_variants', 'sv') + ->whereColumn('sv.id', '=', 'sym_links.size_variant_id') + ->whereIn('photo_id', $photoIDs); + }) + ->delete(); + } + if (!empty($albumIDs)) { + SymLink::query() + ->whereExists(function (BaseBuilder $query) use ($albumIDs) { + $query + ->from('size_variants', 'sv') + ->whereColumn('sv.id', '=', 'sym_links.size_variant_id') + ->join('photos', 'photos.id', '=', 'sv.photo_id') + ->whereIn('photos.album_id', $albumIDs); + }) + ->delete(); + } + if (!empty($photoIDs)) { + SizeVariant::query() + ->whereIn('size_variants.photo_id', $photoIDs) + ->delete(); + } + if (!empty($albumIDs)) { + SizeVariant::query() + ->whereExists(function (BaseBuilder $query) use ($albumIDs) { + $query + ->from('photos', 'p') + ->whereColumn('p.id', '=', 'size_variants.photo_id') + ->whereIn('p.album_id', $albumIDs); + }) + ->delete(); + } + if (!empty($photoIDs)) { + Photo::query()->whereIn('id', $photoIDs)->delete(); + } + if (!empty($albumIDs)) { + Photo::query()->whereIn('album_id', $albumIDs)->delete(); + } } } diff --git a/app/Actions/SizeVariant/Delete.php b/app/Actions/SizeVariant/Delete.php new file mode 100644 index 0000000000..3c9f84c498 --- /dev/null +++ b/app/Actions/SizeVariant/Delete.php @@ -0,0 +1,88 @@ +delete()` on every + * `SizeVariant` model and the `SizeVariant` model would take care of deleting + * its associated media files. + * But this is extremely inefficient due to Laravel's architecture: + * + * - Models are heavyweight god classes such that every instance also carries + * the whole code for serialization/deserialization + * - Models are active records (and don't use the unit-of-work pattern), i.e. + * every deletion of a model directly triggers a DB operation; they are + * not deferred into a batch operation + * + * Moreover, while removing the records for size variants from the DB can be + * implemented rather efficiently, the actual file operations may take some + * time. + * Especially, if the files are not stored locally but on a remote file system. + * Hence, this method collects all files which need to be removed. + * The caller can then decide to delete them asynchronously. + */ +class Delete +{ + /** + * Deletes the designated size variants from the DB. + * + * The method only deletes the records for size variants and potentially + * associated symbolic links from the DB. + * The method does not delete the associated files from the physical + * storage. + * Instead, the method returns an object in which all these files have + * been collected. + * This object can (and must) be used to eventually delete the files, + * however doing so can be deferred. + * + * @param string[] $svIds the size variant IDs + * + * @return FileDeleter contains the collected files which became obsolete + */ + public function do(array $svIds): FileDeleter + { + $fileDeleter = new FileDeleter(); + + // Get all short paths of size variants which are going to be deleted. + // But exclude those short paths which are duplicated by a size + // variant which is not going to be deleted. + $svShortPaths = SizeVariant::query() + ->from('size_variants as sv') + ->select(['sv.short_path']) + ->leftJoin('size_variants as dup', function (JoinClause $join) use ($svIds) { + $join + ->on('dup.short_path', '=', 'sv.short_path') + ->whereNotIn('dup.id', $svIds); + }) + ->whereIn('sv.id', $svIds) + ->whereNull('dup.id') + ->pluck('sv.short_path'); + $fileDeleter->addRegularFilesOrSymbolicLinks($svShortPaths); + + // Get all short paths of symbolic links which point to size variants + // which are going to be deleted + $symLinkPaths = SymLink::query() + ->select(['sym_links.short_path']) + ->whereIn('sym_links.size_variant_id', $svIds) + ->pluck('sym_links.short_path'); + $fileDeleter->addSymbolicLinks($symLinkPaths); + + // Delete records from DB in "inverse" order to not break foreign keys + SymLink::query() + ->whereIn('sym_links.size_variant_id', $svIds) + ->delete(); + SizeVariant::query() + ->whereIn('id', $svIds) + ->delete(); + + return $fileDeleter; + } +} diff --git a/app/Http/Controllers/AlbumController.php b/app/Http/Controllers/AlbumController.php index 494c94fa3b..30371ec337 100644 --- a/app/Http/Controllers/AlbumController.php +++ b/app/Http/Controllers/AlbumController.php @@ -30,6 +30,7 @@ use App\Models\TagAlbum; use Illuminate\Http\Request as IlluminateRequest; use Illuminate\Http\Response as IlluminateResponse; +use Illuminate\Support\Facades\App; use Illuminate\Support\Facades\Storage; use Illuminate\Validation\Rule; use Symfony\Component\HttpFoundation\Response as SymfonyResponse; @@ -238,11 +239,10 @@ public function setLicense(AlbumModelIDRequest $request, SetLicense $setLicense) */ public function delete(AlbumIDsRequest $request, Delete $delete): IlluminateResponse { - if ($delete->do(explode(',', $request['albumIDs']))) { - return response()->noContent(); - } else { - return response('', 500); - } + $fileDeleter = $delete->do(explode(',', $request['albumIDs'])); + App::terminating(fn () => $fileDeleter->do()); + + return response()->noContent(); } /** diff --git a/app/Http/Controllers/PhotoController.php b/app/Http/Controllers/PhotoController.php index daa5f08cbe..59f460626c 100644 --- a/app/Http/Controllers/PhotoController.php +++ b/app/Http/Controllers/PhotoController.php @@ -30,6 +30,7 @@ use Illuminate\Http\Response as IlluminateResponse; use Illuminate\Http\UploadedFile; use Illuminate\Support\Collection; +use Illuminate\Support\Facades\App; use Illuminate\Support\Facades\Storage; use Illuminate\Validation\Rule; use Symfony\Component\HttpFoundation\Response as SymfonyResponse; @@ -227,7 +228,8 @@ public function setLicense(PhotoIDRequest $request, SetLicense $setLicense): Ill */ public function delete(PhotoIDsRequest $request, Delete $delete): IlluminateResponse { - $delete->do(explode(',', $request['photoIDs'])); + $fileDeleter = $delete->do(explode(',', $request['photoIDs'])); + App::terminating(fn () => $fileDeleter->do()); return response()->noContent(); } diff --git a/app/Image/FileDeleter.php b/app/Image/FileDeleter.php new file mode 100644 index 0000000000..347892f1c1 --- /dev/null +++ b/app/Image/FileDeleter.php @@ -0,0 +1,129 @@ + + */ + protected Collection $regularFiles; + + /** + * @var Collection + */ + protected Collection $symbolicLinks; + + /** + * @var Collection + */ + protected Collection $regularFilesOrSymbolicLinks; + + public function __construct() + { + $this->regularFiles = new Collection(); + $this->symbolicLinks = new Collection(); + $this->regularFilesOrSymbolicLinks = new Collection(); + } + + /** + * @param Collection $regularFiles + * + * @return void + */ + public function addRegularFiles(Collection $regularFiles): void + { + $this->regularFiles = $this->regularFiles->merge($regularFiles); + } + + /** + * @param Collection $symbolicLinks + * + * @return void + */ + public function addSymbolicLinks(Collection $symbolicLinks): void + { + $this->symbolicLinks = $this->symbolicLinks->merge($symbolicLinks); + } + + /** + * @param Collection $regularFilesOrSymbolicLinks + * + * @return void + */ + public function addRegularFilesOrSymbolicLinks(Collection $regularFilesOrSymbolicLinks): void + { + $this->regularFilesOrSymbolicLinks = $this->regularFilesOrSymbolicLinks->merge($regularFilesOrSymbolicLinks); + } + + /** + * Deletes the collected files. + * + * @return bool + */ + public function do(): bool + { + $success = true; + + // TODO: When we use proper `File` objects, each file knows its associated disk + // In the mean time, we assume that any regular file is stored on the default disk. + $defaultDisk = Storage::disk(); + foreach ($this->regularFiles as $regularFile) { + if ($defaultDisk->exists($regularFile)) { + $success &= $defaultDisk->delete($regularFile); + } + } + + // If the disk uses the local driver, we use low-level routines as + // these are also able to handle symbolic links in case of doubt + $isLocalDisk = ($defaultDisk->getDriver()->getAdapter() instanceof \League\Flysystem\Adapter\Local); + if ($isLocalDisk) { + foreach ($this->regularFilesOrSymbolicLinks as $fileOrLink) { + $absolutePath = $defaultDisk->path($fileOrLink); + // This seemingly complicated and redundant statement uses + // lazy evaluation and avoid errors for non-existing files. + // Also note, the `file_exist` returns `false` for existing, + // but dead links. + // So the first part takes care of deleting links no matter + // if they are dead or alive. + // The latter part deletes (regular) files, but avoids errors + // in case the file doesn't exist. + $success &= ((is_link($absolutePath) && unlink($absolutePath)) || !file_exists($absolutePath) || unlink($absolutePath)); + } + } else { + // If the disk is not local, we can assume that each file is a regular file + foreach ($this->regularFilesOrSymbolicLinks as $regularFile) { + if ($defaultDisk->exists($regularFile)) { + $success &= $defaultDisk->delete($regularFile); + } + } + } + + // TODO: When we use proper `File` objects, each file knows its associated disk + // In the mean time, we assume that any symbolic link is stored on the same disk + $symlinkDisk = Storage::disk(SymLink::DISK_NAME); + foreach ($this->symbolicLinks as $symbolicLink) { + $absolutePath = $symlinkDisk->path($symbolicLink); + // Laravel and Flysystem does not support symbolic links. + // So we must use low-level methods here. + $success &= ((is_link($absolutePath) && unlink($absolutePath)) || !file_exists($absolutePath)); + } + + return $success; + } +} diff --git a/app/Models/Album.php b/app/Models/Album.php index 10a805c082..dd6048d354 100644 --- a/app/Models/Album.php +++ b/app/Models/Album.php @@ -2,6 +2,7 @@ namespace App\Models; +use App\Actions\Album\Delete; use App\Models\Extensions\AlbumBuilder; use App\Models\Extensions\BaseAlbum; use App\Relations\HasAlbumThumb; @@ -150,44 +151,32 @@ public function toArray(): array return $result; } - public function delete(): bool + /** + * {@inheritDoc} + */ + public function performDeleteOnModel(): void { - try { - $this->refreshNode(); - - $success = true; - - // Delete all recursive child photos first - $photos = $this->all_photos()->lazy(); - /** @var Photo $photo */ - foreach ($photos as $photo) { - // This also takes care of proper deletion of physical files from disk - // Note, we need this strange condition, because `delete` may also - // return `null` on success, so we must explicitly test for - // _not `false`_. - $success &= ($photo->delete() !== false); - } - - if (!$success) { - return false; - } - - // Finally, delete the album itself - // Note, we need this strange condition, because `delete` may also - // return `null` on success, so we must explicitly test for - // _not `false`_. - $success &= (parent::delete() !== false); - - return $success; - } catch (\Exception $e) { - try { - // if anything goes wrong, don't leave the tree in an inconsistent state - $this->newModelQuery()->fixTree(); - } catch (\Throwable) { - // Sic! We cannot do anything about the inner exception - } - throw $e; - } + $fileDeleter = (new Delete())->do([$this->id]); + $this->exists = false; + $fileDeleter->do(); + } + + /** + * This method is a no-op. + * + * This method is originally defined by {@link NodeTrait::deleteDescendants()} + * and called as part of the event listener for the 'deleting' event. + * The event listener is installed by {@link NodeTrait::bootNodeTrait()}. + * + * For efficiency reasons all descendants are deleted by + * {@link Delete::do()}. + * Hence, we must avoid any attempt to delete the descendants twice. + * + * @return void + */ + protected function deleteDescendants(): void + { + // deliberately a no op } /** diff --git a/app/Models/Extensions/SizeVariants.php b/app/Models/Extensions/SizeVariants.php index 4832b5ecaf..32378d496e 100644 --- a/app/Models/Extensions/SizeVariants.php +++ b/app/Models/Extensions/SizeVariants.php @@ -2,6 +2,7 @@ namespace App\Models\Extensions; +use App\Actions\SizeVariant\Delete; use App\Models\Photo; use App\Models\SizeVariant; use Illuminate\Contracts\Support\Arrayable; @@ -206,34 +207,44 @@ public function create(int $sizeVariantType, string $shortPath, int $width, int /** * Deletes all size variants incl. the files from storage. * - * @param bool $keepOriginalFile if true, the original size variant is - * still removed from the DB and the model, - * but the media file is kept - * @param bool $keepAllFiles if true, all size variants are still - * removed from the DB and the model, but - * the media files are kept - * * @return bool True on success, false otherwise */ - public function deleteAll(bool $keepOriginalFile = false, bool $keepAllFiles = false): bool + public function deleteAll(): bool { - $success = true; - $success &= !$this->original || $this->original->delete($keepOriginalFile || $keepAllFiles); - $this->original = null; - $success &= !$this->medium2x || $this->medium2x->delete($keepAllFiles); - $this->medium2x = null; - $success &= !$this->medium || $this->medium->delete($keepAllFiles); - $this->medium = null; - $success &= !$this->small2x || $this->small2x->delete($keepAllFiles); - $this->small2x = null; - $success &= !$this->small || $this->small->delete($keepAllFiles); - $this->small = null; - $success &= !$this->thumb2x || $this->thumb2x->delete($keepAllFiles); - $this->thumb2x = null; - $success &= !$this->thumb || $this->thumb->delete($keepAllFiles); - $this->thumb = null; + $ids = []; + + if ($this->original) { + $ids[] = $this->original->id; + $this->original = null; + } + if ($this->medium2x) { + $ids[] = $this->medium2x->id; + $this->medium2x = null; + } + if ($this->medium) { + $ids[] = $this->medium->id; + $this->medium = null; + } + if ($this->small2x) { + $ids[] = $this->small2x->id; + $this->small2x = null; + } + if ($this->small) { + $ids[] = $this->small->id; + $this->small = null; + } + if ($this->thumb2x) { + $ids[] = $this->thumb2x->id; + $this->thumb2x = null; + } + if ($this->thumb) { + $ids[] = $this->thumb->id; + $this->thumb = null; + } + + $fileDeleter = (new Delete())->do($ids); - return $success; + return $fileDeleter->do(); } public function replicate(Photo $duplicatePhoto): SizeVariants diff --git a/app/Models/Photo.php b/app/Models/Photo.php index 363ee096d0..205bd057a7 100644 --- a/app/Models/Photo.php +++ b/app/Models/Photo.php @@ -2,6 +2,7 @@ namespace App\Models; +use App\Actions\Photo\Delete; use App\Casts\DateTimeWithTimezoneCast; use App\Casts\MustNotSetCast; use App\Contracts\HasRandomID; @@ -427,25 +428,13 @@ public function replicate(array $except = null): Photo return $duplicate; } - public function delete(): bool + /** + * {@inheritDoc} + */ + protected function performDeleteOnModel(): void { - $keepFiles = $this->hasDuplicate(); - if ($keepFiles) { - Logs::notice(__METHOD__, __LINE__, $this->id . ' is a duplicate, files are not deleted!'); - } - $success = true; - // Delete all size variants - $success &= $this->size_variants->deleteAll($keepFiles, $keepFiles); - // Delete Live Photo Video file - $livePhotoShortPath = $this->live_photo_short_path; - if (!$keepFiles && !empty($livePhotoShortPath) && Storage::exists($livePhotoShortPath)) { - $success &= Storage::delete($livePhotoShortPath); - } - - if (!$success) { - return false; - } - - return parent::delete() !== false; + $fileDeleter = (new Delete())->do([$this->id]); + $this->exists = false; + $fileDeleter->do(); } } diff --git a/app/Models/SizeVariant.php b/app/Models/SizeVariant.php index edd9a68d09..43bd74ed77 100644 --- a/app/Models/SizeVariant.php +++ b/app/Models/SizeVariant.php @@ -2,6 +2,7 @@ namespace App\Models; +use App\Actions\SizeVariant\Delete; use App\Casts\MustNotSetCast; use App\Facades\AccessControl; use App\Image\FlysystemFile; @@ -212,39 +213,12 @@ public function setSizeVariantAttribute(int $sizeVariantType): void } /** - * Deletes this model. - * - * @param bool $keepFile If true, the associated file is not removed from storage - * - * @return bool True on success, false otherwise + * {@inheritDoc} */ - public function delete(bool $keepFile = false): bool + protected function performDeleteOnModel(): void { - // Delete all symbolic links pointing to this size variant - // The SymLink model takes care of actually erasing - // the physical symbolic links from disk. - // We must not use a "mass deletion" like $this->sym_links()->delete() - // here, because this doesn't invoke the method `delete` on the model - // and thus the would not delete any actual symbolic link from disk. - $symLinks = $this->sym_links; - /** @var SymLink $symLink */ - foreach ($symLinks as $symLink) { - if ($symLink->delete() === false) { - return false; - } - } - - // Delete the actual media file - if (!$keepFile) { - $disk = Storage::disk(); - $shortPath = $this->short_path; - if (!empty($shortPath) && $disk->exists($shortPath)) { - if ($disk->delete($shortPath) === false) { - return false; - } - } - } - - return parent::delete() !== false; + $fileDeleter = (new Delete())->do([$this->id]); + $this->exists = false; + $fileDeleter->do(); } } diff --git a/app/Models/SymLink.php b/app/Models/SymLink.php index 72434ac409..7a780587eb 100644 --- a/app/Models/SymLink.php +++ b/app/Models/SymLink.php @@ -124,7 +124,7 @@ public function delete(): bool $fullPath = Storage::disk(self::DISK_NAME)->path($this->short_path); // Laravel and Flysystem does not support symbolic links. // So we must use low-level methods here. - if ((is_link($fullPath) && !unlink($fullPath)) || (file_exists($fullPath)) && !is_link($fullPath)) { + if ((is_link($fullPath) && !unlink($fullPath)) || (file_exists($fullPath) && !is_link($fullPath))) { return false; } diff --git a/app/SmartAlbums/UnsortedAlbum.php b/app/SmartAlbums/UnsortedAlbum.php index 8e1b48605a..6f5217d149 100644 --- a/app/SmartAlbums/UnsortedAlbum.php +++ b/app/SmartAlbums/UnsortedAlbum.php @@ -2,8 +2,6 @@ namespace App\SmartAlbums; -use App\Facades\AccessControl; -use App\Models\Photo; use Illuminate\Database\Eloquent\Builder; class UnsortedAlbum extends BaseSmartAlbum @@ -38,30 +36,4 @@ public static function getInstance(): self return self::$instance; } - - /** - * "Deletes" the album of unsorted photos. - * - * Actually, the album itself is not deleted, because it is built-in. - * But all photos within the album which are owned by the current user - * are deleted. - * - * @return bool - */ - public function delete(): bool - { - $success = true; - if (!AccessControl::is_admin()) { - $photos = $this->photos()->where('owner_id', '=', AccessControl::id())->get(); - } else { - $photos = $this->photos()->get(); - } - /** @var Photo $photo */ - foreach ($photos as $photo) { - // This also takes care of proper deletion of physical files from disk - $success &= $photo->delete(); - } - - return $success; - } } diff --git a/database/migrations/2022_04_18_150400_add_index_for_delete.php b/database/migrations/2022_04_18_150400_add_index_for_delete.php new file mode 100644 index 0000000000..d7cc9f0958 --- /dev/null +++ b/database/migrations/2022_04_18_150400_add_index_for_delete.php @@ -0,0 +1,60 @@ +getConnection(); + $this->schemaManager = $connection->getDoctrineSchemaManager(); + } + + public function up() + { + Schema::table('size_variants', function (Blueprint $table) { + // This index is required by \App\Actions\SizeVariant\Delete::do() + // for `SizeVariant::query()` + $table->index(['short_path']); + }); + } + + public function down() + { + Schema::table('size_variants', function (Blueprint $table) { + $this->dropIndexIfExists($table, 'size_variants_short_path_index'); + }); + } + + /** + * A helper function that allows to drop an index if exists. + * + * @param Blueprint $table + * @param string $indexName + * + * @throws DBALException + */ + private function dropIndexIfExists(Blueprint $table, string $indexName) + { + $doctrineTable = $this->schemaManager->listTableDetails($table->getTable()); + if ($doctrineTable->hasIndex($indexName)) { + $table->dropIndex($indexName); + } + } +}