refactor: move tag handling from files handler to files_sharing

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>

apps/files_sharing/lib/Controller/ShareAPIController.php

@@ -94,6 +94,7 @@ class ShareAPIController extends OCSController {
 		private LoggerInterface $logger,
 		private IProviderFactory $factory,
 		private IMailer $mailer,
+		private ITagManager $tagManager,
 		private ?string $userId = null,
 	) {
 		parent::__construct($appName, $request);
@@ -472,7 +473,7 @@ class ShareAPIController extends OCSController {
 				$share = $this->formatShare($share);
 
 				if ($include_tags) {
-					$share = Helper::populateTags([$share], Server::get(ITagManager::class));
+					$share = $this->populateTags([$share]);
 				} else {
 					$share = [$share];
 				}
@@ -847,7 +848,7 @@ class ShareAPIController extends OCSController {
 		}
 
 		if ($includeTags) {
-			$formatted = Helper::populateTags($formatted, Server::get(ITagManager::class));
+			$formatted = $this->populateTags($formatted);
 		}
 
 		return $formatted;
@@ -1100,8 +1101,7 @@ class ShareAPIController extends OCSController {
 		$formatted = $this->fixMissingDisplayName($formatted);
 
 		if ($includeTags) {
-			$formatted =
+			$formatted = $this->populateTags($formatted);
-				Helper::populateTags($formatted, Server::get(ITagManager::class));
 		}
 
 		return $formatted;
@@ -2221,4 +2221,41 @@ class ShareAPIController extends OCSController {
 			throw new OCSException($this->l->t('Failed to generate a unique token'));
 		}
 	}
+
+	/**
+	 * Populate the result set with file tags
+	 *
+	 * @psalm-template T of array{tags?: list<string>, file_source: int, ...array<string, mixed>}
+	 * @param list<T> $fileList
+	 * @return list<T> file list populated with tags
+	 */
+	private function populateTags(array $fileList): array {
+		$tagger = $this->tagManager->load('files');
+		$tags = $tagger->getTagsForObjects(array_map(static fn (array $fileData) => $fileData['file_source'], $fileList));
+
+		if (!is_array($tags)) {
+			throw new \UnexpectedValueException('$tags must be an array');
+		}
+
+		// Set empty tag array
+		foreach ($fileList as &$fileData) {
+			$fileData['tags'] = [];
+		}
+		unset($fileData);
+
+		if (!empty($tags)) {
+			foreach ($tags as $fileId => $fileTags) {
+				foreach ($fileList as &$fileData) {
+					if ($fileId !== $fileData['file_source']) {
+						continue;
+					}
+
+					$fileData['tags'] = $fileTags;
+				}
+				unset($fileData);
+			}
+		}
+
+		return $fileList;
+	}
 }

apps/files_sharing/tests/ApiTest.php

@@ -26,6 +26,7 @@ use OCP\IGroupManager;
 use OCP\IL10N;
 use OCP\IPreview;
 use OCP\IRequest;
+use OCP\ITagManager;
 use OCP\IURLGenerator;
 use OCP\IUserManager;
 use OCP\Mail\IMailer;
@@ -111,6 +112,7 @@ class ApiTest extends TestCase {
 		$logger = $this->createMock(LoggerInterface::class);
 		$providerFactory = $this->createMock(IProviderFactory::class);
 		$mailer = $this->createMock(IMailer::class);
+		$tagManager = $this->createMock(ITagManager::class);
 		$dateTimeZone->method('getTimeZone')->willReturn(new \DateTimeZone(date_default_timezone_get()));
 
 		return new ShareAPIController(
@@ -131,6 +133,7 @@ class ApiTest extends TestCase {
 			$logger,
 			$providerFactory,
 			$mailer,
+			$tagManager,
 			$userId,
 		);
 	}

apps/files_sharing/tests/Controller/ShareAPIControllerTest.php

@@ -28,6 +28,8 @@ use OCP\IGroupManager;
 use OCP\IL10N;
 use OCP\IPreview;
 use OCP\IRequest;
+use OCP\ITagManager;
+use OCP\ITags;
 use OCP\IURLGenerator;
 use OCP\IUser;
 use OCP\IUserManager;
@@ -76,6 +78,7 @@ class ShareAPIControllerTest extends TestCase {
 	private LoggerInterface&MockObject $logger;
 	private IProviderFactory&MockObject $factory;
 	private IMailer&MockObject $mailer;
+	private ITagManager&MockObject $tagManager;
 
 	protected function setUp(): void {
 		$this->shareManager = $this->createMock(IManager::class);
@@ -111,6 +114,7 @@ class ShareAPIControllerTest extends TestCase {
 		$this->logger = $this->createMock(LoggerInterface::class);
 		$this->factory = $this->createMock(IProviderFactory::class);
 		$this->mailer = $this->createMock(IMailer::class);
+		$this->tagManager = $this->createMock(ITagManager::class);
 
 		$this->ocs = new ShareAPIController(
 			$this->appName,
@@ -130,6 +134,7 @@ class ShareAPIControllerTest extends TestCase {
 			$this->logger,
 			$this->factory,
 			$this->mailer,
+			$this->tagManager,
 			$this->currentUser,
 		);
 	}
@@ -157,6 +162,7 @@ class ShareAPIControllerTest extends TestCase {
 				$this->logger,
 				$this->factory,
 				$this->mailer,
+				$this->tagManager,
 				$this->currentUser,
 			])->onlyMethods(['formatShare'])
 			->getMock();
@@ -841,8 +847,8 @@ class ShareAPIControllerTest extends TestCase {
 				$this->logger,
 				$this->factory,
 				$this->mailer,
+				$this->tagManager,
 				$this->currentUser,
-
 			])
 			->onlyMethods(['canAccessShare'])
 			->getMock();
@@ -1474,6 +1480,7 @@ class ShareAPIControllerTest extends TestCase {
 				$this->logger,
 				$this->factory,
 				$this->mailer,
+				$this->tagManager,
 				$this->currentUser,
 			])
 			->onlyMethods(['formatShare'])
@@ -1816,8 +1823,9 @@ class ShareAPIControllerTest extends TestCase {
 				$this->logger,
 				$this->factory,
 				$this->mailer,
+				$this->tagManager,
 				$this->currentUser,
-			])->setMethods(['formatShare'])
+			])->onlyMethods(['formatShare'])
 			->getMock();
 
 		[$userFolder, $path] = $this->getNonSharedUserFile();
@@ -1913,8 +1921,9 @@ class ShareAPIControllerTest extends TestCase {
 				$this->logger,
 				$this->factory,
 				$this->mailer,
+				$this->tagManager,
 				$this->currentUser,
-			])->setMethods(['formatShare'])
+			])->onlyMethods(['formatShare'])
 			->getMock();
 
 		$this->request
@@ -2338,8 +2347,9 @@ class ShareAPIControllerTest extends TestCase {
 				$this->logger,
 				$this->factory,
 				$this->mailer,
+				$this->tagManager,
 				$this->currentUser,
-			])->setMethods(['formatShare'])
+			])->onlyMethods(['formatShare'])
 			->getMock();
 
 		[$userFolder, $path] = $this->getNonSharedUserFile();
@@ -2408,8 +2418,9 @@ class ShareAPIControllerTest extends TestCase {
 				$this->logger,
 				$this->factory,
 				$this->mailer,
+				$this->tagManager,
 				$this->currentUser,
-			])->setMethods(['formatShare'])
+			])->onlyMethods(['formatShare'])
 			->getMock();
 
 		[$userFolder, $path] = $this->getNonSharedUserFile();
@@ -2639,8 +2650,9 @@ class ShareAPIControllerTest extends TestCase {
 				$this->logger,
 				$this->factory,
 				$this->mailer,
+				$this->tagManager,
 				$this->currentUser,
-			])->setMethods(['formatShare'])
+			])->onlyMethods(['formatShare'])
 			->getMock();
 
 		$userFolder = $this->getMockBuilder(Folder::class)->getMock();
@@ -5138,4 +5150,30 @@ class ShareAPIControllerTest extends TestCase {
 		$node->method('getId')->willReturn(42);
 		return [$userFolder, $node];
 	}
+
+	public function testPopulateTags(): void {
+		$tagger = $this->createMock(ITags::class);
+		$this->tagManager->method('load')
+			->with('files')
+			->willReturn($tagger);
+		$data = [
+			['file_source' => 10],
+			['file_source' => 22, 'foo' => 'bar'],
+			['file_source' => 42, 'x' => 'y'],
+		];
+		$tags = [
+			10 => ['tag3'],
+			42 => ['tag1', 'tag2'],
+		];
+		$tagger->method('getTagsForObjects')
+			->with([10, 22, 42])
+			->willReturn($tags);
+
+		$result = self::invokePrivate($this->ocs, 'populateTags', [$data]);
+		$this->assertSame([
+			['file_source' => 10, 'tags' => ['tag3']],
+			['file_source' => 22, 'foo' => 'bar', 'tags' => []],
+			['file_source' => 42, 'x' => 'y', 'tags' => ['tag1', 'tag2']],
+		], $result);
+	}
 }

build/psalm-baseline.xml

@@ -1396,7 +1396,6 @@
   </file>
   <file src="apps/files/lib/Helper.php">
     <UndefinedInterfaceMethod>
-      <code><![CDATA[$file]]></code>
       <code><![CDATA[$i]]></code>
       <code><![CDATA[$i]]></code>
       <code><![CDATA[$i]]></code>