From f4c89a6cc6af1cc7daf390a7e89d0c3fc460b9e6 Mon Sep 17 00:00:00 2001 From: Carl Schwan Date: Wed, 6 Aug 2025 13:40:37 +0200 Subject: [PATCH] refactor(trash): Port deletion code of Trashbin to node based API Instead of using a mix of View and Node based file system manipulation, use the 'new' node based API everywhere. Replace the hooks used in the admin_audit related to the deletion to new typed event that expose the deleted nodes. And remove old calculateSize to get the size of the deleted folder and instead rely on the Node::getSize information. Signed-off-by: Carl Schwan --- .../composer/composer/autoload_classmap.php | 1 + .../composer/composer/autoload_static.php | 1 + apps/admin_audit/lib/AppInfo/Application.php | 14 +- .../lib/Listener/TrashbinEventListener.php | 36 ++++ .../composer/composer/autoload_classmap.php | 2 + .../composer/composer/autoload_static.php | 2 + .../lib/Events/BeforeNodeDeletedEvent.php | 31 +++ .../lib/Events/NodeDeletedEvent.php | 30 +++ apps/files_trashbin/lib/Sabre/TrashRoot.php | 4 +- apps/files_trashbin/lib/Trashbin.php | 183 +++++++----------- build/psalm-baseline.xml | 32 --- 11 files changed, 186 insertions(+), 150 deletions(-) create mode 100644 apps/admin_audit/lib/Listener/TrashbinEventListener.php create mode 100644 apps/files_trashbin/lib/Events/BeforeNodeDeletedEvent.php create mode 100644 apps/files_trashbin/lib/Events/NodeDeletedEvent.php diff --git a/apps/admin_audit/composer/composer/autoload_classmap.php b/apps/admin_audit/composer/composer/autoload_classmap.php index 9f496d408eccb..87cf6ea5b3120 100644 --- a/apps/admin_audit/composer/composer/autoload_classmap.php +++ b/apps/admin_audit/composer/composer/autoload_classmap.php @@ -26,5 +26,6 @@ 'OCA\\AdminAudit\\Listener\\SecurityEventListener' => $baseDir . '/../lib/Listener/SecurityEventListener.php', 'OCA\\AdminAudit\\Listener\\SharingEventListener' => $baseDir . '/../lib/Listener/SharingEventListener.php', 'OCA\\AdminAudit\\Listener\\TagEventListener' => $baseDir . '/../lib/Listener/TagEventListener.php', + 'OCA\\AdminAudit\\Listener\\TrashbinEventListener' => $baseDir . '/../lib/Listener/TrashbinEventListener.php', 'OCA\\AdminAudit\\Listener\\UserManagementEventListener' => $baseDir . '/../lib/Listener/UserManagementEventListener.php', ); diff --git a/apps/admin_audit/composer/composer/autoload_static.php b/apps/admin_audit/composer/composer/autoload_static.php index 4be8380ff543a..ad2505548f876 100644 --- a/apps/admin_audit/composer/composer/autoload_static.php +++ b/apps/admin_audit/composer/composer/autoload_static.php @@ -41,6 +41,7 @@ class ComposerStaticInitAdminAudit 'OCA\\AdminAudit\\Listener\\SecurityEventListener' => __DIR__ . '/..' . '/../lib/Listener/SecurityEventListener.php', 'OCA\\AdminAudit\\Listener\\SharingEventListener' => __DIR__ . '/..' . '/../lib/Listener/SharingEventListener.php', 'OCA\\AdminAudit\\Listener\\TagEventListener' => __DIR__ . '/..' . '/../lib/Listener/TagEventListener.php', + 'OCA\\AdminAudit\\Listener\\TrashbinEventListener' => __DIR__ . '/..' . '/../lib/Listener/TrashbinEventListener.php', 'OCA\\AdminAudit\\Listener\\UserManagementEventListener' => __DIR__ . '/..' . '/../lib/Listener/UserManagementEventListener.php', ); diff --git a/apps/admin_audit/lib/AppInfo/Application.php b/apps/admin_audit/lib/AppInfo/Application.php index 85e215ccbc448..cd1d13b8374e6 100644 --- a/apps/admin_audit/lib/AppInfo/Application.php +++ b/apps/admin_audit/lib/AppInfo/Application.php @@ -27,7 +27,10 @@ use OCA\AdminAudit\Listener\SecurityEventListener; use OCA\AdminAudit\Listener\SharingEventListener; use OCA\AdminAudit\Listener\TagEventListener; +use OCA\AdminAudit\Listener\TrashbinEventListener; use OCA\AdminAudit\Listener\UserManagementEventListener; +use OCA\Files_Trashbin\Events\BeforeNodeDeletedEvent as TrashbinBeforeNodeDeletedEvent; +use OCA\Files_Trashbin\Events\NodeRestoredEvent; use OCA\Files_Versions\Events\VersionRestoredEvent; use OCP\App\Events\AppDisableEvent; use OCP\App\Events\AppEnableEvent; @@ -130,6 +133,10 @@ public function register(IRegistrationContext $context): void { // System tag event $context->registerEventListener(TagCreatedEvent::class, TagEventListener::class); + + // Trashbin events + $context->registerEventListener(TrashbinBeforeNodeDeletedEvent::class, TrashbinEventListener::class); + $context->registerEventListener(NodeRestoredEvent::class, TrashbinEventListener::class); } #[\Override] @@ -152,7 +159,6 @@ private function registerLegacyHooks(IAuditLogger $logger, ContainerInterface $s $eventDispatcher = $serverContainer->get(IEventDispatcher::class); $this->sharingLegacyHooks($logger); $this->fileHooks($logger, $eventDispatcher); - $this->trashbinHooks($logger); $this->versionsHooks($logger); } @@ -215,10 +221,4 @@ private function versionsHooks(IAuditLogger $logger): void { $versionsActions = new Versions($logger); Util::connectHook('\OCP\Versions', 'delete', $versionsActions, 'delete'); } - - private function trashbinHooks(IAuditLogger $logger): void { - $trashActions = new Trashbin($logger); - Util::connectHook('\OCP\Trashbin', 'preDelete', $trashActions, 'delete'); - Util::connectHook('\OCA\Files_Trashbin\Trashbin', 'post_restore', $trashActions, 'restore'); - } } diff --git a/apps/admin_audit/lib/Listener/TrashbinEventListener.php b/apps/admin_audit/lib/Listener/TrashbinEventListener.php new file mode 100644 index 0000000000000..02d167b6f4909 --- /dev/null +++ b/apps/admin_audit/lib/Listener/TrashbinEventListener.php @@ -0,0 +1,36 @@ + + */ +class TrashbinEventListener extends Action implements IEventListener { + + #[Override] + public function handle(Event $event): void { + if ($event instanceof BeforeNodeDeletedEvent) { + $this->log('File "%s" deleted from trash bin.', + ['path' => $event->getSource()->getPath()], ['path'] + ); + } elseif ($event instanceof NodeRestoredEvent) { + $this->log('File "%s" restored from trash bin.', + ['path' => $event->getTarget()->getPath()], ['path'] + ); + } + } +} diff --git a/apps/files_trashbin/composer/composer/autoload_classmap.php b/apps/files_trashbin/composer/composer/autoload_classmap.php index 23e2e7baff64c..af10eb102c612 100644 --- a/apps/files_trashbin/composer/composer/autoload_classmap.php +++ b/apps/files_trashbin/composer/composer/autoload_classmap.php @@ -16,8 +16,10 @@ 'OCA\\Files_Trashbin\\Command\\RestoreAllFiles' => $baseDir . '/../lib/Command/RestoreAllFiles.php', 'OCA\\Files_Trashbin\\Command\\Size' => $baseDir . '/../lib/Command/Size.php', 'OCA\\Files_Trashbin\\Controller\\PreviewController' => $baseDir . '/../lib/Controller/PreviewController.php', + 'OCA\\Files_Trashbin\\Events\\BeforeNodeDeletedEvent' => $baseDir . '/../lib/Events/BeforeNodeDeletedEvent.php', 'OCA\\Files_Trashbin\\Events\\BeforeNodeRestoredEvent' => $baseDir . '/../lib/Events/BeforeNodeRestoredEvent.php', 'OCA\\Files_Trashbin\\Events\\MoveToTrashEvent' => $baseDir . '/../lib/Events/MoveToTrashEvent.php', + 'OCA\\Files_Trashbin\\Events\\NodeDeletedEvent' => $baseDir . '/../lib/Events/NodeDeletedEvent.php', 'OCA\\Files_Trashbin\\Events\\NodeRestoredEvent' => $baseDir . '/../lib/Events/NodeRestoredEvent.php', 'OCA\\Files_Trashbin\\Exceptions\\CopyRecursiveException' => $baseDir . '/../lib/Exceptions/CopyRecursiveException.php', 'OCA\\Files_Trashbin\\Expiration' => $baseDir . '/../lib/Expiration.php', diff --git a/apps/files_trashbin/composer/composer/autoload_static.php b/apps/files_trashbin/composer/composer/autoload_static.php index 307f813aadb6b..4f9d1dc87226b 100644 --- a/apps/files_trashbin/composer/composer/autoload_static.php +++ b/apps/files_trashbin/composer/composer/autoload_static.php @@ -31,8 +31,10 @@ class ComposerStaticInitFiles_Trashbin 'OCA\\Files_Trashbin\\Command\\RestoreAllFiles' => __DIR__ . '/..' . '/../lib/Command/RestoreAllFiles.php', 'OCA\\Files_Trashbin\\Command\\Size' => __DIR__ . '/..' . '/../lib/Command/Size.php', 'OCA\\Files_Trashbin\\Controller\\PreviewController' => __DIR__ . '/..' . '/../lib/Controller/PreviewController.php', + 'OCA\\Files_Trashbin\\Events\\BeforeNodeDeletedEvent' => __DIR__ . '/..' . '/../lib/Events/BeforeNodeDeletedEvent.php', 'OCA\\Files_Trashbin\\Events\\BeforeNodeRestoredEvent' => __DIR__ . '/..' . '/../lib/Events/BeforeNodeRestoredEvent.php', 'OCA\\Files_Trashbin\\Events\\MoveToTrashEvent' => __DIR__ . '/..' . '/../lib/Events/MoveToTrashEvent.php', + 'OCA\\Files_Trashbin\\Events\\NodeDeletedEvent' => __DIR__ . '/..' . '/../lib/Events/NodeDeletedEvent.php', 'OCA\\Files_Trashbin\\Events\\NodeRestoredEvent' => __DIR__ . '/..' . '/../lib/Events/NodeRestoredEvent.php', 'OCA\\Files_Trashbin\\Exceptions\\CopyRecursiveException' => __DIR__ . '/..' . '/../lib/Exceptions/CopyRecursiveException.php', 'OCA\\Files_Trashbin\\Expiration' => __DIR__ . '/..' . '/../lib/Expiration.php', diff --git a/apps/files_trashbin/lib/Events/BeforeNodeDeletedEvent.php b/apps/files_trashbin/lib/Events/BeforeNodeDeletedEvent.php new file mode 100644 index 0000000000000..400dcea0e3cff --- /dev/null +++ b/apps/files_trashbin/lib/Events/BeforeNodeDeletedEvent.php @@ -0,0 +1,31 @@ +source; + } +} diff --git a/apps/files_trashbin/lib/Events/NodeDeletedEvent.php b/apps/files_trashbin/lib/Events/NodeDeletedEvent.php new file mode 100644 index 0000000000000..34101f5d37fae --- /dev/null +++ b/apps/files_trashbin/lib/Events/NodeDeletedEvent.php @@ -0,0 +1,30 @@ +sourcePath; + } +} diff --git a/apps/files_trashbin/lib/Sabre/TrashRoot.php b/apps/files_trashbin/lib/Sabre/TrashRoot.php index 9c730c69d8770..afb0c87162d40 100644 --- a/apps/files_trashbin/lib/Sabre/TrashRoot.php +++ b/apps/files_trashbin/lib/Sabre/TrashRoot.php @@ -20,7 +20,6 @@ use Sabre\DAV\ICollection; class TrashRoot implements ICollection { - public function __construct( private IUser $user, private ITrashManager $trashManager, @@ -33,7 +32,7 @@ public function delete() { throw new Forbidden('Not allowed to delete items from the trash bin'); } - Trashbin::deleteAll(); + Trashbin::deleteAll($this->user); foreach ($this->trashManager->listTrashRoot($this->user) as $trashItem) { $this->trashManager->removeItem($trashItem); } @@ -75,6 +74,7 @@ public function getChildren(): array { #[\Override] public function getChild($name): ITrash { + /** @var list $entries */ $entries = $this->getChildren(); foreach ($entries as $entry) { diff --git a/apps/files_trashbin/lib/Trashbin.php b/apps/files_trashbin/lib/Trashbin.php index b32b2ccb7c687..f8cc53d70535c 100644 --- a/apps/files_trashbin/lib/Trashbin.php +++ b/apps/files_trashbin/lib/Trashbin.php @@ -43,7 +43,6 @@ use OCP\Files\Storage\IStorage; use OCP\FilesMetadata\IFilesMetadataManager; use OCP\IAppConfig; -use OCP\IConfig; use OCP\IDBConnection; use OCP\IRequest; use OCP\IURLGenerator; @@ -615,7 +614,7 @@ public static function restore($file, $filename, $timestamp) { * @param string $uniqueFilename new file name to restore the file without overwriting existing files * @param string $location location if file * @param int $timestamp deletion time - * @return false|null + * @return bool|null */ private static function restoreVersions(View $view, $file, $filename, $uniqueFilename, $location, $timestamp) { if (Server::get(IAppManager::class)->isEnabledForUser('files_versions')) { @@ -648,53 +647,50 @@ private static function restoreVersions(View $view, $file, $filename, $uniqueFil } } } + return true; } + return null; } /** * delete all files from the trash */ - public static function deleteAll() { - $user = OC_User::getUser(); - $userRoot = \OC::$server->getUserFolder($user)->getParent(); - $view = new View('/' . $user); - $fileInfos = $view->getDirectoryContent('files_trashbin/files'); + public static function deleteAll(IUser $user): bool { + $rootFolder = Server::get(IRootFolder::class); + $dispatcher = Server::get(IEventDispatcher::class); + $dbConnection = Server::get(IDBConnection::class); + + $userRoot = $rootFolder->getUserFolder($user->getUID())->getParent(); try { - $trash = $userRoot->get('files_trashbin'); - } catch (NotFoundException $e) { + /** @var Folder $trashRoot */ + $trashRoot = $userRoot->get('files_trashbin'); + /** @var Folder $trashFilesRoot */ + $trashFilesRoot = $trashRoot->get('files'); + } catch (NotFoundException|NotPermittedException) { return false; } - // Array to store the relative path in (after the file is deleted, the view won't be able to relativise the path anymore) - $filePaths = []; - foreach ($fileInfos as $fileInfo) { - $filePaths[] = $view->getRelativePath($fileInfo->getPath()); - } - unset($fileInfos); // save memory - - // Bulk PreDelete-Hook - \OC_Hook::emit('\OCP\Trashbin', 'preDeleteAll', ['paths' => $filePaths]); + $trashNodes = $trashFilesRoot->getDirectoryListing(); // Single-File Hooks - foreach ($filePaths as $path) { - self::emitTrashbinPreDelete($path); + foreach ($trashNodes as $trashNode) { + $event = new Events\BeforeNodeDeletedEvent($trashNode); + $dispatcher->dispatchTyped($event); } // actual file deletion - $trash->delete(); + $trashRoot->delete(); - $query = Server::get(IDBConnection::class)->getQueryBuilder(); + $query = $dbConnection->getQueryBuilder(); $query->delete('files_trash') - ->where($query->expr()->eq('user', $query->createNamedParameter($user))); + ->where($query->expr()->eq('user', $query->createNamedParameter($user->getUID()))); $query->executeStatement(); - // Bulk PostDelete-Hook - \OC_Hook::emit('\OCP\Trashbin', 'deleteAll', ['paths' => $filePaths]); - // Single-File Hooks - foreach ($filePaths as $path) { - self::emitTrashbinPostDelete($path); + foreach ($trashNodes as $trashNode) { + $event = new Events\NodeDeletedEvent($trashNode->getPath()); + $dispatcher->dispatchTyped($event); } $trash = $userRoot->newFolder('files_trashbin'); @@ -704,35 +700,26 @@ public static function deleteAll() { } /** - * wrapper function to emit the 'preDelete' hook of \OCP\Trashbin before a file is deleted - * - * @param string $path - */ - protected static function emitTrashbinPreDelete($path) { - \OC_Hook::emit('\OCP\Trashbin', 'preDelete', ['path' => $path]); - } - - /** - * wrapper function to emit the 'delete' hook of \OCP\Trashbin after a file has been deleted - * - * @param string $path - */ - protected static function emitTrashbinPostDelete($path) { - \OC_Hook::emit('\OCP\Trashbin', 'delete', ['path' => $path]); - } - - /** - * delete file from trash bin permanently + * Delete file from trash bin permanently * * @param string $filename path to the file - * @param string $user - * @param int $timestamp of deletion time - * + * @param ?int $timestamp of deletion time * @return int|float size of deleted files */ - public static function delete($filename, $user, $timestamp = null) { - $userRoot = \OC::$server->getUserFolder($user)->getParent(); - $view = new View('/' . $user); + public static function delete(string $filename, string $user, ?int $timestamp = null): int|float { + $rootFolder = Server::get(IRootFolder::class); + $appManager = Server::get(IAppManager::class); + $dispatcher = Server::get(IEventDispatcher::class); + + $userRoot = $rootFolder->getUserFolder($user)->getParent(); + + try { + /** @var Folder $trashRoot */ + $trashRoot = $userRoot->get('files_trashbin'); + } catch (NotFoundException|NotPermittedException) { + return 0; + } + $size = 0; if ($timestamp) { @@ -741,7 +728,9 @@ public static function delete($filename, $user, $timestamp = null) { $file = $filename; } - $size += self::deleteVersions($view, $file, $filename, $timestamp, $user); + if ($appManager->isEnabledForUser('files_versions')) { + $size += Trashbin::deleteVersions($trashRoot, $file, $filename, $timestamp, $user); + } try { $node = $userRoot->get('/files_trashbin/files/' . $file); @@ -752,15 +741,15 @@ public static function delete($filename, $user, $timestamp = null) { return $size; } - if ($node instanceof Folder) { - $size += self::calculateSize(new View('/' . $user . '/files_trashbin/files/' . $file)); - } elseif ($node instanceof File) { - $size += $view->filesize('/files_trashbin/files/' . $file); - } + $size += $node->getSize(); + + $event = new Events\BeforeNodeDeletedEvent($node); + $dispatcher->dispatchTyped($event); - self::emitTrashbinPreDelete('/files_trashbin/files/' . $file); $node->delete(); - self::emitTrashbinPostDelete('/files_trashbin/files/' . $file); + + $event = new Events\NodeDeletedEvent($node->getPath()); + $dispatcher->dispatchTyped($event); if ($timestamp) { self::deleteTrashRow($user, $filename, $timestamp); @@ -779,26 +768,33 @@ private static function deleteTrashRow(string $user, string $filename, int $time } /** - * @param string $file - * @param string $filename - * @param ?int $timestamp + * Delete version files corresponding to a given file from trash bin permanently. */ - private static function deleteVersions(View $view, $file, $filename, $timestamp, string $user): int|float { + private static function deleteVersions(Folder $trashRoot, string $file, string $filename, ?int $timestamp, string $user): int|float { + if (!Server::get(IAppManager::class)->isEnabledForUser('files_versions')) { + return 0; + } + $size = 0; - if (Server::get(IAppManager::class)->isEnabledForUser('files_versions')) { - if ($view->is_dir('files_trashbin/versions/' . $file)) { - $size += self::calculateSize(new View('/' . $user . '/files_trashbin/versions/' . $file)); - $view->unlink('files_trashbin/versions/' . $file); - } elseif ($versions = self::getVersionsFromTrash($filename, $timestamp, $user)) { - foreach ($versions as $v) { - if ($timestamp) { - $size += $view->filesize('/files_trashbin/versions/' . static::getTrashFilename($filename . '.v' . $v, $timestamp)); - $view->unlink('/files_trashbin/versions/' . static::getTrashFilename($filename . '.v' . $v, $timestamp)); - } else { - $size += $view->filesize('/files_trashbin/versions/' . $filename . '.v' . $v); - $view->unlink('/files_trashbin/versions/' . $filename . '.v' . $v); - } + try { + $fileVersion = $trashRoot->get('versions/' . $file); + } catch (NotFoundException) { + $fileVersion = null; + } + + if ($fileVersion) { + $size += $fileVersion->getSize(); + $fileVersion->delete(); + } elseif ($versions = self::getVersionsFromTrash($filename, $timestamp, $user)) { + foreach ($versions as $v) { + if ($timestamp) { + $node = $trashRoot->get('/files_trashbin/versions/' . static::getTrashFilename($filename . '.v' . $v, $timestamp)); + } else { + $node = $trashRoot->get('/files_trashbin/versions/' . $filename . '.v' . $v); } + + $size += $node->getSize(); + $node->delete(); } } return $size; @@ -1133,37 +1129,6 @@ private static function getUniqueFilename($location, $filename, View $view) { return $filename; } - /** - * get the size from a given root folder - * - * @param View $view file view on the root folder - * @return int|float size of the folder - */ - private static function calculateSize(View $view): int|float { - $root = Server::get(IConfig::class)->getSystemValue('datadirectory', \OC::$SERVERROOT . '/data') . $view->getAbsolutePath(''); - if (!file_exists($root)) { - return 0; - } - $iterator = new \RecursiveIteratorIterator(new \RecursiveDirectoryIterator($root), \RecursiveIteratorIterator::CHILD_FIRST); - $size = 0; - - /** - * RecursiveDirectoryIterator on an NFS path isn't iterable with foreach - * This bug is fixed in PHP 5.5.9 or before - * See #8376 - */ - $iterator->rewind(); - while ($iterator->valid()) { - $path = $iterator->current(); - $relpath = substr($path, strlen($root) - 1); - if (!$view->is_dir($relpath)) { - $size += $view->filesize($relpath); - } - $iterator->next(); - } - return $size; - } - /** * get current size of trash bin from a given user * diff --git a/build/psalm-baseline.xml b/build/psalm-baseline.xml index 49e45aad9f0b2..876bef0b78371 100644 --- a/build/psalm-baseline.xml +++ b/build/psalm-baseline.xml @@ -8,8 +8,6 @@ - - @@ -1942,14 +1940,6 @@ - - - - - - - - @@ -1973,18 +1963,12 @@ Filesystem::normalizePath($file_path), 'trashPath' => Filesystem::normalizePath(static::getTrashFilename($filename, $timestamp))])]]> $targetPath, 'trashPath' => $sourcePath])]]> - - - - - - @@ -2010,18 +1994,11 @@ - - - - - - - @@ -2035,8 +2012,6 @@ - - @@ -2045,11 +2020,7 @@ - - - - @@ -2072,9 +2043,6 @@ - - -