Merge pull request #27339 from owncloud/comm_optimize_propfind

Optimize PROPFIND having CommentsApp enabled by default

apps/dav/lib/Connector/Sabre/CommentPropertiesPlugin.php

@@ -2,6 +2,7 @@
 /**
  * @author Arthur Schiwon <blizzz@arthur-schiwon.de>
  * @author Thomas Müller <thomas.mueller@tmit.eu>
+ * @author Piotr Mrowczynski <piotr@owncloud.com>
  *
  * @copyright Copyright (c) 2017, ownCloud GmbH
  * @license AGPL-3.0
@@ -42,6 +43,11 @@ class CommentPropertiesPlugin extends ServerPlugin {
 	/** @var IUserSession */
 	private $userSession;
 
+	/**
+	 * @var int[]
+	 */
+	private $numberOfCommentsForNodes;
+
 	public function __construct(ICommentsManager $commentsManager, IUserSession $userSession) {
 		$this->commentsManager = $commentsManager;
 		$this->userSession = $userSession;
@@ -79,6 +85,47 @@ class CommentPropertiesPlugin extends ServerPlugin {
 			return;
 		}
 
+		// Prefetch required data if we know that it is parent node
+		if ($node instanceof \OCA\DAV\Connector\Sabre\Directory
+			&& $propFind->getDepth() !== 0
+			&& !is_null($propFind->getStatus(self::PROPERTY_NAME_UNREAD))) {
+			// Get ID of parent folder
+			$folderNodeID = $node->getId();
+			$nodeIdsArray = [$folderNodeID];
+			$this->numberOfCommentsForNodes[$folderNodeID] = 0;
+
+			// Get IDs for all children of the parent folder
+			$children = $node->getChildren();
+			foreach ($children as $childNode) {
+				if (!($childNode instanceof \OCA\DAV\Connector\Sabre\Directory) &&
+					!($childNode instanceof \OCA\DAV\Connector\Sabre\File)) {
+					return;
+				}
+				// Put node ID into an array
+				$nodeId = $childNode->getId();
+				array_push($nodeIdsArray, $nodeId);
+				$this->numberOfCommentsForNodes[$nodeId] = 0;
+			}
+
+			// Get user session
+			$user = $this->userSession->getUser();
+			if(!is_null($user)){
+				// Fetch all unread comments with their nodeIDs
+				$numberOfCommentsForNodes = $this->commentsManager->getNumberOfUnreadCommentsForNodes(
+					'files',
+					$nodeIdsArray,
+					$user);
+
+				if (!is_null($numberOfCommentsForNodes)){
+					// Map them to cached hash table
+					foreach($numberOfCommentsForNodes as $nodeID => $numberOfCommentsForNode) {
+						$this->numberOfCommentsForNodes[$nodeID] = $numberOfCommentsForNode;
+					}
+				}
+			}
+
+		}
+
 		$propFind->handle(self::PROPERTY_NAME_COUNT, function() use ($node) {
 			return $this->commentsManager->getNumberOfCommentsForObject('files', strval($node->getId()));
 		});
@@ -118,14 +165,25 @@ class CommentPropertiesPlugin extends ServerPlugin {
 	 * @return Int|null
 	 */
 	public function getUnreadCount(Node $node) {
+		// Get user session
 		$user = $this->userSession->getUser();
-		if(is_null($user)) {
-			return null;
+		$numberOfCommentsForNode = null;
+
+		// Check if it is cached
+		if (isset($this->numberOfCommentsForNodes[$node->getId()])) {
+			$numberOfCommentsForNode = $this->numberOfCommentsForNodes[$node->getId()];
+		} else if(!is_null($user)) {
+			// Fetch all unread comments for this specific NodeID
+			$numberOfCommentsForNodes = $this->commentsManager->getNumberOfUnreadCommentsForNodes(
+				'files',
+				[$node->getId()],
+				$user);
+
+			if (isset($numberOfCommentsForNodes[$node->getId()])) {
+				$numberOfCommentsForNode = $numberOfCommentsForNodes[$node->getId()];
+			}
 		}
-
-		$lastRead = $this->commentsManager->getReadMark('files', strval($node->getId()), $user);
-
-		return $this->commentsManager->getNumberOfCommentsForObject('files', strval($node->getId()), $lastRead);
+		
+		return $numberOfCommentsForNode;
 	}
-
 }

apps/dav/tests/unit/Connector/Sabre/CommentsPropertiesPluginTest.php

@@ -32,6 +32,7 @@ class CommentsPropertiesPluginTest extends \Test\TestCase {
 	protected $commentsManager;
 	protected $userSession;
 	protected $server;
+	protected $userFolder;
 
 	public function setUp() {
 		parent::setUp();
@@ -39,14 +40,168 @@ class CommentsPropertiesPluginTest extends \Test\TestCase {
 		$this->commentsManager = $this->createMock('\OCP\Comments\ICommentsManager');
 		$this->userSession = $this->createMock('\OCP\IUserSession');
 
+		$this->userFolder = $this->createMock('\OCP\Files\Folder');
+
 		$this->server = $this->getMockBuilder('\Sabre\DAV\Server')
 			->disableOriginalConstructor()
 			->getMock();
 
-		$this->plugin = new CommentPropertiesPluginImplementation($this->commentsManager, $this->userSession);
+		$this->plugin = new CommentPropertiesPluginImplementation($this->commentsManager, $this->userSession, $this->userFolder);
 		$this->plugin->initialize($this->server);
 	}
 
+	public function testHandleGetPropertiesUser() {
+		$sabreNode1 = $this->getMockBuilder('\OCA\DAV\Connector\Sabre\File')
+			->disableOriginalConstructor()
+			->getMock();
+		$sabreNode1->expects($this->any())
+			->method('getId')
+			->will($this->returnValue(111));
+		$sabreNode1->expects($this->never())
+			->method('getPath');
+		$sabreNode2 = $this->getMockBuilder('\OCA\DAV\Connector\Sabre\File')
+			->disableOriginalConstructor()
+			->getMock();
+		$sabreNode2->expects($this->any())
+			->method('getId')
+			->will($this->returnValue(222));
+		$sabreNode2->expects($this->never())
+			->method('getPath');
+
+		$sabreNode = $this->getMockBuilder('\OCA\DAV\Connector\Sabre\Directory')
+			->disableOriginalConstructor()
+			->getMock();
+		$sabreNode->expects($this->any())
+			->method('getId')
+			->will($this->returnValue(123));
+		$sabreNode->expects($this->once())
+			->method('getChildren')
+		->will($this->returnValue([$sabreNode1, $sabreNode2]));
+		$sabreNode->expects($this->any())
+			->method('getPath')
+			->will($this->returnValue('/subdir'));
+
+		// simulate sabre recursive PROPFIND traversal
+		$propFindRoot = new \Sabre\DAV\PropFind(
+			'/subdir',
+			[CommentPropertiesPluginImplementation::PROPERTY_NAME_UNREAD],
+			1
+		);
+
+		$propFind1 = new \Sabre\DAV\PropFind(
+			'/subdir/test.txt',
+			[CommentPropertiesPluginImplementation::PROPERTY_NAME_UNREAD],
+			0
+		);
+		$propFind2 = new \Sabre\DAV\PropFind(
+			'/subdir/test2.txt',
+			[CommentPropertiesPluginImplementation::PROPERTY_NAME_UNREAD],
+			0
+		);
+
+		// Add some comments there
+		$numberOfCommentsForNodes[$sabreNode1->getId()] = 1;
+		$numberOfCommentsForNodes[$sabreNode2->getId()] = 4;
+		$this->commentsManager->expects($this->once())
+			->method('getNumberOfUnreadCommentsForNodes')
+			->willReturn($numberOfCommentsForNodes);
+
+		// Now test with user
+		$this->userSession->expects($this->any())
+			->method('getUser')
+			->willReturn($this->createMock('\OCP\IUser'));
+
+		$this->plugin->handleGetProperties(
+			$propFindRoot,
+			$sabreNode
+		);
+		$this->plugin->handleGetProperties(
+			$propFind1,
+			$sabreNode1
+		);
+		$this->plugin->handleGetProperties(
+			$propFind2,
+			$sabreNode2
+		);
+
+		$result = $propFindRoot->getResultForMultiStatus();
+		$result1 = $propFind1->getResultForMultiStatus();
+		$result2 = $propFind2->getResultForMultiStatus();
+		$this->assertEmpty($result[404]);
+		$this->assertEmpty($result1[404]);
+		$this->assertEmpty($result2[404]);
+		$this->assertEquals(0, $result[200][CommentPropertiesPluginImplementation::PROPERTY_NAME_UNREAD]);
+		$this->assertEquals(1, $result1[200][CommentPropertiesPluginImplementation::PROPERTY_NAME_UNREAD]);
+		$this->assertEquals(4, $result2[200][CommentPropertiesPluginImplementation::PROPERTY_NAME_UNREAD]);
+	}
+
+	public function testHandleGetPropertiesUserLoggedOut() {
+		$sabreNode1 = $this->getMockBuilder('\OCA\DAV\Connector\Sabre\File')
+			->disableOriginalConstructor()
+			->getMock();
+		$sabreNode1->expects($this->any())
+			->method('getId')
+			->will($this->returnValue(111));
+		$sabreNode1->expects($this->never())
+			->method('getPath');
+		$sabreNode2 = $this->getMockBuilder('\OCA\DAV\Connector\Sabre\File')
+			->disableOriginalConstructor()
+			->getMock();
+		$sabreNode2->expects($this->any())
+			->method('getId')
+			->will($this->returnValue(222));
+		$sabreNode2->expects($this->never())
+			->method('getPath');
+
+		$sabreNode = $this->getMockBuilder('\OCA\DAV\Connector\Sabre\Directory')
+			->disableOriginalConstructor()
+			->getMock();
+		$sabreNode->expects($this->any())
+			->method('getId')
+			->will($this->returnValue(123));
+		$sabreNode->expects($this->once())
+			->method('getChildren')
+			->will($this->returnValue([$sabreNode1, $sabreNode2]));
+		$sabreNode->expects($this->any())
+			->method('getPath')
+			->will($this->returnValue('/subdir'));
+		// simulate sabre recursive PROPFIND traversal
+		$propFindRoot = new \Sabre\DAV\PropFind(
+			'/subdir',
+			[CommentPropertiesPluginImplementation::PROPERTY_NAME_UNREAD],
+			1
+		);
+
+		$propFind1 = new \Sabre\DAV\PropFind(
+			'/subdir/test.txt',
+			[CommentPropertiesPluginImplementation::PROPERTY_NAME_UNREAD],
+			0
+		);
+		$propFind2 = new \Sabre\DAV\PropFind(
+			'/subdir/test2.txt',
+			[CommentPropertiesPluginImplementation::PROPERTY_NAME_UNREAD],
+			0
+		);
+
+		$this->plugin->handleGetProperties(
+			$propFindRoot,
+			$sabreNode
+		);
+		$this->plugin->handleGetProperties(
+			$propFind1,
+			$sabreNode1
+		);
+		$this->plugin->handleGetProperties(
+			$propFind2,
+			$sabreNode2
+		);
+
+		$result = $propFindRoot->getResultForMultiStatus();
+		$this->assertEmpty($result[404]);
+		$this->assertEquals(0, $result[200][CommentPropertiesPluginImplementation::PROPERTY_NAME_UNREAD]);
+	}
+
+
 	public function nodeProvider() {
 		$mocks = [];
 		foreach(['\OCA\DAV\Connector\Sabre\File', '\OCA\DAV\Connector\Sabre\Directory', '\Sabre\DAV\INode'] as $class) {
@@ -67,7 +222,7 @@ class CommentsPropertiesPluginTest extends \Test\TestCase {
 	 * @param $node
 	 * @param $expectedSuccessful
 	 */
-	public function testHandleGetProperties($node, $expectedSuccessful) {
+	public function testHandleGetPropertiesCorrectNode($node, $expectedSuccessful) {
 		$propFind = $this->getMockBuilder('\Sabre\DAV\PropFind')
 			->disableOriginalConstructor()
 			->getMock();
@@ -136,9 +291,10 @@ class CommentsPropertiesPluginTest extends \Test\TestCase {
 			->method('getUser')
 			->will($this->returnValue($user));
 
+		$numberOfCommentsForNodes[$node->getId()] = 42;
 		$this->commentsManager->expects($this->any())
-			->method('getNumberOfCommentsForObject')
-			->will($this->returnValue(42));
+			->method('getNumberOfUnreadCommentsForNodes')
+			->willReturn($numberOfCommentsForNodes);
 
 		$unread = $this->plugin->getUnreadCount($node);
 		if(is_null($user)) {

lib/private/Comments/Manager.php

@@ -368,6 +368,57 @@ class Manager implements ICommentsManager {
 		return $comments;
 	}
 
+	/**
+	 * Returns number of unread messages for specified nodeIDs, if there are any unread comments. For more details refer to interface description
+	 *
+	 * @param string $objectType string the object type
+	 * @param int[] $objectIds NodeIDs that may be returned
+	 * @param IUser $user
+	 * @return int[] $unreadCountsForNodes hash table
+	 * @since 10.0.0
+	 */
+	public function getNumberOfUnreadCommentsForNodes($objectType, $objectIds, IUser $user) {
+		$qbMain = $this->dbConn->getQueryBuilder();
+		$qbSup = $this->dbConn->getQueryBuilder();
+		
+		$unreadCountsForNodes = array();
+		$objectIdChunks = array_chunk($objectIds, 100);
+		foreach ($objectIdChunks as $objectIdChunk) {
+			// Fetch only records from oc_comments which are in specified int[] NodeIDs array and satisfy specified $objectType
+			$qbMain->selectAlias('object_id', 'id')->selectAlias($qbMain->createFunction('COUNT(`object_id`)'), 'count')
+				->from('comments', 'c')
+				->where($qbMain->expr()->eq('object_type', $qbMain->createParameter('type')))
+				->andWhere($qbMain->expr()->in('object_id', $qbMain->createParameter('object_ids')))
+				->setParameter('type', $objectType)
+				->setParameter('object_ids', $objectIdChunk, IQueryBuilder::PARAM_INT_ARRAY);
+
+			// For those found object_id, find all records from oc_comments which are not existing in oc_comments_read_markers or
+			// if matched, its timestamp is lower then the one in oc_comments_read_markers
+			// This query will find all unread comments for user oc_comments_read_markers.user_id $user
+			$qbSup->select('object_id')
+				->from('comments_read_markers', 'crm')
+				->where($qbMain->expr()->eq('crm.user_id', $qbMain->createParameter('crm_user_id')))
+				->andWhere($qbMain->expr()->gte('crm.marker_datetime', 'c.creation_timestamp'))
+				->andWhere($qbMain->expr()->eq('c.object_id', 'crm.object_id'));
+			$qbMain->setParameter('crm_user_id', $user->getUID(), IQueryBuilder::PARAM_STR);
+
+			// Add Inner Select into the main query in NOT IN() clause
+			$qbMain->andWhere($qbMain->expr()->notIn('object_id', $qbMain->createFunction($qbSup->getSQL())));
+
+			// We need groupby for count function
+			$qbMain->groupBy('object_id');
+
+			$cursor = $qbMain->execute();
+
+			while ($data = $cursor->fetch()) {
+				$unreadCountsForNodes[$data['id']] = intval($data['count']);
+			}
+			$cursor->closeCursor();
+		}
+
+		return $unreadCountsForNodes;
+	}
+
 	/**
 	 * @param $objectType string the object type, e.g. 'files'
 	 * @param $objectId string the id of the object

lib/public/Comments/ICommentsManager.php

@@ -21,7 +21,7 @@
  *
  */
 namespace OCP\Comments;
-
+use \OCP\IUser;
 /**
  * Interface ICommentsManager
  *
@@ -114,6 +114,27 @@ interface ICommentsManager {
 			\DateTime $notOlderThan = null
 	);
 
+	/**
+	 * Returns number of unread messages for specified nodeIDs, if there are any unread comments
+	 * 
+	 * Example query:
+	 * SELECT object_id, COUNT(object_id) FROM oc_comments C
+	 * 	WHERE object_id IN('79', '80', '34', '36', '38', '33')
+	 * 	AND object_id NOT IN(
+	 * 		SELECT object_id FROM oc_comments_read_markers CRM
+	 * 			WHERE C.object_id = CRM.object_id
+	 * 			AND user_id = 'receiver2'
+	 * 			AND marker_datetime > C.creation_timestamp)
+	 * GROUP BY object_id;
+	 *
+	 * @param string $objectType string the object type: Example 'files'
+	 * @param int[] $objectIds NodeIDs that may be returned: Example { 44, 36, 50, 60 }[4]
+	 * @param IUser $user
+	 * @return int[] $unreadCountsForNodes hash table: Example { 44 => 2, 36 => 1 }[2]
+	 * @since 10.0.0
+	 */
+	public function getNumberOfUnreadCommentsForNodes($objectType, $objectIds, IUser $user);
+
 	/**
 	 * @param $objectType string the object type, e.g. 'files'
 	 * @param $objectId string the id of the object

tests/lib/Comments/FakeManager.php

@@ -38,4 +38,6 @@ class FakeManager implements \OCP\Comments\ICommentsManager {
 	public function deleteReadMarksFromUser(\OCP\IUser $user) {}
 
 	public function deleteReadMarksOnObject($objectType, $objectId) {}
+
+	public function getNumberOfUnreadCommentsForNodes($objectType, $objectIds, \OCP\IUser $user) {}
 }

tests/lib/Comments/ManagerTest.php

@@ -12,14 +12,24 @@ use Test\TestCase;
  */
 class ManagerTest extends TestCase {
 
+	private $dbConn;
+
 	public function setUp() {
 		parent::setUp();
 
-		$sql = \OC::$server->getDatabaseConnection()->getDatabasePlatform()->getTruncateTableSQL('`*PREFIX*comments`');
-		\OC::$server->getDatabaseConnection()->prepare($sql)->execute();
+		$this->dbConn = \OC::$server->getDatabaseConnection();
+		$sql = $this->dbConn->getDatabasePlatform()->getTruncateTableSQL('`*PREFIX*comments`');
+		$this->dbConn->prepare($sql)->execute();
+		$sql = $this->dbConn->getDatabasePlatform()->getTruncateTableSQL('`*PREFIX*comments_read_markers`');
+		$this->dbConn->prepare($sql)->execute();
+	}
+
+	public function tearDown() {
+		$this->dbConn->getQueryBuilder()->delete('comments')->execute();
+		$this->dbConn->getQueryBuilder()->delete('comments_read_markers')->execute();
 	}
 
-	protected function addDatabaseEntry($parentId, $topmostParentId, $creationDT = null, $latestChildDT = null) {
+	protected function addDatabaseEntry($parentId, $topmostParentId, $creationDT = null, $latestChildDT = null, $actor_id = 'alice', $object_id = 'file64') {
 		if(is_null($creationDT)) {
 			$creationDT = new \DateTime();
 		}
@@ -27,7 +37,7 @@ class ManagerTest extends TestCase {
 			$latestChildDT = new \DateTime('yesterday');
 		}
 
-		$qb = \OC::$server->getDatabaseConnection()->getQueryBuilder();
+		$qb = $this->dbConn->getQueryBuilder();
 		$qb
 			->insert('comments')
 			->values([
@@ -35,13 +45,13 @@ class ManagerTest extends TestCase {
 					'topmost_parent_id' 		=> $qb->createNamedParameter($topmostParentId),
 					'children_count' 			=> $qb->createNamedParameter(2),
 					'actor_type' 				=> $qb->createNamedParameter('users'),
-					'actor_id' 					=> $qb->createNamedParameter('alice'),
+					'actor_id' 					=> $qb->createNamedParameter($actor_id),
 					'message' 					=> $qb->createNamedParameter('nice one'),
 					'verb' 						=> $qb->createNamedParameter('comment'),
 					'creation_timestamp' 		=> $qb->createNamedParameter($creationDT, 'datetime'),
 					'latest_child_timestamp'	=> $qb->createNamedParameter($latestChildDT, 'datetime'),
 					'object_type' 				=> $qb->createNamedParameter('files'),
-					'object_id' 				=> $qb->createNamedParameter('file64'),
+					'object_id' 				=> $qb->createNamedParameter($object_id),
 			])
 			->execute();
 
@@ -293,6 +303,65 @@ class ManagerTest extends TestCase {
 		$this->assertSame($amount, 4);
 	}
 
+	public function testGetNumberOfUnreadCommentsForNodes() {
+		$manager = $this->getManager();
+		$user1 = $this->createMock(\OCP\IUser::class);
+		$user1->expects($this->any())
+			->method('getUID')
+			->willReturn('piotr');
+		$user2 = $this->createMock(\OCP\IUser::class);
+		$user2->expects($this->any())
+			->method('getUID')
+			->willReturn('karolina');
+		$user3 = $this->createMock(\OCP\IUser::class);
+		$user3->expects($this->any())
+			->method('getUID')
+			->willReturn('artur');
+
+		// Add comments and karolina never read any comment from piotr
+		$commentsTimeStamp = new \DateTime('2017-03-01 15:00:00 EDT');
+		for ($i = 0; $i< 200; $i++) {
+			$this->addDatabaseEntry(0, 0, $commentsTimeStamp, $commentsTimeStamp, 'piotr', '36');
+		}
+		$this->addDatabaseEntry(0, 0, $commentsTimeStamp, $commentsTimeStamp, 'piotr', '40');
+		$this->addDatabaseEntry(0, 0, $commentsTimeStamp, $commentsTimeStamp, 'piotr', '40');
+		$this->addDatabaseEntry(0, 0, $commentsTimeStamp, $commentsTimeStamp, 'piotr', '20');
+		$this->addDatabaseEntry(0, 0, $commentsTimeStamp, $commentsTimeStamp, 'artur', '21');
+		$this->addDatabaseEntry(0, 0, $commentsTimeStamp, $commentsTimeStamp, 'artur', '15');
+		$manager->setReadMark('files', '36', $commentsTimeStamp, $user1);
+		$manager->setReadMark('files', '40', $commentsTimeStamp, $user1);
+		$manager->setReadMark('files', '20', $commentsTimeStamp, $user1);
+		$manager->setReadMark('files', '21', $commentsTimeStamp, $user3);
+		$manager->setReadMark('files', '15', $commentsTimeStamp, $user3);
+
+		$expectedHashMap = array();
+		$amount = $manager->getNumberOfUnreadCommentsForNodes('files', ['36','40','20','105'], $user1);
+		$this->assertSame($amount, $expectedHashMap);
+
+		$expectedHashMap = array();
+		$expectedHashMap['36'] = 200;
+		$expectedHashMap['40'] = 2;
+		$amount = $manager->getNumberOfUnreadCommentsForNodes('files', ['36','40', '80','25'], $user2);
+		$this->assertSame($amount, $expectedHashMap);
+
+		// Karolina now read the comments from piotr day later
+		$commentsTimeStamp1 = new \DateTime('2017-03-02 15:00:00 EDT');
+		$manager->setReadMark('files', '36', $commentsTimeStamp1, $user2);
+		$manager->setReadMark('files', '40', $commentsTimeStamp1, $user2);
+		$expectedHashMap = array();
+		$amount = $manager->getNumberOfUnreadCommentsForNodes('files', ['36','40','80','25'], $user2);
+		$this->assertSame($amount, $expectedHashMap);
+
+		// Karolina added another comment to piotr after that, so piotr will have one unread comment
+		$commentsTimeStamp2 = new \DateTime('2017-03-02 15:00:01 EDT');
+		$this->addDatabaseEntry(0, 0, $commentsTimeStamp2, $commentsTimeStamp2, 'karolina', '36');
+		$manager->setReadMark('files', '36', $commentsTimeStamp2, $user2);
+		$expectedHashMap = array();
+		$expectedHashMap['36'] = 1;
+		$amount = $manager->getNumberOfUnreadCommentsForNodes('files', ['36','40','20','105'], $user1);
+		$this->assertSame($amount, $expectedHashMap);
+	}
+
 	public function invalidCreateArgsProvider() {
 		return [
 			['', 'aId-1', 'oType-1', 'oId-1'],