Mappers: Implement item purging

[SMillerDev]

Implement missing Mapper feature

[anoymouserver]

Furthermore we need to respect the oc_news_feeds.articles_per_update count to avoid purging more items which would reappear with the next fetch and then be unread and, since they have a higher ID, be at the top of the list.

This would additionally output the article count:

SELECT items.feed_id, COUNT(items.id) itemCount, feeds.articles_per_update
FROM `oc_news_items` items
INNER JOIN `oc_news_feeds` feeds ON feeds.id = items.feed_id
GROUP BY items.feed_id;

otherwise it could be also calculated in the query (not useful, because it's needed again while deleting)

SELECT items.feed_id, (COUNT(items.id) - feeds.articles_per_update) itemCount
/* same as above .. */

[anoymouserver]

A possible minimal solution for the article count to keep it simple:

// first query from previous comment ..
$feedQb->addSelect('items.feed_id', $feedQb->func()->count('items.id', 'itemCount'), 'feeds.articles_per_update')
       ->from($this->tableName, 'items')
       ->innerJoin('items', FeedMapperV2::TABLE_NAME, 'feeds', 'items.feed_id = feeds.id')
       ->groupBy('items.feed_id');


// .. and then use this later on instead of just $threshold
$maxItems = max($threshold, $feed['articles_per_update'])

[anoymouserver]

Looks good so far, but I would test it before approving it.
With the ignored unread I'm still not satisfied, but there I adapt to the majority.

[SMillerDev]

I was running the cleanup command locally and it works. CI will be fixed by #1072

[SMillerDev]

With the ignored unread I'm still not satisfied, but there I adapt to the majority.

If you can make an issue for it I'd be fine with making this optional in the next release.

[Grotax]

I feel a bit uncomfortable with a release of this, it would be an unexpected change when we roll this out and suddenly also unread items get deleted.
While I agree that this a feature admins requested a couple of times, I think this should be optional.

What I could image is that we force the config value to 0/-1 on upgrade to disable the autopurge.
Then the admin could check if he wants use this and announce it to the users.

[anoymouserver]

I've just tested the latest commit with NC 20.0.6 (PHP 7.3.25, MariaDB 10.1.47):

# should do something since the [<purge_count>] is optional, but doesn't
$ occ news:updater:after-update

# does something but crashes :(
$ occ news:updater:after-update 100

In AbstractMySQLDriver.php line 106:

  An exception occurred while executing 'DELETE FROM `oc_news_items` WHERE `id` IN (SELECT `id` FROM `oc_news_items` WHERE (feed_id = ?) AND (starred = 0) AND (unread = 0) LIMIT 18446744073709551615 OFFSET 100)' with params ["1"]:

  SQLSTATE[42000]: Syntax error or access violation: 1235 This version of MariaDB doesn't yet support 'LIMIT & IN/ALL/ANY/SOME subquery'

In PDOStatement.php line 129:

  SQLSTATE[42000]: Syntax error or access violation: 1235 This version of MariaDB doesn't yet support 'LIMIT & IN/ALL/ANY/SOME subquery'

In PDOStatement.php line 127:

  SQLSTATE[42000]: Syntax error or access violation: 1235 This version of MariaDB doesn't yet support 'LIMIT & IN/ALL/ANY/SOME subquery'

news:updater:after-update [<purge_count>]

What type (and version) of database do you use?

[SMillerDev]

should do something since the [<purge_count>] is optional, but doesn't

It does something for me, try running it with --verbose to see the result.

What type (and version) of database do you use?

I tested it successfully with a local SQLite DB. Maybe you can find some ways to get around this issue?

[anoymouserver]

With --verbose it returns "No cleanup needed", so apparently I don't have more items in the debug database than the default value (200).

It seems like MySQL and MariaDB doesn't support LIMIT in subqueries.

• https://mariadb.com/kb/en/subquery-limitations/
• https://dev.mysql.com/doc/refman/5.7/en/subquery-restrictions.html

[SMillerDev]

Okay, moved the query. It's much messier now, but at least it will work on MySQL

[anoymouserver]

$ occ news:updater:after-update 150 --verbose
An unhandled exception has been thrown:
Error: Call to undefined method OC\DB\Connection::executeStatement() in /var/www/cloud/apps/news/lib/Db/ItemMapperV2.php:192
Stack trace:
#0 /var/www/cloud/apps/news/lib/Service/ItemServiceV2.php(135): OCA\News\Db\ItemMapperV2->deleteOverThreshold(150, false)
#1 /var/www/cloud/apps/news/lib/Command/Updater/AfterUpdate.php(55): OCA\News\Service\ItemServiceV2->purgeOverThreshold(150, false)
#2 /var/www/cloud/apps/news/vendor/symfony/console/Command/Command.php(255): OCA\News\Command\Updater\AfterUpdate->execute(Object(Symfony\Component\Console\Input\ArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
#3 /var/www/cloud/apps/news/vendor/symfony/console/Application.php(1009): Symfony\Component\Console\Command\Command->run(Object(Symfony\Component\Console\Input\ArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
#4 /var/www/cloud/apps/news/vendor/symfony/console/Application.php(273): Symfony\Component\Console\Application->doRunCommand(Object(OCA\News\Command\Updater\AfterUpdate), Object(Symfony\Component\Console\Input\ArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
#5 /var/www/cloud/apps/news/vendor/symfony/console/Application.php(149): Symfony\Component\Console\Application->doRun(Object(Symfony\Component\Console\Input\ArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
#6 /var/www/cloud/lib/private/Console/Application.php(215): Symfony\Component\Console\Application->run(Object(Symfony\Component\Console\Input\ArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
#7 /var/www/cloud/console.php(100): OC\Console\Application->run()
#8 /var/www/cloud/occ(11): require_once('/var/www/cloud/...')
#9 {main}

executeStatement only exists as of v21.0.0, previously executeUpdate should be used. (IDBConnection.php#L112-L125)
(this also causes the test failure)

---

After changing this locally I've got

[Doctrine\DBAL\Exception\InvalidFieldNameException]
An exception occurred while executing 'DELETE FROM `oc_news_items` WHERE `id` IN (`113036`, `113224`, `113250`, ...)':

SQLSTATE[42S22]: Column not found: 1054 Unknown column '113036' in 'where clause'

.. apparently you have to use a named (or postional) parameter for the array otherwise it assumes that the array items are column names.

$deleteQb->delete($this->tableName)
         ->where('id IN (?)');

return $this->db->executeUpdate($deleteQb->getSQL(), [$total_items], [IQueryBuilder::PARAM_INT_ARRAY]);

⬆️ seems to work: Removed 2431 item(s)

[anoymouserver]

I've noticed that not the oldest ones are removed but the most recent unread and unstared ones.
After marking the latest one as read and running after-update again, the top one is missing even though there are multiple ones which are over a year old.

-> seems to be solved by adding ->orderBy('id', 'DESC') to the $rangeQuery

[anoymouserver]

All of the mentioned (and tested) changes together:

diff --git a/lib/Db/ItemMapperV2.php b/lib/Db/ItemMapperV2.php
index d888e9403..28cf748c8 100644
--- a/lib/Db/ItemMapperV2.php
+++ b/lib/Db/ItemMapperV2.php
@@ -165,7 +165,8 @@ class ItemMapperV2 extends NewsMapperV2
         $rangeQuery->select('id')
             ->from($this->tableName)
             ->where('feed_id = :feedId')
-            ->andWhere('starred = 0');
+            ->andWhere('starred = 0')
+            ->orderBy('id', 'DESC');
 
         if ($removeUnread === false) {
             $rangeQuery->andWhere('unread = 0');
@@ -187,9 +188,9 @@ class ItemMapperV2 extends NewsMapperV2
 
         $deleteQb = $this->db->getQueryBuilder();
         $deleteQb->delete($this->tableName)
-                 ->where($deleteQb->expr()->in('id', $total_items));
+                 ->where('id IN (?)');
 
-        return $this->db->executeStatement($deleteQb->getSQL());
+        return $this->db->executeUpdate($deleteQb->getSQL(), [$total_items], [IQueryBuilder::PARAM_INT_ARRAY]);
     }
 
     /**

[codecov-io]

Codecov Report

Merging #1065 (81488d9) into master (f9fde62) will decrease coverage by 3.22%.
The diff coverage is 77.88%.

@@             Coverage Diff              @@
##             master    #1065      +/-   ##
============================================
- Coverage     72.07%   68.84%   -3.23%     
- Complexity      681      734      +53     
============================================
  Files            58       73      +15     
  Lines          2417     2690     +273     
============================================
+ Hits           1742     1852     +110     
- Misses          675      838     +163     

Impacted Files	Coverage Δ	Complexity Δ
lib/Command/Updater/AllFeeds.php	0.00% <0.00%> (ø)	4.00 <1.00> (ø)
lib/Config/FetcherConfig.php	0.00% <0.00%> (ø)	6.00 <6.00> (-6.00)
lib/Config/LegacyConfig.php	0.00% <0.00%> (ø)	7.00 <7.00> (?)
lib/Controller/Exceptions/NotLoggedInException.php	0.00% <0.00%> (ø)	1.00 <1.00> (?)
lib/Controller/FeedController.php	99.05% <ø> (-0.95%)	32.00 <0.00> (+8.00)	⬇️
lib/Controller/FolderApiController.php	91.89% <ø> (-8.11%)	15.00 <0.00> (+3.00)	⬇️
lib/Controller/FolderController.php	88.23% <ø> (-11.77%)	20.00 <0.00> (+4.00)	⬇️
lib/Controller/ItemApiController.php	98.38% <ø> (-0.21%)	23.00 <0.00> (ø)
lib/Controller/ItemController.php	100.00% <ø> (ø)	17.00 <0.00> (ø)
lib/Controller/PageController.php	97.14% <ø> (+0.04%)	10.00 <0.00> (ø)
... and 124 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cb10dd1...81488d9. Read the comment docs.