From cc051c6ce7d32f34f25b45e815535f168521d9c1 Mon Sep 17 00:00:00 2001 From: Talon24 Date: Sun, 1 Mar 2020 15:49:39 +0100 Subject: 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 --- .travis.yml | 3 ++ lib/Db/FeedMapper.php | 126 +++++++++++++++++++++++++++++--------------------- 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); -- cgit v1.2.3