From 9d5d35ce2396d0a059b4271ffbd454c0eacab0f8 Mon Sep 17 00:00:00 2001 From: Sean Molenaar Date: Tue, 23 Feb 2021 22:05:03 +0100 Subject: DB: Fix offset quotes Issue GH-1200 Signed-off-by: Sean Molenaar --- CHANGELOG.md | 1 + lib/Db/ItemMapperV2.php | 55 +++++----- tests/Unit/Db/ItemMapperPaginatedTest.php | 173 +++++------------------------- 3 files changed, 59 insertions(+), 170 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c6194af48..bce387bdd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ The format is almost based on [Keep a Changelog](https://keepachangelog.com/en/1 ### Changed ### Fixed +- Item list not using ID for offset 2 (#1200) ## [15.4.0-beta1] - 2021-02-23 ### Changed diff --git a/lib/Db/ItemMapperV2.php b/lib/Db/ItemMapperV2.php index fe4bbabc0..33b4611f9 100644 --- a/lib/Db/ItemMapperV2.php +++ b/lib/Db/ItemMapperV2.php @@ -402,6 +402,22 @@ class ItemMapperV2 extends NewsMapperV2 return $this->findEntities($builder); } + /** + * Generate an expression for the offset. + * + * @param bool $oldestFirst Sorting direction + * + * @return string + */ + private function offsetWhere(bool $oldestFirst): string + { + if ($oldestFirst === true) { + return 'items.id > :offset'; + } + + return 'items.id < :offset'; + } + /** * @param string $userId User identifier * @param int $feedId Feed identifier @@ -424,21 +440,13 @@ class ItemMapperV2 extends NewsMapperV2 ): array { $builder = $this->db->getQueryBuilder(); - if ($oldestFirst === true) { - $offsetWhere = $builder->expr()->lt('items.id', ':offset'); - } else { - $offsetWhere = $builder->expr()->gt('items.id', ':offset'); - } - $builder->select('items.*') ->from($this->tableName, 'items') ->innerJoin('items', FeedMapperV2::TABLE_NAME, 'feeds', 'items.feed_id = feeds.id') ->andWhere('feeds.user_id = :userId') ->andWhere('items.feed_id = :feedId') - ->andWhere($offsetWhere) ->setParameter('userId', $userId) ->setParameter('feedId', $feedId) - ->setParameter('offset', $offset) ->setMaxResults($limit) ->orderBy('items.last_modified', ($oldestFirst ? 'ASC' : 'DESC')) ->addOrderBy('items.id', ($oldestFirst ? 'ASC' : 'DESC')); @@ -451,6 +459,11 @@ class ItemMapperV2 extends NewsMapperV2 } } + if ($offset !== 0) { + $builder->andWhere($this->offsetWhere($oldestFirst)) + ->setParameter('offset', $offset); + } + if ($hideRead === true) { $builder->andWhere('items.unread = 1'); } @@ -486,20 +499,12 @@ class ItemMapperV2 extends NewsMapperV2 $folderWhere = $builder->expr()->eq('feeds.folder_id', new Literal($folderId), IQueryBuilder::PARAM_INT); } - if ($oldestFirst === true) { - $offsetWhere = $builder->expr()->lt('items.id', ':offset'); - } else { - $offsetWhere = $builder->expr()->gt('items.id', ':offset'); - } - $builder->select('items.*') ->from($this->tableName, 'items') ->innerJoin('items', FeedMapperV2::TABLE_NAME, 'feeds', 'items.feed_id = feeds.id') ->andWhere('feeds.user_id = :userId') ->andWhere($folderWhere) - ->andWhere($offsetWhere) ->setParameter('userId', $userId) - ->setParameter('offset', $offset) ->setMaxResults($limit) ->orderBy('items.last_modified', ($oldestFirst ? 'ASC' : 'DESC')) ->addOrderBy('items.id', ($oldestFirst ? 'ASC' : 'DESC')); @@ -512,6 +517,11 @@ class ItemMapperV2 extends NewsMapperV2 } } + if ($offset !== 0) { + $builder->andWhere($this->offsetWhere($oldestFirst)) + ->setParameter('offset', $offset); + } + if ($hideRead === true) { $builder->andWhere('items.unread = 1'); } @@ -540,19 +550,11 @@ class ItemMapperV2 extends NewsMapperV2 ): array { $builder = $this->db->getQueryBuilder(); - if ($oldestFirst === true) { - $offsetWhere = $builder->expr()->lt('items.id', ':offset'); - } else { - $offsetWhere = $builder->expr()->gt('items.id', ':offset'); - } - $builder->select('items.*') ->from($this->tableName, 'items') ->innerJoin('items', FeedMapperV2::TABLE_NAME, 'feeds', 'items.feed_id = feeds.id') ->andWhere('feeds.user_id = :userId') - ->andWhere($offsetWhere) ->setParameter('userId', $userId) - ->setParameter('offset', $offset) ->setMaxResults($limit) ->orderBy('items.last_modified', ($oldestFirst ? 'ASC' : 'DESC')) ->addOrderBy('items.id', ($oldestFirst ? 'ASC' : 'DESC')); @@ -565,6 +567,11 @@ class ItemMapperV2 extends NewsMapperV2 } } + if ($offset !== 0) { + $builder->andWhere($this->offsetWhere($oldestFirst)) + ->setParameter('offset', $offset); + } + switch ($type) { case ListType::STARRED: $builder->andWhere('items.starred = 1'); diff --git a/tests/Unit/Db/ItemMapperPaginatedTest.php b/tests/Unit/Db/ItemMapperPaginatedTest.php index 4db152236..2b4ac0c5f 100644 --- a/tests/Unit/Db/ItemMapperPaginatedTest.php +++ b/tests/Unit/Db/ItemMapperPaginatedTest.php @@ -62,13 +62,6 @@ class ItemMapperPaginatedTest extends MapperTestUtility $this->expectException(ServiceValidationException::class); $this->expectExceptionMessage('Unexpected Feed type in call'); - $expr = $this->getMockBuilder(IExpressionBuilder::class) - ->getMock(); - - $this->builder->expects($this->exactly(1)) - ->method('expr') - ->will($this->returnValue($expr)); - $this->db->expects($this->once()) ->method('getQueryBuilder') ->willReturn($this->builder); @@ -142,18 +135,6 @@ class ItemMapperPaginatedTest extends MapperTestUtility ->method('getQueryBuilder') ->willReturn($this->builder); - $expr = $this->getMockBuilder(IExpressionBuilder::class) - ->getMock(); - - $expr->expects($this->once()) - ->method('lt') - ->with('items.id', ':offset') - ->will($this->returnValue('x < y')); - - $this->builder->expects($this->exactly(1)) - ->method('expr') - ->will($this->returnValue($expr)); - $this->builder->expects($this->once()) ->method('select') ->with('items.*') @@ -173,7 +154,7 @@ class ItemMapperPaginatedTest extends MapperTestUtility ->method('andWhere') ->withConsecutive( ['feeds.user_id = :userId'], - ['x < y'] + ['items.id > :offset'] ) ->will($this->returnSelf()); @@ -225,18 +206,6 @@ class ItemMapperPaginatedTest extends MapperTestUtility ->method('getQueryBuilder') ->willReturn($this->builder); - $expr = $this->getMockBuilder(IExpressionBuilder::class) - ->getMock(); - - $expr->expects($this->once()) - ->method('gt') - ->with('items.id', ':offset') - ->will($this->returnValue('x > y')); - - $this->builder->expects($this->exactly(1)) - ->method('expr') - ->will($this->returnValue($expr)); - $this->builder->expects($this->once()) ->method('select') ->with('items.*') @@ -256,7 +225,7 @@ class ItemMapperPaginatedTest extends MapperTestUtility ->method('andWhere') ->withConsecutive( ['feeds.user_id = :userId'], - ['x > y'], + ['items.id < :offset'], ['items.unread = 1'] ) ->will($this->returnSelf()); @@ -309,18 +278,6 @@ class ItemMapperPaginatedTest extends MapperTestUtility ->method('getQueryBuilder') ->willReturn($this->builder); - $expr = $this->getMockBuilder(IExpressionBuilder::class) - ->getMock(); - - $expr->expects($this->once()) - ->method('gt') - ->with('items.id', ':offset') - ->will($this->returnValue('x > y')); - - $this->builder->expects($this->exactly(1)) - ->method('expr') - ->will($this->returnValue($expr)); - $this->builder->expects($this->once()) ->method('select') ->with('items.*') @@ -340,7 +297,7 @@ class ItemMapperPaginatedTest extends MapperTestUtility ->method('andWhere') ->withConsecutive( ['feeds.user_id = :userId'], - ['x > y'], + ['items.id < :offset'], ['items.starred = 1'] ) ->will($this->returnSelf()); @@ -395,18 +352,6 @@ class ItemMapperPaginatedTest extends MapperTestUtility ->method('escapeLikeParameter') ->will($this->returnArgument(0)); - $expr = $this->getMockBuilder(IExpressionBuilder::class) - ->getMock(); - - $expr->expects($this->once()) - ->method('gt') - ->with('items.id', ':offset') - ->will($this->returnValue('x > y')); - - $this->builder->expects($this->exactly(1)) - ->method('expr') - ->will($this->returnValue($expr)); - $this->builder->expects($this->once()) ->method('select') ->with('items.*') @@ -426,16 +371,16 @@ class ItemMapperPaginatedTest extends MapperTestUtility ->method('andWhere') ->withConsecutive( ['feeds.user_id = :userId'], - ['x > y'], ['items.search_index LIKE :term0'], ['items.search_index LIKE :term1'], + ['items.id < :offset'], ['items.starred = 1'] ) ->will($this->returnSelf()); $this->builder->expects($this->exactly(4)) ->method('setParameter') - ->withConsecutive(['userId', 'jack'], ['offset', 10], ['term0', '%key%'], ['term1', '%word%']) + ->withConsecutive(['userId', 'jack'], ['term0', '%key%'], ['term1', '%word%'], ['offset', 10]) ->will($this->returnSelf()); @@ -481,18 +426,6 @@ class ItemMapperPaginatedTest extends MapperTestUtility ->method('getQueryBuilder') ->willReturn($this->builder); - $expr = $this->getMockBuilder(IExpressionBuilder::class) - ->getMock(); - - $expr->expects($this->once()) - ->method('gt') - ->with('items.id', ':offset') - ->will($this->returnValue('x > y')); - - $this->builder->expects($this->exactly(1)) - ->method('expr') - ->will($this->returnValue($expr)); - $this->builder->expects($this->once()) ->method('select') ->with('items.*') @@ -513,7 +446,7 @@ class ItemMapperPaginatedTest extends MapperTestUtility ->withConsecutive( ['feeds.user_id = :userId'], ['items.feed_id = :feedId'], - ['x > y'] + ['items.id < :offset'] ) ->will($this->returnSelf()); @@ -565,18 +498,6 @@ class ItemMapperPaginatedTest extends MapperTestUtility ->method('getQueryBuilder') ->willReturn($this->builder); - $expr = $this->getMockBuilder(IExpressionBuilder::class) - ->getMock(); - - $expr->expects($this->once()) - ->method('lt') - ->with('items.id', ':offset') - ->will($this->returnValue('x < y')); - - $this->builder->expects($this->exactly(1)) - ->method('expr') - ->will($this->returnValue($expr)); - $this->builder->expects($this->once()) ->method('select') ->with('items.*') @@ -597,7 +518,7 @@ class ItemMapperPaginatedTest extends MapperTestUtility ->withConsecutive( ['feeds.user_id = :userId'], ['items.feed_id = :feedId'], - ['x < y'] + ['items.id > :offset'] ) ->will($this->returnSelf()); @@ -606,13 +527,11 @@ class ItemMapperPaginatedTest extends MapperTestUtility ->withConsecutive(['userId', 'jack'], ['feedId', 2], ['offset', 10]) ->will($this->returnSelf()); - $this->builder->expects($this->exactly(1)) ->method('setMaxResults') ->with(10) ->will($this->returnSelf()); - $this->builder->expects($this->exactly(0)) ->method('setFirstResult') ->with(10) @@ -649,18 +568,6 @@ class ItemMapperPaginatedTest extends MapperTestUtility ->method('getQueryBuilder') ->willReturn($this->builder); - $expr = $this->getMockBuilder(IExpressionBuilder::class) - ->getMock(); - - $expr->expects($this->once()) - ->method('gt') - ->with('items.id', ':offset') - ->will($this->returnValue('x > y')); - - $this->builder->expects($this->exactly(1)) - ->method('expr') - ->will($this->returnValue($expr)); - $this->builder->expects($this->once()) ->method('select') ->with('items.*') @@ -681,7 +588,7 @@ class ItemMapperPaginatedTest extends MapperTestUtility ->withConsecutive( ['feeds.user_id = :userId'], ['items.feed_id = :feedId'], - ['x > y'], + ['items.id < :offset'], ['items.unread = 1'] ) ->will($this->returnSelf()); @@ -737,18 +644,6 @@ class ItemMapperPaginatedTest extends MapperTestUtility ->method('escapeLikeParameter') ->will($this->returnArgument(0)); - $expr = $this->getMockBuilder(IExpressionBuilder::class) - ->getMock(); - - $expr->expects($this->once()) - ->method('gt') - ->with('items.id', ':offset') - ->will($this->returnValue('x > y')); - - $this->builder->expects($this->exactly(1)) - ->method('expr') - ->will($this->returnValue($expr)); - $this->builder->expects($this->once()) ->method('select') ->with('items.*') @@ -769,15 +664,21 @@ class ItemMapperPaginatedTest extends MapperTestUtility ->withConsecutive( ['feeds.user_id = :userId'], ['items.feed_id = :feedId'], - ['x > y'], ['items.search_index LIKE :term0'], - ['items.search_index LIKE :term1'] + ['items.search_index LIKE :term1'], + ['items.id < :offset'] ) ->will($this->returnSelf()); $this->builder->expects($this->exactly(5)) ->method('setParameter') - ->withConsecutive(['userId', 'jack'], ['feedId', 2], ['offset', 10], ['term0', '%key%'], ['term1', '%word%']) + ->withConsecutive( + ['userId', 'jack'], + ['feedId', 2], + ['term0', '%key%'], + ['term1', '%word%'], + ['offset', 10] + ) ->will($this->returnSelf()); @@ -827,16 +728,11 @@ class ItemMapperPaginatedTest extends MapperTestUtility ->with('feeds.folder_id') ->will($this->returnValue('x IS NULL')); - $expr->expects($this->once()) - ->method('gt') - ->with('items.id', ':offset') - ->will($this->returnValue('x > y')); - $this->db->expects($this->once()) ->method('getQueryBuilder') ->willReturn($this->builder); - $this->builder->expects($this->exactly(2)) + $this->builder->expects($this->exactly(1)) ->method('expr') ->will($this->returnValue($expr)); @@ -860,7 +756,7 @@ class ItemMapperPaginatedTest extends MapperTestUtility ->withConsecutive( ['feeds.user_id = :userId'], ['x IS NULL'], - ['x > y'] + ['items.id < :offset'] ) ->will($this->returnSelf()); @@ -916,16 +812,11 @@ class ItemMapperPaginatedTest extends MapperTestUtility ->with('feeds.folder_id') ->will($this->returnValue('x IS NULL')); - $expr->expects($this->once()) - ->method('gt') - ->with('items.id', ':offset') - ->will($this->returnValue('x > y')); - $this->db->expects($this->once()) ->method('getQueryBuilder') ->willReturn($this->builder); - $this->builder->expects($this->exactly(2)) + $this->builder->expects($this->exactly(1)) ->method('expr') ->will($this->returnValue($expr)); @@ -949,7 +840,7 @@ class ItemMapperPaginatedTest extends MapperTestUtility ->withConsecutive( ['feeds.user_id = :userId'], ['x IS NULL'], - ['x > y'], + ['items.id < :offset'], ['items.unread = 1'] ) ->will($this->returnSelf()); @@ -1006,16 +897,11 @@ class ItemMapperPaginatedTest extends MapperTestUtility ->with('feeds.folder_id') ->will($this->returnValue('x IS NULL')); - $expr->expects($this->once()) - ->method('lt') - ->with('items.id', ':offset') - ->will($this->returnValue('x < y')); - $this->db->expects($this->once()) ->method('getQueryBuilder') ->willReturn($this->builder); - $this->builder->expects($this->exactly(2)) + $this->builder->expects($this->exactly(1)) ->method('expr') ->will($this->returnValue($expr)); @@ -1039,7 +925,7 @@ class ItemMapperPaginatedTest extends MapperTestUtility ->withConsecutive( ['feeds.user_id = :userId'], ['x IS NULL'], - ['x < y'], + ['items.id > :offset'], ['items.unread = 1'] ) ->will($this->returnSelf()); @@ -1091,7 +977,7 @@ class ItemMapperPaginatedTest extends MapperTestUtility $expr = $this->getMockBuilder(IExpressionBuilder::class) ->getMock(); - $this->builder->expects($this->exactly(2)) + $this->builder->expects($this->exactly(1)) ->method('expr') ->will($this->returnValue($expr)); @@ -1100,11 +986,6 @@ class ItemMapperPaginatedTest extends MapperTestUtility ->with('feeds.folder_id', new Literal(2)) ->will($this->returnValue('x = y')); - $expr->expects($this->once()) - ->method('gt') - ->with('items.id', ':offset') - ->will($this->returnValue('x > y')); - $this->db->expects($this->once()) ->method('getQueryBuilder') ->willReturn($this->builder); @@ -1132,15 +1013,15 @@ class ItemMapperPaginatedTest extends MapperTestUtility ->withConsecutive( ['feeds.user_id = :userId'], ['x = y'], - ['x > y'], ['items.search_index LIKE :term0'], - ['items.search_index LIKE :term1'] + ['items.search_index LIKE :term1'], + ['items.id < :offset'] ) ->will($this->returnSelf()); $this->builder->expects($this->exactly(4)) ->method('setParameter') - ->withConsecutive(['userId', 'jack'], ['offset', 10], ['term0', '%key%'], ['term1', '%word%']) + ->withConsecutive(['userId', 'jack'], ['term0', '%key%'], ['term1', '%word%'], ['offset', 10]) ->will($this->returnSelf()); -- cgit v1.2.3