DB: Use ID as offset in item queries

Signed-off-by: Sean Molenaar <sean@seanmolenaar.eu>

5 files changed, 1805 insertions, 1326 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index c03e2e62f..e758178d9 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -15,6 +15,7 @@ The format is almost based on [Keep a Changelog](https://keepachangelog.com/en/1
 - Item list throwing error for folder and "all items"
 - Articles with high IDs can be placed lower than articles with low IDs (#1147)
 - Feeds are accidentally moved on rename
+- Item list not using ID for offset
 
 ## [15.3.2] - 2021-02-10
 No changes compared to RC2
diff --git a/lib/Db/ItemMapperV2.php b/lib/Db/ItemMapperV2.php
index 1ef9da244..fe4bbabc0 100644
--- a/lib/Db/ItemMapperV2.php
+++ b/lib/Db/ItemMapperV2.php
@@ -403,13 +403,13 @@ class ItemMapperV2 extends NewsMapperV2
     }
 
     /**
-     * @param string $userId
-     * @param int    $feedId
-     * @param int    $limit
-     * @param int    $offset
-     * @param bool   $hideRead
-     * @param bool   $oldestFirst
-     * @param array  $search
+     * @param string $userId      User identifier
+     * @param int    $feedId      Feed identifier
+     * @param int    $limit       Max items to retrieve
+     * @param int    $offset      First item ID to retrieve
+     * @param bool   $hideRead    Hide read items
+     * @param bool   $oldestFirst Chronological sort
+     * @param array  $search      Search terms
      *
      * @return Item[]
      */
@@ -424,15 +424,22 @@ 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)
-            ->setFirstResult($offset)
             ->orderBy('items.last_modified', ($oldestFirst ? 'ASC' : 'DESC'))
             ->addOrderBy('items.id', ($oldestFirst ? 'ASC' : 'DESC'));
 
@@ -452,13 +459,13 @@ class ItemMapperV2 extends NewsMapperV2
     }
 
     /**
-     * @param string   $userId
-     * @param int|null $folderId
-     * @param int      $limit
-     * @param int      $offset
-     * @param bool     $hideRead
-     * @param bool     $oldestFirst
-     * @param array    $search
+     * @param string   $userId      User identifier
+     * @param int|null $folderId    Folder identifier (null for root)
+     * @param int      $limit       Max items to retrieve
+     * @param int      $offset      First item ID to retrieve
+     * @param bool     $hideRead    Hide read items
+     * @param bool     $oldestFirst Chronological sort
+     * @param array    $search      Search terms
      *
      * @return Item[]
      */
@@ -479,14 +486,21 @@ 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)
-            ->setFirstResult($offset)
             ->orderBy('items.last_modified', ($oldestFirst ? 'ASC' : 'DESC'))
             ->addOrderBy('items.id', ($oldestFirst ? 'ASC' : 'DESC'));
 
@@ -506,12 +520,12 @@ class ItemMapperV2 extends NewsMapperV2
     }
 
     /**
-     * @param string $userId
-     * @param int    $type
-     * @param int    $limit
-     * @param int    $offset
-     * @param bool   $oldestFirst
-     * @param array  $search
+     * @param string $userId      User identifier
+     * @param int    $type        Type of items to retrieve
+     * @param int    $limit       Max items to retrieve
+     * @param int    $offset      First item ID to retrieve
+     * @param bool   $oldestFirst Chronological sort
+     * @param array  $search      Search terms
      *
      * @return Item[]
      * @throws ServiceValidationException
@@ -526,13 +540,20 @@ 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)
-            ->setFirstResult($offset)
             ->orderBy('items.last_modified', ($oldestFirst ? 'ASC' : 'DESC'))
             ->addOrderBy('items.id', ($oldestFirst ? 'ASC' : 'DESC'));
 
diff --git a/tests/Unit/Db/ItemMapperAfterTest.php b/tests/Unit/Db/ItemMapperAfterTest.php
new file mode 100644
index 000000000..dbb883fc9
--- /dev/null
+++ b/tests/Unit/Db/ItemMapperAfterTest.php
@@ -0,0 +1,577 @@
+<?php
+/**
+ * Nextcloud - News
+ *
+ * This file is licensed under the Affero General Public License version 3 or
+ * later. See the COPYING file.
+ *
+ * @author    Alessandro Cosentino <cosenal@gmail.com>
+ * @author    Bernhard Posselt <dev@bernhard-posselt.com>
+ * @copyright 2012 Alessandro Cosentino
+ * @copyright 2012-2014 Bernhard Posselt
+ */
+
+namespace OCA\News\Tests\Unit\Db;
+
+use OC\DB\QueryBuilder\Literal;
+use OCA\News\Db\Feed;
+use OCA\News\Db\FeedMapperV2;
+use OCA\News\Db\Folder;
+use OCA\News\Db\Item;
+use OCA\News\Db\ItemMapperV2;
+use OCA\News\Db\NewsMapperV2;
+use OCA\News\Service\Exceptions\ServiceValidationException;
+use OCA\News\Utility\Time;
+use OCP\AppFramework\Db\DoesNotExistException;
+use OCP\AppFramework\Db\MultipleObjectsReturnedException;
+use OCP\DB\IResult;
+use OCP\DB\QueryBuilder\IExpressionBuilder;
+use OCP\DB\QueryBuilder\IFunctionBuilder;
+use OCP\DB\QueryBuilder\IQueryBuilder;
+use OCP\DB\QueryBuilder\IQueryFunction;
+use OCP\IDBConnection;
+use Test\TestCase;
+
+/**
+ * Class ItemMapperTest
+ *
+ * @package OCA\News\Tests\Unit\Db
+ */
+class ItemMapperAfterTest extends MapperTestUtility
+{
+
+    /** @var Time */
+    private $time;
+    /** @var ItemMapperV2 */
+    private $class;
+
+    /**
+     * @covers \OCA\News\Db\ItemMapperV2::__construct
+     */
+    protected function setUp(): void
+    {
+        parent::setUp();
+        $this->time = $this->getMockBuilder(Time::class)
+                           ->getMock();
+
+        $this->class = new ItemMapperV2($this->db, $this->time);
+    }
+
+    public function testFindAllInFeedAfter()
+    {
+        $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->once())
+            ->method('innerJoin')
+            ->with('items', 'news_feeds', 'feeds', 'items.feed_id = feeds.id')
+            ->will($this->returnSelf());
+
+        $this->builder->expects($this->exactly(3))
+            ->method('andWhere')
+            ->withConsecutive(
+                ['items.last_modified >= :updatedSince'],
+                ['feeds.user_id = :userId'],
+                ['feeds.id = :feedId']
+            )
+            ->will($this->returnSelf());
+
+        $this->builder->expects($this->exactly(1))
+            ->method('setParameters')
+            ->with([
+                'updatedSince' => 1610903351,
+                'feedId' => 4,
+                'userId' => 'jack',
+            ])
+            ->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->once())
+            ->method('execute')
+            ->will($this->returnValue($this->cursor));
+
+        $this->cursor->expects($this->exactly(2))
+            ->method('fetch')
+            ->willReturnOnConsecutiveCalls(
+                ['id' => 4],
+                false
+            );
+
+        $result = $this->class->findAllInFeedAfter('jack', 4, 1610903351, false);
+        $this->assertEquals([Item::fromRow(['id' => 4])], $result);
+    }
+
+    public function testFindAllInFeedAfterHideRead()
+    {
+        $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->once())
+            ->method('innerJoin')
+            ->with('items', 'news_feeds', 'feeds', 'items.feed_id = feeds.id')
+            ->will($this->returnSelf());
+
+        $this->builder->expects($this->exactly(4))
+            ->method('andWhere')
+            ->withConsecutive(
+                ['items.last_modified >= :updatedSince'],
+                ['feeds.user_id = :userId'],
+                ['feeds.id = :feedId'],
+                ['items.unread = 1']
+            )
+            ->will($this->returnSelf());
+
+        $this->builder->expects($this->exactly(1))
+            ->method('setParameters')
+            ->with([
+                'updatedSince' => 1610903351,
+                'feedId' => 4,
+                'userId' => 'jack',
+            ])
+            ->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->once())
+            ->method('execute')
+            ->will($this->returnValue($this->cursor));
+
+        $this->cursor->expects($this->exactly(2))
+            ->method('fetch')
+            ->willReturnOnConsecutiveCalls(
+                ['id' => 4],
+                false
+            );
+
+        $result = $this->class->findAllInFeedAfter('jack', 4, 1610903351, true);
+        $this->assertEquals([Item::fromRow(['id' => 4])], $result);
+    }
+
+    public function testFindAllInFolderAfter()
+    {
+        $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(2))
+            ->method('innerJoin')
+            ->withConsecutive(
+                ['items', 'news_feeds', 'feeds', 'items.feed_id = feeds.id'],
+                ['feeds', 'news_folders', 'folders', 'feeds.folder_id = folders.id']
+            )
+            ->will($this->returnSelf());
+
+        $this->builder->expects($this->exactly(3))
+            ->method('andWhere')
+            ->withConsecutive(
+                ['items.last_modified >= :updatedSince'],
+                ['feeds.user_id = :userId'],
+                ['folders.id = :folderId']
+            )
+            ->will($this->returnSelf());
+
+        $this->builder->expects($this->exactly(1))
+            ->method('setParameters')
+            ->with([
+                'updatedSince' => 1610903351,
+                'folderId' => 4,
+                'userId' => 'jack',
+            ])
+            ->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->once())
+            ->method('execute')
+            ->will($this->returnValue($this->cursor));
+
+        $this->cursor->expects($this->exactly(2))
+            ->method('fetch')
+            ->willReturnOnConsecutiveCalls(
+                ['id' => 4],
+                false
+            );
+
+        $result = $this->class->findAllInFolderAfter('jack', 4, 1610903351, false);
+        $this->assertEquals([Item::fromRow(['id' => 4])], $result);
+    }
+
+    public function testFindAllInFolderAfterHideRead()
+    {
+        $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(2))
+            ->method('innerJoin')
+            ->withConsecutive(
+                ['items', 'news_feeds', 'feeds', 'items.feed_id = feeds.id'],
+                ['feeds', 'news_folders', 'folders', 'feeds.folder_id = folders.id']
+            )
+            ->will($this->returnSelf());
+
+        $this->builder->expects($this->exactly(4))
+            ->method('andWhere')
+            ->withConsecutive(
+                ['items.last_modified >= :updatedSince'],
+                ['feeds.user_id = :userId'],
+                ['folders.id = :folderId'],
+                ['items.unread = 1']
+            )
+            ->will($this->returnSelf());
+
+        $this->builder->expects($this->exactly(1))
+            ->method('setParameters')
+            ->with([
+                'updatedSince' => 1610903351,
+                'folderId' => 4,
+                'userId' => 'jack',
+            ])
+            ->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->once())
+            ->method('execute')
+            ->will($this->returnValue($this->cursor));
+
+        $this->cursor->expects($this->exactly(2))
+            ->method('fetch')
+            ->willReturnOnConsecutiveCalls(
+                ['id' => 4],
+                false
+            );
+
+        $result = $this->class->findAllInFolderAfter('jack', 4, 1610903351, true);
+        $this->assertEquals([Item::fromRow(['id' => 4])], $result);
+    }
+
+    public function testFindAllAfterUnread()
+    {
+        $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(3))
+            ->method('andWhere')
+            ->withConsecutive(
+                ['items.last_modified >= :updatedSince'],
+                ['feeds.user_id = :userId'],
+                ['items.unread = 1']
+            )
+            ->will($this->returnSelf());
+
+        $this->builder->expects($this->exactly(1))
+            ->method('setParameters')
+            ->with([
+                'updatedSince' => 1610903351,
+                'userId' => 'jack',
+            ])
+            ->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->once())
+            ->method('execute')
+            ->will($this->returnValue($this->cursor));
+
+        $this->cursor->expects($this->exactly(2))
+            ->method('fetch')
+            ->willReturnOnConsecutiveCalls(
+                ['id' => 4],
+                false
+            );
+
+        $result = $this->class->findAllAfter('jack', 6, 1610903351);
+        $this->assertEquals([Item::fromRow(['id' => 4])], $result);
+    }
+
+    public function testFindAllAfterStarred()
+    {
+        $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(3))
+            ->method('andWhere')
+            ->withConsecutive(
+                ['items.last_modified >= :updatedSince'],
+                ['feeds.user_id = :userId'],
+                ['items.starred = 1']
+            )
+            ->will($this->returnSelf());
+
+        $this->builder->expects($this->exactly(1))
+            ->method('setParameters')
+            ->with([
+                'updatedSince' => 1610903351,
+                'userId' => 'jack',
+            ])
+            ->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->once())
+            ->method('execute')
+            ->will($this->returnValue($this->cursor));
+
+        $this->cursor->expects($this->exactly(2))
+            ->method('fetch')
+            ->willReturnOnConsecutiveCalls(
+                ['id' => 4],
+                false
+            );
+
+        $result = $this->class->findAllAfter('jack', 2, 1610903351);
+        $this->assertEquals([Item::fromRow(['id' => 4])], $result);
+    }
+
+    public function testFindAllAfterAll()
+    {
+        $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(2))
+            ->method('andWhere')
+            ->withConsecutive(
+                ['items.last_modified >= :updatedSince'],
+                ['feeds.user_id = :userId']
+            )
+            ->will($this->returnSelf());
+
+        $this->builder->expects($this->exactly(1))
+            ->method('setParameters')
+            ->with([
+                'updatedSince' => 1610903351,
+                'userId' => 'jack',
+            ])
+            ->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->once())
+            ->method('execute')
+            ->will($this->returnValue($this->cursor));
+
+        $this->cursor->expects($this->exactly(2))
+            ->method('fetch')
+            ->willReturnOnConsecutiveCalls(
+                ['id' => 4],
+                false
+            );
+
+        $result = $this->class->findAllAfter('jack', 3, 1610903351);
+        $this->assertEquals([Item::fromRow(['id' => 4])], $result);
+    }
+
+    public function testFindAllAfterInvalid()
+    {
+        $this->expectException(ServiceValidationException::class);
+        $this->expectExceptionMessage('Unexpected Feed type in call');
+
+        $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(2))
+            ->method('andWhere')
+            ->withConsecutive(
+                ['items.last_modified >= :updatedSince'],
+                ['feeds.user_id = :userId']
+            )
+            ->will($this->returnSelf());
+
+        $this->builder->expects($this->exactly(1))
+            -