Feat authorization instance #740
Conversation
…atic calls. Update README to reflect changes in role delegation method.
…ation handling in Database class to use instance methods. Adjust tests to reflect changes in method signatures.
WalkthroughAuthorization was converted from a static/global API to an instance-scoped API. Authorization instances are created and propagated via Database/Adapter/Pool/Mirror, an Input descriptor class was added, and tests/examples/tasks were updated to use per-database authorization and instance methods (e.g., Changes
Sequence Diagram(s)sequenceDiagram
participant Test
participant Database
participant Adapter
participant Auth as Authorization (instance)
Note over Test,Auth: Previous (Static) flow
Test->>Auth: Authorization::setRole(role)
Note right of Auth: global roles/status
Adapter->>Auth: Authorization::getRoles()
Auth-->>Adapter: roles
Note over Database,Auth: New (Instance) flow
Test->>Database: new Database()
Database->>Auth: new Authorization()
Database->>Database: setAuthorization(auth)
Database->>Adapter: new Adapter(...)->setAuthorization(auth)
Test->>Database: getAuthorization()
Database-->>Test: auth
Test->>Auth: addRole(role)
Auth-->>Adapter: getRoles()
Adapter->>Database: use auth for permission checks (Input)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (2)📚 Learning: 2025-10-03T02:04:17.803ZApplied to files:
📚 Learning: 2025-10-20T05:29:29.487ZApplied to files:
🧬 Code graph analysis (1)src/Database/Database.php (5)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
tests/e2e/Adapter/Scopes/Relationships/OneToManyTests.php (1)
795-798: Bug: wrong variable in assertion ($account2vs$account1)You fetch
$account1then assert on$account2. Assert on$account1to test the nullified relation.Apply:
- $account1 = $database->getDocument('account', 'account1'); - $this->assertEquals(null, $account2->getAttribute('newCustomer', '')); + $account1 = $database->getDocument('account', 'account1'); + $this->assertEquals(null, $account1->getAttribute('newCustomer', ''));src/Database/Mirror.php (1)
9-10: Use instance Authorization; fix linter: remove unused imports + trim blank blank-line whitespaceGood switch to
$this->getSource()->getAuthorization()->skip(...). CI fails with PSR-12: unused imports and whitespace. Remove unusedAuthorization,DuplicateException, andLimitimports, and remove whitespace in the blank line before the return.Based on pipeline failures
Apply:
@@ -use Utopia\Database\Exception\Duplicate as DuplicateException; -use Utopia\Database\Exception\Limit; use Utopia\Database\Helpers\ID; use Utopia\Database\Mirroring\Filter; -use Utopia\Database\Validator\Authorization; @@ protected function getUpgradeStatus(string $collection): ?Document { if ($collection === 'upgrades' || $collection === Database::METADATA) { return new Document(); } - - return $this->getSource()->getAuthorization()->skip(function () use ($collection) { + + return $this->getSource()->getAuthorization()->skip(function () use ($collection) { try { return $this->source->getDocument('upgrades', $collection); } catch (\Throwable) { return; } }); }Also applies to: 1086-1094
tests/e2e/Adapter/Scopes/AttributeTests.php (1)
23-23: Fix linter: remove unused importThe Authorization validator import is unused; remove to satisfy PSR-12 no_unused_imports.
- use Utopia\Database\Validator\Authorization;tests/e2e/Adapter/Scopes/RelationshipTests.php (1)
1-22: Fix the unused import to resolve the linter error.The pipeline linter reports a 'no_unused_imports' violation. Since the code now accesses
Authorizationthrough$this->getDatabase()->getAuthorization()instead of using the class directly, theuse Utopia\Database\Validator\Authorization;import at line 21 is likely no longer needed and should be removed.Apply this diff to remove the unused import:
use Utopia\Database\Helpers\Role; use Utopia\Database\Query; -use Utopia\Database\Validator\Authorization;bin/tasks/query.php (1)
1-22: Fix import ordering to resolve the linter error.The pipeline linter reports an 'ordered_imports' rule violation. According to PSR-12, imports should be in alphabetical order. The
use Utopia\Database\Validator\Authorization;import at line 21 should come before theuse Utopia\Validator\Boolean;import.Apply this diff to fix the import order:
use Utopia\Database\Query; +use Utopia\Database\Validator\Authorization; use Utopia\Validator\Boolean; use Utopia\Validator\Integer; use Utopia\Validator\Text; -use Utopia\Database\Validator\Authorization;tests/e2e/Adapter/Scopes/DocumentTests.php (1)
21-21: Remove unused import to fix linter.
use Utopia\Database\Validator\Authorization;is unused. Drop it to satisfy PSR-12 no_unused_imports.- use Utopia\Database\Validator\Authorization;tests/e2e/Adapter/Scopes/GeneralTests.php (1)
22-22: Remove unused import to satisfy linter.
use Utopia\Database\Validator\Authorization;isn’t used. Remove it to fix PSR‑12 no_unused_imports.- use Utopia\Database\Validator\Authorization;
🧹 Nitpick comments (20)
bin/tasks/load.php (1)
29-30: Inject the Authorization instance into Database (local $authorization currently unused).You create and mutate $authorization but never set it on the Database; the DB uses its own Authorization instance. Inject it to align with the instance-based model.
Apply this diff at the two Database construction sites:
@@ - $createSchema( - (new Database(new ($cfg['adapter'])($pdo), $cache)) - ->setDatabase($name) - ->setNamespace($namespace) - ->setSharedTables($sharedTables) - ); + $createSchema( + (new Database(new ($cfg['adapter'])($pdo), $cache)) + ->setAuthorization($authorization) + ->setDatabase($name) + ->setNamespace($namespace) + ->setSharedTables($sharedTables) + ); @@ - $database = (new Database(new ($cfg['adapter'])($pdo), $cache)) - ->setDatabase($name) - ->setNamespace($namespace) - ->setSharedTables($sharedTables); + $database = (new Database(new ($cfg['adapter'])($pdo), $cache)) + ->setAuthorization($authorization) + ->setDatabase($name) + ->setNamespace($namespace) + ->setSharedTables($sharedTables);Also applies to: 42-46, 51-66, 125-130, 151-155
bin/tasks/relationships.php (1)
37-38: Propagate Authorization to Database (and into workers).$authorization is set and updated but never attached to Database; also not captured in go() closures. Inject it to enforce the intended roles.
@@ - $database = (new Database(new ($cfg['adapter'])($pdo), $cache)) + $database = (new Database(new ($cfg['adapter'])($pdo), $cache)) + ->setAuthorization($authorization) ->setDatabase($name) ->setNamespace($namespace) ->setSharedTables($sharedTables); @@ - for ($i = 0; $i < $limit / 1000; $i++) { - go(function () use ($cfg, $pool, $name, $namespace, $sharedTables, $cache, $globalDocs) { + for ($i = 0; $i < $limit / 1000; $i++) { + go(function () use ($cfg, $pool, $name, $namespace, $sharedTables, $cache, $globalDocs, $authorization) { try { $pdo = $pool->get(); - $database = (new Database(new ($cfg['adapter'])($pdo), $cache)) + $database = (new Database(new ($cfg['adapter'])($pdo), $cache)) + ->setAuthorization($authorization) ->setDatabase($name) ->setNamespace($namespace) ->setSharedTables($sharedTables);Also applies to: 47-48, 54-66, 169-175, 195-205
README.md (1)
29-29: Clarify how to add roles with the instance-based API.Readers need to know where $authorization comes from. Suggest referencing $database->getAuthorization() or showing setAuthorization usage.
-- **Permissions** - Using permissions, you can decide which roles have read, create, update and delete access for a specific document. The special attribute `$permissions` is used to store permission metadata for each document in the collection. A permission role can be any string you want. You can use `$authorization->addRole()` to delegate new roles to your users, once obtained a new role a user would gain read, create, update or delete access to a relevant document. +- **Permissions** - Using permissions, you can decide which roles have read, create, update and delete access for a specific document. The special attribute `$permissions` is used to store permission metadata for each document in the collection. A permission role can be any string you want. You can use `$database->getAuthorization()->addRole('role')` (or inject a custom Authorization via `$database->setAuthorization($authorization)`) to delegate new roles to your users.tests/e2e/Adapter/Scopes/Relationships/OneToManyTests.php (1)
1839-1844: Consistency: avoid mixing$databaseand$this->getDatabase()within the same testIn this method you create
$database = $this->getDatabase();but then call$this->getDatabase()->...repeatedly. Prefer one access style (use$database) for readability and to simplify future refactors.tests/e2e/Adapter/Scopes/SpatialTests.php (2)
1349-1350: Typo: test nametestSptialAggregationConsider renaming to
testSpatialAggregationfor clarity and grep-ability.Also applies to: 1479-1480, 1781-1782
95-96: NormalizeQuerycasing across test files (style/consistency)Lowercase
query::static calls found in SpatialTests.php and DocumentTests.php should use correct PascalCaseQuery::to comply with PSR naming conventions and improve static analysis resolution.Fix these 7 occurrences:
- SpatialTests.php:168, 169, 203, 212, 1124
- DocumentTests.php:290, 323
src/Database/Adapter/SQL.php (2)
2432-2436: Add fallback ORDER BY to avoid invalid SQL when no sort is providedIf $orders is empty, ORDER BY will be malformed. Default to primary key for stability.
- $sqlOrder = 'ORDER BY ' . implode(', ', $orders); + if (empty($orders)) { + $orders[] = $this->quote('_id') . ' ' . Database::ORDER_ASC; + } + $sqlOrder = 'ORDER BY ' . implode(', ', $orders);
585-606: Remove unused $removeBindKeys scratch array$removeBindKeys is appended to but never used; drop it to reduce noise.
- $bindKey = '_uid_' . $index; - $removeBindKeys[] = ':_uid_' . $index; - $removeBindValues[$bindKey] = $document->getId(); + $bindKey = '_uid_' . $index; + $removeBindValues[$bindKey] = $document->getId(); ... - $removeBindKeys[] = ':' . $bindKey; $removeBindValues[$bindKey] = $permissionsToRemove[$i];tests/e2e/Adapter/Scopes/RelationshipTests.php (1)
1748-1750: Consider caching the authorization reference for better readability.Multiple calls to
$this->getDatabase()->getAuthorization()in quick succession could be simplified by storing the reference in a local variable. While the performance impact is minimal in tests, it improves readability.Example refactor for this test method:
+ $authorization = $this->getDatabase()->getAuthorization(); + $authorization->cleanRoles(); + $authorization->addRole(Role::any()->toString()); - $this->getDatabase()->getAuthorization()->cleanRoles(); - $this->getDatabase()->getAuthorization()->addRole(Role::any()->toString());This pattern appears in multiple places throughout the file (e.g., lines 2249-2250, 1826, 1839).
tests/e2e/Adapter/SQLiteTest.php (1)
60-61: Align table name with SQLite naming (include tenant).SQLite tables are named with namespace_tenant_collection (per index naming). Include tenant to avoid mismatches when tenant is set.
- $sqlTable = "`" . $this->getDatabase()->getNamespace() . "_" . $collection . "`"; + $sqlTable = sprintf( + '`%s_%s_%s`', + $this->getDatabase()->getNamespace(), + $this->getDatabase()->getTenant(), + $collection + );src/Database/Database.php (1)
4909-4913: Address PSR‑12 whitespace warningsThere are trailing spaces/formatting nits (example vicinity). Please run your CS fixer (php‑cs‑fixer/PHPCS) to remove trailing whitespace and enforce single‑space around constructs.
tests/e2e/Adapter/SharedTables/PostgresTest.php (2)
32-65: Instance migration + auth injection look goodSwapping to an instance
getDatabase()and injecting authorization viasetAuthorization(self::$authorization)is aligned with Base and core changes. Minor nit: docblock above still says “@reture Adapter”.Also applies to: 51-55
77-86: Use adapter’s deleteIndex to avoid quoting/compat pitfallsLeverage the adapter API instead of constructing SQL manually; it already handles namespacing, tenant, and “not exists” cases consistently.
Apply this diff:
- protected function deleteIndex(string $collection, string $index): bool - { - $key = "\"".$this->getDatabase()->getNamespace()."_".$this->getDatabase()->getTenant()."_{$collection}_{$index}\""; - - $sql = "DROP INDEX \"".$this->getDatabase()->getDatabase()."\".{$key}"; - - self::$pdo->exec($sql); - - return true; - } + protected function deleteIndex(string $collection, string $index): bool + { + return $this->getDatabase()->getAdapter()->deleteIndex($collection, $index); + }tests/e2e/Adapter/MirrorTest.php (3)
96-101: Explicitly set authorization on both source and destinationMirror’s
setAuthorization()updates its (source) adapter; destination isn’t guaranteed unless set earlier. Make it explicit to avoid coupling to cleanup side-effects.$database ->setDatabase('utopiaTests') - ->setAuthorization(self::$authorization) ->setNamespace(static::$namespace = 'myapp_' . uniqid()); + +// Ensure both sides share the same Authorization explicitly +$database->getSource()->setAuthorization(self::$authorization); +$database->getDestination()->setAuthorization(self::$authorization); + +// Optionally also set on Mirror for symmetry (source will be covered anyway) +$database->setAuthorization(self::$authorization);
316-329: Avoid reliance on static state; use instance-derived source/destinationUse
$this->getDatabase()->getSource()/getDestination()to reduce ordering/initialization risks.- protected function deleteColumn(string $collection, string $column): bool - { - $sqlTable = "`" . self::$source->getDatabase() . "`.`" . self::$source->getNamespace() . "_" . $collection . "`"; + protected function deleteColumn(string $collection, string $column): bool + { + $db = $this->getDatabase(); + $source = $db->getSource(); + $destination = $db->getDestination(); + + $sqlTable = "`" . $source->getDatabase() . "`.`" . $source->getNamespace() . "_" . $collection . "`"; $sql = "ALTER TABLE {$sqlTable} DROP COLUMN `{$column}`"; - self::$sourcePdo->exec($sql); + self::$sourcePdo->exec($sql); - $sqlTable = "`" . self::$destination->getDatabase() . "`.`" . self::$destination->getNamespace() . "_" . $collection . "`"; + $sqlTable = "`" . $destination->getDatabase() . "`.`" . $destination->getNamespace() . "_" . $collection . "`"; $sql = "ALTER TABLE {$sqlTable} DROP COLUMN `{$column}`"; - self::$destinationPdo->exec($sql); + self::$destinationPdo->exec($sql); return true; }
331-344: Same here: prefer instance access and/or adapter API for index dropEither reuse instance-derived DBs or delegate to adapter
deleteIndexfor correctness and fewer SQL edge cases.- protected function deleteIndex(string $collection, string $index): bool - { - $sqlTable = "`" . self::$source->getDatabase() . "`.`" . self::$source->getNamespace() . "_" . $collection . "`"; - $sql = "DROP INDEX `{$index}` ON {$sqlTable}"; - - self::$sourcePdo->exec($sql); - - $sqlTable = "`" . self::$destination->getDatabase() . "`.`" . self::$destination->getNamespace() . "_" . $collection . "`"; - $sql = "DROP INDEX `{$index}` ON {$sqlTable}"; - - self::$destinationPdo->exec($sql); - - return true; - } + protected function deleteIndex(string $collection, string $index): bool + { + $db = $this->getDatabase(); + $ok1 = $db->getSource()->getAdapter()->deleteIndex($collection, $index); + $ok2 = $db->getDestination()->getAdapter()->deleteIndex($collection, $index); + return $ok1 && $ok2; + }tests/e2e/Adapter/SharedTables/SQLiteTest.php (1)
83-91: Delegate index drop to adapter to handle “no such index” gracefullySQLite adapter already swallows “no such index” and normalizes naming; use it directly.
- protected function deleteIndex(string $collection, string $index): bool - { - $index = "`".$this->getDatabase()->getNamespace()."_".$this->getDatabase()->getTenant()."_{$collection}_{$index}`"; - $sql = "DROP INDEX {$index}"; - - self::$pdo->exec($sql); - - return true; - } + protected function deleteIndex(string $collection, string $index): bool + { + return $this->getDatabase()->getAdapter()->deleteIndex($collection, $index); + }tests/e2e/Adapter/SharedTables/MySQLTest.php (1)
80-88: Prefer adapter deleteIndex over raw SQLLets the adapter own quoting and edge cases.
- protected function deleteIndex(string $collection, string $index): bool - { - $sqlTable = "`" . $this->getDatabase()->getDatabase() . "`.`" . $this->getDatabase()->getNamespace() . "_" . $collection . "`"; - $sql = "DROP INDEX `{$index}` ON {$sqlTable}"; - - self::$pdo->exec($sql); - - return true; - } + protected function deleteIndex(string $collection, string $index): bool + { + return $this->getDatabase()->getAdapter()->deleteIndex($collection, $index); + }tests/e2e/Adapter/PostgresTest.php (1)
21-21: LGTM on instance method; fix docblock typoGood migration to instance. Minor: change “@reture Adapter” to “@return Database”.
tests/e2e/Adapter/Base.php (1)
60-65: Prevent role leakage across testsClean roles before adding defaults to avoid cross-test state.
Apply:
- if (is_null(self::$authorization)) { - self::$authorization = new Authorization(); - } - - self::$authorization->addRole('any'); + if (is_null(self::$authorization)) { + self::$authorization = new Authorization(); + } + self::$authorization->cleanRoles(); + self::$authorization->addRole('any');
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (34)
README.md(1 hunks)bin/tasks/load.php(3 hunks)bin/tasks/query.php(2 hunks)bin/tasks/relationships.php(3 hunks)src/Database/Adapter.php(2 hunks)src/Database/Adapter/SQL.php(6 hunks)src/Database/Database.php(29 hunks)src/Database/Mirror.php(1 hunks)src/Database/Validator/Authorization.php(5 hunks)src/Database/Validator/Authorization/Input.php(1 hunks)tests/e2e/Adapter/Base.php(1 hunks)tests/e2e/Adapter/MariaDBTest.php(3 hunks)tests/e2e/Adapter/MirrorTest.php(12 hunks)tests/e2e/Adapter/MySQLTest.php(3 hunks)tests/e2e/Adapter/PoolTest.php(4 hunks)tests/e2e/Adapter/PostgresTest.php(3 hunks)tests/e2e/Adapter/SQLiteTest.php(3 hunks)tests/e2e/Adapter/Scopes/AttributeTests.php(41 hunks)tests/e2e/Adapter/Scopes/CollectionTests.php(29 hunks)tests/e2e/Adapter/Scopes/DocumentTests.php(126 hunks)tests/e2e/Adapter/Scopes/GeneralTests.php(9 hunks)tests/e2e/Adapter/Scopes/IndexTests.php(11 hunks)tests/e2e/Adapter/Scopes/PermissionTests.php(43 hunks)tests/e2e/Adapter/Scopes/RelationshipTests.php(36 hunks)tests/e2e/Adapter/Scopes/Relationships/ManyToManyTests.php(16 hunks)tests/e2e/Adapter/Scopes/Relationships/ManyToOneTests.php(15 hunks)tests/e2e/Adapter/Scopes/Relationships/OneToManyTests.php(17 hunks)tests/e2e/Adapter/Scopes/Relationships/OneToOneTests.php(18 hunks)tests/e2e/Adapter/Scopes/SpatialTests.php(25 hunks)tests/e2e/Adapter/SharedTables/MariaDBTest.php(3 hunks)tests/e2e/Adapter/SharedTables/MySQLTest.php(3 hunks)tests/e2e/Adapter/SharedTables/PostgresTest.php(3 hunks)tests/e2e/Adapter/SharedTables/SQLiteTest.php(3 hunks)tests/unit/Validator/AuthorizationTest.php(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-03T02:04:17.803Z
Learnt from: abnegate
PR: utopia-php/database#721
File: tests/e2e/Adapter/Scopes/DocumentTests.php:6418-6439
Timestamp: 2025-10-03T02:04:17.803Z
Learning: In tests/e2e/Adapter/Scopes/DocumentTests::testSchemalessDocumentInvalidInteralAttributeValidation (PHP), when the adapter reports getSupportForAttributes() === false (schemaless), the test should not expect exceptions from createDocuments for “invalid” internal attributes; remove try/catch and ensure the test passes without exceptions, keeping at least one assertion.
Applied to files:
tests/e2e/Adapter/Scopes/IndexTests.phptests/e2e/Adapter/Scopes/AttributeTests.phptests/e2e/Adapter/Scopes/DocumentTests.php
🧬 Code graph analysis (33)
src/Database/Adapter.php (2)
src/Database/Database.php (2)
Database(38-8163)setAuthorization(867-872)src/Database/Validator/Authorization.php (1)
Authorization(8-240)
tests/e2e/Adapter/Scopes/IndexTests.php (2)
tests/e2e/Adapter/Base.php (2)
getDatabase(40-40)deleteIndex(56-56)tests/e2e/Adapter/MariaDBTest.php (2)
getDatabase(21-53)deleteIndex(65-73)
src/Database/Adapter/SQL.php (1)
src/Database/Validator/Authorization.php (2)
getRoles(109-112)getStatus(161-164)
tests/e2e/Adapter/Scopes/GeneralTests.php (6)
src/Database/Database.php (4)
getDatabase(786-789)ping(1254-1257)getAuthorization(879-882)getAdapter(1231-1234)tests/e2e/Adapter/Base.php (1)
getDatabase(40-40)tests/e2e/Adapter/MirrorTest.php (1)
getDatabase(36-104)src/Database/Adapter.php (3)
getDatabase(178-181)ping(492-492)getSupportForCacheSkipOnFailure(1047-1047)src/Database/Validator/Authorization.php (4)
disable(202-205)reset(212-215)cleanRoles(117-120)addRole(91-94)src/Database/Helpers/Role.php (2)
Role(5-178)any(159-162)
tests/e2e/Adapter/Scopes/Relationships/ManyToManyTests.php (1)
tests/e2e/Adapter/Base.php (1)
getDatabase(40-40)
bin/tasks/load.php (4)
src/Database/Validator/Authorization.php (2)
Authorization(8-240)addRole(91-94)src/Database/Database.php (5)
Database(38-8163)exists(1303-1308)delete(1331-1345)create(1273-1292)createCollection(1360-1494)src/Database/Helpers/Role.php (2)
Role(5-178)any(159-162)src/Database/Helpers/Permission.php (2)
Permission(9-264)read(186-195)
src/Database/Mirror.php (2)
src/Database/Database.php (1)
getAuthorization(879-882)src/Database/Validator/Authorization.php (1)
skip(175-185)
bin/tasks/relationships.php (4)
src/Database/Validator/Authorization.php (2)
Authorization(8-240)addRole(91-94)src/Database/Database.php (3)
Database(38-8163)exists(1303-1308)create(1273-1292)src/Database/Helpers/Role.php (2)
Role(5-178)any(159-162)src/Database/Helpers/Permission.php (3)
Permission(9-264)read(186-195)update(220-229)
tests/e2e/Adapter/Scopes/Relationships/OneToManyTests.php (1)
tests/e2e/Adapter/Base.php (1)
getDatabase(40-40)
src/Database/Validator/Authorization/Input.php (2)
src/Database/Database.php (1)
Database(38-8163)src/Database/Validator/Authorization.php (1)
Authorization(8-240)
tests/e2e/Adapter/SQLiteTest.php (3)
src/Database/Database.php (6)
getDatabase(786-789)Database(38-8163)setAuthorization(867-872)getNamespace(758-761)deleteIndex(3439-3464)getTenant(1084-1087)tests/e2e/Adapter/Base.php (3)
getDatabase(40-40)deleteColumn(48-48)deleteIndex(56-56)src/Database/Adapter/SQLite.php (1)
deleteIndex(491-510)
tests/e2e/Adapter/Scopes/AttributeTests.php (4)
src/Database/Database.php (2)
getDatabase(786-789)getAuthorization(879-882)tests/e2e/Adapter/Base.php (2)
getDatabase(40-40)deleteColumn(48-48)src/Database/Validator/Authorization.php (1)
addRole(91-94)src/Database/Helpers/Role.php (2)
Role(5-178)any(159-162)
tests/e2e/Adapter/SharedTables/MariaDBTest.php (2)
src/Database/Database.php (5)
getDatabase(786-789)Database(38-8163)setAuthorization(867-872)getNamespace(758-761)deleteIndex(3439-3464)tests/e2e/Adapter/Base.php (3)
getDatabase(40-40)deleteColumn(48-48)deleteIndex(56-56)
tests/e2e/Adapter/Scopes/Relationships/OneToOneTests.php (1)
tests/e2e/Adapter/Base.php (1)
getDatabase(40-40)
tests/e2e/Adapter/Scopes/SpatialTests.php (1)
tests/e2e/Adapter/Base.php (1)
getDatabase(40-40)
src/Database/Database.php (4)
src/Database/Validator/Authorization.php (4)
Authorization(8-240)isValid(52-85)getDescription(42-45)skip(175-185)src/Database/Validator/Authorization/Input.php (1)
Input(5-49)src/Database/Adapter.php (1)
setAuthorization(61-66)src/Database/Document.php (4)
getCreate(109-112)getUpdate(117-120)getDelete(125-128)getPermissionsByType(145-157)
tests/e2e/Adapter/Scopes/RelationshipTests.php (5)
src/Database/Database.php (3)
getDatabase(786-789)getAdapter(1231-1234)getAuthorization(879-882)tests/e2e/Adapter/Base.php (1)
getDatabase(40-40)src/Database/Adapter.php (2)
getDatabase(178-181)getSupportForRelationships(1010-1010)src/Database/Validator/Authorization.php (2)
cleanRoles(117-120)addRole(91-94)src/Database/Helpers/Role.php (3)
Role(5-178)any(159-162)user(115-118)
tests/e2e/Adapter/PostgresTest.php (2)
src/Database/Database.php (6)
getDatabase(786-789)Database(38-8163)setAuthorization(867-872)getNamespace(758-761)deleteIndex(3439-3464)getTenant(1084-1087)tests/e2e/Adapter/Base.php (3)
getDatabase(40-40)deleteColumn(48-48)deleteIndex(56-56)
tests/e2e/Adapter/SharedTables/MySQLTest.php (2)
src/Database/Database.php (4)
getDatabase(786-789)Database(38-8163)setAuthorization(867-872)deleteIndex(3439-3464)tests/e2e/Adapter/Base.php (3)
getDatabase(40-40)deleteColumn(48-48)deleteIndex(56-56)
tests/e2e/Adapter/MySQLTest.php (5)
src/Database/Database.php (5)
getDatabase(786-789)Database(38-8163)setAuthorization(867-872)getNamespace(758-761)deleteIndex(3439-3464)tests/e2e/Adapter/Base.php (3)
getDatabase(40-40)deleteColumn(48-48)deleteIndex(56-56)tests/e2e/Adapter/MariaDBTest.php (3)
getDatabase(21-53)deleteColumn(55-63)deleteIndex(65-73)tests/e2e/Adapter/PoolTest.php (3)
getDatabase(35-76)deleteColumn(78-93)deleteIndex(95-110)src/Database/Adapter.php (4)
getDatabase(178-181)setAuthorization(61-66)getNamespace(125-128)deleteIndex(698-698)
tests/e2e/Adapter/Scopes/Relationships/ManyToOneTests.php (1)
tests/e2e/Adapter/Base.php (1)
getDatabase(40-40)
tests/e2e/Adapter/PoolTest.php (2)
src/Database/Database.php (5)
getDatabase(786-789)Database(38-8163)setAuthorization(867-872)getNamespace(758-761)deleteIndex(3439-3464)tests/e2e/Adapter/Base.php (3)
getDatabase(40-40)deleteColumn(48-48)deleteIndex(56-56)
tests/e2e/Adapter/Scopes/DocumentTests.php (3)
src/Database/Database.php (3)
getDatabase(786-789)getAuthorization(879-882)Database(38-8163)tests/e2e/Adapter/Base.php (1)
getDatabase(40-40)src/Database/Validator/Authorization.php (6)
disable(202-205)reset(212-215)addRole(91-94)removeRole(101-104)cleanRoles(117-120)skip(175-185)
tests/e2e/Adapter/SharedTables/PostgresTest.php (3)
src/Database/Database.php (4)
getDatabase(786-789)Database(38-8163)setAuthorization(867-872)deleteIndex(3439-3464)tests/e2e/Adapter/Base.php (3)
getDatabase(40-40)deleteColumn(48-48)deleteIndex(56-56)src/Database/Adapter.php (3)
getDatabase(178-181)setAuthorization(61-66)deleteIndex(698-698)
tests/e2e/Adapter/Scopes/PermissionTests.php (3)
src/Database/Database.php (3)
getDatabase(786-789)getAuthorization(879-882)getAdapter(1231-1234)tests/e2e/Adapter/Base.php (1)
getDatabase(40-40)src/Database/Validator/Authorization.php (5)
disable(202-205)reset(212-215)cleanRoles(117-120)addRole(91-94)skip(175-185)
tests/e2e/Adapter/Scopes/CollectionTests.php (3)
src/Database/Database.php (2)
getDatabase(786-789)getAuthorization(879-882)tests/e2e/Adapter/Base.php (1)
getDatabase(40-40)src/Database/Validator/Authorization.php (2)
skip(175-185)addRole(91-94)
tests/e2e/Adapter/SharedTables/SQLiteTest.php (3)
src/Database/Database.php (6)
getDatabase(786-789)Database(38-8163)setAuthorization(867-872)getNamespace(758-761)deleteIndex(3439-3464)getTenant(1084-1087)tests/e2e/Adapter/Base.php (3)
getDatabase(40-40)deleteColumn(48-48)deleteIndex(56-56)src/Database/Adapter/SQLite.php (1)
deleteIndex(491-510)
src/Database/Validator/Authorization.php (2)
src/Database/Database.php (1)
Database(38-8163)src/Database/Validator/Authorization/Input.php (3)
Input(5-49)getPermissions(40-43)getAction(45-48)
tests/unit/Validator/AuthorizationTest.php (2)
src/Database/Validator/Authorization.php (14)
Authorization(8-240)addRole(91-94)isValid(52-85)getDescription(42-45)isRole(127-130)cleanRoles(117-120)disable(202-205)reset(212-215)setDefaultStatus(139-143)enable(192-195)getRoles(109-112)removeRole(101-104)skip(175-185)getStatus(161-164)src/Database/Validator/Authorization/Input.php (1)
Input(5-49)
tests/e2e/Adapter/MirrorTest.php (3)
src/Database/Database.php (6)
getDatabase(786-789)setAuthorization(867-872)setDatabase(771-776)delete(1331-1345)exists(1303-1308)deleteIndex(3439-3464)tests/e2e/Adapter/Base.php (3)
getDatabase(40-40)deleteColumn(48-48)deleteIndex(56-56)src/Database/Mirror.php (7)
Mirror(11-1102)getSource(56-59)setDatabase(105-110)delete(192-195)getDestination(61-64)exists(182-185)deleteIndex(514-538)
tests/e2e/Adapter/Base.php (3)
src/Database/Validator/Authorization.php (3)
Authorization(8-240)addRole(91-94)setDefaultStatus(139-143)src/Database/Database.php (2)
getDatabase(786-789)Database(38-8163)tests/e2e/Adapter/MariaDBTest.php (1)
getDatabase(21-53)
bin/tasks/query.php (3)
src/Database/Database.php (2)
Database(38-8163)count(6951-6996)src/Database/Validator/Authorization.php (3)
Authorization(8-240)addRole(91-94)getRoles(109-112)src/Database/Adapter/SQL.php (1)
count(2524-2586)
tests/e2e/Adapter/MariaDBTest.php (2)
src/Database/Database.php (5)
getDatabase(786-789)Database(38-8163)setAuthorization(867-872)getNamespace(758-761)deleteIndex(3439-3464)tests/e2e/Adapter/Base.php (3)
getDatabase(40-40)deleteColumn(48-48)deleteIndex(56-56)
🪛 GitHub Actions: CodeQL
src/Database/Database.php
[error] 6991-6991: PHPStan: Variable $skipAuth on left side of ?? always exists and is not nullable.
tests/e2e/Adapter/PostgresTest.php
[error] 56-56: Binary operation "." between '"' and Utopia\Database\Database results in an error.
tests/e2e/Adapter/MySQLTest.php
[error] 63-63: Binary operation "." between '`' and Utopia\Database\Database results in an error.
tests/e2e/Adapter/PoolTest.php
[error] 80-80: Binary operation "." between '`' and Utopia\Database\Database results in an error.
[error] 97-97: Binary operation "." between '`' and Utopia\Database\Database results in an error.
🪛 GitHub Actions: Linter
tests/e2e/Adapter/Scopes/GeneralTests.php
[error] 1-1: PSR 12: 'no_unused_imports' issue.
src/Database/Mirror.php
[error] 1-1: PSR 12: 'no_unused_imports' and 'no_whitespace_in_blank_line' issues.
src/Database/Validator/Authorization/Input.php
[error] 1-1: PSR 12: 'single_blank_line_at_eof' issue.
tests/e2e/Adapter/Scopes/AttributeTests.php
[error] 1-1: PSR 12: 'no_unused_imports' issue.
src/Database/Database.php
[error] 1-1: PSR 12: 'single_space_around_construct' and 'no_trailing_whitespace' issues.
tests/e2e/Adapter/Scopes/RelationshipTests.php
[error] 1-1: PSR 12: 'no_unused_imports' issue.
tests/e2e/Adapter/Scopes/DocumentTests.php
[error] 1-1: PSR 12: 'no_unused_imports' issue.
tests/e2e/Adapter/Scopes/CollectionTests.php
[error] 1-1: PSR 12: 'single_line_after_imports' issue.
src/Database/Validator/Authorization.php
[error] 1-1: PSR 12: 'single_space_around_construct' and related style issues.
tests/unit/Validator/AuthorizationTest.php
[error] 1-1: PSR 12: 'single_blank_line_at_eof' issue.
bin/tasks/query.php
[error] 1-1: PSR 12: 'ordered_imports' rule violation.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Setup & Build Docker Image
🔇 Additional comments (29)
src/Database/Adapter.php (1)
15-16: Adapter authorization plumbing looks good.Import, property, and fluent setter are consistent; subclasses can use protected $authorization. No blockers.
Also applies to: 51-66
tests/e2e/Adapter/Scopes/IndexTests.php (1)
59-61: Switched to instance-based database access in tests — good.Using $this->getDatabase() and $this->deleteIndex() aligns with the new instance model.
Also applies to: 100-104, 162-168, 303-306, 331-333, 358-361, 369-372, 377-379, 407-410, 441-445, 470-472
tests/e2e/Adapter/Scopes/Relationships/OneToOneTests.php (1)
23-24: Instance-based DB access updates look correct.All updated calls use $this->getDatabase(), consistent with the refactor.
Also applies to: 448-449, 1010-1011, 1109-1111, 1201-1203, 1302-1304, 1493-1495, 1574-1576, 1658-1660, 1701-1702, 1766-1768, 1834-1836, 1903-1904, 1969-1971, 2294-2296, 2380-2381
tests/e2e/Adapter/Scopes/Relationships/ManyToOneTests.php (1)
20-21: Consistent migration to $this->getDatabase() in M2O tests.Changes align with instance-based initialization.
Also applies to: 354-355, 822-823, 923-925, 1034-1035, 1207-1209, 1290-1291, 1400-1401, 1466-1467, 1533-1534, 1601-1602, 1684-1685, 1766-1767
tests/e2e/Adapter/Scopes/Relationships/OneToManyTests.php (1)
20-21: LGTM: migrated to instance-based DB accessUsing $this->getDatabase() is consistent with the instance authorization refactor. No issues spotted in these initializations.
Also applies to: 392-393, 843-844, 1002-1003, 1134-1135, 1227-1228, 1322-1323, 1436-1437, 1528-1529, 1571-1572, 1637-1638, 1705-1706, 1773-1774, 1839-1840, 2022-2023, 2080-2081, 2161-2162
tests/e2e/Adapter/Scopes/SpatialTests.php (1)
21-21: LGTM: instance-based DB access throughout Spatial testsThe transition to
$this->getDatabase()looks correct and consistent with the PR’s authorization changes.Also applies to: 95-96, 247-248, 352-353, 400-401, 512-513, 617-618, 722-723, 919-920, 1349-1350, 1479-1480, 1781-1782, 1868-1869, 1954-1955, 2058-2059, 2163-2164, 2233-2234, 2363-2364, 2446-2447, 2487-2488, 2539-2540, 2575-2576, 2637-2638, 2667-2668, 2758-2759
tests/e2e/Adapter/Scopes/Relationships/ManyToManyTests.php (1)
20-20: Instance-based database access LGTMGood migration to $this->getDatabase(); no behavior changes.
Also applies to: 331-331, 785-785, 896-896, 996-996, 1110-1110, 1214-1214, 1257-1257, 1323-1323, 1391-1391, 1459-1459, 1525-1525, 1603-1603, 1724-1724, 1801-1801, 1886-1886
tests/e2e/Adapter/Scopes/AttributeTests.php (1)
62-62: Instance-based Database/Authorization usage LGTMConsistent $this->getDatabase() and $database->getAuthorization() adoption across tests looks good.
Also applies to: 161-161, 184-184, 195-195, 205-205, 229-229, 292-292, 346-346, 393-393, 416-416, 450-450, 512-512, 650-650, 774-774, 787-787, 796-796, 948-948, 1026-1026, 1188-1188, 1209-1209, 1243-1247, 1565-1565, 1655-1655, 1712-1712, 1723-1723, 1743-1743, 1768-1768, 1783-1783, 1810-1810, 1871-1871, 1979-1979, 2010-2010, 2039-2039
tests/e2e/Adapter/Scopes/CollectionTests.php (1)
26-26: Instance-based Database/Authorization usage LGTMGood migration to $this->getDatabase() and auth->skip closures; reads clean and consistent.
Also applies to: 45-45, 80-80, 312-312, 350-350, 363-363, 455-455, 497-497, 539-539, 597-597, 653-653, 727-727, 762-762, 782-782, 795-795, 901-901, 930-930, 947-947, 985-985, 1064-1064, 1248-1248, 1268-1268, 1332-1334, 404-409
src/Database/Adapter/SQL.php (1)
2339-2339: Authorization wiring confirmed
Adapter declaresprotected Authorization $authorizationand Database’s constructor invokessetAuthorization(new Authorization()), so calls togetRoles()/getStatus()are safe.tests/e2e/Adapter/Scopes/RelationshipTests.php (1)
33-33: LGTM! Consistent conversion to instance-based authorization.The changes successfully convert from static
Authorization::setRole()/Authorization::cleanRoles()to instance-based$this->getDatabase()->getAuthorization()->addRole()/cleanRoles()throughout the file. This aligns well with the PR's objective of moving to instance-based authorization.The test semantics are preserved, and the conversion is applied consistently across all test methods.
Also applies to: 407-407, 1748-1750
bin/tasks/query.php (1)
28-44: LGTM! Clean instance-based authorization pattern.The introduction of an
Authorizationinstance and the$setRolesclosure that captures it aligns perfectly with the PR's goal of moving to instance-based authorization. The closure correctly adds roles cumulatively to test query performance with increasing role counts, which appears to be the intended benchmark behavior.tests/e2e/Adapter/Scopes/PermissionTests.php (2)
206-208: LGTM! Consistent instance-based authorization pattern.The conversion from static
Authorization::cleanRoles()/Authorization::setRole()to instance-based$this->getDatabase()->getAuthorization()->cleanRoles()/addRole()is consistent throughout the file and aligns with the PR's objectives.The test logic is preserved, and the changes correctly implement the new instance-based authorization access pattern.
Note: Similar to RelationshipTests.php, there are multiple repeated calls to
$this->getDatabase()->getAuthorization()in some methods (e.g., lines 206-207, 230-236) that could optionally be refactored to use a local variable for better readability, but this is a minor nitpick.Also applies to: 230-236
322-346: Authorization skip pattern is correctly implemented.The
skip()method insrc/Database/Validator/Authorization.php(lines 175–185) properly uses a try-finally block to restore authorization state, guaranteeing restoration even if the callback throws an exception. The implementation is safe for nested calls and exception scenarios. No changes needed.src/Database/Database.php (2)
420-421: Centralized Authorization instance wiring looks solidDefault auth instantiation + setAuthorization propagation to Adapter is correct and consistent with the new instance-based model.
Also applies to: 867-872
3536-3541: Consistent READ gating via Authorization InputGood use of Input(action, permissions) combining collection READ and document READ when documentSecurity is enabled. Returning empty Document on denial preserves prior semantics.
Also applies to: 3563-3569
tests/e2e/Adapter/MariaDBTest.php (1)
21-21: Tests aligned to instance‑based Database and AuthorizationInstance
getDatabase(), auth injection viasetAuthorization(...), and instance helpers for DDL look correct for the new model.Also applies to: 41-42, 55-58, 65-68
tests/e2e/Adapter/MirrorTest.php (1)
110-116: LGTM on instance callsite updatesSwitching to
$this->getDatabase()across tests is consistent and clear.Also applies to: 122-129, 136-146, 154-182, 184-196, 205-236, 247-285, 287-315
tests/unit/Validator/AuthorizationTest.php (1)
12-21: Solid migration to instance-based Authorization with InputCoverage of roles, status defaults, reset/enable/disable, and skip (incl. nested) looks good.
Also applies to: 29-101, 106-125
tests/e2e/Adapter/SharedTables/SQLiteTest.php (1)
33-71: Instance migration and auth injection look correctInitialization, cache reset, and fluent chain are consistent with Base and core.
Also applies to: 55-61
tests/e2e/Adapter/SharedTables/MySQLTest.php (1)
33-68: Instance getDatabase + auth injection look goodSetup aligns with Base and other adapters.
Also applies to: 52-59
tests/e2e/Adapter/PostgresTest.php (1)
40-40: Authorization injection looks goodChaining setAuthorization(self::$authorization) aligns with the new instance-based flow.
tests/e2e/Adapter/Base.php (3)
32-35: Static Authorization holder introduced — OKCentralizing a shared Authorization for tests is fine.
40-40: Abstract methods migrated to instance — OKSignature matches new usage across tests.
69-70: Tear down status reset — OKStatus reset to default is appropriate.
tests/e2e/Adapter/SharedTables/MariaDBTest.php (3)
50-57: Authorization injection and init chain — OKFluent initialization with setAuthorization(self::$authorization) is correct.
68-76: deleteColumn SQL assembly — OKUses database and namespace correctly for MariaDB.
78-86: deleteIndex SQL assembly — OKIndex drop statement is correct for MariaDB.
tests/e2e/Adapter/PoolTest.php (1)
63-65: Authorization injection — OKWiring setAuthorization(self::$authorization) looks correct.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bin/tasks/relationships.php (1)
191-205: Fix PDOPool connection leak on exceptionsOn errors, the PDO proxy isn’t returned to the pool, causing resource exhaustion under concurrency. Return it in finally.
Apply:
- go(function () use ($cfg, $pool, $name, $namespace, $sharedTables, $cache, $globalDocs) { - try { - $pdo = $pool->get(); - - $database = (new Database(new ($cfg['adapter'])($pdo), $cache)) - ->setDatabase($name) - ->setNamespace($namespace) - ->setSharedTables($sharedTables); - - createRelationshipDocuments($database, $globalDocs['categories'], $globalDocs['users']); - $pool->put($pdo); - } catch (\Throwable $error) { - // Errors caught but documents still created successfully - likely concurrent update race conditions - } - }); + go(function () use ($cfg, $pool, $name, $namespace, $sharedTables, $cache, $globalDocs) { + $pdo = null; + try { + $pdo = $pool->get(); + $database = (new Database(new ($cfg['adapter'])($pdo), $cache)) + ->setDatabase($name) + ->setNamespace($namespace) + ->setSharedTables($sharedTables); + // Optional: be explicit about auth context + // $database->getAuthorization()->addRole(Role::any()->toString()); + createRelationshipDocuments($database, $globalDocs['categories'], $globalDocs['users']); + } catch (\Throwable $error) { + // swallow; concurrent races are acceptable in this generator + } finally { + if ($pdo !== null) { + $pool->put($pdo); + } + } + });
♻️ Duplicate comments (2)
src/Database/Database.php (1)
6973-6992: Fix conditional: remove null‑coalescing on $skipAuth (breaks static analysis)
$skipAuthis a bool;?? falseis redundant and flagged by PHPStan. Use a plain ternary.Apply this diff:
- $skipAuth = $this->authorization->isValid(new Input(self::PERMISSION_READ, $collection->getRead())); + $skipAuth = $this->authorization->isValid(new Input(self::PERMISSION_READ, $collection->getRead())); ... - $getCount = fn () => $this->adapter->count($collection, $queries, $max); - $count = $skipAuth ?? false ? $this->authorization->skip($getCount) : $getCount(); + $getCount = fn () => $this->adapter->count($collection, $queries, $max); + $count = $skipAuth ? $this->authorization->skip($getCount) : $getCount();#!/bin/bash # Scan for any remaining incorrect patterns like "?? false ? $this->authorization->skip(" across the repo. rg -n -C2 -P '\?\?\s*false\s*\?\s*\$this->authorization->skip\s*\('tests/unit/Validator/AuthorizationTest.php (1)
126-126: Fix PSR-12: ensure single blank line at EOF.This issue was previously flagged and remains unresolved. Add a trailing newline at end of file to satisfy the linter.
-} +} +
🧹 Nitpick comments (5)
tests/e2e/Adapter/Scopes/GeneralTests.php (2)
90-93: Consider storing the database reference once.Lines 90 and 93 both call
$this->getDatabase(). While not incorrect, storing the database reference in a local variable once would be more efficient.Apply this diff to consolidate the calls:
- $this->getDatabase()->getAuthorization()->disable(); - - /** @var Database $database */ - $database = $this->getDatabase(); + /** @var Database $database */ + $database = $this->getDatabase(); + $database->getAuthorization()->disable();
185-188: Consider storing the database reference once.Lines 185 and 188 both call
$this->getDatabase(). While not incorrect, storing the database reference in a local variable once would be more efficient.Apply this diff to consolidate the calls:
- $this->getDatabase()->getAuthorization()->disable(); - - /** @var Database $database */ - $database = $this->getDatabase(); + /** @var Database $database */ + $database = $this->getDatabase(); + $database->getAuthorization()->disable();src/Database/Database.php (1)
861-872: Return type nit: preferstaticfor fluent API consistencyMost fluent setters in this class return
static. Consider changingsetAuthorization(...): selftostaticfor consistency and better subclass chaining. Behavior unchanged.- public function setAuthorization(Authorization $authorization): self + public function setAuthorization(Authorization $authorization): static { $this->adapter->setAuthorization($authorization); $this->authorization = $authorization; return $this; }bin/tasks/query.php (2)
37-42: Make generated roles unique to avoid collisionsFaker numerify can repeat; collisions skew role counts. Use Faker’s unique() (and increase digits) to ensure uniqueness.
Apply:
- $setRoles = function (Authorization $authorization, $faker, $count): int { + $setRoles = function (Authorization $authorization, $faker, $count): int { for ($i = 0; $i < $count; $i++) { - $authorization->addRole($faker->numerify('user####')); + $authorization->addRole($faker->unique()->numerify('user########')); } return \count($authorization->getRoles()); };
142-145: Harden results persistence (error handling and atomic write)Use file_put_contents with LOCK_EX and check JSON encoding errors.
Apply:
- $results = \fopen("bin/view/results/{$adapter}_{$name}_{$limit}_{$time}.json", 'w'); - \fwrite($results, \json_encode($report)); - \fclose($results); + $path = "bin/view/results/{$adapter}_{$name}_{$limit}_{$time}.json"; + $json = \json_encode($report, JSON_PRETTY_PRINT | JSON_UNESCAPED_SLASHES); + if ($json === false) { + Console::error('Failed to encode report: ' . \json_last_error_msg()); + return; + } + if (\file_put_contents($path, $json, LOCK_EX) === false) { + Console::error("Failed to write results to {$path}"); + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
bin/tasks/load.php(3 hunks)bin/tasks/query.php(3 hunks)bin/tasks/relationships.php(3 hunks)bin/view/results/mariadb_testing_1000_1760626673.json(1 hunks)src/Database/Database.php(29 hunks)src/Database/Mirror.php(1 hunks)src/Database/Validator/Authorization.php(4 hunks)src/Database/Validator/Authorization/Input.php(1 hunks)tests/e2e/Adapter/Scopes/AttributeTests.php(41 hunks)tests/e2e/Adapter/Scopes/CollectionTests.php(29 hunks)tests/e2e/Adapter/Scopes/DocumentTests.php(126 hunks)tests/e2e/Adapter/Scopes/GeneralTests.php(9 hunks)tests/e2e/Adapter/Scopes/RelationshipTests.php(36 hunks)tests/unit/Validator/AuthorizationTest.php(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- bin/view/results/mariadb_testing_1000_1760626673.json
🚧 Files skipped from review as they are similar to previous changes (5)
- src/Database/Mirror.php
- src/Database/Validator/Authorization/Input.php
- tests/e2e/Adapter/Scopes/AttributeTests.php
- tests/e2e/Adapter/Scopes/DocumentTests.php
- bin/tasks/load.php
🧰 Additional context used
🧬 Code graph analysis (8)
tests/e2e/Adapter/Scopes/CollectionTests.php (4)
src/Database/Database.php (2)
getDatabase(786-789)getAuthorization(879-882)tests/e2e/Adapter/Base.php (1)
getDatabase(40-40)src/Database/Validator/Authorization.php (2)
skip(175-185)addRole(91-94)src/Database/Helpers/Role.php (2)
Role(5-178)label(149-152)
tests/unit/Validator/AuthorizationTest.php (3)
src/Database/Database.php (1)
Database(38-8163)src/Database/Validator/Authorization.php (14)
Authorization(8-240)addRole(91-94)isValid(52-85)getDescription(42-45)isRole(127-130)cleanRoles(117-120)disable(202-205)reset(212-215)setDefaultStatus(139-143)enable(192-195)getRoles(109-112)removeRole(101-104)skip(175-185)getStatus(161-164)src/Database/Validator/Authorization/Input.php (1)
Input(5-49)
tests/e2e/Adapter/Scopes/RelationshipTests.php (5)
src/Database/Database.php (2)
getDatabase(786-789)getAuthorization(879-882)tests/e2e/Adapter/Base.php (1)
getDatabase(40-40)src/Database/Adapter.php (2)
getDatabase(178-181)getSupportForRelationships(1010-1010)src/Database/Validator/Authorization.php (2)
cleanRoles(117-120)addRole(91-94)src/Database/Helpers/Role.php (3)
Role(5-178)any(159-162)user(115-118)
bin/tasks/query.php (3)
src/Database/Validator/Authorization.php (3)
Authorization(8-240)addRole(91-94)getRoles(109-112)src/Database/Database.php (2)
count(6951-6996)getAuthorization(879-882)src/Database/Adapter/SQL.php (1)
count(2524-2586)
tests/e2e/Adapter/Scopes/GeneralTests.php (5)
src/Database/Database.php (3)
getDatabase(786-789)getAuthorization(879-882)getAdapter(1231-1234)tests/e2e/Adapter/Base.php (1)
getDatabase(40-40)src/Database/Adapter.php (2)
getDatabase(178-181)getSupportForCacheSkipOnFailure(1047-1047)src/Database/Validator/Authorization.php (4)
disable(202-205)reset(212-215)cleanRoles(117-120)addRole(91-94)src/Database/Helpers/Role.php (2)
Role(5-178)any(159-162)
bin/tasks/relationships.php (3)
src/Database/Database.php (2)
Database(38-8163)getAuthorization(879-882)src/Database/Validator/Authorization.php (1)
addRole(91-94)src/Database/Helpers/Role.php (2)
Role(5-178)any(159-162)
src/Database/Database.php (3)
src/Database/Validator/Authorization.php (4)
Authorization(8-240)isValid(52-85)getDescription(42-45)skip(175-185)src/Database/Validator/Authorization/Input.php (1)
Input(5-49)src/Database/Adapter.php (1)
setAuthorization(61-66)
src/Database/Validator/Authorization.php (2)
src/Database/Database.php (1)
Database(38-8163)src/Database/Validator/Authorization/Input.php (3)
Input(5-49)getPermissions(40-43)getAction(45-48)
🪛 GitHub Actions: CodeQL
src/Database/Database.php
[error] 6991-6991: PHPStan: Variable $skipAuth on left side of ?? always exists and is not nullable.
🔇 Additional comments (33)
tests/e2e/Adapter/Scopes/GeneralTests.php (7)
27-27: LGTM!The migration from
static::getDatabase()to$this->getDatabase()is correct.
46-46: LGTM!The migration from
static::getDatabase()to$this->getDatabase()is correct.
180-180: LGTM!The migration to instance-based authorization reset is correct.
285-285: LGTM!The migration to instance-based authorization reset is correct.
310-310: LGTM!The migration from
static::getDatabase()to$this->getDatabase()is correct.
369-369: LGTM!The migration from
static::getDatabase()to$this->getDatabase()is correct.
424-424: LGTM!The migration from
static::getDatabase()to$this->getDatabase()is correct.src/Database/Database.php (19)
28-28: Import for Authorization Input is correctNew Input class is properly imported for instance-based checks.
401-405: Private Authorization instance on DatabaseGood encapsulation; aligns all checks through a single instance.
420-421: Initialize default Authorization and wire to adapterConstructing and setting the default Authorization in ctor is correct and avoids null access.
3536-3541: Read check on cached document pathUsing instance Authorization with Input(action: read, merged collection/document permissions) is correct. Early-return with empty Document on deny is consistent.
3563-3567: Read check on fresh document pathSame as cached path; consistent permission enforcement.
4229-4232: Create permission gate on createDocumentCollection-level create validation via Authorization::isValid(Input) is correct; throwing AuthorizationException with validator description is appropriate.
4336-4338: Create permission gate on createDocuments (batch)Batch path mirrors single-create semantics; good.
4763-4766: Bypass auth to fetch “old” document for updateWrapping the for-update read with authorization->skip prevents spurious read-denials while still checking update/read permissions later. Good pattern.
4889-4907: Update vs Read permission check splitBuilding update/read permission sets and validating only when needed (based on actual changes) is efficient and correct.
4998-5002: Short-circuit when lacking collection-level update and no document securityCorrectly blocks mass updates when docSecurity is off and caller lacks collection-level update.
5661-5670: Upsert: old-document fetch under auth skipCorrectly skips authorization to probe existence (including per-tenant variant), deferring actual auth checks to create/update paths.
5701-5709: Upsert: permission checks per operationValidates create vs update with merged doc permissions when documentSecurity is enabled; correct behavior.
5801-5805: Upsert: wrap adapter call in authorization->skipAvoids redundant per-row auth during engine upsert; higher-level checks already enforced.
5901-5915: Increase attribute: auth-skip read + update permission checkPattern matches updateDocument. Good separation of concerns.
5999-6013: Decrease attribute: auth-skip read + update permission checkMirrors increase path; correct.
6065-6081: Delete: auth-skip read-for-update + delete permission checkCorrect to fetch under skip then enforce delete permissions (collection and document when applicable).
6244-6272: Cascade helpers: use authorization->skip for internal maintenance writesUsing skip in deleteRestrict to clear inverse keys avoids requiring user write privileges for system-enforced integrity.
6305-6371: Cascade helpers (setNull): all maintenance updates under auth skipConsistently skipping authorization for integrity updates is appropriate.
6751-6756: find(): collection-level bypass via skipAuthComputing skipAuth per requested permission and conditionally wrapping adapter call in authorization->skip is correct and mirrors other paths.
Also applies to: 6831-6832
tests/unit/Validator/AuthorizationTest.php (3)
12-12: LGTM! Clean setup for instance-based authorization.The introduction of the
Inputimport and the protected$authorizationproperty, initialized insetUp, correctly establishes the instance-based authorization pattern. This aligns well with the broader refactoring effort to move away from static authorization state.Also applies to: 16-21
29-101: LGTM! Thorough migration to instance-based API.The refactoring correctly replaces all static authorization calls with instance methods:
- Role management:
addRole,removeRole,cleanRoles,isRole,getRoles- Status management:
disable,enable,reset,setDefaultStatus,getStatus- Validation: All
isValidcalls properly wrap permissions withnew Input(action, permissions)- Skip functionality: Correctly uses instance method with Input wrapper
The test logic remains comprehensive and validates all authorization behaviors with the new API.
104-125: LGTM! Nested skip test correctly uses instance API.The nested skip test properly validates that the authorization status is correctly managed across multiple skip levels, using
$this->authorization->getStatus()to check the state and$this->authorization->skip()for nesting.tests/e2e/Adapter/Scopes/RelationshipTests.php (1)
1748-1749: LGTM: migrated to instance-based Authorization in testsClean, instance-scoped role management via $this->getDatabase()->getAuthorization() matches the new API and improves isolation.
Also applies to: 1825-1826, 1838-1839
tests/e2e/Adapter/Scopes/CollectionTests.php (2)
403-407: LGTM: correct use of Authorization->skip for privileged cleanupWrapping deletes in skip avoids false auth failures and keeps tests fast and deterministic.
919-924: LGTM: label role test aligns with per-instance rolesAdding Role::label('reader') on the instance precisely validates label-based access.
src/Database/Validator/Authorization.php (1)
52-85: LGTM: solid instance-scoped Authorization with Input
- Type-safe isValid(Input) with clear messages.
- Per-instance roles/status; safe skip() restore.
- API (add/remove/clean/getRoles, enable/disable/reset) looks coherent.
Also applies to: 91-120, 151-165, 175-215
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Database/Database.php (1)
7033-7051: Bug: sum() missing authorization gating and exception path (inconsistent with find()/count())sum() neither throws when collection has no doc security nor bypasses engine‑level filtering when collection scope exists. This can yield incorrect sums or inconsistent errors.
Apply this diff to align with find()/count():
public function sum(string $collection, string $attribute, array $queries = [], ?int $max = null): float|int { $collection = $this->silent(fn () => $this->getCollection($collection)); $attributes = $collection->getAttribute('attributes', []); $indexes = $collection->getAttribute('indexes', []); @@ if ($this->validate) { $validator = new DocumentsValidator( $attributes, $indexes, $this->adapter->getIdAttributeType(), $this->maxQueryValues, $this->adapter->getMinDateTime(), $this->adapter->getMaxDateTime(), ); if (!$validator->isValid($queries)) { throw new QueryException($validator->getDescription()); } } + $documentSecurity = $collection->getAttribute('documentSecurity', false); + $skipAuth = $this->authorization->isValid(new Input(self::PERMISSION_READ, $collection->getRead())); + if (!$skipAuth && !$documentSecurity && $collection->getId() !== self::METADATA) { + throw new AuthorizationException($this->authorization->getDescription()); + } + $relationships = \array_filter( $collection->getAttribute('attributes', []), fn (Document $attribute) => $attribute->getAttribute('type') === self::VAR_RELATIONSHIP ); $queries = $this->convertQueries($collection, $queries); @@ - $sum = $this->adapter->sum($collection, $attribute, $queries, $max); + $getSum = fn () => $this->adapter->sum($collection, $attribute, $queries, $max); + $sum = $skipAuth ? $this->authorization->skip($getSum) : $getSum(); $this->trigger(self::EVENT_DOCUMENT_SUM, $sum); return $sum; }
♻️ Duplicate comments (1)
tests/e2e/Adapter/PostgresTest.php (1)
66-68: Fix null tenant handling and Postgres DROP INDEX schema qualifier.Two issues remain unresolved from the previous review:
Line 66: When
getTenant()returnsnull, the key becomes"namespace__collection_index"(double underscore). Handle the null case explicitly:Line 68: Postgres
DROP INDEXexpects schema qualification, not database qualification. Use$this->getDatabase()->getNamespace()instead of$this->getDatabase()->getDatabase().Apply this diff to fix both issues:
- $key = "\"".$this->getDatabase()->getNamespace()."_".$this->getDatabase()->getTenant()."_{$collection}_{$index}\""; + $tenant = $this->getDatabase()->getTenant(); + $tenantPart = $tenant ? "{$tenant}_" : ""; + $key = "\"".$this->getDatabase()->getNamespace()."_{$tenantPart}{$collection}_{$index}\""; - $sql = "DROP INDEX \"".$this->getDatabase()->getDatabase()."\".{$key}"; + $sql = "DROP INDEX \"".$this->getDatabase()->getNamespace()."\".{$key}";
🧹 Nitpick comments (2)
src/Database/Database.php (2)
861-883: Unify setter return type: preferstaticfor fluencyMost setters in this class return
static. AlignsetAuthorizationfor consistency and better subclass fluency.- /** - * Sets instance of authorization for permission checks - * - * @param Authorization $authorization - * @return self - */ - public function setAuthorization(Authorization $authorization): self + /** + * Sets instance of authorization for permission checks + * + * @param Authorization $authorization + * @return static + */ + public function setAuthorization(Authorization $authorization): static { $this->adapter->setAuthorization($authorization); $this->authorization = $authorization; return $this; }
6245-6245: Set‑null/Restrict operations useauthorization->skipappropriately; verify cascade intentSet‑null/Restrict paths bypass user perms for integrity writes. However, cascade delete path (Lines 6417‑6489) calls
deleteDocument(...)withoutauthorization->skip, requiring delete permission on related docs.Would you like cascades to bypass user perms as well (consistent with set‑null/restrict)? If yes, wrap those
deleteDocumentcalls inauthorization->skip.- $this->deleteDocument( + $this->authorization->skip(fn () => $this->deleteDocument( $relatedCollection->getId(), - ($value instanceof Document) ? $value->getId() : $value - ); + ($value instanceof Document) ? $value->getId() : $value + ));Also applies to: 6269-6269, 6305-6305, 6337-6337, 6363-6363
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Database/Database.php(29 hunks)tests/e2e/Adapter/MySQLTest.php(3 hunks)tests/e2e/Adapter/PoolTest.php(4 hunks)tests/e2e/Adapter/PostgresTest.php(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/e2e/Adapter/PoolTest.php
🧰 Additional context used
🧬 Code graph analysis (3)
tests/e2e/Adapter/PostgresTest.php (3)
src/Database/Database.php (5)
getDatabase(786-789)Database(38-8163)setAuthorization(867-872)getNamespace(758-761)deleteIndex(3439-3464)tests/e2e/Adapter/Base.php (3)
getDatabase(40-40)deleteColumn(48-48)deleteIndex(56-56)src/Database/Adapter/Postgres.php (1)
deleteIndex(909-922)
src/Database/Database.php (4)
src/Database/Validator/Authorization.php (4)
Authorization(8-240)isValid(52-85)getDescription(42-45)skip(175-185)src/Database/Validator/Authorization/Input.php (1)
Input(5-49)src/Database/Adapter.php (8)
setAuthorization(61-66)updateDocument(743-743)deleteDocument(791-791)getTenant(234-237)getDocument(709-709)withTransaction(395-442)upsertDocuments(770-774)count(843-843)src/Database/Adapter/SQL.php (3)
getDocument(350-416)upsertDocuments(2109-2266)count(2524-2586)
tests/e2e/Adapter/MySQLTest.php (3)
src/Database/Database.php (5)
getDatabase(786-789)Database(38-8163)setAuthorization(867-872)getNamespace(758-761)deleteIndex(3439-3464)tests/e2e/Adapter/PoolTest.php (3)
getDatabase(35-76)deleteColumn(78-93)deleteIndex(95-110)tests/e2e/Adapter/Base.php (3)
getDatabase(40-40)deleteColumn(48-48)deleteIndex(56-56)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Setup & Build Docker Image
🔇 Additional comments (15)
tests/e2e/Adapter/MySQLTest.php (3)
27-58: LGTM! Instance-based refactoring applied correctly.The conversion from static to instance method is correct, and the authorization is properly wired through
setAuthorization(self::$authorization).
61-69: LGTM! Database concatenation issue resolved.The string concatenation now correctly calls
$this->getDatabase()->getDatabase()to obtain the database name string instead of concatenating the Database object directly. This resolves the critical issue flagged in the previous review.
71-79: LGTM! Database concatenation issue resolved.Similar to
deleteColumn, the string concatenation correctly uses$this->getDatabase()->getDatabase()to obtain the database name string.tests/e2e/Adapter/PostgresTest.php (2)
21-51: LGTM! Instance-based refactoring applied correctly.The conversion from static to instance method is correct, and authorization is properly wired through
setAuthorization(self::$authorization).
54-62: LGTM! Database concatenation issue resolved.The string concatenation now correctly calls
$this->getDatabase()->getDatabase()to obtain the database name string instead of concatenating the Database object directly. This resolves the critical issue flagged in the previous review.src/Database/Database.php (10)
400-405: Authorization instance wiring looks goodPrivate Authorization property, default initialization, and propagation to Adapter are correct and cohesive.
Also applies to: 420-421
3536-3543: Read checks on cached and fresh getDocument are correctUnion of collection/document read scopes via
Inputand returning emptyDocumenton denial is consistent.Also applies to: 3563-3569
4229-4232: Create permission checks correctly migrated to instanceAuthorizationSimple and batch create both validate via
Input(PERMISSION_CREATE, $collection->getCreate())and throw on denial. Good.Also applies to: 4336-4339
4763-4766: Update flow: safe read for update and proper gate on read/update
- Reading old doc under
authorization->skipavoids requiring read permission just to update.- Conditional check for UPDATE vs READ based on actual changes is solid.
Also applies to: 4889-4907
5369-5369: Relationship updates honor internal integrity without leaking permissionsUsing
authorization->skipfor side-effect updates/deletes on related docs/junctions is appropriate.Also applies to: 5496-5496
5661-5670: Upsert authorization checks and adapter call strategy look correct
- Old-fetch under
skipis safe.- Create vs Update checks via
Inputmirror single-doc flows.- Wrapping adapter upsert in
authorization->skipavoids redundant engine-level filtering after prechecks.Also applies to: 5701-5709, 5801-5805
5901-5901: Increase/Decrease attribute: permission and locking behavior is correctLocks with
forUpdate, validates update scopes (collection + optional document), then mutates. Good.Also applies to: 5910-5915, 5999-5999, 6008-6013
6065-6077: Delete: secure locked read and delete permission gate are correctFetching under
skipwithforUpdate, then checking delete scopes (collection + optional document) is sound.
6751-6756: Find: collection‑level bypass vs document‑level filtering is consistent
- Throws when no collection scope and no doc security.
- Uses
authorization->skipto avoid engine‑level permission predicates when collection scope exists. Matches new model.Also applies to: 6831-6831
6973-6992: Count: aligns with find’s authorization semanticsCollection‑scope check +
skiparound adapter call mirrors find(); good parity.
…ion method in Database Pool class for improved authorization handling.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/Database/Adapter/Pool.php (1)
573-577: Implementation correctly follows the Adapter pattern.The setter properly stores the authorization instance and returns
$thisfor method chaining, consistent with the parent class implementation.For consistency with other imports in the file, consider adding an import statement instead of using the fully qualified class name:
use Utopia\Database\Adapter; use Utopia\Database\Database; use Utopia\Database\Document; use Utopia\Database\Exception as DatabaseException; +use Utopia\Database\Validator\Authorization; use Utopia\Pools\Pool as UtopiaPool;Then update the method signature:
- public function setAuthorization(\Utopia\Database\Validator\Authorization $authorization): self + public function setAuthorization(Authorization $authorization): self
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
phpunit.xml(1 hunks)src/Database/Adapter/Pool.php(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- phpunit.xml
🧰 Additional context used
🧬 Code graph analysis (1)
src/Database/Adapter/Pool.php (3)
src/Database/Database.php (2)
setAuthorization(867-872)Database(38-8163)src/Database/Adapter.php (1)
setAuthorization(61-66)src/Database/Validator/Authorization.php (1)
Authorization(8-240)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (SharedTables/SQLite)
- GitHub Check: Adapter Tests (SharedTables/Postgres)
- GitHub Check: Adapter Tests (MySQL)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (MariaDB)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Adapter Tests (Postgres)
- GitHub Check: Adapter Tests (Pool)
…rization-instance
…ce methods for role management and permission checks.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/e2e/Adapter/Scopes/Relationships/ManyToOneTests.php (1)
1826-1826: Fix inconsistent database access pattern.This line uses
static::getDatabase()while all other test methods in this PR use$this->getDatabase(). Update for consistency with the instance-based refactor.Apply this diff:
- $database = static::getDatabase(); + $database = $this->getDatabase();tests/e2e/Adapter/Scopes/Relationships/OneToManyTests.php (1)
2221-2221: Fix inconsistent database access pattern.These lines use
static::getDatabase()while all other test methods in this PR use$this->getDatabase(). Update for consistency with the instance-based refactor.Apply this diff to all five occurrences:
- $database = static::getDatabase(); + $database = $this->getDatabase();Also applies to: 2326-2326, 2424-2424, 2540-2540, 2595-2595
🧹 Nitpick comments (2)
tests/e2e/Adapter/Scopes/DocumentTests.php (1)
585-597: Use try/finally to guarantee authorization resetIf
upsertDocumentsever throws, the authorization status stays disabled for everything that follows, which will corrupt this test and subsequent ones. Capture the instance once and reset it in afinallyblock.- $this->getDatabase()->getAuthorization()->disable(); - - $results = []; - $count = $database->upsertDocuments( - __FUNCTION__, - $documents, - onNext: function ($doc) use (&$results) { - $results[] = $doc; - } - ); - - $this->getDatabase()->getAuthorization()->reset(); + $authorization = $database->getAuthorization(); + $authorization->disable(); + + $results = []; + $count = 0; + + try { + $count = $database->upsertDocuments( + __FUNCTION__, + $documents, + onNext: function ($doc) use (&$results) { + $results[] = $doc; + } + ); + } finally { + $authorization->reset(); + }tests/e2e/Adapter/Scopes/RelationshipTests.php (1)
3260-3260: Update for consistency with instance-based pattern.Line 3260 uses
static::getDatabase()while all other test methods in this file now use$this->getDatabase(). For consistency with the instance-based architecture introduced in this PR, consider updating this line.Apply this diff:
- $database = static::getDatabase(); + $database = $this->getDatabase();
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
src/Database/Adapter.php(2 hunks)src/Database/Adapter/Pool.php(2 hunks)src/Database/Adapter/SQL.php(6 hunks)src/Database/Database.php(29 hunks)tests/e2e/Adapter/Base.php(1 hunks)tests/e2e/Adapter/PostgresTest.php(3 hunks)tests/e2e/Adapter/Scopes/CollectionTests.php(29 hunks)tests/e2e/Adapter/Scopes/DocumentTests.php(126 hunks)tests/e2e/Adapter/Scopes/IndexTests.php(11 hunks)tests/e2e/Adapter/Scopes/PermissionTests.php(43 hunks)tests/e2e/Adapter/Scopes/RelationshipTests.php(36 hunks)tests/e2e/Adapter/Scopes/Relationships/ManyToManyTests.php(16 hunks)tests/e2e/Adapter/Scopes/Relationships/ManyToOneTests.php(15 hunks)tests/e2e/Adapter/Scopes/Relationships/OneToManyTests.php(17 hunks)tests/e2e/Adapter/Scopes/Relationships/OneToOneTests.php(18 hunks)tests/e2e/Adapter/Scopes/SpatialTests.php(25 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- src/Database/Adapter/Pool.php
- tests/e2e/Adapter/Scopes/IndexTests.php
- tests/e2e/Adapter/Scopes/Relationships/ManyToManyTests.php
- src/Database/Adapter.php
- tests/e2e/Adapter/Scopes/Relationships/OneToOneTests.php
- src/Database/Adapter/SQL.php
- tests/e2e/Adapter/Scopes/SpatialTests.php
- tests/e2e/Adapter/PostgresTest.php
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-03T02:04:17.803Z
Learnt from: abnegate
PR: utopia-php/database#721
File: tests/e2e/Adapter/Scopes/DocumentTests.php:6418-6439
Timestamp: 2025-10-03T02:04:17.803Z
Learning: In tests/e2e/Adapter/Scopes/DocumentTests::testSchemalessDocumentInvalidInteralAttributeValidation (PHP), when the adapter reports getSupportForAttributes() === false (schemaless), the test should not expect exceptions from createDocuments for “invalid” internal attributes; remove try/catch and ensure the test passes without exceptions, keeping at least one assertion.
Applied to files:
tests/e2e/Adapter/Scopes/DocumentTests.php
🧬 Code graph analysis (8)
tests/e2e/Adapter/Base.php (3)
src/Database/Validator/Authorization.php (3)
Authorization(8-240)addRole(91-94)setDefaultStatus(139-143)src/Database/Database.php (2)
getDatabase(837-840)Database(38-8296)tests/e2e/Adapter/PostgresTest.php (1)
getDatabase(21-52)
tests/e2e/Adapter/Scopes/Relationships/OneToManyTests.php (1)
tests/e2e/Adapter/Base.php (1)
getDatabase(42-42)
tests/e2e/Adapter/Scopes/DocumentTests.php (3)
src/Database/Database.php (3)
getDatabase(837-840)getAuthorization(930-933)Database(38-8296)tests/e2e/Adapter/Base.php (1)
getDatabase(42-42)src/Database/Validator/Authorization.php (6)
disable(202-205)reset(212-215)addRole(91-94)removeRole(101-104)cleanRoles(117-120)skip(175-185)
tests/e2e/Adapter/Scopes/RelationshipTests.php (3)
src/Database/Database.php (2)
getDatabase(837-840)getAuthorization(930-933)tests/e2e/Adapter/Base.php (1)
getDatabase(42-42)src/Database/Validator/Authorization.php (2)
cleanRoles(117-120)addRole(91-94)
tests/e2e/Adapter/Scopes/Relationships/ManyToOneTests.php (1)
tests/e2e/Adapter/Base.php (1)
getDatabase(42-42)
tests/e2e/Adapter/Scopes/PermissionTests.php (6)
src/Database/Database.php (2)
getDatabase(837-840)getAuthorization(930-933)tests/e2e/Adapter/PostgresTest.php (1)
getDatabase(21-52)tests/e2e/Adapter/Base.php (1)
getDatabase(42-42)tests/e2e/Adapter/MySQLTest.php (1)
getDatabase(27-59)tests/e2e/Adapter/SQLiteTest.php (1)
getDatabase(21-56)src/Database/Validator/Authorization.php (5)
disable(202-205)reset(212-215)cleanRoles(117-120)addRole(91-94)skip(175-185)
src/Database/Database.php (6)
src/Database/Validator/Authorization.php (4)
Authorization(8-240)isValid(52-85)getDescription(42-45)skip(175-185)src/Database/Validator/Authorization/Input.php (1)
Input(5-49)src/Database/Adapter/Pool.php (1)
setAuthorization(578-582)src/Database/Mirror.php (1)
setAuthorization(1103-1116)src/Database/Adapter.php (1)
setAuthorization(61-66)src/Database/Document.php (1)
getPermissionsByType(145-157)
tests/e2e/Adapter/Scopes/CollectionTests.php (4)
src/Database/Database.php (2)
getDatabase(837-840)getAuthorization(930-933)tests/e2e/Adapter/PostgresTest.php (1)
getDatabase(21-52)tests/e2e/Adapter/Base.php (1)
getDatabase(42-42)src/Database/Validator/Authorization.php (2)
skip(175-185)addRole(91-94)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (SharedTables/SQLite)
- GitHub Check: Adapter Tests (SharedTables/Postgres)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Adapter Tests (Postgres)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (MariaDB)
- GitHub Check: Adapter Tests (MySQL)
🔇 Additional comments (11)
tests/e2e/Adapter/Scopes/CollectionTests.php (4)
25-25: LGTM! Database retrieval correctly migrated from static to instance method.The consistent replacement of
static::getDatabase()with$this->getDatabase()throughout all test methods aligns with the PR's refactoring objective to move from static global authorization to per-instance objects.Also applies to: 44-44, 79-79, 311-311, 349-349, 362-362, 454-454, 495-495, 537-537, 596-596, 652-652, 726-726, 761-761, 781-781, 793-793, 900-900, 929-929, 945-945, 983-983, 1063-1063, 1247-1247, 1267-1267, 1462-1462, 1487-1487, 1506-1506, 1533-1533
403-407: LGTM! Authorization skip pattern correctly migrated.The change from
Authorization::skip(...)to$this->getDatabase()->getAuthorization()->skip(...)properly delegates authorization control to the database instance, maintaining the same callback-based skip logic.
919-919: LGTM! Role addition correctly uses instance-based API.The migration from
Authorization::setRole(...)to$this->getDatabase()->getAuthorization()->addRole(Role::label('reader')->toString())correctly:
- Obtains the authorization instance from the database
- Calls the new
addRole()API (replacing the oldsetRole())- Passes the role string obtained from
Role::label('reader')->toString()
1331-1332: LGTM! Nested authorization skip and database retrieval correctly refactored.The authorization skip wrapper now correctly uses the instance-based API, and the database is retrieved within the callback using the instance method. The nesting and scoping are appropriate.
tests/e2e/Adapter/Scopes/RelationshipTests.php (2)
32-32: LGTM! Consistent migration to instance-based database access.The refactoring from
static::getDatabase()to$this->getDatabase()is consistent throughout the test methods and aligns with the PR's objective of moving to instance-based architecture.Also applies to: 406-406, 478-478, 644-644, 1100-1100, 1134-1134, 1234-1234, 1293-1293, 1368-1368, 1662-1662, 1741-1741, 1900-1900, 1921-1921, 1944-1944, 1975-1975, 2000-2000, 2018-2018, 2050-2050, 2072-2072, 2100-2100, 2202-2202, 2356-2356, 2662-2662, 2882-2882, 3017-3017, 3506-3506, 3673-3673, 3927-3927, 4036-4036, 4141-4141, 4242-4242, 4498-4498, 4659-4659
1747-1748: LGTM! Proper migration to instance-based Authorization API.The changes correctly refactor authorization state management:
Authorization::cleanRoles()→$this->getDatabase()->getAuthorization()->cleanRoles()Authorization::setRole(...)→$this->getDatabase()->getAuthorization()->addRole(...)The API method name change from
setRole()toaddRole()better reflects the additive nature of role assignment, and routing through the database instance properly scopes authorization to the test instance.Also applies to: 1825-1825, 1838-1838, 2248-2249
src/Database/Database.php (1)
918-923: Nice instance-level authorization hook.Moving the adapter wiring into
setAuthorization()keeps the Database and Adapter in sync without relying on globals, which makes swapping authorization contexts per request much cleaner.tests/e2e/Adapter/Scopes/PermissionTests.php (1)
19-19: LGTM! Authorization refactoring is consistent.The migration from static to instance-based authorization is systematic and correct throughout this test file. Database access and authorization state management now properly route through per-database instances.
Also applies to: 157-159, 173-173, 206-207, 210-210, 230-230, 236-236, 244-244, 303-303, 322-346, 358-360, 389-389, 404-406, 427-427, 457-458, 461-461, 478-479, 482-482, 498-499, 503-503, 522-523, 526-526, 550-551, 556-556, 572-573, 576-576, 587-587, 603-604, 609-609, 623-624, 627-627, 652-653, 656-656, 675-676, 679-679, 697-697, 777-778, 781-781, 789-790, 798-799, 816-817, 821-821, 841-842, 847-847, 863-864, 867-867, 919-920, 923-923, 939-940, 943-943, 958-960, 974-975, 993-994, 997-997, 1016-1017, 1020-1020, 1033-1034, 1058-1059, 1064-1064, 1082-1083, 1086-1086, 1096-1097, 1118-1119, 1123-1123, 1141-1142, 1145-1145, 1164-1164, 1173-1174, 1239-1239, 1248-1248, 1255-1256
tests/e2e/Adapter/Base.php (1)
34-37: LGTM! Test base properly supports instance-based authorization.The Authorization instance is correctly initialized at the test base level and propagated to database instances. The lazy initialization pattern in
setUp()ensures the instance is reused across test methods.Also applies to: 42-42, 50-50, 58-58, 62-66, 71-71
tests/e2e/Adapter/Scopes/Relationships/ManyToOneTests.php (1)
20-20: LGTM! Database access properly uses instance method.These changes correctly migrate from static to instance-based database access, consistent with the broader authorization refactor.
Also applies to: 354-354, 822-822, 923-923, 1034-1034, 1136-1136, 1207-1207, 1290-1290, 1334-1334, 1400-1400, 1466-1466, 1533-1533, 1601-1601, 1684-1684, 1766-1766
tests/e2e/Adapter/Scopes/Relationships/OneToManyTests.php (1)
20-20: LGTM! Database access properly uses instance method.These changes correctly migrate from static to instance-based database access, consistent with the broader authorization refactor.
Also applies to: 392-392, 843-843, 1002-1002, 1134-1134, 1227-1227, 1322-1322, 1436-1436, 1528-1528, 1571-1571, 1637-1637, 1705-1705, 1773-1773, 1839-1839, 2022-2022, 2079-2079, 2161-2161
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/Database/Adapter/Pool.php (1)
70-70: Uninitialized property access remains unresolved.The critical issue previously identified still exists:
$this->authorizationis accessed without being initialized in the Pool constructor. If any method that callsdelegate()is invoked beforesetAuthorization()is called on the Pool instance, it will cause an uninitialized property error.Required fix: Initialize the authorization property before it's accessed. Options include:
- Require an
Authorizationparameter in Pool's constructor and either callparent::__construct($authorization)or$this->setAuthorization($authorization)- Make the property nullable and add a null check:
if (isset($this->authorization)) { $adapter->setAuthorization($this->authorization); }- Ensure all Pool instantiation sites call
setAuthorization()immediately after construction (fragile and error-prone)Option 1 or 2 is recommended for robustness.
🧹 Nitpick comments (1)
src/Database/Adapter/Pool.php (1)
579-583: Consider calling parent method for consistency.The implementation is functionally correct, but for consistency with the
Mirrorclass pattern (which callsparent::setAuthorization()before additional propagation), consider refactoring to call the parent method.public function setAuthorization(Authorization $authorization): self { - $this->authorization = $authorization; + parent::setAuthorization($authorization); return $this; }This makes the inheritance chain explicit and ensures any future logic added to
Adapter::setAuthorization()will be automatically inherited.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Database/Adapter/Pool.php(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/Database/Adapter/Pool.php (4)
src/Database/Database.php (2)
Database(38-8296)setAuthorization(918-923)src/Database/Validator/Authorization.php (1)
Authorization(8-240)src/Database/Mirror.php (1)
setAuthorization(1103-1116)src/Database/Adapter.php (1)
setAuthorization(61-66)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Adapter Tests (Postgres)
- GitHub Check: Adapter Tests (SharedTables/SQLite)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (SharedTables/Postgres)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (MariaDB)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Adapter Tests (MySQL)
- GitHub Check: Unit Test
🔇 Additional comments (1)
src/Database/Adapter/Pool.php (1)
9-9: LGTM!The import is necessary for the new instance-based authorization functionality.
…rization-instance # Conflicts: # src/Database/Adapter/Pool.php # tests/e2e/Adapter/Scopes/AttributeTests.php # tests/e2e/Adapter/Scopes/DocumentTests.php # tests/e2e/Adapter/Scopes/GeneralTests.php # tests/e2e/Adapter/Scopes/IndexTests.php
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/Database/Database.php (1)
7239-7261: Fix sum(): use instance Authorization + Input; remove static call and null‑coalesceCurrent code instantiates Authorization with a ctor arg, calls static skip, and uses
?? false. This violates the new instance model and breaks CI.Apply:
- $authorization = new Authorization(self::PERMISSION_READ); - if ($authorization->isValid($collection->getRead())) { - $skipAuth = true; - } + $skipAuth = $this->authorization->isValid(new Input(self::PERMISSION_READ, $collection->getRead())); @@ - $getSum = fn () => $this->adapter->sum($collection, $attribute, $queries, $max); - $sum = $skipAuth ?? false ? Authorization::skip($getSum) : $getSum(); + $getSum = fn () => $this->adapter->sum($collection, $attribute, $queries, $max); + $sum = $skipAuth ? $this->authorization->skip($getSum) : $getSum();This aligns with find()/count() and resolves the CodeQL error.
tests/e2e/Adapter/Scopes/DocumentTests.php (1)
1899-1929: Fix method name casing:getSupportForFulltextWildcardIndex()requires lowercase 'c'.The canonical method name across all adapters is
getSupportForFulltextWildcardIndex(). Two calls in the test file incorrectly use uppercase 'C' (getSupportForFulltextWildCardIndex()), which will cause fatal errors at runtime.Location 1 (line 1921):
- if ($this->getDatabase()->getAdapter()->getSupportForFulltextWildCardIndex()) { + if ($this->getDatabase()->getAdapter()->getSupportForFulltextWildcardIndex()) {Location 2 (line 3409):
- if ($this->getDatabase()->getAdapter()->getSupportForFulltextWildCardIndex()) { + if ($this->getDatabase()->getAdapter()->getSupportForFulltextWildcardIndex()) {
♻️ Duplicate comments (2)
src/Database/Database.php (1)
4927-4930: Update path auth is correct; prefer spread over array_merge (prior feedback)
- Using
authorization->skip(...)to fetch the old doc for update is right.- Minor: build permission lists with array spread for consistency with the rest of the file.
Example:
- $updatePermissions = array_merge( - $collection->getUpdate(), - $documentSecurity ? $old->getUpdate() : [] - ); - $readPermissions = array_merge( - $collection->getRead(), - $documentSecurity ? $old->getRead() : [] - ); + $updatePermissions = [ + ...$collection->getUpdate(), + ...($documentSecurity ? $old->getUpdate() : []), + ]; + $readPermissions = [ + ...$collection->getRead(), + ...($documentSecurity ? $old->getRead() : []), + ];Also applies to: 5053-5071
tests/e2e/Adapter/Scopes/IndexTests.php (1)
56-63: Verify adapter method overrides are non-static (same concern as Base).Avoid static implementations for
getDatabase/deleteIndex/deleteColumnin adapter tests; matches Base’s protected instance signatures.#!/bin/bash # Re-check in this scope specifically rg -nP '^\s*(public|protected)\s+static\s+function\s+(getDatabase|deleteColumn|deleteIndex)\b' tests/e2e/Adapter/Scopes -C1Also applies to: 331-339
🧹 Nitpick comments (5)
tests/e2e/Adapter/Base.php (2)
36-39: Docblock type is not nullable while property is.Adjust the annotation to match
?Authorization.- /** - * @var Authorization - */ + /** + * @var Authorization|null + */
62-69: Avoid auth state leakage between tests; reset roles and status.
tearDown()only resets status default; roles added during tests can persist. Normalize both roles and status per test for isolation.public function setUp(): void { - if (is_null(self::$authorization)) { - self::$authorization = new Authorization(); - } - - self::$authorization->addRole('any'); + if (is_null(self::$authorization)) { + self::$authorization = new Authorization(); + } + // Ensure a clean baseline + self::$authorization->setDefaultStatus(true); + self::$authorization->cleanRoles(); + self::$authorization->addRole('any'); } public function tearDown(): void { - self::$authorization->setDefaultStatus(true); + // Restore baseline for next test + self::$authorization->setDefaultStatus(true); + self::$authorization->cleanRoles(); + self::$authorization->addRole('any'); }Also applies to: 71-75
tests/e2e/Adapter/Scopes/DocumentTests.php (1)
618-631: Reduce boilerplate and guard against auth-state drift with small helpers.Introduce Base helpers to scope authorization changes, e.g.,
withAuthDisabled(callable)andwithRoles(array $roles, callable), to centralize disable/reset and role setup/restore.I can open a follow-up PR to add these helpers and refactor the tests.
Also applies to: 4491-4501, 4514-4531
tests/e2e/Adapter/Scopes/CollectionTests.php (1)
403-407: Consider caching the database instance.Within the
skipcallback,$this->getDatabase()is called multiple times. For better readability and to avoid redundant calls, consider assigning it to a local variable at the start of the callback.Apply this diff:
- $this->getDatabase()->getAuthorization()->skip(function () use ($loopCount) { + $database = $this->getDatabase(); + $database->getAuthorization()->skip(function () use ($database, $loopCount) { for ($i = 0; $i < $loopCount; $i++) { - $this->getDatabase()->deleteDocument('sizeTest2', 'doc' . $i); + $database->deleteDocument('sizeTest2', 'doc' . $i); } });tests/e2e/Adapter/Scopes/PermissionTests.php (1)
206-207: Consider caching the authorization instance for readability.Throughout the file, the authorization object is accessed repeatedly via
$this->getDatabase()->getAuthorization()for consecutive operations likecleanRoles()followed byaddRole(). While functionally correct, caching the authorization instance in a local variable would improve readability.Example refactor:
- $this->getDatabase()->getAuthorization()->cleanRoles(); - $this->getDatabase()->getAuthorization()->addRole(Role::any()->toString()); + $authorization = $this->getDatabase()->getAuthorization(); + $authorization->cleanRoles(); + $authorization->addRole(Role::any()->toString());Also applies to: 230-230, 236-236, 457-458, 478-479, 498-499, 522-523, 550-551, 572-573, 603-604, 623-624, 632-633, 652-653, 675-676
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/Database/Adapter.php(2 hunks)src/Database/Adapter/Pool.php(3 hunks)src/Database/Adapter/SQL.php(6 hunks)src/Database/Database.php(29 hunks)tests/e2e/Adapter/Base.php(1 hunks)tests/e2e/Adapter/Scopes/AttributeTests.php(41 hunks)tests/e2e/Adapter/Scopes/CollectionTests.php(29 hunks)tests/e2e/Adapter/Scopes/DocumentTests.php(126 hunks)tests/e2e/Adapter/Scopes/GeneralTests.php(9 hunks)tests/e2e/Adapter/Scopes/IndexTests.php(13 hunks)tests/e2e/Adapter/Scopes/PermissionTests.php(43 hunks)tests/e2e/Adapter/Scopes/SpatialTests.php(25 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- src/Database/Adapter/SQL.php
- src/Database/Adapter/Pool.php
- src/Database/Adapter.php
- tests/e2e/Adapter/Scopes/AttributeTests.php
- tests/e2e/Adapter/Scopes/SpatialTests.php
- tests/e2e/Adapter/Scopes/GeneralTests.php
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-03T02:04:17.803Z
Learnt from: abnegate
PR: utopia-php/database#721
File: tests/e2e/Adapter/Scopes/DocumentTests.php:6418-6439
Timestamp: 2025-10-03T02:04:17.803Z
Learning: In tests/e2e/Adapter/Scopes/DocumentTests::testSchemalessDocumentInvalidInteralAttributeValidation (PHP), when the adapter reports getSupportForAttributes() === false (schemaless), the test should not expect exceptions from createDocuments for “invalid” internal attributes; remove try/catch and ensure the test passes without exceptions, keeping at least one assertion.
Applied to files:
tests/e2e/Adapter/Scopes/DocumentTests.php
🧬 Code graph analysis (6)
tests/e2e/Adapter/Scopes/DocumentTests.php (5)
src/Database/Database.php (4)
getDatabase(837-840)getAuthorization(930-933)getAdapter(1282-1285)Database(38-8376)tests/e2e/Adapter/Base.php (1)
getDatabase(44-44)tests/e2e/Adapter/MongoDBTest.php (1)
getDatabase(32-66)tests/e2e/Adapter/SharedTables/MongoDBTest.php (1)
getDatabase(33-68)src/Database/Validator/Authorization.php (6)
disable(202-205)reset(212-215)addRole(91-94)removeRole(101-104)cleanRoles(117-120)skip(175-185)
tests/e2e/Adapter/Scopes/IndexTests.php (2)
tests/e2e/Adapter/Base.php (2)
getDatabase(44-44)deleteIndex(60-60)tests/e2e/Adapter/MongoDBTest.php (2)
getDatabase(32-66)deleteIndex(105-108)
tests/e2e/Adapter/Base.php (7)
src/Database/Validator/Authorization.php (3)
Authorization(8-240)addRole(91-94)setDefaultStatus(139-143)src/Database/Database.php (3)
getDatabase(837-840)Database(38-8376)deleteIndex(3593-3618)tests/e2e/Adapter/MySQLTest.php (3)
getDatabase(27-59)deleteColumn(61-69)deleteIndex(71-79)tests/e2e/Adapter/PostgresTest.php (3)
getDatabase(21-52)deleteColumn(54-62)deleteIndex(64-73)tests/e2e/Adapter/PoolTest.php (2)
getDatabase(35-76)deleteColumn(78-93)src/Database/Adapter.php (2)
getDatabase(178-181)deleteIndex(698-698)tests/e2e/Adapter/SQLiteTest.php (3)
getDatabase(21-56)deleteColumn(58-66)deleteIndex(68-76)
src/Database/Database.php (6)
src/Database/Validator/Authorization.php (4)
Authorization(8-240)isValid(52-85)getDescription(42-45)skip(175-185)src/Database/Validator/Authorization/Input.php (1)
Input(5-49)src/Database/Adapter/Pool.php (6)
setAuthorization(615-619)updateDocument(240-243)deleteDocument(255-258)getDocument(225-228)upsertDocuments(250-253)count(275-278)src/Database/Mirror.php (5)
setAuthorization(1103-1116)silent(172-175)updateDocument(658-704)deleteDocument(845-886)upsertDocuments(774-843)src/Database/Adapter.php (7)
setAuthorization(61-66)updateDocument(743-743)deleteDocument(791-791)getTenant(234-237)getDocument(709-709)upsertDocuments(770-774)count(843-843)src/Database/Document.php (9)
getRead(101-104)getCreate(109-112)getUpdate(117-120)getId(63-66)getCollection(85-88)getTenant(178-187)isEmpty(396-399)getDelete(125-128)getPermissionsByType(145-157)
tests/e2e/Adapter/Scopes/CollectionTests.php (3)
src/Database/Database.php (2)
getDatabase(837-840)getAuthorization(930-933)tests/e2e/Adapter/Base.php (1)
getDatabase(44-44)src/Database/Validator/Authorization.php (2)
skip(175-185)addRole(91-94)
tests/e2e/Adapter/Scopes/PermissionTests.php (4)
src/Database/Database.php (2)
getDatabase(837-840)getAuthorization(930-933)tests/e2e/Adapter/Base.php (1)
getDatabase(44-44)tests/e2e/Adapter/MySQLTest.php (1)
getDatabase(27-59)src/Database/Validator/Authorization.php (5)
disable(202-205)reset(212-215)cleanRoles(117-120)addRole(91-94)skip(175-185)
🪛 GitHub Actions: CodeQL
src/Database/Database.php
[error] 7239-7260: Class Utopia\Database\Validator\Authorization does not have a constructor and must be instantiated without any parameters; static call to Utopia\Database\Validator\Authorization::skip().
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Setup & Build Docker Image
🔇 Additional comments (15)
src/Database/Database.php (10)
414-419: Authorization instance wiring looks correctPrivate property, constructor init via setAuthorization(new Authorization()), and adapter propagation via setAuthorization()/getAuthorization() are consistent.
Also applies to: 434-435, 912-934
3691-3696: Read checks on cached and fresh document pathsUsing
$this->authorization->isValid(new Input(...))with collection and (optional) document permissions is correct. Returns empty Document on denial.Also applies to: 3719-3726
4386-4389: Create permission checks migrated to instance modelCollection-level create checks via Input before create/createDocuments are correct.
Also applies to: 4496-4499
5169-5173: Batch update permission gating and per-batch checks are solidPre-check for collection update when doc security is off; then
find(... forPermission: update)and per-doc merging with$skipPermissionsUpdateare correct.Also applies to: 5258-5263, 5311-5316
5839-5847: Upsert paths: correct use of skip() for reads/writes and per-case create/update checksAuth checks for create vs update with documentSecurity considered; wrapping adapter upsert with authorization->skip is appropriate.
Also applies to: 5880-5887, 5983-5987
6091-6104: Atomic increase/decrease: correct skip-for-read and update permission checksFetching with forUpdate under skip(), then validating update permission against collection/doc is correct.
Also applies to: 6191-6204
6256-6272: Delete: skip-for-read on lock + delete permission check is correctPattern mirrors update; good.
6435-6463: Relationship cleanup: using authorization->skip to avoid requiring user read/update during cascading opsGood defensive use of skip() for internal maintenance writes.
Also applies to: 6495-6520, 6528-6535, 6553-6561
6948-6953: find(): proper Input-based skipAuth and instance skip() usageMatches the new model; no null‑coalescing.
Also applies to: 7034-7035
7178-7197: count(): fixed ternary and instance skip()Matches prior guidance to remove
?? falseand static calls.tests/e2e/Adapter/Scopes/IndexTests.php (2)
59-60: LGTM: migrate to instance-based DB accessor.Using
$this->getDatabase()in traits is correct and aligns with per-instance authorization propagation.
100-104: Pre-dropping index via adapter hook is appropriate.Calling
$this->deleteIndex(...)before$database->deleteIndex(...)correctly validates idempotent deletion behavior.tests/e2e/Adapter/Scopes/CollectionTests.php (1)
25-25: LGTM! Migration to instance-based database access.The systematic replacement of
static::getDatabase()with$this->getDatabase()is correct and aligns with the PR's objective of moving from static to instance-based access patterns.Also applies to: 44-44, 79-79, 311-311, 349-349, 362-362, 454-454, 495-495, 537-537, 596-596, 652-652, 726-726, 761-761, 781-781, 793-793, 900-900, 929-929, 945-945, 983-983, 1063-1063, 1247-1247, 1267-1267, 1332-1332, 1462-1462, 1487-1487, 1506-1506, 1533-1533
tests/e2e/Adapter/Scopes/PermissionTests.php (2)
19-19: LGTM! Consistent migration to instance-based database access.The replacement of
static::getDatabase()with$this->getDatabase()throughout the file is correct and aligns with the instance-based authorization architecture.Also applies to: 173-173, 210-210, 244-244, 303-303, 358-358, 427-427, 461-461, 482-482, 503-503, 526-526, 556-556, 576-576, 587-587, 609-609, 627-627, 656-656, 679-679, 697-697, 781-781, 821-821, 847-847, 867-867, 923-923, 943-943, 1002-1002, 1025-1025, 1074-1074, 1096-1096, 1133-1133, 1155-1155, 1174-1174, 1184-1184, 1258-1258
322-346: LGTM! Well-implemented authorization skip callbacks.The
skipcallback implementations correctly pass the necessary variables through theuseclause and properly utilize instance-based authorization. The mix of traditional closures and arrow functions is appropriate for their respective use cases.Also applies to: 360-362, 404-406, 1249-1249
… Mongo classes, improving consistency and readability. Update DocumentTests to remove unnecessary whitespace.
…rization-instance
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
tests/e2e/Adapter/Scopes/DocumentTests.php (3)
115-116: Fix assertion attribute: use bigint_unsigned.Second assertion’s type check targets bigint_signed again. It should assert on bigint_unsigned before comparing its value.
Apply this diff:
- $this->assertIsInt($document->getAttribute('bigint_signed')); + $this->assertIsInt($document->getAttribute('bigint_unsigned'));
464-475: Initialize $hash to avoid notices.$hash is first used via indexed assignment. Initialize to an empty array to prevent E_NOTICE in stricter environments.
- $offset = 1000000; + $offset = 1000000; + $hash = [];
1921-1927: Method name mismatch: Fulltext wildcard capability check.Inconsistent method names: getSupportForFulltextWildCardIndex vs getSupportForFulltextWildcardIndex. Use one valid name across the file to avoid fatal errors.
- if ($this->getDatabase()->getAdapter()->getSupportForFulltextWildCardIndex()) { + if ($this->getDatabase()->getAdapter()->getSupportForFulltextWildcardIndex()) {Also confirm the same spelling is used consistently elsewhere.
Also applies to: 1970-1974
tests/e2e/Adapter/Scopes/SchemalessTests.php (1)
119-125: Schemaless invalid internal attributes: don’t expect exceptionsWhen
getSupportForAttributes() === false, schemaless adapters shouldn’t throw on “invalid” internal attributes during bulk create. Remove try/catch and keep at least one assertion to ensure the call returns without error.Apply:
- try { - $docs = [ - new Document(['$id' => true, 'freeA' => 'doc1']), - new Document(['$id' => true, 'freeB' => 'test']), - new Document(['$id' => true]), - ]; - $database->createDocuments($colName, $docs); - } catch (\Throwable $e) { - $this->assertInstanceOf(StructureException::class, $e); - } - - try { - $docs = [ - new Document(['$createdAt' => true, 'freeA' => 'doc1']), - new Document(['$updatedAt' => true, 'freeB' => 'test']), - new Document(['$permissions' => 12]), - ]; - $database->createDocuments($colName, $docs); - } catch (\Throwable $e) { - $this->assertInstanceOf(StructureException::class, $e); - } + $docs = [ + new Document(['$id' => true, 'freeA' => 'doc1']), + new Document(['$id' => true, 'freeB' => 'test']), + new Document(['$id' => true]), + ]; + $database->createDocuments($colName, $docs); + + $docs = [ + new Document(['$createdAt' => true, 'freeA' => 'doc1']), + new Document(['$updatedAt' => true, 'freeB' => 'test']), + new Document(['$permissions' => 12]), + ]; + $database->createDocuments($colName, $docs); + $this->assertTrue(true); // ensure at least one assertionBased on learnings
Also applies to: 129-149
src/Database/Adapter/Mongo.php (1)
1709-1715: Min/max checks clobber and skip zero; combine and use null-checksUsing truthy checks skips 0 and second assignment overwrites the first.
- if ($max) { - $filters[$attribute] = ['$lte' => $max]; - } - - if ($min) { - $filters[$attribute] = ['$gte' => $min]; - } + if ($max !== null) { + $filters[$attribute]['$lte'] = $max; + } + if ($min !== null) { + $filters[$attribute]['$gte'] = $min; + }tests/e2e/Adapter/Schemaless/MongoDBTest.php (1)
32-36: Fix stale static call to getDatabase()
getDatabase()is now an instance method; calling it statically will error on PHP 8+.- $this->assertNotNull(static::getDatabase()->create()); + $this->assertNotNull($this->getDatabase()->create());Also applies to: 75-80
src/Database/Database.php (1)
2529-2533: Method casing nit: use setAttribute (capital “A”)One occurrence uses setattribute which can trip static analysis/IDE tooling.
Apply this diff:
- ->setattribute('key', $newKey ?? $id) + ->setAttribute('key', $newKey ?? $id)
🧹 Nitpick comments (14)
tests/e2e/Adapter/Scopes/DocumentTests.php (4)
216-236: Schemaless adapters: don’t expect exceptions; assert success instead.When getSupportForAttributes() is false, this test should not expect an exception. Branch explicitly and keep an assertion in the schemaless path.
- try { - $database->createDocument('documents', new Document([ - 'string' => '', - 'integer_signed' => 0, - 'integer_unsigned' => 0, - 'bigint_signed' => 0, - 'bigint_unsigned' => 0, - 'float_signed' => 0, - 'float_unsigned' => -5.55, - 'boolean' => true, - 'colors' => [], - 'empty' => [], - ])); - $this->fail('Failed to throw exception'); - } catch (Throwable $e) { - if ($database->getAdapter()->getSupportForAttributes()) { - $this->assertTrue($e instanceof StructureException); - $this->assertStringContainsString('Invalid document structure: Attribute "float_unsigned" has invalid type. Value must be a valid range between 0 and', $e->getMessage()); - } - } + if ($database->getAdapter()->getSupportForAttributes()) { + try { + $database->createDocument('documents', new Document([ + 'string' => '', + 'integer_signed' => 0, + 'integer_unsigned' => 0, + 'bigint_signed' => 0, + 'bigint_unsigned' => 0, + 'float_signed' => 0, + 'float_unsigned' => -5.55, + 'boolean' => true, + 'colors' => [], + 'empty' => [], + ])); + $this->fail('Failed to throw exception'); + } catch (Throwable $e) { + $this->assertInstanceOf(StructureException::class, $e); + $this->assertStringContainsString('Invalid document structure: Attribute "float_unsigned" has invalid type. Value must be a valid range between 0 and', $e->getMessage()); + } + } else { + $doc = $database->createDocument('documents', new Document([ + 'string' => '', + 'integer_signed' => 0, + 'integer_unsigned' => 0, + 'bigint_signed' => 0, + 'bigint_unsigned' => 0, + 'float_signed' => 0, + 'float_unsigned' => -5.55, + 'boolean' => true, + 'colors' => [], + 'empty' => [], + ])); + $this->assertNotEmpty($doc->getId()); + }Based on learnings.
237-256: Schemaless adapters: don’t expect exceptions; assert success instead.Same branching pattern should be applied here for bigint_unsigned negative value.
- try { - $database->createDocument('documents', new Document([ - 'string' => '', - 'integer_signed' => 0, - 'integer_unsigned' => 0, - 'bigint_signed' => 0, - 'bigint_unsigned' => -10, - 'float_signed' => 0, - 'float_unsigned' => 0, - 'boolean' => true, - 'colors' => [], - 'empty' => [], - ])); - $this->fail('Failed to throw exception'); - } catch (Throwable $e) { - if ($database->getAdapter()->getSupportForAttributes()) { - $this->assertTrue($e instanceof StructureException); - $this->assertEquals('Invalid document structure: Attribute "bigint_unsigned" has invalid type. Value must be a valid range between 0 and 9,223,372,036,854,775,807', $e->getMessage()); - } - } + if ($database->getAdapter()->getSupportForAttributes()) { + try { + $database->createDocument('documents', new Document([ + 'string' => '', + 'integer_signed' => 0, + 'integer_unsigned' => 0, + 'bigint_signed' => 0, + 'bigint_unsigned' => -10, + 'float_signed' => 0, + 'float_unsigned' => 0, + 'boolean' => true, + 'colors' => [], + 'empty' => [], + ])); + $this->fail('Failed to throw exception'); + } catch (Throwable $e) { + $this->assertInstanceOf(StructureException::class, $e); + $this->assertEquals('Invalid document structure: Attribute "bigint_unsigned" has invalid type. Value must be a valid range between 0 and 9,223,372,036,854,775,807', $e->getMessage()); + } + } else { + $doc = $database->createDocument('documents', new Document([ + 'string' => '', + 'integer_signed' => 0, + 'integer_unsigned' => 0, + 'bigint_signed' => 0, + 'bigint_unsigned' => -10, + 'float_signed' => 0, + 'float_unsigned' => 0, + 'boolean' => true, + 'colors' => [], + 'empty' => [], + ])); + $this->assertNotEmpty($doc->getId()); + }Based on learnings.
258-280: Schemaless adapters: don’t expect exceptions; assert success instead.Same for invalid $sequence value when attributes aren’t enforced.
- try { - $database->createDocument('documents', new Document([ - '$sequence' => '0', - '$permissions' => [], - 'string' => '', - 'integer_signed' => 1, - 'integer_unsigned' => 1, - 'bigint_signed' => 1, - 'bigint_unsigned' => 1, - 'float_signed' => 1, - 'float_unsigned' => 1, - 'boolean' => true, - 'colors' => [], - 'empty' => [], - 'with-dash' => '', - ])); - $this->fail('Failed to throw exception'); - } catch (Throwable $e) { - if ($database->getAdapter()->getSupportForAttributes()) { - $this->assertTrue($e instanceof StructureException); - $this->assertEquals('Invalid document structure: Attribute "$sequence" has invalid type. Invalid sequence value', $e->getMessage()); - } - } + if ($database->getAdapter()->getSupportForAttributes())) { + try { + $database->createDocument('documents', new Document([ + '$sequence' => '0', + '$permissions' => [], + 'string' => '', + 'integer_signed' => 1, + 'integer_unsigned' => 1, + 'bigint_signed' => 1, + 'bigint_unsigned' => 1, + 'float_signed' => 1, + 'float_unsigned' => 1, + 'boolean' => true, + 'colors' => [], + 'empty' => [], + 'with-dash' => '', + ])); + $this->fail('Failed to throw exception'); + } catch (Throwable $e) { + $this->assertInstanceOf(StructureException::class, $e); + $this->assertEquals('Invalid document structure: Attribute "$sequence" has invalid type. Invalid sequence value', $e->getMessage()); + } + } else { + $doc = $database->createDocument('documents', new Document([ + '$sequence' => '0', + '$permissions' => [], + 'string' => '', + 'integer_signed' => 1, + 'integer_unsigned' => 1, + 'bigint_signed' => 1, + 'bigint_unsigned' => 1, + 'float_signed' => 1, + 'float_unsigned' => 1, + 'boolean' => true, + 'colors' => [], + 'empty' => [], + 'with-dash' => '', + ])); + $this->assertNotEmpty($doc->getId()); + }Based on learnings.
309-311: Consistent class casing for Query.Class names are case-insensitive in PHP, but using consistent casing improves readability.
- query::isNull('id') + Query::isNull('id')- query::equal('id', [$sequence]) + Query::equal('id', [$sequence])Also applies to: 347-349
src/Database/Adapter/Mongo.php (2)
2106-2134: Preserve maxTimeMS for count aggregation
$optionsare overwritten, dropping timeout. Wrap existing options withgetTransactionOptions($options).- $options = $this->getTransactionOptions(); + $options = $this->getTransactionOptions($options);
2235-2237: Add aggregation timeout in sumParity with find/count; include
maxTimeMSbefore transaction options.- $options = $this->getTransactionOptions(); + $options = []; + if ($this->timeout) { + $options['maxTimeMS'] = $this->timeout; + } + $options = $this->getTransactionOptions($options);tests/e2e/Adapter/SharedTables/MongoDBTest.php (1)
103-111: Unused parameters in stubsTiny nit: add
unset($collection, $column);andunset($collection, $index);to silence analyzers, or suppress via annotations.tests/e2e/Adapter/Schemaless/MongoDBTest.php (1)
102-110: Unused parameters in stubsSame nit as other Mongo tests: consider
unset(...)in stub methods to quiet static analysis.tests/unit/Validator/AuthorizationTest.php (3)
45-47: Use assertTrue/assertFalse and expected-first order for clarity.Current assertions pass but reverse the expected/actual order and compare to booleans via assertEquals. Prefer assertTrue/assertFalse and assertSame for strings to improve failure messages and strictness.
- $this->assertEquals($object->isValid(new Input(Database::PERMISSION_READ, $document->getRead())), false); - $this->assertEquals($object->isValid(new Input(Database::PERMISSION_READ, [])), false); - $this->assertEquals($object->getDescription(), 'No permissions provided for action \'read\''); + $this->assertFalse($object->isValid(new Input(Database::PERMISSION_READ, $document->getRead()))); + $this->assertFalse($object->isValid(new Input(Database::PERMISSION_READ, []))); + $this->assertSame('No permissions provided for action \'read\'', $object->getDescription()); - $this->assertEquals($this->authorization->hasRole(Role::user('456')->toString()), true); - $this->assertEquals($this->authorization->hasRole(Role::user('457')->toString()), false); - $this->assertEquals($this->authorization->hasRole(''), false); - $this->assertEquals($this->authorization->hasRole(Role::any()->toString()), true); + $this->assertTrue($this->authorization->hasRole(Role::user('456')->toString())); + $this->assertFalse($this->authorization->hasRole(Role::user('457')->toString())); + $this->assertFalse($this->authorization->hasRole('')); + $this->assertTrue($this->authorization->hasRole(Role::any()->toString())); - $this->assertEquals($object->isValid(new Input(Database::PERMISSION_READ, $document->getRead())), true); + $this->assertTrue($object->isValid(new Input(Database::PERMISSION_READ, $document->getRead()))); - $this->assertEquals($object->isValid(new Input(Database::PERMISSION_READ, $document->getRead())), false); + $this->assertFalse($object->isValid(new Input(Database::PERMISSION_READ, $document->getRead()))); - $this->assertEquals($object->isValid(new Input(Database::PERMISSION_READ, $document->getRead())), true); + $this->assertTrue($object->isValid(new Input(Database::PERMISSION_READ, $document->getRead()))); - $this->assertEquals($object->isValid(new Input(Database::PERMISSION_READ, $document->getRead())), true); + $this->assertTrue($object->isValid(new Input(Database::PERMISSION_READ, $document->getRead()))); - $this->assertEquals($object->isValid(new Input(Database::PERMISSION_READ, $document->getRead())), false); + $this->assertFalse($object->isValid(new Input(Database::PERMISSION_READ, $document->getRead()))); - $this->assertEquals($object->isValid(new Input(Database::PERMISSION_READ, $document->getRead())), true); + $this->assertTrue($object->isValid(new Input(Database::PERMISSION_READ, $document->getRead()))); - $this->assertEquals($object->isValid(new Input(Database::PERMISSION_READ, $document->getRead())), true); + $this->assertTrue($object->isValid(new Input(Database::PERMISSION_READ, $document->getRead()))); - $this->assertEquals($object->isValid(new Input(Database::PERMISSION_READ, $document->getRead())), false); + $this->assertFalse($object->isValid(new Input(Database::PERMISSION_READ, $document->getRead()))); - $this->assertEquals($object->isValid(new Input(Database::PERMISSION_READ, $document->getRead())), false); + $this->assertFalse($object->isValid(new Input(Database::PERMISSION_READ, $document->getRead()))); - $this->assertEquals($this->authorization->skip(function () use ($object, $document) { - return $object->isValid(new Input(Database::PERMISSION_READ, $document->getRead())); - }), true); + $this->assertTrue($this->authorization->skip(function () use ($object, $document) { + return $object->isValid(new Input(Database::PERMISSION_READ, $document->getRead())); + })); - $this->assertEquals(true, $this->authorization->getStatus()); + $this->assertTrue($this->authorization->getStatus()); - $this->assertEquals(false, $this->authorization->getStatus()); + $this->assertFalse($this->authorization->getStatus()); - $this->assertEquals(false, $this->authorization->getStatus()); + $this->assertFalse($this->authorization->getStatus()); - $this->authorization->skip(function () { - $this->assertEquals(false, $this->authorization->getStatus()); - }); + $this->authorization->skip(function () { + $this->assertFalse($this->authorization->getStatus()); + }); - $this->assertEquals(false, $this->authorization->getStatus()); + $this->assertFalse($this->authorization->getStatus()); - $this->assertEquals(false, $this->authorization->getStatus()); + $this->assertFalse($this->authorization->getStatus()); - $this->assertEquals(true, $this->authorization->getStatus()); + $this->assertTrue($this->authorization->getStatus());Also applies to: 52-57, 57-57, 61-61, 65-65, 70-70, 74-74, 79-79, 83-83, 87-87, 98-101, 106-106, 109-116, 118-118, 121-121, 124-124
29-29: Redundant addRole(any).Authorization initializes with role 'any' by default, so this call is unnecessary. Either remove it or assert the default explicitly.
- $this->authorization->addRole(Role::any()->toString()); + // Default includes 'any'; no need to add explicitly.
43-44: Inline the $object alias.Using $this->authorization directly reduces noise in assertions.
- $object = $this->authorization; + // Use $this->authorization directly for readability.tests/e2e/Adapter/MongoDBTest.php (1)
101-104: Silence PHPMD UnusedFormalParameter in no-op adapter helpers.Parameters are required by the abstract signature but unused here. Rename with underscores and add a suppress annotation.
- protected function deleteColumn(string $collection, string $column): bool + /** + * @SuppressWarnings(PHPMD.UnusedFormalParameter) + */ + protected function deleteColumn(string $_collection, string $_column): bool { return true; } - protected function deleteIndex(string $collection, string $index): bool + /** + * @SuppressWarnings(PHPMD.UnusedFormalParameter) + */ + protected function deleteIndex(string $_collection, string $_index): bool { return true; }Also applies to: 106-109
src/Database/Database.php (1)
3691-3696: DRY the READ permission check in getDocumentSame permission array and isValid logic is duplicated for cache hit and DB path. Extract a tiny private helper (e.g., canReadDocument(Document $collection, Document $doc): bool) and invoke it in both spots.
Also applies to: 3721-3725
src/Database/Validator/Authorization.php (1)
71-81: Consider improving the error message clarity.Line 79 uses
$permissionwhich is the last permission from the loop. If multiple permissions are checked and none match, the error message saysMissing "action" permission for role "role3"(showing only the last one), which could be misleading during debugging.While the full permissions array is shown in the "was given" part, consider either:
- Removing the
for role "$permission"part since the full array is already displayed- Initializing
$permissionto a more meaningful valueExample refactor:
- $permission = '-'; - foreach ($permissions as $permission) { if (\array_key_exists($permission, $this->roles)) { return true; } } - $this->message = 'Missing "'.$action.'" permission for role "'.$permission.'". Only "'.\json_encode($this->getRoles()).'" scopes are allowed and "'.\json_encode($permissions).'" was given.'; + $this->message = 'Missing "'.$action.'" permission. Only "'.\json_encode($this->getRoles()).'" scopes are allowed and "'.\json_encode($permissions).'" was given.';
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
composer.json(1 hunks)src/Database/Adapter.php(2 hunks)src/Database/Adapter/Mongo.php(3 hunks)src/Database/Adapter/Pool.php(4 hunks)src/Database/Database.php(31 hunks)src/Database/Validator/Authorization.php(4 hunks)src/Database/Validator/Authorization/Input.php(1 hunks)tests/e2e/Adapter/MongoDBTest.php(4 hunks)tests/e2e/Adapter/Schemaless/MongoDBTest.php(4 hunks)tests/e2e/Adapter/Scopes/DocumentTests.php(126 hunks)tests/e2e/Adapter/Scopes/GeneralTests.php(9 hunks)tests/e2e/Adapter/Scopes/SchemalessTests.php(19 hunks)tests/e2e/Adapter/SharedTables/MongoDBTest.php(4 hunks)tests/unit/Validator/AuthorizationTest.php(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/Database/Adapter/Pool.php
- src/Database/Validator/Authorization/Input.php
- tests/e2e/Adapter/Scopes/GeneralTests.php
- src/Database/Adapter.php
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-03T02:04:17.803Z
Learnt from: abnegate
PR: utopia-php/database#721
File: tests/e2e/Adapter/Scopes/DocumentTests.php:6418-6439
Timestamp: 2025-10-03T02:04:17.803Z
Learning: In tests/e2e/Adapter/Scopes/DocumentTests::testSchemalessDocumentInvalidInteralAttributeValidation (PHP), when the adapter reports getSupportForAttributes() === false (schemaless), the test should not expect exceptions from createDocuments for “invalid” internal attributes; remove try/catch and ensure the test passes without exceptions, keeping at least one assertion.
Applied to files:
tests/e2e/Adapter/Scopes/SchemalessTests.phptests/e2e/Adapter/Scopes/DocumentTests.php
🧬 Code graph analysis (9)
tests/e2e/Adapter/Schemaless/MongoDBTest.php (3)
src/Database/Adapter.php (6)
getDatabase(182-185)setAuthorization(61-66)delete(537-537)create(510-510)setDatabase(167-172)deleteIndex(702-702)src/Database/Database.php (7)
getDatabase(838-841)Database(38-8379)setAuthorization(919-924)delete(1383-1397)create(1325-1344)setDatabase(823-828)deleteIndex(3594-3619)tests/e2e/Adapter/MongoDBTest.php (3)
getDatabase(32-67)deleteColumn(101-104)deleteIndex(106-109)
tests/e2e/Adapter/SharedTables/MongoDBTest.php (3)
src/Database/Database.php (7)
getDatabase(838-841)Database(38-8379)setAuthorization(919-924)create(1325-1344)delete(1383-1397)setDatabase(823-828)deleteIndex(3594-3619)tests/e2e/Adapter/MongoDBTest.php (3)
getDatabase(32-67)deleteColumn(101-104)deleteIndex(106-109)tests/e2e/Adapter/Base.php (3)
getDatabase(44-44)deleteColumn(52-52)deleteIndex(60-60)
tests/e2e/Adapter/Scopes/SchemalessTests.php (4)
tests/e2e/Adapter/Schemaless/MongoDBTest.php (1)
getDatabase(32-68)src/Database/Database.php (2)
getDatabase(838-841)getAuthorization(931-934)tests/e2e/Adapter/Base.php (1)
getDatabase(44-44)src/Database/Validator/Authorization.php (3)
cleanRoles(113-116)addRole(87-90)skip(171-181)
tests/e2e/Adapter/Scopes/DocumentTests.php (4)
tests/e2e/Adapter/SharedTables/MongoDBTest.php (1)
getDatabase(33-69)src/Database/Database.php (3)
getDatabase(838-841)getAuthorization(931-934)Database(38-8379)tests/e2e/Adapter/Base.php (1)
getDatabase(44-44)src/Database/Validator/Authorization.php (6)
disable(198-201)reset(208-211)addRole(87-90)removeRole(97-100)cleanRoles(113-116)skip(171-181)
src/Database/Database.php (3)
src/Database/Validator/Authorization.php (4)
Authorization(8-236)isValid(52-81)getDescription(42-45)skip(171-181)src/Database/Validator/Authorization/Input.php (1)
Input(5-49)src/Database/Adapter.php (2)
setAuthorization(61-66)getAuthorization(68-71)
tests/e2e/Adapter/MongoDBTest.php (5)
tests/e2e/Adapter/SharedTables/MongoDBTest.php (3)
getDatabase(33-69)deleteColumn(103-106)deleteIndex(108-111)tests/e2e/Adapter/Schemaless/MongoDBTest.php (2)
getDatabase(32-68)deleteColumn(102-105)src/Database/Database.php (7)
getDatabase(838-841)Database(38-8379)setAuthorization(919-924)create(1325-1344)delete(1383-1397)setDatabase(823-828)deleteIndex(3594-3619)tests/e2e/Adapter/MySQLTest.php (3)
getDatabase(27-59)deleteColumn(61-69)deleteIndex(71-79)tests/e2e/Adapter/Base.php (3)
getDatabase(44-44)deleteColumn(52-52)deleteIndex(60-60)
src/Database/Adapter/Mongo.php (1)
src/Database/Validator/Authorization.php (2)
getStatus(157-160)getRoles(105-108)
src/Database/Validator/Authorization.php (2)
src/Database/Database.php (1)
Database(38-8379)src/Database/Validator/Authorization/Input.php (3)
Input(5-49)getPermissions(40-43)getAction(45-48)
tests/unit/Validator/AuthorizationTest.php (2)
src/Database/Validator/Authorization.php (14)
Authorization(8-236)addRole(87-90)isValid(52-81)getDescription(42-45)hasRole(123-126)cleanRoles(113-116)disable(198-201)reset(208-211)setDefaultStatus(135-139)enable(188-191)getRoles(105-108)removeRole(97-100)skip(171-181)getStatus(157-160)src/Database/Validator/Authorization/Input.php (1)
Input(5-49)
🪛 PHPMD (2.15.0)
tests/e2e/Adapter/Schemaless/MongoDBTest.php
102-102: Avoid unused parameters such as '$collection'. (undefined)
(UnusedFormalParameter)
102-102: Avoid unused parameters such as '$column'. (undefined)
(UnusedFormalParameter)
107-107: Avoid unused parameters such as '$collection'. (undefined)
(UnusedFormalParameter)
107-107: Avoid unused parameters such as '$index'. (undefined)
(UnusedFormalParameter)
tests/e2e/Adapter/SharedTables/MongoDBTest.php
103-103: Avoid unused parameters such as '$collection'. (undefined)
(UnusedFormalParameter)
103-103: Avoid unused parameters such as '$column'. (undefined)
(UnusedFormalParameter)
108-108: Avoid unused parameters such as '$collection'. (undefined)
(UnusedFormalParameter)
108-108: Avoid unused parameters such as '$index'. (undefined)
(UnusedFormalParameter)
tests/e2e/Adapter/MongoDBTest.php
101-101: Avoid unused parameters such as '$collection'. (undefined)
(UnusedFormalParameter)
101-101: Avoid unused parameters such as '$column'. (undefined)
(UnusedFormalParameter)
106-106: Avoid unused parameters such as '$collection'. (undefined)
(UnusedFormalParameter)
106-106: Avoid unused parameters such as '$index'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (20)
tests/e2e/Adapter/Scopes/DocumentTests.php (2)
619-631: Instance-based authorization usage looks consistent.disable/reset/skip/addRole/removeRole now flow through $this->getDatabase()->getAuthorization(), matching the instance-based design.
Also applies to: 3897-3910, 4491-4500, 4514-4531, 4992-4995
5467-5471: Permission::write() exists and is properly supported—no changes needed.Verification confirms that
Permission::write()is a fully supported aggregate permission in the database layer. It is explicitly documented as an "Aggregate permission" (Database.php line 119), validated in the permission system (Permissions.php:25 and Permission.php:93), and tested successfully. The method exists, is honored by database permission checks, and all usages at the cited lines are correct.tests/e2e/Adapter/Scopes/SchemalessTests.php (2)
22-22: Instance access: good migrationSwitch to
$this->getDatabase()aligns tests with instance-based authorization propagation.
779-790: Authorization calls migrated correctlyUse of
$database->getAuthorization()->cleanRoles()/addRole()/skip()matches the new instance API and restores state viaskip()finally-block.
Please confirm no tests rely on global Authorization state anymore (e.g., static role leakage across tests).Also applies to: 798-806, 811-823, 838-839, 931-933
tests/e2e/Adapter/SharedTables/MongoDBTest.php (1)
33-69: Instance migration looks correct
getDatabase()is now an instance method and used consistently in tests.composer.json (1)
43-44: Manual test execution and lockfile update neededThe dependency change from
0.11.0to0.11.*is compatible with no breaking changes in utopia-php/mongo 0.11.0 (only a readConcern bug fix). Composer resolution shows no conflicts. However, verify by:
- Running
composer update utopia-php/mongoto update composer.lock- Executing the test suite to ensure MongoDB adapter tests pass
tests/unit/Validator/AuthorizationTest.php (1)
12-12: Good shift to instance-based Authorization and Input wrapper.Import and property initialization align with the new API and improve test isolation.
Also applies to: 16-21
tests/e2e/Adapter/MongoDBTest.php (1)
32-32: Instance-based database and per-instance Authorization: LGTM.Visibility widening on getDatabase() and propagating Authorization via setAuthorization() matches the refactor across adapters and tests.
Also applies to: 56-56, 72-79
src/Database/Database.php (3)
415-420: Per‑instance Authorization: injection and propagation look correctPrivate Authorization property, default initialization in the constructor, and Adapter propagation via setAuthorization/getAuthorization are consistent. Good foundation for instance‑scoped auth across Database/Adapter.
Also applies to: 435-436, 913-935
5548-5553: Good use of Authorization::skip for relationship side‑effectsWrapping cross‑document updates/deletes under skip avoids unnecessary READ checks while preserving outer UPDATE/DELETE authorization gates. LGTM.
Also applies to: 5676-5677
6101-6106: Increase/Decrease permission checks are correctUPDATE checks incorporate collection+document ACLs; read bypass for row locking is done via skip before auth gate. Solid.
Also applies to: 6201-6206
src/Database/Validator/Authorization.php (9)
5-5: LGTM!The import is necessary for the new Input DTO-based validation approach.
26-28: Verify the security implications of the default 'any' role.The default role 'any' => true means authorization checks will pass for any permission that includes 'any'. Ensure this is the intended behavior and that it aligns with your security model.
If this is intentional (e.g., for public resources or backward compatibility), consider adding a comment explaining why 'any' is included by default to prevent confusion.
52-57: LGTM!The type check is correct and necessary given the mixed signature. The instanceof check properly validates the input type before proceeding with validation logic.
59-64: LGTM!The extraction of permissions and action from the Input object is clean, and the status check correctly short-circuits validation when authorization is disabled.
66-69: LGTM!The empty permissions check is correct, and the error message appropriately includes the action for better debugging context.
87-126: LGTM!The role management methods are correctly implemented as instance methods, and the method renames (addRole, removeRole, hasRole) improve semantic clarity.
135-139: Verify the intended behavior of setDefaultStatus.The method sets both
statusDefault(line 137) and the currentstatus(line 138). This means callingsetDefaultStatus(false)immediately disables authorization, not just changing whatreset()will restore to.If this is intentional, consider updating the docblock to clarify that this method affects both the default status and the current status immediately.
Suggested docblock update:
/** - * Change default status. - * This will be used for the - * value set on the $this->reset() method + * Change default status and current status. + * Sets both the current authorization status and the default + * status that will be restored when reset() is called. + * * @param bool $status * @return void */
147-160: LGTM!The status getter and setter methods are straightforward and correctly implemented.
171-211: LGTM!The status control methods are correctly implemented. The
skip()method properly uses try/finally to ensure the status is always restored, even if the callback throws an exception.
…r authorization initialization in Pool adapter
…rization-instance # Conflicts: # src/Database/Adapter/Pool.php # tests/e2e/Adapter/Scopes/DocumentTests.php
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
tests/e2e/Adapter/Scopes/SpatialTests.php (1)
2813-2813: Inconsistent database access pattern.This line still uses
static::getDatabase()while all other test methods in this file were updated to use$this->getDatabase(). This inconsistency should be corrected to align with the PR's migration to instance-based database access.Apply this diff to fix the inconsistency:
- $database = static::getDatabase(); + $database = $this->getDatabase();src/Database/Database.php (1)
745-747: Critical: Static call to Authorization::skip — use instance methodThis triggers PHPStan/CodeQL error and breaks the new instance API.
Apply:
- $refetched = Authorization::skip(fn () => $this->silent( + $refetched = $this->authorization->skip(fn () => $this->silent( fn () => $this->find($collection->getId(), [Query::equal('$id', $docIds)]) ));tests/e2e/Adapter/Schemaless/MongoDBTest.php (1)
74-81: Fix non-static call used statically.
getDatabase()is an instance method; callingstatic::getDatabase()will fail. Use$this->getDatabase()consistently.- $this->assertNotNull(static::getDatabase()->create()); + $this->assertNotNull($this->getDatabase()->create());tests/e2e/Adapter/Scopes/DocumentTests.php (1)
1922-1929: Fix method name case mismatch:getSupportForFulltextWildCardIndex()→getSupportForFulltextWildcardIndex()The adapter API uses lowercase
cinWildcard, but the test file calls it with uppercaseCinWildCard. This causes a runtime "Call to undefined method" error.Incorrect usages found:
- Line 1922
- Line 3410
Both must change from
getSupportForFulltextWildCardIndex()togetSupportForFulltextWildcardIndex().- if ($this->getDatabase()->getAdapter()->getSupportForFulltextWildCardIndex()) { + if ($this->getDatabase()->getAdapter()->getSupportForFulltextWildcardIndex()) {
♻️ Duplicate comments (5)
src/Database/Database.php (3)
7314-7333: count(): also guard skip() with documentSecurityWithout this, aggregates can leak/inflate results when doc‑level ACLs are enabled.
- $skipAuth = $this->authorization->isValid(new Input(self::PERMISSION_READ, $collection->getRead())); + $documentSecurity = $collection->getAttribute('documentSecurity', false); + $skipAuth = $this->authorization->isValid(new Input(self::PERMISSION_READ, $collection->getRead())); @@ - $count = $skipAuth ? $this->authorization->skip($getCount) : $getCount(); + $count = ($skipAuth && !$documentSecurity) ? $this->authorization->skip($getCount) : $getCount();Based on learnings
7376-7395: sum(): also guard skip() with documentSecurityMirror the fix from find()/count() to keep doc‑level filters in place.
- $skipAuth = $this->authorization->isValid(new Input(self::PERMISSION_READ, $collection->getRead())); + $documentSecurity = $collection->getAttribute('documentSecurity', false); + $skipAuth = $this->authorization->isValid(new Input(self::PERMISSION_READ, $collection->getRead())); @@ - $sum = $skipAuth ? $this->authorization->skip($getSum) : $getSum(); + $sum = ($skipAuth && !$documentSecurity) ? $this->authorization->skip($getSum) : $getSum();Based on learnings
7169-7170: Don’t bypass doc‑level ACLs in find() when documentSecurity=trueGuard skip() with !$documentSecurity so adapter filters remain active for documentSecurity collections. Previously flagged.
- $results = $skipAuth ? $this->authorization->skip($getResults) : $getResults(); + // Only skip all auth when document-level security is disabled + $results = ($skipAuth && !$documentSecurity) ? $this->authorization->skip($getResults) : $getResults();#!/bin/bash # Verify no unsafe skips remain in find/count/sum rg -nP -C2 '\$results\s*=\s*\$skipAuth\s*\?' src/Database/Database.php rg -nP -C2 '\$count\s*=\s*\$skipAuth\s*\?' src/Database/Database.php rg -nP -C2 '\$sum\s*=\s*\$skipAuth\s*\?' src/Database/Database.php # Audit any lingering static calls rg -n 'Authorization::skip\s*\('Based on learnings
tests/e2e/Adapter/Scopes/DocumentTests.php (1)
3946-3947: Nit resolved: role string corrected to 'user:x'.Previous review flagged
userx; current code usesremoveRole('user:x'). Looks good.src/Database/Adapter/Mongo.php (1)
1871-1873: Critical: Authorization bypass via empty roles and unescaped regex.This security issue was previously flagged. When authorization is enabled but
getRoles()returns an empty array, the regex pattern becomes{$forPermission}\(".*(?:).*"\)which matches any permission string, effectively bypassing authorization. Additionally, role identifiers are not escaped before regex interpolation, allowing potential regex injection if roles contain special characters.Apply the fix previously suggested:
- if ($this->authorization->getStatus()) { - $roles = \implode('|', $this->authorization->getRoles()); - $filters['_permissions']['$in'] = [new Regex("{$forPermission}\\(\".*(?:{$roles}).*\"\\)", 'i')]; - } + if ($this->authorization->getStatus()) { + $roleList = $this->authorization->getRoles(); + if (!empty($roleList)) { + $escaped = \array_map(static fn ($r) => \preg_quote($r, '/'), $roleList); + $pattern = \sprintf('%s\(".*(?:%s).*"\)', $forPermission, \implode('|', $escaped)); + $filters['_permissions']['$in'] = [new Regex($pattern, 'i')]; + } else { + // No roles => no access + $filters['_uid'] = ['$in' => []]; + } + }Apply similar fixes at lines 2118-2120 and 2208-2210.
Also applies to: 2118-2120, 2208-2210
🧹 Nitpick comments (4)
src/Database/Database.php (1)
2564-2576: Nit: normalize method casing to setAttributeKeep API usage consistent; PHP allows case‑insensitivity, but consistency helps readability.
- ->setattribute('key', $newKey ?? $id) + ->setAttribute('key', $newKey ?? $id)tests/e2e/Adapter/Scopes/DocumentTests.php (2)
2176-2181: Prefer assertCount over assertEmpty(count(...)) for clarity.Replace
assertEmpty(count($documents))withassertCount(0, $documents)here and similarly elsewhere for zero-count checks.- $this->assertEmpty(count($documents)); + $this->assertCount(0, $documents);
3938-3941: Remove duplicate sum assertion.The
sum('price', [year=2019])assertion is repeated twice back-to-back with identical expectations. Drop one to reduce noise.- $sum = $database->sum('movies', 'price', [Query::equal('year', [2019]),]); - $this->assertEquals(round(39.50 + 25.99, 2), round($sum, 2));tests/e2e/Adapter/Schemaless/MongoDBTest.php (1)
103-111: Silence unused parameter warnings in stubbed trait methods.Parameters are intentionally unused in these adapter-specific stubs. Prefix with underscores to satisfy PHPMD without changing behavior.
-protected function deleteColumn(string $collection, string $column): bool +protected function deleteColumn(string $_collection, string $_column): bool { return true; } -protected function deleteIndex(string $collection, string $index): bool +protected function deleteIndex(string $_collection, string $_index): bool { return true; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/Database/Adapter.php(2 hunks)src/Database/Adapter/Mongo.php(3 hunks)src/Database/Adapter/Pool.php(4 hunks)src/Database/Adapter/SQL.php(6 hunks)src/Database/Database.php(31 hunks)tests/e2e/Adapter/Base.php(1 hunks)tests/e2e/Adapter/Schemaless/MongoDBTest.php(4 hunks)tests/e2e/Adapter/Scopes/DocumentTests.php(121 hunks)tests/e2e/Adapter/Scopes/SpatialTests.php(25 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/Database/Adapter/Pool.php
- src/Database/Adapter.php
- tests/e2e/Adapter/Base.php
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-10-03T02:04:17.803Z
Learnt from: abnegate
Repo: utopia-php/database PR: 721
File: tests/e2e/Adapter/Scopes/DocumentTests.php:6418-6439
Timestamp: 2025-10-03T02:04:17.803Z
Learning: In tests/e2e/Adapter/Scopes/DocumentTests::testSchemalessDocumentInvalidInteralAttributeValidation (PHP), when the adapter reports getSupportForAttributes() === false (schemaless), the test should not expect exceptions from createDocuments for “invalid” internal attributes; remove try/catch and ensure the test passes without exceptions, keeping at least one assertion.
Applied to files:
tests/e2e/Adapter/Scopes/DocumentTests.phptests/e2e/Adapter/Scopes/SpatialTests.phpsrc/Database/Database.phptests/e2e/Adapter/Schemaless/MongoDBTest.php
📚 Learning: 2025-10-29T12:27:57.071Z
Learnt from: ArnabChatterjee20k
Repo: utopia-php/database PR: 747
File: src/Database/Adapter/Mongo.php:1449-1453
Timestamp: 2025-10-29T12:27:57.071Z
Learning: In src/Database/Adapter/Mongo.php, when getSupportForAttributes() returns false (schemaless mode), the updateDocument method intentionally uses a raw document without $set operator for replacement-style updates, as confirmed by the repository maintainer ArnabChatterjee20k.
Applied to files:
tests/e2e/Adapter/Scopes/DocumentTests.phpsrc/Database/Adapter/Mongo.phptests/e2e/Adapter/Schemaless/MongoDBTest.php
📚 Learning: 2025-10-16T09:37:33.531Z
Learnt from: fogelito
Repo: utopia-php/database PR: 733
File: src/Database/Adapter/MariaDB.php:1801-1806
Timestamp: 2025-10-16T09:37:33.531Z
Learning: In the MariaDB adapter (src/Database/Adapter/MariaDB.php), only duplicate `_uid` violations should throw `DuplicateException`. All other unique constraint violations, including `PRIMARY` key collisions on the internal `_id` field, should throw `UniqueException`. This is the intended design to distinguish between user-facing document duplicates and internal/user-defined unique constraint violations.
Applied to files:
tests/e2e/Adapter/Scopes/DocumentTests.php
📚 Learning: 2025-10-20T05:29:29.487Z
Learnt from: abnegate
Repo: utopia-php/database PR: 731
File: src/Database/Database.php:6987-6988
Timestamp: 2025-10-20T05:29:29.487Z
Learning: In Utopia\Database\Database::convertRelationshipQueries, Many-to-Many filtering does not need the parent collection or a direct junction query: it calls find() on the related collection without skipping relationships, which populates relationship attributes (including the two-way key). Parent IDs are derived from that populated attribute. Therefore, calls should remain convertRelationshipQueries($relationships, $queries).
Applied to files:
tests/e2e/Adapter/Scopes/SpatialTests.phpsrc/Database/Database.php
🧬 Code graph analysis (6)
tests/e2e/Adapter/Scopes/DocumentTests.php (3)
src/Database/Database.php (3)
getDatabase(874-877)getAuthorization(967-970)Database(38-8521)tests/e2e/Adapter/Base.php (1)
getDatabase(46-46)src/Database/Validator/Authorization.php (6)
disable(198-201)reset(208-211)addRole(87-90)removeRole(97-100)cleanRoles(113-116)skip(171-181)
tests/e2e/Adapter/Scopes/SpatialTests.php (2)
tests/e2e/Adapter/Base.php (1)
getDatabase(46-46)tests/e2e/Adapter/MariaDBTest.php (1)
getDatabase(21-53)
src/Database/Adapter/SQL.php (1)
src/Database/Validator/Authorization.php (2)
getRoles(105-108)getStatus(157-160)
src/Database/Database.php (7)
src/Database/Validator/Authorization.php (4)
Authorization(8-236)isValid(52-81)getDescription(42-45)skip(171-181)src/Database/Validator/Authorization/Input.php (1)
Input(5-49)src/Database/Adapter/Pool.php (3)
setAuthorization(630-634)count(280-283)sum(275-278)src/Database/Adapter.php (4)
setAuthorization(61-66)getAuthorization(68-71)count(847-847)sum(836-836)src/Database/Mirror.php (1)
setAuthorization(1103-1116)src/Database/Adapter/Mongo.php (2)
count(2093-2180)sum(2195-2237)src/Database/Adapter/SQL.php (2)
count(3172-3234)sum(3247-3310)
src/Database/Adapter/Mongo.php (1)
src/Database/Validator/Authorization.php (2)
getStatus(157-160)getRoles(105-108)
tests/e2e/Adapter/Schemaless/MongoDBTest.php (3)
src/Database/Database.php (7)
getDatabase(874-877)Database(38-8521)setAuthorization(955-960)delete(1419-1433)create(1361-1380)setDatabase(859-864)deleteIndex(3630-3655)src/Database/Adapter.php (6)
getDatabase(182-185)setAuthorization(61-66)delete(537-537)create(510-510)setDatabase(167-172)deleteIndex(702-702)tests/e2e/Adapter/MongoDBTest.php (3)
getDatabase(32-67)deleteColumn(101-104)deleteIndex(106-109)
🪛 GitHub Actions: CodeQL
src/Database/Database.php
[error] 745-745: PHPStan: Static call to instance method Utopia\Database\Validator\Authorization::skip().
🪛 PHPMD (2.15.0)
tests/e2e/Adapter/Schemaless/MongoDBTest.php
103-103: Avoid unused parameters such as '$collection'. (undefined)
(UnusedFormalParameter)
103-103: Avoid unused parameters such as '$column'. (undefined)
(UnusedFormalParameter)
108-108: Avoid unused parameters such as '$collection'. (undefined)
(UnusedFormalParameter)
108-108: Avoid unused parameters such as '$index'. (undefined)
(UnusedFormalParameter)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Setup & Build Docker Image
🔇 Additional comments (2)
tests/e2e/Adapter/Scopes/SpatialTests.php (1)
21-21: LGTM! Consistent migration to instance-based database access.All test methods correctly updated from
static::getDatabase()to$this->getDatabase(), aligning with the PR's refactoring of the authorization system to instance-based patterns. The changes are uniform and maintain the existing test logic.Also applies to: 96-96, 249-249, 354-354, 402-402, 514-514, 619-619, 724-724, 921-921, 1352-1352, 1483-1483, 1786-1786, 1874-1874, 1961-1961, 2066-2066, 2172-2172, 2243-2243, 2375-2375, 2460-2460, 2502-2502, 2554-2554, 2589-2589, 2651-2651, 2683-2683, 2775-2775
src/Database/Adapter/SQL.php (1)
2960-2960: LGTM! Instance-based authorization properly implemented.The migration from static to instance-based authorization access is correct. The roles retrieved via
$this->authorization->getRoles()are safely used ingetSQLPermissionsCondition()where they're properly quoted viaPDO::quote()(line 1841), preventing SQL injection.Also applies to: 3057-3058, 3177-3177, 3195-3196, 3253-3253, 3271-3272
Summary by CodeRabbit
New Features
Refactor
Tests