From 3028684d8972c58a2146ebbdf1918fd3f730249e Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Tue, 9 Feb 2016 11:39:22 +0100 Subject: 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. --- apps/dav/lib/connector/sabre/filesreportplugin.php | 5 +++++ .../unit/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 5bdd7a71ddc..d7e4f8beda1 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 853e52f5039..54badd323f7 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') -- cgit v1.2.3 From 178914104c494a40b078638396bc9fdb511806ab Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Tue, 9 Feb 2016 11:47:33 +0100 Subject: Add a test for empty mid-result --- .../unit/connector/sabre/filesreportplugin.php | 107 ++++++++++++++++----- 1 file changed, 84 insertions(+), 23 deletions(-) diff --git a/apps/dav/tests/unit/connector/sabre/filesreportplugin.php b/apps/dav/tests/unit/connector/sabre/filesreportplugin.php index 54badd323f7..b528e2d2427 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,18 +408,71 @@ 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)) + ->withConsecutive( + ['123', 'files'], + ['456', 'files'] + ) + ->willReturnMap([ + ['123', 'files', ['111', '222']], + ['456', 'files', []], + ]); + + $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 testProcessFilterRulesAndConditionWithFirstEmptyResult() { + $this->groupManager->expects($this->any()) + ->method('isAdmin') + ->will($this->returnValue(true)); + + $this->tagMapper->expects($this->exactly(1)) ->method('getObjectIdsForTags') - ->with('456') - ->will($this->returnValue([])); + ->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))); -- cgit v1.2.3 From e8d9c288bcc6ace78177426b7a9647d1e317d371 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Tue, 9 Feb 2016 12:07:30 +0100 Subject: 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 d7e4f8beda1..141b684360e 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; } -- cgit v1.2.3