diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon index 9b8ac82db4..24c78f3e52 100644 --- a/phpstan-baseline.neon +++ b/phpstan-baseline.neon @@ -32220,12 +32220,6 @@ parameters: count: 1 path: tests/integration/Core/Repository/BaseURLServiceTest.php - - - message: '#^Call to method PHPUnit\\Framework\\Assert\:\:assertInstanceOf\(\) with ''Ibexa\\\\Contracts\\\\Core\\\\Repository\\\\Values\\\\Bookmark\\\\BookmarkList'' and Ibexa\\Contracts\\Core\\Repository\\Values\\Bookmark\\BookmarkList will always evaluate to true\.$#' - identifier: method.alreadyNarrowedType - count: 1 - path: tests/integration/Core/Repository/BookmarkServiceTest.php - - message: '#^Method Ibexa\\Tests\\Integration\\Core\\Repository\\BookmarkServiceTest\:\:testCreateBookmark\(\) has no return type specified\.$#' identifier: missingType.return @@ -69499,12 +69493,6 @@ parameters: count: 1 path: tests/lib/Repository/Service/Mock/BookmarkTest.php - - - message: '#^Method Ibexa\\Tests\\Core\\Repository\\Service\\Mock\\BookmarkTest\:\:testLoadBookmarksEmptyList\(\) has no return type specified\.$#' - identifier: missingType.return - count: 1 - path: tests/lib/Repository/Service/Mock/BookmarkTest.php - - message: '#^Method Ibexa\\Tests\\Core\\Repository\\Service\\Mock\\BookmarkTest\:\:testLocationShouldBeBookmarked\(\) has no return type specified\.$#' identifier: missingType.return diff --git a/src/contracts/Persistence/Bookmark/Handler.php b/src/contracts/Persistence/Bookmark/Handler.php index f9ef1a1cae..dd5ea7dcf5 100644 --- a/src/contracts/Persistence/Bookmark/Handler.php +++ b/src/contracts/Persistence/Bookmark/Handler.php @@ -50,6 +50,8 @@ public function loadUserIdsByLocation(Location $location): array; /** * Loads bookmarks owned by user. * + * @deprecated 4.6.32 The "Handler::loadUserBookmarks()" method is deprecated, will be removed in 6.0.0. Use "LocationService::find()" and "Criterion\Location\IsBookmarked" instead. + * * @param int $userId * @param int $offset the start offset for paging * @param int $limit the number of bookmarked locations returned @@ -61,6 +63,8 @@ public function loadUserBookmarks(int $userId, int $offset = 0, int $limit = -1) /** * Count bookmarks owned by user. * + * @deprecated 4.6.32 The "Handler::countUserBookmarks()" method is deprecated, will be removed in 6.0.0. Use "LocationService::count()" and "Criterion\Location\IsBookmarked" instead. + * * @param int $userId * * @return int diff --git a/src/contracts/Repository/Values/Content/Query/Criterion/Location/IsBookmarked.php b/src/contracts/Repository/Values/Content/Query/Criterion/Location/IsBookmarked.php index 36d7972270..af06a4bbc8 100644 --- a/src/contracts/Repository/Values/Content/Query/Criterion/Location/IsBookmarked.php +++ b/src/contracts/Repository/Values/Content/Query/Criterion/Location/IsBookmarked.php @@ -15,6 +15,8 @@ /** * This criterion only works for current user reference. + * + * When used with Content filtering, it matches bookmarks placed on main Locations only. */ final class IsBookmarked extends Location implements FilteringCriterion { diff --git a/src/contracts/Repository/Values/Content/Query/SortClause/Location/Bookmark/Id.php b/src/contracts/Repository/Values/Content/Query/SortClause/Location/Bookmark/Id.php new file mode 100644 index 0000000000..29cb02d232 --- /dev/null +++ b/src/contracts/Repository/Values/Content/Query/SortClause/Location/Bookmark/Id.php @@ -0,0 +1,24 @@ +permissionResolver = $permissionResolver; + } + + public function accepts(FilteringCriterion $criterion): bool + { + return $criterion instanceof IsBookmarked; + } + + /** + * @param \Ibexa\Contracts\Core\Repository\Values\Content\Query\Criterion\Location\IsBookmarked $criterion + * + * @throws \Ibexa\Contracts\Core\Repository\Exceptions\InvalidArgumentException + */ + public function buildQueryConstraint( + FilteringQueryBuilder $queryBuilder, + FilteringCriterion $criterion + ): string { + parent::buildQueryConstraint($queryBuilder, $criterion); + + $isBookmarked = $criterion->value[0] ?? null; + if (!is_bool($isBookmarked)) { + throw new InvalidArgumentException( + '$criterion', + 'IsBookmarked criterion value must be boolean at index 0.' + ); + } + + $userId = $this->permissionResolver->getCurrentUserReference()->getUserId(); + + $subQueryBuilder = new QueryBuilder($queryBuilder->getConnection()); + $subQueryBuilder + ->select('1') + ->from(DoctrineDatabase::TABLE_BOOKMARKS, self::ALIAS) + ->where( + $subQueryBuilder->expr()->eq( + self::ALIAS . '.' . DoctrineDatabase::COLUMN_USER_ID, + $queryBuilder->createNamedParameter($userId, ParameterType::INTEGER) + ), + $subQueryBuilder->expr()->eq( + self::ALIAS . '.' . DoctrineDatabase::COLUMN_LOCATION_ID, + 'location.node_id' + ) + ); + + return sprintf( + $isBookmarked ? 'EXISTS (%s)' : 'NOT EXISTS (%s)', + $subQueryBuilder->getSQL() + ); + } +} diff --git a/src/lib/Persistence/Legacy/Filter/SortClauseQueryBuilder/Location/BaseLocationSortClauseQueryBuilder.php b/src/lib/Persistence/Legacy/Filter/SortClauseQueryBuilder/Location/BaseLocationSortClauseQueryBuilder.php index 2932835436..f9572a8e8d 100644 --- a/src/lib/Persistence/Legacy/Filter/SortClauseQueryBuilder/Location/BaseLocationSortClauseQueryBuilder.php +++ b/src/lib/Persistence/Legacy/Filter/SortClauseQueryBuilder/Location/BaseLocationSortClauseQueryBuilder.php @@ -28,18 +28,29 @@ public function buildQuery( $locationContext = $this->prepareLocationContext($queryBuilder); $locationAlias = $locationContext['alias']; - $sort = $this->getSortingExpressionForAlias($locationAlias); - $sortAlias = $this->getSortFieldAlias($sort); - $queryBuilder->addSelect(sprintf('%s AS %s', $sort, $sortAlias)); - if ($locationContext['needsMainLocationJoin']) { $this->joinMainLocationOnly($queryBuilder, $locationAlias); } + $this->joinAdditionalTables($queryBuilder, $locationAlias); + + $sort = $this->getSortingExpressionForAlias($locationAlias); + $sortAlias = $this->getSortFieldAlias($sort); + $queryBuilder->addSelect(sprintf('%s AS %s', $sort, $sortAlias)); + /** @var \Ibexa\Contracts\Core\Repository\Values\Content\Query\SortClause $sortClause */ $queryBuilder->addOrderBy($sortAlias, $sortClause->direction); } + /** + * Hook for sort clauses which need to join further tables against the resolved Location alias. + */ + protected function joinAdditionalTables( + FilteringQueryBuilder $queryBuilder, + string $locationAlias + ): void { + } + /** * @return array{alias: string, needsMainLocationJoin: bool} */ diff --git a/src/lib/Persistence/Legacy/Filter/SortClauseQueryBuilder/Location/Bookmark/IdSortClauseQueryBuilder.php b/src/lib/Persistence/Legacy/Filter/SortClauseQueryBuilder/Location/Bookmark/IdSortClauseQueryBuilder.php new file mode 100644 index 0000000000..b3d2f57822 --- /dev/null +++ b/src/lib/Persistence/Legacy/Filter/SortClauseQueryBuilder/Location/Bookmark/IdSortClauseQueryBuilder.php @@ -0,0 +1,95 @@ +permissionResolver = $permissionResolver; + } + + public function accepts(FilteringSortClause $sortClause): bool + { + return $sortClause instanceof Id; + } + + /** + * @throws \Ibexa\Contracts\Core\Repository\Exceptions\InvalidArgumentException + */ + public function buildQuery( + FilteringQueryBuilder $queryBuilder, + FilteringSortClause $sortClause + ): void { + if (!$sortClause instanceof Id) { + throw new InvalidArgumentException( + '$sortClause', + sprintf('Expected %s, got %s', Id::class, get_class($sortClause)) + ); + } + + parent::buildQuery($queryBuilder, $sortClause); + } + + protected function joinAdditionalTables( + FilteringQueryBuilder $queryBuilder, + string $locationAlias + ): void { + $userId = $this->permissionResolver->getCurrentUserReference()->getUserId(); + + $queryBuilder->leftJoinOnce( + $locationAlias, + DoctrineDatabase::TABLE_BOOKMARKS, + self::ALIAS, + (string)$queryBuilder->expr()->and( + sprintf( + '%s.node_id = %s.%s', + $locationAlias, + self::ALIAS, + DoctrineDatabase::COLUMN_LOCATION_ID + ), + $queryBuilder->expr()->eq( + sprintf('%s.%s', self::ALIAS, DoctrineDatabase::COLUMN_USER_ID), + $queryBuilder->createNamedParameter($userId, ParameterType::INTEGER) + ) + ) + ); + } + + protected function getSortingExpression(): string + { + return self::ALIAS . '.' . DoctrineDatabase::COLUMN_ID; + } + + protected function getSortingExpressionForAlias(string $locationAlias): string + { + return $this->getSortingExpression(); + } + + protected function getSortFieldName(string $sortExpression): string + { + return 'bookmark_id'; + } +} diff --git a/src/lib/Repository/BookmarkService.php b/src/lib/Repository/BookmarkService.php index a74c79d5cb..e838766509 100644 --- a/src/lib/Repository/BookmarkService.php +++ b/src/lib/Repository/BookmarkService.php @@ -9,33 +9,34 @@ namespace Ibexa\Core\Repository; use Exception; -use Ibexa\Contracts\Core\Persistence\Bookmark\Bookmark; use Ibexa\Contracts\Core\Persistence\Bookmark\CreateStruct; use Ibexa\Contracts\Core\Persistence\Bookmark\Handler as BookmarkHandler; use Ibexa\Contracts\Core\Repository\BookmarkService as BookmarkServiceInterface; +use Ibexa\Contracts\Core\Repository\Exceptions\Exception as RepositoryException; use Ibexa\Contracts\Core\Repository\Repository as RepositoryInterface; use Ibexa\Contracts\Core\Repository\Values\Bookmark\BookmarkList; use Ibexa\Contracts\Core\Repository\Values\Content\Location; +use Ibexa\Contracts\Core\Repository\Values\Content\Query; +use Ibexa\Contracts\Core\Repository\Values\Content\Query\Criterion; +use Ibexa\Contracts\Core\Repository\Values\Content\Query\SortClause; +use Ibexa\Contracts\Core\Repository\Values\Filter\Filter; use Ibexa\Core\Base\Exceptions\InvalidArgumentException; +use Psr\Log\LoggerInterface; +use Psr\Log\NullLogger; class BookmarkService implements BookmarkServiceInterface { - /** @var \Ibexa\Contracts\Core\Repository\Repository */ - protected $repository; + protected RepositoryInterface $repository; - /** @var \Ibexa\Contracts\Core\Persistence\Bookmark\Handler */ - protected $bookmarkHandler; + protected BookmarkHandler $bookmarkHandler; - /** - * BookmarkService constructor. - * - * @param \Ibexa\Contracts\Core\Repository\Repository $repository - * @param \Ibexa\Contracts\Core\Persistence\Bookmark\Handler $bookmarkHandler - */ - public function __construct(RepositoryInterface $repository, BookmarkHandler $bookmarkHandler) + private LoggerInterface $logger; + + public function __construct(RepositoryInterface $repository, BookmarkHandler $bookmarkHandler, ?LoggerInterface $logger = null) { $this->repository = $repository; $this->bookmarkHandler = $bookmarkHandler; + $this->logger = $logger ?? new NullLogger(); } /** @@ -95,17 +96,25 @@ public function deleteBookmark(Location $location): void */ public function loadBookmarks(int $offset = 0, int $limit = 25): BookmarkList { - $currentUserId = $this->getCurrentUserId(); + $filter = new Filter(); + try { + $filter + ->withCriterion(new Criterion\Location\IsBookmarked()) + ->withSortClause(new SortClause\Location\Bookmark\Id(Query::SORT_DESC)) + ->sliceBy($limit, $offset); + + $result = $this->repository->getLocationService()->find($filter, []); + } catch (RepositoryException $e) { + $this->logger->error($e->getMessage(), [ + 'exception' => $e, + ]); + + return new BookmarkList(); + } $list = new BookmarkList(); - $list->totalCount = $this->bookmarkHandler->countUserBookmarks($currentUserId); - if ($list->totalCount > 0) { - $bookmarks = $this->bookmarkHandler->loadUserBookmarks($currentUserId, $offset, $limit); - - $list->items = array_map(function (Bookmark $bookmark) { - return $this->repository->getLocationService()->loadLocation($bookmark->locationId); - }, $bookmarks); - } + $list->totalCount = $result->totalCount; + $list->items = iterator_to_array($result->getIterator()); return $list; } diff --git a/src/lib/Repository/Repository.php b/src/lib/Repository/Repository.php index fa165c2222..81fccd268d 100644 --- a/src/lib/Repository/Repository.php +++ b/src/lib/Repository/Repository.php @@ -608,7 +608,8 @@ public function getBookmarkService(): BookmarkServiceInterface if ($this->bookmarkService === null) { $this->bookmarkService = new BookmarkService( $this, - $this->persistenceHandler->bookmarkHandler() + $this->persistenceHandler->bookmarkHandler(), + $this->logger ); } diff --git a/tests/integration/Core/Repository/BookmarkServiceTest.php b/tests/integration/Core/Repository/BookmarkServiceTest.php index b4d1542d59..cfb194935a 100644 --- a/tests/integration/Core/Repository/BookmarkServiceTest.php +++ b/tests/integration/Core/Repository/BookmarkServiceTest.php @@ -8,8 +8,15 @@ namespace Ibexa\Tests\Integration\Core\Repository; +use Doctrine\DBAL\Connection; +use Doctrine\DBAL\ParameterType; use Ibexa\Contracts\Core\Repository\Exceptions\InvalidArgumentException; -use Ibexa\Contracts\Core\Repository\Values\Bookmark\BookmarkList; +use Ibexa\Contracts\Core\Repository\Values\Content\Content; +use Ibexa\Contracts\Core\Repository\Values\Content\Location; +use Ibexa\Contracts\Core\Repository\Values\Content\Query\Criterion; +use Ibexa\Contracts\Core\Repository\Values\Filter\Filter; +use Ibexa\Contracts\Core\Repository\Values\User\Limitation\SectionLimitation; +use Ibexa\Core\Persistence\Legacy\Bookmark\Gateway\DoctrineDatabase; /** * Test case for the BookmarkService. @@ -21,9 +28,6 @@ class BookmarkServiceTest extends BaseTest public const LOCATION_ID_BOOKMARKED = 5; public const LOCATION_ID_NOT_BOOKMARKED = 44; - /** - * @covers \Ibexa\Contracts\Core\Repository\BookmarkService::isBookmarked - */ public function testIsBookmarked() { $repository = $this->getRepository(); @@ -36,9 +40,6 @@ public function testIsBookmarked() $this->assertTrue($isBookmarked); } - /** - * @covers \Ibexa\Contracts\Core\Repository\BookmarkService::isBookmarked - */ public function testIsNotBookmarked() { $repository = $this->getRepository(); @@ -51,9 +52,6 @@ public function testIsNotBookmarked() $this->assertFalse($isBookmarked); } - /** - * @covers \Ibexa\Contracts\Core\Repository\BookmarkService::createBookmark - */ public function testCreateBookmark() { $repository = $this->getRepository(); @@ -73,7 +71,6 @@ public function testCreateBookmark() } /** - * @covers \Ibexa\Contracts\Core\Repository\BookmarkService::createBookmark * @depends testCreateBookmark */ public function testCreateBookmarkThrowsInvalidArgumentException() @@ -91,9 +88,6 @@ public function testCreateBookmarkThrowsInvalidArgumentException() /* END: Use Case */ } - /** - * @covers \Ibexa\Contracts\Core\Repository\BookmarkService::deleteBookmark - */ public function testDeleteBookmark() { $repository = $this->getRepository(); @@ -114,7 +108,6 @@ public function testDeleteBookmark() } /** - * @covers \Ibexa\Contracts\Core\Repository\BookmarkService::deleteBookmark * @depends testDeleteBookmark */ public function testDeleteBookmarkThrowsInvalidArgumentException() @@ -132,9 +125,6 @@ public function testDeleteBookmarkThrowsInvalidArgumentException() /* END: Use Case */ } - /** - * @covers \Ibexa\Contracts\Core\Repository\BookmarkService::loadBookmarks - */ public function testLoadBookmarks() { $repository = $this->getRepository(); @@ -143,13 +133,198 @@ public function testLoadBookmarks() $bookmarks = $repository->getBookmarkService()->loadBookmarks(1, 3); /* END: Use Case */ - $this->assertInstanceOf(BookmarkList::class, $bookmarks); - $this->assertEquals($bookmarks->totalCount, 5); + self::assertEquals(5, $bookmarks->totalCount); // Assert bookmarks order: recently added should be first - $this->assertEquals([15, 13, 12], array_map(static function ($location) { + self::assertEquals([15, 13, 12], array_map(static function ($location) { return $location->id; }, $bookmarks->items)); } + + public function testCountBookmarks(): void + { + $repository = $this->getRepository(); + + $filter = new Filter(); + $filter + ->withCriterion(new Criterion\Location\IsBookmarked(true)); + $count = $repository->getLocationService()->count($filter, []); + + self::assertEquals(5, $count); + } + + /** + * Regression test for IBX-6773: bookmarking an item and then losing read access to it used to + * make the whole bookmark list explode with an UnauthorizedException, because every bookmark + * was resolved through LocationService::loadLocation(). The item must simply be filtered out + * instead. + */ + public function testLoadBookmarksSkipsBookmarksUserLostAccessTo(): void + { + $repository = $this->getRepository(); + $sectionService = $repository->getSectionService(); + $permissionResolver = $repository->getPermissionResolver(); + $bookmarkService = $repository->getBookmarkService(); + + $administratorUser = $permissionResolver->getCurrentUserReference(); + + // A section the restricted user will *not* be allowed to read + $sectionCreateStruct = $sectionService->newSectionCreateStruct(); + $sectionCreateStruct->name = 'Restricted'; + $sectionCreateStruct->identifier = 'restricted_bookmarks'; + $restrictedSection = $sectionService->createSection($sectionCreateStruct); + + // Created as administrator, so it lands in the Standard section (ID 1) + $folder = $this->createFolder(['eng-GB' => 'Bookmarked folder'], 2); + $folderLocationId = $folder->getContentInfo()->getMainLocationId(); + self::assertNotNull($folderLocationId); + + // User may only read content in the Standard section + $user = $this->createUserWithPolicies( + 'bookmark_section_limited', + [ + [ + 'module' => 'content', + 'function' => 'read', + 'limitations' => [new SectionLimitation(['limitationValues' => [1]])], + ], + ] + ); + + $permissionResolver->setCurrentUserReference($user); + + $bookmarkService->createBookmark( + $repository->getLocationService()->loadLocation($folderLocationId) + ); + + // Sanity check: while readable, the bookmark shows up + $bookmarks = $bookmarkService->loadBookmarks(); + self::assertSame(1, $bookmarks->totalCount); + self::assertSame( + [$folderLocationId], + array_map( + static function (Location $location): int { + return $location->getId(); + }, + $bookmarks->items + ) + ); + + // Move the bookmarked item out of reach of the user + $permissionResolver->setCurrentUserReference($administratorUser); + $sectionService->assignSection($folder->getContentInfo(), $restrictedSection); + + $permissionResolver->setCurrentUserReference($user); + + // Used to throw UnauthorizedException + $bookmarksAfterLosingAccess = $bookmarkService->loadBookmarks(); + + self::assertSame( + 0, + $bookmarksAfterLosingAccess->totalCount, + 'Bookmark of a no longer readable item should not be counted' + ); + self::assertSame( + [], + $bookmarksAfterLosingAccess->items, + 'Bookmark of a no longer readable item should not be listed' + ); + } + + public function testLoadBookmarksAfterTrashingBookmarkedLocation(): void + { + $repository = $this->getRepository(); + $bookmarkService = $repository->getBookmarkService(); + + $folder = $this->createFolder(['eng-GB' => 'Folder to be trashed'], 2); + $location = $this->loadMainLocation($folder); + + $bookmarkService->createBookmark($location); + self::assertBookmarkRowCount(1, $location->getId(), $this->getRawDatabaseConnection()); + + $repository->getTrashService()->trash($location); + + $this->assertBookmarkGone($location->getId()); + } + + public function testLoadBookmarksAfterDeletingBookmarkedContent(): void + { + $repository = $this->getRepository(); + $bookmarkService = $repository->getBookmarkService(); + + $folder = $this->createFolder(['eng-GB' => 'Folder to be deleted'], 2); + $location = $this->loadMainLocation($folder); + + $bookmarkService->createBookmark($location); + self::assertBookmarkRowCount(1, $location->getId(), $this->getRawDatabaseConnection()); + + $repository->getContentService()->deleteContent($folder->getContentInfo()); + + $this->assertBookmarkGone($location->getId()); + } + + /** + * @throws \Ibexa\Contracts\Core\Repository\Exceptions\NotFoundException + * @throws \Ibexa\Contracts\Core\Repository\Exceptions\UnauthorizedException + */ + private function loadMainLocation(Content $content): Location + { + $mainLocationId = $content->getContentInfo()->getMainLocationId(); + self::assertNotNull($mainLocationId); + + return $this->getRepository()->getLocationService()->loadLocation($mainLocationId); + } + + /** + * Asserts both that the bookmark is no longer listed and that its row is actually gone. + * + * @throws \Doctrine\DBAL\DBALException + * @throws \ErrorException + */ + private function assertBookmarkGone(int $locationId): void + { + $bookmarks = $this->getRepository()->getBookmarkService()->loadBookmarks(0, 9999); + + foreach ($bookmarks as $bookmarkedLocation) { + self::assertNotEquals( + $locationId, + $bookmarkedLocation->getId(), + 'Bookmark of a removed Location should not be listed' + ); + } + + self::assertBookmarkRowCount(0, $locationId, $this->getRawDatabaseConnection()); + } + + /** + * @throws \Doctrine\DBAL\DBALException + */ + private static function assertBookmarkRowCount( + int $expectedCount, + int $locationId, + Connection $connection + ): void { + $query = $connection->createQueryBuilder(); + $query + ->select('COUNT(' . DoctrineDatabase::COLUMN_ID . ')') + ->from(DoctrineDatabase::TABLE_BOOKMARKS) + ->where( + $query->expr()->eq( + DoctrineDatabase::COLUMN_LOCATION_ID, + $query->createNamedParameter($locationId, ParameterType::INTEGER) + ) + ); + + self::assertSame( + $expectedCount, + (int)$query->execute()->fetchColumn(), + sprintf( + 'Expected %d "%s" row(s) for Location %d', + $expectedCount, + DoctrineDatabase::TABLE_BOOKMARKS, + $locationId + ) + ); + } } class_alias(BookmarkServiceTest::class, 'eZ\Publish\API\Repository\Tests\BookmarkServiceTest'); diff --git a/tests/integration/Core/Repository/Filtering/ContentFilteringTest.php b/tests/integration/Core/Repository/Filtering/ContentFilteringTest.php index a5a490520c..cc16341f21 100644 --- a/tests/integration/Core/Repository/Filtering/ContentFilteringTest.php +++ b/tests/integration/Core/Repository/Filtering/ContentFilteringTest.php @@ -33,6 +33,69 @@ */ final class ContentFilteringTest extends BaseRepositoryFilteringTestCase { + private const BOOKMARKED_LOCATION_ID = 52; + + /** + * @return iterable + */ + public function isBookmarkedProvider(): iterable + { + // [isBookmarkedCriterion, initialCount, afterCreateCount, afterDeleteCount] + yield 'bookmarked=true' => [true, 0, 1, 0]; + yield 'bookmarked=false' => [false, 1, 0, 1]; + } + + /** + * @dataProvider isBookmarkedProvider + */ + public function testIsBookmarkedTrueAndFalse( + bool $isBookmarked, + int $initialCount, + int $afterCreateCount, + int $afterDeleteCount + ): void { + $repository = $this->getRepository(false); + $locationService = $repository->getLocationService(); + $contentService = $repository->getContentService(); + $bookmarkService = $repository->getBookmarkService(); + + $bookmarkedLocation = $locationService->loadLocation(self::BOOKMARKED_LOCATION_ID); + + $filter = new Filter(); + $filter->withCriterion(new Criterion\Location\IsBookmarked($isBookmarked)) + ->andWithCriterion(new Criterion\LocationId(self::BOOKMARKED_LOCATION_ID)); + + self::assertCount( + $initialCount, + $contentService->find($filter), + 'Unexpected initial bookmark state for IsBookmarked(' . ($isBookmarked ? 'true' : 'false') . ')' + ); + + $bookmarkService->createBookmark($bookmarkedLocation); + + $filter = new Filter(); + $filter->withCriterion(new Criterion\Location\IsBookmarked($isBookmarked)) + ->andWithCriterion(new Criterion\LocationId(self::BOOKMARKED_LOCATION_ID)); + + self::assertCount( + $afterCreateCount, + $contentService->find($filter), + 'Unexpected state after creating bookmark for IsBookmarked(' . ($isBookmarked ? 'true' : 'false') . ')' + ); + + $bookmarkService->deleteBookmark($bookmarkedLocation); + + $filter = new Filter(); + $filter->withCriterion(new Criterion\Location\IsBookmarked($isBookmarked)) + ->andWithCriterion(new Criterion\LocationId(self::BOOKMARKED_LOCATION_ID)); + + self::assertCount( + $afterDeleteCount, + $contentService->find($filter), + 'Unexpected state after deleting bookmark for IsBookmarked(' . ($isBookmarked ? 'true' : 'false') . ')' + ); + } + /** * Test that special cases of Location Sort Clauses are working correctly. * diff --git a/tests/integration/Core/Repository/Filtering/LocationFilteringTest.php b/tests/integration/Core/Repository/Filtering/LocationFilteringTest.php index f5d165f34d..f27807032b 100644 --- a/tests/integration/Core/Repository/Filtering/LocationFilteringTest.php +++ b/tests/integration/Core/Repository/Filtering/LocationFilteringTest.php @@ -22,6 +22,134 @@ */ final class LocationFilteringTest extends BaseRepositoryFilteringTestCase { + private const BOOKMARKED_LOCATION_ID = 52; + + public function testBookmarkedAndNotBookmarkedCountsMatchTotal(): void + { + $locationService = $this->getRepository(false)->getLocationService(); + + $baseFilter = new Filter(); + $totalCount = $locationService->count($baseFilter); + + $bookmarkedFilter = clone $baseFilter; + $bookmarkedFilter->withCriterion(new Criterion\Location\IsBookmarked(true)); + $bookmarkedCount = $locationService->count($bookmarkedFilter); + + $notBookmarkedFilter = clone $baseFilter; + $notBookmarkedFilter->withCriterion(new Criterion\Location\IsBookmarked(false)); + $notBookmarkedCount = $locationService->count($notBookmarkedFilter); + + self::assertSame( + $totalCount, + $bookmarkedCount + $notBookmarkedCount, + sprintf( + 'Mismatch: total=%d, bookmarked=%d, notBookmarked=%d', + $totalCount, + $bookmarkedCount, + $notBookmarkedCount + ) + ); + } + + /** + * @return iterable + */ + public function isBookmarkedProvider(): iterable + { + // [isBookmarkedCriterion, initialCount, afterCreateCount, afterDeleteCount] + yield 'bookmarked=true' => [true, 0, 1, 0]; + yield 'bookmarked=false' => [false, 1, 0, 1]; + } + + /** + * @dataProvider isBookmarkedProvider + */ + public function testIsBookmarkedTrueAndFalse( + bool $isBookmarked, + int $initialCount, + int $afterCreateCount, + int $afterDeleteCount + ): void { + $repository = $this->getRepository(false); + $locationService = $repository->getLocationService(); + $bookmarkService = $repository->getBookmarkService(); + + $bookmarkedLocation = $locationService->loadLocation(self::BOOKMARKED_LOCATION_ID); + + $filter = new Filter(); + $filter->withCriterion(new Criterion\Location\IsBookmarked($isBookmarked)) + ->andWithCriterion(new Criterion\LocationId(self::BOOKMARKED_LOCATION_ID)); + + self::assertCount( + $initialCount, + $locationService->find($filter), + 'Unexpected initial bookmark state for IsBookmarked(' . ($isBookmarked ? 'true' : 'false') . ')' + ); + + $bookmarkService->createBookmark($bookmarkedLocation); + + $filter = new Filter(); + $filter->withCriterion(new Criterion\Location\IsBookmarked($isBookmarked)) + ->andWithCriterion(new Criterion\LocationId(self::BOOKMARKED_LOCATION_ID)); + + self::assertCount( + $afterCreateCount, + $locationService->find($filter), + 'Unexpected state after creating bookmark for IsBookmarked(' . ($isBookmarked ? 'true' : 'false') . ')' + ); + + $bookmarkService->deleteBookmark($bookmarkedLocation); + + $filter = new Filter(); + $filter->withCriterion(new Criterion\Location\IsBookmarked($isBookmarked)) + ->andWithCriterion(new Criterion\LocationId(self::BOOKMARKED_LOCATION_ID)); + + self::assertCount( + $afterDeleteCount, + $locationService->find($filter), + 'Unexpected state after deleting bookmark for IsBookmarked(' . ($isBookmarked ? 'true' : 'false') . ')' + ); + } + + public function testLogicalOrOfBookmarkedAndNotBookmarkedMatchesEveryLocationOnce(): void + { + $repository = $this->getRepository(false); + $locationService = $repository->getLocationService(); + $bookmarkService = $repository->getBookmarkService(); + + $bookmarkedLocation = $locationService->loadLocation(self::BOOKMARKED_LOCATION_ID); + $bookmarkService->createBookmark($bookmarkedLocation); + + try { + $totalCount = $locationService->count(new Filter()); + + $orFilter = new Filter(); + $orFilter->withCriterion( + new Criterion\LogicalOr([ + new Criterion\Location\IsBookmarked(true), + new Criterion\Location\IsBookmarked(false), + ]) + ); + + self::assertSame($totalCount, $locationService->count($orFilter)); + + $locationIds = array_map( + static function ($location) { + return $location->getId(); + }, + iterator_to_array($locationService->find($orFilter)) + ); + + self::assertSame( + $totalCount, + count(array_unique($locationIds)), + 'LogicalOr(IsBookmarked(true), IsBookmarked(false)) returned duplicate locations' + ); + } finally { + $bookmarkService->deleteBookmark($bookmarkedLocation); + } + } + /** * @throws \Ibexa\Contracts\Core\Repository\Exceptions\InvalidArgumentException */ diff --git a/tests/integration/Core/Repository/LocationServiceTest.php b/tests/integration/Core/Repository/LocationServiceTest.php index 350347bda4..3c91328d55 100644 --- a/tests/integration/Core/Repository/LocationServiceTest.php +++ b/tests/integration/Core/Repository/LocationServiceTest.php @@ -6,6 +6,7 @@ */ namespace Ibexa\Tests\Integration\Core\Repository; +use Doctrine\DBAL\ParameterType; use Exception; use Ibexa\Contracts\Core\Repository\Exceptions\BadStateException; use Ibexa\Contracts\Core\Repository\Exceptions\InvalidArgumentException; @@ -1968,6 +1969,7 @@ public function testBookmarksAreSwappedAfterSwapLocation() $mediaLocationId = $this->generateId('location', 43); $demoDesignLocationId = $this->generateId('location', 56); + $contactUsLocationId = $this->generateId('location', 60); /* BEGIN: Use Case */ $locationService = $repository->getLocationService(); @@ -1975,6 +1977,7 @@ public function testBookmarksAreSwappedAfterSwapLocation() $mediaLocation = $locationService->loadLocation($mediaLocationId); $demoDesignLocation = $locationService->loadLocation($demoDesignLocationId); + $contactUsLocation = $locationService->loadLocation($contactUsLocationId); // Bookmark locations $bookmarkService->createBookmark($mediaLocation); @@ -1983,13 +1986,13 @@ public function testBookmarksAreSwappedAfterSwapLocation() $beforeSwap = $bookmarkService->loadBookmarks(); // Swaps the content referred to by the locations - $locationService->swapLocation($mediaLocation, $demoDesignLocation); + $locationService->swapLocation($demoDesignLocation, $contactUsLocation); $afterSwap = $bookmarkService->loadBookmarks(); /* END: Use Case */ - $this->assertEquals($beforeSwap->items[0]->id, $afterSwap->items[1]->id); - $this->assertEquals($beforeSwap->items[1]->id, $afterSwap->items[0]->id); + self::assertEquals($contactUsLocationId, $afterSwap->items[0]->getId()); + self::assertEquals($beforeSwap->items[1]->getId(), $afterSwap->items[1]->getId()); } /** @@ -2342,6 +2345,26 @@ public function testDeleteLocationDeletesRelatedBookmarks() foreach ($bookmarkService->loadBookmarks(0, 9999) as $bookmarkedLocation) { $this->assertNotEquals($childLocation->id, $bookmarkedLocation->id); } + + // The assertion above only proves the bookmark is not *listed* but loadBookmarks() + // Check the row itself to actually cover the cleanup. + $connection = $this->getRawDatabaseConnection(); + $query = $connection->createQueryBuilder(); + $query + ->select('COUNT(id)') + ->from('ezcontentbrowsebookmark') + ->where( + $query->expr()->eq( + 'node_id', + $query->createNamedParameter($childLocation->getId(), ParameterType::INTEGER) + ) + ); + + self::assertSame( + 0, + (int)$query->execute()->fetchColumn(), + 'Bookmark row of a deleted Location should have been removed' + ); } /** diff --git a/tests/lib/Persistence/Legacy/Filter/CriterionQueryBuilder/Location/IsBookmarkedQueryBuilderTest.php b/tests/lib/Persistence/Legacy/Filter/CriterionQueryBuilder/Location/IsBookmarkedQueryBuilderTest.php new file mode 100644 index 0000000000..2ff31b3a3f --- /dev/null +++ b/tests/lib/Persistence/Legacy/Filter/CriterionQueryBuilder/Location/IsBookmarkedQueryBuilderTest.php @@ -0,0 +1,72 @@ +}> + * + * @throws \Ibexa\Contracts\Core\Repository\Exceptions\InvalidCriterionArgumentException + */ + public function getFilteringCriteriaQueryData(): iterable + { + yield 'IsBookmarked(true)' => [ + new Criterion\Location\IsBookmarked(true), + sprintf('EXISTS (%s)', sprintf(self::BOOKMARK_EXISTS_SUBQUERY, 1)), + ['dcValue1' => self::CURRENT_USER_ID], + ]; + + yield 'IsBookmarked(false)' => [ + new Criterion\Location\IsBookmarked(false), + sprintf('NOT EXISTS (%s)', sprintf(self::BOOKMARK_EXISTS_SUBQUERY, 1)), + ['dcValue1' => self::CURRENT_USER_ID], + ]; + + yield 'IsBookmarked(true) OR IsBookmarked(false)' => [ + new Criterion\LogicalOr( + [ + new Criterion\Location\IsBookmarked(true), + new Criterion\Location\IsBookmarked(false), + ] + ), + sprintf( + '(EXISTS (%s)) OR (NOT EXISTS (%s))', + sprintf(self::BOOKMARK_EXISTS_SUBQUERY, 1), + sprintf(self::BOOKMARK_EXISTS_SUBQUERY, 2) + ), + ['dcValue1' => self::CURRENT_USER_ID, 'dcValue2' => self::CURRENT_USER_ID], + ]; + } + + protected function getCriterionQueryBuilders(): iterable + { + $userReference = $this->createMock(UserReference::class); + $userReference->method('getUserId')->willReturn(self::CURRENT_USER_ID); + + $permissionResolver = $this->createMock(PermissionResolver::class); + $permissionResolver->method('getCurrentUserReference')->willReturn($userReference); + + return [new IsBookmarkedQueryBuilder($permissionResolver)]; + } +} diff --git a/tests/lib/Persistence/Legacy/Filter/SortClauseQueryBuilder/Location/Bookmark/IdSortClauseQueryBuilderTest.php b/tests/lib/Persistence/Legacy/Filter/SortClauseQueryBuilder/Location/Bookmark/IdSortClauseQueryBuilderTest.php new file mode 100644 index 0000000000..f31817a6d2 --- /dev/null +++ b/tests/lib/Persistence/Legacy/Filter/SortClauseQueryBuilder/Location/Bookmark/IdSortClauseQueryBuilderTest.php @@ -0,0 +1,215 @@ +createLocationFilteringQueryBuilder(); + + $builder = $this->createBuilder(); + $sortClause = new Id(Query::SORT_DESC); + + self::assertTrue($builder->accepts($sortClause)); + + $builder->buildQuery($queryBuilder, $sortClause); + + self::assertContains( + sprintf('%s.id AS %s', self::BOOKMARK_ALIAS, self::SORT_ALIAS), + $queryBuilder->getQueryPart('select') + ); + + $joins = $queryBuilder->getQueryPart('join'); + self::assertArrayHasKey('location', $joins); + + $bookmarkJoin = $this->findJoinByAlias($joins['location'], self::BOOKMARK_ALIAS); + self::assertNotNull($bookmarkJoin, 'Bookmarks table was not joined against "location"'); + self::assertSame(DoctrineDatabase::TABLE_BOOKMARKS, $bookmarkJoin['joinTable']); + self::assertSame( + sprintf( + '(location.node_id = %1$s.node_id) AND (%1$s.user_id = :dcValue1)', + self::BOOKMARK_ALIAS + ), + (string)$bookmarkJoin['joinCondition'] + ); + self::assertSame(['dcValue1' => self::CURRENT_USER_ID], $queryBuilder->getParameters()); + + self::assertSame( + [self::SORT_ALIAS . ' DESC'], + $queryBuilder->getQueryPart('orderBy') + ); + + // the whole join graph has to resolve + self::assertStringContainsString(self::BOOKMARK_ALIAS, $queryBuilder->getSQL()); + } + + /** + * Content filtering: there is no "location" FROM table, so the Content item's main Location + * has to be joined first and the bookmarks table joined against *that* alias. + */ + public function testBuildQueryInContentFilteringContext(): void + { + $queryBuilder = $this->createContentFilteringQueryBuilder(); + + $this->createBuilder()->buildQuery($queryBuilder, new Id(Query::SORT_DESC)); + + $joins = $queryBuilder->getQueryPart('join'); + + // main Location joined off the "content" FROM table... + self::assertArrayHasKey('content', $joins); + self::assertSame(LocationGateway::CONTENT_TREE_TABLE, $joins['content'][0]['joinTable']); + self::assertSame(self::CONTENT_LOCATION_ALIAS, $joins['content'][0]['joinAlias']); + + // ...and bookmarks joined off that alias, not off a hardcoded "location" + self::assertArrayHasKey(self::CONTENT_LOCATION_ALIAS, $joins); + self::assertSame( + DoctrineDatabase::TABLE_BOOKMARKS, + $joins[self::CONTENT_LOCATION_ALIAS][0]['joinTable'] + ); + self::assertSame( + self::BOOKMARK_ALIAS, + $joins[self::CONTENT_LOCATION_ALIAS][0]['joinAlias'] + ); + self::assertSame( + sprintf( + '(%1$s.node_id = %2$s.node_id) AND (%2$s.user_id = :dcValue1)', + self::CONTENT_LOCATION_ALIAS, + self::BOOKMARK_ALIAS + ), + (string)$joins[self::CONTENT_LOCATION_ALIAS][0]['joinCondition'] + ); + + self::assertArrayNotHasKey('location', $joins); + + self::assertSame( + [self::SORT_ALIAS . ' DESC'], + $queryBuilder->getQueryPart('orderBy') + ); + + self::assertStringContainsString(self::BOOKMARK_ALIAS, $queryBuilder->getSQL()); + } + + /** + * @return iterable + */ + public function standaloneContextProvider(): iterable + { + yield 'Location filtering' => [$this->createLocationFilteringQueryBuilder()]; + yield 'Content filtering' => [$this->createContentFilteringQueryBuilder()]; + } + + /** + * Test that sort clause works without an IsBookmarked criterion having joined anything first. + * + * @dataProvider standaloneContextProvider + */ + public function testBuildQueryStandaloneProducesResolvableSql( + FilteringQueryBuilder $queryBuilder + ): void { + $this->createBuilder()->buildQuery($queryBuilder, new Id(Query::SORT_ASC)); + + $sql = $queryBuilder->getSQL(); + + self::assertStringContainsString(DoctrineDatabase::TABLE_BOOKMARKS, $sql); + self::assertStringContainsString('ORDER BY ' . self::SORT_ALIAS . ' ASC', $sql); + } + + /** + * @param array> $joins + * + * @return array|null + */ + private function findJoinByAlias(array $joins, string $joinAlias): ?array + { + foreach ($joins as $join) { + if ($join['joinAlias'] === $joinAlias) { + return $join; + } + } + + return null; + } + + private function createBuilder(): IdSortClauseQueryBuilder + { + $userReference = $this->createMock(UserReference::class); + $userReference->method('getUserId')->willReturn(self::CURRENT_USER_ID); + + $permissionResolver = $this->createMock(PermissionResolver::class); + $permissionResolver->method('getCurrentUserReference')->willReturn($userReference); + + return new IdSortClauseQueryBuilder($permissionResolver); + } + + /** + * Mirrors the baseline query built by + * {@see \Ibexa\Core\Persistence\Legacy\Filter\Gateway\Location\Doctrine\DoctrineGateway}: + * "location" is the FROM table and "content" is joined off it. + */ + private function createLocationFilteringQueryBuilder(): FilteringQueryBuilder + { + $queryBuilder = new FilteringQueryBuilder($this->createInMemoryConnection()); + $queryBuilder + ->select('location.node_id') + ->from(LocationGateway::CONTENT_TREE_TABLE, 'location') + ->join( + 'location', + self::CONTENT_ITEM_TABLE, + 'content', + 'content.id = location.contentobject_id' + ); + + return $queryBuilder; + } + + /** + * Mirrors the baseline query built by + * {@see \Ibexa\Core\Persistence\Legacy\Filter\Gateway\Content\Doctrine\DoctrineGateway}: + * "content" is the FROM table and there is no "location" alias at all. + */ + private function createContentFilteringQueryBuilder(): FilteringQueryBuilder + { + $queryBuilder = new FilteringQueryBuilder($this->createInMemoryConnection()); + $queryBuilder + ->select('content.id') + ->from(self::CONTENT_ITEM_TABLE, 'content'); + + return $queryBuilder; + } + + private function createInMemoryConnection(): \Doctrine\DBAL\Connection + { + return DriverManager::getConnection(['url' => 'sqlite:///:memory:']); + } +} diff --git a/tests/lib/Repository/Service/Mock/BookmarkTest.php b/tests/lib/Repository/Service/Mock/BookmarkTest.php index 536a0c2390..5bed42d23a 100644 --- a/tests/lib/Repository/Service/Mock/BookmarkTest.php +++ b/tests/lib/Repository/Service/Mock/BookmarkTest.php @@ -15,6 +15,7 @@ use Ibexa\Contracts\Core\Repository\LocationService; use Ibexa\Contracts\Core\Repository\PermissionResolver; use Ibexa\Contracts\Core\Repository\Values\Content\ContentInfo; +use Ibexa\Contracts\Core\Repository\Values\Content\LocationList; use Ibexa\Core\Repository\BookmarkService; use Ibexa\Core\Repository\Values\Content\Location; use Ibexa\Core\Repository\Values\User\UserReference; @@ -220,28 +221,13 @@ public function testLoadBookmarks() $expectedItems = array_map(function ($locationId) { return $this->createLocation($locationId); }, range(1, $expectedTotalCount)); - - $this->bookmarkHandler - ->expects($this->once()) - ->method('countUserBookmarks') - ->with(self::CURRENT_USER_ID) - ->willReturn($expectedTotalCount); - - $this->bookmarkHandler - ->expects($this->once()) - ->method('loadUserBookmarks') - ->with(self::CURRENT_USER_ID, $offset, $limit) - ->willReturn(array_map(static function ($locationId) { - return new Bookmark(['locationId' => $locationId]); - }, range(1, $expectedTotalCount))); + $locationList = new LocationList(['totalCount' => $expectedTotalCount, 'locations' => $expectedItems]); $locationServiceMock = $this->createMock(LocationService::class); $locationServiceMock - ->expects($this->exactly($expectedTotalCount)) - ->method('loadLocation') - ->willReturnCallback(function ($locationId) { - return $this->createLocation($locationId); - }); + ->expects(self::once()) + ->method('find') + ->willReturn($locationList); $repository = $this->getRepositoryMock(); $repository @@ -249,33 +235,17 @@ public function testLoadBookmarks() ->method('getLocationService') ->willReturn($locationServiceMock); + // All tests in this class expect a call to getPermissionResolver()->getCurrentUserReference(), except this very test + // This is because PermissionResolver is called from BookmarkQueryBuilder, not from BookmarkService when loading bookmarks + // As it is defined in setup() that getCurrentUserReference needs to be called at least once, we'll force a call here + $repository->getPermissionResolver()->getCurrentUserReference(); + $bookmarks = $this->createBookmarkService()->loadBookmarks($offset, $limit); $this->assertEquals($expectedTotalCount, $bookmarks->totalCount); $this->assertEquals($expectedItems, $bookmarks->items); } - /** - * @covers \Ibexa\Contracts\Core\Repository\BookmarkService::loadBookmarks - */ - public function testLoadBookmarksEmptyList() - { - $this->bookmarkHandler - ->expects($this->once()) - ->method('countUserBookmarks') - ->with(self::CURRENT_USER_ID) - ->willReturn(0); - - $this->bookmarkHandler - ->expects($this->never()) - ->method('loadUserBookmarks'); - - $bookmarks = $this->createBookmarkService()->loadBookmarks(0, 10); - - $this->assertEquals(0, $bookmarks->totalCount); - $this->assertEmpty($bookmarks->items); - } - /** * @covers \Ibexa\Contracts\Core\Repository\BookmarkService::isBookmarked */ @@ -290,9 +260,6 @@ public function testLocationShouldNotBeBookmarked() $this->assertFalse($this->createBookmarkService()->isBookmarked($this->createLocation(self::LOCATION_ID))); } - /** - * @covers \Ibexa\Contracts\Core\Repository\BookmarkService::isBookmarked - */ public function testLocationShouldBeBookmarked() { $this->bookmarkHandler