Merge pull request #29416 from owncloud/stable10-fix-29233

[stable10] Use chunks of 999 when preload directory content
owncloud · Nov 3, 2017 · 093b13b · 093b13b
2 parents 29d1661 + 9cace53
commit 093b13b
Show file tree

Hide file tree

Showing 2 changed files with 96 additions and 12 deletions.
diff --git a/apps/dav/lib/DAV/FileCustomPropertiesBackend.php b/apps/dav/lib/DAV/FileCustomPropertiesBackend.php
@@ -24,6 +24,8 @@
 namespace OCA\DAV\DAV;
 
 use Doctrine\DBAL\Connection;
+use Doctrine\DBAL\Platforms\OraclePlatform;
+use Doctrine\DBAL\Platforms\SqlitePlatform;
 use OCA\DAV\Connector\Sabre\Directory;
 use OCA\DAV\Connector\Sabre\Node;
 use Sabre\DAV\INode;
@@ -187,19 +189,20 @@ protected function loadChildrenProperties(INode $node, $requestedProperties) {
 		$sql = 'SELECT * FROM `*PREFIX*properties` WHERE `fileid` IN (?)';
 		$sql .= ' AND `propertyname` in (?) ORDER BY `propertyname`';
 
-		$result = $this->connection->executeQuery(
-			$sql,
-			[$childrenIds, $requestedProperties],
-			[Connection::PARAM_STR_ARRAY, Connection::PARAM_STR_ARRAY]
-		);
-
+		$fileIdChunks = $this->getChunks($childrenIds, count($requestedProperties));
 		$props = [];
-		while ($row = $result->fetch()) {
-			$props[$row['propertyname']] = $row['propertyvalue'];
-			$this->offsetSet($row['fileid'], $props);
+		foreach ($fileIdChunks as $chunk) {
+			$result = $this->connection->executeQuery(
+				$sql,
+				[$chunk, $requestedProperties],
+				[Connection::PARAM_STR_ARRAY, Connection::PARAM_STR_ARRAY]
+			);
+			while ($row = $result->fetch()) {
+				$props[$row['propertyname']] = $row['propertyvalue'];
+				$this->offsetSet($row['fileid'], $props);
+			}
+			$result->closeCursor();
 		}
-
-		$result->closeCursor();
 	}
 
 	/**
@@ -214,4 +217,25 @@ protected function getNodeForPath($path){
 		return $node;
 	}
 
+	/**
+	 * Chunk items from a single dimension array into a bidimensional array
+	 * that has a limit of items for the second dimension
+	 * This limit equals to 999 by default
+	 * and is decreased by $otherPlaceholdersCount
+	 *
+	 * @param int[] $toSlice
+	 * @param int $otherPlaceholdersCount
+	 * @return array
+	 */
+	private function getChunks($toSlice, $otherPlaceholdersCount = 0) {
+		$databasePlatform = $this->connection->getDatabasePlatform();
+		if ($databasePlatform instanceof OraclePlatform || $databasePlatform instanceof SqlitePlatform) {
+			$slicer = 999 - $otherPlaceholdersCount;
+			$slices = array_chunk($toSlice, $slicer);
+		} else {
+			$slices = count($toSlice) ? [ 0 => $toSlice] : [];
+		}
+		return $slices;
+	}
+
 }
diff --git a/apps/dav/tests/unit/DAV/FileCustomPropertiesBackendTest.php b/apps/dav/tests/unit/DAV/FileCustomPropertiesBackendTest.php
@@ -23,6 +23,9 @@
 
 namespace OCA\DAV\Tests\unit\DAV;
 
+use Doctrine\DBAL\Platforms\AbstractPlatform;
+use Doctrine\DBAL\Platforms\MySqlPlatform;
+use Doctrine\DBAL\Platforms\SqlitePlatform;
 use OCA\DAV\DAV\FileCustomPropertiesBackend;
 use Sabre\DAV\PropFind;
 use Sabre\DAV\SimpleCollection;
@@ -79,7 +82,6 @@ public function setUp() {
 			$this->user
 		);
 
-
 		$connection = \OC::$server->getDatabaseConnection();
 		$qb = $connection->getQueryBuilder();
 		$maxFunction = $qb->createFunction(
@@ -359,4 +361,62 @@ public function testDeleteProperty() {
 		$result = $propPatch->getResult();
 		$this->assertEquals(204, $result['customprop']);
 	}
+
+	public function slicesProvider() {
+		$emptyFileIds = [];
+		$fiveFileIds = [1, 2, 3, 4, 5];
+		$thousandFileIds = [];
+		for ($i=0;$i<1000;$i++){
+			$thousandFileIds[] = $i;
+		}
+
+		$sqlitePlatform = new SqlitePlatform();
+		$mysqlPlatform = new MySqlPlatform();
+
+		return [
+			[$emptyFileIds, 0, $sqlitePlatform, []],
+			[$emptyFileIds, 5, $sqlitePlatform, []],
+			[$fiveFileIds, 0, $sqlitePlatform, [ 0 => $fiveFileIds]],
+			[$fiveFileIds, 5, $sqlitePlatform, [ 0 => $fiveFileIds]],
+			[$fiveFileIds, 994, $sqlitePlatform, [ 0 => $fiveFileIds]],
+			[$fiveFileIds, 995, $sqlitePlatform, [ 0 => [1,2,3,4] , 1 => [5]]],
+			[$thousandFileIds, 0, $sqlitePlatform, array_chunk($thousandFileIds, 999)],
+			[$thousandFileIds, 5, $sqlitePlatform, array_chunk($thousandFileIds, 994)],
+
+			[$emptyFileIds, 0, $mysqlPlatform, []],
+			[$emptyFileIds, 5, $mysqlPlatform, []],
+			[$fiveFileIds, 0, $mysqlPlatform, [ 0 => $fiveFileIds]],
+			[$fiveFileIds, 5, $mysqlPlatform, [ 0 => $fiveFileIds]],
+			[$fiveFileIds, 994, $mysqlPlatform, [ 0 => $fiveFileIds]],
+			[$fiveFileIds, 995, $mysqlPlatform, [0 => $fiveFileIds]],
+			[$thousandFileIds, 0, $mysqlPlatform, [0 => $thousandFileIds]],
+			[$thousandFileIds, 5, $mysqlPlatform, [0 => $thousandFileIds]],
+		];
+	}
+
+	/**
+	 * @dataProvider slicesProvider
+	 * @param $toSlice
+	 * @param $otherPlaceholdersCount
+	 * @param AbstractPlatform $platform
+	 * @param $expected
+	 */
+	public function testGetChunks($toSlice, $otherPlaceholdersCount, AbstractPlatform $platform, $expected) {
+		$dbConnectionMock = $this->getMockBuilder(\OCP\IDBConnection::class)
+			->disableOriginalConstructor()
+			->getMock();
+
+		$dbConnectionMock->expects($this->any())
+			->method('getDatabasePlatform')
+			->will($this->returnValue($platform));
+
+		$this->plugin = new FileCustomPropertiesBackend(
+			$this->tree,
+			$dbConnectionMock,
+			$this->user
+		);
+
+		$actual = $this->invokePrivate($this->plugin, 'getChunks', [$toSlice, $otherPlaceholdersCount]);
+		$this->assertEquals($expected, $actual);
+	}
 }