Merge pull request #27284 from owncloud/optimize_propfind

Optimize PROPFIND for non-shared/shared by reducing the number of que…
owncloud · Mar 10, 2017 · a651f4f · a651f4f
2 parents 2840b6b + 28ff6fe
commit a651f4f
Show file tree

Hide file tree

Showing 10 changed files with 604 additions and 42 deletions.
diff --git a/apps/dav/lib/Connector/Sabre/SharesPlugin.php b/apps/dav/lib/Connector/Sabre/SharesPlugin.php
@@ -104,34 +104,49 @@ public function initialize(\Sabre\DAV\Server $server) {
 	}
 
 	/**
-	 * Return a list of share types for outgoing shares
+	 * Converts IShare[] to int[][] hash map
 	 *
-	 * @param \OCP\Files\Node $node file node
+	 * @param  IShare[] $allShares - array containing shares
+	 * @param  int[][] $initShareTypes - array containing hash map nodeIds->shareTypes $initShareTypes[$currentNodeID][$currentShareType]
 	 *
-	 * @return int[] array of share types
+	 * @return int[][] hashmap of share types
 	 */
-	private function getShareTypes(\OCP\Files\Node $node) {
-		$shareTypes = [];
+	private function convertToHashMap($allShares, $initShareTypes) {
+		// Use some already preinitialized hash map which may contain some values e.g. empty arrays
+		$shareTypes = $initShareTypes;
+
+		foreach ($allShares as $share) {
+			$currentNodeID = $share->getNodeId();
+			$currentShareType = $share->getShareType();
+			$shareTypes[$currentNodeID][$currentShareType] = true;
+		}
+
+		return $shareTypes;
+	}
+
+	/**
+	 * Get all shares for specific nodeIDs
+	 *
+	 * @param int[] $nodeIDs - array of folder/file nodeIDs
+	 *
+	 * @return IShare[] array containing shares
+	 */
+	private function getSharesForNodeIds($nodeIDs) {
 		$requestedShareTypes = [
 			\OCP\Share::SHARE_TYPE_USER,
 			\OCP\Share::SHARE_TYPE_GROUP,
 			\OCP\Share::SHARE_TYPE_LINK,
 			\OCP\Share::SHARE_TYPE_REMOTE
 		];
-		foreach ($requestedShareTypes as $requestedShareType) {
-			// one of each type is enough to find out about the types
-			$shares = $this->shareManager->getSharesBy(
-				$this->userId,
-				$requestedShareType,
-				$node,
-				false,
-				1
-			);
-			if (!empty($shares)) {
-				$shareTypes[] = $requestedShareType;
-			}
-		}
-		return $shareTypes;
+
+		// Query DB for share types for specified node IDs
+		$allShares = $this->shareManager->getAllSharesBy(
+			$this->userId,
+			$requestedShareTypes,
+			$nodeIDs
+		);
+
+		return $allShares;
 	}
 
 	/**
@@ -156,19 +171,50 @@ public function handleGetProperties(
 			$folderNode = $this->userFolder->get($sabreNode->getPath());
 			$children = $folderNode->getDirectoryListing();
 
-			$this->cachedShareTypes[$folderNode->getId()] = $this->getShareTypes($folderNode);
+			// Get ID of parent folder
+			$folderNodeID = intval($folderNode->getId());
+			$nodeIdsArray = [$folderNodeID];
+
+			// Initialize share types array for this node in case there would be no shares for this node
+			$initShareTypes[$folderNodeID] = [];
+
+			// Get IDs for all children of the parent folder
 			foreach ($children as $childNode) {
-				$this->cachedShareTypes[$childNode->getId()] = $this->getShareTypes($childNode);
+				// Ensure that they are of File or Folder type
+				if (!($childNode instanceof \OCP\Files\File) &&
+					!($childNode instanceof \OCP\Files\Folder)) {
+					return;
+				}
+
+				// Put node ID into an array and initialize cache for it
+				$nodeId = intval($childNode->getId());
+				array_push($nodeIdsArray, $nodeId);
+
+				// Initialize share types array for this node in case there would be no shares for this node
+				$initShareTypes[$nodeId] = [];
 			}
+
+			// Get all shares for specified nodes obtaining them from DB
+			$returnedShares = $this->getSharesForNodeIds($nodeIdsArray);
+
+			// Convert to hash map and cache so that $propFind->handle() can use it
+			$this->cachedShareTypes = $this->convertToHashMap($returnedShares, $initShareTypes);
 		}
 
 		$propFind->handle(self::SHARETYPES_PROPERTYNAME, function() use ($sabreNode) {
 			if (isset($this->cachedShareTypes[$sabreNode->getId()])) {
-				$shareTypes = $this->cachedShareTypes[$sabreNode->getId()];
+				// Share types in cache for this node
+				$shareTypesHash = $this->cachedShareTypes[$sabreNode->getId()];
 			} else {
-				$node = $this->userFolder->get($sabreNode->getPath());
-				$shareTypes = $this->getShareTypes($node);
+				// Share types for this node not in cache, obtain if any
+				$nodeId = $this->userFolder->get($sabreNode->getPath())->getId();
+				$returnedShares = $this->getSharesForNodeIds([$nodeId]);
+
+				// Initialize share types for this node and obtain share types hash if any
+				$initShareTypes[$nodeId] = [];
+				$shareTypesHash = $this->convertToHashMap($returnedShares, $initShareTypes)[$nodeId];
 			}
+			$shareTypes = array_keys($shareTypesHash);
 
 			return new ShareTypeList($shareTypes);
 		});

diff --git a/apps/dav/tests/unit/Connector/Sabre/SharesPluginTest.php b/apps/dav/tests/unit/Connector/Sabre/SharesPluginTest.php
@@ -22,6 +22,8 @@
  */
 namespace OCA\DAV\Tests\unit\Connector\Sabre;
 
+use OCP\Share\IShare;
+
 class SharesPluginTest extends \Test\TestCase {
 
 	const SHARETYPES_PROPERTYNAME = \OCA\DAV\Connector\Sabre\SharesPlugin::SHARETYPES_PROPERTYNAME;
@@ -94,26 +96,41 @@ public function testGetProperties($shareTypes) {
 
 		// node API nodes
 		$node = $this->createMock('\OCP\Files\Folder');
+		$node->expects($this->any())
+			->method('getId')
+			->will($this->returnValue(123));
 
 		$this->userFolder->expects($this->once())
 			->method('get')
 			->with('/subdir')
 			->will($this->returnValue($node));
 
+		$requestedShareTypes = [
+			\OCP\Share::SHARE_TYPE_USER,
+			\OCP\Share::SHARE_TYPE_GROUP,
+			\OCP\Share::SHARE_TYPE_LINK,
+			\OCP\Share::SHARE_TYPE_REMOTE
+		];
+
 		$this->shareManager->expects($this->any())
-			->method('getSharesBy')
+			->method('getAllSharesBy')
 			->with(
 				$this->equalTo('user1'),
-				$this->anything(),
-				$this->anything(),
-				$this->equalTo(false),
-				$this->equalTo(1)
+				$requestedShareTypes,
+				$this->anything()
 			)
-			->will($this->returnCallback(function($userId, $requestedShareType, $node, $flag, $limit) use ($shareTypes){
-				if (in_array($requestedShareType, $shareTypes)) {
-					return ['dummyshare'];
+			->will($this->returnCallback(function($userId, $requestedShareTypes, $node) use ($shareTypes){
+				$allShares = array();
+				foreach($requestedShareTypes as $requestedShareType){
+					$share = $this->createMock(IShare::class);
+					$share->method('getShareType')->willReturn($requestedShareType);
+					$share->method('getNodeId')->willReturn(123);
+					if (in_array($requestedShareType, $shareTypes)) {
+						array_push($allShares, $share);
+					}
 				}
-				return [];
+
+				return $allShares;
 			}));
 
 		$propFind = new \Sabre\DAV\PropFind(
@@ -190,21 +207,32 @@ public function testPreloadThenGetProperties($shareTypes) {
 			->with('/subdir')
 			->will($this->returnValue($node));
 
+		$requestedShareTypes = [
+			\OCP\Share::SHARE_TYPE_USER,
+			\OCP\Share::SHARE_TYPE_GROUP,
+			\OCP\Share::SHARE_TYPE_LINK,
+			\OCP\Share::SHARE_TYPE_REMOTE
+		];
+
 		$this->shareManager->expects($this->any())
-			->method('getSharesBy')
+			->method('getAllSharesBy')
 			->with(
 				$this->equalTo('user1'),
-				$this->anything(),
-				$this->anything(),
-				$this->equalTo(false),
-				$this->equalTo(1)
+				$requestedShareTypes,
+				$this->anything()
 			)
-			->will($this->returnCallback(function($userId, $requestedShareType, $node, $flag, $limit) use ($shareTypes){
-				if ($node->getId() === 111 && in_array($requestedShareType, $shareTypes)) {
-					return ['dummyshare'];
+			->will($this->returnCallback(function($userId, $requestedShareTypes, $node) use ($shareTypes){
+				$allShares = array();
+				foreach($requestedShareTypes as $requestedShareType){
+					$share = $this->createMock(IShare::class);
+					$share->method('getShareType')->willReturn($requestedShareType);
+					$share->method('getNodeId')->willReturn(111);
+					if (in_array($requestedShareType, $shareTypes)) {
+						array_push($allShares, $share);
+					}
 				}
 
-				return [];
+				return $allShares;
 			}));
 
 		// simulate sabre recursive PROPFIND traversal

diff --git a/apps/federatedfilesharing/lib/FederatedShareProvider.php b/apps/federatedfilesharing/lib/FederatedShareProvider.php
@@ -36,6 +36,7 @@
 use OCP\Share\Exceptions\ShareNotFound;
 use OCP\Files\NotFoundException;
 use OCP\IDBConnection;
+use OCP\DB\QueryBuilder\IQueryBuilder;
 use OCP\Files\Node;
 
 /**
@@ -561,6 +562,61 @@ public function deleteFromSelf(IShare $share, $recipient) {
 		return;
 	}
 
+	/**
+	 * @inheritdoc
+	 */
+	public function getAllSharesBy($userId, $shareTypes, $nodeIDs, $reshares) {
+		$shares = [];
+		$qb = $this->dbConnection->getQueryBuilder();
+
+		$nodeIdsChunks = array_chunk($nodeIDs, 100);
+		foreach ($nodeIdsChunks as $nodeIdsChunk) {
+			// In federates sharing currently we have only one share_type_remote
+			$qb->select('*')
+				->from('share');
+
+			$qb->andWhere($qb->expr()->eq('share_type', $qb->createNamedParameter(self::SHARE_TYPE_REMOTE)));
+
+			/**
+			 * Reshares for this user are shares where they are the owner.
+			 */
+			if ($reshares === false) {
+				//Special case for old shares created via the web UI
+				$or1 = $qb->expr()->andX(
+					$qb->expr()->eq('uid_owner', $qb->createNamedParameter($userId)),
+					$qb->expr()->isNull('uid_initiator')
+				);
+
+				$qb->andWhere(
+					$qb->expr()->orX(
+						$qb->expr()->eq('uid_initiator', $qb->createNamedParameter($userId)),
+						$or1
+					)
+				);
+			} else {
+				$qb->andWhere(
+					$qb->expr()->orX(
+						$qb->expr()->eq('uid_owner', $qb->createNamedParameter($userId)),
+						$qb->expr()->eq('uid_initiator', $qb->createNamedParameter($userId))
+					)
+				);
+			}
+
+			$qb->andWhere($qb->expr()->in('file_source', $qb->createParameter('file_source_ids')));
+			$qb->setParameter('file_source_ids', $nodeIdsChunk, IQueryBuilder::PARAM_INT_ARRAY);
+
+			$qb->orderBy('id');
+
+			$cursor = $qb->execute();
+			while($data = $cursor->fetch()) {
+				$shares[] = $this->createShareObject($data);
+			}
+			$cursor->closeCursor();
+		}
+
+		return $shares;
+	}
+
 	/**
 	 * @inheritdoc
 	 */

diff --git a/apps/federatedfilesharing/tests/FederatedShareProviderTest.php b/apps/federatedfilesharing/tests/FederatedShareProviderTest.php
@@ -462,6 +462,89 @@ public function datatTestUpdate() {
 		];
 	}
 
+
+	public function testGetAllSharesByNodes() {
+		$node = $this->createMock('\OCP\Files\File');
+		$node->method('getId')->willReturn(42);
+		$node->method('getName')->willReturn('myFile');
+
+		$this->tokenHandler->method('generateToken')->willReturn('token');
+		$this->notifications
+			->method('sendRemoteShare')
+			->willReturn(true);
+
+		$this->rootFolder->expects($this->never())->method($this->anything());
+
+		$share = $this->shareManager->newShare();
+		$share->setSharedWith('user@server.com')
+			->setSharedBy('sharedBy')
+			->setShareOwner('shareOwner')
+			->setPermissions(19)
+			->setNode($node);
+		$this->provider->create($share);
+
+		$node2 = $this->createMock('\OCP\Files\File');
+		$node2->method('getId')->willReturn(43);
+		$node2->method('getName')->willReturn('myOtherFile');
+
+		$share2 = $this->shareManager->newShare();
+		$share2->setSharedWith('user@server.com')
+			->setSharedBy('sharedBy')
+			->setShareOwner('shareOwner')
+			->setPermissions(19)
+			->setNode($node2);
+		$this->provider->create($share2);
+
+		$shares = $this->provider->getAllSharesBy('sharedBy', [\OCP\Share::SHARE_TYPE_REMOTE], [$node2->getId()], false);
+
+		$this->assertCount(1, $shares);
+		$this->assertEquals(43, $shares[0]->getNodeId());
+	}
+
+	public function testGetAllSharedByWithReshares() {
+		$node = $this->createMock('\OCP\Files\File');
+		$node->method('getId')->willReturn(42);
+		$node->method('getName')->willReturn('myFile');
+
+		$this->tokenHandler->method('generateToken')->willReturn('token');
+		$this->notifications
+			->method('sendRemoteShare')
+			->willReturn(true);
+
+		$this->rootFolder->expects($this->never())->method($this->anything());
+
+		$share = $this->shareManager->newShare();
+		$share->setSharedWith('user@server.com')
+			->setSharedBy('shareOwner')
+			->setShareOwner('shareOwner')
+			->setPermissions(19)
+			->setNode($node);
+		$this->provider->create($share);
+
+		$share2 = $this->shareManager->newShare();
+		$share2->setSharedWith('user2@server.com')
+			->setSharedBy('sharedBy')
+			->setShareOwner('shareOwner')
+			->setPermissions(19)
+			->setNode($node);
+		$this->provider->create($share2);
+
+		for($i = 0; $i < 200; $i++) {
+			$receiver = strval($i)."user2@server.com";
+			$share2 = $this->shareManager->newShare();
+			$share2->setSharedWith(strval($receiver))
+				->setSharedBy('sharedBy')
+				->setShareOwner('shareOwner')
+				->setPermissions(19)
+				->setNode($node);
+			$this->provider->create($share2);
+		}
+
+		$shares = $this->provider->getAllSharesBy('shareOwner', [\OCP\Share::SHARE_TYPE_REMOTE], [$node->getId()], true);
+
+		$this->assertCount(202, $shares);
+	}
+
 	public function testGetSharedBy() {
 		$node = $this->createMock('\OCP\Files\File');
 		$node->method('getId')->willReturn(42);