owncloud · PVince81 · Jul 18, 2018 · Jun 14, 2018
diff --git a/lib/private/Repair/RepairSubShares.php b/lib/private/Repair/RepairSubShares.php
@@ -48,55 +48,75 @@ public function getName() {
 	}
 
 	/**
-	 * Set query to remove duplicate rows.
-	 * i.e, except id all columns are same for oc_share
-	 * Also set query to select rows which have duplicate rows of share.
+	 * Get query builder to select rows which have duplicate rows of share
+	 * @return IQueryBuilder
 	 */
-	private function setRemoveAndSelectQuery() {
+	private function getSelectQueryToDetectDuplicatesBuilder() {
 		/**
 		 * 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')
-			->where($builder->expr()->eq('share_type', $builder->createNamedParameter(2)))
-			->groupBy('parent')
-			->addGroupBy('id')
-			->addGroupBy('share_with')
-			->having('count(*) > 1')->setMaxResults(1000);
+			->groupBy('share_with')
+			->addGroupBy('parent');
 
-		$this->getDuplicateRows = $builder;
+		$builder->select('id')
+			->from('share')
+			->where($builder->expr()->notIn('id', $builder->createFunction($subQuery->getSQL())))
+			->andWhere($builder->expr()->eq('share_type', $builder->createNamedParameter(2)));
+		return $builder;
+	}
 
+	/**
+	 * Get query builder to delete rows which have duplicate rows of share based
+	 * on ids
+	 * @return IQueryBuilder
+	 */
+	private function getDeleteShareIdsBuilder() {
 		$builder = $this->connection->getQueryBuilder();
 		$builder
 			->delete('share')
-			->where($builder->expr()->eq('id', $builder->createParameter('shareId')));
-
-		$this->deleteShareId = $builder;
+			->where($builder->expr()->in('id', $builder->createParameter('shareIds')));
+		return $builder;
 	}
 
 	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;
+		$selectDuplicates = $this->getSelectQueryToDetectDuplicatesBuilder();
+
+		$results = $selectDuplicates->execute();
+		$rows = $results->fetchAll();
+		$results->closeCursor();
+		$rowIds = [];
+		if (\count($rows) > 0) {
+			$rowIds = \array_map(
+				function ($value) {
+					return (int)$value['id'];
+				},
+				$rows
+			);
+		}
 
-			foreach ($rows as $row) {
-				$deletedEntries += $this->deleteShareId->setParameter('shareId', (int) $row['id'])
+		if (\count($rows) > 0) {
+			//Delete in a batch of 1000 ids
+			$deleteShareIds = $this->getDeleteShareIdsBuilder();
+			foreach (\array_chunk($rowIds, 1000) as $getIds) {
+				$deletedEntries += $deleteShareIds->setParameter('shareIds', $getIds, IQueryBuilder::PARAM_INT_ARRAY)
 					->execute();
-				$lastResultCount++;
 			}
-		} while($lastResultCount > 0);
+		}
 
 		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,87 @@ 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;
+		//This array holds the id, share_with and parent per user
+		$getAllIdsPerUser = [];
+		\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', 'user2'];
+		foreach ($usersinsharetable as $user) {
+			//start with a simple insert query and alter later based on users.
+			$inserValues = [
+				'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')
+			];
+
+			$userIndex = \array_search($user, $usersinsharetable, true);
+			if ($userIndex === 0) {
+				$inserValues['share_type'] = $qb->expr()->literal(1);
+				$inserValues['share_with'] = $qb->expr()->literal($groupName);
+				$user = $groupName;
+			} else {
+				$inserValues['share_type'] = $qb->expr()->literal(2);
+				$inserValues['share_with'] = $qb->expr()->literal($user);
+				$inserValues['parent'] = $qb->expr()->literal(1);
+			}
+
+			$qb->insert('share')
+				->values($inserValues)
+				->execute();
+			$getAllIdsPerUser[$user][] = ['id' => $this->getLastSharedId(), 'share_with' => $user];
+		}
+
+		$outputMock = $this->createMock(IOutput::class);
+		$this->repair->run($outputMock);
+
+		$qb = $this->invokePrivate($this->repair, 'getSelectQueryToDetectDuplicatesBuilder', []);
+		$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('id', 'share_with')
+			->from('share');
+		$results = $qb->execute()->fetchAll();
+		//Only 3 entries should be there
+		$this->assertCount(3, $results);
+
+		foreach ($results as $id) {
+			$this->assertTrue(\in_array($id, $getAllIdsPerUser[$id['share_with']]));
+		}
+	}
+
 	/**
 	 * This is a very basic test
 	 * This test would populate DB with data
@@ -83,12 +164,14 @@ public function testPopulateDBAndRemoveDuplicates() {
 		$userName = "user";
 		$groupName = "group";
 		$folderName = "/test";
-		$time = time();
+		$time = 1523892;
 		$groupCount = 1;
 		$totalGroups = 3;
 		$parent = 1;
+		//This array holds the id, share_with and parent per user
+		$getAllIdsPerUser = [];
 		$multipleOf = 2;
-		for($userCount = 1; $userCount <= 10; $userCount++) {
+		for ($userCount = 1; $userCount <= 10; $userCount++) {
 			$user = $this->createUser($userName.$userCount);
 			if (\OC::$server->getGroupManager()->groupExists($groupName.$groupCount) === false) {
 				\OC::$server->getGroupManager()->createGroup($groupName.$groupCount);
@@ -98,7 +181,7 @@ public function testPopulateDBAndRemoveDuplicates() {
 			//Create a group share
 			$qb->insert('share')
 				->values([
-					'share_type' => $qb->expr()->literal('2'),
+					'share_type' => $qb->expr()->literal(2),
 					'share_with' => $qb->expr()->literal($userName.$groupCount),
 					'uid_owner' => $qb->expr()->literal('admin'),
 					'uid_initiator' => $qb->expr()->literal('admin'),
@@ -111,13 +194,14 @@ public function testPopulateDBAndRemoveDuplicates() {
 					'stime' => $qb->expr()->literal($time),
 				])
 				->execute();
+			$getAllIdsPerUser['ids'][] = ['id' => $this->getLastSharedId()];
 
 			/**
 			 * Group count incremented once value of userCount reaches multiple of 3
 			 */
 			if (($userCount % $totalGroups) === 0) {
 				$groupCount++;
-				$time = time();
+				$time += 1;
 			}
 
 			/**
@@ -128,20 +212,30 @@ public function testPopulateDBAndRemoveDuplicates() {
 			}
 		}
 
+		$qb = $this->invokePrivate($this->repair, 'getSelectQueryToDetectDuplicatesBuilder', []);
+		$idsNotPresent = $qb->execute()->fetchAll();
+
 		$outputMock = $this->createMock(IOutput::class);
 		$this->repair->run($outputMock);
 
-		$qb = $this->connection->getQueryBuilder();
-		$qb->select('id', 'parent', $qb->createFunction('count(*)'))
-			->from('share')
-			->where($qb->expr()->eq('share_type', $qb->createNamedParameter(2)))
-			->groupBy('parent')
-			->addGroupBy('id')
-			->addGroupBy('share_with')
-			->having('count(*) > 1')->setMaxResults(1000);
-
 		$results = $qb->execute()->fetchAll();
 		$this->assertCount(0, $results);
+
+		$qb = $this->connection->getQueryBuilder();
+		$qb->select('id')
+			->from('share');
+		$results = $qb->execute()->fetchAll();
+		//Only 7 entries should be there
+		$this->assertCount(7, $results);
+
+		foreach ($results as $id) {
+			$this->assertTrue(\in_array($id, $getAllIdsPerUser['ids']));
+		}
+
+		//Verify that these ids are not there
+		foreach ($results as $id) {
+			$this->assertFalse(\in_array($id['id'], $idsNotPresent, true));
+		}
 	}
 
 	/**
@@ -151,11 +245,13 @@ public function testPopulateDBAndRemoveDuplicates() {
 	public function testLargeDuplicateShareRows() {
 		$qb = $this->connection->getQueryBuilder();
 		$userName = "user";
-		$time = time();
+		$time = 15238923;
 		$groupCount = 0;
 		$folderName = "/test";
 		$maxUsersPerGroup = 1000;
 		$parent = $groupCount + 1;
+		//This array holds the id, share_with and parent per user
+		$getAllIdsPerUser = [];
 		for ($userCount = 0; $userCount < 5500; $userCount++) {
 			/**
 			 * groupCount is incremented once userCount reaches
@@ -167,7 +263,7 @@ public function testLargeDuplicateShareRows() {
 			}
 			$qb->insert('share')
 				->values([
-					'share_type' => $qb->expr()->literal('2'),
+					'share_type' => $qb->expr()->literal(2),
 					'share_with' => $qb->expr()->literal($userName.$groupCount),
 					'uid_owner' => $qb->expr()->literal('admin'),
 					'uid_initiator' => $qb->expr()->literal('admin'),
@@ -180,21 +276,49 @@ public function testLargeDuplicateShareRows() {
 					'stime' => $qb->expr()->literal($time),
 				])
 				->execute();
+			$getAllIdsPerUser['ids'][] = [
+				'id' => $this->getLastSharedId()];
 		}
 
 		$outputMock = $this->createMock(IOutput::class);
 		$this->repair->run($outputMock);
 
-		$qb = $this->connection->getQueryBuilder();
-		$qb->select('id', 'parent', $qb->createFunction('count(*)'))
-			->from('share')
-			->where($qb->expr()->eq('share_type', $qb->createNamedParameter(2)))
-			->groupBy('parent')
-			->addGroupBy('id')
-			->addGroupBy('share_with')
-			->having('count(*) > 1')->setMaxResults(1000);
+		$qb = $this->invokePrivate($this->repair, 'getSelectQueryToDetectDuplicatesBuilder', []);
 
 		$results = $qb->execute()->fetchAll();
 		$this->assertCount(0, $results);
+
+		$qb = $this->connection->getQueryBuilder();
+		$qb->select('id')
+			->from('share');
+		$results = $qb->execute()->fetchAll();
+
+		$this->assertCount(6, $results);
+
+		foreach ($results as $id) {
+			$this->assertTrue(\in_array($id, $getAllIdsPerUser['ids']));
+			if (\array_search($id, $results, true) === 5) {
+				for ($i = $id['id'] + 1; $i < $id['id'] + 486; $i++) {
+					$this->assertFalse(\in_array(['id' => $i], $results));
+				}
+			} else {
+				//The next 1000 ids will not be there.
+				for ($i = $id['id'] + 1; $i < $id['id'] + 999; $i++) {
+					$this->assertFalse(\in_array(['id' => $i], $results));
+				}
+			}
+		}
+	}
+
+	public function testName() {
+		$this->assertEquals('Repair sub shares', $this->repair->getName());
+	}
+
+	/**
+	 * Returns last inserted id to the oc_share table
+	 * @return int
+	 */
+	protected function getLastSharedId() {
+		return $this->connection->lastInsertId('*PREFIX*share');
 	}
 }