Correctly format OCS response with favorites

The helper funtion did not handle the response correctly and basically
only returned the last share with tags.

This is a simple rewrite. That is still understandable. Loops maybe more
than strictly required. But preformance is not the issue here.

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

apps/files/lib/Helper.php

@@ -32,6 +32,7 @@
 namespace OCA\Files;
 
 use OCP\Files\FileInfo;
+use OCP\ITagManager;
 
 /**
  * Helper class for manipulating file information
@@ -206,40 +207,39 @@ class Helper {
 	 *
 	 * @param array $fileList
 	 * @param string $fileIdentifier identifier attribute name for values in $fileList
+	 * @param ITagManager $tagManager
 	 * @return array file list populated with tags
 	 */
-	public static function populateTags(array $fileList, $fileIdentifier = 'fileid') {
-		$filesById = [];
+	public static function populateTags(array $fileList, $fileIdentifier = 'fileid', ITagManager $tagManager) {
+		$ids = [];
 		foreach ($fileList as $fileData) {
-			$filesById[$fileData[$fileIdentifier]] = $fileData;
+			$ids[] = $fileData[$fileIdentifier];
 		}
-		$tagger = \OC::$server->getTagManager()->load('files');
-		$tags = $tagger->getTagsForObjects(array_keys($filesById));
+		$tagger = $tagManager->load('files');
+		$tags = $tagger->getTagsForObjects($ids);
 
 		if (!is_array($tags)) {
 			throw new \UnexpectedValueException('$tags must be an array');
 		}
 
-		if (!empty($tags)) {
-			foreach ($tags as $fileId => $fileTags) {
-				$filesById[$fileId]['tags'] = $fileTags;
-			}
-
-			foreach ($filesById as $key => $fileWithTags) {
-				foreach($fileList as $key2 => $file){
-					if( $file[$fileIdentifier] === $key){
-						$fileList[$key2] = $fileWithTags;
-					}
-				}
-			}
-
-			foreach ($fileList as $key => $file) {
-				if (!array_key_exists('tags', $file)) {
+		// Set empty tag array
+		foreach ($fileList as $key => $fileData) {
 			$fileList[$key]['tags'] = [];
 		}
+
+		if (!empty($tags)) {
+			foreach ($tags as $fileId => $fileTags) {
+
+				foreach ($fileList as $key => $fileData) {
+					if ($fileId !== $fileData[$fileIdentifier]) {
+						continue;
 					}
 
+					$fileList[$key]['tags'] = $fileTags;
 				}
+			}
+		}
+
 		return $fileList;
 	}
 

apps/files/tests/HelperTest.php

@@ -112,4 +112,35 @@ class HelperTest extends \Test\TestCase {
 		);
 	}
 
+	public function testPopulateTags() {
+		$tagManager = $this->createMock(\OCP\ITagManager::class);
+		$tagger = $this->createMock(\OCP\ITags::class);
+
+		$tagManager->method('load')
+			->with('files')
+			->willReturn($tagger);
+
+		$data = [
+			['id' => 10],
+			['id' => 22, 'foo' => 'bar'],
+			['id' => 42, 'x' => 'y'],
+		];
+
+		$tags = [
+			10 => ['tag3'],
+			42 => ['tag1', 'tag2'],
+		];
+
+		$tagger->method('getTagsForObjects')
+			->with([10, 22, 42])
+			->willReturn($tags);
+
+		$result = \OCA\Files\Helper::populateTags($data, 'id', $tagManager);
+
+		$this->assertSame([
+			['id' => 10, 'tags' => ['tag3']],
+			['id' => 22, 'foo' => 'bar', 'tags' => []],
+			['id' => 42, 'x' => 'y', 'tags' => ['tag1', 'tag2']],
+		], $result);
+	}
 }

apps/files_sharing/lib/Controller/ShareAPIController.php

@@ -514,7 +514,7 @@ class ShareAPIController extends OCSController {
 		}
 
 		if ($includeTags) {
-			$formatted = Helper::populateTags($formatted, 'file_source');
+			$formatted = Helper::populateTags($formatted, 'file_source', \OC::$server->getTagManager());
 		}
 
 		return new DataResponse($formatted);
@@ -644,7 +644,7 @@ class ShareAPIController extends OCSController {
 		}
 
 		if ($include_tags) {
-			$formatted = Helper::populateTags($formatted, 'file_source');
+			$formatted = Helper::populateTags($formatted, 'file_source', \OC::$server->getTagManager());
 		}
 
 		return new DataResponse($formatted);