Optimize resolveGroupShare

owncloud · Mar 22, 2017 · c64f0e4 · c64f0e4
1 parent 7fbc935
commit c64f0e4
Show file tree

Hide file tree

Showing 3 changed files with 143 additions and 31 deletions.
diff --git a/lib/private/Share20/DefaultShareProvider.php b/lib/private/Share20/DefaultShareProvider.php
@@ -601,7 +601,14 @@ public function getShareById($id, $recipientId = null) {
 
 		// If the recipient is set for a group share resolve to that user
 		if ($recipientId !== null && $share->getShareType() === \OCP\Share::SHARE_TYPE_GROUP) {
-			$share = $this->resolveGroupShare($share, $recipientId);
+			$resolvedShares = $this->resolveGroupShares([$share], $recipientId);
+			if (count($resolvedShares) === 1){
+				// If we pass to resolveGroupShares() an with one element,
+				// we expect to receive exactly one element, otherwise it is error
+				$share = $resolvedShares[0];
+			} else {
+				throw new ProviderException("ResolveGroupShares() returned wrong result");
+			}
 		}
 
 		return $share;
@@ -765,18 +772,15 @@ public function getSharedWith($userId, $shareType, $node, $limit, $offset) {
 				$cursor->closeCursor();
 			}
 
-			/*
- 			 * Resolve all group shares to user specific shares
- 			 * TODO: Optmize this!
- 			 */
-			foreach($shares2 as $share) {
-				$shares[] = $this->resolveGroupShare($share, $userId);
+			//Resolve all group shares to user specific shares
+			if (!empty($shares2)) {
+				$resolvedGroupShares = $this->resolveGroupShares($shares2, $userId);
+				$shares = array_merge($shares, $resolvedGroupShares);
 			}
 		} else {
 			throw new BackendError('Invalid backend');
 		}
 
-
 		return $shares;
 	}
 
@@ -860,37 +864,100 @@ private function createShare($data) {
 	}
 
 	/**
-	 * Resolve a group share to a user specific share
-	 * Thus if the user moved their group share make sure this is properly reflected here.
+	 * Will return two maps:
+	 * - $chunkedShareIds responsible to split shareIds into chunks containing 100 elements
+	 * 	 e.g. $chunkedShareIds { { "4", "52", "54",... }[100], { .. }[2] }[2]
 	 *
-	 * @param \OCP\Share\IShare $share
+	 * - $shareIdToShareMap responsible to split shareIds into chunks containing 100 elements
+	 * 	 e.g. $shareIdToShareMap { "4" => IShare, "52" => IShare, "54" => IShare, ... }[102]
+	 *
+	 * @param \OCP\Share\IShare[] $shares
+	 * @return array $chunkedSharesToMaps e.g { $chunkedShareIds, $shareIdToShareMap }[2]
+	 */
+	private function chunkSharesToMaps($shares) {
+		$chunkedShareIds = [];
+		$shareIdToShareMap = [];
+		$chunkId = 0;
+		$shareNo = 0;
+		foreach($shares as $share) {
+			// Map unique shareIds to IShare
+			$shareId = $share->getId();
+			$shareIdToShareMap[$shareId] = $share;
+
+			// Chunk shareId array
+			if ($shareNo >= 100) {
+				// If we have over 100 shares in the array, start next chunk
+				$shareNo = 0;
+				$chunkId++;
+			} else {
+				// Increase number of shares in current array
+				$shareNo++;
+			}
+			$chunkedShareIds[$chunkId][] = $shareId;
+		}
+
+		$chunkedSharesToMaps = array($chunkedShareIds, $shareIdToShareMap);
+		return $chunkedSharesToMaps;
+	}
+
+	/**
+	 * Resolve a group shares to a user specific share.
+	 * Thus if the user moved their group share make sure this is properly reflected here,
+	 * If $shares array contains exactly 2 elements, where
+	 * only 1 will be changed(resolved), it returns exactly 2 elements, containing the resolved one.
+	 *
+	 * @param \OCP\Share\IShare[] $shares e.g. { IShare, IShare }[2]
 	 * @param string $userId
-	 * @return Share Returns the updated share if one was found else return the original share.
+	 * @return \OCP\Share\IShare[] $resolvedShares
+	 * @throws ProviderException
 	 */
-	private function resolveGroupShare(\OCP\Share\IShare $share, $userId) {
+	private function resolveGroupShares($shares, $userId) {
 		$qb = $this->dbConn->getQueryBuilder();
 
-		$stmt = $qb->select('*')
-			->from('share')
-			->where($qb->expr()->eq('parent', $qb->createNamedParameter($share->getId())))
-			->andWhere($qb->expr()->eq('share_type', $qb->createNamedParameter(self::SHARE_TYPE_USERGROUP)))
-			->andWhere($qb->expr()->eq('share_with', $qb->createNamedParameter($userId)))
-			->andWhere($qb->expr()->orX(
-				$qb->expr()->eq('item_type', $qb->createNamedParameter('file')),
-				$qb->expr()->eq('item_type', $qb->createNamedParameter('folder'))
-			))
-			->setMaxResults(1)
-			->execute();
+		list($chunkedShareIds, $shareIdToShareMap) = $this->chunkSharesToMaps($shares);
+		foreach($chunkedShareIds as $shareIdsChunk) {
+			$qb->select('*')
+				->from('share')
+				->where($qb->expr()->in('parent', $qb->createNamedParameter(
+					$shareIdsChunk,
+					IQueryBuilder::PARAM_STR_ARRAY
+				)))
+				->andWhere($qb->expr()->eq('share_type', $qb->createNamedParameter(self::SHARE_TYPE_USERGROUP)))
+				->andWhere($qb->expr()->eq('share_with', $qb->createNamedParameter($userId)))
+				->andWhere($qb->expr()->orX(
+					$qb->expr()->eq('item_type', $qb->createNamedParameter('file')),
+					$qb->expr()->eq('item_type', $qb->createNamedParameter('folder'))
+				));
 
-		$data = $stmt->fetch();
-		$stmt->closeCursor();
+			$stmt = $qb->execute();
 
-		if ($data !== false) {
-			$share->setPermissions((int)$data['permissions']);
-			$share->setTarget($data['file_target']);
-		}
+			// Resolve $shareIdToShareMap array containing group shares
+			$shareParents = [];
+			while($data = $stmt->fetch()) {
+				// Get share parent
+				$shareParent = $data['parent'];
 
-		return $share;
+				// Ensure uniquenes of parents
+				if (!isset($shareParents[$shareParent])) {
+					$shareParents[] = $shareParent;
+				} else {
+					throw new ProviderException('Parent of share should be unique');
+				}
+
+				// Resolve only shares contained in the map.
+				// This will ensure that we return the same amount of shares in the input as in the output
+				// If $shareParent is contained in $shareIdToShareMap, it means that needs resolving
+				if (isset($shareIdToShareMap[$shareParent])) {
+					$share = $shareIdToShareMap[$shareParent];
+					$share->setPermissions(intval($data['permissions']));
+					$share->setTarget($data['file_target']);
+				}
+			}
+			$stmt->closeCursor();
+		}
+
+		$resolvedShares = array_values($shareIdToShareMap);
+		return $resolvedShares;
 	}
 
 	/**

diff --git a/lib/public/Share/IShareProvider.php b/lib/public/Share/IShareProvider.php
@@ -123,6 +123,7 @@ public function getAllSharesBy($userId, $shareTypes, $nodeIDs, $reshares);
 	 * @param string|null $recipientId
 	 * @return \OCP\Share\IShare
 	 * @throws ShareNotFound
+	 * @throws ProviderException
 	 * @since 9.0.0
 	 */
 	public function getShareById($id, $recipientId = null);

diff --git a/tests/lib/Share20/DefaultShareProviderTest.php b/tests/lib/Share20/DefaultShareProviderTest.php
@@ -1125,6 +1125,50 @@ public function testGetSharedWithGroupWithNode($storageStringId, $fileName1, $fi
 		$this->assertEquals(Share::SHARE_TYPE_GROUP, $share->getShareType());
 	}
 
+	public function testChunkedGetSharedWithGroupWithNode() {
+		$storageStringId = 'home::shareOwner';
+		$fileName1 = 'files/test.txt';
+		$storageId = $this->createTestStorageEntry($storageStringId);
+		$fileId = $this->createTestFileEntry($fileName1, $storageId);
+
+		$user0 = $this->createMock(IUser::class);
+		$user0->method('getUID')->willReturn('user0');
+		$user1 = $this->createMock(IUser::class);
+		$user1->method('getUID')->willReturn('user1');
+
+		$this->userManager->method('get')->willReturnMap([
+			['user0', $user0],
+			['user1', $user1],
+		]);
+
+		for($i = 0; $i < 105; $i++) {
+			$group = $this->createMock(IGroup::class);
+			$groupId = "group$i";
+			$group->method('getGID')->willReturn($groupId);
+			$this->groupManager->method('get')->with($groupId)->willReturn($group);
+			$groupArray[] = $group;
+			$ids[] = $this->addShareToDB(Share::SHARE_TYPE_GROUP, $groupId, 'user1', 'user1',
+				'file', $fileId, 'myTarget', 31, null, null, null);
+		}
+		$this->groupManager->method('getUserGroups')->with($user0)->willReturn($groupArray);
+
+		$node = $this->createMock(File::class);
+		$node->method('getId')->willReturn($fileId);
+		$this->rootFolder->method('getUserFolder')->with('user1')->will($this->returnSelf());
+		$this->rootFolder->method('getById')->with($fileId)->willReturn([$node]);
+
+		$share = $this->provider->getSharedWith('user0', Share::SHARE_TYPE_GROUP, $node, -1, 0);
+		$this->assertCount(105, $share);
+
+		$share = $share[0];
+		$this->assertEquals($ids[0], $share->getId());
+		$this->assertSame('group0', $share->getSharedWith());
+		$this->assertSame('user1', $share->getShareOwner());
+		$this->assertSame('user1', $share->getSharedBy());
+		$this->assertSame($node, $share->getNode());
+		$this->assertEquals(Share::SHARE_TYPE_GROUP, $share->getShareType());
+	}
+
 	public function shareTypesProvider() {
 		return [
 			[Share::SHARE_TYPE_USER, false],