diff --git a/.github/workflows/integration.yml b/.github/workflows/integration.yml index 154c6e497..8516631f4 100644 --- a/.github/workflows/integration.yml +++ b/.github/workflows/integration.yml @@ -74,11 +74,12 @@ jobs: uses: shivammathur/setup-php@2.24.0 with: php-version: ${{ matrix.php-versions }} - tools: phpunit - extensions: mbstring, iconv, fileinfo, intl, sqlite, pdo_sqlite, mysql, pdo_mysql, pgsql, pdo_pgsql, + extensions: mbstring, iconv, fileinfo, intl, sqlite, pdo_sqlite, mysql, pdo_mysql, pgsql, pdo_pgsql, apcu + ini-values: + apc.enable_cli=on coverage: none - - name: Set up PHPUnit + - name: Set up dependencies working-directory: apps/${{ env.APP_NAME }} run: composer i --no-dev @@ -91,11 +92,63 @@ jobs: fi mkdir data ./occ maintenance:install --verbose --database=${{ matrix.databases }} --database-name=nextcloud --database-host=127.0.0.1 --database-port=$DB_PORT --database-user=root --database-pass=rootpassword --admin-user admin --admin-pass admin + ./occ config:system:set hashing_default_password --value=true --type=boolean + ./occ config:system:set memcache.local --value="\\OC\\Memcache\\APCu" + ./occ config:system:set memcache.distributed --value="\\OC\\Memcache\\APCu" cat config/config.php ./occ user:list ./occ app:enable --force ${{ env.APP_NAME }} + ./occ config:system:set query_log_file --value '/home/runner/work/${{ env.APP_NAME }}/${{ env.APP_NAME }}/query.log' php -S localhost:8080 & - name: Run behat working-directory: apps/${{ env.APP_NAME }}/tests/integration run: ./run.sh + + - name: Query count + if: ${{ matrix.databases == 'mysql' }} + uses: actions/github-script@v5 + with: + github-token: ${{secrets.GITHUB_TOKEN}} + script: | + let myOutput = '' + let myError = '' + + const options = {} + options.listeners = { + stdout: (data) => { + myOutput += data.toString() + }, + stderr: (data) => { + myError += data.toString() + } + } + await exec.exec(`/bin/bash -c "cat /home/runner/work/${{ env.APP_NAME }}/${{ env.APP_NAME }}/query.log | wc -l"`, [], options) + msg = myOutput + const queryCount = parseInt(myOutput, 10) + + myOutput = '' + await exec.exec('cat', ['/home/runner/work/${{ env.APP_NAME }}/${{ env.APP_NAME }}/apps/${{ env.APP_NAME }}/tests/integration/base-query-count.txt'], options) + const baseCount = parseInt(myOutput, 10) + + const absoluteIncrease = queryCount - baseCount + const relativeIncrease = baseCount <= 0 ? 100 : (parseInt((absoluteIncrease / baseCount * 10000), 10) / 100) + + if (absoluteIncrease >= 100 || relativeIncrease > 5) { + const comment = `🐢 Performance warning.\nIt looks like the query count of the integration tests increased with this PR.\nDatabase query count is now ` + queryCount + ' was ' + baseCount + ' (+' + relativeIncrease + '%)\nPlease check your code again. If you added a new test this can be expected and the base value in tests/integration/base-query-count.txt can be increased.' + github.rest.issues.createComment({ + issue_number: context.issue.number, + owner: context.repo.owner, + repo: context.repo.repo, + body: comment + }) + } + if (queryCount < 100) { + const comment = `🐈 Performance messuring seems broken. Failed to get query count.` + github.rest.issues.createComment({ + issue_number: context.issue.number, + owner: context.repo.owner, + repo: context.repo.repo, + body: comment + }) + } \ No newline at end of file diff --git a/lib/Controller/BoardApiController.php b/lib/Controller/BoardApiController.php index cc974f083..a128b5c64 100644 --- a/lib/Controller/BoardApiController.php +++ b/lib/Controller/BoardApiController.php @@ -65,7 +65,7 @@ class BoardApiController extends ApiController { public function index($details = null) { $modified = $this->request->getHeader('If-Modified-Since'); if ($modified === null || $modified === '') { - $boards = $this->boardService->findAll(0, $details); + $boards = $this->boardService->findAll(0, $details === true); } else { $date = Util::parseHTTPDate($modified); if (!$date) { diff --git a/lib/DAV/DeckCalendarBackend.php b/lib/DAV/DeckCalendarBackend.php index a1849abe9..cbb602c94 100644 --- a/lib/DAV/DeckCalendarBackend.php +++ b/lib/DAV/DeckCalendarBackend.php @@ -59,7 +59,7 @@ class DeckCalendarBackend { } public function getBoards(): array { - return $this->boardService->findAll(-1, null, false); + return $this->boardService->findAll(-1, false, false); } public function getBoard(int $id): Board { diff --git a/lib/Db/AclMapper.php b/lib/Db/AclMapper.php index cac999d4d..a74244370 100644 --- a/lib/Db/AclMapper.php +++ b/lib/Db/AclMapper.php @@ -52,6 +52,20 @@ class AclMapper extends DeckMapper implements IPermissionMapper { return $this->findEntities($qb); } + public function findIn(array $boardIds, ?int $limit = null, ?int $offset = null): array { + $qb = $this->db->getQueryBuilder(); + $qb->select('id', 'board_id', 'type', 'participant', 'permission_edit', 'permission_share', 'permission_manage') + ->from('deck_board_acl') + ->where($qb->expr()->in('board_id', $qb->createParameter('boardIds'))) + ->setMaxResults($limit) + ->setFirstResult($offset); + + return iterator_to_array($this->chunkQuery($boardIds, function (array $ids) use ($qb) { + $qb->setParameter('boardIds', $ids, IQueryBuilder::PARAM_INT_ARRAY); + return $this->findEntities($qb); + })); + } + /** * @param numeric $userId * @param numeric $id diff --git a/lib/Db/AssignmentMapper.php b/lib/Db/AssignmentMapper.php index d7d3a1eb8..f2731431f 100644 --- a/lib/Db/AssignmentMapper.php +++ b/lib/Db/AssignmentMapper.php @@ -28,15 +28,13 @@ namespace OCA\Deck\Db; use OCA\Deck\NotFoundException; use OCA\Deck\Service\CirclesService; use OCP\AppFramework\Db\Entity; -use OCP\AppFramework\Db\QBMapper; use OCP\DB\QueryBuilder\IQueryBuilder; use OCP\IDBConnection; use OCP\IGroupManager; use OCP\IUserManager; -use PDO; -/** @template-extends QBMapper */ -class AssignmentMapper extends QBMapper implements IPermissionMapper { +/** @template-extends DeckMapper */ +class AssignmentMapper extends DeckMapper implements IPermissionMapper { /** @var CardMapper */ private $cardMapper; @@ -60,7 +58,7 @@ class AssignmentMapper extends QBMapper implements IPermissionMapper { $qb = $this->db->getQueryBuilder(); $qb->select('*') ->from('deck_assigned_users') - ->where($qb->expr()->eq('card_id', $qb->createNamedParameter($cardId, PDO::PARAM_INT))); + ->where($qb->expr()->eq('card_id', $qb->createNamedParameter($cardId, IQueryBuilder::PARAM_INT))); $users = $this->findEntities($qb); foreach ($users as $user) { $this->mapParticipant($user); @@ -68,12 +66,29 @@ class AssignmentMapper extends QBMapper implements IPermissionMapper { return $users; } + public function findIn(array $cardIds): array { + $qb = $this->db->getQueryBuilder(); + $qb->select('*') + ->from('deck_assigned_users') + ->where($qb->expr()->in('card_id', $qb->createParameter('cardIds'))); + + $users = iterator_to_array($this->chunkQuery($cardIds, function (array $ids) use ($qb) { + $qb->setParameter('cardIds', $ids, IQueryBuilder::PARAM_INT_ARRAY); + return $this->findEntities($qb); + })); + + foreach ($users as $user) { + $this->mapParticipant($user); + } + return $users; + } + public function findByParticipant(string $participant, $type = Assignment::TYPE_USER): array { $qb = $this->db->getQueryBuilder(); $qb->select('*') ->from('deck_assigned_users') - ->where($qb->expr()->eq('participant', $qb->createNamedParameter($participant, PDO::PARAM_STR))) - ->andWhere($qb->expr()->eq('type', $qb->createNamedParameter($type, PDO::PARAM_INT))); + ->where($qb->expr()->eq('participant', $qb->createNamedParameter($participant, IQueryBuilder::PARAM_STR))) + ->andWhere($qb->expr()->eq('type', $qb->createNamedParameter($type, IQueryBuilder::PARAM_INT))); return $this->findEntities($qb); } @@ -132,8 +147,8 @@ class AssignmentMapper extends QBMapper implements IPermissionMapper { private function getOrigin(Assignment $assignment) { if ($assignment->getType() === Assignment::TYPE_USER) { - $origin = $this->userManager->get($assignment->getParticipant()); - return $origin ? new User($origin) : null; + $origin = $this->userManager->userExists($assignment->getParticipant()); + return $origin ? new User($assignment->getParticipant(), $this->userManager) : null; } if ($assignment->getType() === Assignment::TYPE_GROUP) { $origin = $this->groupManager->get($assignment->getParticipant()); diff --git a/lib/Db/BoardMapper.php b/lib/Db/BoardMapper.php index 5db3c3994..456152527 100644 --- a/lib/Db/BoardMapper.php +++ b/lib/Db/BoardMapper.php @@ -90,9 +90,6 @@ class BoardMapper extends QBMapper implements IPermissionMapper { $this->boardCache[$id] = $this->findEntity($qb); } - // FIXME is this necessary? it was NOT done with the old mapper - // $this->mapOwner($board); - // Add labels if ($withLabels && $this->boardCache[$id]->getLabels() === null) { $labels = $this->labelMapper->findAll($id); @@ -160,6 +157,20 @@ class BoardMapper extends QBMapper implements IPermissionMapper { $groupBoards = $this->findAllByGroups($userId, $groups, null, null, $since, $includeArchived, $before, $term); $circleBoards = $this->findAllByCircles($userId, null, null, $since, $includeArchived, $before, $term); $allBoards = array_unique(array_merge($userBoards, $groupBoards, $circleBoards)); + + // Could be moved outside + $acls = $this->aclMapper->findIn(array_map(function ($board) { + return $board->getId(); + }, $allBoards)); + + /* @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); + } + foreach ($allBoards as $board) { $this->boardCache[$board->getId()] = $board; } @@ -259,11 +270,7 @@ class BoardMapper extends QBMapper implements IPermissionMapper { $entry->setShared(1); } $entries = array_merge($entries, $sharedEntries); - /* @var Board $entry */ - foreach ($entries as $entry) { - $acl = $this->aclMapper->findAll($entry->id); - $entry->setAcl($acl); - } + return $entries; } @@ -336,11 +343,6 @@ class BoardMapper extends QBMapper implements IPermissionMapper { foreach ($entries as $entry) { $entry->setShared(2); } - /* @var Board $entry */ - foreach ($entries as $entry) { - $acl = $this->aclMapper->findAll($entry->id); - $entry->setAcl($acl); - } return $entries; } @@ -397,11 +399,6 @@ class BoardMapper extends QBMapper implements IPermissionMapper { foreach ($entries as $entry) { $entry->setShared(2); } - /* @var Board $entry */ - foreach ($entries as $entry) { - $acl = $this->aclMapper->findAll($entry->id); - $entry->setAcl($acl); - } return $entries; } @@ -455,13 +452,11 @@ class BoardMapper extends QBMapper implements IPermissionMapper { } public function mapAcl(Acl &$acl) { - $userManager = $this->userManager; $groupManager = $this->groupManager; $acl->resolveRelation('participant', function ($participant) use (&$acl, &$userManager, &$groupManager) { if ($acl->getType() === Acl::PERMISSION_TYPE_USER) { - $user = $userManager->get($participant); - if ($user !== null) { - return new User($user); + if ($this->userManager->userExists($acl->getParticipant())) { + return new User($acl->getParticipant(), $this->userManager); } $this->logger->debug('User ' . $acl->getId() . ' not found when mapping acl ' . $acl->getParticipant()); return null; @@ -499,9 +494,8 @@ class BoardMapper extends QBMapper implements IPermissionMapper { public function mapOwner(Board &$board) { $userManager = $this->userManager; $board->resolveRelation('owner', function ($owner) use (&$userManager) { - $user = $userManager->get($owner); - if ($user !== null) { - return new User($user); + if ($this->userManager->userExists($owner)) { + return new User($owner, $userManager); } return null; }); diff --git a/lib/Db/CardMapper.php b/lib/Db/CardMapper.php index d1b70c600..ac070aebf 100644 --- a/lib/Db/CardMapper.php +++ b/lib/Db/CardMapper.php @@ -254,13 +254,13 @@ class CardMapper extends QBMapper implements IPermissionMapper { return $this->findEntities($qb); } - public function findAllWithDue($boardId) { + public function findAllWithDue(array $boardIds) { $qb = $this->db->getQueryBuilder(); $qb->select('c.*') ->from('deck_cards', 'c') ->innerJoin('c', 'deck_stacks', 's', 's.id = c.stack_id') ->innerJoin('s', 'deck_boards', 'b', 'b.id = s.board_id') - ->where($qb->expr()->eq('s.board_id', $qb->createNamedParameter($boardId, IQueryBuilder::PARAM_INT))) + ->where($qb->expr()->in('s.board_id', $qb->createNamedParameter($boardIds, IQueryBuilder::PARAM_INT_ARRAY))) ->andWhere($qb->expr()->isNotNull('c.duedate')) ->andWhere($qb->expr()->eq('c.archived', $qb->createNamedParameter(false, IQueryBuilder::PARAM_BOOL))) ->andWhere($qb->expr()->eq('c.deleted_at', $qb->createNamedParameter(0, IQueryBuilder::PARAM_INT))) @@ -270,14 +270,14 @@ class CardMapper extends QBMapper implements IPermissionMapper { return $this->findEntities($qb); } - public function findToMeOrNotAssignedCards($boardId, $username) { + public function findToMeOrNotAssignedCards(array $boardIds, string $username) { $qb = $this->db->getQueryBuilder(); $qb->select('c.*') ->from('deck_cards', 'c') ->innerJoin('c', 'deck_stacks', 's', 's.id = c.stack_id') ->innerJoin('s', 'deck_boards', 'b', 'b.id = s.board_id') ->leftJoin('c', 'deck_assigned_users', 'u', 'c.id = u.card_id') - ->where($qb->expr()->eq('s.board_id', $qb->createNamedParameter($boardId, IQueryBuilder::PARAM_INT))) + ->where($qb->expr()->in('s.board_id', $qb->createNamedParameter($boardIds, IQueryBuilder::PARAM_INT_ARRAY))) ->andWhere($qb->expr()->orX( $qb->expr()->eq('u.participant', $qb->createNamedParameter($username, IQueryBuilder::PARAM_STR)), $qb->expr()->isNull('u.participant')) @@ -607,9 +607,8 @@ class CardMapper extends QBMapper implements IPermissionMapper { public function mapOwner(Card &$card) { $userManager = $this->userManager; $card->resolveRelation('owner', function ($owner) use (&$userManager) { - $user = $userManager->get($owner); - if ($user !== null) { - return new User($user); + if ($userManager->userExists($owner)) { + return new User($owner, $this->userManager); } return null; }); diff --git a/lib/Db/DeckMapper.php b/lib/Db/DeckMapper.php index 887774409..884079447 100644 --- a/lib/Db/DeckMapper.php +++ b/lib/Db/DeckMapper.php @@ -23,6 +23,7 @@ namespace OCA\Deck\Db; +use Generator; use OCP\AppFramework\Db\Entity; use OCP\AppFramework\Db\QBMapper; use OCP\DB\QueryBuilder\IQueryBuilder; @@ -35,7 +36,7 @@ abstract class DeckMapper extends QBMapper { /** * @param $id - * @return \OCP\AppFramework\Db\Entity if not found + * @return T * @throws \OCP\AppFramework\Db\MultipleObjectsReturnedException * @throws \OCP\AppFramework\Db\DoesNotExistException */ @@ -47,4 +48,21 @@ abstract class DeckMapper extends QBMapper { return $this->findEntity($qb); } + + /** + * Helper function to split passed array into chunks of 1000 elements and + * call a given callback for fetching query results + * + * Can be useful to limit to 1000 results per query for oracle compatiblity + * but still iterate over all results + */ + public function chunkQuery(array $ids, callable $callback): Generator { + $limit = 1000; + while (!empty($ids)) { + $slice = array_splice($ids, 0, $limit); + foreach ($callback($slice) as $item) { + yield $item; + } + } + } } diff --git a/lib/Db/LabelMapper.php b/lib/Db/LabelMapper.php index 7f6de33bf..aeccef539 100644 --- a/lib/Db/LabelMapper.php +++ b/lib/Db/LabelMapper.php @@ -79,6 +79,19 @@ class LabelMapper extends DeckMapper implements IPermissionMapper { return $this->findEntities($qb); } + public function findAssignedLabelsForCards($cardIds, $limit = null, $offset = null): array { + $qb = $this->db->getQueryBuilder(); + $qb->select('l.*', 'card_id') + ->from($this->getTableName(), 'l') + ->innerJoin('l', 'deck_assigned_labels', 'al', 'l.id = al.label_id') + ->where($qb->expr()->in('card_id', $qb->createNamedParameter($cardIds, IQueryBuilder::PARAM_INT_ARRAY))) + ->orderBy('l.id') + ->setMaxResults($limit) + ->setFirstResult($offset); + + return $this->findEntities($qb); + } + /** * @param numeric $boardId * @param int|null $limit diff --git a/lib/Db/RelationalObject.php b/lib/Db/RelationalObject.php index bd176e1b8..1928db097 100644 --- a/lib/Db/RelationalObject.php +++ b/lib/Db/RelationalObject.php @@ -33,7 +33,7 @@ class RelationalObject implements JsonSerializable { * RelationalObject constructor. * * @param $primaryKey string - * @param $object + * @param callable|mixed $object */ public function __construct($primaryKey, $object) { $this->primaryKey = $primaryKey; @@ -47,16 +47,24 @@ class RelationalObject implements JsonSerializable { ); } + public function getObject() { + if (is_callable($this->object)) { + $this->object = call_user_func($this->object, $this); + } + + return $this->object; + } + /** * This method should be overwritten if object doesn't implement \JsonSerializable * * @throws \Exception */ public function getObjectSerialization() { - if ($this->object instanceof JsonSerializable) { - return $this->object->jsonSerialize(); + if ($this->getObject() instanceof JsonSerializable) { + return $this->getObject()->jsonSerialize(); } else { - throw new \Exception('jsonSerialize is not implemented on ' . get_class($this)); + throw new \Exception('jsonSerialize is not implemented on ' . get_class($this->getObject())); } } diff --git a/lib/Db/StackMapper.php b/lib/Db/StackMapper.php index 7403b6426..624c739e7 100644 --- a/lib/Db/StackMapper.php +++ b/lib/Db/StackMapper.php @@ -26,16 +26,19 @@ namespace OCA\Deck\Db; use OCP\AppFramework\Db\DoesNotExistException; use OCP\AppFramework\Db\Entity; use OCP\AppFramework\Db\MultipleObjectsReturnedException; +use OCP\Cache\CappedMemoryCache; use OCP\DB\QueryBuilder\IQueryBuilder; use OCP\IDBConnection; /** @template-extends DeckMapper */ class StackMapper extends DeckMapper implements IPermissionMapper { - private $cardMapper; + private CappedMemoryCache $stackCache; + private CardMapper $cardMapper; public function __construct(IDBConnection $db, CardMapper $cardMapper) { parent::__construct($db, 'deck_stacks', Stack::class); $this->cardMapper = $cardMapper; + $this->stackCache = new CappedMemoryCache(); } @@ -47,12 +50,17 @@ class StackMapper extends DeckMapper implements IPermissionMapper { * @throws \OCP\DB\Exception */ public function find($id): Stack { + if (isset($this->stackCache[(string)$id])) { + return $this->stackCache[(string)$id]; + } + $qb = $this->db->getQueryBuilder(); $qb->select('*') ->from($this->getTableName()) ->where($qb->expr()->eq('id', $qb->createNamedParameter($id, IQueryBuilder::PARAM_INT))); - return $this->findEntity($qb); + $this->stackCache[(string)$id] = $this->findEntity($qb); + return $this->stackCache[(string)$id]; } /** @@ -113,9 +121,16 @@ class StackMapper extends DeckMapper implements IPermissionMapper { return $this->findEntities($qb); } + public function update(Entity $entity): Entity { + $result = parent::update($entity); + $this->stackCache[(string)$entity->getId()] = $result; + return $result; + } + public function delete(Entity $entity): Entity { // delete cards on stack $this->cardMapper->deleteByStack($entity->getId()); + unset($this->stackCache[(string)$entity->getId()]); return parent::delete($entity); } diff --git a/lib/Db/User.php b/lib/Db/User.php index d595ece75..b33e264d9 100644 --- a/lib/Db/User.php +++ b/lib/Db/User.php @@ -23,27 +23,30 @@ namespace OCA\Deck\Db; -use OCP\IUser; +use OCP\IUserManager; class User extends RelationalObject { - public function __construct(IUser $user) { - $primaryKey = $user->getUID(); - parent::__construct($primaryKey, $user); + private IUserManager $userManager; + public function __construct($uid, IUserManager $userManager) { + $this->userManager = $userManager; + parent::__construct($uid, function ($object) { + return $this->userManager->get($object->getPrimaryKey()); + }); } public function getObjectSerialization() { return [ - 'uid' => $this->object->getUID(), - 'displayname' => $this->object->getDisplayName(), - 'type' => 0 + 'uid' => $this->getObject()->getUID(), + 'displayname' => $this->getObject()->getDisplayName(), + 'type' => Acl::PERMISSION_TYPE_USER ]; } public function getUID() { - return $this->object->getUID(); + return $this->getPrimaryKey(); } public function getDisplayName() { - return $this->object->getDisplayName(); + return $this->userManager->getDisplayName($this->getPrimaryKey()); } } diff --git a/lib/Event/AAclEvent.php b/lib/Event/AAclEvent.php index e5534f743..f7107fe1c 100644 --- a/lib/Event/AAclEvent.php +++ b/lib/Event/AAclEvent.php @@ -31,7 +31,7 @@ use OCP\EventDispatcher\Event; abstract class AAclEvent extends Event { private $acl; - + public function __construct(Acl $acl) { parent::__construct(); @@ -41,4 +41,8 @@ abstract class AAclEvent extends Event { public function getAcl(): Acl { return $this->acl; } + + public function getBoardId(): int { + return $this->acl->getBoardId(); + } } diff --git a/lib/Service/BoardService.php b/lib/Service/BoardService.php index a7388f29d..1ef0a42cf 100644 --- a/lib/Service/BoardService.php +++ b/lib/Service/BoardService.php @@ -79,7 +79,8 @@ class BoardService { private IEventDispatcher $eventDispatcher; private ChangeHelper $changeHelper; private CardMapper $cardMapper; - private ?array $boardsCache = null; + private ?array $boardsCacheFull = null; + private ?array $boardsCachePartial = null; private IURLGenerator $urlGenerator; private IDBConnection $connection; private BoardServiceValidator $boardServiceValidator; @@ -147,96 +148,45 @@ class BoardService { } /** - * @return array + * @return Board[] */ - public function findAll($since = -1, $details = null, $includeArchived = true) { - if ($this->boardsCache) { - return $this->boardsCache; + public function findAll(int $since = -1, bool $fullDetails = false, bool $includeArchived = true): array { + if ($this->boardsCacheFull && $fullDetails) { + return $this->boardsCacheFull; } + + if ($this->boardsCachePartial && !$fullDetails) { + return $this->boardsCachePartial; + } + $complete = $this->getUserBoards($since, $includeArchived); - $result = []; - /** @var Board $item */ - foreach ($complete as &$item) { - $this->boardMapper->mapOwner($item); - if ($item->getAcl() !== null) { - foreach ($item->getAcl() as &$acl) { - $this->boardMapper->mapAcl($acl); - } - } - if ($details !== null) { - $this->enrichWithStacks($item); - $this->enrichWithLabels($item); - $this->enrichWithUsers($item); - } - $permissions = $this->permissionService->matchPermissions($item); - $item->setPermissions([ - 'PERMISSION_READ' => $permissions[Acl::PERMISSION_READ] ?? false, - 'PERMISSION_EDIT' => $permissions[Acl::PERMISSION_EDIT] ?? false, - 'PERMISSION_MANAGE' => $permissions[Acl::PERMISSION_MANAGE] ?? false, - 'PERMISSION_SHARE' => $permissions[Acl::PERMISSION_SHARE] ?? false - ]); - $this->enrichWithBoardSettings($item); - $result[$item->getId()] = $item; - } - $this->boardsCache = $result; - return array_values($result); + return $this->enrichBoards($complete, $fullDetails); } /** - * @param $boardId - * @return Board * @throws DoesNotExistException * @throws \OCA\Deck\NoPermissionException * @throws \OCP\AppFramework\Db\MultipleObjectsReturnedException * @throws BadRequestException */ - public function find($boardId) { + public function find(int $boardId, bool $fullDetails = true): Board { $this->boardServiceValidator->check(compact('boardId')); - if ($this->boardsCache && isset($this->boardsCache[$boardId])) { - return $this->boardsCache[$boardId]; + + if (isset($this->boardsCacheFull[$boardId]) && $fullDetails) { + return $this->boardsCacheFull[$boardId]; } - if (is_numeric($boardId) === false) { - throw new BadRequestException('board id must be a number'); + + if (isset($this->boardsCachePartial[$boardId]) && !$fullDetails) { + return $this->boardsCachePartial[$boardId]; } $this->permissionService->checkPermission($this->boardMapper, $boardId, Acl::PERMISSION_READ); /** @var Board $board */ $board = $this->boardMapper->find($boardId, true, true); - $this->boardMapper->mapOwner($board); - if ($board->getAcl() !== null) { - foreach ($board->getAcl() as $acl) { - if ($acl !== null) { - $this->boardMapper->mapAcl($acl); - } - } - } - $permissions = $this->permissionService->matchPermissions($board); - $board->setPermissions([ - 'PERMISSION_READ' => $permissions[Acl::PERMISSION_READ] ?? false, - 'PERMISSION_EDIT' => $permissions[Acl::PERMISSION_EDIT] ?? false, - 'PERMISSION_MANAGE' => $permissions[Acl::PERMISSION_MANAGE] ?? false, - 'PERMISSION_SHARE' => $permissions[Acl::PERMISSION_SHARE] ?? false - ]); - $this->enrichWithUsers($board); - $this->enrichWithBoardSettings($board); - $this->enrichWithActiveSessions($board); - $this->boardsCache[$board->getId()] = $board; + [$board] = $this->enrichBoards([$board], $fullDetails); return $board; } - /** - * @return array - */ - private function getBoardPrerequisites() { - $groups = $this->groupManager->getUserGroupIds( - $this->userManager->get($this->userId) - ); - return [ - 'user' => $this->userId, - 'groups' => $groups - ]; - } - /** * @param $mapper * @param $id @@ -456,7 +406,7 @@ class BoardService { public function enrichWithActiveSessions(Board $board) { $sessions = $this->sessionMapper->findAllActive($board->getId()); - + $board->setActiveSessions(array_values( array_unique( array_map(function (Session $session) { @@ -691,6 +641,45 @@ class BoardService { return $board; } + /** @param Board[] $boards */ + private function enrichBoards(array $boards, bool $fullDetails = true): array { + $result = []; + foreach ($boards as $board) { + // FIXME The enrichment in here could make use of combined queries + $this->boardMapper->mapOwner($board); + if ($board->getAcl() !== null) { + foreach ($board->getAcl() as &$acl) { + $this->boardMapper->mapAcl($acl); + } + } + + $permissions = $this->permissionService->matchPermissions($board); + $board->setPermissions([ + 'PERMISSION_READ' => $permissions[Acl::PERMISSION_READ] ?? false, + 'PERMISSION_EDIT' => $permissions[Acl::PERMISSION_EDIT] ?? false, + 'PERMISSION_MANAGE' => $permissions[Acl::PERMISSION_MANAGE] ?? false, + 'PERMISSION_SHARE' => $permissions[Acl::PERMISSION_SHARE] ?? false + ]); + + if ($fullDetails) { + $this->enrichWithStacks($board); + $this->enrichWithLabels($board); + $this->enrichWithUsers($board); + $this->enrichWithBoardSettings($board); + $this->enrichWithActiveSessions($board); + } + + // Cache for further usage + if ($fullDetails) { + $this->boardsCacheFull[$board->getId()] = $board; + } else { + $this->boardsCachePartial[$board->getId()] = $board; + } + } + + return $boards; + } + private function enrichWithStacks($board, $since = -1) { $stacks = $this->stackMapper->findAll($board->getId(), null, null, $since); @@ -713,7 +702,7 @@ class BoardService { private function enrichWithUsers($board, $since = -1) { $boardUsers = $this->permissionService->findUsers($board->getId()); - if (\count($boardUsers) === 0) { + if ($boardUsers === null || \count($boardUsers) === 0) { return; } $board->setUsers(array_values($boardUsers)); @@ -723,10 +712,6 @@ class BoardService { return $this->urlGenerator->linkToRouteAbsolute('deck.page.index') . '#' . $endpoint; } - private function clearBoardsCache() { - $this->boardsCache = null; - } - /** * Clean a given board data from the Cache */ @@ -735,7 +720,8 @@ class BoardService { $boardOwnerId = $board->getOwner(); $this->boardMapper->flushCache($boardId, $boardOwnerId); - unset($this->boardsCache[$boardId]); + unset($this->boardsCacheFull[$boardId]); + unset($this->boardsCachePartial[$boardId]); } private function enrichWithCards($board) { diff --git a/lib/Service/CardService.php b/lib/Service/CardService.php index 95dbcf59a..b44a094d9 100644 --- a/lib/Service/CardService.php +++ b/lib/Service/CardService.php @@ -28,11 +28,13 @@ namespace OCA\Deck\Service; use OCA\Deck\Activity\ActivityManager; use OCA\Deck\Activity\ChangeSet; +use OCA\Deck\Db\Assignment; use OCA\Deck\Db\AssignmentMapper; use OCA\Deck\Db\Card; use OCA\Deck\Db\CardMapper; use OCA\Deck\Db\Acl; use OCA\Deck\Db\ChangeHelper; +use OCA\Deck\Db\Label; use OCA\Deck\Db\StackMapper; use OCA\Deck\Event\CardCreatedEvent; use OCA\Deck\Event\CardDeletedEvent; @@ -114,32 +116,52 @@ class CardService { $this->cardServiceValidator = $cardServiceValidator; } - public function enrich($card) { - $cardId = $card->getId(); - $this->cardMapper->mapOwner($card); - $card->setAssignedUsers($this->assignedUsersMapper->findAll($cardId)); - $card->setLabels($this->labelMapper->findAssignedLabelsForCard($cardId)); - $card->setAttachmentCount($this->attachmentService->count($cardId)); + public function enrichCards($cards) { $user = $this->userManager->get($this->currentUser); - $lastRead = $this->commentsManager->getReadMark('deckCard', (string)$card->getId(), $user); - $countUnreadComments = $this->commentsManager->getNumberOfCommentsForObject('deckCard', (string)$card->getId(), $lastRead); - $countComments = $this->commentsManager->getNumberOfCommentsForObject('deckCard', (string)$card->getId()); - $card->setCommentsUnread($countUnreadComments); - $card->setCommentsCount($countComments); - $stack = $this->stackMapper->find($card->getStackId()); - $board = $this->boardService->find($stack->getBoardId()); - $card->setRelatedStack($stack); - $card->setRelatedBoard($board); + $cardIds = array_map(function (Card $card) use ($user) { + // 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)); + + // 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()); + $lastRead = $countComments > 0 ? $this->commentsManager->getReadMark('deckCard', (string)$card->getId(), $user) : null; + $countUnreadComments = $lastRead ? $this->commentsManager->getNumberOfCommentsForObject('deckCard', (string)$card->getId(), $lastRead) : 0; + $card->setCommentsUnread($countUnreadComments); + $card->setCommentsCount($countComments); + + $stack = $this->stackMapper->find($card->getStackId()); + $board = $this->boardService->find($stack->getBoardId(), false); + $card->setRelatedStack($stack); + $card->setRelatedBoard($board); + + return $card->getId(); + }, $cards); + + $assignedLabels = $this->labelMapper->findAssignedLabelsForCards($cardIds); + $assignedUsers = $this->assignedUsersMapper->findIn($cardIds); + + 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); + } + + return $cards; } - public function fetchDeleted($boardId) { $this->cardServiceValidator->check(compact('boardId')); $this->permissionService->checkPermission($this->boardMapper, $boardId, Acl::PERMISSION_READ); $cards = $this->cardMapper->findDeleted($boardId); - foreach ($cards as $card) { - $this->enrich($card); - } + $this->enrichCards($cards); return $cards; } @@ -153,16 +175,17 @@ class CardService { public function find(int $cardId) { $this->permissionService->checkPermission($this->cardMapper, $cardId, Acl::PERMISSION_READ); $card = $this->cardMapper->find($cardId); - $assignedUsers = $this->assignedUsersMapper->findAll($card->getId()); + [$card] = $this->enrichCards([$card]); + + // Attachments are only enriched on individual card fetching $attachments = $this->attachmentService->findAll($cardId, true); if ($this->request->getParam('apiVersion') === '1.0') { $attachments = array_filter($attachments, function ($attachment) { return $attachment->getType() === 'deck_file'; }); } - $card->setAssignedUsers($assignedUsers); $card->setAttachments($attachments); - $this->enrich($card); + return $card; } @@ -174,9 +197,7 @@ class CardService { return []; } $cards = $this->cardMapper->findCalendarEntries($boardId); - foreach ($cards as $card) { - $this->enrich($card); - } + $this->enrichCards($cards); return $cards; } diff --git a/lib/Service/DefaultBoardService.php b/lib/Service/DefaultBoardService.php index a9f31d0ac..5698994d6 100644 --- a/lib/Service/DefaultBoardService.php +++ b/lib/Service/DefaultBoardService.php @@ -62,9 +62,8 @@ class DefaultBoardService { */ public function checkFirstRun($userId): bool { $firstRun = $this->config->getUserValue($userId, Application::APP_ID, 'firstRun', 'yes'); - $userBoards = $this->boardMapper->findAllByUser($userId); - if ($firstRun === 'yes' && count($userBoards) === 0) { + if ($firstRun === 'yes') { try { $this->config->setUserValue($userId, Application::APP_ID, 'firstRun', 'no'); } catch (PreConditionNotMetException $e) { diff --git a/lib/Service/OverviewService.php b/lib/Service/OverviewService.php index 8b8b07511..7c1f4479f 100644 --- a/lib/Service/OverviewService.php +++ b/lib/Service/OverviewService.php @@ -28,7 +28,7 @@ declare(strict_types=1); namespace OCA\Deck\Service; use OCA\Deck\Db\AssignmentMapper; -use OCA\Deck\Db\Card; +use OCA\Deck\Db\Board; use OCA\Deck\Db\CardMapper; use OCA\Deck\Model\CardDetails; use OCP\Comments\ICommentsManager; @@ -37,6 +37,7 @@ use OCA\Deck\Db\LabelMapper; use OCP\IUserManager; class OverviewService { + private CardService $cardService; private BoardMapper $boardMapper; private LabelMapper $labelMapper; private CardMapper $cardMapper; @@ -46,6 +47,7 @@ class OverviewService { private AttachmentService $attachmentService; public function __construct( + CardService $cardService, BoardMapper $boardMapper, LabelMapper $labelMapper, CardMapper $cardMapper, @@ -54,6 +56,7 @@ class OverviewService { ICommentsManager $commentsManager, AttachmentService $attachmentService ) { + $this->cardService = $cardService; $this->boardMapper = $boardMapper; $this->labelMapper = $labelMapper; $this->cardMapper = $cardMapper; @@ -63,66 +66,43 @@ class OverviewService { $this->attachmentService = $attachmentService; } - public function enrich(Card $card, string $userId): void { - $cardId = $card->getId(); - - $this->cardMapper->mapOwner($card); - $card->setAssignedUsers($this->assignedUsersMapper->findAll($cardId)); - $card->setLabels($this->labelMapper->findAssignedLabelsForCard($cardId)); - $card->setAttachmentCount($this->attachmentService->count($cardId)); - - $user = $this->userManager->get($userId); - if ($user !== null) { - $lastRead = $this->commentsManager->getReadMark('deckCard', (string)$card->getId(), $user); - $count = $this->commentsManager->getNumberOfCommentsForObject('deckCard', (string)$card->getId(), $lastRead); - $card->setCommentsUnread($count); - } - } - - public function findAllWithDue(string $userId): array { - $userBoards = $this->boardMapper->findAllForUser($userId); - $allDueCards = []; - foreach ($userBoards as $userBoard) { - $allDueCards[] = array_map(function ($card) use ($userBoard, $userId) { - $this->enrich($card, $userId); - return (new CardDetails($card, $userBoard))->jsonSerialize(); - }, $this->cardMapper->findAllWithDue($userBoard->getId())); - } - return array_merge(...$allDueCards); - } - public function findUpcomingCards(string $userId): array { $userBoards = $this->boardMapper->findAllForUser($userId); + + $boardOwnerIds = array_filter(array_map(function (Board $board) { + return count($board->getAcl()) === 0 ? $board->getId() : null; + }, $userBoards)); + $boardSharedIds = array_filter(array_map(function (Board $board) { + return count($board->getAcl()) > 0 ? $board->getId() : null; + }, $userBoards)); + + $foundCards = array_merge( + // private board: get cards with due date + $this->cardMapper->findAllWithDue($boardOwnerIds), + // shared board: get all my assigned or unassigned cards + $this->cardMapper->findToMeOrNotAssignedCards($boardSharedIds, $userId) + ); + + $this->cardService->enrichCards($foundCards); $overview = []; - foreach ($userBoards as $userBoard) { - if (count($userBoard->getAcl()) === 0) { - // private board: get cards with due date - $cards = $this->cardMapper->findAllWithDue($userBoard->getId()); - } else { - // shared board: get all my assigned or unassigned cards - $cards = $this->cardMapper->findToMeOrNotAssignedCards($userBoard->getId(), $userId); + foreach ($foundCards as $card) { + $diffDays = $card->getDaysUntilDue(); + + $key = 'later'; + if ($diffDays === null) { + $key = 'nodue'; + } elseif ($diffDays < 0) { + $key = 'overdue'; + } elseif ($diffDays === 0) { + $key = 'today'; + } elseif ($diffDays === 1) { + $key = 'tomorrow'; + } elseif ($diffDays <= 7) { + $key = 'nextSevenDays'; } - foreach ($cards as $card) { - $this->enrich($card, $userId); - $diffDays = $card->getDaysUntilDue(); - - $key = 'later'; - if ($diffDays === null) { - $key = 'nodue'; - } elseif ($diffDays < 0) { - $key = 'overdue'; - } elseif ($diffDays === 0) { - $key = 'today'; - } elseif ($diffDays === 1) { - $key = 'tomorrow'; - } elseif ($diffDays <= 7) { - $key = 'nextSevenDays'; - } - - $card = (new CardDetails($card, $userBoard)); - $overview[$key][] = $card->jsonSerialize(); - } + $card = (new CardDetails($card, $card->getRelatedBoard())); + $overview[$key][] = $card->jsonSerialize(); } return $overview; } diff --git a/lib/Service/PermissionService.php b/lib/Service/PermissionService.php index d0630a633..2956b3234 100644 --- a/lib/Service/PermissionService.php +++ b/lib/Service/PermissionService.php @@ -143,7 +143,7 @@ class PermissionService { * @return bool * @throws NoPermissionException */ - public function checkPermission($mapper, $id, $permission, $userId = null) { + public function checkPermission($mapper, $id, $permission, $userId = null): bool { $boardId = $id; if ($mapper instanceof IPermissionMapper && !($mapper instanceof BoardMapper)) { $boardId = $mapper->findBoardId($id); @@ -153,23 +153,11 @@ class PermissionService { throw new NoPermissionException('Permission denied'); } - if ($permission === Acl::PERMISSION_SHARE && $this->shareManager->sharingDisabledForUser($this->userId)) { - throw new NoPermissionException('Permission denied'); - } - - if ($this->userIsBoardOwner($boardId, $userId)) { + $permissions = $this->getPermissions($boardId); + if ($permissions[$permission] === true) { return true; } - try { - $acls = $this->getBoard($boardId)->getAcl() ?? []; - $result = $this->userCan($acls, $permission, $userId); - if ($result) { - return true; - } - } catch (DoesNotExistException | MultipleObjectsReturnedException $e) { - } - // Throw NoPermission to not leak information about existing entries throw new NoPermissionException('Permission denied'); } @@ -260,22 +248,20 @@ class PermissionService { } $users = []; - $owner = $this->userManager->get($board->getOwner()); - if ($owner === null) { + if (!$this->userManager->userExists($board->getOwner())) { $this->logger->info('No owner found for board ' . $board->getId()); } else { - $users[$owner->getUID()] = new User($owner); + $users[$board->getOwner()] = new User($board->getOwner(), $this->userManager); } $acls = $this->aclMapper->findAll($boardId); /** @var Acl $acl */ foreach ($acls as $acl) { if ($acl->getType() === Acl::PERMISSION_TYPE_USER) { - $user = $this->userManager->get($acl->getParticipant()); - if ($user === null) { + if (!$this->userManager->userExists($acl->getParticipant())) { $this->logger->info('No user found for acl rule ' . $acl->getId()); continue; } - $users[$user->getUID()] = new User($user); + $users[$acl->getParticipant()] = new User($acl->getParticipant(), $this->userManager); } if ($acl->getType() === Acl::PERMISSION_TYPE_GROUP) { $group = $this->groupManager->get($acl->getParticipant()); @@ -284,7 +270,7 @@ class PermissionService { continue; } foreach ($group->getUsers() as $user) { - $users[$user->getUID()] = new User($user); + $users[$user->getUID()] = new User($user->getUID(), $this->userManager); } } @@ -305,7 +291,7 @@ class PermissionService { if ($user === null) { $this->logger->info('No user found for circle member ' . $member->getUserId()); } else { - $users[$member->getUserId()] = new User($user); + $users[$member->getUserId()] = new User($member->getUserId(), $this->userManager); } } } catch (\Exception $e) { diff --git a/lib/Service/SearchService.php b/lib/Service/SearchService.php index 11bfedf33..421747403 100644 --- a/lib/Service/SearchService.php +++ b/lib/Service/SearchService.php @@ -82,11 +82,7 @@ class SearchService { }, $boards); $matchedCards = $this->cardMapper->search($boardIds, $this->filterStringParser->parse($term), $limit, $cursor); - $self = $this; - return array_map(function (Card $card) use ($self) { - $self->cardService->enrich($card); - return $card; - }, $matchedCards); + return $this->cardService->enrichCards($matchedCards); } public function searchBoards(string $term, ?int $limit, ?int $cursor): array { @@ -117,7 +113,8 @@ class SearchService { $comment = $this->commentsManager->get($cardRow['comment_id']); unset($cardRow['comment_id']); $card = Card::fromRow($cardRow); - $self->cardService->enrich($card); + // TODO: Only perform one enrich call here + $self->cardService->enrichCards([$card]); $user = $this->userManager->get($comment->getActorId()); $displayName = $user ? $user->getDisplayName() : ''; return new CommentSearchResultEntry($comment->getId(), $comment->getMessage(), $displayName, $card, $this->urlGenerator, $this->l10n); diff --git a/lib/Service/StackService.php b/lib/Service/StackService.php index d41c00de0..83ca7d23e 100644 --- a/lib/Service/StackService.php +++ b/lib/Service/StackService.php @@ -94,9 +94,9 @@ class StackService { return; } + $this->cardService->enrichCards($cards); $cards = array_map( function (Card $card): CardDetails { - $this->cardService->enrich($card); return new CardDetails($card); }, $cards diff --git a/tests/integration/base-query-count.txt b/tests/integration/base-query-count.txt new file mode 100644 index 000000000..bd7d9524a --- /dev/null +++ b/tests/integration/base-query-count.txt @@ -0,0 +1 @@ +61324 diff --git a/tests/integration/features/bootstrap/ServerContext.php b/tests/integration/features/bootstrap/ServerContext.php index 6880f764c..f7c192384 100644 --- a/tests/integration/features/bootstrap/ServerContext.php +++ b/tests/integration/features/bootstrap/ServerContext.php @@ -40,7 +40,6 @@ class ServerContext implements Context { } public function getCookieJar(): CookieJar { - echo $this->currentUser; return $this->cookieJar; } diff --git a/tests/integration/run.sh b/tests/integration/run.sh index bcabf9ed7..1dfe35b70 100755 --- a/tests/integration/run.sh +++ b/tests/integration/run.sh @@ -27,16 +27,16 @@ if [ -z "$EXECUTOR_NUMBER" ]; then fi PORT=$((9090 + $EXECUTOR_NUMBER)) echo $PORT -php -S localhost:$PORT -t $OC_PATH & +php -q -S localhost:$PORT -t $OC_PATH & PHPPID=$! echo $PHPPID export TEST_SERVER_URL="http://localhost:$PORT/ocs/" -vendor/bin/behat $SCENARIO_TO_RUN +vendor/bin/behat --colors $SCENARIO_TO_RUN RESULT=$? -kill $PHPPID +kill -9 $PHPPID echo "runsh: Exit code: $RESULT" exit $RESULT diff --git a/tests/unit/Db/UserTest.php b/tests/unit/Db/UserTest.php index 8f7e84816..290331f6d 100644 --- a/tests/unit/Db/UserTest.php +++ b/tests/unit/Db/UserTest.php @@ -24,6 +24,7 @@ namespace OCA\Deck\Db; use OCP\IUser; +use OCP\IUserManager; class UserTest extends \Test\TestCase { public function testGroupObjectSerialize() { @@ -35,7 +36,11 @@ class UserTest extends \Test\TestCase { $user->expects($this->any()) ->method('getDisplayName') ->willReturn('myuser displayname'); - $userRelationalObject = new User($user); + $userManager = $this->createMock(IUserManager::class); + $userManager->expects($this->any()) + ->method('get') + ->willReturn($user); + $userRelationalObject = new User('myuser', $userManager); $expected = [ 'uid' => 'myuser', 'displayname' => 'myuser displayname', @@ -53,7 +58,11 @@ class UserTest extends \Test\TestCase { $user->expects($this->any()) ->method('getDisplayName') ->willReturn('myuser displayname'); - $userRelationalObject = new User($user); + $userManager = $this->createMock(IUserManager::class); + $userManager->expects($this->any()) + ->method('get') + ->willReturn($user); + $userRelationalObject = new User('myuser', $userManager); $expected = [ 'uid' => 'myuser', 'displayname' => 'myuser displayname', diff --git a/tests/unit/Notification/NotificationHelperTest.php b/tests/unit/Notification/NotificationHelperTest.php index c4f0fd9cd..4e0803dcd 100644 --- a/tests/unit/Notification/NotificationHelperTest.php +++ b/tests/unit/Notification/NotificationHelperTest.php @@ -37,6 +37,7 @@ use OCP\IConfig; use OCP\IGroup; use OCP\IGroupManager; use OCP\IUser; +use OCP\IUserManager; use OCP\Notification\IManager; use OCP\Notification\INotification; use PHPUnit\Framework\MockObject\MockObject; @@ -219,8 +220,9 @@ class NotificationHelperTest extends \Test\TestCase { 'title' => 'MyCardTitle', 'duedate' => '2020-12-24' ]); + $userManager = $this->createMock(IUserManager::class); $card->setAssignedUsers([ - new User($users[0]) + new User($users[0]->getUID(), $userManager) ]); $this->cardMapper->expects($this->once()) ->method('findBoardId') @@ -308,8 +310,9 @@ class NotificationHelperTest extends \Test\TestCase { 'title' => 'MyCardTitle', 'duedate' => '2020-12-24' ]); + $userManager = $this->createMock(IUserManager::class); $card->setAssignedUsers([ - new User($users[0]) + new User($users[0]->getUID(), $userManager) ]); $this->cardMapper->expects($this->once()) ->method('findBoardId') diff --git a/tests/unit/Service/CardServiceTest.php b/tests/unit/Service/CardServiceTest.php index 8c8691977..1952747b3 100644 --- a/tests/unit/Service/CardServiceTest.php +++ b/tests/unit/Service/CardServiceTest.php @@ -24,6 +24,7 @@ namespace OCA\Deck\Service; use OCA\Deck\Activity\ActivityManager; +use OCA\Deck\Db\Assignment; use OCA\Deck\Db\AssignmentMapper; use OCA\Deck\Db\Board; use OCA\Deck\Db\Card; @@ -155,7 +156,8 @@ class CardServiceTest extends TestCase { ->method('getNumberOfCommentsForObject') ->willReturn(0); $boardMock = $this->createMock(Board::class); - $stackMock = $this->createMock(Stack::class); + $stackMock = new Stack(); + $stackMock->setBoardId(1234); $this->stackMapper->expects($this->any()) ->method('find') ->willReturn($stackMock); @@ -168,13 +170,21 @@ class CardServiceTest extends TestCase { ->method('find') ->with(123) ->willReturn($card); + $a1 = new Assignment(); + $a1->setCardId(1337); + $a1->setType(0); + $a1->setParticipant('user1'); + $a2 = new Assignment(); + $a2->setCardId(1337); + $a2->setType(0); + $a2->setParticipant('user2'); $this->assignedUsersMapper->expects($this->any()) - ->method('findAll') - ->with(1337) - ->willReturn(['user1', 'user2']); + ->method('findIn') + ->with([1337]) + ->willReturn([$a1, $a2]); $cardExpected = new Card(); $cardExpected->setId(1337); - $cardExpected->setAssignedUsers(['user1', 'user2']); + $cardExpected->setAssignedUsers([$a1, $a2]); $cardExpected->setRelatedBoard($boardMock); $cardExpected->setRelatedStack($stackMock); $cardExpected->setLabels([]); diff --git a/tests/unit/Service/DefaultBoardServiceTest.php b/tests/unit/Service/DefaultBoardServiceTest.php index 804bb6226..2e38c83bc 100644 --- a/tests/unit/Service/DefaultBoardServiceTest.php +++ b/tests/unit/Service/DefaultBoardServiceTest.php @@ -83,10 +83,6 @@ class DefaultBoardServiceTest extends TestCase { ->method('getUserValue') ->willReturn('yes'); - $this->boardMapper->expects($this->once()) - ->method('findAllByUser') - ->willReturn($userBoards); - $this->config->expects($this->once()) ->method('setUserValue'); @@ -107,10 +103,6 @@ class DefaultBoardServiceTest extends TestCase { ->method('getUserValue') ->willReturn('no'); - $this->boardMapper->expects($this->once()) - ->method('findAllByUser') - ->willReturn($userBoards); - $result = $this->service->checkFirstRun($this->userId); $this->assertEquals($result, false); } diff --git a/tests/unit/Service/Importer/Systems/TrelloJsonServiceTest.php b/tests/unit/Service/Importer/Systems/TrelloJsonServiceTest.php index 46c0799d1..a1e18b11c 100644 --- a/tests/unit/Service/Importer/Systems/TrelloJsonServiceTest.php +++ b/tests/unit/Service/Importer/Systems/TrelloJsonServiceTest.php @@ -30,6 +30,9 @@ use OCP\IUserManager; use OCP\Server; use PHPUnit\Framework\MockObject\MockObject; +/** + * @group DB + */ class TrelloJsonServiceTest extends \Test\TestCase { private TrelloJsonService $service; /** @var IURLGenerator|MockObject */ @@ -57,7 +60,7 @@ class TrelloJsonServiceTest extends \Test\TestCase { } public function testValidateUsersWithInvalidUser() { - $this->expectErrorMessage('Trello user trello_user not found in property "members" of json data'); + $this->expectExceptionMessage('Trello user trello_user not found in property "members" of json data'); $importService = $this->createMock(BoardImportService::class); $importService ->method('getConfig') diff --git a/tests/unit/Service/PermissionServiceTest.php b/tests/unit/Service/PermissionServiceTest.php index 0994a778e..d0d236b2e 100644 --- a/tests/unit/Service/PermissionServiceTest.php +++ b/tests/unit/Service/PermissionServiceTest.php @@ -236,6 +236,11 @@ class PermissionServiceTest extends \Test\TestCase { $board->setAcl($this->getAcls($boardId)); $this->boardMapper->expects($this->any())->method('find')->willReturn($board); + $this->aclMapper->expects($this->any()) + ->method('findAll') + ->with($boardId) + ->willReturn($this->getAcls($boardId)); + $this->shareManager->expects($this->any()) ->method('sharingDisabledForUser') ->willReturn(false); @@ -262,12 +267,17 @@ class PermissionServiceTest extends \Test\TestCase { $this->boardMapper->expects($this->any())->method('find')->willReturn($board); } + $this->aclMapper->expects($this->any()) + ->method('findAll') + ->with($boardId) + ->willReturn($this->getAcls($boardId)); + if ($result) { - $actual = $this->service->checkPermission($mapper, 1234, $permission); + $actual = $this->service->checkPermission($mapper, $boardId, $permission); $this->assertTrue($actual); } else { $this->expectException(NoPermissionException::class); - $this->service->checkPermission($mapper, 1234, $permission); + $this->service->checkPermission($mapper, $boardId, $permission); } } @@ -340,7 +350,7 @@ class PermissionServiceTest extends \Test\TestCase { $aclGroup->setParticipant('group1'); $board = $this->createMock(Board::class); - $board->expects($this->once()) + $board->expects($this->any()) ->method('__call') ->with('getOwner', []) ->willReturn('user1'); @@ -352,8 +362,8 @@ class PermissionServiceTest extends \Test\TestCase { ->method('find') ->with(123) ->willReturn($board); - $this->userManager->expects($this->exactly(2)) - ->method('get') + $this->userManager->expects($this->any()) + ->method('userExists') ->withConsecutive(['user1'], ['user2']) ->willReturnOnConsecutiveCalls($user1, $user2); @@ -367,9 +377,9 @@ class PermissionServiceTest extends \Test\TestCase { ->willReturn($group); $users = $this->service->findUsers(123); $this->assertEquals([ - 'user1' => new User($user1), - 'user2' => new User($user2), - 'user3' => new User($user3), + 'user1' => new User($user1->getUID(), $this->userManager), + 'user2' => new User($user2->getUID(), $this->userManager), + 'user3' => new User($user3->getUID(), $this->userManager), ], $users); } } diff --git a/tests/unit/Service/StackServiceTest.php b/tests/unit/Service/StackServiceTest.php index af0a720ac..78737ab68 100644 --- a/tests/unit/Service/StackServiceTest.php +++ b/tests/unit/Service/StackServiceTest.php @@ -110,10 +110,12 @@ class StackServiceTest extends TestCase { public function testFindAll() { $this->permissionService->expects($this->once())->method('checkPermission'); $this->stackMapper->expects($this->once())->method('findAll')->willReturn($this->getStacks()); - $this->cardService->expects($this->atLeastOnce())->method('enrich')->will( + $this->cardService->expects($this->atLeastOnce())->method('enrichCards')->will( $this->returnCallback( - function ($card) { - $card->setLabels($this->getLabels()[$card->getId()]); + function ($cards) { + foreach ($cards as $card) { + $card->setLabels($this->getLabels()[$card->getId()]); + } } ) ); diff --git a/tests/unit/controller/BoardControllerTest.php b/tests/unit/controller/BoardControllerTest.php index fdb0ae203..357219907 100644 --- a/tests/unit/controller/BoardControllerTest.php +++ b/tests/unit/controller/BoardControllerTest.php @@ -24,6 +24,7 @@ namespace OCA\Deck\Controller; use OCA\Deck\Db\Acl; +use OCA\Deck\Db\Board; use OCP\IUser; class BoardControllerTest extends \Test\TestCase { @@ -88,11 +89,12 @@ class BoardControllerTest extends \Test\TestCase { } public function testRead() { + $board = new Board(); $this->boardService->expects($this->once()) ->method('find') ->with(123) - ->willReturn(1); - $this->assertEquals(1, $this->controller->read(123)); + ->willReturn($board); + $this->assertEquals($board, $this->controller->read(123)); } public function testCreate() {