added supportForAttributes in sql adapters #751
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughAdds a conditional gate for structure validation in Database::createDocuments and an end-to-end test that bypasses structure validation for attributes, then re-enables it to confirm validation behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant DB as Database
participant Validator as StructureValidator
rect rgb(248,250,255)
Note over DB: createDocuments — validation is now conditional on $this->validate
end
Caller->>DB: createDocuments(documents, ...)
alt $this->validate == true
DB->>Validator: new Structure(...)\nvalidate(document)
alt validation succeeds
Validator-->>DB: OK
DB->>DB: persist document
else validation fails
Validator-->>DB: throw StructureException
DB-->>Caller: StructureException
end
else $this->validate == false
rect rgb(240,255,240)
Note over DB: Skip Structure validation and persist documents
end
DB->>DB: persist document (no validation)
end
DB-->>Caller: result(s)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Database/Adapter/SQL.php(3 hunks)tests/e2e/Adapter/Scopes/DocumentTests.php(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
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.
📚 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.php
📚 Learning: 2025-07-30T19:17:53.630Z
Learnt from: ArnabChatterjee20k
Repo: utopia-php/database PR: 642
File: src/Database/Validator/PartialStructure.php:43-52
Timestamp: 2025-07-30T19:17:53.630Z
Learning: In PartialStructure validator, when filtering for required attributes validation using the $requiredAttributes parameter, $this->attributes should be used instead of the merged $attributes array because this validation is specifically for internal attributes like $createdAt and $updatedAt that are defined in the base Structure class, not collection-specific attributes.
Applied to files:
tests/e2e/Adapter/Scopes/DocumentTests.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.php
🧬 Code graph analysis (1)
tests/e2e/Adapter/Scopes/DocumentTests.php (6)
src/Database/Database.php (2)
Database(37-8505)createDocuments(4484-4587)src/Database/Adapter.php (2)
setSupportForAttributes(1406-1406)createDocuments(713-713)src/Database/Validator/Authorization.php (3)
Authorization(7-225)cleanRoles(109-112)setRole(83-86)src/Database/Adapter/Pool.php (2)
setSupportForAttributes(613-616)createDocuments(233-236)src/Database/Adapter/Mongo.php (2)
setSupportForAttributes(2650-2654)createDocuments(1343-1382)src/Database/Document.php (3)
Document(12-470)getAttributes(194-212)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). (1)
- GitHub Check: Setup & Build Docker Image
🔇 Additional comments (1)
src/Database/Adapter/SQL.php (1)
950-953: LGTM!The getter modification correctly returns the new property value while maintaining backward compatibility with the default
truevalue.
ArnabChatterjee20k
force-pushed
the
dat-851
branch
from
e2c6cde to
2e8576e
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/e2e/Adapter/Scopes/DocumentTests.php (1)
6188-6228: Exercise adapter flag instead of global validation; wrap in try/finally, assert insert count, and avoid$docsshadowing.This test is meant to verify “supportForAttributes” but currently toggles Database validation, not the adapter capability. It also risks leaking global state and reuses
$docsfor two different purposes. Switch to$adapter->setSupportForAttributes(false)to actually cover the feature, add a try/finally to restore the flag and cleanup, assert the insert count, and use distinct variable names.Apply:
@@ - // bypass required - $database->disableValidation(); - - $permissions = [Permission::read(Role::any()), Permission::write(Role::any()), Permission::update(Role::any()), Permission::delete(Role::any())]; - $docs = $database->createDocuments($collectionId, [ - new Document(['attrA' => null,'attrB' => 'B','$permissions' => $permissions]) - ]); - - $docs = $database->find($collectionId); - foreach ($docs as $doc) { - $this->assertArrayHasKey('attrA', $doc->getAttributes()); - $this->assertNull($doc->getAttribute('attrA')); - $this->assertEquals('B', $doc->getAttribute('attrB')); - } - // reset - $database->enableValidation(); - - try { - $database->createDocuments($collectionId, [ - new Document(['attrA' => null,'attrB' => 'B','$permissions' => $permissions]) - ]); - $this->fail('Failed to throw exception'); - } catch (Exception $e) { - $this->assertInstanceOf(StructureException::class, $e); - } - - $database->deleteCollection($collectionId); + $adapter = $database->getAdapter(); + $prevSupport = $adapter->getSupportForAttributes(); + $permissions = [Permission::read(Role::any()), Permission::write(Role::any()), Permission::update(Role::any()), Permission::delete(Role::any())]; + try { + // Bypass structure via adapter flag (schemaless mode) + $adapter->setSupportForAttributes(false); + + $insertCount = $database->createDocuments($collectionId, [ + new Document(['attrA' => null, 'attrB' => 'B', '$permissions' => $permissions]) + ]); + $this->assertEquals(1, $insertCount); + + $found = $database->find($collectionId); + $this->assertCount(1, $found); + foreach ($found as $doc) { + $this->assertArrayHasKey('attrA', $doc->getAttributes()); + $this->assertNull($doc->getAttribute('attrA')); + $this->assertEquals('B', $doc->getAttribute('attrB')); + } + + // Re-enable structure and expect validation failure + $adapter->setSupportForAttributes(true); + try { + $database->createDocuments($collectionId, [ + new Document(['attrA' => null, 'attrB' => 'B', '$permissions' => $permissions]) + ]); + $this->fail('Failed to throw exception'); + } catch (Exception $e) { + $this->assertInstanceOf(StructureException::class, $e); + } + } finally { + // Always restore adapter flag and cleanup + $adapter->setSupportForAttributes($prevSupport); + $database->deleteCollection($collectionId); + }Optionally, make the collection ID collision-proof:
- $collectionId = 'successive_update_single'; + $collectionId = __FUNCTION__;Based on learnings
🧹 Nitpick comments (1)
src/Database/Database.php (1)
4537-4547: Minor perf: reuse Structure validator inside the foreach.Instantiate Structure once before the loop when validation is enabled; reuse per document to cut allocations in large batches.
- foreach ($documents as $document) { + $structureValidator = null; + if ($this->validate) { + $structureValidator = new Structure( + $collection, + $this->adapter->getIdAttributeType(), + $this->adapter->getMinDateTime(), + $this->adapter->getMaxDateTime(), + $this->adapter->getSupportForAttributes() + ); + } + foreach ($documents as $document) { ... - if ($this->validate) { - $validator = new Structure( - ... - ); - if (!$validator->isValid($document)) { + if ($this->validate) { + if (!$structureValidator->isValid($document)) { throw new StructureException($validator->getDescription()); } }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Database/Database.php(1 hunks)tests/e2e/Adapter/Scopes/DocumentTests.php(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
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.
📚 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.phpsrc/Database/Database.php
📚 Learning: 2025-07-30T19:17:53.630Z
Learnt from: ArnabChatterjee20k
Repo: utopia-php/database PR: 642
File: src/Database/Validator/PartialStructure.php:43-52
Timestamp: 2025-07-30T19:17:53.630Z
Learning: In PartialStructure validator, when filtering for required attributes validation using the $requiredAttributes parameter, $this->attributes should be used instead of the merged $attributes array because this validation is specifically for internal attributes like $createdAt and $updatedAt that are defined in the base Structure class, not collection-specific attributes.
Applied to files:
tests/e2e/Adapter/Scopes/DocumentTests.phpsrc/Database/Database.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.php
🧬 Code graph analysis (2)
tests/e2e/Adapter/Scopes/DocumentTests.php (1)
src/Database/Database.php (5)
createAttribute(1784-1843)Database(37-8507)disableValidation(1064-1069)createDocuments(4484-4589)enableValidation(1052-1057)
src/Database/Database.php (5)
src/Database/Validator/Structure.php (3)
Structure(20-460)isValid(211-245)getDescription(197-200)src/Database/Adapter/SQL.php (2)
getIdAttributeType(2212-2215)getSupportForAttributes(949-952)src/Database/Adapter/Mongo.php (3)
getIdAttributeType(3104-3107)getMinDateTime(2589-2592)getSupportForAttributes(2645-2648)src/Database/Adapter/Pool.php (3)
getIdAttributeType(523-526)getMinDateTime(318-321)getSupportForAttributes(328-331)src/Database/Adapter.php (4)
getIdAttributeType(897-897)getMinDateTime(890-890)getMaxDateTime(904-907)getSupportForAttributes(921-921)
⏰ 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). (13)
- GitHub Check: Adapter Tests (SharedTables/MongoDB)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (Schemaless/MongoDB)
- GitHub Check: Adapter Tests (SharedTables/SQLite)
- GitHub Check: Adapter Tests (SharedTables/Postgres)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (MySQL)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Adapter Tests (MongoDB)
- GitHub Check: Adapter Tests (Postgres)
- GitHub Check: Adapter Tests (MariaDB)
🔇 Additional comments (1)
src/Database/Database.php (1)
4537-4547: Good: batch structure validation now respects disableValidation()/skipValidation.This lets migrations bypass Structure checks during createDocuments when desired.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/e2e/Adapter/Scopes/DocumentTests.php (1)
6204-6220: Avoid global validation leakage; use skipValidation and ensure cleanup; add count assertion and stop variable shadowing.disableValidation()/enableValidation() can leak across tests if an assertion throws. Use skipValidation() and a try/finally for collection cleanup; also assert the insert count and avoid reusing $docs for different types.
Apply this diff:
// bypass required - $database->disableValidation(); - - $permissions = [Permission::read(Role::any()), Permission::write(Role::any()), Permission::update(Role::any()), Permission::delete(Role::any())]; - $docs = $database->createDocuments($collectionId, [ - new Document(['attrA' => null,'attrB' => 'B','$permissions' => $permissions]) - ]); - - $docs = $database->find($collectionId); - foreach ($docs as $doc) { - $this->assertArrayHasKey('attrA', $doc->getAttributes()); - $this->assertNull($doc->getAttribute('attrA')); - $this->assertEquals('B', $doc->getAttribute('attrB')); - } - // reset - $database->enableValidation(); + $permissions = [Permission::read(Role::any()), Permission::write(Role::any()), Permission::update(Role::any()), Permission::delete(Role::any())]; + try { + // insert while bypassing validation, scoped + $count = $database->skipValidation(function () use ($database, $collectionId, $permissions) { + return $database->createDocuments($collectionId, [ + new Document(['attrA' => null, 'attrB' => 'B', '$permissions' => $permissions]) + ]); + }); + $this->assertEquals(1, $count); + + $documents = $database->find($collectionId); + foreach ($documents as $doc) { + $this->assertArrayHasKey('attrA', $doc->getAttributes()); + $this->assertNull($doc->getAttribute('attrA')); + $this->assertEquals('B', $doc->getAttribute('attrB')); + } - try { + // validation enabled again here – expect structure error $database->createDocuments($collectionId, [ new Document(['attrA' => null,'attrB' => 'B','$permissions' => $permissions]) ]); $this->fail('Failed to throw exception'); - } catch (Exception $e) { + } catch (\Throwable $e) { $this->assertInstanceOf(StructureException::class, $e); + } finally { + // Always cleanup even if assertions fail + $database->deleteCollection($collectionId); } - - $database->deleteCollection($collectionId);Also applies to: 6221-6231
🧹 Nitpick comments (1)
tests/e2e/Adapter/Scopes/DocumentTests.php (1)
6221-6228: Use Throwable or expectException for clarity.Catching Throwable (or using $this->expectException(StructureException::class)) avoids missing Errors and mirrors patterns used elsewhere in this file.
- } catch (Exception $e) { + } catch (\Throwable $e) { $this->assertInstanceOf(StructureException::class, $e); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/e2e/Adapter/Scopes/DocumentTests.php(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
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.
📚 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.php
📚 Learning: 2025-07-30T19:17:53.630Z
Learnt from: ArnabChatterjee20k
Repo: utopia-php/database PR: 642
File: src/Database/Validator/PartialStructure.php:43-52
Timestamp: 2025-07-30T19:17:53.630Z
Learning: In PartialStructure validator, when filtering for required attributes validation using the $requiredAttributes parameter, $this->attributes should be used instead of the merged $attributes array because this validation is specifically for internal attributes like $createdAt and $updatedAt that are defined in the base Structure class, not collection-specific attributes.
Applied to files:
tests/e2e/Adapter/Scopes/DocumentTests.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.php
🧬 Code graph analysis (1)
tests/e2e/Adapter/Scopes/DocumentTests.php (5)
src/Database/Adapter.php (7)
getDatabase(160-163)getSupportForAttributes(921-921)createCollection(525-525)createAttribute(557-557)delete(515-515)createDocuments(713-713)find(802-802)src/Database/Database.php (10)
getDatabase(865-868)getAdapter(1287-1290)createCollection(1416-1554)createAttribute(1784-1843)Database(37-8507)disableValidation(1064-1069)delete(1387-1401)createDocuments(4484-4589)find(7032-7172)enableValidation(1052-1057)tests/e2e/Adapter/Base.php (1)
getDatabase(42-42)tests/e2e/Adapter/MongoDBTest.php (1)
getDatabase(32-66)src/Database/Adapter/Mongo.php (6)
getSupportForAttributes(2645-2648)createCollection(404-569)createAttribute(665-668)delete(388-393)createDocuments(1343-1382)find(1859-2021)
⏰ 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)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Adapter Tests (Schemaless/MongoDB)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (Postgres)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Adapter Tests (SharedTables/SQLite)
- GitHub Check: Adapter Tests (SharedTables/Postgres)
- GitHub Check: Adapter Tests (MariaDB)
- GitHub Check: Adapter Tests (MongoDB)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (SharedTables/MongoDB)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (MySQL)
- GitHub Check: Unit Test
🔇 Additional comments (1)
tests/e2e/Adapter/Scopes/DocumentTests.php (1)
6193-6196: Good guard for schemaless adapters.Skipping when getSupportForAttributes() is false aligns with our prior schemaless expectations.
Based on learnings
abnegate
approved these changes
Nov 4, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Added supportForAttributes in sql adapters as well to bypass structure so that migration and data moving becomes easy
Summary by CodeRabbit
Tests
Improvements