Use uid from filepath (#116)

* Read user id from filepath, don't use fileowner * Use new \OCP\BackgroundJob\QueuedJob base class * Closing #110
R0Wi-DEV · May 11, 2022 · 65bed02 · 65bed02
1 parent 99c8218
commit 65bed02
Show file tree

Hide file tree

Showing 5 changed files with 51 additions and 56 deletions.
diff --git a/lib/BackgroundJobs/ProcessFileJob.php b/lib/BackgroundJobs/ProcessFileJob.php
@@ -37,6 +37,7 @@
 use OCA\WorkflowOcr\Service\IOcrService;
 use OCA\WorkflowOcr\Wrapper\IFilesystem;
 use OCA\WorkflowOcr\Wrapper\IViewFactory;
+use OCP\AppFramework\Utility\ITimeFactory;
 use OCP\Files\FileInfo;
 use OCP\Files\Node;
 use OCP\IUserManager;
@@ -48,7 +49,7 @@
  * Represents a QuedJob which processes
  * a OCR on a single file.
  */
-class ProcessFileJob extends \OC\BackgroundJob\QueuedJob {
+class ProcessFileJob extends \OCP\BackgroundJob\QueuedJob {
 
 	/** @var LoggerInterface */
 	protected $logger;
@@ -75,7 +76,9 @@ public function __construct(
 		IFilesystem $filesystem,
 		IUserManager $userManager,
 		IUserSession $userSession,
-		IProcessingFileAccessor $processingFileAccessor) {
+		IProcessingFileAccessor $processingFileAccessor,
+		ITimeFactory $timeFactory) {
+		parent::__construct($timeFactory);
 		$this->logger = $logger;
 		$this->rootFolder = $rootFolder;
 		$this->ocrService = $ocrService;
@@ -121,21 +124,23 @@ private function tryParseArguments($argument) : array {
 		}
 
 		$filePath = null;
+		$uid = null;
 		$filePathKey = 'filePath';
 		if (array_key_exists($filePathKey, $argument)) {
 			$filePath = $argument[$filePathKey];
+			// '', admin, 'files', 'path/to/file.pdf'
+			$splitted = explode('/', $filePath, 4);
+			if (count($splitted) < 4) {
+				$this->logger->warning('File path "' . $filePath . '" is not valid in ' . self::class . ' method \'tryParseArguments\'.');
+				return [
+					false
+				];
+			}
+			$uid = $splitted[1];
 		} else {
 			$this->logVariableKeyNotSet($filePathKey, 'tryParseArguments');
 		}
 
-		$uid = null;
-		$uidKey = 'uid';
-		if (array_key_exists($uidKey, $argument)) {
-			$uid = $argument[$uidKey];
-		} else {
-			$this->logVariableKeyNotSet($uidKey, 'tryParseArguments');
-		}
-
 		$settings = null;
 		$settingsKey = 'settings';
 		if (array_key_exists($settingsKey, $argument)) {

diff --git a/lib/Operation.php b/lib/Operation.php
@@ -105,30 +105,17 @@ public function isAvailableForScope(int $scope): bool {
 	public function onEvent(string $eventName, Event $event, IRuleMatcher $ruleMatcher): void {
 		$this->logger->debug('onEvent: ' . $eventName);
 
-		if (!($match = $this->getMatch($ruleMatcher))) {
+		// $node and $argsArray will be passed by reference
+		if (!($match = $this->getMatch($ruleMatcher)) ||
+			!$this->tryGetFile($eventName, $event, $node) ||
+			$this->eventTriggeredByOcrProcess($node) ||
+			!$this->tryGetJobArgs($node, $match, $argsArray)) {
 			return;
 		}
-
-		// $node will be passed by reference
-		if (!$this->tryGetFile($eventName, $event, $node)) {
-			return;
-		}
-
-		if (!$this->pathIsValid($node) ||
-			!$this->ownerExists($node) ||
-			$this->eventTriggeredByOcrProcess($node)) {
-			return;
-		}
-
-		$args = [
-			'filePath' => $node->getPath(),
-			'uid' => $node->getOwner()->getUID(),
-			'settings' => $match['operation']
-		];
 
-		$this->logger->debug('Adding file to jobqueue: ' . json_encode($args));
+		$this->logger->debug('Adding file to jobqueue: ' . json_encode($argsArray));
 
-		$this->jobList->add(ProcessFileJob::class, $args);
+		$this->jobList->add(ProcessFileJob::class, $argsArray);
 	}
 
 	public function getEntityId(): string {
@@ -205,7 +192,12 @@ private function getMatch(IRuleMatcher $ruleMatcher) : array {
 		return $match;
 	}
 
-	private function pathIsValid(Node $node) : bool {
+	/**
+	 * @param Node $node
+	 * @param array $match
+	 * @param array $argsArray
+	 */
+	private function tryGetJobArgs(Node $node, $match, & $argsArray) : bool {
 		// Check path has valid structure
 		$filePath = $node->getPath();
 		// '', admin, 'files', 'path/to/file.pdf'
@@ -216,17 +208,10 @@ private function pathIsValid(Node $node) : bool {
 			return false;
 		}
 
-		return true;
-	}
-
-	private function ownerExists(Node $node) : bool {
-		// Check owner of file exists
-		$owner = $node->getOwner();
-		if ($owner === null) {
-			$this->logger->debug('Not processing event because file with path \'{path}\' has no owner.',
-					['path' => $node->getPath()]);
-			return false;
-		}
+		$argsArray = [
+			'filePath' => $filePath,
+			'settings' => $match['operation']
+		];
 
 		return true;
 	}

diff --git a/tests/Unit/BackgroundJobs/ProcessFileJobTest.php b/tests/Unit/BackgroundJobs/ProcessFileJobTest.php
@@ -105,8 +105,10 @@ public function setUp() : void {
 			$this->filesystem,
 			$this->userManager,
 			$this->userSession,
-			$this->processingFileAccessor
+			$this->processingFileAccessor,
+			$this->createMock(ITimeFactory::class)
 		);
+		$this->processFileJob->setId(111);
 
 		/** @var IConfig */
 		$configMock = $this->createMock(IConfig::class);
@@ -335,9 +337,11 @@ public function testThrowsNoUserException_OnNonExistingUser() {
 			$this->filesystem,
 			$userManager,
 			$this->userSession,
-			$this->processingFileAccessor
+			$this->processingFileAccessor,
+			$this->createMock(ITimeFactory::class)
 		);
-		$arguments = ['filePath' => '/admin/files/someInvalidStuff', 'uid' => 'nonexistinguser', 'settings' => '{}'];
+		$processFileJob->setId(111);
+		$arguments = ['filePath' => '/nonexistinguser/files/someInvalidStuff', 'settings' => '{}'];
 		$processFileJob->setArgument($arguments);
 
 		$processFileJob->execute($this->jobList);
@@ -393,18 +397,17 @@ public function testCallsProcessingFileAccessor(array $arguments, string $user,
 	public function dataProvider_InvalidArguments() {
 		$arr = [
 			[null, 1],
-			[['mykey' => 'myvalue'], 3],
-			[['someotherkey' => 'someothervalue', 'k2' => 'v2'], 3],
-			[['uid' => 'someuser'], 2],
-			[['filePath' => 'somepath'], 2]
+			[['mykey' => 'myvalue'], 2],
+			[['someotherkey' => 'someothervalue', 'k2' => 'v2'], 2],
+			[['filePath' => 'someInvalidPath'], 1]
 		];
 		return $arr;
 	}
 
 	public function dataProvider_ValidArguments() {
 		$arr = [
-			[['filePath' => '/admin/files/somefile.pdf', 'uid' => 'admin', 'settings' => '{}'], 'admin', '/admin/files'],
-			[['filePath' => '/myuser/files/subfolder/someotherfile.docx', 'uid' => 'myuser', 'settings' => '{}'], 'myuser', '/myuser/files']
+			[['filePath' => '/admin/files/somefile.pdf', 'settings' => '{}'], 'admin', '/admin/files'],
+			[['filePath' => '/myuser/files/subfolder/someotherfile.docx', 'settings' => '{}'], 'myuser', '/myuser/files']
 		];
 		return $arr;
 	}

diff --git a/tests/Unit/OperationTest.php b/tests/Unit/OperationTest.php
@@ -176,6 +176,8 @@ public function testDoesNothingOnInvalidFilePath(string $filePath) {
 			->willReturn(FileInfo::TYPE_FILE);
 		$fileMock->method('getPath')
 			->willReturn($filePath);
+		$fileMock->method('getId')
+			->willReturn(42);
 		$event = new GenericEvent($fileMock);
 		$eventName = '\OCP\Files::postCreate';
 
@@ -212,13 +214,13 @@ public function testAddWithCorrectFilePathAndUser() {
 		$uid = 'admin';
 		$this->jobList->expects($this->once())
 			->method('add')
-			->with(ProcessFileJob::class, ['filePath' => $filePath, 'uid' => $uid, 'settings' => self::SETTINGS]);
+			->with(ProcessFileJob::class, ['filePath' => $filePath, 'settings' => self::SETTINGS]);
 
 		$operation = new Operation($this->jobList, $this->l, $this->logger, $this->urlGenerator, $this->processingFileAccessor, $this->rootFolder);
 
 		/** @var MockObject|IUser */
 		$userMock = $this->createMock(IUser::class);
-		$userMock->expects($this->once())
+		$userMock->expects($this->never())
 			->method('getUID')
 			->willReturn($uid);
 		/** @var MockObject|Node */
@@ -402,11 +404,11 @@ public function testFileAddedToQueueOnTagAssignedEvent() {
 
 		$this->jobList->expects($this->once())
 			->method('add')
-			->with(ProcessFileJob::class, ['filePath' => $filePath, 'uid' => $uid, 'settings' => self::SETTINGS]);
+			->with(ProcessFileJob::class, ['filePath' => $filePath, 'settings' => self::SETTINGS]);
 
 		/** @var MockObject|IUser */
 		$userMock = $this->createMock(IUser::class);
-		$userMock->expects($this->once())
+		$userMock->expects($this->never())
 			->method('getUID')
 			->willReturn($uid);
 		/** @var MockObject|\OCP\Files\File */

diff --git a/tests/psalm-baseline.xml b/tests/psalm-baseline.xml
@@ -8,7 +8,7 @@
   </file>
   <file src="lib/BackgroundJobs/ProcessFileJob.php">
     <UndefinedClass occurrences="1">
-      <code>\OC\BackgroundJob\QueuedJob</code>
+      <code>\OCP\BackgroundJob\QueuedJob</code>
     </UndefinedClass>
   </file>
   <file src="lib/Controller/GlobalSettingsController.php">