Skip to content

Commit cff45d2

Browse files
R0WiR0Wi
authored
Cherry pick fix wrong user nc31 (#331)
* Fix wrong user problem when using tag assignment in TeamFolders * Fix PHPUnit tearDown for LocalBackend Integrationtest Signed-off-by: Robin Windey <ro.windey@gmail.com> * Apply php-cs-fixer changes * Update psalm --------- Signed-off-by: Robin Windey <ro.windey@gmail.com> Co-authored-by: R0Wi <R0Wi@users.noreply.github.com>
1 parent 4bddf96 commit cff45d2

File tree

7 files changed

+76
-106
lines changed

7 files changed

+76
-106
lines changed

‎lib/Operation.php‎

Lines changed: 12 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -46,32 +46,15 @@
4646
use Psr\Log\LoggerInterface;
4747

4848
class Operation implements ISpecificOperation {
49-
/** @var IJobList */
50-
private $jobList;
51-
/** @var IL10N */
52-
private $l;
53-
/** @var LoggerInterface */
54-
private $logger;
55-
/** @var IURLGenerator */
56-
private $urlGenerator;
57-
/** @var IProcessingFileAccessor */
58-
private $processingFileAccessor;
59-
/** @var IRootFolder */
60-
private $rootFolder;
6149

6250
public function __construct(
63-
IJobList $jobList,
64-
IL10N $l,
65-
LoggerInterface $logger,
66-
IURLGenerator $urlGenerator,
67-
IProcessingFileAccessor $processingFileAccessor,
68-
IRootFolder $rootFolder) {
69-
$this->jobList = $jobList;
70-
$this->l = $l;
71-
$this->logger = $logger;
72-
$this->urlGenerator = $urlGenerator;
73-
$this->processingFileAccessor = $processingFileAccessor;
74-
$this->rootFolder = $rootFolder;
51+
private IJobList $jobList,
52+
private IL10N $l,
53+
private LoggerInterface $logger,
54+
private IURLGenerator $urlGenerator,
55+
private IProcessingFileAccessor $processingFileAccessor,
56+
private IRootFolder $rootFolder,
57+
) {
7558
}
7659

7760
/**
@@ -178,16 +161,18 @@ private function tryGetFileFromMapperEvent(string $eventName, MapperEvent $event
178161
return false;
179162
}
180163

181-
$files = $this->rootFolder->getById($fileId);
182-
if (empty($files) || !($files[0] instanceof \OCP\Files\File)) {
164+
// #324: The root folder object can potentially return multiple nodes with the same id when using 'getById'.
165+
// Threfore we use 'getFirstNodeById' here to ensure we only get one node which belongs to the current user.
166+
$file = $this->rootFolder->getFirstNodeById($fileId);
167+
if (!$file || !($file instanceof \OCP\Files\File)) {
183168
$this->logger->warning(
184169
'Not processing event {eventname} because node with id \'{fileid}\' could not be found or is not a file.',
185170
['eventname' => $eventName],
186171
['fileid' => $fileId]);
187172
return false;
188173
}
189174

190-
$node = $files[0];
175+
$node = $file;
191176
return true;
192177
}
193178

‎lib/Service/OcrService.php‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -217,14 +217,14 @@ private function shutdownUserEnvironment() : void {
217217
}
218218

219219
private function getNode(int $fileId) : Node {
220-
/** @var File[] */
221-
$nodeArr = $this->rootFolder->getById($fileId);
222-
if (count($nodeArr) === 0) {
220+
// #324: The root folder object can potentially return multiple nodes with the same id when using 'getById'.
221+
// Therefore we use 'getFirstNodeById' here to ensure we only get one node which belongs to the current user.
222+
$node = $this->rootFolder->getFirstNodeById($fileId);
223+
224+
if ($node === null) {
223225
throw new NotFoundException('Could not process file with id \'' . $fileId . '\'. File was not found');
224226
}
225227

226-
$node = array_shift($nodeArr);
227-
228228
if (!$node instanceof Node || $node->getType() !== FileInfo::TYPE_FILE) {
229229
throw new \InvalidArgumentException('Skipping process for file with id \'' . $fileId . '\'. It is not a file');
230230
}

‎phpunit.integration.xml‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,12 @@
55
colors="true"
66
timeoutForSmallTests="900"
77
timeoutForMediumTests="900"
8-
timeoutForLargeTests="900">
8+
timeoutForLargeTests="900"
9+
displayDetailsOnTestsThatTriggerDeprecations="true"
10+
displayDetailsOnTestsThatTriggerErrors="true"
11+
displayDetailsOnTestsThatTriggerNotices="true"
12+
displayDetailsOnTestsThatTriggerWarnings="true"
13+
displayDetailsOnPhpunitDeprecations="true">
914
<testsuite name="integration">
1015
<directory>./tests/Integration</directory>
1116
</testsuite>

‎tests/Integration/LocalBackendTest.php‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,9 @@ protected function setUp(): void {
7676

7777
protected function tearDown(): void {
7878
parent::tearDown();
79-
$this->dispatcher->removeListener(TextRecognizedEvent::class, $this->eventListener);
79+
if (isset($this->dispatcher) && isset($this->eventListener)) {
80+
$this->dispatcher->removeListener(TextRecognizedEvent::class, $this->eventListener);
81+
}
8082
$this->capturedEvents = [];
8183
}
8284

‎tests/Unit/OperationTest.php‎

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -323,9 +323,9 @@ public function testDoesNothingOnMapperEventIfFileNotFound() {
323323
/** @var MockObject|IRootFolder */
324324
$this->rootFolder = $this->createMock(IRootFolder::class);
325325
$this->rootFolder->expects($this->once())
326-
->method('getById')
326+
->method('getFirstNodeById')
327327
->with(42)
328-
->willReturn([]);
328+
->willReturn(null);
329329

330330
$operation = new Operation($this->jobList, $this->l, $this->logger, $this->urlGenerator, $this->processingFileAccessor, $this->rootFolder);
331331

@@ -344,9 +344,9 @@ public function testDoesNothingOnMapperEventIfObjectIdIsFolder() {
344344
/** @var MockObject|IRootFolder */
345345
$this->rootFolder = $this->createMock(IRootFolder::class);
346346
$this->rootFolder->expects($this->once())
347-
->method('getById')
347+
->method('getFirstNodeById')
348348
->with(42)
349-
->willReturn([$this->createMock(Folder::class)]);
349+
->willReturn($this->createMock(Folder::class));
350350

351351
$operation = new Operation($this->jobList, $this->l, $this->logger, $this->urlGenerator, $this->processingFileAccessor, $this->rootFolder);
352352

@@ -375,10 +375,11 @@ public function testFileAddedToQueueOnTagAssignedEvent() {
375375
->willReturn($fileId);
376376
/** @var MockObject|IRootFolder */
377377
$rootFolder = $this->createMock(IRootFolder::class);
378+
$rootFolder->expects($this->never())->method('getById');
378379
$rootFolder->expects($this->once())
379-
->method('getById')
380+
->method('getFirstNodeById')
380381
->with($fileId)
381-
->willReturn([$fileMock]);
382+
->willReturn($fileMock);
382383

383384
$operation = new Operation($this->jobList, $this->l, $this->logger, $this->urlGenerator, $this->processingFileAccessor, $rootFolder);
384385

‎tests/Unit/Service/OcrServiceTest.php‎

Lines changed: 42 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,7 @@ class OcrServiceTest extends TestCase {
9494
/** @var INotificationService|MockObject */
9595
private $notificationService;
9696
/** @var File[] */
97-
private $rootFolderGetById42ReturnValue;
97+
private $rootFolderGetFirstNodeById42ReturnValue;
9898
/** @var OcrService */
9999
private $ocrService;
100100
/** @var File|MockObject */
@@ -124,12 +124,12 @@ public function setUp() : void {
124124

125125
/** @var MockObject|IRootFolder */
126126
$this->rootFolder = $this->createMock(IRootFolder::class);
127-
$this->rootFolderGetById42ReturnValue = [$this->createValidFileMock()];
127+
$this->rootFolderGetFirstNodeById42ReturnValue = $this->createValidFileMock();
128128
$this->rootFolder->expects($this->any())
129-
->method('getById')
129+
->method('getFirstNodeById')
130130
->with(42)
131131
->willReturnCallback(function () {
132-
return $this->rootFolderGetById42ReturnValue;
132+
return $this->rootFolderGetFirstNodeById42ReturnValue;
133133
});
134134

135135
/** @var MockObject|IUserManager */
@@ -348,7 +348,7 @@ public function testCallsInitFilesystem() {
348348
public function testCallsGetOnRootFolder() {
349349
$settings = new WorkflowSettings();
350350
$this->rootFolder->expects($this->once())
351-
->method('getById')
351+
->method('getFirstNodeById')
352352
->with(42);
353353

354354
$this->ocrService->runOcrProcess(42, 'usr', $settings);
@@ -361,7 +361,7 @@ public function testCallsOcrProcessorWithFile() {
361361
$mimeType = 'application/pdf';
362362
$content = 'someFileContent';
363363
$fileMock = $this->createValidFileMock($mimeType, $content);
364-
$this->rootFolderGetById42ReturnValue = [$fileMock];
364+
$this->rootFolderGetFirstNodeById42ReturnValue = $fileMock;
365365

366366
$this->globalSettingsService->expects($this->once())
367367
->method('getGlobalSettings')
@@ -385,7 +385,7 @@ public function testCreatesNewFileVersionAndEmitsTextRecognizedEvent(string $ori
385385
$ocrResult = new OcrProcessorResult($ocrContent, 'pdf', $ocrContent); // Extend this cases if we add new OCR processors
386386
$originalFileMock = $this->createValidFileMock($mimeType, $content, $rootFolderPath, $originalFilename);
387387

388-
$this->rootFolderGetById42ReturnValue = [$originalFileMock];
388+
$this->rootFolderGetFirstNodeById42ReturnValue = $originalFileMock;
389389

390390
$this->ocrProcessor->expects($this->once())
391391
->method('ocrFile')
@@ -410,17 +410,18 @@ public function testCreatesNewFileVersionAndEmitsTextRecognizedEvent(string $ori
410410

411411
public function testThrowsNotFoundExceptionWhenFileNotFound() {
412412
$settings = new WorkflowSettings();
413-
$this->rootFolderGetById42ReturnValue = [];
413+
$this->rootFolderGetFirstNodeById42ReturnValue = null;
414414

415415
$this->expectException(NotFoundException::class);
416416
$this->ocrService->runOcrProcess(42, 'usr', $settings);
417417
}
418418

419-
#[DataProvider('dataProvider_InvalidNodes')]
420-
public function testDoesNotCallOcr_OnNonFile(callable $invalidNodeCallback) {
421-
$invalidNode = $invalidNodeCallback($this);
419+
public function testDoesNotCallOcr_OnNonFile() {
420+
$invalidNode = $this->createMock(Node::class);
421+
$invalidNode->method('getType')
422+
->willReturn(FileInfo::TYPE_FOLDER);
422423
$settings = new WorkflowSettings();
423-
$this->rootFolderGetById42ReturnValue = [$invalidNode];
424+
$this->rootFolderGetFirstNodeById42ReturnValue = $invalidNode;
424425

425426
$this->ocrProcessor->expects($this->never())
426427
->method('ocrFile');
@@ -473,7 +474,7 @@ public function testCallsProcessingFileAccessor() {
473474
$filePath = '/admin/files/somefile.pdf';
474475
$ocrResult = new OcrProcessorResult($ocrContent, 'pdf', $ocrContent); // Extend this cases if we add new OCR processors
475476

476-
$this->rootFolderGetById42ReturnValue = [$this->createValidFileMock($mimeType, $content)];
477+
$this->rootFolderGetFirstNodeById42ReturnValue = $this->createValidFileMock($mimeType, $content);
477478

478479
$this->ocrProcessor->expects($this->once())
479480
->method('ocrFile')
@@ -517,10 +518,12 @@ public function testDoesNotCreateNewFileVersionIfOcrContentWasEmpty() {
517518
$ocrResult = new OcrProcessorResult($ocrContent, 'pdf', $ocrContent);
518519
$fileId = 42;
519520

521+
$this->rootFolder->expects($this->never())->method('getById');
522+
520523
$this->rootFolder->expects($this->once())
521-
->method('getById')
524+
->method('getFirstNodeById')
522525
->with($fileId)
523-
->willReturn([$this->createValidFileMock($mimeType, $content)]);
526+
->willReturn($this->createValidFileMock($mimeType, $content));
524527

525528
$this->ocrProcessor->expects($this->once())
526529
->method('ocrFile')
@@ -600,7 +603,7 @@ public function testRestoreOriginalFileModificationDate() {
600603
$ocrResult = new OcrProcessorResult($ocrContent, 'pdf', $ocrContent); // Extend this cases if we add new OCR processors
601604

602605
$fileMock = $this->createValidFileMock($mimeType, $content);
603-
$this->rootFolderGetById42ReturnValue = [$fileMock];
606+
$this->rootFolderGetFirstNodeById42ReturnValue = $fileMock;
604607

605608
$this->ocrProcessor->expects($this->once())
606609
->method('ocrFile')
@@ -648,7 +651,8 @@ public function testRunOcrProcessWithJobArgumentLogsErrorAndDoesNothingOnInvalid
648651
}
649652

650653
public function testRunOcrProcessWithJobArgumentLogsErrorAndSendsNotificationOnNotFound() {
651-
$this->rootFolder->method('getById')
654+
// This will never be thrown in real live, it's just to test the error handling
655+
$this->rootFolder->method('getFirstNodeById')
652656
->willThrowException(new NotFoundException('File was not found'));
653657
$this->logger->expects($this->once())
654658
->method('error')
@@ -681,7 +685,7 @@ public function testCreatesNewFileVersionWithSuffixIfNodeIsNotUpdateable() {
681685
$ocrResult = new OcrProcessorResult($ocrContent, 'pdf', $ocrContent); // Extend this cases if we add new OCR processors
682686

683687
$fileMock = $this->createValidFileMock($mimeType, $content, '/admin/files', 'somefile.pdf', false);
684-
$this->rootFolderGetById42ReturnValue = [$fileMock];
688+
$this->rootFolderGetFirstNodeById42ReturnValue = $fileMock;
685689

686690
$this->ocrProcessor->expects($this->once())
687691
->method('ocrFile')
@@ -707,7 +711,7 @@ public function testSetsFileVersionsLabelIfKeepOriginalFileVersionIsTrue() {
707711
$ocrResult = new OcrProcessorResult($ocrContent, 'pdf', $ocrContent);
708712

709713
$fileMock = $this->createValidFileMock($mimeType, $content);
710-
$this->rootFolderGetById42ReturnValue = [$fileMock];
714+
$this->rootFolderGetFirstNodeById42ReturnValue = $fileMock;
711715

712716
$this->ocrProcessor->expects($this->once())
713717
->method('ocrFile')
@@ -750,6 +754,24 @@ public function testSetsFileVersionsLabelIfKeepOriginalFileVersionIsTrue() {
750754
$this->ocrService->runOcrProcess(42, 'usr', $settings);
751755
}
752756

757+
public function testRunOcrProcessThrowsNonOcrAlreadyDoneExceptionIfModeIsNotSkip() {
758+
$settings = new WorkflowSettings('{"ocrMode": ' . WorkflowSettings::OCR_MODE_FORCE_OCR . '}');
759+
$mimeType = 'application/pdf';
760+
$content = 'someFileContent';
761+
762+
$fileMock = $this->createValidFileMock($mimeType, $content);
763+
$this->rootFolderGetFirstNodeById42ReturnValue = $fileMock;
764+
765+
$ex = new OcrAlreadyDoneException('oops');
766+
$this->ocrProcessor->expects($this->once())
767+
->method('ocrFile')
768+
->willThrowException($ex);
769+
770+
$this->expectException(OcrAlreadyDoneException::class);
771+
772+
$this->ocrService->runOcrProcess(42, 'usr', $settings);
773+
}
774+
753775
// Related to #307
754776
public function testRunOcrProcessWorksWithoutIVersionManager() {
755777
$settings = new WorkflowSettings('{"keepOriginalFileVersion": true}');
@@ -759,7 +781,7 @@ public function testRunOcrProcessWorksWithoutIVersionManager() {
759781
$ocrResult = new OcrProcessorResult($ocrContent, 'pdf', $ocrContent);
760782

761783
$fileMock = $this->createValidFileMock($mimeType, $content);
762-
$this->rootFolderGetById42ReturnValue = [$fileMock];
784+
$this->rootFolderGetFirstNodeById42ReturnValue = $fileMock;
763785

764786
$this->ocrProcessor->expects($this->once())
765787
->method('ocrFile')
@@ -804,42 +826,6 @@ public function testRunOcrProcessWorksWithoutIVersionManager() {
804826
$this->assertTrue($logged, 'Expected debug log message not found');
805827
}
806828

807-
public function testRunOcrProcessThrowsNonOcrAlreadyDoneExceptionIfModeIsNotSkip() {
808-
$settings = new WorkflowSettings('{"ocrMode": ' . WorkflowSettings::OCR_MODE_FORCE_OCR . '}');
809-
$mimeType = 'application/pdf';
810-
$content = 'someFileContent';
811-
812-
$fileMock = $this->createValidFileMock($mimeType, $content);
813-
$this->rootFolderGetById42ReturnValue = [$fileMock];
814-
815-
$ex = new OcrAlreadyDoneException('oops');
816-
$this->ocrProcessor->expects($this->once())
817-
->method('ocrFile')
818-
->willThrowException($ex);
819-
820-
$this->expectException(OcrAlreadyDoneException::class);
821-
822-
$this->ocrService->runOcrProcess(42, 'usr', $settings);
823-
}
824-
825-
public static function dataProvider_InvalidNodes() {
826-
$folderMockCallable = function (self $testClass) {
827-
/** @var MockObject|Node */
828-
$folderMock = $testClass->createMock(Node::class);
829-
$folderMock->method('getType')
830-
->willReturn(FileInfo::TYPE_FOLDER);
831-
return $folderMock;
832-
};
833-
$fileInfoMockCallable = fn (self $testClass) => $testClass->createMock(FileInfo::class);
834-
$arr = [
835-
[$folderMockCallable],
836-
[$fileInfoMockCallable],
837-
[fn () => null],
838-
[fn () => (object)['someField' => 'someValue']]
839-
];
840-
return $arr;
841-
}
842-
843829
public static function dataProvider_OcrModesThrowOnEmptyResult() {
844830
return [
845831
[WorkflowSettings::OCR_MODE_FORCE_OCR],

‎tests/psalm-baseline.xml‎

Lines changed: 1 addition & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -207,23 +207,14 @@
207207
<file src="lib/Operation.php">
208208
<MissingDependency>
209209
<code><![CDATA[$this->rootFolder]]></code>
210-
<code><![CDATA[IRootFolder]]></code>
211-
<code><![CDATA[IRootFolder]]></code>
210+
<code><![CDATA[private]]></code>
212211
</MissingDependency>
213212
<NullReference>
214213
<code><![CDATA[$argsArray]]></code>
215214
</NullReference>
216215
<PossiblyUnusedMethod>
217216
<code><![CDATA[__construct]]></code>
218217
</PossiblyUnusedMethod>
219-
<PossiblyUnusedParam>
220-
<code><![CDATA[$jobList]]></code>
221-
<code><![CDATA[$l]]></code>
222-
<code><![CDATA[$logger]]></code>
223-
<code><![CDATA[$processingFileAccessor]]></code>
224-
<code><![CDATA[$rootFolder]]></code>
225-
<code><![CDATA[$urlGenerator]]></code>
226-
</PossiblyUnusedParam>
227218
<UndefinedClass>
228219
<code><![CDATA[File]]></code>
229220
</UndefinedClass>

0 commit comments

Comments
 (0)