Enhance permission handling in SQL adapter and add tests for document… #664
Conversation
… creation with varied attributes
WalkthroughConsolidates per-document permission parameter bindings inside Adapter/SQL::createDocuments by collecting all permission bind key/value pairs into a single map and binding them in one pass with explicit PDO types; adds an end-to-end test method verifying create/update visibility and permission counts for documents. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SQL_Adapter as Adapter/SQL::createDocuments
participant DB
Client->>SQL_Adapter: createDocuments(collection, documents[])
SQL_Adapter->>SQL_Adapter: Build document rows and permission SQL entries
SQL_Adapter->>SQL_Adapter: Collect $bindValuesPermissions (:_uid_X, :_tenant_X...)
SQL_Adapter->>DB: Execute batch INSERT documents (bound params)
SQL_Adapter->>DB: Execute batch INSERT name_perms (consolidated binds)
DB-->>SQL_Adapter: Insert results
SQL_Adapter-->>Client: Return created documents
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/Database/Adapter/SQL.php (2)
1900-1910: Bind permission text instead of embedding it in SQL to avoid manual quoting and align with other code pathsRight now
_permissionis interpolated directly into the SQL (after removing double quotes). While current role strings are constrained, binding them is safer, avoids SQL quoting edge-cases, and matches the binding style used elsewhere (e.g., in update/remove permission diffs).Suggested refactor (keeps your new binding map and extends it to include
_permission):- foreach ($document->getPermissionsByType($type) as $permission) { + foreach ($document->getPermissionsByType($type) as $permIdx => $permission) { $tenantBind = $this->sharedTables ? ", :_tenant_{$index}" : ''; - $permission = \str_replace('"', '', $permission); - $permission = "('{$type}', '{$permission}', :_uid_{$index} {$tenantBind})"; - $permissions[] = $permission; + $permBindKey = ":_perm_{$index}_{$type}_{$permIdx}"; + $permissions[] = "('{$type}', {$permBindKey}, :_uid_{$index} {$tenantBind})"; + $bindValuesPermissions[$permBindKey] = $permission; $bindValuesPermissions[":_uid_{$index}"] = $document->getId(); if ($this->sharedTables) { $bindValuesPermissions[":_tenant_{$index}"] = $document->getTenant(); } }This change:
- Eliminates manual quoting for
_permission- Preserves your fix for
_uid/_tenantbinding- Keeps column order and SQL shape intact
1936-1941: Consolidated binding with explicit PDO types — looks correctIterating
$bindValuesPermissionsand binding withgetPDOType($value)is consistent with the rest of the adapter and removes the prior mismatch between placeholders and bound values.If you want to reduce redundant work when a document has multiple permissions, you can avoid repeatedly setting the same
:_uid_{index}/:_tenant_{index}in the map by guarding withisset(...). It won’t change correctness, just trims a few array writes.tests/e2e/Adapter/Scopes/DocumentTests.php (1)
372-403: Solid E2E that reproduces the original issue; make Authorization explicit for determinismThe scenario (only the last doc has read permissions, and only that doc is returned post-insert) is a good end-to-end guard for the binding fix. To make the test resilient to global authorization state, set the role explicitly during the assertions:
$this->assertEquals(3, $database->createDocuments($colName, $docs)); - // we should get only one document as read permission provided to the last document only + // we should get only one document as read permission provided to the last document only + \Utopia\Database\Validator\Authorization::setRole(\Utopia\Database\Helpers\Role::any()->toString()); $addedDocs = $database->find($colName); $this->assertCount(1, $addedDocs); $doc = $addedDocs[0]; $this->assertEquals('doc3', $doc->getId()); $this->assertNotEmpty($doc->getPermissions()); $this->assertCount(3, $doc->getPermissions()); + \Utopia\Database\Validator\Authorization::cleanRoles();Minor note: using
Permission::write(Role::any())together withPermission::update(Role::any())intentionally exercises deduplication (expecting 3 unique permissions). Nice touch.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/Database/Adapter/SQL.php(3 hunks)tests/e2e/Adapter/Scopes/DocumentTests.php(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/Database/Adapter/SQL.php (3)
src/Database/Document.php (2)
getId(63-66)getTenant(178-187)src/Database/Adapter/MariaDB.php (1)
getPDOType(1835-1843)src/Database/Adapter/Postgres.php (1)
getPDOType(1954-1963)
🔇 Additional comments (1)
src/Database/Adapter/SQL.php (1)
1863-1868: Collected per-permission bind values to fix placeholder/bind mismatch — good changeInitializing and using
$bindValuesPermissionsensures every placeholder added to the VALUES list has a matching bound value. This addresses the reported "Invalid number of token" error in bulk permission inserts.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/e2e/Adapter/Scopes/DocumentTests.php (2)
5852-5860: Make Authorization explicit to avoid order-dependent flakinessThe visibility assertions rely on read("any") being active. Set Role::any explicitly to make the test self-contained and order-independent.
Apply this diff (can be placed right after the batch-ops guard):
/** @var Database $database */ $database = static::getDatabase(); + Authorization::cleanRoles(); + Authorization::setRole(Role::any()->toString());
5892-5904: Strengthen assertions: verify updated keys and that non-permitted docs remain unchangedAfter updateDocuments(), assert that:
- doc3 and doc4 have key updated to 'new doc' (they have update("any")).
- doc2 remains unchanged (no document-level update permission). Use Authorization::skip to fetch it.
Apply this diff after asserting the update count:
- $this->assertEquals(2, $database->updateDocuments($colName, new Document(['key' => 'new doc']))); + $this->assertEquals(2, $database->updateDocuments($colName, new Document(['key' => 'new doc']))); + + // Verify updated keys for docs with update permissions + $doc3After = $database->getDocument($colName, 'doc3'); + $doc4After = $database->getDocument($colName, 'doc4'); + $this->assertEquals('new doc', $doc3After->getAttribute('key')); + $this->assertEquals('new doc', $doc4After->getAttribute('key')); + + // Verify doc without update permission was not modified + $doc2 = Authorization::skip(function () use ($database, $colName) { + return $database->getDocument($colName, 'doc2'); + }); + $this->assertEquals('doc2', $doc2->getAttribute('key')); + $this->assertEquals('test', $doc2->getAttribute('value'));
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
tests/e2e/Adapter/Scopes/DocumentTests.php(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/e2e/Adapter/Scopes/DocumentTests.php (4)
tests/e2e/Adapter/Base.php (1)
getDatabase(34-34)src/Database/Adapter/SQLite.php (1)
createCollection(141-226)src/Database/Helpers/Role.php (2)
Role(5-178)any(159-162)src/Database/Document.php (4)
Document(12-470)getId(63-66)getPermissions(93-96)getAttribute(224-231)
⏰ 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/Postgres)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (SharedTables/SQLite)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Adapter Tests (Postgres)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Adapter Tests (MySQL)
- GitHub Check: Adapter Tests (MariaDB)
- GitHub Check: Adapter Tests (Pool)
| /** @var Database $database */ | ||
| $database = static::getDatabase(); | ||
|
|
||
| // with different set of attributes | ||
| $colName = "docs_with_diff"; | ||
| $database->createCollection($colName); | ||
| $database->createAttribute($colName, 'key', Database::VAR_STRING, 50, true); | ||
| $database->createAttribute($colName, 'value', Database::VAR_STRING, 50, false, 'value'); | ||
| $permissions = [Permission::read(Role::any()), Permission::write(Role::any()),Permission::update(Role::any())]; |
There was a problem hiding this comment.
Add adapter capability guard before using batch updates
This test uses updateDocuments(), which is not supported by all adapters. Other tests in this suite guard on getSupportForBatchOperations(). Add the same guard to avoid false negatives on adapters without batch support.
Apply this diff after obtaining $database:
/** @var Database $database */
$database = static::getDatabase();
+ if (!$database->getAdapter()->getSupportForBatchOperations()) {
+ $this->expectNotToPerformAssertions();
+ return;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** @var Database $database */ | |
| $database = static::getDatabase(); | |
| // with different set of attributes | |
| $colName = "docs_with_diff"; | |
| $database->createCollection($colName); | |
| $database->createAttribute($colName, 'key', Database::VAR_STRING, 50, true); | |
| $database->createAttribute($colName, 'value', Database::VAR_STRING, 50, false, 'value'); | |
| $permissions = [Permission::read(Role::any()), Permission::write(Role::any()),Permission::update(Role::any())]; | |
| /** @var Database $database */ | |
| $database = static::getDatabase(); | |
| if (!$database->getAdapter()->getSupportForBatchOperations()) { | |
| $this->expectNotToPerformAssertions(); | |
| return; | |
| } | |
| // with different set of attributes | |
| $colName = "docs_with_diff"; | |
| $database->createCollection($colName); | |
| $database->createAttribute($colName, 'key', Database::VAR_STRING, 50, true); | |
| $database->createAttribute($colName, 'value', Database::VAR_STRING, 50, false, 'value'); | |
| $permissions = [Permission::read(Role::any()), Permission::write(Role::any()),Permission::update(Role::any())]; |
🤖 Prompt for AI Agents
In tests/e2e/Adapter/Scopes/DocumentTests.php around lines 5852 to 5860, the
test calls updateDocuments() but does not guard for adapters that lack batch
update support; add a capability check right after obtaining $database: if
(!$this->getSupportForBatchOperations()) { $this->markTestSkipped('Adapter does
not support batch operations'); } so the test is skipped for adapters without
batch support, preventing false negatives.
Previous issue
Case => Permissions not provided in any of the documents in the createDocuments
Problem => Invalid number of token in the sql query
Reason =>
During binding of the value
we were doing
Here number of bindings => 5 document
Now added all the permissons in the sql statement but the value was going short
Solution
Storing the binding key, values together in a separate array during the permission iteration itself
Summary by CodeRabbit
Bug Fixes
Tests