Return removed articles correctly in Miniflux

CHANGELOG

@@ -2,6 +2,7 @@ Version 0.1?.? (2022-??-??)
 ===========================
 
 Bug fixes:
+- Return all removed articles when multiple statuses are requested in Miniflux
 - Allow multiple date ranges in search strings in Tiny Tiny RSS
 - Honour user time zone when interpreting search strings in Tiny Tiny RSS
 - Perform MySQL table maintenance more reliably

docs/en/030_Supported_Protocols/005_Miniflux.md

@@ -39,7 +39,6 @@ Miniflux version 2.0.28 is emulated, though not all features are implemented
 - Filtering rules may not function identically (see below for details)
 - The `checked_at` field of feeds indicates when the feed was last updated rather than when it was last checked
 - Creating a feed with the `scrape` property set to `true` might not return scraped content for the initial synchronization
-- Querying articles for both read/unread and removed statuses will not return all removed articles
 - Search strings will match partial words
 - OPML import either succeeds or fails atomically: if one feed fails, no feeds are imported
 

lib/Database.php

@@ -10,7 +10,10 @@ use JKingWeb\DrUUID\UUID;
 use JKingWeb\Arsse\Db\Statement;
 use JKingWeb\Arsse\Misc\Query;
 use JKingWeb\Arsse\Context\Context;
+use JKingWeb\Arsse\Context\UnionContext;
+use JKingWeb\Arsse\Context\RootContext;
 use JKingWeb\Arsse\Misc\Date;
+use JKingWeb\Arsse\Misc\QueryFilter;
 use JKingWeb\Arsse\Misc\ValueInfo as V;
 use JKingWeb\Arsse\Misc\URL;
 use JKingWeb\Arsse\Rule\Rule;
@@ -1518,33 +1521,11 @@ class Database {
      * If an empty column list is supplied, a count of articles matching the context is queried instead
      *
      * @param string $user The user whose articles are to be queried
-     * @param Context $context The search context
+     * @param RootContext $context The search context
      * @param array $cols The columns to request in the result set
      */
-    protected function articleQuery(string $user, Context $context, array $cols = ["id"]): Query {
+    protected function articleQuery(string $user, RootContext $context, array $cols = ["id"]): Query {
-        // validate input
+        // prepare the output column list; the column definitions are also used for ordering
-        if ($context->subscription()) {
-            $this->subscriptionValidateId($user, $context->subscription);
-        }
-        if ($context->folder()) {
-            $this->folderValidateId($user, $context->folder);
-        }
-        if ($context->folderShallow()) {
-            $this->folderValidateId($user, $context->folderShallow);
-        }
-        if ($context->edition()) {
-            $this->articleValidateEdition($user, $context->edition);
-        }
-        if ($context->article()) {
-            $this->articleValidateId($user, $context->article);
-        }
-        if ($context->label()) {
-            $this->labelValidateId($user, $context->label, false);
-        }
-        if ($context->labelName()) {
-            $this->labelValidateId($user, $context->labelName, true);
-        }
-        // prepare the output column list; the column definitions are also used later
         $colDefs = $this->articleColumns();
         if (!$cols) {
             // if no columns are specified return a count; don't borther with sorting
@@ -1602,6 +1583,67 @@ class Database {
             [$user, $user, $user, $user, $user, $user]
         );
         $q->setLimit($context->limit, $context->offset);
+        if ($context instanceof UnionContext) {
+            // if the context is a union context, we compute each context in turn
+            $q->setWhereRestrictive(false);
+            foreach ($context as $c) {
+                $q->setWhereGroup($this->articleFilter($c));
+            }
+        } else {
+            // if the context is not a union, first validate input to catch 404s and the like
+            if ($context->subscription()) {
+                $this->subscriptionValidateId($user, $context->subscription);
+            }
+            if ($context->folder()) {
+                $this->folderValidateId($user, $context->folder);
+            }
+            if ($context->folderShallow()) {
+                $this->folderValidateId($user, $context->folderShallow);
+            }
+            if ($context->edition()) {
+                $this->articleValidateEdition($user, $context->edition);
+            }
+            if ($context->article()) {
+                $this->articleValidateId($user, $context->article);
+            }
+            if ($context->label()) {
+                $this->labelValidateId($user, $context->label, false);
+            }
+            if ($context->labelName()) {
+                $this->labelValidateId($user, $context->labelName, true);
+            }
+            // ensure any used array-type context options contain at least one member
+            foreach ([
+                "articles", 
+                "editions",
+                "subscriptions",
+                "folders", 
+                "foldersShallow", 
+                "labels", 
+                "labelNames", 
+                "tags", 
+                "tagNames", 
+                "searchTerms", 
+                "titleTerms", 
+                "authorTerms", 
+                "annotationTerms",
+                "modifiedRanges",
+                "markedRanges",
+                ] as $m) {
+                    if ($context->$m() && !$context->$m) {
+                        throw new Db\ExceptionInput("tooShort", ['field' => $m, 'action' => $this->caller(), 'min' => 1]);
+                    }
+                }
+            // next compute the context, supplying the query to manipulate directly
+            $this->articleFilter($context, $q);
+        }
+        // return the query
+        return $q;
+    }
+
+    protected function articleFilter(Context $context, QueryFilter $q = null) {
+        $q = $q ?? new QueryFilter;
+        $colDefs = $this->articleColumns();
         // handle the simple context options
         $options = [
             // each context array consists of a column identifier (see $colDefs above), a comparison operator, and a data type; the "between" operator has special handling
@@ -1639,9 +1681,6 @@ class Database {
                     }
                 } elseif (is_array($context->$m)) {
                     // context option is an array of values
-                    if (!$context->$m) {
-                        throw new Db\ExceptionInput("tooShort", ['field' => $m, 'action' => $this->caller(), 'min' => 1]); // must have at least one array element
-                    }
                     [$clause, $types, $values] = $this->generateIn($context->$m, $type);
                     $q->setWhere("{$colDefs[$col]} $op ($clause)", $types, $values);
                 } else {
@@ -1691,9 +1730,6 @@ class Database {
         foreach ($options as $m => [$cte, $outerCol, $selection, $innerCol, $op, $type]) {
             if ($context->$m()) {
                 if ($op === "in") {
-                    if (!$context->$m) {
-                        throw new Db\ExceptionInput("tooShort", ['field' => $m, 'action' => $this->caller(), 'min' => 1]); // must have at least one array element
-                    }
                     [$inClause, $inTypes, $inValues] = $this->generateIn($context->$m, $type);
                     $q->setWhere("{$colDefs[$outerCol]} in (select $selection from $cte where $innerCol in($inClause))", $inTypes, $inValues);
                 } else {
@@ -1727,9 +1763,6 @@ class Database {
                 return $colDefs[$c];
             }, $columns);
             if ($context->$m()) {
-                if (!$context->$m) {
-                    throw new Db\ExceptionInput("tooShort", ['field' => $m, 'action' => $this->caller(), 'min' => 1]); // must have at least one array element
-                }
                 $q->setWhere(...$this->generateSearch($context->$m, $columns));
             }
             // handle the exclusionary version
@@ -1744,31 +1777,20 @@ class Database {
         ];
         foreach ($options as $m => [$col, $type]) {
             if ($context->$m()) {
-                if (!$context->$m) {
+                $subq = (new QueryFilter)->setWhereRestrictive(false);
-                    throw new Db\ExceptionInput("tooShort", ['field' => $m, 'action' => $this->caller(), 'min' => 1]); // must have at least one array element
-                }
-                $w = [];
-                $t = [];
-                $v = [];
                 foreach ($context->$m as $r) {
                     if ($r[0] === null) {
                         // range is open at the low end
-                        $w[] = "{$colDefs[$col]} <= ?";
+                        $subq->setWhere("{$colDefs[$col]} <= ?", $type, $r[1]);
-                        $t[] = $type;
-                        $v[] = $r[1];
                     } elseif ($r[1] === null) {
                         // range is open at the high end
-                        $w[] = "{$colDefs[$col]} >= ?";
+                        $subq->setWhere("{$colDefs[$col]} >= ?", $type, $r[0]);
-                        $t[] = $type;
-                        $v[] = $r[0];
                     } else {
                         // range is bounded in both directions
-                        $w[] = "{$colDefs[$col]} BETWEEN ? AND ?";
+                        $subq->setWhere("{$colDefs[$col]} BETWEEN ? AND ?", [$type, $type], $r);
-                        $t[] = [$type, $type];
-                        $v[] = $r;
                     }
                 }
-                $q->setWhere("(".implode(" OR ", $w).")", $t, $v);
+                $q->setWhereGroup($subq);
             }
             // handle the exclusionary version
             if ($context->not->$m() && $context->not->$m) {
@@ -1786,7 +1808,6 @@ class Database {
                 }
             }
         }
-        // return the query
         return $q;
     }
 
@@ -1795,11 +1816,11 @@ class Database {
      * If an empty column list is supplied, a count of articles is returned instead
      *
      * @param string $user The user whose articles are to be listed
-     * @param Context $context The search context
+     * @param RootContext $context The search context
      * @param array $fieldss The columns to return in the result set, any of: id, edition, url, title, author, content, guid, fingerprint, folder, subscription, feed, starred, unread, note, published_date, edited_date, modified_date, marked_date, subscription_title, media_url, media_type
      * @param array $sort The columns to sort the result by eg. "edition desc" in decreasing order of importance
      */
-    public function articleList(string $user, Context $context = null, array $fields = ["id"], array $sort = []): Db\Result {
+    public function articleList(string $user, RootContext $context = null, array $fields = ["id"], array $sort = []): Db\Result {
         // make a base query based on context and output columns
         $context = $context ?? new Context;
         $q = $this->articleQuery($user, $context, $fields);
@@ -1841,9 +1862,9 @@ class Database {
     /** Returns a count of articles which match the given query context
      *
      * @param string $user The user whose articles are to be counted
-     * @param Context $context The search context
+     * @param RootContext $context The search context
      */
-    public function articleCount(string $user, Context $context = null): int {
+    public function articleCount(string $user, RootContext $context = null): int {
         $context = $context ?? new Context;
         $q = $this->articleQuery($user, $context, []);
         return (int) $this->db->prepare($q->getQuery(), $q->getTypes())->run($q->getValues())->getValue();
@@ -1860,10 +1881,10 @@ class Database {
      *
      * @param string $user The user who owns the articles to be modified
      * @param array $data An associative array of properties to modify. Anything not specified will remain unchanged
-     * @param Context $context The query context to match articles against
+     * @param RootContext $context The query context to match articles against
      * @param bool $updateTimestamp Whether to also update the timestamp. This should only be false if a mark is changed as a result of an automated action not taken by the user
      */
-    public function articleMark(string $user, array $data, Context $context = null, bool $updateTimestamp = true): int {
+    public function articleMark(string $user, array $data, RootContext $context = null, bool $updateTimestamp = true): int {
         $data = [
             'read'    => $data['read'] ?? null,
             'starred' => $data['starred'] ?? null,
@@ -2147,7 +2168,7 @@ class Database {
     }
 
     /** Returns the numeric identifier of the most recent edition of an article matching the given context */
-    public function editionLatest(string $user, Context $context = null): int {
+    public function editionLatest(string $user, RootContext $context = null): int {
         $context = $context ?? new Context;
         $q = $this->articleQuery($user, $context, ["latest_edition"]);
         return (int) $this->db->prepare((string) $q, $q->getTypes())->run($q->getValues())->getValue();
@@ -2350,11 +2371,11 @@ class Database {
      *
      * @param string $user The owner of the label
      * @param integer|string $id The numeric identifier or name of the label
-     * @param Context $context The query context matching the desired articles
+     * @param RootContext $context The query context matching the desired articles
      * @param int $mode Whether to add (ASSOC_ADD), remove (ASSOC_REMOVE), or replace with (ASSOC_REPLACE) the matching associations
      * @param boolean $byName Whether to interpret the $id parameter as the label's name (true) or identifier (false)
      */
-    public function labelArticlesSet(string $user, $id, Context $context, int $mode = self::ASSOC_ADD, bool $byName = false): int {
+    public function labelArticlesSet(string $user, $id, RootContext $context, int $mode = self::ASSOC_ADD, bool $byName = false): int {
         assert(in_array($mode, [self::ASSOC_ADD, self::ASSOC_REMOVE, self::ASSOC_REPLACE]), new Exception("constantUnknown", $mode));
         // validate the tag ID, and get the numeric ID if matching by name
         $id = $this->labelValidateId($user, $id, $byName, true)['id'];

lib/REST/Miniflux/V1.php

@@ -12,6 +12,8 @@ use JKingWeb\Arsse\ExceptionType;
 use JKingWeb\Arsse\Feed\Exception as FeedException;
 use JKingWeb\Arsse\AbstractException;
 use JKingWeb\Arsse\Context\Context;
+use JKingWeb\Arsse\Context\UnionContext;
+use JKingWeb\Arsse\Context\RootContext;
 use JKingWeb\Arsse\Db\ExceptionInput;
 use JKingWeb\Arsse\ImportExport\OPML;
 use JKingWeb\Arsse\ImportExport\Exception as ImportException;
@@ -886,12 +888,11 @@ class V1 extends \JKingWeb\Arsse\REST\AbstractHandler {
         ]);
     }
 
-    protected function computeContext(array $query, Context $c = null): Context {
+    protected function computeContext(array $query, Context $c): RootContext {
         if ($query['before'] && $query['before']->getTimestamp() === 0) {
             $query['before'] = null; // NOTE: This workaround is needed for compatibility with "Microflux for Miniflux", an Android Client
         }
-        $c = ($c ?? new Context)
+        $c->limit($query['limit'] ?? self::DEFAULT_ENTRY_LIMIT) // NOTE: This does not honour user preferences
-            ->limit($query['limit'] ?? self::DEFAULT_ENTRY_LIMIT) // NOTE: This does not honour user preferences
             ->offset($query['offset'])
             ->starred($query['starred'])
             ->modifiedRange($query['after'], $query['before']) // FIXME: This may not be the correct date field
@@ -904,17 +905,20 @@ class V1 extends \JKingWeb\Arsse\REST\AbstractHandler {
                 $c->folder($query['category_id'] - 1);
             }
         }
-        // FIXME: specifying e.g. ?status=read&status=removed should yield all hidden articles and all read articles, but the best we can do is all read articles which are or are not hidden
         $status = array_unique($query['status']);
         sort($status);
         if ($status === ["read", "removed"]) {
-            $c->unread(false);
+            $c1 = $c;
+            $c2 = clone $c;
+            $c = new UnionContext($c1->unread(false), $c2->hidden(true));
         } elseif ($status === ["read", "unread"]) {
             $c->hidden(false);
         } elseif ($status === ["read"]) {
             $c->hidden(false)->unread(false);
         } elseif ($status === ["removed", "unread"]) {
-            $c->unread(true);
+            $c1 = $c;
+            $c2 = clone $c;
+            $c = new UnionContext($c1->unread(true), $c2->hidden(true));
         } elseif ($status === ["removed"]) {
             $c->hidden(true);
         } elseif ($status === ["unread"]) {

tests/cases/Database/SeriesArticle.php

@@ -9,6 +9,8 @@ namespace JKingWeb\Arsse\TestCase\Database;
 use JKingWeb\Arsse\Database;
 use JKingWeb\Arsse\Arsse;
 use JKingWeb\Arsse\Context\Context;
+use JKingWeb\Arsse\Context\UnionContext;
+use JKingWeb\Arsse\Context\RootContext;
 use JKingWeb\Arsse\Misc\Date;
 use JKingWeb\Arsse\Misc\ValueInfo;
 
@@ -423,7 +425,7 @@ trait SeriesArticle {
     }
 
     /** @dataProvider provideContextMatches */
-    public function testListArticlesCheckingContext(Context $c, array $exp): void {
+    public function testListArticlesCheckingContext(RootContext $c, array $exp): void {
         $ids = array_column($ids = Arsse::$db->articleList("john.doe@example.com", $c, ["id"], ["id"])->getAll(), "id");
         sort($ids);
         sort($exp);
@@ -538,6 +540,7 @@ trait SeriesArticle {
             'Not modified in 2010 or 2015'                               => [(new Context)->not->modifiedRanges([["2010-01-01T00:00:00Z", "2010-12-31T23:59:59Z"], ["2015-01-01T00:00:00Z", "2015-12-31T23:59:59Z"]]), [1,3,5,7,19]],
             'Modified prior to 2010 or since 2015'                       => [(new Context)->modifiedRanges([[null, "2009-12-31T23:59:59Z"], ["2015-01-01T00:00:00Z", null]]), [1,3,5,7,19]],
             'Not modified prior to 2010 or since 2015'                   => [(new Context)->not->modifiedRanges([[null, "2009-12-31T23:59:59Z"], ["2015-01-01T00:00:00Z", null]]), [2,4,6,8,20]],
+            'Either read or hidden'                                      => [(new UnionContext((new Context)->unread(false), (new Context)->hidden(true))), [1, 6, 19]],
         ];
     }
 

tests/cases/REST/Miniflux/TestV1.php

@@ -10,6 +10,8 @@ use Eloquent\Phony\Mock\Handle\InstanceHandle;
 use Eloquent\Phony\Phpunit\Phony;
 use JKingWeb\Arsse\Arsse;
 use JKingWeb\Arsse\Context\Context;
+use JKingWeb\Arsse\Context\RootContext;
+use JKingWeb\Arsse\Context\UnionContext;
 use JKingWeb\Arsse\User;
 use JKingWeb\Arsse\Database;
 use JKingWeb\Arsse\Db\Transaction;
@@ -711,7 +713,7 @@ class TestV1 extends \JKingWeb\Arsse\Test\AbstractTest {
     }
 
     /** @dataProvider provideEntryQueries */
-    public function testGetEntries(string $url, ?Context $c, ?array $order, $out, bool $count, ResponseInterface $exp): void {
+    public function testGetEntries(string $url, ?RootContext $c, ?array $order, $out, bool $count, ResponseInterface $exp): void {
         $this->dbMock->subscriptionList->returns(new Result($this->v(self::FEEDS)));
         $this->dbMock->articleCount->returns(2112);
         if ($out instanceof \Exception) {
@@ -760,9 +762,9 @@ class TestV1 extends \JKingWeb\Arsse\Test\AbstractTest {
             ["/entries?status=read",                               (clone $c)->unread(false)->hidden(false),                              $o,                        self::ENTRIES,                   false, new Response(['total' => sizeof(self::ENTRIES_OUT), 'entries' => self::ENTRIES_OUT])],
             ["/entries?status=removed",                            (clone $c)->hidden(true),                                              $o,                        self::ENTRIES,                   false, new Response(['total' => sizeof(self::ENTRIES_OUT), 'entries' => self::ENTRIES_OUT])],
             ["/entries?status=unread&status=read",                 (clone $c)->hidden(false),                                             $o,                        self::ENTRIES,                   false, new Response(['total' => sizeof(self::ENTRIES_OUT), 'entries' => self::ENTRIES_OUT])],
-            ["/entries?status=unread&status=removed",              (clone $c)->unread(true),                   $o,                        self::ENTRIES,                   false, new Response(['total' => sizeof(self::ENTRIES_OUT), 'entries' => self::ENTRIES_OUT])],
+            ["/entries?status=unread&status=removed",              new UnionContext((clone $c)->unread(true), (clone $c)->hidden(true)),  $o,                        self::ENTRIES,                   false, new Response(['total' => sizeof(self::ENTRIES_OUT), 'entries' => self::ENTRIES_OUT])],
-            ["/entries?status=removed&status=read",                (clone $c)->unread(false),                  $o,                        self::ENTRIES,                   false, new Response(['total' => sizeof(self::ENTRIES_OUT), 'entries' => self::ENTRIES_OUT])],
+            ["/entries?status=removed&status=read",                new UnionContext((clone $c)->unread(false), (clone $c)->hidden(true)), $o,                        self::ENTRIES,                   false, new Response(['total' => sizeof(self::ENTRIES_OUT), 'entries' => self::ENTRIES_OUT])],
-            ["/entries?status=removed&status=read&status=removed", (clone $c)->unread(false),                  $o,                        self::ENTRIES,                   false, new Response(['total' => sizeof(self::ENTRIES_OUT), 'entries' => self::ENTRIES_OUT])],
+            ["/entries?status=removed&status=read&status=removed", new UnionContext((clone $c)->unread(false), (clone $c)->hidden(true)), $o,                        self::ENTRIES,                   false, new Response(['total' => sizeof(self::ENTRIES_OUT), 'entries' => self::ENTRIES_OUT])],
             ["/entries?status=removed&status=read&status=unread",  $c,                                                                    $o,                        self::ENTRIES,                   false, new Response(['total' => sizeof(self::ENTRIES_OUT), 'entries' => self::ENTRIES_OUT])],
             ["/entries?starred",                                   (clone $c)->starred(true),                                             $o,                        self::ENTRIES,                   false, new Response(['total' => sizeof(self::ENTRIES_OUT), 'entries' => self::ENTRIES_OUT])],
             ["/entries?starred=",                                  (clone $c)->starred(true),                                             $o,                        self::ENTRIES,                   false, new Response(['total' => sizeof(self::ENTRIES_OUT), 'entries' => self::ENTRIES_OUT])],