Sync 3.x #754
Sync 3.x #754
Conversation
added supportForAttributes in sql adapters
Fix min/max not bound
WalkthroughThis PR parameterizes max/min constraints in increaseDocumentAttribute across MariaDB and Postgres adapters, fixes combined filter handling in the Mongo adapter, gates structural validation behind a validate flag in Database.php to allow bypassing validation checks, and adds comprehensive tests for validation toggle behavior. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 1
🧹 Nitpick comments (10)
src/Database/Adapter/Mongo.php (1)
1709-1716: Combined $min/$max filter is correct; consider normalizing attribute name.Change correctly applies both bounds without overwriting. To be defensive with internal aliases (e.g., $id → _uid, $sequence → _id), normalize the attribute before using it in filters and $inc.
Apply this small tweak near the attribute initialization:
- $attribute = $this->filter($attribute); + $attribute = $this->filter($this->getInternalKeyForAttribute($attribute));src/Database/Adapter/Postgres.php (1)
1436-1442: Good parameterization; bind numeric types explicitly for :max/:min.Using placeholders for bounds is solid. Bind with getPDOType to ensure correct numeric typing.
- if ($max !== null) { - $stmt->bindValue(':max', $max); - } + if ($max !== null) { + $stmt->bindValue(':max', $max, $this->getPDOType($max)); + } - if ($min !== null) { - $stmt->bindValue(':min', $min); - } + if ($min !== null) { + $stmt->bindValue(':min', $min, $this->getPDOType($min)); + }Also applies to: 1457-1462
src/Database/Adapter/MariaDB.php (1)
1315-1317: Parameterized guards look good; bind :max/:min with proper PDO types.Keep placeholders, but bind with getPDOType for numeric correctness and plan stability.
- if ($max !== null) { - $stmt->bindValue(':max', $max); - } + if ($max !== null) { + $stmt->bindValue(':max', $max, $this->getPDOType($max)); + } - if ($min !== null) { - $stmt->bindValue(':min', $min); - } + if ($min !== null) { + $stmt->bindValue(':min', $min, $this->getPDOType($min)); + }Also applies to: 1319-1321, 1336-1341
tests/e2e/Adapter/Scopes/DocumentTests.php (6)
6205-6220: Scope validation toggles with skipValidation() to prevent leakage.Replace disable/enable pairs with skipValidation closures so validation always restores even on exceptions.
Example for creation:
$database->skipValidation(function () use ($database, $collectionId, $permissions) { $count = $database->createDocuments($collectionId, [ new Document(['attrA' => null,'attrB' => 'B', '$permissions' => $permissions]) ]); $this->assertEquals(1, $count); });Example for update/upsert:
$database->skipValidation(function () use ($database, $collection) { $database->updateDocument($collection, 'valid', new Document(['age' => null])); });Also applies to: 6267-6276, 6296-6302, 6324-6330, 6349-6361
6208-6213: Avoid variable shadowing and assert createDocuments count.$docs is first an int then reused as array. Use a separate $count variable and assert it.
- $docs = $database->createDocuments($collectionId, [ - new Document(['attrA' => null,'attrB' => 'B','$permissions' => $permissions]) - ]); - - $docs = $database->find($collectionId); + $count = $database->createDocuments($collectionId, [ + new Document(['attrA' => null,'attrB' => 'B','$permissions' => $permissions]) + ]); + $this->assertEquals(1, $count); + + $docs = $database->find($collectionId); + $this->assertCount(1, $docs);
6192-6196: Clarify the schemaless comment.Comment says “for schemaless … skipped” but this branch runs only when attributes are supported. Reword to avoid confusion.
- // for schemaless the validation will be automatically skipped + // Skip when adapter is schemaless (no attribute support) — validation bypass there is implicit
6269-6275: Strengthen post-conditions after bypassed creation.Verify required fields are actually stored as null.
$doc = $database->createDocument($collection, new Document([ '$id' => 'created-null', '$permissions' => [Permission::read(Role::any()), Permission::create(Role::any()), Permission::update(Role::any())], 'name' => null, 'age' => null, ])); $this->assertEquals('created-null', $doc->getId()); + $fetched = $database->getDocument($collection, 'created-null'); + $this->assertNull($fetched->getAttribute('name')); + $this->assertNull($fetched->getAttribute('age'));
6324-6330: Validate batch update effect on at least one document.Add a quick read-back to assert “name” became null under disabled validation.
$database->disableValidation(); $count = $database->updateDocuments($collection, new Document([ 'name' => null, ])); $this->assertGreaterThanOrEqual(3, $count); // at least the seeded docs are updated $database->enableValidation(); + $one = $database->getDocument($collection, 'b0'); + $this->assertNull($one->getAttribute('name'));
6349-6361: Verify upsert-with-increase results.Assert the persisted “value” and required-null after bypass to catch adapter-specific behavior.
$database->disableValidation(); $ucount = $database->upsertDocumentsWithIncrease( collection: $collection, attribute: 'value', documents: [new Document([ '$id' => 'u1', 'name' => null, 'value' => 1, ])] ); $this->assertEquals(1, $ucount); $database->enableValidation(); + $u1 = $database->getDocument($collection, 'u1'); + $this->assertEquals(1, $u1->getAttribute('value')); + $this->assertNull($u1->getAttribute('name'));src/Database/Database.php (1)
6257-6266: Avoid truthy checks for $max/$min (0 gets treated as no limit)Using truthiness can mis-handle 0. Prefer explicit null checks to preserve numeric 0 semantics.
Apply:
- $max = $max ? $max - $value : null; + $max = ($max !== null) ? ($max - $value) : null; @@ - $min = $min ? $min + $value : null; + $min = ($min !== null) ? ($min + $value) : null;Also applies to: 6357-6366
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Database/Adapter/MariaDB.php(2 hunks)src/Database/Adapter/Mongo.php(1 hunks)src/Database/Adapter/Postgres.php(2 hunks)src/Database/Database.php(5 hunks)tests/e2e/Adapter/Scopes/DocumentTests.php(1 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 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-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:
src/Database/Adapter/Mongo.phpsrc/Database/Adapter/MariaDB.php
📚 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:
src/Database/Adapter/Mongo.phpsrc/Database/Adapter/MariaDB.phptests/e2e/Adapter/Scopes/DocumentTests.phpsrc/Database/Database.php
📚 Learning: 2025-10-03T01:50:11.943Z
Learnt from: abnegate
Repo: utopia-php/database PR: 721
File: tests/e2e/Adapter/Scopes/AttributeTests.php:1329-1334
Timestamp: 2025-10-03T01:50:11.943Z
Learning: MongoDB has a 1024kb (1,048,576 bytes) limit for index entries. The MongoDB adapter's getMaxIndexLength() method should return this limit rather than 0.
Applied to files:
src/Database/Adapter/Mongo.php
📚 Learning: 2025-07-01T11:31:37.438Z
Learnt from: ArnabChatterjee20k
Repo: utopia-php/database PR: 613
File: src/Database/Adapter/Postgres.php:1254-1319
Timestamp: 2025-07-01T11:31:37.438Z
Learning: In PostgreSQL adapter methods like getUpsertStatement, complexity for database-specific SQL generation is acceptable when the main business logic is properly separated in the parent SQL adapter class, following the adapter pattern where each database adapter handles its own SQL syntax requirements.
Applied to files:
src/Database/Adapter/Postgres.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
🧬 Code graph analysis (4)
src/Database/Adapter/Postgres.php (2)
src/Database/Adapter/SQLite.php (1)
getSQLTable(1128-1131)src/Database/Adapter/SQL.php (1)
getSQLTable(1860-1863)
src/Database/Adapter/MariaDB.php (2)
src/Database/Adapter/SQLite.php (1)
getSQLTable(1128-1131)src/Database/Adapter/SQL.php (1)
getSQLTable(1860-1863)
tests/e2e/Adapter/Scopes/DocumentTests.php (4)
src/Database/Database.php (11)
getAdapter(1319-1322)createAttribute(1816-1875)Database(38-8531)disableValidation(1096-1101)createDocuments(4518-4622)enableValidation(1084-1089)create(1361-1380)createDocument(4403-4502)updateDocument(4959-5191)count(7300-7347)upsertDocumentsWithIncrease(5896-6180)src/Database/Adapter.php (9)
getSupportForAttributes(943-943)createAttribute(579-579)createDocuments(735-735)create(510-510)createDocument(723-723)updateDocument(747-747)getSupportForBatchOperations(1030-1030)count(847-847)getSupportForUpserts(1051-1051)src/Database/Adapter/Mongo.php (9)
getSupportForAttributes(2647-2650)createAttribute(665-668)createDocuments(1343-1382)create(328-331)createDocument(1159-1186)updateDocument(1432-1459)getSupportForBatchOperations(2728-2731)count(2095-2182)getSupportForUpserts(2778-2781)src/Database/Adapter/SQL.php (5)
getSupportForAttributes(948-951)createAttribute(246-260)createDocuments(2410-2543)getSupportForBatchOperations(998-1001)count(3172-3234)
src/Database/Database.php (6)
src/Database/Validator/Structure.php (3)
Structure(20-460)isValid(211-245)getDescription(197-200)src/Database/Adapter/Mongo.php (3)
getIdAttributeType(3106-3109)getMinDateTime(2591-2594)getSupportForAttributes(2647-2650)src/Database/Adapter/SQL.php (2)
getIdAttributeType(2211-2214)getSupportForAttributes(948-951)src/Database/Adapter/Pool.php (3)
getIdAttributeType(530-533)getMinDateTime(325-328)getSupportForAttributes(335-338)src/Database/Adapter.php (4)
getIdAttributeType(919-919)getMinDateTime(912-912)getMaxDateTime(926-929)getSupportForAttributes(943-943)src/Database/Validator/PartialStructure.php (2)
isValid(19-61)PartialStructure(8-62)
⏰ 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 (5)
src/Database/Database.php (5)
4467-4477: createDocument: validation gate is correctly scopedStructure validation is now cleanly behind $this->validate after encoding. Good placement and parity with adapter capabilities.
Please confirm tests cover both enabled/disabled validation for createDocument (including schemaless adapters).
4570-4581: createDocuments: consistent conditional Structure validationBatch path mirrors single-create gating; looks good and consistent.
Verify bulk create with validation disabled accepts records that would otherwise fail Structure, and with validation enabled still rejects them.
5134-5146: updateDocument: guarded Structure validation with current document contextPassing $old to Structure only when validation is on preserves prior semantics while allowing bypass.
Ensure tests cover operator updates and relationship updates with validation toggled.
5291-5304: updateDocuments: PartialStructure behind validation flagUsing PartialStructure for bulk updates and gating it behind $this->validate matches intent and prior guidance on required attribute handling.
Based on learnings
6057-6070: upsertDocumentsWithIncrease: parity in Structure validationUpsert path now aligns with create/update behavior; old document passed when present.
Confirm tests cover upsert with validation disabled (including operator and permission fields).
| public function testBypassStructureWithSupportForAttributes(): void | ||
| { | ||
| /** @var Database $database */ | ||
| $database = static::getDatabase(); |
There was a problem hiding this comment.
🧩 Analysis chain
Prefer instance method: replace static::getDatabase() with $this->getDatabase().
Aligns with the rest of this trait and avoids static calls to potentially non‑static methods.
Run to confirm method signature across the repo:
Apply this diff:
- $database = static::getDatabase();
+ $database = $this->getDatabase();Also applies to: 6236-6236
🏁 Script executed:
#!/bin/bash
# Show static vs instance definitions of getDatabase()
rg -nP '^\s*(public|protected|private)?\s*static\s+function\s+getDatabase\s*\(' -C2
rg -nP '^\s*(public|protected|private)?\s*function\s+getDatabase\s*\(' -C2Length of output: 4997
Replace static::getDatabase() with $this->getDatabase() on lines 6191 and 6236.
Calling an instance method via static:: is incorrect. Verification confirms getDatabase() is consistently defined as a public instance method across all test classes, never static. This pattern can trigger deprecation warnings or errors in PHP 8.1+.
Apply this diff to both lines:
- $database = static::getDatabase();
+ $database = $this->getDatabase();📝 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.
| $database = static::getDatabase(); | |
| $database = $this->getDatabase(); |
🤖 Prompt for AI Agents
In tests/e2e/Adapter/Scopes/DocumentTests.php around lines 6191 and 6236,
replace calls to static::getDatabase() with $this->getDatabase() because
getDatabase() is an instance method, not static; update both occurrences to use
$this-> to avoid PHP 8.1+ deprecation/errors and keep consistent instance method
invocation.
Summary by CodeRabbit
Bug Fixes
New Features
Tests