Merge pull request #22234 from owncloud/systemtags-filter-intersect-empty

Fix system tag filter AND condition

2 files changed, 113 insertions, 19 deletions

diff --git a/apps/dav/lib/connector/sabre/filesreportplugin.php b/apps/dav/lib/connector/sabre/filesreportplugin.php
index 5bdd7a71ddc..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') {
@@ -239,11 +239,22 @@ class FilesReportPlugin extends ServerPlugin {
 		foreach ($systemTagIds as $systemTagId) {
 			$fileIds = $this->tagMapper->getObjectIdsForTags($systemTagId, 'files');
 
-			if (empty($resultFileIds)) {
+			if (empty($fileIds)) {
+				// This tag has no files, nothing can ever show up
+				return [];
+			}
+
+			// first run ?
+			if ($resultFileIds === null) {
 				$resultFileIds = $fileIds;
 			} else {
 				$resultFileIds = array_intersect($resultFileIds, $fileIds);
 			}
+
+			if (empty($resultFileIds)) {
+				// Empty intersection, nothing can show up anymore
+				return [];
+			}
 		}
 		return $resultFileIds;
 	}
diff --git a/apps/dav/tests/unit/connector/sabre/filesreportplugin.php b/apps/dav/tests/unit/connector/sabre/filesreportplugin.php
index 853e52f5039..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))
-			->method('getObjectIdsForTags')
-			->with('123')
-			->will($this->returnValue(['111', '222']));
-		$this->tagMapper->expects($this->at(1))
+		$this->tagMapper->expects($this->exactly(2))
 			->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'],
@@ -395,6 +403,81 @@ 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->exactly(2))
+			->method('getObjectIdsForTags')
+			->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')
+			->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')