diff --git a/lib/Operation.php b/lib/Operation.php index 5adb050..d941d95 100644 --- a/lib/Operation.php +++ b/lib/Operation.php @@ -46,32 +46,15 @@ use Psr\Log\LoggerInterface; class Operation implements ISpecificOperation { - /** @var IJobList */ - private $jobList; - /** @var IL10N */ - private $l; - /** @var LoggerInterface */ - private $logger; - /** @var IURLGenerator */ - private $urlGenerator; - /** @var IProcessingFileAccessor */ - private $processingFileAccessor; - /** @var IRootFolder */ - private $rootFolder; public function __construct( - IJobList $jobList, - IL10N $l, - LoggerInterface $logger, - IURLGenerator $urlGenerator, - IProcessingFileAccessor $processingFileAccessor, - IRootFolder $rootFolder) { - $this->jobList = $jobList; - $this->l = $l; - $this->logger = $logger; - $this->urlGenerator = $urlGenerator; - $this->processingFileAccessor = $processingFileAccessor; - $this->rootFolder = $rootFolder; + private IJobList $jobList, + private IL10N $l, + private LoggerInterface $logger, + private IURLGenerator $urlGenerator, + private IProcessingFileAccessor $processingFileAccessor, + private IRootFolder $rootFolder, + ) { } /** @@ -178,8 +161,10 @@ private function tryGetFileFromMapperEvent(string $eventName, MapperEvent $event return false; } - $files = $this->rootFolder->getById($fileId); - if (empty($files) || !($files[0] instanceof \OCP\Files\File)) { + // #324: The root folder object can potentially return multiple nodes with the same id when using 'getById'. + // Threfore we use 'getFirstNodeById' here to ensure we only get one node which belongs to the current user. + $file = $this->rootFolder->getFirstNodeById($fileId); + if (!$file || !($file instanceof \OCP\Files\File)) { $this->logger->warning( 'Not processing event {eventname} because node with id \'{fileid}\' could not be found or is not a file.', ['eventname' => $eventName], @@ -187,7 +172,7 @@ private function tryGetFileFromMapperEvent(string $eventName, MapperEvent $event return false; } - $node = $files[0]; + $node = $file; return true; } diff --git a/lib/Service/OcrService.php b/lib/Service/OcrService.php index 84af5ca..e7777e1 100644 --- a/lib/Service/OcrService.php +++ b/lib/Service/OcrService.php @@ -217,14 +217,14 @@ private function shutdownUserEnvironment() : void { } private function getNode(int $fileId) : Node { - /** @var File[] */ - $nodeArr = $this->rootFolder->getById($fileId); - if (count($nodeArr) === 0) { + // #324: The root folder object can potentially return multiple nodes with the same id when using 'getById'. + // Therefore we use 'getFirstNodeById' here to ensure we only get one node which belongs to the current user. + $node = $this->rootFolder->getFirstNodeById($fileId); + + if ($node === null) { throw new NotFoundException('Could not process file with id \'' . $fileId . '\'. File was not found'); } - $node = array_shift($nodeArr); - if (!$node instanceof Node || $node->getType() !== FileInfo::TYPE_FILE) { throw new \InvalidArgumentException('Skipping process for file with id \'' . $fileId . '\'. It is not a file'); } diff --git a/tests/Unit/OperationTest.php b/tests/Unit/OperationTest.php index 73c4818..e9c9b92 100644 --- a/tests/Unit/OperationTest.php +++ b/tests/Unit/OperationTest.php @@ -323,9 +323,9 @@ public function testDoesNothingOnMapperEventIfFileNotFound() { /** @var MockObject|IRootFolder */ $this->rootFolder = $this->createMock(IRootFolder::class); $this->rootFolder->expects($this->once()) - ->method('getById') + ->method('getFirstNodeById') ->with(42) - ->willReturn([]); + ->willReturn(null); $operation = new Operation($this->jobList, $this->l, $this->logger, $this->urlGenerator, $this->processingFileAccessor, $this->rootFolder); @@ -344,9 +344,9 @@ public function testDoesNothingOnMapperEventIfObjectIdIsFolder() { /** @var MockObject|IRootFolder */ $this->rootFolder = $this->createMock(IRootFolder::class); $this->rootFolder->expects($this->once()) - ->method('getById') + ->method('getFirstNodeById') ->with(42) - ->willReturn([$this->createMock(Folder::class)]); + ->willReturn($this->createMock(Folder::class)); $operation = new Operation($this->jobList, $this->l, $this->logger, $this->urlGenerator, $this->processingFileAccessor, $this->rootFolder); @@ -375,10 +375,11 @@ public function testFileAddedToQueueOnTagAssignedEvent() { ->willReturn($fileId); /** @var MockObject|IRootFolder */ $rootFolder = $this->createMock(IRootFolder::class); + $rootFolder->expects($this->never())->method('getById'); $rootFolder->expects($this->once()) - ->method('getById') + ->method('getFirstNodeById') ->with($fileId) - ->willReturn([$fileMock]); + ->willReturn($fileMock); $operation = new Operation($this->jobList, $this->l, $this->logger, $this->urlGenerator, $this->processingFileAccessor, $rootFolder); diff --git a/tests/Unit/Service/OcrServiceTest.php b/tests/Unit/Service/OcrServiceTest.php index f6f7034..1b0d1d4 100644 --- a/tests/Unit/Service/OcrServiceTest.php +++ b/tests/Unit/Service/OcrServiceTest.php @@ -94,7 +94,7 @@ class OcrServiceTest extends TestCase { /** @var INotificationService|MockObject */ private $notificationService; /** @var File[] */ - private $rootFolderGetById42ReturnValue; + private $rootFolderGetFirstNodeById42ReturnValue; /** @var OcrService */ private $ocrService; /** @var File|MockObject */ @@ -124,12 +124,12 @@ public function setUp() : void { /** @var MockObject|IRootFolder */ $this->rootFolder = $this->createMock(IRootFolder::class); - $this->rootFolderGetById42ReturnValue = [$this->createValidFileMock()]; + $this->rootFolderGetFirstNodeById42ReturnValue = $this->createValidFileMock(); $this->rootFolder->expects($this->any()) - ->method('getById') + ->method('getFirstNodeById') ->with(42) ->willReturnCallback(function () { - return $this->rootFolderGetById42ReturnValue; + return $this->rootFolderGetFirstNodeById42ReturnValue; }); /** @var MockObject|IUserManager */ @@ -348,7 +348,7 @@ public function testCallsInitFilesystem() { public function testCallsGetOnRootFolder() { $settings = new WorkflowSettings(); $this->rootFolder->expects($this->once()) - ->method('getById') + ->method('getFirstNodeById') ->with(42); $this->ocrService->runOcrProcess(42, 'usr', $settings); @@ -361,7 +361,7 @@ public function testCallsOcrProcessorWithFile() { $mimeType = 'application/pdf'; $content = 'someFileContent'; $fileMock = $this->createValidFileMock($mimeType, $content); - $this->rootFolderGetById42ReturnValue = [$fileMock]; + $this->rootFolderGetFirstNodeById42ReturnValue = $fileMock; $this->globalSettingsService->expects($this->once()) ->method('getGlobalSettings') @@ -385,7 +385,7 @@ public function testCreatesNewFileVersionAndEmitsTextRecognizedEvent(string $ori $ocrResult = new OcrProcessorResult($ocrContent, 'pdf', $ocrContent); // Extend this cases if we add new OCR processors $originalFileMock = $this->createValidFileMock($mimeType, $content, $rootFolderPath, $originalFilename); - $this->rootFolderGetById42ReturnValue = [$originalFileMock]; + $this->rootFolderGetFirstNodeById42ReturnValue = $originalFileMock; $this->ocrProcessor->expects($this->once()) ->method('ocrFile') @@ -410,17 +410,18 @@ public function testCreatesNewFileVersionAndEmitsTextRecognizedEvent(string $ori public function testThrowsNotFoundExceptionWhenFileNotFound() { $settings = new WorkflowSettings(); - $this->rootFolderGetById42ReturnValue = []; + $this->rootFolderGetFirstNodeById42ReturnValue = null; $this->expectException(NotFoundException::class); $this->ocrService->runOcrProcess(42, 'usr', $settings); } - #[DataProvider('dataProvider_InvalidNodes')] - public function testDoesNotCallOcr_OnNonFile(callable $invalidNodeCallback) { - $invalidNode = $invalidNodeCallback($this); + public function testDoesNotCallOcr_OnNonFile() { + $invalidNode = $this->createMock(Node::class); + $invalidNode->method('getType') + ->willReturn(FileInfo::TYPE_FOLDER); $settings = new WorkflowSettings(); - $this->rootFolderGetById42ReturnValue = [$invalidNode]; + $this->rootFolderGetFirstNodeById42ReturnValue = $invalidNode; $this->ocrProcessor->expects($this->never()) ->method('ocrFile'); @@ -473,7 +474,7 @@ public function testCallsProcessingFileAccessor() { $filePath = '/admin/files/somefile.pdf'; $ocrResult = new OcrProcessorResult($ocrContent, 'pdf', $ocrContent); // Extend this cases if we add new OCR processors - $this->rootFolderGetById42ReturnValue = [$this->createValidFileMock($mimeType, $content)]; + $this->rootFolderGetFirstNodeById42ReturnValue = $this->createValidFileMock($mimeType, $content); $this->ocrProcessor->expects($this->once()) ->method('ocrFile') @@ -517,10 +518,12 @@ public function testDoesNotCreateNewFileVersionIfOcrContentWasEmpty() { $ocrResult = new OcrProcessorResult($ocrContent, 'pdf', $ocrContent); $fileId = 42; + $this->rootFolder->expects($this->never())->method('getById'); + $this->rootFolder->expects($this->once()) - ->method('getById') + ->method('getFirstNodeById') ->with($fileId) - ->willReturn([$this->createValidFileMock($mimeType, $content)]); + ->willReturn($this->createValidFileMock($mimeType, $content)); $this->ocrProcessor->expects($this->once()) ->method('ocrFile') @@ -600,7 +603,7 @@ public function testRestoreOriginalFileModificationDate() { $ocrResult = new OcrProcessorResult($ocrContent, 'pdf', $ocrContent); // Extend this cases if we add new OCR processors $fileMock = $this->createValidFileMock($mimeType, $content); - $this->rootFolderGetById42ReturnValue = [$fileMock]; + $this->rootFolderGetFirstNodeById42ReturnValue = $fileMock; $this->ocrProcessor->expects($this->once()) ->method('ocrFile') @@ -648,7 +651,8 @@ public function testRunOcrProcessWithJobArgumentLogsErrorAndDoesNothingOnInvalid } public function testRunOcrProcessWithJobArgumentLogsErrorAndSendsNotificationOnNotFound() { - $this->rootFolder->method('getById') + // This will never be thrown in real live, it's just to test the error handling + $this->rootFolder->method('getFirstNodeById') ->willThrowException(new NotFoundException('File was not found')); $this->logger->expects($this->once()) ->method('error') @@ -681,7 +685,7 @@ public function testCreatesNewFileVersionWithSuffixIfNodeIsNotUpdateable() { $ocrResult = new OcrProcessorResult($ocrContent, 'pdf', $ocrContent); // Extend this cases if we add new OCR processors $fileMock = $this->createValidFileMock($mimeType, $content, '/admin/files', 'somefile.pdf', false); - $this->rootFolderGetById42ReturnValue = [$fileMock]; + $this->rootFolderGetFirstNodeById42ReturnValue = $fileMock; $this->ocrProcessor->expects($this->once()) ->method('ocrFile') @@ -707,7 +711,7 @@ public function testSetsFileVersionsLabelIfKeepOriginalFileVersionIsTrue() { $ocrResult = new OcrProcessorResult($ocrContent, 'pdf', $ocrContent); $fileMock = $this->createValidFileMock($mimeType, $content); - $this->rootFolderGetById42ReturnValue = [$fileMock]; + $this->rootFolderGetFirstNodeById42ReturnValue = $fileMock; $this->ocrProcessor->expects($this->once()) ->method('ocrFile') @@ -756,7 +760,7 @@ public function testRunOcrProcessThrowsNonOcrAlreadyDoneExceptionIfModeIsNotSkip $content = 'someFileContent'; $fileMock = $this->createValidFileMock($mimeType, $content); - $this->rootFolderGetById42ReturnValue = [$fileMock]; + $this->rootFolderGetFirstNodeById42ReturnValue = $fileMock; $ex = new OcrAlreadyDoneException('oops'); $this->ocrProcessor->expects($this->once()) @@ -777,7 +781,7 @@ public function testRunOcrProcessWorksWithoutIVersionManager() { $ocrResult = new OcrProcessorResult($ocrContent, 'pdf', $ocrContent); $fileMock = $this->createValidFileMock($mimeType, $content); - $this->rootFolderGetById42ReturnValue = [$fileMock]; + $this->rootFolderGetFirstNodeById42ReturnValue = $fileMock; $this->ocrProcessor->expects($this->once()) ->method('ocrFile') @@ -822,24 +826,6 @@ public function testRunOcrProcessWorksWithoutIVersionManager() { $this->assertTrue($logged, 'Expected debug log message not found'); } - public static function dataProvider_InvalidNodes() { - $folderMockCallable = function (self $testClass) { - /** @var MockObject|Node */ - $folderMock = $testClass->createMock(Node::class); - $folderMock->method('getType') - ->willReturn(FileInfo::TYPE_FOLDER); - return $folderMock; - }; - $fileInfoMockCallable = fn (self $testClass) => $testClass->createMock(FileInfo::class); - $arr = [ - [$folderMockCallable], - [$fileInfoMockCallable], - [fn () => null], - [fn () => (object)['someField' => 'someValue']] - ]; - return $arr; - } - public static function dataProvider_OcrModesThrowOnEmptyResult() { return [ [WorkflowSettings::OCR_MODE_FORCE_OCR], diff --git a/tests/psalm-baseline.xml b/tests/psalm-baseline.xml index 66b0f2f..34c1c65 100644 --- a/tests/psalm-baseline.xml +++ b/tests/psalm-baseline.xml @@ -207,8 +207,7 @@ rootFolder]]> - - + @@ -216,14 +215,6 @@ - - - - - - - -