From 11e9ce669a67981031bf2bc32c467c666b9e9aa9 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Wed, 17 Aug 2016 09:58:53 +0200 Subject: [PATCH] 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(-) diff --git a/lib/private/Repair/RepairUnmergedShares.php b/lib/private/Repair/RepairUnmergedShares.php index c29de87c4e..d57bc3779f 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 7304bfd692..7b9d257938 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])); + } }