From ed8dd418774c3fcc6d1ccc13f807f1ebc792a6f9 Mon Sep 17 00:00:00 2001 From: Benjamin Frueh Date: Fri, 2 Oct 2026 14:44:33 +0200 Subject: [PATCH] fix(files_trashbin): make trashbin:expire continue on errors Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: Benjamin Frueh --- .../lib/Command/ExpireTrash.php | 19 ++++++--- .../tests/Command/ExpireTrashTest.php | 40 ++++++++++++++----- 2 files changed, 44 insertions(+), 15 deletions(-) diff --git a/apps/files_trashbin/lib/Command/ExpireTrash.php b/apps/files_trashbin/lib/Command/ExpireTrash.php index ba1091429763a..60586d3a27218 100644 --- a/apps/files_trashbin/lib/Command/ExpireTrash.php +++ b/apps/files_trashbin/lib/Command/ExpireTrash.php @@ -13,8 +13,10 @@ use OCA\Files_Trashbin\Trashbin; use OCP\Files\Folder; use OCP\Files\IRootFolder; +use OCP\Files\NotFoundException; use OCP\IUser; use OCP\IUserManager; +use Psr\Log\LoggerInterface; use Symfony\Component\Console\Helper\ProgressBar; use Symfony\Component\Console\Input\InputArgument; use Symfony\Component\Console\Input\InputInterface; @@ -27,6 +29,7 @@ public function __construct( private readonly ?Expiration $expiration, private readonly SetupManager $setupManager, private readonly IRootFolder $rootFolder, + private readonly LoggerInterface $logger, ) { parent::__construct(); } @@ -65,8 +68,6 @@ protected function execute(InputInterface $input, OutputInterface $output): int if ($user) { $output->writeln("Remove deleted files of $userId"); $this->expireTrashForUser($user, $output); - $output->writeln("Unknown user $userId"); - return 1; } else { $output->writeln("Unknown user $userId"); return 1; @@ -90,19 +91,27 @@ protected function execute(InputInterface $input, OutputInterface $output): int private function expireTrashForUser(IUser $user, OutputInterface $output): void { try { $trashRoot = $this->getTrashRoot($user); + if ($trashRoot === null) { + $output->writeln('No trashbin found for user ' . $user->getUID() . ', skipping', OutputInterface::VERBOSITY_VERBOSE); + return; + } Trashbin::expire($trashRoot, $user); } catch (\Throwable $e) { $output->writeln('Error while expiring trashbin for user ' . $user->getUID() . ''); - throw $e; + $this->logger->error('Error while expiring trashbin for user ' . $user->getUID(), ['exception' => $e]); } finally { $this->setupManager->tearDown(); } } - private function getTrashRoot(IUser $user): Folder { + private function getTrashRoot(IUser $user): ?Folder { $this->setupManager->setupForUser($user); - $folder = $this->rootFolder->getUserFolder($user->getUID())->getParent()->get('files_trashbin'); + try { + $folder = $this->rootFolder->getUserFolder($user->getUID())->getParent()->get('files_trashbin'); + } catch (NotFoundException) { + return null; + } if (!$folder instanceof Folder) { throw new \LogicException("Didn't expect files_trashbin to be a file instead of a folder"); } diff --git a/apps/files_trashbin/tests/Command/ExpireTrashTest.php b/apps/files_trashbin/tests/Command/ExpireTrashTest.php index 55859da0557b3..9b0b7cf6d7ee6 100644 --- a/apps/files_trashbin/tests/Command/ExpireTrashTest.php +++ b/apps/files_trashbin/tests/Command/ExpireTrashTest.php @@ -20,6 +20,7 @@ use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\Attributes\Group; use PHPUnit\Framework\MockObject\MockObject; +use Psr\Log\LoggerInterface; use Symfony\Component\Console\Input\InputInterface; use Symfony\Component\Console\Output\OutputInterface; use Test\TestCase; @@ -38,6 +39,7 @@ class ExpireTrashTest extends TestCase { private IUserManager $userManager; private IUser $user; private ITimeFactory&MockObject $timeFactory; + private LoggerInterface&MockObject $logger; protected function setUp(): void { parent::setUp(); @@ -46,6 +48,7 @@ protected function setUp(): void { $this->timeFactory = $this->createMock(ITimeFactory::class); $this->expiration = Server::get(Expiration::class); $this->invokePrivate($this->expiration, 'timeFactory', [$this->timeFactory]); + $this->logger = $this->createMock(LoggerInterface::class); $userId = self::getUniqueID('user'); $this->userManager = Server::get(IUserManager::class); @@ -91,24 +94,41 @@ public function testRetentionObligation(string $obligation, string $quota, int $ $trashFiles = Helper::getTrashFiles('/', $userId); $this->assertEquals(1, count($trashFiles)); - $outputInterface = $this->createMock(OutputInterface::class); - $inputInterface = $this->createMock(InputInterface::class); - $inputInterface->expects($this->any()) - ->method('getArgument') - ->with('user_id') - ->willReturn([$userId]); + $this->logger->expects($this->never())->method('error'); + $this->assertSame(0, $this->executeCommand([$userId])); + + $trashFiles = Helper::getTrashFiles('/', $userId); + $this->assertEquals($shouldExpire ? 0 : 1, count($trashFiles)); + } + + public function testUserWithoutTrashbinIsSkipped(): void { + $this->expiration->setRetentionObligation('auto'); + $this->logger->expects($this->never())->method('error'); + + $this->assertSame(0, $this->executeCommand([$this->user->getUID()])); + } + + public function testErrorDoesNotAbortRemainingUsers(): void { + $this->expiration->setRetentionObligation('auto'); + $this->userFolder->getParent()->newFile('files_trashbin'); + $this->logger->expects($this->exactly(2))->method('error'); + + $this->assertSame(0, $this->executeCommand([$this->user->getUID(), $this->user->getUID()])); + } + + private function executeCommand(array $userIds): int { + $input = $this->createMock(InputInterface::class); + $input->method('getArgument')->with('user_id')->willReturn($userIds); $command = new ExpireTrash( Server::get(IUserManager::class), $this->expiration, Server::get(SetupManager::class), Server::get(IRootFolder::class), + $this->logger, ); - $this->invokePrivate($command, 'execute', [$inputInterface, $outputInterface]); - - $trashFiles = Helper::getTrashFiles('/', $userId); - $this->assertEquals($shouldExpire ? 0 : 1, count($trashFiles)); + return $this->invokePrivate($command, 'execute', [$input, $this->createMock(OutputInterface::class)]); } public static function retentionObligationProvider(): array {