DB: Updates should use set()

Issue GH-1211 Signed-off-by: Sean Molenaar <sean@seanmolenaar.eu>
nextcloud · Mar 17, 2021 · 8a0ad2f · 8a0ad2f
1 parent 5ab065b
commit 8a0ad2f
Show file tree

Hide file tree

Showing 7 changed files with 113 additions and 45 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -10,6 +10,7 @@ The format is almost based on [Keep a Changelog](https://keepachangelog.com/en/1
 - Update serialization of item to include categories (#1248)
 
 ### Fixed
+- Fix update queries (#1211)
 
 ## [15.4.0-beta2] - 2021-02-27
 ### Fixed

diff --git a/lib/Db/FeedMapperV2.php b/lib/Db/FeedMapperV2.php
@@ -13,12 +13,14 @@
 
 namespace OCA\News\Db;
 
+use OC\DB\QueryBuilder\Parameter;
 use OCA\News\Utility\Time;
 use OCP\AppFramework\Db\DoesNotExistException;
 use OCP\AppFramework\Db\MultipleObjectsReturnedException;
 use OCP\DB\QueryBuilder\IQueryBuilder;
 use OCP\IDBConnection;
 use OCP\AppFramework\Db\Entity;
+use PDO;
 
 /**
  * Class FeedMapper
@@ -158,23 +160,34 @@ public function findAllFromFolder(?int $id): array
      * @param string   $userId
      * @param int      $id
      * @param int|null $maxItemID
+     *
+     * @throws \OCP\DB\Exception
+     *
+     * @TODO Update for NC 21
      */
     public function read(string $userId, int $id, ?int $maxItemID = null): void
     {
-        $builder = $this->db->getQueryBuilder();
-        $builder->update(ItemMapperV2::TABLE_NAME, 'items')
+        $idBuilder = $this->db->getQueryBuilder();
+        $idBuilder->select('items.id')
+            ->from(ItemMapperV2::TABLE_NAME, 'items')
             ->innerJoin('items', FeedMapperV2::TABLE_NAME, 'feeds', 'items.feed_id = feeds.id')
-            ->setValue('unread', 0)
             ->andWhere('feeds.user_id = :userId')
-            ->andWhere('feeds.id = :feedId')
-            ->setParameter('userId', $userId)
-            ->setParameter('feedId', $id);
+            ->andWhere('feeds.id = :feedId');
+
+        $idList = $this->db->executeQuery($idBuilder->getSQL(), $idBuilder->getParameters())
+                           ->fetch(PDO::FETCH_COLUMN);
+
+        $builder = $this->db->getQueryBuilder();
+        $builder->update(ItemMapperV2::TABLE_NAME)
+            ->set('unread', $builder->createParameter('unread'))
+            ->where($builder->expr()->in('id', $idList))
+            ->setParameter('unread', false);
 
         if ($maxItemID !== null) {
-            $builder->andWhere('items.id =< :maxItemId')
+            $builder->andWhere('id <= :maxItemId')
                 ->setParameter('maxItemId', $maxItemID);
         }
 
-        $this->db->executeUpdate($builder->getSQL());
+        $this->db->executeUpdate($builder->getSQL(), $builder->getParameters());
     }
 }
diff --git a/lib/Db/FolderMapperV2.php b/lib/Db/FolderMapperV2.php
@@ -16,6 +16,7 @@
 use OCA\News\Utility\Time;
 use OCP\AppFramework\Db\Entity;
 use OCP\IDBConnection;
+use PDO;
 
 /**
  * Class FolderMapper
@@ -102,23 +103,35 @@ public function findFromUser(string $userId, int $id): Entity
      * @param int|null $maxItemID
      *
      * @return void
+     * @throws \OCP\DB\Exception
+     *
+     * @TODO Update for NC 21
      */
     public function read(string $userId, int $id, ?int $maxItemID = null): void
     {
+        $idBuilder = $this->db->getQueryBuilder();
+        $idBuilder->select('items.id')
+                  ->from(ItemMapperV2::TABLE_NAME, 'items')
+                  ->innerJoin('items', FeedMapperV2::TABLE_NAME, 'feeds', 'items.feed_id = feeds.id')
+                  ->andWhere('feeds.user_id = :userId')
+                  ->andWhere('feeds.folder_id = :folderId')
+                  ->setParameter('userId', $userId)
+                  ->setParameter('folderId', $id);
+
+        $idList = $this->db->executeQuery($idBuilder->getSQL(), $idBuilder->getParameters())
+                           ->fetch(PDO::FETCH_COLUMN);
+
         $builder = $this->db->getQueryBuilder();
-        $builder->update(ItemMapperV2::TABLE_NAME, 'items')
-            ->innerJoin('items', FeedMapperV2::TABLE_NAME, 'feeds', 'items.feed_id = feeds.id')
-            ->setValue('unread', 0)
-            ->andWhere('feeds.user_id = :userId')
-            ->andWhere('feeds.folder_id = :folderId')
-            ->setParameter('userId', $userId)
-            ->setParameter('folderId', $id);
+        $builder->update(ItemMapperV2::TABLE_NAME)
+            ->set('unread', $builder->createParameter('unread'))
+            ->where($builder->expr()->in('id', $idList))
+            ->setParameter('unread', false);
 
         if ($maxItemID !== null) {
-            $builder->andWhere('items.id =< :maxItemId')
+            $builder->andWhere('id <= :maxItemId')
                     ->setParameter('maxItemId', $maxItemID);
         }
 
-        $this->db->executeUpdate($builder->getSQL());
+        $this->db->executeUpdate($builder->getSQL(), $builder->getParameters());
     }
 }
diff --git a/lib/Db/ItemMapperV2.php b/lib/Db/ItemMapperV2.php
@@ -13,6 +13,7 @@
 namespace OCA\News\Db;
 
 use OC\DB\QueryBuilder\Literal;
+use OC\DB\QueryBuilder\Parameter;
 use OCA\News\Service\Exceptions\ServiceValidationException;
 use Doctrine\DBAL\FetchMode;
 use OCA\News\Utility\Time;
@@ -254,20 +255,32 @@ public function purgeDeleted(?string $userID, ?int $oldestDelete): void
      * @param int    $maxItemId
      *
      * @TODO: Update this for NC 21
+     *
+     * @throws \OCP\DB\Exception
      */
     public function readAll(string $userId, int $maxItemId): void
     {
+        $idBuilder = $this->db->getQueryBuilder();
+        $idBuilder->select('id')
+            ->from(ItemMapperV2::TABLE_NAME, 'items')
+            ->innerJoin('items', FeedMapperV2::TABLE_NAME, 'feeds', 'items.feed_id = feeds.id')
+            ->andWhere('feeds.user_id = :userId')
+            ->setParameter('userId', $userId);
+
+        $idList = $this->db->executeQuery($idBuilder->getSQL(), $idBuilder->getParameters())
+            ->fetch(PDO::FETCH_COLUMN);
+
         $builder = $this->db->getQueryBuilder();
 
         $builder->update($this->tableName, 'items')
                 ->innerJoin('items', FeedMapperV2::TABLE_NAME, 'feeds', 'items.feed_id = feeds.id')
-                ->setValue('unread', 0)
-                ->andWhere('items.id =< :maxItemId')
-                ->andWhere('feeds.user_id = :userId')
+                ->set('unread', $builder->createParameter('unread'))
+                ->andWhere('items.id <= :maxItemId')
+                ->where($builder->expr()->in('id', $idList))
                 ->setParameter('maxItemId', $maxItemId)
-                ->setParameter('userId', $userId);
+                ->setParameter('unread', false);
 
-        $this->db->executeUpdate($builder->getSQL());
+        $this->db->executeUpdate($builder->getSQL(), $builder->getParameters());
     }
 
     /**

diff --git a/tests/Unit/Db/FeedMapperTest.php b/tests/Unit/Db/FeedMapperTest.php
@@ -13,6 +13,7 @@
 
 namespace OCA\News\Tests\Unit\Db;
 
+use OC\DB\QueryBuilder\Parameter;
 use OCA\News\Db\Feed;
 use OCA\News\Db\FeedMapperV2;
 use OCA\News\Utility\Time;
@@ -466,24 +467,29 @@ public function testRead()
             ->with('news_items', 'items')
             ->will($this->returnSelf());
 
+        $this->builder->expects($this->once())
+            ->method('createParameter')
+            ->with('unread')
+            ->will($this->returnArgument(0));
+
         $this->builder->expects($this->once())
             ->method('innerJoin')
             ->with('items', 'news_feeds', 'feeds', 'items.feed_id = feeds.id')
             ->will($this->returnSelf());
 
         $this->builder->expects($this->once())
-            ->method('setValue')
-            ->with('unread', 0)
+            ->method('set')
+            ->with('unread', 'unread')
             ->will($this->returnSelf());
 
         $this->builder->expects($this->exactly(2))
             ->method('andWhere')
             ->withConsecutive(['feeds.user_id = :userId'], ['feeds.id = :feedId'])
             ->will($this->returnSelf());
 
-        $this->builder->expects($this->exactly(2))
+        $this->builder->expects($this->exactly(3))
             ->method('setParameter')
-            ->withConsecutive(['userId', 'admin'], ['feedId', 1])
+            ->withConsecutive(['userId', 'admin'], ['feedId', 1], ['unread', false])
             ->will($this->returnSelf());
 
         $this->builder->expects($this->exactly(1))
@@ -506,6 +512,11 @@ public function testReadWithMaxId()
             ->method('getQueryBuilder')
             ->willReturn($this->builder);
 
+        $this->builder->expects($this->once())
+            ->method('createParameter')
+            ->with('unread')
+            ->will($this->returnArgument(0));
+
         $this->builder->expects($this->once())
             ->method('update')
             ->with('news_items', 'items')
@@ -517,18 +528,18 @@ public function testReadWithMaxId()
             ->will($this->returnSelf());
 
         $this->builder->expects($this->once())
-            ->method('setValue')
-            ->with('unread', 0)
+            ->method('set')
+            ->with('unread', 'unread')
             ->will($this->returnSelf());
 
         $this->builder->expects($this->exactly(3))
             ->method('andWhere')
             ->withConsecutive(['feeds.user_id = :userId'], ['feeds.id = :feedId'], ['items.id =< :maxItemId'])
             ->will($this->returnSelf());
 
-        $this->builder->expects($this->exactly(3))
+        $this->builder->expects($this->exactly(4))
             ->method('setParameter')
-            ->withConsecutive(['userId', 'admin'], ['feedId', 1], ['maxItemId', 4])
+            ->withConsecutive(['userId', 'admin'], ['feedId', 1], ['unread', false], ['maxItemId', 4])
             ->will($this->returnSelf());
 
         $this->builder->expects($this->exactly(1))
@@ -541,4 +552,4 @@ public function testReadWithMaxId()
 
         $this->class->read('admin', 1, 4);
     }
-}
+}
diff --git a/tests/Unit/Db/FolderMapperTest.php b/tests/Unit/Db/FolderMapperTest.php
@@ -13,6 +13,7 @@
 
 namespace OCA\News\Tests\Unit\Db;
 
+use OC\DB\QueryBuilder\Parameter;
 use OCA\News\Db\Folder;
 use OCA\News\Db\FolderMapperV2;
 use OCA\News\Utility\Time;
@@ -289,6 +290,11 @@ public function testRead()
             ->method('getQueryBuilder')
             ->willReturn($this->builder);
 
+        $this->builder->expects($this->once())
+            ->method('createParameter')
+            ->with('unread')
+            ->will($this->returnArgument(0));
+
         $this->builder->expects($this->once())
             ->method('update')
             ->with('news_items', 'items')
@@ -300,18 +306,18 @@ public function testRead()
             ->will($this->returnSelf());
 
         $this->builder->expects($this->once())
-            ->method('setValue')
-            ->with('unread', 0)
+            ->method('set')
+            ->with('unread', 'unread')
             ->will($this->returnSelf());
 
         $this->builder->expects($this->exactly(2))
             ->method('andWhere')
             ->withConsecutive(['feeds.user_id = :userId'], ['feeds.folder_id = :folderId'])
             ->will($this->returnSelf());
 
-        $this->builder->expects($this->exactly(2))
+        $this->builder->expects($this->exactly(3))
             ->method('setParameter')
-            ->withConsecutive(['userId', 'admin'], ['folderId', 1])
+            ->withConsecutive(['userId', 'admin'], ['folderId', 1], ['unread', false])
             ->will($this->returnSelf());
 
         $this->builder->expects($this->exactly(1))
@@ -334,6 +340,11 @@ public function testReadWithMaxId()
             ->method('getQueryBuilder')
             ->willReturn($this->builder);
 
+        $this->builder->expects($this->once())
+            ->method('createParameter')
+            ->with('unread')
+            ->will($this->returnArgument(0));
+
         $this->builder->expects($this->once())
             ->method('update')
             ->with('news_items', 'items')
@@ -345,18 +356,18 @@ public function testReadWithMaxId()
             ->will($this->returnSelf());
 
         $this->builder->expects($this->once())
-            ->method('setValue')
-            ->with('unread', 0)
+            ->method('set')
+            ->with('unread', 'unread')
             ->will($this->returnSelf());
 
         $this->builder->expects($this->exactly(3))
             ->method('andWhere')
             ->withConsecutive(['feeds.user_id = :userId'], ['feeds.folder_id = :folderId'], ['items.id =< :maxItemId'])
             ->will($this->returnSelf());
 
-        $this->builder->expects($this->exactly(3))
+        $this->builder->expects($this->exactly(4))
             ->method('setParameter')
-            ->withConsecutive(['userId', 'admin'], ['folderId', 1], ['maxItemId', 4])
+            ->withConsecutive(['userId', 'admin'], ['folderId', 1], ['unread', false], ['maxItemId', 4])
             ->will($this->returnSelf());
 
         $this->builder->expects($this->exactly(1))
@@ -369,4 +380,4 @@ public function testReadWithMaxId()
 
         $this->class->read('admin', 1, 4);
     }
-}
+}
diff --git a/tests/Unit/Db/ItemMapperTest.php b/tests/Unit/Db/ItemMapperTest.php
@@ -14,6 +14,7 @@
 namespace OCA\News\Tests\Unit\Db;
 
 use OC\DB\QueryBuilder\Literal;
+use OC\DB\QueryBuilder\Parameter;
 use OCA\News\Db\Feed;
 use OCA\News\Db\FeedMapperV2;
 use OCA\News\Db\Folder;
@@ -476,6 +477,11 @@ public function testReadAll()
             ->method('getQueryBuilder')
             ->willReturn($this->builder);
 
+        $this->builder->expects($this->once())
+            ->method('createParameter')
+            ->with('unread')
+            ->will($this->returnArgument(0));
+
         $this->builder->expects($this->once())
             ->method('update')
             ->with('news_items', 'items')
@@ -487,18 +493,18 @@ public function testReadAll()
             ->will($this->returnSelf());
 
         $this->builder->expects($this->once())
-            ->method('setValue')
-            ->with('unread', 0)
+            ->method('set')
+            ->with('unread', 'unread')
             ->will($this->returnSelf());
 
         $this->builder->expects($this->exactly(2))
             ->method('andWhere')
             ->withConsecutive(['items.id =< :maxItemId'], ['feeds.user_id = :userId'])
             ->will($this->returnSelf());
 
-        $this->builder->expects($this->exactly(2))
+        $this->builder->expects($this->exactly(3))
             ->method('setParameter')
-            ->withConsecutive(['maxItemId', 4], ['userId', 'jack'])
+            ->withConsecutive(['maxItemId', 4], ['userId', 'jack'], ['unread', false])
             ->will($this->returnSelf());
 
         $this->builder->expects($this->exactly(1))
@@ -1055,4 +1061,4 @@ public function testDeleteOverThresholdSuccessUnreadSkipsIfUnderThreshold()
         $this->assertSame(10, $res);
     }
 
-}
+}