From 56b94b220da4d947438ffee7a4e889ef7f61bbde Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Fri, 12 Aug 2016 11:44:34 +0200 Subject: Improve file_target finding logic when repairing unmerged shares Pick the most recent subshare that has no parenthesis from duplication which should match whichever name the user picked last. If all subshares have duplicate parenthesis names, use the least recent group share's target instead. --- lib/private/Repair/RepairUnmergedShares.php | 40 ++++++++++++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-) (limited to 'lib') diff --git a/lib/private/Repair/RepairUnmergedShares.php b/lib/private/Repair/RepairUnmergedShares.php index 353877bb873..f14e98c1761 100644 --- a/lib/private/Repair/RepairUnmergedShares.php +++ b/lib/private/Repair/RepairUnmergedShares.php @@ -148,6 +148,44 @@ class RepairUnmergedShares implements IRepairStep { return $groupedShares; } + /** + * Decide on the best target name based on all group shares and subshares, + * goal is to increase the likeliness that the chosen name matches what + * the user is expecting. + * + * For this, we discard the entries with parenthesis "(2)". + * In case the user also renamed the duplicates to a legitimate name, this logic + * will still pick the most recent one as it's the one the user is most likely to + * remember renaming. + * + * If no suitable subshare is found, use the least recent group share instead. + * + * @param array $groupShares group share entries + * @param array $subShares sub share entries, sorted by stime + * + * @return string chosen target name + */ + private function findBestTargetName($groupShares, $subShares) { + $pickedShare = null; + // note subShares are sorted by stime from oldest to newest + foreach ($subShares as $subShare) { + // skip entries that have parenthesis with numbers + if (preg_match('/\([0-9]*\)/', $subShare['file_target']) === 1) { + continue; + } + // pick any share found that would match, the last being the most recent + $pickedShare = $subShare; + } + + // no suitable subshare found + if ($pickedShare === null) { + // use least recent group share target instead + $pickedShare = $groupShares[0]; + } + + return $pickedShare['file_target']; + } + /** * Fix the given received share represented by the set of group shares * and matching sub shares @@ -171,7 +209,7 @@ class RepairUnmergedShares implements IRepairStep { return false; } - $targetPath = $groupShares[0]['file_target']; + $targetPath = $this->findBestTargetName($groupShares, $subShares); // check whether the user opted out completely of all subshares $optedOut = true; -- cgit v1.2.3 From 7a2d25fab4bb2b452b513e41adda4dec68e81bbe Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Fri, 12 Aug 2016 12:06:57 +0200 Subject: Fix unmerged shares repair with mixed group and direct shares Whenever a group share is created after a direct share, the stime order needs to be properly considered in the repair routine, considering that the direct user share is appended to the $subShares array and breaking its order. --- lib/private/Repair/RepairUnmergedShares.php | 16 ++++++++-- tests/lib/Repair/RepairUnmergedSharesTest.php | 45 ++++++++++++++++++++++++++- 2 files changed, 57 insertions(+), 4 deletions(-) (limited to 'lib') diff --git a/lib/private/Repair/RepairUnmergedShares.php b/lib/private/Repair/RepairUnmergedShares.php index f14e98c1761..c29de87c4e8 100644 --- a/lib/private/Repair/RepairUnmergedShares.php +++ b/lib/private/Repair/RepairUnmergedShares.php @@ -93,7 +93,7 @@ class RepairUnmergedShares implements IRepairStep { */ $query = $this->connection->getQueryBuilder(); $query - ->select('item_source', 'id', 'file_target', 'permissions', 'parent', 'share_type') + ->select('item_source', 'id', 'file_target', 'permissions', 'parent', 'share_type', 'stime') ->from('share') ->where($query->expr()->eq('share_type', $query->createParameter('shareType'))) ->andWhere($query->expr()->in('share_with', $query->createParameter('shareWiths'))) @@ -161,13 +161,23 @@ class RepairUnmergedShares implements IRepairStep { * If no suitable subshare is found, use the least recent group share instead. * * @param array $groupShares group share entries - * @param array $subShares sub share entries, sorted by stime + * @param array $subShares sub share entries * * @return string chosen target name */ private function findBestTargetName($groupShares, $subShares) { $pickedShare = null; - // note subShares are sorted by stime from oldest to newest + // sort by stime, this also properly sorts the direct user share if any + @usort($subShares, function($a, $b) { + if ($a['stime'] < $b['stime']) { + return -1; + } else if ($a['stime'] > $b['stime']) { + return 1; + } + + return 0; + }); + foreach ($subShares as $subShare) { // skip entries that have parenthesis with numbers if (preg_match('/\([0-9]*\)/', $subShare['file_target']) === 1) { diff --git a/tests/lib/Repair/RepairUnmergedSharesTest.php b/tests/lib/Repair/RepairUnmergedSharesTest.php index 9fe9a073881..7304bfd6920 100644 --- a/tests/lib/Repair/RepairUnmergedSharesTest.php +++ b/tests/lib/Repair/RepairUnmergedSharesTest.php @@ -44,6 +44,9 @@ class RepairUnmergedSharesTest extends TestCase { /** @var \OCP\IDBConnection */ private $connection; + /** @var int */ + private $lastShareTime; + protected function setUp() { parent::setUp(); @@ -92,6 +95,9 @@ class RepairUnmergedSharesTest extends TestCase { } })); + // used to generate incremental stimes + $this->lastShareTime = time(); + /** @var \OCP\IConfig $config */ $this->repair = new RepairUnmergedShares($config, $this->connection, $userManager, $groupManager); } @@ -108,6 +114,7 @@ class RepairUnmergedSharesTest extends TestCase { } private function createShare($type, $sourceId, $recipient, $targetName, $permissions, $parentId = null) { + $this->lastShareTime += 100; $qb = $this->connection->getQueryBuilder(); $values = [ 'share_type' => $qb->expr()->literal($type), @@ -119,7 +126,7 @@ class RepairUnmergedSharesTest extends TestCase { 'file_source' => $qb->expr()->literal($sourceId), 'file_target' => $qb->expr()->literal($targetName), 'permissions' => $qb->expr()->literal($permissions), - 'stime' => $qb->expr()->literal(time()), + 'stime' => $qb->expr()->literal($this->lastShareTime), ]; if ($parentId !== null) { $values['parent'] = $qb->expr()->literal($parentId); @@ -458,6 +465,42 @@ class RepairUnmergedSharesTest extends TestCase { ['/test (4)', 31], ] ], + [ + // #13 bogus share: + // - outsider shares with group1, user2 and then group2 + // - user renamed share as soon as it arrived before the next share (order) + // - one subshare for each group share + // - one extra share entry for direct share to user2 + // - non-matching targets + [ + // first share with group + [Constants::SHARE_TYPE_GROUP, 123, 'recipientgroup1', '/test', 31], + // recipient renames + [DefaultShareProvider::SHARE_TYPE_USERGROUP, 123, 'user2', '/first', 31, 0], + // then direct share, user renames too + [Constants::SHARE_TYPE_USER, 123, 'user2', '/second', 31], + // another share with the second group + [Constants::SHARE_TYPE_GROUP, 123, 'recipientgroup2', '/test', 31], + // use renames it + [DefaultShareProvider::SHARE_TYPE_USERGROUP, 123, 'user2', '/third', 31, 1], + // different unrelated share + [Constants::SHARE_TYPE_GROUP, 456, 'recipientgroup1', '/test (5)', 31], + ], + [ + // group share with group1 left alone + ['/test', 31], + // first subshare repaired + ['/third', 31], + // direct user share repaired + ['/third', 31], + // group share with group2 left alone + ['/test', 31], + // second subshare repaired + ['/third', 31], + // leave unrelated alone + ['/test (5)', 31], + ] + ], ]; } -- cgit v1.2.3 From df9b509ed33ef6e3041b76b4b7ce1b22c7d81fcc Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Wed, 17 Aug 2016 09:58:53 +0200 Subject: Improve regexp to detect duplicate folders when repairing unmerged shares --- lib/private/Repair/RepairUnmergedShares.php | 14 ++-- tests/lib/Repair/RepairUnmergedSharesTest.php | 102 +++++++++++++++++--------- 2 files changed, 74 insertions(+), 42 deletions(-) (limited to 'lib') diff --git a/lib/private/Repair/RepairUnmergedShares.php b/lib/private/Repair/RepairUnmergedShares.php index c29de87c4e8..d57bc3779f8 100644 --- a/lib/private/Repair/RepairUnmergedShares.php +++ b/lib/private/Repair/RepairUnmergedShares.php @@ -148,6 +148,10 @@ class RepairUnmergedShares implements IRepairStep { return $groupedShares; } + private function isPotentialDuplicateName($name) { + return (preg_match('/\(\d+\)(\.[^\.]+)?$/', $name) === 1); + } + /** * Decide on the best target name based on all group shares and subshares, * goal is to increase the likeliness that the chosen name matches what @@ -169,18 +173,12 @@ class RepairUnmergedShares implements IRepairStep { $pickedShare = null; // sort by stime, this also properly sorts the direct user share if any @usort($subShares, function($a, $b) { - if ($a['stime'] < $b['stime']) { - return -1; - } else if ($a['stime'] > $b['stime']) { - return 1; - } - - return 0; + return ((int)$a['stime'] - (int)$b['stime']); }); foreach ($subShares as $subShare) { // skip entries that have parenthesis with numbers - if (preg_match('/\([0-9]*\)/', $subShare['file_target']) === 1) { + if ($this->isPotentialDuplicateName($subShare['file_target'])) { continue; } // pick any share found that would match, the last being the most recent diff --git a/tests/lib/Repair/RepairUnmergedSharesTest.php b/tests/lib/Repair/RepairUnmergedSharesTest.php index 7304bfd6920..7b9d2579389 100644 --- a/tests/lib/Repair/RepairUnmergedSharesTest.php +++ b/tests/lib/Repair/RepairUnmergedSharesTest.php @@ -28,6 +28,8 @@ use OCP\Migration\IOutput; use OCP\Migration\IRepairStep; use Test\TestCase; use OC\Share20\DefaultShareProvider; +use OCP\IUserManager; +use OCP\IGroupManager; /** * Tests for repairing invalid shares @@ -47,6 +49,12 @@ class RepairUnmergedSharesTest extends TestCase { /** @var int */ private $lastShareTime; + /** @var IUserManager */ + private $userManager; + + /** @var IGroupManager */ + private $groupManager; + protected function setUp() { parent::setUp(); @@ -61,45 +69,14 @@ class RepairUnmergedSharesTest extends TestCase { $this->connection = \OC::$server->getDatabaseConnection(); $this->deleteAllShares(); - $user1 = $this->getMock('\OCP\IUser'); - $user1->expects($this->any()) - ->method('getUID') - ->will($this->returnValue('user1')); - - $user2 = $this->getMock('\OCP\IUser'); - $user2->expects($this->any()) - ->method('getUID') - ->will($this->returnValue('user2')); - - $users = [$user1, $user2]; - - $groupManager = $this->getMock('\OCP\IGroupManager'); - $groupManager->expects($this->any()) - ->method('getUserGroupIds') - ->will($this->returnValueMap([ - // owner - [$user1, ['samegroup1', 'samegroup2']], - // recipient - [$user2, ['recipientgroup1', 'recipientgroup2']], - ])); - - $userManager = $this->getMock('\OCP\IUserManager'); - $userManager->expects($this->once()) - ->method('countUsers') - ->will($this->returnValue([2])); - $userManager->expects($this->once()) - ->method('callForAllUsers') - ->will($this->returnCallback(function(\Closure $closure) use ($users) { - foreach ($users as $user) { - $closure($user); - } - })); + $this->userManager = $this->getMock('\OCP\IUserManager'); + $this->groupManager = $this->getMock('\OCP\IGroupManager'); // used to generate incremental stimes $this->lastShareTime = time(); /** @var \OCP\IConfig $config */ - $this->repair = new RepairUnmergedShares($config, $this->connection, $userManager, $groupManager); + $this->repair = new RepairUnmergedShares($config, $this->connection, $this->userManager, $this->groupManager); } protected function tearDown() { @@ -510,6 +487,38 @@ class RepairUnmergedSharesTest extends TestCase { * @dataProvider sharesDataProvider */ public function testMergeGroupShares($shares, $expectedShares) { + $user1 = $this->getMock('\OCP\IUser'); + $user1->expects($this->any()) + ->method('getUID') + ->will($this->returnValue('user1')); + + $user2 = $this->getMock('\OCP\IUser'); + $user2->expects($this->any()) + ->method('getUID') + ->will($this->returnValue('user2')); + + $users = [$user1, $user2]; + + $this->groupManager->expects($this->any()) + ->method('getUserGroupIds') + ->will($this->returnValueMap([ + // owner + [$user1, ['samegroup1', 'samegroup2']], + // recipient + [$user2, ['recipientgroup1', 'recipientgroup2']], + ])); + + $this->userManager->expects($this->once()) + ->method('countUsers') + ->will($this->returnValue([2])); + $this->userManager->expects($this->once()) + ->method('callForAllUsers') + ->will($this->returnCallback(function(\Closure $closure) use ($users) { + foreach ($users as $user) { + $closure($user); + } + })); + $shareIds = []; foreach ($shares as $share) { @@ -536,5 +545,30 @@ class RepairUnmergedSharesTest extends TestCase { $this->assertEquals($expectedShare[1], $share['permissions']); } } + + public function duplicateNamesProvider() { + return [ + // matching + ['filename (1).txt', true], + ['folder (2)', true], + ['filename (1)(2).txt', true], + // non-matching + ['filename ().txt', false], + ['folder ()', false], + ['folder (1x)', false], + ['folder (x1)', false], + ['filename (a)', false], + ['filename (1).', false], + ['filename (1).txt.txt', false], + ['filename (1)..txt', false], + ]; + } + + /** + * @dataProvider duplicateNamesProvider + */ + public function testIsPotentialDuplicateName($name, $expectedResult) { + $this->assertEquals($expectedResult, $this->invokePrivate($this->repair, 'isPotentialDuplicateName', [$name])); + } } -- cgit v1.2.3