diff options
author | Daniel Opitz <danopz@users.noreply.github.com> | 2017-08-14 10:34:53 +0200 |
---|---|---|
committer | Bernhard Posselt <BernhardPosselt@users.noreply.github.com> | 2017-08-14 10:34:53 +0200 |
commit | a97dd58e3b499b60ac8b37786d402d7f2e371a88 (patch) | |
tree | 98bb8a6c750fb33fbef38d22407fa29fbf6c7b1e | |
parent | 7d8a85c82c4c13a71b70ddb4ecb8c40ede4c9b70 (diff) |
Split binary to booleans (#203)
* replaced old status with 2 flags for unread and starred
* add fields to db, replace int(1,0) with booleans in sql queries, removed StatusFlags class + refactor code relying to it
* add repair step for migration
* again use integer(1,0) instead of bool in sql queries, because of sqlite doesn't support true/false
* add/fix unit tests for new boolean status
* set unread/starred flags as statements in sql
* fixed mysql unknown column items.unread, fixed marking of read items on repair step
* remove unnecessary bool casts
* add empty checks to Items::is* methods
* update migration to use native sql instead of the querybuilder
* don't cast the flags manually, let the api do the work
27 files changed, 548 insertions, 1299 deletions
diff --git a/appinfo/database.xml b/appinfo/database.xml index 1044208cd..e9c65a947 100644 --- a/appinfo/database.xml +++ b/appinfo/database.xml @@ -364,6 +364,18 @@ <notnull>true</notnull> </field> <field> + <name>unread</name> + <type>boolean</type> + <default>false</default> + <notnull>true</notnull> + </field> + <field> + <name>starred</name> + <type>boolean</type> + <default>false</default> + <notnull>true</notnull> + </field> + <field> <name>last_modified</name> <type>integer</type> <default>0</default> diff --git a/appinfo/info.xml b/appinfo/info.xml index 6f97c052f..099f3d824 100644 --- a/appinfo/info.xml +++ b/appinfo/info.xml @@ -8,7 +8,7 @@ Before you update to a new version, [check the changelog](https://github.com/nextcloud/news/blob/master/CHANGELOG.md) to avoid surprises. **Important**: To enable feed updates you will need to enable either [Nextcloud system cron](https://docs.nextcloud.com/server/10/admin_manual/configuration_server/background_jobs_configuration.html#cron) or use [an updater](https://github.com/nextcloud/news-updater) which uses the built in update API and disable cron updates. More information can be found [in the README](https://github.com/nextcloud/news).]]></description> - <version>11.0.5</version> + <version>11.0.6</version> <licence>agpl</licence> <author>Bernhard Posselt</author> <author>Alessandro Cosentino</author> @@ -42,6 +42,12 @@ Before you update to a new version, [check the changelog](https://github.com/nex <job>OCA\News\Cron\Updater</job> </background-jobs> + <repair-steps> + <post-migration> + <step>OCA\News\Migration\MigrateStatusFlags</step> + </post-migration> + </repair-steps> + <settings> <admin>OCA\News\Settings\Admin</admin> <admin-section>OCA\News\Settings\Section</admin-section> diff --git a/lib/Db/FeedMapper.php b/lib/Db/FeedMapper.php index 056296e49..ad54ffc37 100644 --- a/lib/Db/FeedMapper.php +++ b/lib/Db/FeedMapper.php @@ -35,12 +35,11 @@ class FeedMapper extends NewsMapper { // work because prepared statements dont work. This is a // POSSIBLE SQL INJECTION RISK WHEN MODIFIED WITHOUT THOUGHT. // think twice when changing this - 'AND (`items`.`status` & ' . StatusFlag::UNREAD . ') = ' . - StatusFlag::UNREAD . ' ' . + 'AND unread = ? ' . 'WHERE `feeds`.`id` = ? ' . 'AND `feeds`.`user_id` = ? ' . 'GROUP BY `feeds`.`id`'; - $params = [$id, $userId]; + $params = [true, $id, $userId]; return $this->findEntity($sql, $params); } @@ -57,15 +56,14 @@ class FeedMapper extends NewsMapper { // work because prepared statements dont work. This is a // POSSIBLE SQL INJECTION RISK WHEN MODIFIED WITHOUT THOUGHT. // think twice when changing this - 'AND (`items`.`status` & ' . StatusFlag::UNREAD . ') = ' . - StatusFlag::UNREAD . ' ' . + '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`'; - $params = [$userId]; + $params = [true, $userId]; return $this->findEntities($sql, $params); } @@ -82,15 +80,14 @@ class FeedMapper extends NewsMapper { // work because prepared statements dont work. This is a // POSSIBLE SQL INJECTION RISK WHEN MODIFIED WITHOUT THOUGHT. // think twice when changing this - 'AND (`items`.`status` & ' . StatusFlag::UNREAD . ') = ' . - StatusFlag::UNREAD . ' ' . + 'AND unread = ? ' . 'WHERE (`feeds`.`folder_id` = 0 ' . 'OR `folders`.`deleted_at` = 0' . ')' . 'AND `feeds`.`deleted_at` = 0 ' . 'GROUP BY `feeds`.`id`'; - return $this->findEntities($sql); + return $this->findEntities($sql, [true]); } @@ -103,12 +100,11 @@ class FeedMapper extends NewsMapper { // work because prepared statements dont work. This is a // POSSIBLE SQL INJECTION RISK WHEN MODIFIED WITHOUT THOUGHT. // think twice when changing this - 'AND (`items`.`status` & ' . StatusFlag::UNREAD . ') = ' . - StatusFlag::UNREAD . ' ' . + 'AND unread = ? ' . 'WHERE `feeds`.`url_hash` = ? ' . 'AND `feeds`.`user_id` = ? ' . 'GROUP BY `feeds`.`id`'; - $params = [$hash, $userId]; + $params = [true, $hash, $userId]; return $this->findEntity($sql, $params); } diff --git a/lib/Db/Item.php b/lib/Db/Item.php index f19843c8e..a781048fd 100644 --- a/lib/Db/Item.php +++ b/lib/Db/Item.php @@ -37,14 +37,14 @@ use \OCP\AppFramework\Db\Entity; * @method void setEnclosureLink(string $value) * @method integer getFeedId() * @method void setFeedId(integer $value) - * @method integer getStatus() - * @method void setStatus(integer $value) * @method void setRtl(boolean $value) * @method string getLastModified() * @method void setLastModified(string $value) * @method void setFingerprint(string $value) * @method void setContentHash(string $value) * @method void setSearchIndex(string $value) + * @method void setUnread(bool $value) + * @method void setStarred(bool $value) */ class Item extends Entity implements IAPI, \JsonSerializable { @@ -67,53 +67,28 @@ class Item extends Entity implements IAPI, \JsonSerializable { protected $searchIndex; protected $rtl; protected $fingerprint; + protected $unread = false; + protected $starred = false; public function __construct() { $this->addType('pubDate', 'integer'); $this->addType('updatedDate', 'integer'); $this->addType('feedId', 'integer'); - $this->addType('status', 'integer'); $this->addType('rtl', 'boolean'); - } - - public function setRead() { - $this->markFieldUpdated('status'); - $this->status &= ~StatusFlag::UNREAD; - } - - public function isRead() { - return !(($this->status & StatusFlag::UNREAD) === StatusFlag::UNREAD); - } - - public function setUnread() { - $this->markFieldUpdated('status'); - $this->status |= StatusFlag::UNREAD; + $this->addType('unread', 'boolean'); + $this->addType('starred', 'boolean'); } public function isUnread() { - return !$this->isRead(); - } - - public function setStarred() { - $this->markFieldUpdated('status'); - $this->status |= StatusFlag::STARRED; + return $this->unread; } public function isStarred() { - return ($this->status & StatusFlag::STARRED) === StatusFlag::STARRED; - } - - public function setUnstarred() { - $this->markFieldUpdated('status'); - $this->status &= ~StatusFlag::STARRED; - } - - public function isUnstarred() { - return !$this->isStarred(); + return $this->starred; } /** - * Turns entitie attributes into an array + * Turns entity attributes into an array */ public function jsonSerialize() { return [ @@ -196,16 +171,8 @@ class Item extends Entity implements IAPI, \JsonSerializable { $item->setEnclosureMime($import['enclosureMime']); $item->setEnclosureLink($import['enclosureLink']); $item->setRtl($import['rtl']); - if ($import['unread']) { - $item->setUnread(); - } else { - $item->setRead(); - } - if ($import['starred']) { - $item->setStarred(); - } else { - $item->setUnstarred(); - } + $item->setUnread($import['unread']); + $item->setStarred($import['starred']); return $item; } diff --git a/lib/Db/ItemMapper.php b/lib/Db/ItemMapper.php index b49cdd5ea..777d0d392 100644 --- a/lib/Db/ItemMapper.php +++ b/lib/Db/ItemMapper.php @@ -15,6 +15,7 @@ namespace OCA\News\Db; use Exception; use OCA\News\Utility\Time; +use OCP\DB\QueryBuilder\IQueryBuilder; use OCP\IDBConnection; @@ -45,18 +46,28 @@ class ItemMapper extends NewsMapper { 'ORDER BY `items`.`id` ' . $ordering; } - private function makeSelectQueryStatus($prependTo, $status, - $oldestFirst = false, $search = [], - $distinctFingerprint = false) { - $status = (int)$status; - $count = count($search); + /** + * check if type is feed or all items should be shown + * @param bool $showAll + * @param int|null $type + * @return string + */ + private function buildStatusQueryPart($showAll, $type = null) { + $sql = ''; + + if (isset($type) && $type === FeedType::STARRED) { + $sql = 'AND `items`.`starred` = '; + $sql .= $this->db->quote(true, IQueryBuilder::PARAM_BOOL) . ' '; + } elseif (!$showAll) { + $sql .= 'AND `items`.`unread` = '; + $sql .= $this->db->quote(true, IQueryBuilder::PARAM_BOOL) . ' '; + } - // WARNING: Potential SQL injection if you change this carelessly - $sql = 'AND ((`items`.`status` & ' . $status . ') = ' . $status . ') '; - $sql .= str_repeat('AND `items`.`search_index` LIKE ? ', $count); - $sql .= $prependTo; + return $sql; + } - return $this->makeSelectQuery($sql, $oldestFirst, $distinctFingerprint); + private function buildSearchQueryPart(array $search = []) { + return str_repeat('AND `items`.`search_index` LIKE ? ', count($search)); } /** @@ -88,14 +99,13 @@ class ItemMapper extends NewsMapper { 'ON `feeds`.`id` = `items`.`feed_id` ' . 'AND `feeds`.`deleted_at` = 0 ' . 'AND `feeds`.`user_id` = ? ' . - 'AND ((`items`.`status` & ' . StatusFlag::STARRED . ') = ' . - StatusFlag::STARRED . ')' . + 'AND `items`.`starred` = ? ' . 'LEFT OUTER JOIN `*PREFIX*news_folders` `folders` ' . 'ON `folders`.`id` = `feeds`.`folder_id` ' . 'WHERE `feeds`.`folder_id` = 0 ' . 'OR `folders`.`deleted_at` = 0'; - $params = [$userId]; + $params = [$userId, true]; $result = $this->execute($sql, $params)->fetch(); @@ -105,21 +115,21 @@ class ItemMapper extends NewsMapper { public function readAll($highestItemId, $time, $userId) { $sql = 'UPDATE `*PREFIX*news_items` ' . - 'SET `status` = `status` & ? ' . + 'SET unread = ? ' . ', `last_modified` = ? ' . 'WHERE `feed_id` IN (' . 'SELECT `id` FROM `*PREFIX*news_feeds` ' . 'WHERE `user_id` = ? ' . ') ' . 'AND `id` <= ?'; - $params = [~StatusFlag::UNREAD, $time, $userId, $highestItemId]; + $params = [false, $time, $userId, $highestItemId]; $this->execute($sql, $params); } public function readFolder($folderId, $highestItemId, $time, $userId) { $sql = 'UPDATE `*PREFIX*news_items` ' . - 'SET `status` = `status` & ? ' . + 'SET unread = ? ' . ', `last_modified` = ? ' . 'WHERE `feed_id` IN (' . 'SELECT `id` FROM `*PREFIX*news_feeds` ' . @@ -127,7 +137,7 @@ class ItemMapper extends NewsMapper { 'AND `user_id` = ? ' . ') ' . 'AND `id` <= ?'; - $params = [~StatusFlag::UNREAD, $time, $folderId, $userId, + $params = [false, $time, $folderId, $userId, $highestItemId]; $this->execute($sql, $params); } @@ -135,7 +145,7 @@ class ItemMapper extends NewsMapper { public function readFeed($feedId, $highestItemId, $time, $userId) { $sql = 'UPDATE `*PREFIX*news_items` ' . - 'SET `status` = `status` & ? ' . + 'SET unread = ? ' . ', `last_modified` = ? ' . 'WHERE `feed_id` = ? ' . 'AND `id` <= ? ' . @@ -143,7 +153,7 @@ class ItemMapper extends NewsMapper { 'SELECT * FROM `*PREFIX*news_feeds` ' . 'WHERE `user_id` = ? ' . 'AND `id` = ? ) '; - $params = [~StatusFlag::UNREAD, $time, $feedId, $highestItemId, + $params = [false, $time, $feedId, $highestItemId, $userId, $feedId]; $this->execute($sql, $params); @@ -159,27 +169,33 @@ class ItemMapper extends NewsMapper { } - public function findAllNew($updatedSince, $status, $userId) { - $sql = $this->makeSelectQueryStatus( - 'AND `items`.`last_modified` >= ? ', $status); + public function findAllNew($updatedSince, $type, $showAll, $userId) { + $sql = $this->buildStatusQueryPart($showAll, $type); + + $sql .= 'AND `items`.`last_modified` >= ? '; + $sql = $this->makeSelectQuery($sql); $params = [$userId, $updatedSince]; return $this->findEntities($sql, $params); } - public function findAllNewFolder($id, $updatedSince, $status, $userId) { - $sql = 'AND `feeds`.`folder_id` = ? ' . + public function findAllNewFolder($id, $updatedSince, $showAll, $userId) { + $sql = $this->buildStatusQueryPart($showAll); + + $sql .= 'AND `feeds`.`folder_id` = ? ' . 'AND `items`.`last_modified` >= ? '; - $sql = $this->makeSelectQueryStatus($sql, $status); + $sql = $this->makeSelectQuery($sql); $params = [$userId, $id, $updatedSince]; return $this->findEntities($sql, $params); } - public function findAllNewFeed($id, $updatedSince, $status, $userId) { - $sql = 'AND `items`.`feed_id` = ? ' . + public function findAllNewFeed($id, $updatedSince, $showAll, $userId) { + $sql = $this->buildStatusQueryPart($showAll); + + $sql .= 'AND `items`.`feed_id` = ? ' . 'AND `items`.`last_modified` >= ? '; - $sql = $this->makeSelectQueryStatus($sql, $status); + $sql = $this->makeSelectQuery($sql); $params = [$userId, $id, $updatedSince]; return $this->findEntities($sql, $params); } @@ -195,63 +211,68 @@ class ItemMapper extends NewsMapper { } - public function findAllFeed($id, $limit, $offset, $status, $oldestFirst, + public function findAllFeed($id, $limit, $offset, $showAll, $oldestFirst, $userId, $search = []) { $params = [$userId]; $params = array_merge($params, $this->buildLikeParameters($search)); $params[] = $id; - $sql = 'AND `items`.`feed_id` = ? '; + $sql = $this->buildStatusQueryPart($showAll); + $sql .= $this->buildSearchQueryPart($search); + + $sql .= 'AND `items`.`feed_id` = ? '; if ($offset !== 0) { $sql .= 'AND `items`.`id` ' . $this->getOperator($oldestFirst) . ' ? '; $params[] = $offset; } - $sql = $this->makeSelectQueryStatus($sql, $status, $oldestFirst, - $search); + $sql = $this->makeSelectQuery($sql, $oldestFirst, $search); return $this->findEntitiesIgnoringNegativeLimit($sql, $params, $limit); } - public function findAllFolder($id, $limit, $offset, $status, $oldestFirst, + public function findAllFolder($id, $limit, $offset, $showAll, $oldestFirst, $userId, $search = []) { $params = [$userId]; $params = array_merge($params, $this->buildLikeParameters($search)); $params[] = $id; - $sql = 'AND `feeds`.`folder_id` = ? '; + $sql = $this->buildStatusQueryPart($showAll); + $sql .= $this->buildSearchQueryPart($search); + + $sql .= 'AND `feeds`.`folder_id` = ? '; if ($offset !== 0) { $sql .= 'AND `items`.`id` ' . $this->getOperator($oldestFirst) . ' ? '; $params[] = $offset; } - $sql = $this->makeSelectQueryStatus($sql, $status, $oldestFirst, - $search); + $sql = $this->makeSelectQuery($sql, $oldestFirst, $search); return $this->findEntitiesIgnoringNegativeLimit($sql, $params, $limit); } - public function findAll($limit, $offset, $status, $oldestFirst, $userId, + public function findAll($limit, $offset, $type, $showAll, $oldestFirst, $userId, $search = []) { $params = [$userId]; $params = array_merge($params, $this->buildLikeParameters($search)); - $sql = ''; + $sql = $this->buildStatusQueryPart($showAll, $type); + $sql .= $this->buildSearchQueryPart($search); + if ($offset !== 0) { $sql .= 'AND `items`.`id` ' . $this->getOperator($oldestFirst) . ' ? '; $params[] = $offset; } - $sql = $this->makeSelectQueryStatus($sql, $status, $oldestFirst, - $search); + + $sql = $this->makeSelectQuery($sql, $oldestFirst); return $this->findEntitiesIgnoringNegativeLimit($sql, $params, $limit); } public function findAllUnreadOrStarred($userId) { - $params = [$userId]; - $status = StatusFlag::UNREAD | StatusFlag::STARRED; - $sql = 'AND ((`items`.`status` & ' . $status . ') > 0) '; + $params = [$userId, true, true]; + $sql = 'AND (`items`.`unread` = ? OR `items`.`starred` = ?) '; $sql = $this->makeSelectQuery($sql); return $this->findEntities($sql, $params); } @@ -272,15 +293,15 @@ class ItemMapper extends NewsMapper { * @param int $threshold the number of items that should be deleted */ public function deleteReadOlderThanThreshold($threshold) { - $status = StatusFlag::STARRED | StatusFlag::UNREAD; - $params = [$status, $threshold]; + $params = [false, false, $threshold]; $sql = 'SELECT (COUNT(*) - `feeds`.`articles_per_update`) AS `size`, ' . '`feeds`.`id` AS `feed_id`, `feeds`.`articles_per_update` ' . 'FROM `*PREFIX*news_items` `items` ' . 'JOIN `*PREFIX*news_feeds` `feeds` ' . 'ON `feeds`.`id` = `items`.`feed_id` ' . - 'AND NOT ((`items`.`status` & ?) > 0) ' . + 'AND `items`.`unread` = ? ' . + 'AND `items`.`starred` = ? ' . 'GROUP BY `feeds`.`id`, `feeds`.`articles_per_update` ' . 'HAVING COUNT(*) > ?'; @@ -292,12 +313,13 @@ class ItemMapper extends NewsMapper { $limit = $size - $threshold; if ($limit > 0) { - $params = [$status, $row['feed_id'], $limit]; + $params = [false, false, $row['feed_id'], $limit]; $sql = 'DELETE FROM `*PREFIX*news_items` ' . 'WHERE `id` IN (' . 'SELECT `id` FROM `*PREFIX*news_items` ' . - 'WHERE NOT ((`status` & ?) > 0) ' . + 'WHERE `unread` = ? ' . + 'AND `starred` = ? ' . 'AND `feed_id` = ? ' . 'ORDER BY `id` ASC ' . 'LIMIT ?' . @@ -383,19 +405,18 @@ class ItemMapper extends NewsMapper { // as unread if ($isRead) { $sql = 'UPDATE `*PREFIX*news_items` - SET `status` = `status` & ?, + SET `unread` = ?, `last_modified` = ? WHERE `fingerprint` = ? AND `feed_id` IN ( SELECT `f`.`id` FROM `*PREFIX*news_feeds` AS `f` WHERE `f`.`user_id` = ? )'; - $params = [~StatusFlag::UNREAD, $lastModified, - $item->getFingerprint(), $userId]; + $params = [false, $lastModified, $item->getFingerprint(), $userId]; $this->execute($sql, $params); } else { $item->setLastModified($lastModified); - $item->setUnread(); + $item->setUnread(true); $this->update($item); } } diff --git a/lib/Db/Mysql/ItemMapper.php b/lib/Db/Mysql/ItemMapper.php index 8053c3d87..ad373eafb 100644 --- a/lib/Db/Mysql/ItemMapper.php +++ b/lib/Db/Mysql/ItemMapper.php @@ -16,9 +16,6 @@ namespace OCA\News\Db\Mysql; use OCA\News\Utility\Time; use OCP\IDBConnection; -use OCA\News\Db\StatusFlag; - - class ItemMapper extends \OCA\News\Db\ItemMapper { public function __construct(IDBConnection $db, Time $time){ @@ -29,19 +26,19 @@ class ItemMapper extends \OCA\News\Db\ItemMapper { /** * Delete all items for feeds that have over $threshold unread and not * starred items - * @param int $threshold the number of items that should be deleted + * @param int $threshold the number of items that should be deleted */ public function deleteReadOlderThanThreshold($threshold){ - $status = StatusFlag::STARRED | StatusFlag::UNREAD; $sql = 'SELECT (COUNT(*) - `feeds`.`articles_per_update`) AS `size`, ' . '`feeds`.`id` AS `feed_id`, `feeds`.`articles_per_update` ' . 'FROM `*PREFIX*news_items` `items` ' . 'JOIN `*PREFIX*news_feeds` `feeds` ' . 'ON `feeds`.`id` = `items`.`feed_id` ' . - 'AND NOT ((`items`.`status` & ?) > 0) ' . + 'AND `items`.`unread` = ? ' . + 'AND `items`.`starred` = ? ' . 'GROUP BY `feeds`.`id`, `feeds`.`articles_per_update` ' . 'HAVING COUNT(*) > ?'; - $params = [$status, $threshold]; + $params = [false, false, $threshold]; $result = $this->execute($sql, $params); while($row = $result->fetch()) { @@ -50,10 +47,11 @@ class ItemMapper extends \OCA\News\Db\ItemMapper { $limit = $size - $threshold; if($limit > 0) { - $params = [$status, $row['feed_id'], $limit]; + $params = [false, false, $row['feed_id'], $limit]; $sql = 'DELETE FROM `*PREFIX*news_items` ' . - 'WHERE NOT ((`status` & ?) > 0) ' . + 'WHERE `unread` = ? ' . + 'AND `starred` = ? ' . 'AND `feed_id` = ? ' . 'ORDER BY `id` ASC ' . 'LIMIT ?'; @@ -71,16 +69,15 @@ class ItemMapper extends \OCA\News\Db\ItemMapper { $sql = 'UPDATE `*PREFIX*news_items` `items` JOIN `*PREFIX*news_feeds` `feeds` ON `feeds`.`id` = `items`.`feed_id` - SET `items`.`status` = `items`.`status` & ?, + SET `items`.`unread` = ?, `items`.`last_modified` = ? WHERE `items`.`fingerprint` = ? AND `feeds`.`user_id` = ?'; - $params = [~StatusFlag::UNREAD, $lastModified, - $item->getFingerprint(), $userId]; + $params = [false, $lastModified, $item->getFingerprint(), $userId]; $this->execute($sql, $params); } else { $item->setLastModified($lastModified); - $item->setUnread(); + $item->setUnread(true); $this->update($item); } } diff --git a/lib/Db/StatusFlag.php b/lib/Db/StatusFlag.php deleted file mode 100644 index 314a81d01..000000000 --- a/lib/Db/StatusFlag.php +++ /dev/null @@ -1,47 +0,0 @@ -<?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 Alessandro Cosentino 2012 |