perf(cards): fetch all cards at once

Instead of one by one

Signed-off-by: Carl Schwan <carl.schwan@nextclound.com>

Author: Carl Schwan

lib/Db/CardMapper.php

@@ -131,7 +131,11 @@ public function find($id, bool $enhance = true): Card {
 		return $card;
 	}
 
-	public function findAll($stackId, $limit = null, $offset = null, $since = -1) {
+	/**
+	 * @return Card[]
+	 * @throws \OCP\DB\Exception
+	 */
+	public function findAll(int $stackId, ?int $limit = null, ?int $offset = null, int $since = -1) {
 		$qb = $this->db->getQueryBuilder();
 		$qb->select('*')
 			->from('deck_cards')
@@ -146,6 +150,32 @@ public function findAll($stackId, $limit = null, $offset = null, $since = -1) {
 		return $this->findEntities($qb);
 	}
 
+	/**
+	 * @param int[] $stackIds
+	 * @return array<int, null|Card[]>
+	 * @throws \OCP\DB\Exception
+	 */
+	public function findAllForStacks(array $stackIds, ?int $limit = null, ?int $offset = null, int $since = -1): array {
+		$qb = $this->db->getQueryBuilder();
+		$qb->select('*')
+			->from('deck_cards')
+			->where($qb->expr()->in('stack_id', $qb->createNamedParameter($stackIds, IQueryBuilder::PARAM_INT_ARRAY)))
+			->andWhere($qb->expr()->eq('archived', $qb->createNamedParameter(false, IQueryBuilder::PARAM_BOOL)))
+			->andWhere($qb->expr()->eq('deleted_at', $qb->createNamedParameter(0, IQueryBuilder::PARAM_INT)))
+			->andWhere($qb->expr()->gt('last_modified', $qb->createNamedParameter($since, IQueryBuilder::PARAM_INT)))
+			->setMaxResults($limit)
+			->setFirstResult($offset)
+			->orderBy('order')
+			->addOrderBy('id');
+
+		$rawCards = $this->findEntities($qb);
+		$cards = array_fill_keys($stackIds, null);
+		foreach ($rawCards as $card) {
+			$cards[$card->getId()][] = $card;
+		}
+		return $cards;
+	}
+
 	public function queryCardsByBoard(int $boardId): IQueryBuilder {
 		$qb = $this->db->getQueryBuilder();
 		$qb->select('c.*')

lib/Db/StackMapper.php

@@ -77,12 +77,10 @@ public function findStackFromCardId($cardId): ?Stack {
 
 	/**
 	 * @param numeric $boardId
-	 * @param int|null $limit
-	 * @param int|null $offset
 	 * @return Stack[]
 	 * @throws \OCP\DB\Exception
 	 */
-	public function findAll($boardId, $limit = null, $offset = null): array {
+	public function findAll($boardId, ?int $limit = null, ?int $offset = null): array {
 		$qb = $this->db->getQueryBuilder();
 		$qb->select('*')
 			->from($this->getTableName())

lib/Service/StackService.php

@@ -75,19 +75,21 @@ public function __construct(
 		$this->stackServiceValidator = $stackServiceValidator;
 	}
 
-	private function enrichStackWithCards($stack, $since = -1) {
-		$cards = $this->cardMapper->findAll($stack->getId(), null, null, $since);
+	/** @param Stack[] $stacks */
+	private function enrichStacksWithCards(array $stacks, $since = -1): void {
+		$cardsByStackId = $this->cardMapper->findAllForStacks(array_map(fn (Stack $stack) => $stack->getId(), $stacks), null, null, $since);
 
-		if (\count($cards) === 0) {
-			return;
-		}
-
-		$stack->setCards($this->cardService->enrichCards($cards));
-	}
+		foreach ($cardsByStackId as $stackId => $cards) {
+			if (!$cards) {
+				continue;
+			}
 
-	private function enrichStacksWithCards($stacks, $since = -1) {
-		foreach ($stacks as $stack) {
-			$this->enrichStackWithCards($stack, $since);
+			foreach ($stacks as $stack) {
+				if ($stack->getId() === $stackId) {
+					$stack->setCards($this->cardService->enrichCards($cards));
+					break;
+				}
+			}
 		}
 	}
 
@@ -247,7 +249,7 @@ public function delete($id) {
 		);
 		$this->changeHelper->boardChanged($stack->getBoardId());
 		$this->eventDispatcher->dispatchTyped(new BoardUpdatedEvent($stack->getBoardId()));
-		$this->enrichStackWithCards($stack);
+		$this->enrichStacksWithCards([$stack]);
 
 		return $stack;
 	}

tests/unit/Service/StackServiceTest.php

@@ -37,6 +37,7 @@
 use OCA\Deck\Model\CardDetails;
 use OCA\Deck\Validators\StackServiceValidator;
 use OCP\EventDispatcher\IEventDispatcher;
+use PHPUnit\Framework\MockObject\MockObject;
 use Psr\Log\LoggerInterface;
 use Test\TestCase;
 
@@ -129,12 +130,17 @@ function ($cards) {
 				}
 			)
 		);
-		$this->cardMapper->expects($this->any())->method('findAll')->willReturn($this->getCards(222));
-
+		$this->cardMapper->expects($this->any())->method('findAllForStacks')->willReturnCallback(function (array $stackIds) {
+			$r = [];
+			foreach ($stackIds as $stackId) {
+				$r[$stackId] = $this->getCards(222);
+			}
+			return $r;
+		});
 
 		$actual = $this->stackService->findAll(123);
 		for ($stackId = 0; $stackId < 3; $stackId++) {
-			for ($cardId = 0;$cardId < 10;$cardId++) {
+			for ($cardId = 0; $cardId < 10; $cardId++) {
 				$this->assertEquals($actual[0]->getCards()[$cardId]->getId(), $cardId);
 				$this->assertEquals($actual[0]->getCards()[$cardId]->getStackId(), 222);
 				$this->assertEquals($actual[0]->getCards()[$cardId]->getLabels(), $this->getLabels()[$cardId]);
@@ -211,7 +217,7 @@ public function testDelete() {
 		$stackToBeDeleted->setBoardId(1);
 		$this->stackMapper->expects($this->once())->method('find')->willReturn($stackToBeDeleted);
 		$this->stackMapper->expects($this->once())->method('update')->willReturn($stackToBeDeleted);
-		$this->cardMapper->expects($this->once())->method('findAll')->willReturn([]);
+		$this->cardMapper->expects($this->once())->method('findAllForStacks')->willReturn([]);
 		$this->stackService->delete(123);
 		$this->assertTrue($stackToBeDeleted->getDeletedAt() <= time(), 'deletedAt is in the past');
 		$this->assertTrue($stackToBeDeleted->getDeletedAt() > 0, 'deletedAt is set');