From 0202a725846d5fc57784e13ac024cd5b1621d028 Mon Sep 17 00:00:00 2001 From: Git'Fellow <12234510+solracsf@users.noreply.github.com> Date: Sun, 29 Mar 2026 18:03:00 +0200 Subject: [PATCH] perf: improve DB queries Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com> --- lib/Db/AttachmentMapper.php | 28 ++ lib/Db/BoardMapper.php | 10 +- lib/Db/CardMapper.php | 30 +- lib/Db/LabelMapper.php | 24 ++ lib/Db/StackMapper.php | 37 ++ lib/Service/AttachmentService.php | 56 ++- lib/Service/BoardService.php | 45 +- lib/Service/CardService.php | 35 +- lib/Service/FilesAppService.php | 18 +- lib/Service/ICustomAttachmentService.php | 9 + lib/Service/LabelService.php | 19 +- lib/Service/StackService.php | 34 +- tests/unit/Service/AttachmentServiceTest.php | 2 +- .../Service/BatchQueryPerformanceTest.php | 385 ++++++++++++++++++ tests/unit/Service/StackServiceTest.php | 3 +- 15 files changed, 663 insertions(+), 72 deletions(-) create mode 100644 tests/unit/Service/BatchQueryPerformanceTest.php diff --git a/lib/Db/AttachmentMapper.php b/lib/Db/AttachmentMapper.php index 36d947c61a..37bebb1ded 100644 --- a/lib/Db/AttachmentMapper.php +++ b/lib/Db/AttachmentMapper.php @@ -64,6 +64,34 @@ public function findByData(int $cardId, string $data): Attachment { return $this->findEntity($qb); } + /** + * Returns a map of cardId => attachment count for the given card IDs in a single query. + * + * @param int[] $cardIds + * @return array + * @throws \OCP\DB\Exception + */ + public function findCountByCardIds(array $cardIds): array { + if (empty($cardIds)) { + return []; + } + $qb = $this->db->getQueryBuilder(); + $qb->select('card_id') + ->selectAlias($qb->func()->count('id'), 'attachment_count') + ->from($this->getTableName()) + ->where($qb->expr()->in('card_id', $qb->createNamedParameter($cardIds, IQueryBuilder::PARAM_INT_ARRAY))) + ->andWhere($qb->expr()->eq('deleted_at', $qb->createNamedParameter(0, IQueryBuilder::PARAM_INT))) + ->groupBy('card_id'); + + $counts = []; + $cursor = $qb->executeQuery(); + while ($row = $cursor->fetch()) { + $counts[(int)$row['card_id']] = (int)$row['attachment_count']; + } + $cursor->closeCursor(); + return $counts; + } + /** * @return Entity[] * @throws \OCP\DB\Exception diff --git a/lib/Db/BoardMapper.php b/lib/Db/BoardMapper.php index 3589668190..2583a117cd 100644 --- a/lib/Db/BoardMapper.php +++ b/lib/Db/BoardMapper.php @@ -163,12 +163,14 @@ public function findAllForUser(string $userId, ?int $since = null, bool $include return $board->getId(); }, $allBoards)); + // Pre-group ACLs by board ID + $aclsByBoard = []; + foreach ($acls as $acl) { + $aclsByBoard[$acl->getBoardId()][] = $acl; + } /* @var Board $entry */ foreach ($allBoards as $entry) { - $boardAcls = array_values(array_filter($acls, function ($acl) use ($entry) { - return $acl->getBoardId() === $entry->getId(); - })); - $entry->setAcl($boardAcls); + $entry->setAcl($aclsByBoard[$entry->getId()] ?? []); } foreach ($allBoards as $board) { diff --git a/lib/Db/CardMapper.php b/lib/Db/CardMapper.php index f47bea2d95..761c9cfad4 100644 --- a/lib/Db/CardMapper.php +++ b/lib/Db/CardMapper.php @@ -237,7 +237,7 @@ public function findCalendarEntries(int $boardId, ?int $limit = null, $offset = return $this->findEntities($qb); } - public function findAllArchived($stackId, $limit = null, $offset = null) { + public function findAllArchived(int $stackId, ?int $limit = null, ?int $offset = null): array { $qb = $this->db->getQueryBuilder(); $qb->select('*') ->from('deck_cards') @@ -250,7 +250,33 @@ public function findAllArchived($stackId, $limit = null, $offset = null) { return $this->findEntities($qb); } - public function findAllByStack($stackId, $limit = null, $offset = null) { + /** + * Batch-fetch all archived cards for multiple stacks in a single query. + * + * @param int[] $stackIds + * @return array Map of stackId => Card[] + * @throws \OCP\DB\Exception + */ + public function findAllArchivedForStacks(array $stackIds): array { + if (empty($stackIds)) { + return []; + } + $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(true, IQueryBuilder::PARAM_BOOL))) + ->andWhere($qb->expr()->eq('deleted_at', $qb->createNamedParameter(0, IQueryBuilder::PARAM_INT))) + ->orderBy('last_modified'); + + $cards = array_fill_keys($stackIds, []); + foreach ($this->findEntities($qb) as $card) { + $cards[$card->getStackId()][] = $card; + } + return $cards; + } + + public function findAllByStack(int $stackId, ?int $limit = null, ?int $offset = null): array { $qb = $this->db->getQueryBuilder(); $qb->select('*') ->from('deck_cards') diff --git a/lib/Db/LabelMapper.php b/lib/Db/LabelMapper.php index 717f6c4bba..8ec9cbe690 100644 --- a/lib/Db/LabelMapper.php +++ b/lib/Db/LabelMapper.php @@ -33,6 +33,30 @@ public function findAll(int $boardId, ?int $limit = null, int $offset = 0): arra return $this->findEntities($qb); } + /** + * Check if a label with the given title already exists on the board. + * Pass $excludeId to skip a specific label (used during updates). + * + * @throws \OCP\DB\Exception + */ + public function existsByBoardIdAndTitle(int $boardId, string $title, ?int $excludeId = null): bool { + $qb = $this->db->getQueryBuilder(); + $qb->select('id') + ->from($this->getTableName()) + ->where($qb->expr()->eq('board_id', $qb->createNamedParameter($boardId, IQueryBuilder::PARAM_INT))) + ->andWhere($qb->expr()->eq('title', $qb->createNamedParameter($title, IQueryBuilder::PARAM_STR))) + ->setMaxResults(1); + + if ($excludeId !== null) { + $qb->andWhere($qb->expr()->neq('id', $qb->createNamedParameter($excludeId, IQueryBuilder::PARAM_INT))); + } + + $cursor = $qb->executeQuery(); + $exists = $cursor->fetch() !== false; + $cursor->closeCursor(); + return $exists; + } + public function delete(Entity $entity): Entity { // delete assigned labels $this->deleteLabelAssignments($entity->getId()); diff --git a/lib/Db/StackMapper.php b/lib/Db/StackMapper.php index 1ce0d5fa72..302828e218 100644 --- a/lib/Db/StackMapper.php +++ b/lib/Db/StackMapper.php @@ -53,6 +53,43 @@ public function find(int $id): Stack { return $this->stackCache[(string)$id]; } + /** + * Fetch multiple stacks by their IDs in a single query. + * + * @param int[] $ids + * @return array Map of stackId => Stack + * @throws \OCP\DB\Exception + */ + public function findByIds(array $ids): array { + if (empty($ids)) { + return []; + } + + $stacks = []; + $uncachedIds = []; + foreach ($ids as $id) { + if (isset($this->stackCache[(string)$id])) { + $stacks[$id] = $this->stackCache[(string)$id]; + } else { + $uncachedIds[] = $id; + } + } + + if (!empty($uncachedIds)) { + $qb = $this->db->getQueryBuilder(); + $qb->select('*') + ->from($this->getTableName()) + ->where($qb->expr()->in('id', $qb->createNamedParameter($uncachedIds, IQueryBuilder::PARAM_INT_ARRAY))); + + foreach ($this->findEntities($qb) as $stack) { + $this->stackCache[(string)$stack->getId()] = $stack; + $stacks[$stack->getId()] = $stack; + } + } + + return $stacks; + } + /** * @throws \OCP\DB\Exception */ diff --git a/lib/Service/AttachmentService.php b/lib/Service/AttachmentService.php index b01bb0ce0e..e8db822aa6 100644 --- a/lib/Service/AttachmentService.php +++ b/lib/Service/AttachmentService.php @@ -137,21 +137,57 @@ public function findAll(int $cardId, bool $withDeleted = false): array { * @throws \OCP\DB\Exception */ public function count(int $cardId): int { - $count = $this->attachmentCacheHelper->getAttachmentCount($cardId); - if ($count === null) { - $count = count($this->attachmentMapper->findAll($cardId)); - - foreach (array_keys($this->services) as $attachmentType) { - $service = $this->getService($attachmentType); - if ($service instanceof ICustomAttachmentService) { - $count += $service->getAttachmentCount($cardId); + return ($this->countForCards([$cardId]))[$cardId] ?? 0; + } + + /** + * Returns a map of cardId => attachment count for the given card IDs using batch queries. + * + * @param int[] $cardIds + * @return array + * @throws \OCP\DB\Exception + */ + public function countForCards(array $cardIds): array { + if (empty($cardIds)) { + return []; + } + + $counts = []; + $uncachedIds = []; + foreach ($cardIds as $cardId) { + $cached = $this->attachmentCacheHelper->getAttachmentCount($cardId); + if ($cached !== null) { + $counts[$cardId] = $cached; + } else { + $uncachedIds[] = $cardId; + $counts[$cardId] = 0; + } + } + + if (empty($uncachedIds)) { + return $counts; + } + + // Batch query for deck_file attachments stored in the deck_attachment table + foreach ($this->attachmentMapper->findCountByCardIds($uncachedIds) as $cardId => $count) { + $counts[$cardId] = $count; + } + + // Add counts from custom attachment services (e.g. files shared via FilesAppService) + foreach (array_keys($this->services) as $attachmentType) { + $service = $this->getService($attachmentType); + if ($service instanceof ICustomAttachmentService) { + foreach ($service->getAttachmentCountForCards($uncachedIds) as $cardId => $count) { + $counts[$cardId] = ($counts[$cardId] ?? 0) + $count; } } + } - $this->attachmentCacheHelper->setAttachmentCount($cardId, $count); + foreach ($uncachedIds as $cardId) { + $this->attachmentCacheHelper->setAttachmentCount($cardId, $counts[$cardId]); } - return $count; + return $counts; } /** diff --git a/lib/Service/BoardService.php b/lib/Service/BoardService.php index faf7102bc1..e08b34b8c9 100644 --- a/lib/Service/BoardService.php +++ b/lib/Service/BoardService.php @@ -721,13 +721,14 @@ private function cloneCards(Board $board, Board $newBoard, bool $withAssignments usort($stacks, $stackSorter); usort($newStacks, $stackSorter); + $stackIds = array_map(fn (Stack $stack) => $stack->getId(), $stacks); + $activeCardsByStack = $this->cardMapper->findAllForStacks($stackIds); + $archivedCardsByStack = $this->cardMapper->findAllArchivedForStacks($stackIds); + $i = 0; foreach ($stacks as $stack) { - $cards = $this->cardMapper->findAll($stack->getId()); - $archivedCards = $this->cardMapper->findAllArchived($stack->getId()); - /** @var Card[] $cards */ - $cards = array_merge($cards, $archivedCards); + $cards = array_merge($activeCardsByStack[$stack->getId()] ?? [], $archivedCardsByStack[$stack->getId()] ?? []); foreach ($cards as $card) { $targetStackId = $moveCardsToLeftStack ? $newStacks[0]->getId() : $newStacks[$i]->getId(); @@ -830,22 +831,38 @@ private function clearBoardFromCache(Board $board): void { private function enrichWithCards(Board $board): void { $stacks = $this->stackMapper->findAll($board->getId()); + if (\count($stacks) === 0) { + return; + } + + $stackIds = array_map(fn (Stack $stack) => $stack->getId(), $stacks); + + // Fetch all active cards for all stacks in one query + $cardsByStack = $this->cardMapper->findAllForStacks($stackIds); + + $allCards = array_merge(...array_values(array_filter($cardsByStack))); + $allCardIds = array_map(fn (Card $card) => $card->getId(), $allCards); + + // Batch-fetch labels and assigned users for all cards + $labelsByCard = []; + foreach ($this->labelMapper->findAssignedLabelsForCards($allCardIds) as $label) { + $labelsByCard[$label->getCardId()][] = $label; + } + $usersByCard = []; + foreach ($this->assignedUsersMapper->findIn($allCardIds) as $assignment) { + $usersByCard[$assignment->getCardId()][] = $assignment; + } + foreach ($stacks as $stack) { - $cards = $this->cardMapper->findAllByStack($stack->getId()); $fullCards = []; - foreach ($cards as $card) { - $fullCard = $this->cardMapper->find($card->getId()); - $assignedUsers = $this->assignedUsersMapper->findAll($card->getId()); - $fullCard->setAssignedUsers($assignedUsers); - array_push($fullCards, $fullCard); + foreach ($cardsByStack[$stack->getId()] ?? [] as $card) { + $card->setLabels($labelsByCard[$card->getId()] ?? []); + $card->setAssignedUsers($usersByCard[$card->getId()] ?? []); + $fullCards[] = $card; } $stack->setCards($fullCards); } - if (\count($stacks) === 0) { - return; - } - $board->setStacks($stacks); } } diff --git a/lib/Service/CardService.php b/lib/Service/CardService.php index 4c7515d108..7e7d55b357 100644 --- a/lib/Service/CardService.php +++ b/lib/Service/CardService.php @@ -11,13 +11,11 @@ use OCA\Deck\Activity\ChangeSet; use OCA\Deck\BadRequestException; use OCA\Deck\Db\Acl; -use OCA\Deck\Db\Assignment; use OCA\Deck\Db\AssignmentMapper; use OCA\Deck\Db\BoardMapper; use OCA\Deck\Db\Card; use OCA\Deck\Db\CardMapper; use OCA\Deck\Db\ChangeHelper; -use OCA\Deck\Db\Label; use OCA\Deck\Db\LabelMapper; use OCA\Deck\Db\StackMapper; use OCA\Deck\Event\CardCreatedEvent; @@ -71,12 +69,19 @@ public function __construct( public function enrichCards(array $cards): array { $user = $this->userManager->get($this->userId); - $cardIds = array_map(function (Card $card) use ($user): int { + $allCardIds = array_map(fn (Card $card) => $card->getId(), $cards); + $attachmentCounts = $this->attachmentService->countForCards($allCardIds); + + // Pre-fetch all stacks for this batch in one query and index by ID + $stackIds = array_unique(array_map(fn (Card $card) => $card->getStackId(), $cards)); + $stacksById = $this->stackMapper->findByIds($stackIds); + + $cardIds = array_map(function (Card $card) use ($user, $attachmentCounts, $stacksById): int { // Everything done in here might be heavy as it is executed for every card $cardId = $card->getId(); $this->cardMapper->mapOwner($card); - $card->setAttachmentCount($this->attachmentService->count($cardId)); + $card->setAttachmentCount($attachmentCounts[$cardId] ?? 0); // TODO We should find a better way just to get the comment count so we can save 1-3 queries per card here $countComments = $this->commentsManager->getNumberOfCommentsForObject('deckCard', (string)$card->getId()); @@ -85,7 +90,7 @@ public function enrichCards(array $cards): array { $card->setCommentsUnread($countUnreadComments); $card->setCommentsCount($countComments); - $stack = $this->stackMapper->find($card->getStackId()); + $stack = $stacksById[$card->getStackId()] ?? $this->stackMapper->find($card->getStackId()); $board = $this->boardService->find($stack->getBoardId(), false); $card->setRelatedStack($stack); $card->setRelatedBoard($board); @@ -96,15 +101,19 @@ public function enrichCards(array $cards): array { $assignedLabels = $this->labelMapper->findAssignedLabelsForCards($cardIds); $assignedUsers = $this->assignedUsersMapper->findIn($cardIds); + // Pre-group labels and users by card ID + $labelsByCard = []; + foreach ($assignedLabels as $label) { + $labelsByCard[$label->getCardId()][] = $label; + } + $usersByCard = []; + foreach ($assignedUsers as $assignment) { + $usersByCard[$assignment->getCardId()][] = $assignment; + } + foreach ($cards as $card) { - $cardLabels = array_values(array_filter($assignedLabels, function (Label $label) use ($card) { - return $label->getCardId() === $card->getId(); - })); - $cardAssignedUsers = array_values(array_filter($assignedUsers, function (Assignment $assignment) use ($card) { - return $assignment->getCardId() === $card->getId(); - })); - $card->setLabels($cardLabels); - $card->setAssignedUsers($cardAssignedUsers); + $card->setLabels($labelsByCard[$card->getId()] ?? []); + $card->setAssignedUsers($usersByCard[$card->getId()] ?? []); } return array_map( diff --git a/lib/Service/FilesAppService.php b/lib/Service/FilesAppService.php index 931668d037..1ccf47d14c 100644 --- a/lib/Service/FilesAppService.php +++ b/lib/Service/FilesAppService.php @@ -18,6 +18,7 @@ use OCA\Deck\StatusException; use OCP\AppFramework\Http\StreamResponse; use OCP\Constants; +use OCP\DB\QueryBuilder\IQueryBuilder; use OCP\Files\File; use OCP\Files\Folder; use OCP\Files\IFilenameValidator; @@ -112,29 +113,36 @@ public function listAttachments(int $cardId): array { } public function getAttachmentCount(int $cardId): int { + return $this->getAttachmentCountForCards([$cardId])[$cardId] ?? 0; + } + + public function getAttachmentCountForCards(array $cardIds): array { + if (empty($cardIds)) { + return []; + } $qb = $this->connection->getQueryBuilder(); - $qb->select('s.id', 'f.fileid', 'f.path') + $qb->select('s.id', 's.share_with', 'f.fileid', 'f.path') ->selectAlias('st.id', 'storage_string_id') ->from('share', 's') ->leftJoin('s', 'filecache', 'f', $qb->expr()->eq('s.file_source', 'f.fileid')) ->leftJoin('f', 'storages', 'st', $qb->expr()->eq('f.storage', 'st.numeric_id')) ->andWhere($qb->expr()->eq('s.share_type', $qb->createNamedParameter(IShare::TYPE_DECK))) - ->andWhere($qb->expr()->eq('s.share_with', $qb->createNamedParameter($cardId))) + ->andWhere($qb->expr()->in('s.share_with', $qb->createNamedParameter(array_map('strval', $cardIds), IQueryBuilder::PARAM_STR_ARRAY))) ->andWhere($qb->expr()->isNull('s.parent')) ->andWhere($qb->expr()->orX( $qb->expr()->eq('s.item_type', $qb->createNamedParameter('file')), $qb->expr()->eq('s.item_type', $qb->createNamedParameter('folder')) )); - $count = 0; + $counts = array_fill_keys($cardIds, 0); $cursor = $qb->executeQuery(); while ($data = $cursor->fetch()) { if ($this->shareProvider->isAccessibleResult($data)) { - $count++; + $counts[(int)$data['share_with']]++; } } $cursor->closeCursor(); - return $count; + return $counts; } public function extendData(Attachment $attachment) { diff --git a/lib/Service/ICustomAttachmentService.php b/lib/Service/ICustomAttachmentService.php index fe6c4200a9..316018c960 100644 --- a/lib/Service/ICustomAttachmentService.php +++ b/lib/Service/ICustomAttachmentService.php @@ -20,4 +20,13 @@ interface ICustomAttachmentService { public function listAttachments(int $cardId): array; public function getAttachmentCount(int $cardId): int; + + /** + * Returns a map of cardId => attachment count for the given card IDs in a single query. + * Implementations should avoid issuing one query per card. + * + * @param int[] $cardIds + * @return array + */ + public function getAttachmentCountForCards(array $cardIds): array; } diff --git a/lib/Service/LabelService.php b/lib/Service/LabelService.php index 309100c748..625ef8a442 100644 --- a/lib/Service/LabelService.php +++ b/lib/Service/LabelService.php @@ -49,12 +49,8 @@ public function create(string $title, string $color, int $boardId): Label { $this->permissionService->checkPermission(null, $boardId, Acl::PERMISSION_MANAGE); - $boardLabels = $this->labelMapper->findAll($boardId); - foreach ($boardLabels as $boardLabel) { - if ($boardLabel->getTitle() === $title) { - throw new BadRequestException('title must be unique'); - break; - } + if ($this->labelMapper->existsByBoardIdAndTitle($boardId, $title)) { + throw new BadRequestException('title must be unique'); } if ($this->boardService->isArchived(null, $boardId)) { @@ -115,15 +111,8 @@ public function update(int $id, string $title, string $color): Label { $label = $this->find($id); - $boardLabels = $this->labelMapper->findAll($label->getBoardId()); - foreach ($boardLabels as $boardLabel) { - if ($boardLabel->getId() === $label->getId()) { - continue; - } - if ($boardLabel->getTitle() === $title) { - throw new BadRequestException('title must be unique'); - break; - } + if ($this->labelMapper->existsByBoardIdAndTitle($label->getBoardId(), $title, $label->getId())) { + throw new BadRequestException('title must be unique'); } if ($this->boardService->isArchived($this->labelMapper, $id)) { diff --git a/lib/Service/StackService.php b/lib/Service/StackService.php index 67202f7dbd..5ebab6a4e5 100644 --- a/lib/Service/StackService.php +++ b/lib/Service/StackService.php @@ -103,15 +103,20 @@ public function find(int $stackId): Stack { $this->permissionService->checkPermission($this->stackMapper, $stackId, Acl::PERMISSION_READ); $stack = $this->stackMapper->find($stackId); + $allCards = $this->cardMapper->findAll($stackId); + $cardIds = array_map(fn (Card $card) => $card->getId(), $allCards); + $attachmentCounts = $this->attachmentService->countForCards($cardIds); + $assignedUsers = $this->assignedUsersMapper->findIn($cardIds); + $cards = array_map( - function (Card $card): CardDetails { - $assignedUsers = $this->assignedUsersMapper->findAll($card->getId()); - $card->setAssignedUsers($assignedUsers); - $card->setAttachmentCount($this->attachmentService->count($card->getId())); + function (Card $card) use ($attachmentCounts, $assignedUsers): CardDetails { + $cardAssignedUsers = array_values(array_filter($assignedUsers, fn ($a) => $a->getCardId() === $card->getId())); + $card->setAssignedUsers($cardAssignedUsers); + $card->setAttachmentCount($attachmentCounts[$card->getId()] ?? 0); return new CardDetails($card); }, - $this->cardMapper->findAll($stackId) + $allCards ); $stack->setCards($cards); @@ -167,13 +172,28 @@ public function findAllArchived(int $boardId): array { $this->permissionService->checkPermission(null, $boardId, Acl::PERMISSION_READ); $stacks = $this->stackMapper->findAll($boardId); $labels = $this->labelMapper->getAssignedLabelsForBoard($boardId); + + $stackIds = array_map(fn (Stack $stack) => $stack->getId(), $stacks); + + // Fetch all archived cards for all stacks in a single query + $cardsByStackId = $this->cardMapper->findAllArchivedForStacks($stackIds); + + $allArchivedCardIds = []; + foreach ($cardsByStackId as $cards) { + foreach ($cards as $card) { + $allArchivedCardIds[] = $card->getId(); + } + } + + $attachmentCounts = $this->attachmentService->countForCards($allArchivedCardIds); + foreach ($stacks as $stackIndex => $stack) { - $cards = $this->cardMapper->findAllArchived($stack->id); + $cards = $cardsByStackId[$stack->getId()] ?? []; foreach ($cards as $cardIndex => $card) { if (array_key_exists($card->id, $labels)) { $cards[$cardIndex]->setLabels($labels[$card->id]); } - $cards[$cardIndex]->setAttachmentCount($this->attachmentService->count($card->getId())); + $cards[$cardIndex]->setAttachmentCount($attachmentCounts[$card->getId()] ?? 0); } $stacks[$stackIndex]->setCards($cards); } diff --git a/tests/unit/Service/AttachmentServiceTest.php b/tests/unit/Service/AttachmentServiceTest.php index 0a4b4e3445..638819073b 100644 --- a/tests/unit/Service/AttachmentServiceTest.php +++ b/tests/unit/Service/AttachmentServiceTest.php @@ -283,7 +283,7 @@ public function testFindAllWithDeleted() { public function testCount() { $this->attachmentCacheHelper->expects($this->once())->method('getAttachmentCount')->with(123)->willReturn(null); - $this->attachmentMapper->expects($this->once())->method('findAll')->willReturn([1,2,3,4]); + $this->attachmentMapper->expects($this->once())->method('findCountByCardIds')->with([123])->willReturn([123 => 4]); $this->attachmentCacheHelper->expects($this->once())->method('setAttachmentCount')->with(123, 4); $this->assertEquals(4, $this->attachmentService->count(123)); } diff --git a/tests/unit/Service/BatchQueryPerformanceTest.php b/tests/unit/Service/BatchQueryPerformanceTest.php new file mode 100644 index 0000000000..a15502ff6e --- /dev/null +++ b/tests/unit/Service/BatchQueryPerformanceTest.php @@ -0,0 +1,385 @@ +. + */ + +namespace OCA\Deck\Service; + +use OCA\Deck\Activity\ActivityManager; +use OCA\Deck\AppInfo\Application; +use OCA\Deck\Cache\AttachmentCacheHelper; +use OCA\Deck\Db\AssignmentMapper; +use OCA\Deck\Db\AttachmentMapper; +use OCA\Deck\Db\Board; +use OCA\Deck\Db\BoardMapper; +use OCA\Deck\Db\Card; +use OCA\Deck\Db\CardMapper; +use OCA\Deck\Db\ChangeHelper; +use OCA\Deck\Db\LabelMapper; +use OCA\Deck\Db\Stack; +use OCA\Deck\Db\StackMapper; +use OCA\Deck\Notification\NotificationHelper; +use OCA\Deck\Validators\AttachmentServiceValidator; +use OCA\Deck\Validators\CardServiceValidator; +use OCP\AppFramework\IAppContainer; +use OCP\Collaboration\Reference\IReferenceManager; +use OCP\Comments\ICommentsManager; +use OCP\EventDispatcher\IEventDispatcher; +use OCP\IL10N; +use OCP\IRequest; +use OCP\IURLGenerator; +use OCP\IUser; +use OCP\IUserManager; +use PHPUnit\Framework\MockObject\MockObject; +use Psr\Log\LoggerInterface; +use Test\TestCase; + +/** + * Verifies that the batch-query optimisations hold for the reference scenario + * described in the feature specification: + * + * 13 boards × 100 cards × 5 attachments = 1 300 cards / 6 500 attachments + * + * Each test asserts that a critical DB-layer method is called exactly once + * (batched), never once-per-card or once-per-board. + */ +class BatchQueryPerformanceTest extends TestCase { + private const BOARDS = 13; + private const CARDS_PER_BOARD = 100; + private const ATTACHMENTS_PER_CARD = 5; + /** 13 × 100 = 1 300 */ + private const TOTAL_CARDS = self::BOARDS * self::CARDS_PER_BOARD; + private AttachmentMapper|MockObject $attachmentMapper; + private AttachmentCacheHelper|MockObject $attachmentCacheHelper; + private FileService|MockObject $fileServiceImpl; + private FilesAppService|MockObject $filesAppServiceImpl; + private AttachmentService $attachmentService; + private CardMapper|MockObject $cardMapper; + private StackMapper|MockObject $stackMapper; + private LabelMapper|MockObject $labelMapper; + private AssignmentMapper|MockObject $assignedUsersMapper; + private AttachmentService|MockObject $attachmentServiceMock; + private BoardService|MockObject $boardService; + private IUserManager|MockObject $userManager; + private ICommentsManager|MockObject $commentsManager; + private IReferenceManager|MockObject $referenceManager; + private CardService $cardService; + + protected function setUp(): void { + parent::setUp(); + $this->setUpAttachmentService(); + $this->setUpCardService(); + } + + /** + * Cold-cache scenario: 13 boards × 100 cards = 1 300 card IDs. + * + */ + public function testCountForCardsIssuesSingleBatchQueryFor1300Cards(): void { + $cardIds = range(1, self::TOTAL_CARDS); + + // Cold cache: every card is uncached + $this->attachmentCacheHelper + ->expects($this->exactly(self::TOTAL_CARDS)) + ->method('getAttachmentCount') + ->willReturn(null); + + // deck_attachment table: queried ONCE for all 1 300 IDs + $this->attachmentMapper + ->expects($this->once()) + ->method('findCountByCardIds') + ->with($cardIds) + ->willReturn(array_fill_keys($cardIds, self::ATTACHMENTS_PER_CARD)); + + // FilesApp shares table: also queried ONCE for all 1 300 IDs + $this->filesAppServiceImpl + ->expects($this->once()) + ->method('getAttachmentCountForCards') + ->with($cardIds) + ->willReturn(array_fill_keys($cardIds, 0)); + + // Results must be written to cache (one entry per uncached card) + $this->attachmentCacheHelper + ->expects($this->exactly(self::TOTAL_CARDS)) + ->method('setAttachmentCount'); + + $counts = $this->attachmentService->countForCards($cardIds); + + $this->assertCount(self::TOTAL_CARDS, $counts); + foreach ($counts as $count) { + $this->assertSame(self::ATTACHMENTS_PER_CARD, $count); + } + } + + /** + * Warm-cache scenario: all 1 300 cards are already cached. + * + * No DB query of any kind must be issued. + */ + public function testCountForCardsSkipsCachedCardsAndIssuesNoDatabaseQuery(): void { + $cardIds = range(1, self::TOTAL_CARDS); + + $this->attachmentCacheHelper + ->expects($this->exactly(self::TOTAL_CARDS)) + ->method('getAttachmentCount') + ->willReturn(self::ATTACHMENTS_PER_CARD); + + $this->attachmentMapper->expects($this->never())->method('findCountByCardIds'); + $this->filesAppServiceImpl->expects($this->never())->method('getAttachmentCountForCards'); + $this->attachmentCacheHelper->expects($this->never())->method('setAttachmentCount'); + + $counts = $this->attachmentService->countForCards($cardIds); + + $this->assertCount(self::TOTAL_CARDS, $counts); + foreach ($counts as $count) { + $this->assertSame(self::ATTACHMENTS_PER_CARD, $count); + } + } + + /** + * Empty input must short-circuit without any DB or cache interaction. + */ + public function testCountForCardsWithEmptyInputReturnsEmptyWithoutAnyQuery(): void { + $this->attachmentMapper->expects($this->never())->method('findCountByCardIds'); + $this->filesAppServiceImpl->expects($this->never())->method('getAttachmentCountForCards'); + $this->attachmentCacheHelper->expects($this->never())->method('getAttachmentCount'); + $this->attachmentCacheHelper->expects($this->never())->method('setAttachmentCount'); + + $this->assertSame([], $this->attachmentService->countForCards([])); + } + + /** + * enrichCards() with 100 cards (one board's worth) must call + * AttachmentService::countForCards() exactly once, passing all 100 IDs + * in a single call — never once-per-card. + * + */ + public function testEnrichCardsCallsAttachmentCountAsSingleBatchQuery(): void { + $cards = $this->buildCards(self::CARDS_PER_BOARD, stackId: 1); + $cardIds = array_map(fn (Card $c) => $c->getId(), $cards); + + $this->setUpEnrichCardsStubs(stackId: 1); + + // THE ASSERTION: attachment counts must be fetched in one batch call + $this->attachmentServiceMock + ->expects($this->once()) + ->method('countForCards') + ->with($cardIds) + ->willReturn(array_fill_keys($cardIds, self::ATTACHMENTS_PER_CARD)); + + $result = $this->cardService->enrichCards($cards); + + $this->assertCount(self::CARDS_PER_BOARD, $result); + foreach ($result as $cardDetail) { + $this->assertSame(self::ATTACHMENTS_PER_CARD, $cardDetail->getAttachmentCount()); + } + } + + /** + * enrichCards() across all 13 boards (1 300 cards, 13 unique stacks) must + * call StackMapper::findByIds() exactly once, passing all 13 unique stack + * IDs — never once-per-card (which would produce 1 300 individual + * StackMapper::find() queries). + * + */ + public function testEnrichCardsCallsStackFetchAsSingleBatchQueryAcross13Boards(): void { + $cards = []; + $expectedStackIds = []; + $stackMap = []; + + for ($board = 1; $board <= self::BOARDS; $board++) { + $stackId = $board * 10; + $expectedStackIds[] = $stackId; + + $stack = new Stack(); + $stack->setId($stackId); + $stack->setBoardId($board); + $stackMap[$stackId] = $stack; + + foreach ($this->buildCards(self::CARDS_PER_BOARD, stackId: $stackId, idOffset: ($board - 1) * self::CARDS_PER_BOARD) as $card) { + $cards[] = $card; + } + } + sort($expectedStackIds); + + // Loose stubs for non-critical collaborators + $this->attachmentServiceMock->method('countForCards')->willReturn([]); + $this->labelMapper->method('findAssignedLabelsForCards')->willReturn([]); + $this->assignedUsersMapper->method('findIn')->willReturn([]); + $this->userManager->method('get')->willReturn($this->createMock(IUser::class)); + $this->commentsManager->method('getNumberOfCommentsForObject')->willReturn(0); + $this->commentsManager->method('getReadMark')->willReturn(null); + $this->boardService->method('find')->willReturn($this->createMock(Board::class)); + $this->referenceManager->method('extractReferences')->willReturn([]); + + // THE ASSERTION: all 13 stack IDs fetched in a single findByIds() call + $this->stackMapper + ->expects($this->once()) + ->method('findByIds') + ->with($this->callback(function (array $ids) use ($expectedStackIds): bool { + sort($ids); + return $ids === $expectedStackIds; + })) + ->willReturn($stackMap); + + $this->cardService->enrichCards($cards); + } + + /** + * enrichCards() with 100 cards must call LabelMapper::findAssignedLabelsForCards() + * and AssignmentMapper::findIn() each exactly once with all 100 card IDs. + * + */ + public function testEnrichCardsCallsLabelAndUserQueriesOnceForAllCards(): void { + $cards = $this->buildCards(self::CARDS_PER_BOARD, stackId: 1); + $cardIds = array_map(fn (Card $c) => $c->getId(), $cards); + + $this->setUpEnrichCardsStubs(stackId: 1); + $this->attachmentServiceMock->method('countForCards')->willReturn([]); + + // THE ASSERTIONS: label and user data must be fetched in one call each + $this->labelMapper + ->expects($this->once()) + ->method('findAssignedLabelsForCards') + ->with($cardIds) + ->willReturn([]); + + $this->assignedUsersMapper + ->expects($this->once()) + ->method('findIn') + ->with($cardIds) + ->willReturn([]); + + $result = $this->cardService->enrichCards($cards); + + $this->assertCount(self::CARDS_PER_BOARD, $result); + } + + /** + * Build an array of Card entities with sequential IDs. + * + * @param int $idOffset Added to each sequential ID so test sets are unique. + * @return Card[] + */ + private function buildCards(int $count, int $stackId, int $idOffset = 0): array { + $cards = []; + for ($i = 1; $i <= $count; $i++) { + $card = new Card(); + $card->setId($idOffset + $i); + $card->setStackId($stackId); + $card->setTitle('Card ' . ($idOffset + $i)); + $cards[] = $card; + } + return $cards; + } + + /** + * Configure loose stubs for all enrichCards() collaborators that are NOT + * under test in a given test method, so they don't interfere. + */ + private function setUpEnrichCardsStubs(int $stackId): void { + $stack = new Stack(); + $stack->setId($stackId); + $stack->setBoardId(1); + + $this->userManager->method('get')->willReturn($this->createMock(IUser::class)); + $this->commentsManager->method('getNumberOfCommentsForObject')->willReturn(0); + $this->commentsManager->method('getReadMark')->willReturn(null); + $this->stackMapper->method('findByIds')->willReturn([$stackId => $stack]); + $this->boardService->method('find')->willReturn($this->createMock(Board::class)); + $this->referenceManager->method('extractReferences')->willReturn([]); + // findAssignedLabelsForCards and findIn are stubbed here only when not the + // method under test. countForCards is intentionally omitted so that tests + // that assert on it can register their own expectation without a catch-all + // stub taking priority. + $this->labelMapper->method('findAssignedLabelsForCards')->willReturn([]); + $this->assignedUsersMapper->method('findIn')->willReturn([]); + } + + private function setUpAttachmentService(): void { + $this->attachmentMapper = $this->createMock(AttachmentMapper::class); + $this->attachmentCacheHelper = $this->createMock(AttachmentCacheHelper::class); + $this->fileServiceImpl = $this->createMock(FileService::class); + // FilesAppService implements ICustomAttachmentService, so mock the full class + $this->filesAppServiceImpl = $this->createMock(FilesAppService::class); + + $appContainer = $this->createMock(IAppContainer::class); + $appContainer->method('get') + ->willReturnMap([ + [FileService::class, $this->fileServiceImpl], + [FilesAppService::class, $this->filesAppServiceImpl], + ]); + + $application = $this->createMock(Application::class); + $application->method('getContainer')->willReturn($appContainer); + + $this->attachmentService = new AttachmentService( + $this->attachmentMapper, + $this->createMock(CardMapper::class), + $this->createMock(IUserManager::class), + $this->createMock(ChangeHelper::class), + $this->createMock(PermissionService::class), + $application, + $this->attachmentCacheHelper, + 'user1', + $this->createMock(IL10N::class), + $this->createMock(ActivityManager::class), + $this->createMock(AttachmentServiceValidator::class), + ); + } + + private function setUpCardService(): void { + $this->cardMapper = $this->createMock(CardMapper::class); + $this->stackMapper = $this->createMock(StackMapper::class); + $this->labelMapper = $this->createMock(LabelMapper::class); + $this->assignedUsersMapper = $this->createMock(AssignmentMapper::class); + $this->attachmentServiceMock = $this->createMock(AttachmentService::class); + $this->boardService = $this->createMock(BoardService::class); + $this->userManager = $this->createMock(IUserManager::class); + $this->commentsManager = $this->createMock(ICommentsManager::class); + $this->referenceManager = $this->createMock(IReferenceManager::class); + + $this->cardService = new CardService( + $this->cardMapper, + $this->stackMapper, + $this->createMock(BoardMapper::class), + $this->labelMapper, + $this->createMock(LabelService::class), + $this->createMock(PermissionService::class), + $this->boardService, + $this->createMock(NotificationHelper::class), + $this->assignedUsersMapper, + $this->attachmentServiceMock, + $this->createMock(ActivityManager::class), + $this->commentsManager, + $this->userManager, + $this->createMock(ChangeHelper::class), + $this->createMock(IEventDispatcher::class), + $this->createMock(IURLGenerator::class), + $this->createMock(LoggerInterface::class), + $this->createMock(IRequest::class), + $this->createMock(CardServiceValidator::class), + $this->createMock(AssignmentService::class), + $this->referenceManager, + 'user1', + ); + } +} diff --git a/tests/unit/Service/StackServiceTest.php b/tests/unit/Service/StackServiceTest.php index 9f7e01febf..4b7c6476eb 100644 --- a/tests/unit/Service/StackServiceTest.php +++ b/tests/unit/Service/StackServiceTest.php @@ -152,7 +152,8 @@ public function testFindAllArchived() { $this->permissionService->expects($this->once())->method('checkPermission'); $this->stackMapper->expects($this->once())->method('findAll')->willReturn($this->getStacks()); $this->labelMapper->expects($this->once())->method('getAssignedLabelsForBoard')->willReturn($this->getLabels()); - $this->cardMapper->expects($this->any())->method('findAllArchived')->willReturn($this->getCards(222)); + $this->cardMapper->expects($this->once())->method('findAllArchivedForStacks')->willReturn([222 => $this->getCards(222)]); + $this->attachmentService->method('countForCards')->willReturn([]); $actual = $this->stackService->findAllArchived(123); for ($stackId = 0; $stackId < 3; $stackId++) {