From 3028684d8972c58a2146ebbdf1918fd3f730249e Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Tue, 9 Feb 2016 11:39:22 +0100 Subject: [PATCH 1/3] Fix system tag filter AND condition If one of the results is empty, no need to do array_intersect and return an empty result directly. --- .../lib/connector/sabre/filesreportplugin.php | 5 +++++ .../connector/sabre/filesreportplugin.php | 22 +++++++++++++++++++ 2 files changed, 27 insertions(+) diff --git a/apps/dav/lib/connector/sabre/filesreportplugin.php b/apps/dav/lib/connector/sabre/filesreportplugin.php index 5bdd7a71dd..d7e4f8beda 100644 --- a/apps/dav/lib/connector/sabre/filesreportplugin.php +++ b/apps/dav/lib/connector/sabre/filesreportplugin.php @@ -239,6 +239,11 @@ class FilesReportPlugin extends ServerPlugin { foreach ($systemTagIds as $systemTagId) { $fileIds = $this->tagMapper->getObjectIdsForTags($systemTagId, 'files'); + if (empty($fileIds)) { + return []; + } + + // first run ? if (empty($resultFileIds)) { $resultFileIds = $fileIds; } else { diff --git a/apps/dav/tests/unit/connector/sabre/filesreportplugin.php b/apps/dav/tests/unit/connector/sabre/filesreportplugin.php index 853e52f503..54badd323f 100644 --- a/apps/dav/tests/unit/connector/sabre/filesreportplugin.php +++ b/apps/dav/tests/unit/connector/sabre/filesreportplugin.php @@ -395,6 +395,28 @@ class FilesReportPlugin extends \Test\TestCase { $this->assertEquals(['222'], array_values($this->plugin->processFilterRules($rules))); } + public function testProcessFilterRulesAndConditionWithOneEmptyResult() { + $this->groupManager->expects($this->any()) + ->method('isAdmin') + ->will($this->returnValue(true)); + + $this->tagMapper->expects($this->at(0)) + ->method('getObjectIdsForTags') + ->with('123') + ->will($this->returnValue(['111', '222'])); + $this->tagMapper->expects($this->at(1)) + ->method('getObjectIdsForTags') + ->with('456') + ->will($this->returnValue([])); + + $rules = [ + ['name' => '{http://owncloud.org/ns}systemtag', 'value' => '123'], + ['name' => '{http://owncloud.org/ns}systemtag', 'value' => '456'], + ]; + + $this->assertEquals([], array_values($this->plugin->processFilterRules($rules))); + } + public function testProcessFilterRulesInvisibleTagAsAdmin() { $this->groupManager->expects($this->any()) ->method('isAdmin') From 178914104c494a40b078638396bc9fdb511806ab Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Tue, 9 Feb 2016 11:47:33 +0100 Subject: [PATCH 2/3] Add a test for empty mid-result --- .../connector/sabre/filesreportplugin.php | 109 ++++++++++++++---- 1 file changed, 85 insertions(+), 24 deletions(-) diff --git a/apps/dav/tests/unit/connector/sabre/filesreportplugin.php b/apps/dav/tests/unit/connector/sabre/filesreportplugin.php index 54badd323f..b528e2d242 100644 --- a/apps/dav/tests/unit/connector/sabre/filesreportplugin.php +++ b/apps/dav/tests/unit/connector/sabre/filesreportplugin.php @@ -30,16 +30,16 @@ use OCP\IGroupManager; use OCP\SystemTag\ISystemTagManager; class FilesReportPlugin extends \Test\TestCase { - /** @var \Sabre\DAV\Server */ + /** @var \Sabre\DAV\Server|\PHPUnit_Framework_MockObject_MockObject */ private $server; - /** @var \Sabre\DAV\Tree */ + /** @var \Sabre\DAV\Tree|\PHPUnit_Framework_MockObject_MockObject */ private $tree; - /** @var ISystemTagObjectMapper */ + /** @var ISystemTagObjectMapper|\PHPUnit_Framework_MockObject_MockObject */ private $tagMapper; - /** @var ISystemTagManager */ + /** @var ISystemTagManager|\PHPUnit_Framework_MockObject_MockObject */ private $tagManager; /** @var \OCP\IUserSession */ @@ -48,13 +48,13 @@ class FilesReportPlugin extends \Test\TestCase { /** @var FilesReportPluginImplementation */ private $plugin; - /** @var View **/ + /** @var View|\PHPUnit_Framework_MockObject_MockObject **/ private $view; - /** @var IGroupManager **/ + /** @var IGroupManager|\PHPUnit_Framework_MockObject_MockObject **/ private $groupManager; - /** @var Folder **/ + /** @var Folder|\PHPUnit_Framework_MockObject_MockObject **/ private $userFolder; public function setUp() { @@ -254,6 +254,7 @@ class FilesReportPlugin extends \Test\TestCase { ->with('222') ->will($this->returnValue([$filesNode2])); + /** @var \OCA\DAV\Connector\Sabre\Directory|\PHPUnit_Framework_MockObject_MockObject $reportTargetNode */ $result = $this->plugin->findNodesByFileIds($reportTargetNode, ['111', '222']); $this->assertCount(2, $result); @@ -304,6 +305,7 @@ class FilesReportPlugin extends \Test\TestCase { ->with('222') ->will($this->returnValue([$filesNode2])); + /** @var \OCA\DAV\Connector\Sabre\Directory|\PHPUnit_Framework_MockObject_MockObject $reportTargetNode */ $result = $this->plugin->findNodesByFileIds($reportTargetNode, ['111', '222']); $this->assertCount(2, $result); @@ -361,10 +363,14 @@ class FilesReportPlugin extends \Test\TestCase { ->method('isAdmin') ->will($this->returnValue(true)); - $this->tagMapper->expects($this->once()) + $this->tagMapper->expects($this->exactly(1)) ->method('getObjectIdsForTags') - ->with('123') - ->will($this->returnValue(['111', '222'])); + ->withConsecutive( + ['123', 'files'] + ) + ->willReturnMap([ + ['123', 'files', ['111', '222']], + ]); $rules = [ ['name' => '{http://owncloud.org/ns}systemtag', 'value' => '123'], @@ -378,14 +384,16 @@ class FilesReportPlugin extends \Test\TestCase { ->method('isAdmin') ->will($this->returnValue(true)); - $this->tagMapper->expects($this->at(0)) + $this->tagMapper->expects($this->exactly(2)) ->method('getObjectIdsForTags') - ->with('123') - ->will($this->returnValue(['111', '222'])); - $this->tagMapper->expects($this->at(1)) - ->method('getObjectIdsForTags') - ->with('456') - ->will($this->returnValue(['222', '333'])); + ->withConsecutive( + ['123', 'files'], + ['456', 'files'] + ) + ->willReturnMap([ + ['123', 'files', ['111', '222']], + ['456', 'files', ['222', '333']], + ]); $rules = [ ['name' => '{http://owncloud.org/ns}systemtag', 'value' => '123'], @@ -400,14 +408,16 @@ class FilesReportPlugin extends \Test\TestCase { ->method('isAdmin') ->will($this->returnValue(true)); - $this->tagMapper->expects($this->at(0)) + $this->tagMapper->expects($this->exactly(2)) ->method('getObjectIdsForTags') - ->with('123') - ->will($this->returnValue(['111', '222'])); - $this->tagMapper->expects($this->at(1)) - ->method('getObjectIdsForTags') - ->with('456') - ->will($this->returnValue([])); + ->withConsecutive( + ['123', 'files'], + ['456', 'files'] + ) + ->willReturnMap([ + ['123', 'files', ['111', '222']], + ['456', 'files', []], + ]); $rules = [ ['name' => '{http://owncloud.org/ns}systemtag', 'value' => '123'], @@ -417,6 +427,57 @@ class FilesReportPlugin extends \Test\TestCase { $this->assertEquals([], array_values($this->plugin->processFilterRules($rules))); } + public function testProcessFilterRulesAndConditionWithFirstEmptyResult() { + $this->groupManager->expects($this->any()) + ->method('isAdmin') + ->will($this->returnValue(true)); + + $this->tagMapper->expects($this->exactly(1)) + ->method('getObjectIdsForTags') + ->withConsecutive( + ['123', 'files'], + ['456', 'files'] + ) + ->willReturnMap([ + ['123', 'files', []], + ['456', 'files', ['111', '222']], + ]); + + $rules = [ + ['name' => '{http://owncloud.org/ns}systemtag', 'value' => '123'], + ['name' => '{http://owncloud.org/ns}systemtag', 'value' => '456'], + ]; + + $this->assertEquals([], array_values($this->plugin->processFilterRules($rules))); + } + + public function testProcessFilterRulesAndConditionWithEmptyMidResult() { + $this->groupManager->expects($this->any()) + ->method('isAdmin') + ->will($this->returnValue(true)); + + $this->tagMapper->expects($this->exactly(2)) + ->method('getObjectIdsForTags') + ->withConsecutive( + ['123', 'files'], + ['456', 'files'], + ['789', 'files'] + ) + ->willReturnMap([ + ['123', 'files', ['111', '222']], + ['456', 'files', ['333']], + ['789', 'files', ['111', '222']], + ]); + + $rules = [ + ['name' => '{http://owncloud.org/ns}systemtag', 'value' => '123'], + ['name' => '{http://owncloud.org/ns}systemtag', 'value' => '456'], + ['name' => '{http://owncloud.org/ns}systemtag', 'value' => '789'], + ]; + + $this->assertEquals([], array_values($this->plugin->processFilterRules($rules))); + } + public function testProcessFilterRulesInvisibleTagAsAdmin() { $this->groupManager->expects($this->any()) ->method('isAdmin') From e8d9c288bcc6ace78177426b7a9647d1e317d371 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Tue, 9 Feb 2016 12:07:30 +0100 Subject: [PATCH 3/3] Stop when a mid result is empty --- apps/dav/lib/connector/sabre/filesreportplugin.php | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/apps/dav/lib/connector/sabre/filesreportplugin.php b/apps/dav/lib/connector/sabre/filesreportplugin.php index d7e4f8beda..141b684360 100644 --- a/apps/dav/lib/connector/sabre/filesreportplugin.php +++ b/apps/dav/lib/connector/sabre/filesreportplugin.php @@ -211,7 +211,7 @@ class FilesReportPlugin extends ServerPlugin { */ public function processFilterRules($filterRules) { $ns = '{' . $this::NS_OWNCLOUD . '}'; - $resultFileIds = []; + $resultFileIds = null; $systemTagIds = []; foreach ($filterRules as $filterRule) { if ($filterRule['name'] === $ns . 'systemtag') { @@ -240,15 +240,21 @@ class FilesReportPlugin extends ServerPlugin { $fileIds = $this->tagMapper->getObjectIdsForTags($systemTagId, 'files'); if (empty($fileIds)) { + // This tag has no files, nothing can ever show up return []; } // first run ? - if (empty($resultFileIds)) { + if ($resultFileIds === null) { $resultFileIds = $fileIds; } else { $resultFileIds = array_intersect($resultFileIds, $fileIds); } + + if (empty($resultFileIds)) { + // Empty intersection, nothing can show up anymore + return []; + } } return $resultFileIds; }