[stable34] fix(files_trashbin): make trashbin:expire continue on errors

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 <info>$userId</info>");
 					$this->expireTrashForUser($user, $output);
-					$output->writeln("<error>Unknown user $userId</error>");
-					return 1;
 				} else {
 					$output->writeln("<error>Unknown user $userId</error>");
 					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 <info>' . $user->getUID() . '</info>, skipping', OutputInterface::VERBOSITY_VERBOSE);
+				return;
+			}
 			Trashbin::expire($trashRoot, $user);
 		} catch (\Throwable $e) {
 			$output->writeln('<error>Error while expiring trashbin for user ' . $user->getUID() . '</error>');
-			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");
 		}

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 {