Merge pull request #3690 from nextcloud/bugfix/optimise_notifier_queries

Optimise queries when preparing card related notifications

lib/Db/Stack.php

@@ -25,6 +25,13 @@ namespace OCA\Deck\Db;
 
 use Sabre\VObject\Component\VCalendar;
 
+/**
+ * @method int getId()
+ * @method int getBoardId()
+ * @method int getDeletedAt()
+ * @method int getLastModified()
+ * @method int getOrder()
+ */
 class Stack extends RelationalEntity {
 	protected $title;
 	protected $boardId;

lib/Db/StackMapper.php

@@ -48,6 +48,25 @@ class StackMapper extends DeckMapper implements IPermissionMapper {
 		return $this->findEntity($sql, [$id]);
 	}
 
+	/**
+	 * @param $cardId
+	 * @return Stack|null
+	 */
+	public function findStackFromCardId($cardId): ?Stack {
+		$sql = <<<SQL
+SELECT s.* 
+FROM `*PREFIX*deck_stacks` as `s`
+INNER JOIN `*PREFIX*deck_cards` as `c` ON s.id = c.stack_id
+WHERE c.id = ?
+SQL;
+		try {
+			return $this->findEntity($sql, [$cardId]);
+		} catch (MultipleObjectsReturnedException|DoesNotExistException $e) {
+		}
+
+		return null;
+	}
+
 
 	public function findAll($boardId, $limit = null, $offset = null) {
 		$sql = 'SELECT * FROM `*PREFIX*deck_stacks` WHERE `board_id` = ? AND deleted_at = 0 ORDER BY `order`, `id`';

lib/Notification/Notifier.php

@@ -101,15 +101,12 @@ class Notifier implements INotifier {
 		switch ($notification->getSubject()) {
 			case 'card-assigned':
 				$cardId = $notification->getObjectId();
-				$boardId = $this->cardMapper->findBoardId($cardId);
+				$stack = $this->stackMapper->findStackFromCardId($cardId);
+				$boardId = $stack ? $stack->getBoardId() : null;
 				if (!$boardId) {
 					throw new AlreadyProcessedException();
 				}
 
-				$card = $this->cardMapper->find($cardId);
-				$stackId = $card->getStackId();
-				$stack = $this->stackMapper->find($stackId);
-
 				$initiator = $this->userManager->get($params[2]);
 				if ($initiator !== null) {
 					$dn = $initiator->getDisplayName();
@@ -147,15 +144,12 @@ class Notifier implements INotifier {
 				break;
 			case 'card-overdue':
 				$cardId = $notification->getObjectId();
-				$boardId = $this->cardMapper->findBoardId($cardId);
+				$stack = $this->stackMapper->findStackFromCardId($cardId);
+				$boardId = $stack ? $stack->getBoardId() : null;
 				if (!$boardId) {
 					throw new AlreadyProcessedException();
 				}
 
-				$card = $this->cardMapper->find($cardId);
-				$stackId = $card->getStackId();
-				$stack = $this->stackMapper->find($stackId);
-
 				$notification->setParsedSubject(
 					(string) $l->t('The card "%s" on "%s" has reached its due date.', $params)
 				);
@@ -182,15 +176,12 @@ class Notifier implements INotifier {
 				break;
 			case 'card-comment-mentioned':
 				$cardId = $notification->getObjectId();
-				$boardId = $this->cardMapper->findBoardId($cardId);
+				$stack = $this->stackMapper->findStackFromCardId($cardId);
+				$boardId = $stack ? $stack->getBoardId() : null;
 				if (!$boardId) {
 					throw new AlreadyProcessedException();
 				}
 
-				$card = $this->cardMapper->find($cardId);
-				$stackId = $card->getStackId();
-				$stack = $this->stackMapper->find($stackId);
-
 				$initiator = $this->userManager->get($params[2]);
 				if ($initiator !== null) {
 					$dn = $initiator->getDisplayName();

tests/unit/Notification/NotifierTest.php

@@ -25,6 +25,7 @@ namespace OCA\Deck\Notification;
 
 use OCA\Deck\Db\BoardMapper;
 use OCA\Deck\Db\CardMapper;
+use OCA\Deck\Db\Stack;
 use OCA\Deck\Db\StackMapper;
 use OCP\IL10N;
 use OCP\IURLGenerator;
@@ -103,9 +104,9 @@ class NotifierTest extends \Test\TestCase {
 		$notification->expects($this->once())
 			->method('getObjectId')
 			->willReturn('123');
-		$this->cardMapper->expects($this->once())
+		$this->stackMapper->expects($this->once())
-			->method('findBoardId')
+			->method('findStackFromCardId')
-			->willReturn(999);
+			->willReturn($this->buildMockStack());
 		$expectedMessage = 'The card "Card title" on "Board title" has reached its due date.';
 		$notification->expects($this->once())
 			->method('setParsedSubject')
@@ -146,9 +147,9 @@ class NotifierTest extends \Test\TestCase {
 		$notification->expects($this->once())
 			->method('getObjectId')
 			->willReturn('123');
-		$this->cardMapper->expects($this->once())
+		$this->stackMapper->expects($this->once())
-			->method('findBoardId')
+			->method('findStackFromCardId')
-			->willReturn(999);
+			->willReturn($this->buildMockStack());
 		$expectedMessage = 'admin has mentioned you in a comment on "Card title".';
 		$notification->expects($this->once())
 			->method('setParsedSubject')
@@ -183,9 +184,9 @@ class NotifierTest extends \Test\TestCase {
 
 	/** @dataProvider dataPrepareCardAssigned */
 	public function testPrepareCardAssigned($withUserFound = true) {
-		$this->cardMapper->expects($this->once())
+		$this->stackMapper->expects($this->once())
-			->method('findBoardId')
+			->method('findStackFromCardId')
-			->willReturn(123);
+			->willReturn($this->buildMockStack(123));
 
 		/** @var INotification $notification */
 		$notification = $this->createMock(INotification::class);
@@ -338,4 +339,17 @@ class NotifierTest extends \Test\TestCase {
 
 		$this->assertEquals($notification, $actualNotification);
 	}
+
+	/**
+	 * @param int $boardId
+	 * @return Stack|MockObject
+	 */
+	private function buildMockStack(int $boardId = 999) {
+		$mockStack = $this->getMockBuilder(Stack::class)
+			->addMethods(['getBoardId'])
+			->getMock();
+
+		$mockStack->method('getBoardId')->willReturn($boardId);
+		return $mockStack;
+	}
 }