Binary Flags instead of booleans make queries complex

[BernhardPosselt]

This is a technical debt issue from the very beginning of the News app that I was basically avoiding because migration is work intensive.

Instead of having boolean fields for stuff like unread and starred the news app uses binary flags:

class StatusFlag {
    const UNREAD    = 0x02;
    const STARRED   = 0x04;
    const DELETED   = 0x08;
    const UPDATED   = 0x16;
}

In binary this would look like:

0000

every 0 means false, every 1 means true.

0011

for instance means that the item is unread and starred.

Note: only unread and starred are actually used!

As you see this is basically doing binary math with php binary operators and not only leads to brainfuck but also to issues like having to string concatenate Sql statements because you can't do binary math with parameters:

    public function starredCount($userId) {
        $sql = 'SELECT COUNT(*) AS size FROM `*PREFIX*news_items` `items` ' .
            'JOIN `*PREFIX*news_feeds` `feeds` ' .
            'ON `feeds`.`id` = `items`.`feed_id` ' .
            'AND `feeds`.`deleted_at` = 0 ' .
            'AND `feeds`.`user_id` = ? ' .
            'AND ((`items`.`status` & ' . StatusFlag::STARRED . ') = ' .
            StatusFlag::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];

        $result = $this->execute($sql, $params)->fetch();

        return (int)$result['size'];
    }

This should be refactored into boolean columns instead (including a migration that migrates the previous status)

[davull]

It also makes query very slow.
Current query takes on my system with 100k rows in *_news_items 5sec.
Rewritten to something like 'status = 4' and an index on the status-column takes only a view ms.

MySql can't use index in combination with binary operations.

[cosenal]

I introduced that as a legacy from Akregator and I totally agree that we should work on removing it.

[sonologic]

Anyone willing to work on this?

[sonologic]

Btw, 0x16 is quite hilarious actually

[sonologic]

11:38 <@BernhardPosselt> gmc: regarding the binary flag PR
11:38 <@BernhardPosselt> the statusflag class can be killed
11:38 <@BernhardPosselt> all binary flags should be moved to bools in the sql queries
11:39 <@BernhardPosselt> as for migrating: just add 2 new columns and keep the old one
11:39 <@BernhardPosselt> we dont have a way to add migrations yet
11:39 <@BernhardPosselt> will apparently be featured in nextcloud 13
11:39 <@BernhardPosselt> https://github.com/nextcloud/news/blob/master/lib/Db/StatusFlag.php#L30
11:40 <@BernhardPosselt> what the method actually does is that it takes a type and showall flag
11:40 <@BernhardPosselt> the type is a type of feed
11:40 <@BernhardPosselt> e.g. FEED, FOLDER, etc
11:41 <@BernhardPosselt> in case of showAll you want to show all articles
11:41 <@BernhardPosselt> but that filter is not added for starred items
11:41 <@BernhardPosselt> if you display the starred items "feed" you will see read and unread items regardless of your setting
11:42 <@BernhardPosselt> at work right now btw
11:46 <@BernhardPosselt> you can probably replace it with
11:47 <@BernhardPosselt> $showUnread = $showUnread || $feedType === FeedType::STARRED
11:47 <@BernhardPosselt> ehm
11:47 <@BernhardPosselt> $showRead = $showRead || $feedType === FeedType::STARRED

[sonologic]

http://nextcloudappstore.readthedocs.io/en/latest/developer.html#info-xml under 'repair-steps' mentions how to migrate databases, but also mentions 'will not be used, only validated' for anything under repair-steps.. However, a quick grep through NC codebase suggests repair-steps are indeed executed.

[BernhardPosselt]

The will not be used comment basically means that the app store ignores it not nextcloud

[BernhardPosselt]

Fixed by @danopz