Optimize tag permissions (#126)

The new implementation generates a subquery of IDs instead of sending big arrays of data to/from the database. This massively speeds up performance.

extensions/tags/.github/workflows/test.yml

@@ -0,0 +1,78 @@
+name: Tests
+
+on: [push, pull_request]
+
+jobs:
+  test:
+    runs-on: ubuntu-latest
+
+    strategy:
+      matrix:
+        php: [7.3, 7.4, '8.0']
+        service: ['mysql:5.7', mariadb]
+        prefix: ['', flarum_]
+
+        include:
+          - service: 'mysql:5.7'
+            db: MySQL
+          - service: mariadb
+            db: MariaDB
+          - prefix: flarum_
+            prefixStr: (prefix)
+
+        exclude:
+          - php: 7.3
+            service: 'mysql:5.7'
+            prefix: flarum_
+          - php: 7.3
+            service: mariadb
+            prefix: flarum_
+          - php: 8.0
+            service: 'mysql:5.7'
+            prefix: flarum_
+          - php: 8.0
+            service: mariadb
+            prefix: flarum_
+
+    services:
+      mysql:
+        image: ${{ matrix.service }}
+        ports:
+          - 13306:3306
+
+    name: 'PHP ${{ matrix.php }} / ${{ matrix.db }} ${{ matrix.prefixStr }}'
+
+    steps:
+      - uses: actions/checkout@master
+
+      - name: Setup PHP
+        uses: shivammathur/setup-php@v2
+        with:
+          php-version: ${{ matrix.php }}
+          coverage: xdebug
+          extensions: curl, dom, gd, json, mbstring, openssl, pdo_mysql, tokenizer, zip
+          tools: phpunit, composer:v2
+
+      # The authentication alter is necessary because newer mysql versions use the `caching_sha2_password` driver,
+      # which isn't supported prior to PHP7.4
+      # When we drop support for PHP7.3, we should remove this from the setup.
+      - name: Create MySQL Database
+        run: |
+          sudo systemctl start mysql
+          mysql -uroot -proot -e 'CREATE DATABASE flarum_test;' --port 13306
+          mysql -uroot -proot -e "ALTER USER 'root'@'localhost' IDENTIFIED WITH mysql_native_password BY 'root';" --port 13306
+
+      - name: Install Composer dependencies
+        run: composer install
+
+      - name: Setup Composer tests
+        run: composer test:setup
+        env:
+          DB_PORT: 13306
+          DB_PASSWORD: root
+          DB_PREFIX: ${{ matrix.prefix }}
+
+      - name: Run Composer tests
+        run: composer test
+        env:
+          COMPOSER_PROCESS_TIMEOUT: 600

extensions/tags/composer.json

@@ -2,7 +2,9 @@
     "name": "flarum/tags",
     "description": "Organize discussions into a hierarchy of tags and categories.",
     "type": "flarum-extension",
-    "keywords": ["discussion"],
+    "keywords": [
+        "discussion"
+    ],
     "license": "MIT",
     "support": {
         "issues": "https://github.com/flarum/core/issues",
@@ -24,6 +26,11 @@
             "Flarum\\Tags\\": "src/"
         }
     },
+    "autoload-dev": {
+        "psr-4": {
+            "Flarum\\Tags\\Tests\\": "tests/"
+        }
+    },
     "extra": {
         "branch-alias": {
             "dev-master": "0.1.x-dev"
@@ -37,5 +44,24 @@
                 "color": "#fff"
             }
         }
+    },
+    "scripts": {
+        "test": [
+            "@test:unit",
+            "@test:integration"
+        ],
+        "test:unit": "phpunit -c tests/phpunit.unit.xml",
+        "test:integration": "phpunit -c tests/phpunit.integration.xml",
+        "test:setup": "@php tests/integration/setup.php"
+    },
+    "scripts-descriptions": {
+        "test": "Runs all tests.",
+        "test:unit": "Runs all unit tests.",
+        "test:integration": "Runs all integration tests.",
+        "test:setup": "Sets up a database for use with integration tests. Execute this only once."
+    },
+    "require-dev": {
+        "flarum/core": "0.1.x-dev",
+        "flarum/testing": "*@dev"
     }
 }

extensions/tags/src/Access/GlobalPolicy.php

@@ -37,9 +37,8 @@ public function can(User $actor, string $ability)
             if ($actor->hasPermission($ability) && $actor->hasPermission('bypassTagCounts')) {
                 return $this->allow();
             }
-
-            $enoughPrimary = count(Tag::getIdsWhereCan($actor, $ability, true, false)) >= $this->settings->get('flarum-tags.min_primary_tags');
-            $enoughSecondary = count(Tag::getIdsWhereCan($actor, $ability, false, true)) >= $this->settings->get('flarum-tags.min_secondary_tags');
+            $enoughPrimary = Tag::queryIdsWhereCan(Tag::query()->getQuery(), $actor, $ability, true, false)->count() >= $this->settings->get('flarum-tags.min_primary_tags');
+            $enoughSecondary = Tag::queryIdsWhereCan(Tag::query()->getQuery(), $actor, $ability, false, true)->count() >= $this->settings->get('flarum-tags.min_secondary_tags');
 
             if ($enoughPrimary && $enoughSecondary) {
                 return $this->allow();

extensions/tags/src/Access/ScopeDiscussionVisibility.php

@@ -27,7 +27,9 @@ public function __invoke(User $actor, Builder $query)
                 ->whereNotIn('discussions.id', function ($query) use ($actor) {
                     return $query->select('discussion_id')
                         ->from('discussion_tag')
-                        ->whereIn('tag_id', Tag::getIdsWhereCannot($actor, 'viewDiscussions'));
+                        ->whereNotIn('tag_id', function ($query) use ($actor) {
+                            Tag::queryIdsWhereCan($query->from('tags'), $actor, 'viewDiscussions');
+                        });
                 })
                 ->orWhere(function ($query) use ($actor) {
                     $query->whereVisibleTo($actor, 'viewDiscussionsInRestrictedTags');

extensions/tags/src/Access/ScopeDiscussionVisibilityForAbility.php

@@ -32,7 +32,9 @@ public function __invoke(User $actor, Builder $query, $ability)
         $query->whereIn('discussions.id', function ($query) use ($actor, $ability) {
             return $query->select('discussion_id')
                 ->from('discussion_tag')
-                ->whereIn('tag_id', Tag::getIdsWhereCan($actor, 'discussion.'.$ability));
+                ->whereIn('tag_id', function ($query) use ($actor, $ability) {
+                    Tag::queryIdsWhereCan($query->from('tags'), $actor, 'discussion.'.$ability);
+                });
         });
     }
 }

extensions/tags/src/Access/ScopeFlagVisibility.php

@@ -28,7 +28,9 @@ public function __invoke(User $actor, Builder $query)
             ->whereNotExists(function ($query) use ($actor) {
                 return $query->selectRaw('1')
                     ->from('discussion_tag')
-                    ->whereIn('tag_id', Tag::getIdsWhereCannot($actor, 'discussion.viewFlags'))
+                    ->whereNotIn('tag_id', function ($query) use ($actor) {
+                        Tag::queryIdsWhereCan($query->from('tags'), $actor, 'discussion.viewFlags');
+                    })
                     ->whereColumn('discussions.id', 'discussion_id');
             });
     }

extensions/tags/src/Access/ScopeTagVisibility.php

@@ -21,6 +21,8 @@ class ScopeTagVisibility
      */
     public function __invoke(User $actor, Builder $query)
     {
-        $query->whereNotIn('id', Tag::getIdsWhereCannot($actor, 'viewDiscussions'));
+        $query->whereIn('id', function ($query) use ($actor) {
+            Tag::queryIdsWhereCan($query, $actor, 'viewDiscussions');
+        });
     }
 }

extensions/tags/src/Tag.php

@@ -15,6 +15,7 @@
 use Flarum\Group\Permission;
 use Flarum\User\User;
 use Illuminate\Database\Eloquent\Builder;
+use Illuminate\Database\Query\Builder as QueryBuilder;
 
 /**
  * @property int $id
@@ -190,45 +191,70 @@ public function deletePermissions()
         Permission::where('permission', 'like', "tag{$this->id}.%")->delete();
     }
 
-    protected static function getIdsWherePermission(User $user, string $permission, bool $condition = true, bool $includePrimary = true, bool $includeSecondary = true): array
+    protected static function buildPermissionSubquery($base, $isAdmin, $hasGlobalPermission, $tagIdsWithPermission)
     {
-        static $tags;
-
-        if (! $tags) {
-            $tags = static::with('parent')->get();
+        $base
+            ->from('tags as perm_tags')
+            ->select('perm_tags.id');
+
+        // This needs to be a special case, as `tagIdsWithPermissions`
+        // won't include admin perms (which are all perms by default).
+        if ($isAdmin) {
+            return;
         }
 
-        $ids = [];
-        $hasGlobalPermission = $user->hasPermission($permission);
-
-        $canForTag = function (self $tag) use ($user, $permission, $hasGlobalPermission) {
-            return ($hasGlobalPermission && ! $tag->is_restricted) || ($tag->is_restricted && $user->hasPermission('tag'.$tag->id.'.'.$permission));
-        };
-
-        foreach ($tags as $tag) {
-            $can = $canForTag($tag);
-
-            if ($can && $tag->parent) {
-                $can = $canForTag($tag->parent);
-            }
-
-            $isPrimary = $tag->position !== null && ! $tag->parent;
+        $base->where(function ($query) use ($tagIdsWithPermission) {
+            $query
+                ->where('perm_tags.is_restricted', true)
+                ->whereIn('perm_tags.id', $tagIdsWithPermission);
+        });
 
-            if ($can === $condition && ($includePrimary && $isPrimary || $includeSecondary && ! $isPrimary)) {
-                $ids[] = $tag->id;
-            }
+        if ($hasGlobalPermission) {
+            $base->orWhere('perm_tags.is_restricted', false);
         }
-
-        return $ids;
-    }
-
-    public static function getIdsWhereCan(User $user, string $permission, bool $includePrimary = true, bool $includeSecondary = true): array
-    {
-        return static::getIdsWherePermission($user, $permission, true, $includePrimary, $includeSecondary);
     }
 
-    public static function getIdsWhereCannot(User $user, string $permission, bool $includePrimary = true, bool $includeSecondary = true): array
+    public static function queryIdsWhereCan(QueryBuilder $base, User $user, string $currPermission, bool $includePrimary = true, bool $includeSecondary = true): QueryBuilder
     {
-        return static::getIdsWherePermission($user, $permission, false, $includePrimary, $includeSecondary);
+        $hasGlobalPermission = $user->hasPermission($currPermission);
+        $isAdmin = $user->isAdmin();
+        $allPermissions = $user->getPermissions();
+
+        $tagIdsWithPermission = collect($allPermissions)
+            ->filter(function ($permission) use ($currPermission) {
+                return substr($permission, 0, 3) === 'tag' && strpos($permission, $currPermission) !== false;
+            })
+            ->map(function ($permission) {
+                $scopeFragment = explode('.', $permission, 2)[0];
+
+                return substr($scopeFragment, 3);
+            })
+            ->values();
+
+        $validTags = $base
+            ->from('tags as s_tags')
+            ->where(function ($query) use ($isAdmin, $hasGlobalPermission, $tagIdsWithPermission) {
+                $query
+                    ->whereIn('s_tags.id', function ($query) use ($isAdmin, $hasGlobalPermission, $tagIdsWithPermission) {
+                        static::buildPermissionSubquery($query, $isAdmin, $hasGlobalPermission, $tagIdsWithPermission);
+                    })
+                    ->where(function ($query) use ($isAdmin, $hasGlobalPermission, $tagIdsWithPermission) {
+                        $query
+                            ->whereIn('s_tags.parent_id', function ($query) use ($isAdmin, $hasGlobalPermission, $tagIdsWithPermission) {
+                                static::buildPermissionSubquery($query, $isAdmin, $hasGlobalPermission, $tagIdsWithPermission);
+                            })
+                            ->orWhere('s_tags.parent_id', null);
+                    });
+            })
+            ->where(function ($query) use ($includePrimary, $includeSecondary) {
+                if (! $includePrimary) {
+                    $query->where('s_tags.position', '=', null);
+                }
+                if (! $includeSecondary) {
+                    $query->where('s_tags.position', '!=', null);
+                }
+            });
+
+        return $validTags->select('s_tags.id');
     }
 }

extensions/tags/tests/integration/RetrievesRepresentativeTags.php

@@ -0,0 +1,30 @@
+<?php
+
+/*
+ * This file is part of Flarum.
+ *
+ * For detailed copyright and license information, please view the
+ * LICENSE file that was distributed with this source code.
+ */
+
+namespace Flarum\Tags\Tests\integration;
+
+trait RetrievesRepresentativeTags
+{
+    protected function tags()
+    {
+        return [
+            ['id' => 1, 'name' => 'Primary 1', 'slug' => 'primary-1', 'position' => 0, 'parent_id' => null],
+            ['id' => 2, 'name' => 'Primary 2', 'slug' => 'primary-2', 'position' => 1, 'parent_id' => null],
+            ['id' => 3, 'name' => 'Primary 2 Child 1', 'slug' => 'primary-2-child-1', 'position' => 2, 'parent_id' => 2],
+            ['id' => 4, 'name' => 'Primary 2 Child 2', 'slug' => 'primary-2-child-2', 'position' => 3, 'parent_id' => 2],
+            ['id' => 5, 'name' => 'Primary 2 Child Restricted', 'slug' => 'primary-2-child-restricted', 'position' => 4, 'parent_id' => 2, 'is_restricted' => true],
+            ['id' => 6, 'name' => 'Primary Restricted', 'slug' => 'primary-restricted', 'position' => 5, 'parent_id' => null, 'is_restricted' => true],
+            ['id' => 7, 'name' => 'Primary Restricted Child 1', 'slug' => 'primary-restricted-child-1', 'position' => 6, 'parent_id' => 6],
+            ['id' => 8, 'name' => 'Primary Restricted Child Restricted', 'slug' => 'primary-restricted-child-restricted', 'position' => 7, 'parent_id' => 6, 'is_restricted' => true],
+            ['id' => 9, 'name' => 'Secondary 1', 'slug' => 'secondary-1', 'position' => null, 'parent_id' => null],
+            ['id' => 10, 'name' => 'Secondary 2', 'slug' => 'secondary-2', 'position' => null, 'parent_id' => null],
+            ['id' => 11, 'name' => 'Secondary Restricted', 'slug' => 'secondary-restricted', 'position' => null, 'parent_id' => null, 'is_restricted' => true],
+        ];
+    }
+}