diff options
author | Alexandre Alapetite <alexandre@alapetite.fr> | 2024-07-21 14:54:34 +0200 |
---|---|---|
committer | GitHub <noreply@github.com> | 2024-07-21 14:54:34 +0200 |
commit | 0eeac4a669e46fb6521ae1765b23656c9afc43de (patch) | |
tree | 7c5a08b80b628f855b11c8850aa993c0b509feb1 | |
parent | 47b5e40e7240b5ccb0d0b6c54e0a958243689c83 (diff) | |
download | freshrss-0eeac4a669e46fb6521ae1765b23656c9afc43de.tar.gz freshrss-0eeac4a669e46fb6521ae1765b23656c9afc43de.zip |
Revisit keepMaxUnreads (#6632)
* Revisit keepMaxUnreads
Again, follow-up of https://github.com/FreshRSS/FreshRSS/pull/5905
fix https://github.com/FreshRSS/FreshRSS/issues/6620
* Refactoring to address buggy cases
* Fix minor test
-rw-r--r-- | app/Controllers/feedController.php | 166 | ||||
-rw-r--r-- | app/Models/EntryDAO.php | 43 | ||||
-rw-r--r-- | app/Models/Feed.php | 7 | ||||
-rw-r--r-- | app/Models/FeedDAO.php | 23 | ||||
-rwxr-xr-x | cli/actualize-user.php | 8 | ||||
-rw-r--r-- | p/api/greader.php | 5 | ||||
-rw-r--r-- | p/api/pshb.php | 7 |
7 files changed, 114 insertions, 145 deletions
diff --git a/app/Controllers/feedController.php b/app/Controllers/feedController.php index 2ecf6c374..ba5596a3c 100644 --- a/app/Controllers/feedController.php +++ b/app/Controllers/feedController.php @@ -110,11 +110,7 @@ class FreshRSS_feed_Controller extends FreshRSS_ActionController { $feed->_id($id); // Ok, feed has been added in database. Now we have to refresh entries. - [, , $nb_new_articles] = self::actualizeFeeds($id, $url); - if ($nb_new_articles > 0) { - self::commitNewEntries(); - } - + self::actualizeFeedsAndCommit($id, $url); return $feed; } @@ -385,7 +381,8 @@ class FreshRSS_feed_Controller extends FreshRSS_ActionController { } /** - * @return array{0:int,1:FreshRSS_Feed|null,2:int} Number of updated feeds, first feed or null, number of new articles + * @return array{0:int,1:FreshRSS_Feed|null,2:int,3:array<FreshRSS_Feed>} Number of updated feeds, first feed or null, number of new articles, + * list of feeds for which a cache refresh is needed * @throws FreshRSS_BadUrl_Exception */ public static function actualizeFeeds(?int $feed_id = null, ?string $feed_url = null, ?int $maxFeeds = null, ?SimplePie $simplePiePush = null): array { @@ -435,8 +432,9 @@ class FreshRSS_feed_Controller extends FreshRSS_ActionController { $pubsubhubbubEnabledGeneral = FreshRSS_Context::systemConf()->pubsubhubbub_enabled; $pshbMinAge = time() - (3600 * 24); //TODO: Make a configuration. - $updated_feeds = 0; - $nb_new_articles = 0; + $nbUpdatedFeeds = 0; + $nbNewArticles = 0; + $feedsCacheToRefresh = []; foreach ($feeds as $feed) { $feed = Minz_ExtensionManager::callHook('feed_before_actualize', $feed); @@ -598,9 +596,6 @@ class FreshRSS_feed_Controller extends FreshRSS_ActionController { // If the entry has changed, there is a good chance for the full content to have changed as well. $entry->loadCompleteContent(true); - if (!$entryDAO->inTransaction()) { - $entryDAO->beginTransaction(); - } $entryDAO->updateEntry($entry->toArray()); } } else { @@ -619,6 +614,8 @@ class FreshRSS_feed_Controller extends FreshRSS_ActionController { $titlesAsRead[$entry->title()] = true; } + $needFeedCacheRefresh = true; + if ($pubSubHubbubEnabled && !$simplePiePush) { //We use push, but have discovered an article by pull! $text = 'An article was discovered by pull although we use PubSubHubbub!: Feed ' . SimplePie_Misc::url_remove_credentials($url) . @@ -629,29 +626,20 @@ class FreshRSS_feed_Controller extends FreshRSS_ActionController { $feed->pubSubHubbubError(true); } - if (!$entryDAO->inTransaction()) { - $entryDAO->beginTransaction(); + if ($entryDAO->addEntry($entry->toArray(), true)) { + $nbNewArticles++; } - $entryDAO->addEntry($entry->toArray(), true); - - $nb_new_articles++; } } // N.B.: Applies to _entry table and not _entrytmp: $entryDAO->updateLastSeen($feed->id(), array_keys($newGuids), $mtime); } elseif ($feedIsUnchanged) { // Feed cache was unchanged, so mark as seen the same entries as last time - if (!$entryDAO->inTransaction()) { - $entryDAO->beginTransaction(); - } $entryDAO->updateLastSeenUnchanged($feed->id(), $mtime); } unset($entries); if (rand(0, 30) === 1) { // Remove old entries once in 30. - if (!$entryDAO->inTransaction()) { - $entryDAO->beginTransaction(); - } $nb = $feed->cleanOldEntries(); if ($nb > 0) { $needFeedCacheRefresh = true; @@ -659,20 +647,12 @@ class FreshRSS_feed_Controller extends FreshRSS_ActionController { } $feedDAO->updateLastUpdate($feed->id(), false, $mtime); - if ($feed->keepMaxUnread() !== null && ($feed->nbNotRead() + $nbMarkedUnread > $feed->keepMaxUnread())) { - Minz_Log::debug('Existing unread entries (' . ($feed->nbNotRead() + $nbMarkedUnread) . ') exceeding max number of ' . - $feed->keepMaxUnread() . ' for [' . $feed->url(false) . ']'); - $needFeedCacheRefresh |= ($feed->markAsReadMaxUnread() != false); - } if ($simplePiePush === null) { // Do not call for WebSub events, as we do not know the list of articles still on the upstream feed. $needFeedCacheRefresh |= ($feed->markAsReadUponGone($feedIsEmpty, $mtime) != false); } if ($needFeedCacheRefresh) { - $feedDAO->updateCachedValues($feed->id()); - } - if ($entryDAO->inTransaction()) { - $entryDAO->commit(); + $feedsCacheToRefresh[] = $feed; } $feedProperties = []; @@ -735,43 +715,37 @@ class FreshRSS_feed_Controller extends FreshRSS_ActionController { } } $feed->unlock(); - $updated_feeds++; + $nbUpdatedFeeds++; unset($feed); gc_collect_cycles(); - if ($updated_feeds >= $maxFeeds) { + if ($nbUpdatedFeeds >= $maxFeeds) { break; } } - return [$updated_feeds, reset($feeds) ?: null, $nb_new_articles]; + return [$nbUpdatedFeeds, reset($feeds) ?: null, $nbNewArticles, $feedsCacheToRefresh]; } /** - * @return int|false The number of articles marked as read, of false if error + * Feeds on which to apply a the keep max unreads policy, or all feeds if none specified. + * @return int The number of articles marked as read */ - private static function keepMaxUnreads() { + private static function keepMaxUnreads(FreshRSS_Feed ...$feeds): int { $affected = 0; - $entryDAO = FreshRSS_Factory::createEntryDao(); - $newUnreadEntriesPerFeed = $entryDAO->newUnreadEntriesPerFeed(); - $feedDAO = FreshRSS_Factory::createFeedDao(); - $feeds = $feedDAO->listFeedsOrderUpdate(-1); + + if (empty($feeds)) { + $feedDAO = FreshRSS_Factory::createFeedDao(); + $feeds = $feedDAO->listFeedsOrderUpdate(-1); + } + foreach ($feeds as $feed) { - if (!empty($newUnreadEntriesPerFeed[$feed->id()]) && $feed->keepMaxUnread() !== null && - ($feed->nbNotRead() + $newUnreadEntriesPerFeed[$feed->id()] > $feed->keepMaxUnread())) { - Minz_Log::debug('New unread entries (' . ($feed->nbNotRead() + $newUnreadEntriesPerFeed[$feed->id()]) . ') exceeding max number of ' . - $feed->keepMaxUnread() . ' for [' . $feed->url(false) . ']'); - $n = $feed->markAsReadMaxUnread(); - if ($n === false) { - $affected = false; - break; - } else { - $affected += $n; - } + $n = $feed->markAsReadMaxUnread(); + if ($n !== false && $n > 0) { + Minz_Log::debug($n . ' unread entries exceeding max number of ' . $feed->keepMaxUnread() . ' for [' . $feed->url(false) . ']'); + $affected += $n; } } - if ($feedDAO->updateCachedValues() === false) { - $affected = false; - } + return $affected; } @@ -807,23 +781,38 @@ class FreshRSS_feed_Controller extends FreshRSS_ActionController { return $tagDAO->tagEntries($applyLabels); } - public static function commitNewEntries(): void { + public static function commitNewEntries(): int { $entryDAO = FreshRSS_Factory::createEntryDao(); - ['all' => $nbNewEntries, 'unread' => $nbNewUnreadEntries] = $entryDAO->countNewEntries(); + $nbNewEntries = $entryDAO->countNewEntries(); if ($nbNewEntries > 0) { - if (!$entryDAO->inTransaction()) { - $entryDAO->beginTransaction(); - } if ($entryDAO->commitNewEntries()) { self::applyLabelActions($nbNewEntries); - if ($nbNewUnreadEntries > 0) { - self::keepMaxUnreads(); - } - } - if ($entryDAO->inTransaction()) { - $entryDAO->commit(); } } + return $nbNewEntries; + } + + /** + * @return array{0:int,1:FreshRSS_Feed|null,2:int,3:array<FreshRSS_Feed>} Number of updated feeds, first feed or null, number of new articles, + * list of feeds for which a cache refresh is needed + * @throws FreshRSS_BadUrl_Exception + */ + public static function actualizeFeedsAndCommit(?int $feed_id = null, ?string $feed_url = null, ?int $maxFeeds = null, ?SimplePie $simplePiePush = null): array { + $entryDAO = FreshRSS_Factory::createEntryDao(); + $entryDAO->beginTransaction(); + [$nbUpdatedFeeds, $feed, $nbNewArticles, $feedsCacheToRefresh] = FreshRSS_feed_Controller::actualizeFeeds($feed_id, $feed_url, $maxFeeds, $simplePiePush); + if ($nbNewArticles > 0) { + FreshRSS_feed_Controller::commitNewEntries(); + } + if (count($feedsCacheToRefresh) > 0) { + $feedDAO = FreshRSS_Factory::createFeedDao(); + self::keepMaxUnreads(...$feedsCacheToRefresh); + $feedDAO->updateCachedValues(...array_map(fn(FreshRSS_Feed $f) => $f->id(), $feedsCacheToRefresh)); + } + if ($entryDAO->inTransaction()) { + $entryDAO->commit(); + } + return [$nbUpdatedFeeds, $feed, $nbNewArticles, $feedsCacheToRefresh]; } /** @@ -844,9 +833,11 @@ class FreshRSS_feed_Controller extends FreshRSS_ActionController { $noCommit = ($_POST['noCommit'] ?? 0) == 1; if ($id === -1 && !$noCommit) { //Special request only to commit & refresh DB cache - $updated_feeds = 0; + $nbUpdatedFeeds = 0; $feed = null; - self::commitNewEntries(); + FreshRSS_feed_Controller::commitNewEntries(); + $feedDAO = FreshRSS_Factory::createFeedDao(); + $feedDAO->updateCachedValues(); } else { if ($id === 0 && $url === '') { // Case of a batch refresh (e.g. cron) @@ -855,11 +846,32 @@ class FreshRSS_feed_Controller extends FreshRSS_ActionController { Minz_ExtensionManager::callHookVoid('freshrss_user_maintenance'); FreshRSS_feed_Controller::commitNewEntries(); + $feedDAO = FreshRSS_Factory::createFeedDao(); + $feedDAO->updateCachedValues(); FreshRSS_category_Controller::refreshDynamicOpmls(); } - [$updated_feeds, $feed, $nbNewArticles] = self::actualizeFeeds($id, $url, $maxFeeds); - if (!$noCommit && $nbNewArticles > 0) { - FreshRSS_feed_Controller::commitNewEntries(); + $entryDAO = FreshRSS_Factory::createEntryDao(); + $entryDAO->beginTransaction(); + [$nbUpdatedFeeds, $feed, $nbNewArticles, $feedsCacheToRefresh] = self::actualizeFeeds($id, $url, $maxFeeds); + if (!$noCommit) { + if ($nbNewArticles > 0) { + FreshRSS_feed_Controller::commitNewEntries(); + } + $feedDAO = FreshRSS_Factory::createFeedDao(); + if ($id !== 0 && $id !== -1) { + if ($feed instanceof FreshRSS_Feed) { + self::keepMaxUnreads($feed); + } + // Case of single feed refreshed, always update its cache + $feedDAO->updateCachedValues($id); + } elseif (count($feedsCacheToRefresh) > 0) { + self::keepMaxUnreads(...$feedsCacheToRefresh); + // Case of multiple feeds refreshed, only update cache of affected feeds + $feedDAO->updateCachedValues(...array_map(fn(FreshRSS_Feed $f) => $f->id(), $feedsCacheToRefresh)); + } + } + if ($entryDAO->inTransaction()) { + $entryDAO->commit(); } } @@ -875,12 +887,12 @@ class FreshRSS_feed_Controller extends FreshRSS_ActionController { Minz_Request::good(_t('feedback.sub.feed.actualized', $feed->name()), [ 'params' => ['get' => 'f_' . $id] ]); - } elseif ($updated_feeds >= 1) { - Minz_Request::good(_t('feedback.sub.feed.n_actualized', $updated_feeds), []); + } elseif ($nbUpdatedFeeds >= 1) { + Minz_Request::good(_t('feedback.sub.feed.n_actualized', $nbUpdatedFeeds), []); } else { Minz_Request::good(_t('feedback.sub.feed.no_refresh'), []); } - return $updated_feeds; + return $nbUpdatedFeeds; } /** @@ -1047,8 +1059,6 @@ class FreshRSS_feed_Controller extends FreshRSS_ActionController { $limit = Minz_Request::paramInt('reload_limit') ?: 10; $feedDAO = FreshRSS_Factory::createFeedDao(); - $entryDAO = FreshRSS_Factory::createEntryDao(); - $feed = $feedDAO->searchById($feed_id); if ($feed === null) { Minz_Request::bad(_t('feedback.sub.feed.not_found'), []); @@ -1057,12 +1067,10 @@ class FreshRSS_feed_Controller extends FreshRSS_ActionController { //Re-fetch articles as if the feed was new. $feedDAO->updateFeed($feed->id(), [ 'lastUpdate' => 0 ]); - [, , $nb_new_articles] = self::actualizeFeeds($feed_id); - if ($nb_new_articles > 0) { - FreshRSS_feed_Controller::commitNewEntries(); - } + self::actualizeFeedsAndCommit($feed_id); //Extract all feed entries from database, load complete content and store them back in database. + $entryDAO = FreshRSS_Factory::createEntryDao(); $entries = $entryDAO->listWhere('f', $feed_id, FreshRSS_Entry::STATE_ALL, 'DESC', $limit); //We need another DB connection in parallel for unbuffered streaming diff --git a/app/Models/EntryDAO.php b/app/Models/EntryDAO.php index 0b289eb41..ec627dda1 100644 --- a/app/Models/EntryDAO.php +++ b/app/Models/EntryDAO.php @@ -274,45 +274,14 @@ SQL; } /** - * Count the number of new entries in the temporary table (which have not yet been committed), grouped by read / unread. - * @return array{'all':int,'unread':int,'read':int} + * Count the number of new entries in the temporary table (which have not yet been committed). */ - public function countNewEntries(): array { + public function countNewEntries(): int { $sql = <<<'SQL' - SELECT is_read, COUNT(id) AS nb_entries FROM `_entrytmp` - GROUP BY is_read + SELECT COUNT(id) AS nb_entries FROM `_entrytmp` SQL; - $lines = $this->fetchAssoc($sql) ?? []; - $nbRead = 0; - $nbUnread = 0; - foreach ($lines as $line) { - if (empty($line['is_read'])) { - $nbUnread = (int)($line['nb_entries'] ?? 0); - } else { - $nbRead = (int)($line['nb_entries'] ?? 0); - } - } - return ['all' => $nbRead + $nbUnread, 'unread' => $nbUnread, 'read' => $nbRead]; - } - - /** - * Count the number of new unread entries in the temporary table (which have not yet been committed), grouped by feed ID. - * @return array<int,int> - */ - public function newUnreadEntriesPerFeed(): array { - $sql = <<<'SQL' - SELECT id_feed, COUNT(id) AS nb_entries FROM `_entrytmp` - WHERE is_read = 0 - GROUP BY id_feed - SQL; - $lines = $this->fetchAssoc($sql) ?? []; - $result = []; - foreach ($lines as $line) { - if (!empty($line['id_feed'])) { - $result[(int)$line['id_feed']] = (int)($line['nb_entries'] ?? 0); - } - } - return $result; + $res = $this->fetchColumn($sql, 0); + return isset($res[0]) ? (int)$res[0] : -1; } /** @@ -651,7 +620,7 @@ SQL; } /** - * Remember to call updateCachedValue($id_feed) or updateCachedValues() just after. + * Remember to call updateCachedValues($id_feed) or updateCachedValues() just after. * @param array<string,bool|int|string> $options * @return int|false */ diff --git a/app/Models/Feed.php b/app/Models/Feed.php index 0274ded0a..e9d031a82 100644 --- a/app/Models/Feed.php +++ b/app/Models/Feed.php @@ -835,15 +835,12 @@ class FreshRSS_Feed extends Minz_Model { } $feedDAO = FreshRSS_Factory::createFeedDao(); $affected = $feedDAO->markAsReadMaxUnread($this->id(), $keepMaxUnread); - if ($affected > 0) { - Minz_Log::debug(__METHOD__ . " $affected items [" . $this->url(false) . ']'); - } return $affected; } /** * Applies the *mark as read upon gone* policy, if enabled. - * Remember to call `updateCachedValue($id_feed)` or `updateCachedValues()` just after. + * Remember to call `updateCachedValues($id_feed)` or `updateCachedValues()` just after. * @return int|false the number of lines affected, or false if not applicable */ public function markAsReadUponGone(bool $upstreamIsEmpty, int $maxTimestamp = 0) { @@ -871,7 +868,7 @@ class FreshRSS_Feed extends Minz_Model { } /** - * Remember to call `updateCachedValue($id_feed)` or `updateCachedValues()` just after + * Remember to call `updateCachedValues($id_feed)` or `updateCachedValues()` just after * @return int|false */ public function cleanOldEntries() { diff --git a/app/Models/FeedDAO.php b/app/Models/FeedDAO.php index c0b2e9e7a..4a75e3dea 100644 --- a/app/Models/FeedDAO.php +++ b/app/Models/FeedDAO.php @@ -203,7 +203,7 @@ class FreshRSS_FeedDAO extends Minz_ModelPdo { /** * @return int|false - * @see updateCachedValue() + * @see updateCachedValues() */ public function updateLastUpdate(int $id, bool $inError = false, int $mtime = 0) { $sql = 'UPDATE `_feed` SET `lastUpdate`=?, error=? WHERE id=?'; @@ -453,20 +453,21 @@ SQL; } /** + * Update cached values for selected feeds, or all feeds if no feed ID is provided. * @return int|false */ - public function updateCachedValues(int $id = 0) { + public function updateCachedValues(int ...$feedIds) { //2 sub-requests with FOREIGN KEY(e.id_feed), INDEX(e.is_read) faster than 1 request with GROUP BY or CASE - $sql = 'UPDATE `_feed` ' - . 'SET `cache_nbEntries`=(SELECT COUNT(e1.id) FROM `_entry` e1 WHERE e1.id_feed=`_feed`.id),' - . '`cache_nbUnreads`=(SELECT COUNT(e2.id) FROM `_entry` e2 WHERE e2.id_feed=`_feed`.id AND e2.is_read=0)' - . ($id != 0 ? ' WHERE id=:id' : ''); - $stm = $this->pdo->prepare($sql); - if ($stm !== false && $id != 0) { - $stm->bindParam(':id', $id, PDO::PARAM_INT); + $sql = <<<SQL +UPDATE `_feed` +SET `cache_nbEntries`=(SELECT COUNT(e1.id) FROM `_entry` e1 WHERE e1.id_feed=`_feed`.id), + `cache_nbUnreads`=(SELECT COUNT(e2.id) FROM `_entry` e2 WHERE e2.id_feed=`_feed`.id AND e2.is_read=0) +SQL; + if (count($feedIds) > 0) { + $sql .= ' WHERE id IN (' . str_repeat('?,', count($feedIds) - 1). '?)'; } - - if ($stm !== false && $stm->execute()) { + $stm = $this->pdo->prepare($sql); + if ($stm !== false && $stm->execute($feedIds)) { return $stm->rowCount(); } else { $info = $stm == null ? $this->pdo->errorInfo() : $stm->errorInfo(); diff --git a/cli/actualize-user.php b/cli/actualize-user.php index c07cf6d0e..29514769b 100755 --- a/cli/actualize-user.php +++ b/cli/actualize-user.php @@ -29,6 +29,9 @@ $databaseDAO->minorDbMaintenance(); Minz_ExtensionManager::callHookVoid('freshrss_user_maintenance'); FreshRSS_feed_Controller::commitNewEntries(); +$feedDAO = FreshRSS_Factory::createFeedDao(); +$feedDAO->updateCachedValues(); + $result = FreshRSS_category_Controller::refreshDynamicOpmls(); if (!empty($result['errors'])) { $errors = $result['errors']; @@ -39,10 +42,7 @@ if (!empty($result['successes'])) { echo "FreshRSS refreshed $successes dynamic OPMLs for $username\n"; } -[$nbUpdatedFeeds, , $nbNewArticles] = FreshRSS_feed_Controller::actualizeFeeds(); -if ($nbNewArticles > 0) { - FreshRSS_feed_Controller::commitNewEntries(); -} +[$nbUpdatedFeeds, , $nbNewArticles] = FreshRSS_feed_Controller::actualizeFeedsAndCommit(); echo "FreshRSS actualized $nbUpdatedFeeds feeds for $username ($nbNewArticles new articles)\n"; diff --git a/p/api/greader.php b/p/api/greader.php index 71cf40884..9c3479546 100644 --- a/p/api/greader.php +++ b/p/api/greader.php @@ -331,10 +331,7 @@ final class GReaderAPI { $importService = new FreshRSS_Import_Service($user); $importService->importOpml($opml); if ($importService->lastStatus()) { - [, , $nb_new_articles] = FreshRSS_feed_Controller::actualizeFeeds(); - if ($nb_new_articles > 0) { - FreshRSS_feed_Controller::commitNewEntries(); - } + FreshRSS_feed_Controller::actualizeFeedsAndCommit(); invalidateHttpCache($user); exit('OK'); } else { diff --git a/p/api/pshb.php b/p/api/pshb.php index 9f5b4822c..8fcc5ab34 100644 --- a/p/api/pshb.php +++ b/p/api/pshb.php @@ -133,11 +133,8 @@ foreach ($users as $userFilename) { Minz_ExtensionManager::enableByList(FreshRSS_Context::userConf()->extensions_enabled, 'user'); Minz_Translate::reset(FreshRSS_Context::userConf()->language); - [$updated_feeds, , $nb_new_articles] = FreshRSS_feed_Controller::actualizeFeeds(null, $self, null, $simplePie); - if ($nb_new_articles > 0) { - FreshRSS_feed_Controller::commitNewEntries(); - } - if ($updated_feeds > 0) { + [$nbUpdatedFeeds, ] = FreshRSS_feed_Controller::actualizeFeedsAndCommit(null, $self, null, $simplePie); + if ($nbUpdatedFeeds > 0) { $nb++; } else { Minz_Log::warning('Warning: User ' . $username . ' does not subscribe anymore to ' . $self, PSHB_LOG); |