From 9da4a3260d0291d5a4df4aeef020dd1938d2d1ba Mon Sep 17 00:00:00 2001 From: Jake Barnby Date: Wed, 13 May 2026 01:13:37 +1200 Subject: [PATCH] (refactor): improve advisor module perf, security, and maintainability MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Fix N+1 in Reports/XList (51→4 queries) via skipFilters + batch fetch - Add skipFilters to Reports/Delete and cursor fetch (avoid loading all nested insights/CTAs just for ownership check) - Fix N+1 in deleteReport worker (flat CTA deletion instead of per-insight) - Add advisor entity cleanup on project deletion (reports, insights, CTAs) - Remove resourceInternalId, parentResourceInternalId, $permissions from Insight response model (internal IDs leak DB internals, permissions unused) - Remove dead subQueryInsightCTAs filter registration - Remove stale enum-value comments from platform schema - Fix _key_dismissedAt index to include projectInternalId - Fix scope category from 'Other' to 'Advisor' - Switch action base class from Utopia\Platform\Action to Appwrite\Platform\Action Co-Authored-By: Claude Opus 4.6 --- app/config/collections/platform.php | 11 ++-- app/config/scopes/project.php | 8 +-- app/init/database/filters.php | 15 ------ .../Modules/Advisor/Http/Insights/Get.php | 2 +- .../Modules/Advisor/Http/Insights/XList.php | 2 +- .../Modules/Advisor/Http/Reports/Delete.php | 7 ++- .../Modules/Advisor/Http/Reports/Get.php | 2 +- .../Modules/Advisor/Http/Reports/XList.php | 50 ++++++++++++++++-- src/Appwrite/Platform/Workers/Deletes.php | 52 ++++++++++++++++--- .../Utopia/Response/Model/Insight.php | 19 ------- 10 files changed, 108 insertions(+), 60 deletions(-) diff --git a/app/config/collections/platform.php b/app/config/collections/platform.php index c0211bd7be..651eb7ebf5 100644 --- a/app/config/collections/platform.php +++ b/app/config/collections/platform.php @@ -2007,7 +2007,6 @@ $platformCollections = [ 'filters' => [], ], [ - // Analyzer that produced the report. Possible values: lighthouse, audit, databaseAnalyzer '$id' => ID::custom('type'), 'type' => Database::VAR_STRING, 'format' => '', @@ -2189,7 +2188,6 @@ $platformCollections = [ 'filters' => [], ], [ - // Possible values: databaseIndex, databasePerformance, sitePerformance, siteAccessibility, siteSeo, functionPerformance '$id' => ID::custom('type'), 'type' => Database::VAR_STRING, 'format' => '', @@ -2201,7 +2199,6 @@ $platformCollections = [ 'filters' => [], ], [ - // Possible values: info, warning, critical '$id' => ID::custom('severity'), 'type' => Database::VAR_STRING, 'format' => '', @@ -2213,7 +2210,6 @@ $platformCollections = [ 'filters' => [], ], [ - // Possible values: active, dismissed '$id' => ID::custom('status'), 'type' => Database::VAR_STRING, 'format' => '', @@ -2225,7 +2221,6 @@ $platformCollections = [ 'filters' => [], ], [ - // Possible values: databases, collections, sites, functions '$id' => ID::custom('resourceType'), 'type' => Database::VAR_STRING, 'format' => '', @@ -2417,11 +2412,11 @@ $platformCollections = [ 'orders' => [], ], [ - '$id' => ID::custom('_key_dismissedAt'), + '$id' => ID::custom('_key_project_dismissedAt'), 'type' => Database::INDEX_KEY, - 'attributes' => ['dismissedAt'], + 'attributes' => ['projectInternalId', 'dismissedAt'], 'lengths' => [], - 'orders' => [Database::ORDER_DESC], + 'orders' => [Database::ORDER_ASC, Database::ORDER_DESC], ], ], ], diff --git a/app/config/scopes/project.php b/app/config/scopes/project.php index e55ad370cd..3d8998fb2f 100644 --- a/app/config/scopes/project.php +++ b/app/config/scopes/project.php @@ -365,18 +365,18 @@ return [ // Advisor 'insights.read' => [ 'description' => 'Access to read insights under Advisor service.', - 'category' => 'Other', + 'category' => 'Advisor', ], 'insights.write' => [ 'description' => 'Reserved for Advisor insight ingestion outside CE.', - 'category' => 'Other', + 'category' => 'Advisor', ], 'reports.read' => [ 'description' => 'Access to read reports under Advisor service.', - 'category' => 'Other', + 'category' => 'Advisor', ], 'reports.write' => [ 'description' => 'Access to delete reports under Advisor service.', - 'category' => 'Other', + 'category' => 'Advisor', ], ]; diff --git a/app/init/database/filters.php b/app/init/database/filters.php index c1d79014bb..8a8c3666d1 100644 --- a/app/init/database/filters.php +++ b/app/init/database/filters.php @@ -476,21 +476,6 @@ Database::addFilter( } ); -Database::addFilter( - 'subQueryInsightCTAs', - function (mixed $value) { - return; - }, - function (mixed $value, Document $document, Database $database) { - return $database->getAuthorization()->skip(fn () => $database - ->find('insightCTAs', [ - Query::equal('projectInternalId', [$document->getAttribute('projectInternalId')]), - Query::equal('insightInternalId', [$document->getSequence()]), - Query::limit(APP_LIMIT_SUBQUERY), - ])); - } -); - Database::addFilter( 'subQueryReportInsights', function (mixed $value) { diff --git a/src/Appwrite/Platform/Modules/Advisor/Http/Insights/Get.php b/src/Appwrite/Platform/Modules/Advisor/Http/Insights/Get.php index 2e38060cab..b98f7b9e31 100644 --- a/src/Appwrite/Platform/Modules/Advisor/Http/Insights/Get.php +++ b/src/Appwrite/Platform/Modules/Advisor/Http/Insights/Get.php @@ -3,6 +3,7 @@ namespace Appwrite\Platform\Modules\Advisor\Http\Insights; use Appwrite\Extend\Exception; +use Appwrite\Platform\Action; use Appwrite\SDK\AuthType; use Appwrite\SDK\Method; use Appwrite\SDK\Response as SDKResponse; @@ -11,7 +12,6 @@ use Utopia\Database\Database; use Utopia\Database\Document; use Utopia\Database\Query; use Utopia\Database\Validator\UID; -use Utopia\Platform\Action; use Utopia\Platform\Scope\HTTP; class Get extends Action diff --git a/src/Appwrite/Platform/Modules/Advisor/Http/Insights/XList.php b/src/Appwrite/Platform/Modules/Advisor/Http/Insights/XList.php index f5d4f1e0af..51c1517135 100644 --- a/src/Appwrite/Platform/Modules/Advisor/Http/Insights/XList.php +++ b/src/Appwrite/Platform/Modules/Advisor/Http/Insights/XList.php @@ -3,6 +3,7 @@ namespace Appwrite\Platform\Modules\Advisor\Http\Insights; use Appwrite\Extend\Exception; +use Appwrite\Platform\Action; use Appwrite\SDK\AuthType; use Appwrite\SDK\Method; use Appwrite\SDK\Response as SDKResponse; @@ -15,7 +16,6 @@ use Utopia\Database\Exception\Query as QueryException; use Utopia\Database\Query; use Utopia\Database\Validator\Query\Cursor; use Utopia\Database\Validator\UID; -use Utopia\Platform\Action; use Utopia\Platform\Scope\HTTP; use Utopia\Validator\Boolean; diff --git a/src/Appwrite/Platform/Modules/Advisor/Http/Reports/Delete.php b/src/Appwrite/Platform/Modules/Advisor/Http/Reports/Delete.php index b562ea701e..6b1dfba31b 100644 --- a/src/Appwrite/Platform/Modules/Advisor/Http/Reports/Delete.php +++ b/src/Appwrite/Platform/Modules/Advisor/Http/Reports/Delete.php @@ -5,6 +5,7 @@ namespace Appwrite\Platform\Modules\Advisor\Http\Reports; use Appwrite\Event\Delete as DeleteEvent; use Appwrite\Event\Event; use Appwrite\Extend\Exception; +use Appwrite\Platform\Action; use Appwrite\SDK\AuthType; use Appwrite\SDK\ContentType; use Appwrite\SDK\Method; @@ -13,7 +14,6 @@ use Appwrite\Utopia\Response; use Utopia\Database\Database; use Utopia\Database\Document; use Utopia\Database\Validator\UID; -use Utopia\Platform\Action; use Utopia\Platform\Scope\HTTP; class Delete extends Action @@ -71,7 +71,10 @@ class Delete extends Action DeleteEvent $queueForDeletes, Event $queueForEvents ): void { - $report = $dbForPlatform->getDocument('reports', $reportId); + $report = $dbForPlatform->skipFilters( + fn () => $dbForPlatform->getDocument('reports', $reportId), + ['subQueryReportInsights'], + ); if ($report->isEmpty() || $report->getAttribute('projectInternalId') !== $project->getSequence()) { throw new Exception(Exception::REPORT_NOT_FOUND); diff --git a/src/Appwrite/Platform/Modules/Advisor/Http/Reports/Get.php b/src/Appwrite/Platform/Modules/Advisor/Http/Reports/Get.php index 2d1c124deb..ea5b017ad3 100644 --- a/src/Appwrite/Platform/Modules/Advisor/Http/Reports/Get.php +++ b/src/Appwrite/Platform/Modules/Advisor/Http/Reports/Get.php @@ -3,6 +3,7 @@ namespace Appwrite\Platform\Modules\Advisor\Http\Reports; use Appwrite\Extend\Exception; +use Appwrite\Platform\Action; use Appwrite\SDK\AuthType; use Appwrite\SDK\Method; use Appwrite\SDK\Response as SDKResponse; @@ -10,7 +11,6 @@ use Appwrite\Utopia\Response; use Utopia\Database\Database; use Utopia\Database\Document; use Utopia\Database\Validator\UID; -use Utopia\Platform\Action; use Utopia\Platform\Scope\HTTP; class Get extends Action diff --git a/src/Appwrite/Platform/Modules/Advisor/Http/Reports/XList.php b/src/Appwrite/Platform/Modules/Advisor/Http/Reports/XList.php index 0b154470b3..1c843f186b 100644 --- a/src/Appwrite/Platform/Modules/Advisor/Http/Reports/XList.php +++ b/src/Appwrite/Platform/Modules/Advisor/Http/Reports/XList.php @@ -3,6 +3,7 @@ namespace Appwrite\Platform\Modules\Advisor\Http\Reports; use Appwrite\Extend\Exception; +use Appwrite\Platform\Action; use Appwrite\SDK\AuthType; use Appwrite\SDK\Method; use Appwrite\SDK\Response as SDKResponse; @@ -14,7 +15,6 @@ use Utopia\Database\Exception\Order as OrderException; use Utopia\Database\Exception\Query as QueryException; use Utopia\Database\Query; use Utopia\Database\Validator\Query\Cursor; -use Utopia\Platform\Action; use Utopia\Platform\Scope\HTTP; use Utopia\Validator\Boolean; @@ -82,7 +82,10 @@ class XList extends Action } $reportId = $cursor->getValue(); - $cursorDocument = $dbForPlatform->getDocument('reports', $reportId); + $cursorDocument = $dbForPlatform->skipFilters( + fn () => $dbForPlatform->getDocument('reports', $reportId), + ['subQueryReportInsights'], + ); if ($cursorDocument->isEmpty() || $cursorDocument->getAttribute('projectInternalId') !== $project->getSequence()) { throw new Exception(Exception::GENERAL_CURSOR_NOT_FOUND, "Report '{$reportId}' for the 'cursor' value not found."); @@ -94,12 +97,53 @@ class XList extends Action $filterQueries = Query::groupByType($queries)['filters']; try { - $reports = $dbForPlatform->find('reports', $queries); + $reports = $dbForPlatform->skipFilters( + fn () => $dbForPlatform->find('reports', $queries), + ['subQueryReportInsights'], + ); $total = $includeTotal ? $dbForPlatform->count('reports', $filterQueries, APP_LIMIT_COUNT) : 0; } catch (OrderException $e) { throw new Exception(Exception::DATABASE_QUERY_ORDER_NULL, "The order attribute '{$e->getAttribute()}' had a null value. Cursor pagination requires all documents order attribute values are non-null."); } + if (!empty($reports)) { + $reportSequences = \array_map(fn (Document $r) => $r->getSequence(), $reports); + + $insights = $dbForPlatform->find('insights', [ + Query::equal('projectInternalId', [$project->getSequence()]), + Query::equal('reportInternalId', $reportSequences), + Query::limit(APP_LIMIT_SUBQUERY), + ]); + + if (!empty($insights)) { + $insightSequences = \array_map(fn (Document $i) => $i->getSequence(), $insights); + + $ctas = $dbForPlatform->find('insightCTAs', [ + Query::equal('projectInternalId', [$project->getSequence()]), + Query::equal('insightInternalId', $insightSequences), + Query::limit(APP_LIMIT_SUBQUERY), + ]); + + $ctasByInsight = []; + foreach ($ctas as $cta) { + $ctasByInsight[$cta->getAttribute('insightInternalId')][] = $cta; + } + + foreach ($insights as $insight) { + $insight->setAttribute('ctas', $ctasByInsight[$insight->getSequence()] ?? []); + } + } + + $insightsByReport = []; + foreach ($insights as $insight) { + $insightsByReport[$insight->getAttribute('reportInternalId')][] = $insight; + } + + foreach ($reports as $report) { + $report->setAttribute('insights', $insightsByReport[$report->getSequence()] ?? []); + } + } + $response->dynamic(new Document([ 'reports' => $reports, 'total' => $total, diff --git a/src/Appwrite/Platform/Workers/Deletes.php b/src/Appwrite/Platform/Workers/Deletes.php index 97b4497c1c..cf403371f8 100644 --- a/src/Appwrite/Platform/Workers/Deletes.php +++ b/src/Appwrite/Platform/Workers/Deletes.php @@ -229,15 +229,25 @@ class Deletes extends Action $projectInternalId = $project->getSequence(); $reportInternalId = $report->getSequence(); + $insights = $dbForPlatform->find('insights', [ + Query::equal('projectInternalId', [$projectInternalId]), + Query::equal('reportInternalId', [$reportInternalId]), + Query::limit(APP_LIMIT_SUBQUERY), + ]); + + if (!empty($insights)) { + $insightSequences = \array_map(fn (Document $i) => $i->getSequence(), $insights); + + $this->deleteByGroup('insightCTAs', [ + Query::equal('projectInternalId', [$projectInternalId]), + Query::equal('insightInternalId', $insightSequences), + ], $dbForPlatform); + } + $this->deleteByGroup('insights', [ Query::equal('projectInternalId', [$projectInternalId]), Query::equal('reportInternalId', [$reportInternalId]), - ], $dbForPlatform, function (Document $insight) use ($dbForPlatform, $projectInternalId) { - $this->deleteByGroup('insightCTAs', [ - Query::equal('projectInternalId', [$projectInternalId]), - Query::equal('insightInternalId', [$insight->getSequence()]), - ], $dbForPlatform); - }); + ], $dbForPlatform); } private function cleanDatabase( @@ -735,6 +745,36 @@ class Deletes extends Action Console::error('Failed to delete schedules: ' . $th->getMessage()); } + // Delete Advisor CTAs + try { + $this->deleteByGroup('insightCTAs', [ + Query::equal('projectInternalId', [$projectInternalId]), + Query::orderAsc() + ], $dbForPlatform); + } catch (Throwable $th) { + Console::error('Failed to delete insight CTAs: ' . $th->getMessage()); + } + + // Delete Advisor insights + try { + $this->deleteByGroup('insights', [ + Query::equal('projectInternalId', [$projectInternalId]), + Query::orderAsc() + ], $dbForPlatform); + } catch (Throwable $th) { + Console::error('Failed to delete insights: ' . $th->getMessage()); + } + + // Delete Advisor reports + try { + $this->deleteByGroup('reports', [ + Query::equal('projectInternalId', [$projectInternalId]), + Query::orderAsc() + ], $dbForPlatform); + } catch (Throwable $th) { + Console::error('Failed to delete reports: ' . $th->getMessage()); + } + /** * @var Database $dbForProject */ diff --git a/src/Appwrite/Utopia/Response/Model/Insight.php b/src/Appwrite/Utopia/Response/Model/Insight.php index 0eee80584a..39a12a9832 100644 --- a/src/Appwrite/Utopia/Response/Model/Insight.php +++ b/src/Appwrite/Utopia/Response/Model/Insight.php @@ -28,13 +28,6 @@ class Insight extends Model 'default' => '', 'example' => self::TYPE_DATETIME_EXAMPLE, ]) - ->addRule('$permissions', [ - 'type' => self::TYPE_STRING, - 'description' => 'Insight permissions. [Learn more about permissions](https://appwrite.io/docs/permissions).', - 'default' => [], - 'example' => ['read("any")'], - 'array' => true, - ]) ->addRule('reportId', [ 'type' => self::TYPE_STRING, 'description' => 'Parent report ID. Insights always belong to a report.', @@ -71,12 +64,6 @@ class Insight extends Model 'default' => '', 'example' => 'main', ]) - ->addRule('resourceInternalId', [ - 'type' => self::TYPE_STRING, - 'description' => 'Internal ID of the resource the insight is about.', - 'default' => '', - 'example' => '5e5ea5c16897e', - ]) ->addRule('parentResourceType', [ 'type' => self::TYPE_STRING, 'description' => 'Plural noun for the parent resource that contains the insight\'s resource, e.g. an insight about a column index on a table → resourceType=indexes, parentResourceType=tables. Empty when the resource has no parent.', @@ -89,12 +76,6 @@ class Insight extends Model 'default' => '', 'example' => 'orders', ]) - ->addRule('parentResourceInternalId', [ - 'type' => self::TYPE_STRING, - 'description' => 'Internal ID of the parent resource. Empty when the resource has no parent.', - 'default' => '', - 'example' => '5e5ea5c16897e', - ]) ->addRule('title', [ 'type' => self::TYPE_STRING, 'description' => 'Insight title.',