[stable10] Update the sql query to delete duplicate sub share

Removal of 'group by id' is required to achieve the purpose of the repair script. Also made modification to the query which should support the updated version of postgresql, mysql. Signed-off-by: Sujith H <[email protected]>
owncloud · Jun 14, 2018 · 0ca42bd · 0ca42bd
1 parent db877df
commit 0ca42bd
Show file tree

Hide file tree

Showing 2 changed files with 205 additions and 39 deletions.
diff --git a/lib/private/Repair/RepairSubShares.php b/lib/private/Repair/RepairSubShares.php
@@ -55,17 +55,29 @@ public function getName() {
 	private function setRemoveAndSelectQuery() {
 		/**
 		 * Retrieves the duplicate rows with different id's
-		 * of oc_share
+		 * of oc_share. The query would look like:
+		 * SELECT id
+		 * FROM oc_share
+		 * WHERE id NOT IN (
+		 * 		SELECT MIN(id)
+		 * 		FROM oc_share
+		 * 		GROUP BY share_with, parent
+		 * )
+		 * AND share_type=2;
 		 */
 		$builder = $this->connection->getQueryBuilder();
-		$builder
-			->select('id', 'parent', $builder->createFunction('count(*)'))
+		$subQuery = $this->connection->getQueryBuilder();
+		$subQuery
+			->select($subQuery->createFunction("MIN(`id`)"))
+			->from('share')
+			->groupBy('share_with')
+			->addGroupBy('parent');
+
+		//$testing = $subQuery->getSQL();
+		$builder->select('id')
 			->from('share')
-			->where($builder->expr()->eq('share_type', $builder->createNamedParameter(2)))
-			->groupBy('parent')
-			->addGroupBy('id')
-			->addGroupBy('share_with')
-			->having('count(*) > 1')->setMaxResults(1000);
+			->where($builder->expr()->notIn('id', $builder->createFunction($subQuery->getSQL())))
+			->andWhere($builder->expr()->eq('share_type', $builder->createNamedParameter(2)));
 
 		$this->getDuplicateRows = $builder;
 
@@ -81,22 +93,13 @@ public function run(IOutput $output) {
 		$deletedEntries = 0;
 		$this->setRemoveAndSelectQuery();
 
-		/**
-		 * Going for pagination because if there are 1 million rows
-		 * it wont be easy to scale the data
-		 */
-		do {
-			$results = $this->getDuplicateRows->execute();
-			$rows = $results->fetchAll();
-			$results->closeCursor();
-			$lastResultCount = 0;
-
-			foreach ($rows as $row) {
-				$deletedEntries += $this->deleteShareId->setParameter('shareId', (int) $row['id'])
-					->execute();
-				$lastResultCount++;
-			}
-		} while($lastResultCount > 0);
+		$results = $this->getDuplicateRows->execute();
+		$rows = $results->fetchAll();
+		$results->closeCursor();
+		foreach ($rows as $row) {
+			$deletedEntries += $this->deleteShareId->setParameter('shareId', (int) $row['id'])
+				->execute();
+		}
 
 		if ($deletedEntries > 0) {
 			$output->info('Removed ' . $deletedEntries . ' shares where duplicate rows where found');

diff --git a/tests/lib/Repair/RepairSubSharesTest.php b/tests/lib/Repair/RepairSubSharesTest.php
@@ -69,6 +69,126 @@ public function deleteAllShares() {
 		$qb->delete('share')->execute();
 	}
 
+	/**
+	 * A test case to verify steps repair does address the simple step mentioned
+	 * at https://github.com/owncloud/core/issues/27990#issuecomment-357899190
+	 */
+
+	public function testRemoveDuplicateSubShare() {
+		$qb = $this->connection->getQueryBuilder();
+		/*
+		 * Create 3 users: user1, user2 and user3.
+		 */
+		$userName = "user";
+		$groupName = "group1";
+		$time = 1523892;
+		\OC::$server->getGroupManager()->createGroup($groupName);
+		for($userCount = 1; $userCount <= 3; $userCount++) {
+			$user = $this->createUser($userName.$userCount);
+			\OC::$server->getGroupManager()->get($groupName)->addUser($user);
+		}
+
+		$usersinsharetable = ['admin', 'user1', 'user2'];
+		foreach ($usersinsharetable as $user) {
+			if ($user === 'admin') {
+				$qb->insert('share')
+					->values([
+						'share_type' => $qb->expr()->literal('1'),
+						'share_with' => $qb->expr()->literal($groupName),
+						'uid_owner' => $qb->expr()->literal('admin'),
+						'uid_initiator' => $qb->expr()->literal('admin'),
+						'item_type' => $qb->expr()->literal('folder'),
+						'item_source' => $qb->expr()->literal(23),
+						'file_source' => $qb->expr()->literal(23),
+						'file_target' => $qb->expr()->literal('/test'),
+						'permissions' => $qb->expr()->literal(31),
+						'stime' => $qb->expr()->literal($time),
+						'mail_send' => $qb->expr()->literal('0'),
+						'accepted' => $qb->expr()->literal('0')
+					])
+					->execute();
+			} elseif ($user === 'user1') {
+				$qb->insert('share')
+					->values([
+						'share_type' => $qb->expr()->literal('2'),
+						'share_with' => $qb->expr()->literal($user),
+						'uid_owner' => $qb->expr()->literal('admin'),
+						'uid_initiator' => $qb->expr()->literal('admin'),
+						'parent' => $qb->expr()->literal('1'),
+						'item_type' => $qb->expr()->literal('folder'),
+						'item_source' => $qb->expr()->literal(23),
+						'file_source' => $qb->expr()->literal(23),
+						'file_target' => $qb->expr()->literal('/test'),
+						'permissions' => $qb->expr()->literal(31),
+						'stime' => $qb->expr()->literal($time),
+						'mail_send' => $qb->expr()->literal('0'),
+						'accepted' => $qb->expr()->literal('0')
+					])
+					->execute();
+			} else {
+				for ($i = 0; $i < 2; $i++) {
+					$qb->insert('share')
+						->values([
+							'share_type' => $qb->expr()->literal('2'),
+							'share_with' => $qb->expr()->literal($user),
+							'uid_owner' => $qb->expr()->literal('admin'),
+							'uid_initiator' => $qb->expr()->literal('admin'),
+							'parent' => $qb->expr()->literal('1'),
+							'item_type' => $qb->expr()->literal('folder'),
+							'item_source' => $qb->expr()->literal(23),
+							'file_source' => $qb->expr()->literal(23),
+							'file_target' => $qb->expr()->literal('/test_mine'),
+							'permissions' => $qb->expr()->literal(31),
+							'stime' => $qb->expr()->literal($time),
+							'mail_send' => $qb->expr()->literal('0'),
+							'accepted' => $qb->expr()->literal('0')
+						])
+						->execute();
+				}
+			}
+		}
+
+		$outputMock = $this->createMock(IOutput::class);
+		$this->repair->run($outputMock);
+
+		$qb = $this->connection->getQueryBuilder();
+		$subQuery = $this->connection->getQueryBuilder();
+		$subQuery
+			->select($subQuery->createFunction('MIN(`id`)'))
+			->from('share')
+			->groupBy('share_with')
+			->addGroupBy('parent');
+		$qb->select('id');
+		$qb->from('share');
+		$qb->where($qb->expr()->notIn('id', $qb->createFunction($subQuery->getSQL())));
+		$qb->andWhere($qb->expr()->eq('share_type', $qb->createNamedParameter(2)));
+		$results = $qb->execute()->fetchAll();
+		$this->assertCount(0, $results);
+
+		//There should be only one entry for share_with as 'user2'
+		$qb = $this->connection->getQueryBuilder();
+		$qb->select($qb->createFunction('count(*)'))
+			->from('share')
+			->where($qb->expr()->eq('share_with', $qb->createNamedParameter('user2')));
+		$results = $qb->execute()->fetchAll();
+		$this->assertCount(1, $results);
+
+		$qb = $this->connection->getQueryBuilder();
+		$qb->select('share_with')
+			->from('share');
+		$results = $qb->execute()->fetchAll();
+		//Only 3 entries should be there
+		$this->assertCount(3, $results);
+		$userArray = [
+			array('share_with' => 'group1'),
+			array('share_with' => 'user1'),
+			array('share_with' => 'user2')
+		];
+		foreach ($userArray as $sharewith) {
+			$this->assertTrue(in_array($sharewith, $results));
+		}
+	}
+
 	/**
 	 * This is a very basic test
 	 * This test would populate DB with data
@@ -83,7 +203,7 @@ public function testPopulateDBAndRemoveDuplicates() {
 		$userName = "user";
 		$groupName = "group";
 		$folderName = "/test";
-		$time = time();
+		$time = 1523892;
 		$groupCount = 1;
 		$totalGroups = 3;
 		$parent = 1;
@@ -117,7 +237,7 @@ public function testPopulateDBAndRemoveDuplicates() {
 			 */
 			if (($userCount % $totalGroups) === 0) {
 				$groupCount++;
-				$time = time();
+				$time += 1;
 			}
 
 			/**
@@ -132,16 +252,38 @@ public function testPopulateDBAndRemoveDuplicates() {
 		$this->repair->run($outputMock);
 
 		$qb = $this->connection->getQueryBuilder();
-		$qb->select('id', 'parent', $qb->createFunction('count(*)'))
+		$subQuery = $this->connection->getQueryBuilder();
+		$subQuery
+			->select($subQuery->createFunction('MIN(`id`)'))
 			->from('share')
-			->where($qb->expr()->eq('share_type', $qb->createNamedParameter(2)))
-			->groupBy('parent')
-			->addGroupBy('id')
-			->addGroupBy('share_with')
-			->having('count(*) > 1')->setMaxResults(1000);
+			->groupBy('share_with')
+			->addGroupBy('parent');
+		$qb->select('id');
+		$qb->from('share');
+		$qb->where($qb->expr()->notIn('id', $qb->createFunction($subQuery->getSQL())));
+		$qb->andWhere($qb->expr()->eq('share_type', $qb->createNamedParameter(2)));
 
 		$results = $qb->execute()->fetchAll();
 		$this->assertCount(0, $results);
+
+		$qb = $this->connection->getQueryBuilder();
+		$qb->select('share_with', 'parent')
+			->from('share');
+		$results = $qb->execute()->fetchAll();
+		//Only 7 entries should be there
+		$this->assertCount(7, $results);
+		$userArray = [
+			array('share_with' => 'user1', 'parent' => '1'),
+			array('share_with' => 'user1', 'parent' => '2'),
+			array('share_with' => 'user2', 'parent' => '2'),
+			array('share_with' => 'user2', 'parent' => '3'),
+			array('share_with' => 'user3', 'parent' => '4'),
+			array('share_with' => 'user3', 'parent' => '5'),
+			array('share_with' => 'user4', 'parent' => '5'),
+		];
+		foreach ($userArray as $sharewith) {
+			$this->assertTrue(in_array($sharewith, $results));
+		}
 	}
 
 	/**
@@ -151,7 +293,7 @@ public function testPopulateDBAndRemoveDuplicates() {
 	public function testLargeDuplicateShareRows() {
 		$qb = $this->connection->getQueryBuilder();
 		$userName = "user";
-		$time = time();
+		$time = 15238923;
 		$groupCount = 0;
 		$folderName = "/test";
 		$maxUsersPerGroup = 1000;
@@ -186,15 +328,36 @@ public function testLargeDuplicateShareRows() {
 		$this->repair->run($outputMock);
 
 		$qb = $this->connection->getQueryBuilder();
-		$qb->select('id', 'parent', $qb->createFunction('count(*)'))
+		$subQuery = $this->connection->getQueryBuilder();
+		$subQuery
+			->select($subQuery->createFunction('MIN(`id`)'))
 			->from('share')
-			->where($qb->expr()->eq('share_type', $qb->createNamedParameter(2)))
-			->groupBy('parent')
-			->addGroupBy('id')
-			->addGroupBy('share_with')
-			->having('count(*) > 1')->setMaxResults(1000);
+			->groupBy('share_with')
+			->addGroupBy('parent');
+		$qb->select('id');
+		$qb->from('share');
+		$qb->where($qb->expr()->notIn('id', $qb->createFunction($subQuery->getSQL())));
+		$qb->andWhere($qb->expr()->eq('share_type', $qb->createNamedParameter(2)));
 
 		$results = $qb->execute()->fetchAll();
 		$this->assertCount(0, $results);
+
+		$qb = $this->connection->getQueryBuilder();
+		$qb->select('share_with', 'parent')
+			->from('share');
+		$results = $qb->execute()->fetchAll();
+
+		$this->assertCount(6, $results);
+		$userArray = [
+			array('share_with' => 'user1', 'parent' => '1'),
+			array('share_with' => 'user2', 'parent' => '2'),
+			array('share_with' => 'user3', 'parent' => '3'),
+			array('share_with' => 'user4', 'parent' => '4'),
+			array('share_with' => 'user5', 'parent' => '5'),
+			array('share_with' => 'user6', 'parent' => '6'),
+		];
+		foreach ($userArray as $sharewith) {
+			$this->assertTrue(in_array($sharewith, $results));
+		}
 	}
 }