Different solution for ONLY_FULL_GROUP_BY (see #406) (Issue #80) (#407)

* Group by moved to subquery, get feed details from outer query (see #406)

Signed-off-by: Talon <talontalon24@gmail.com>

2 files changed, 76 insertions, 53 deletions

diff --git a/.travis.yml b/.travis.yml
index 4b2cc66a3..8279d1830 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -1,5 +1,6 @@
 os: linux
 dist: bionic
+sudo: false
 language: php
 
 php:
@@ -45,6 +46,8 @@ before_script:
     - if [[ "$DB" == 'mysql' ]]; then sudo mysql -u root -e 'CREATE DATABASE oc_autotest;'; fi
     - if [[ "$DB" == 'mysql' ]]; then sudo mysql -u root -e "CREATE USER 'oc_autotest'@'localhost' IDENTIFIED BY 'oc_autotest';"; fi
     - if [[ "$DB" == 'mysql' ]]; then sudo mysql -u root -e "GRANT ALL ON oc_autotest.* TO 'oc_autotest'@'localhost';"; fi
+    - if [[ "$DB" == 'mysql' ]]; then sudo mysql -u root -e "SET GLOBAL sql_mode = 'STRICT_ALL_TABLES,ONLY_FULL_GROUP_BY';"; fi
+
     # fill nextcloud with default configs and enable news
     - cd nextcloud
     - mkdir data
diff --git a/lib/Db/FeedMapper.php b/lib/Db/FeedMapper.php
index 6446e484d..b23ced239 100644
--- a/lib/Db/FeedMapper.php
+++ b/lib/Db/FeedMapper.php
@@ -29,18 +29,23 @@ class FeedMapper extends NewsMapper
 
     public function find($id, $userId)
     {
-        $sql = 'SELECT `feeds`.*, COUNT(`items`.`id`) AS `unread_count` ' .
+        $sql = 'SELECT `feeds`.*, `item_numbers`.`unread_count` ' .
             'FROM `*PREFIX*news_feeds` `feeds` ' .
-            'LEFT JOIN `*PREFIX*news_items` `items` ' .
-                'ON `feeds`.`id` = `items`.`feed_id` ' .
-                // WARNING: this is a desperate attempt at making this query
-                // work because prepared statements dont work. This is a
-                // POSSIBLE SQL INJECTION RISK WHEN MODIFIED WITHOUT THOUGHT.
-                // think twice when changing this
-                'AND unread = ? ' .
-            'WHERE `feeds`.`id` = ? ' .
-                'AND `feeds`.`user_id` = ? ' .
-            'GROUP BY `feeds`.`id`';
+            'JOIN ( ' .
+                'SELECT `feeds`.`id`, COUNT(`items`.`id`) AS `unread_count` ' .
+                'FROM `*PREFIX*news_feeds` `feeds` ' .
+                'LEFT JOIN `*PREFIX*news_items` `items` ' .
+                    'ON `feeds`.`id` = `items`.`feed_id` ' .
+                    // WARNING: this is a desperate attempt at making this query
+                    // work because prepared statements dont work. This is a
+                    // POSSIBLE SQL INJECTION RISK WHEN MODIFIED WITHOUT THOUGHT.
+                    // think twice when changing this
+                    'AND `items`.`unread` = ? ' .
+                'WHERE `feeds`.`id` = ? ' .
+                  'AND `feeds`.`user_id` = ? ' .
+                'GROUP BY `feeds`.`id` ' .
+            ') `item_numbers` ' .
+            'ON `item_numbers`.`id` = `feeds`.`id` ';
         $params = [true, $id, $userId];
 
         return $this->findEntity($sql, $params);
@@ -49,23 +54,28 @@ class FeedMapper extends NewsMapper
 
     public function findAllFromUser($userId)
     {
-        $sql = 'SELECT `feeds`.*, COUNT(`items`.`id`) AS `unread_count` ' .
+        $sql = 'SELECT `feeds`.*, `item_numbers`.`unread_count` ' .
             'FROM `*PREFIX*news_feeds` `feeds` ' .
-            'LEFT OUTER JOIN `*PREFIX*news_folders` `folders` ' .
-                'ON `feeds`.`folder_id` = `folders`.`id` ' .
-            'LEFT JOIN `*PREFIX*news_items` `items` ' .
-                'ON `feeds`.`id` = `items`.`feed_id` ' .
-                // WARNING: this is a desperate attempt at making this query
-                // work because prepared statements dont work. This is a
-                // POSSIBLE SQL INJECTION RISK WHEN MODIFIED WITHOUT THOUGHT.
-                // think twice when changing this
-                'AND unread = ? ' .
-            'WHERE `feeds`.`user_id` = ? ' .
-            'AND (`feeds`.`folder_id` = 0 ' .
-                'OR `folders`.`deleted_at` = 0' .
-            ')' .
-            'AND `feeds`.`deleted_at` = 0 ' .
-            'GROUP BY `feeds`.`id`';
+            'JOIN ( ' .
+                'SELECT `feeds`.`id`, COUNT(`items`.`id`) AS `unread_count` ' .
+                'FROM `*PREFIX*news_feeds` `feeds` ' .
+                'LEFT OUTER JOIN `*PREFIX*news_folders` `folders` ' .
+                    'ON `feeds`.`folder_id` = `folders`.`id` ' .
+                'LEFT JOIN `*PREFIX*news_items` `items` ' .
+                    'ON `feeds`.`id` = `items`.`feed_id` ' .
+                    // WARNING: this is a desperate attempt at making this query
+                    // work because prepared statements dont work. This is a
+                    // POSSIBLE SQL INJECTION RISK WHEN MODIFIED WITHOUT THOUGHT.
+                    // think twice when changing this
+                    'AND `items`.`unread` = ? ' .
+                'WHERE `feeds`.`user_id` = ? ' .
+                  'AND (`feeds`.`folder_id` = 0 ' .
+                   'OR `folders`.`deleted_at` = 0 ' .
+                  ') ' .
+                  'AND `feeds`.`deleted_at` = 0 ' .
+                'GROUP BY `feeds`.`id` ' .
+            ') `item_numbers` ' .
+            'ON `item_numbers`.`id` = `feeds`.`id` ';
         $params = [true, $userId];
 
         return $this->findEntities($sql, $params);
@@ -74,22 +84,27 @@ class FeedMapper extends NewsMapper
 
     public function findAll()
     {
-        $sql = 'SELECT `feeds`.*, COUNT(`items`.`id`) AS `unread_count` ' .
+        $sql = 'SELECT `feeds`.*, `item_numbers`.`unread_count` ' .
             'FROM `*PREFIX*news_feeds` `feeds` ' .
-            'LEFT OUTER JOIN `*PREFIX*news_folders` `folders` ' .
-                'ON `feeds`.`folder_id` = `folders`.`id` ' .
-            'LEFT JOIN `*PREFIX*news_items` `items` ' .
-                'ON `feeds`.`id` = `items`.`feed_id` ' .
-                // WARNING: this is a desperate attempt at making this query
-                // work because prepared statements dont work. This is a
-                // POSSIBLE SQL INJECTION RISK WHEN MODIFIED WITHOUT THOUGHT.
-                // think twice when changing this
-                'AND unread = ? ' .
-            'WHERE (`feeds`.`folder_id` = 0 ' .
-                'OR `folders`.`deleted_at` = 0' .
-            ')' .
-            'AND `feeds`.`deleted_at` = 0 ' .
-            'GROUP BY `feeds`.`id`';
+            'JOIN ( ' .
+                'SELECT `feeds`.`id`, COUNT(`items`.`id`) AS `unread_count` ' .
+                'FROM `*PREFIX*news_feeds` `feeds` ' .
+                'LEFT OUTER JOIN `*PREFIX*news_folders` `folders` ' .
+                    'ON `feeds`.`folder_id` = `folders`.`id` ' .
+                'LEFT JOIN `*PREFIX*news_items` `items` ' .
+                    'ON `feeds`.`id` = `items`.`feed_id` ' .
+                    // WARNING: this is a desperate attempt at making this query
+                    // work because prepared statements dont work. This is a
+                    // POSSIBLE SQL INJECTION RISK WHEN MODIFIED WITHOUT THOUGHT.
+                    // think twice when changing this
+                    'AND `items`.`unread` = ? ' .
+                'WHERE (`feeds`.`folder_id` = 0 ' .
+                   'OR `folders`.`deleted_at` = 0 ' .
+                ') ' .
+                'AND `feeds`.`deleted_at` = 0 ' .
+                'GROUP BY `feeds`.`id` ' .
+            ') `item_numbers` ' .
+            'ON `item_numbers`.`id` = `feeds`.`id` ';
 
         return $this->findEntities($sql, [true]);
     }
@@ -97,18 +112,23 @@ class FeedMapper extends NewsMapper
 
     public function findByUrlHash($hash, $userId)
     {
-        $sql = 'SELECT `feeds`.*, COUNT(`items`.`id`) AS `unread_count` ' .
+        $sql = 'SELECT `feeds`.*, `item_numbers`.`unread_count` ' .
             'FROM `*PREFIX*news_feeds` `feeds` ' .
-            'LEFT JOIN `*PREFIX*news_items` `items` ' .
-                'ON `feeds`.`id` = `items`.`feed_id` ' .
-                // WARNING: this is a desperate attempt at making this query
-                // work because prepared statements dont work. This is a
-                // POSSIBLE SQL INJECTION RISK WHEN MODIFIED WITHOUT THOUGHT.
-                // think twice when changing this
-                'AND unread = ? ' .
-            'WHERE `feeds`.`url_hash` = ? ' .
-                'AND `feeds`.`user_id` = ? ' .
-            'GROUP BY `feeds`.`id`';
+            'JOIN ( ' .
+                'SELECT `feeds`.`id`, COUNT(`items`.`id`) AS `unread_count` ' .
+                'FROM `*PREFIX*news_feeds` `feeds` ' .
+                'LEFT JOIN `*PREFIX*news_items` `items` ' .
+                    'ON `feeds`.`id` = `items`.`feed_id` ' .
+                    // WARNING: this is a desperate attempt at making this query
+                    // work because prepared statements dont work. This is a
+                    // POSSIBLE SQL INJECTION RISK WHEN MODIFIED WITHOUT THOUGHT.
+                    // think twice when changing this
+                    'AND `items`.`unread` = ? ' .
+                'WHERE `feeds`.`url_hash` = ? ' .
+                  'AND `feeds`.`user_id` = ? ' .
+                'GROUP BY `feeds`.`id` ' .
+            ') `item_numbers` ' .
+            'ON `item_numbers`.`id` = `feeds`.`id` ';
         $params = [true, $hash, $userId];
 
         return $this->findEntity($sql, $params);