From 951f3eb7eefae9b7236517a2eca8fcbfe77567b2 Mon Sep 17 00:00:00 2001 From: Sean Molenaar Date: Mon, 5 Apr 2021 15:13:30 +0200 Subject: DB: Allow negative limits Signed-off-by: Sean Molenaar --- AUTHORS.md | 1 + CHANGELOG.md | 1 + lib/Db/ItemMapperV2.php | 15 +- tests/Unit/Db/ItemMapperPaginatedTest.php | 223 ++++++++++++++++++++++++++++++ tests/integration/items.bats | 41 +++++- 5 files changed, 274 insertions(+), 7 deletions(-) diff --git a/AUTHORS.md b/AUTHORS.md index 6062595a1..56f3aceb1 100644 --- a/AUTHORS.md +++ b/AUTHORS.md @@ -151,6 +151,7 @@ * [nexus-uw](mailto:you@example.com) * [repat](mailto:repat@repat.de) * [ritchiewilson](mailto:rawilson52@gmail.com) +* [skiingwiz](mailto:skiingwiz@gmail.com) * [sonologic](mailto:gmc@sonologic.nl) * [tflidd](mailto:tflidd@aspekte.net) * [waffshappen](mailto:44290023+waffshappen@users.noreply.github.com) diff --git a/CHANGELOG.md b/CHANGELOG.md index c6da0678a..fcdfcbdd1 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 +- Allow negative limits (#1275) ## [15.4.0-beta3] - 2021-04-03 ### Changed diff --git a/lib/Db/ItemMapperV2.php b/lib/Db/ItemMapperV2.php index c79ac035f..b2e3bcb04 100644 --- a/lib/Db/ItemMapperV2.php +++ b/lib/Db/ItemMapperV2.php @@ -467,7 +467,6 @@ class ItemMapperV2 extends NewsMapperV2 ->andWhere('items.feed_id = :feedId') ->setParameter('userId', $userId) ->setParameter('feedId', $feedId) - ->setMaxResults($limit) ->orderBy('items.last_modified', ($oldestFirst ? 'ASC' : 'DESC')) ->addOrderBy('items.id', ($oldestFirst ? 'ASC' : 'DESC')); @@ -479,6 +478,10 @@ class ItemMapperV2 extends NewsMapperV2 } } + if ($limit >= 1) { + $builder->setMaxResults($limit); + } + if ($offset !== 0) { $builder->andWhere($this->offsetWhere($oldestFirst)) ->setParameter('offset', $offset); @@ -526,7 +529,6 @@ class ItemMapperV2 extends NewsMapperV2 ->andWhere('feeds.deleted_at = 0') ->andWhere($folderWhere) ->setParameter('userId', $userId) - ->setMaxResults($limit) ->orderBy('items.last_modified', ($oldestFirst ? 'ASC' : 'DESC')) ->addOrderBy('items.id', ($oldestFirst ? 'ASC' : 'DESC')); @@ -538,6 +540,10 @@ class ItemMapperV2 extends NewsMapperV2 } } + if ($limit >= 1) { + $builder->setMaxResults($limit); + } + if ($offset !== 0) { $builder->andWhere($this->offsetWhere($oldestFirst)) ->setParameter('offset', $offset); @@ -577,7 +583,6 @@ class ItemMapperV2 extends NewsMapperV2 ->andWhere('feeds.user_id = :userId') ->andWhere('feeds.deleted_at = 0') ->setParameter('userId', $userId) - ->setMaxResults($limit) ->orderBy('items.last_modified', ($oldestFirst ? 'ASC' : 'DESC')) ->addOrderBy('items.id', ($oldestFirst ? 'ASC' : 'DESC')); @@ -589,6 +594,10 @@ class ItemMapperV2 extends NewsMapperV2 } } + if ($limit >= 1) { + $builder->setMaxResults($limit); + } + if ($offset !== 0) { $builder->andWhere($this->offsetWhere($oldestFirst)) ->setParameter('offset', $offset); diff --git a/tests/Unit/Db/ItemMapperPaginatedTest.php b/tests/Unit/Db/ItemMapperPaginatedTest.php index 6bd6906d3..93521f541 100644 --- a/tests/Unit/Db/ItemMapperPaginatedTest.php +++ b/tests/Unit/Db/ItemMapperPaginatedTest.php @@ -259,6 +259,75 @@ class ItemMapperPaginatedTest extends MapperTestUtility $this->assertEquals([Item::fromRow(['id' => 4])], $result); } + public function testFindAllItemsUnreadNoLimit() + { + $this->db->expects($this->once()) + ->method('getQueryBuilder') + ->willReturn($this->builder); + + $this->builder->expects($this->once()) + ->method('select') + ->with('items.*') + ->will($this->returnSelf()); + + $this->builder->expects($this->once()) + ->method('from') + ->with('news_items', 'items') + ->will($this->returnSelf()); + + $this->builder->expects($this->exactly(1)) + ->method('innerJoin') + ->withConsecutive(['items', 'news_feeds', 'feeds', 'items.feed_id = feeds.id']) + ->will($this->returnSelf()); + + $this->builder->expects($this->exactly(4)) + ->method('andWhere') + ->withConsecutive( + ['feeds.user_id = :userId'], + ['feeds.deleted_at = 0'], + ['items.id < :offset'], + ['items.unread = 1'] + ) + ->will($this->returnSelf()); + + $this->builder->expects($this->exactly(2)) + ->method('setParameter') + ->withConsecutive(['userId', 'jack'], ['offset', 10]) + ->will($this->returnSelf()); + + $this->builder->expects($this->never()) + ->method('setMaxResults'); + + $this->builder->expects($this->exactly(0)) + ->method('setFirstResult') + ->with(10) + ->will($this->returnSelf()); + + $this->builder->expects($this->once()) + ->method('orderBy') + ->with('items.last_modified', 'DESC') + ->will($this->returnSelf()); + + $this->builder->expects($this->once()) + ->method('addOrderBy') + ->with('items.id', 'DESC') + ->willReturnSelf(); + + $this->builder->expects($this->exactly(1)) + ->method('execute') + ->will($this->returnValue($this->cursor)); + + $this->cursor->expects($this->exactly(2)) + ->method('fetch') + ->willReturnOnConsecutiveCalls( + ['id' => 4], + false + ); + + $result = $this->class->findAllItems('jack', 6, -1, 10, false, []); + $this->assertEquals([Item::fromRow(['id' => 4])], $result); + } + public function testFindAllItemsStarred() { $this->db->expects($this->once()) @@ -482,6 +551,77 @@ class ItemMapperPaginatedTest extends MapperTestUtility $this->assertEquals([Item::fromRow(['id' => 4])], $result); } + public function testFindAllFeedNoLimit() + { + $this->db->expects($this->once()) + ->method('getQueryBuilder') + ->willReturn($this->builder); + + $this->builder->expects($this->once()) + ->method('select') + ->with('items.*') + ->will($this->returnSelf()); + + $this->builder->expects($this->once()) + ->method('from') + ->with('news_items', 'items') + ->will($this->returnSelf()); + + $this->builder->expects($this->exactly(1)) + ->method('innerJoin') + ->withConsecutive(['items', 'news_feeds', 'feeds', 'items.feed_id = feeds.id']) + ->will($this->returnSelf()); + + $this->builder->expects($this->exactly(4)) + ->method('andWhere') + ->withConsecutive( + ['feeds.deleted_at = 0'], + ['feeds.user_id = :userId'], + ['items.feed_id = :feedId'], + ['items.id < :offset'] + ) + ->will($this->returnSelf()); + + $this->builder->expects($this->exactly(3)) + ->method('setParameter') + ->withConsecutive(['userId', 'jack'], ['feedId', 2], ['offset', 10]) + ->will($this->returnSelf()); + + + $this->builder->expects($this->never()) + ->method('setMaxResults'); + + + $this->builder->expects($this->exactly(0)) + ->method('setFirstResult') + ->with(10) + ->will($this->returnSelf()); + + $this->builder->expects($this->once()) + ->method('orderBy') + ->with('items.last_modified', 'DESC') + ->will($this->returnSelf()); + + $this->builder->expects($this->once()) + ->method('addOrderBy') + ->with('items.id', 'DESC') + ->willReturnSelf(); + + $this->builder->expects($this->exactly(1)) + ->method('execute') + ->will($this->returnValue($this->cursor)); + + $this->cursor->expects($this->exactly(2)) + ->method('fetch') + ->willReturnOnConsecutiveCalls( + ['id' => 4], + false + ); + + $result = $this->class->findAllFeed('jack', 2, -1, 10, false, false, []); + $this->assertEquals([Item::fromRow(['id' => 4])], $result); + } + public function testFindAllFeedInverted() { $this->db->expects($this->once()) @@ -796,6 +936,89 @@ class ItemMapperPaginatedTest extends MapperTestUtility $this->assertEquals([Item::fromRow(['id' => 4])], $result); } + public function testFindAllFolderIdNullNoLimit() + { + $expr = $this->getMockBuilder(IExpressionBuilder::class) + ->getMock(); + + $expr->expects($this->once()) + ->method('isNull') + ->with('feeds.folder_id') + ->will($this->returnValue('x IS NULL')); + + $this->db->expects($this->once()) + ->method('getQueryBuilder') + ->willReturn($this->builder); + + $this->builder->expects($this->exactly(1)) + ->method('expr') + ->will($this->returnValue($expr)); + + $this->builder->expects($this->once()) + ->method('select') + ->with('items.*') + ->will($this->returnSelf()); + + $this->builder->expects($this->once()) + ->method('from') + ->with('news_items', 'items') + ->will($this->returnSelf()); + + $this->builder->expects($this->exactly(1)) + ->method('innerJoin') + ->withConsecutive(['items', 'news_feeds', 'feeds', 'items.feed_id = feeds.id']) + ->will($this->returnSelf()); + + $this->builder->expects($this->exactly(4)) + ->method('andWhere') + ->withConsecutive( + ['feeds.user_id = :userId'], + ['feeds.deleted_at = 0'], + ['x IS NULL'], + ['items.id < :offset'] + ) + ->will($this->returnSelf()); + + $this->builder->expects($this->exactly(2)) + ->method('setParameter') + ->withConsecutive(['userId', 'jack'], ['offset', 10]) + ->will($this->returnSelf()); + + + $this->builder->expects($this->never(1)) + ->method('setMaxResults'); + + + $this->builder->expects($this->exactly(0)) + ->method('setFirstResult') + ->with(10) + ->will($this->returnSelf()); + + $this->builder->expects($this->once()) + ->method('orderBy') + ->with('items.last_modified', 'DESC') + ->will($this->returnSelf()); + + $this->builder->expects($this->once()) + ->method('addOrderBy') + ->with('items.id', 'DESC') + ->willReturnSelf(); + + $this->builder->expects($this->exactly(1)) + ->method('execute') + ->will($this->returnValue($this->cursor)); + + $this->cursor->expects($this->exactly(2)) + ->method('fetch') + ->willReturnOnConsecutiveCalls( + ['id' => 4], + false + ); + + $result = $this->class->findAllFolder('jack', null, -1, 10, false, false, []); + $this->assertEquals([Item::fromRow(['id' => 4])], $result); + } + public function testFindAllFolderHideRead() { $expr = $this->getMockBuilder(IExpressionBuilder::class) diff --git a/tests/integration/items.bats b/tests/integration/items.bats index e85c8956c..22a4c73c7 100644 --- a/tests/integration/items.bats +++ b/tests/integration/items.bats @@ -17,7 +17,7 @@ teardown() { fi } -@test "[$TESTSUITE] List all items in feed" { +@test "[$TESTSUITE] List 200 items in feed" { run ./occ news:item:list-feed "$user" "$ID" --limit 200 [ "$status" -eq 0 ] @@ -28,7 +28,18 @@ teardown() { fi } -@test "[$TESTSUITE] List all items in folder" { +@test "[$TESTSUITE] List all items in feed" { + run ./occ news:item:list-feed "$user" "$ID" --limit 0 + [ "$status" -eq 0 ] + + if ! echo "$output" | grep "$TAG"; then + ret_status=$? + echo "Release not found in feed list" + return $ret_status + fi +} + +@test "[$TESTSUITE] List 200 items in folder" { run ./occ news:item:list-folder "$user" --limit 200 [ "$status" -eq 0 ] @@ -39,7 +50,18 @@ teardown() { fi } -@test "[$TESTSUITE] List all items" { +@test "[$TESTSUITE] List all items in folder" { + run ./occ news:item:list-folder "$user" --limit 0 + [ "$status" -eq 0 ] + + if ! echo "$output" | grep "$TAG"; then + ret_status=$? + echo "Release not found in folder list" + return $ret_status + fi +} + +@test "[$TESTSUITE] List 200 items" { run ./occ news:item:list "$user" --limit 200 [ "$status" -eq 0 ] @@ -48,4 +70,15 @@ teardown() { echo "Release not found in list" return $ret_status fi -} \ No newline at end of file +} + +@test "[$TESTSUITE] List all items" { + run ./occ news:item:list "$user" --limit 0 + [ "$status" -eq 0 ] + + if ! echo "$output" | grep "$TAG"; then + ret_status=$? + echo "Release not found in list" + return $ret_status + fi +} -- cgit v1.2.3