From c64f0e4a02adb803baf8d919bb6859af7b07399e Mon Sep 17 00:00:00 2001 From: Piotr M Date: Mon, 20 Mar 2017 16:41:39 +0100 Subject: [PATCH] Optimize resolveGroupShare --- lib/private/Share20/DefaultShareProvider.php | 129 +++++++++++++----- lib/public/Share/IShareProvider.php | 1 + .../lib/Share20/DefaultShareProviderTest.php | 44 ++++++ 3 files changed, 143 insertions(+), 31 deletions(-) diff --git a/lib/private/Share20/DefaultShareProvider.php b/lib/private/Share20/DefaultShareProvider.php index 69956f09340b..65ac5d7aa8b6 100644 --- 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 index a705276e44f8..7659e71a9fea 100644 --- 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 index 6a21d128b12c..6ad0e352345e 100644 --- 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],