Feat mongodb #731
Feat mongodb #731
Conversation
…and composer.lock
…nd MongoDB configuration adjustments
…and composer.lock
# Conflicts: # docker-compose.yml # src/Database/Database.php # tests/e2e/Adapter/Scopes/AttributeTests.php
…ocuments, improving sequence retrieval logic and preserving document structure.
…uments for sequences without existing IDs, and update index length checks in tests to align with maximum index length constraints.
…ngo-transactions # Conflicts: # composer.json # composer.lock # src/Database/Adapter/MariaDB.php
…files to improve code readability and maintainability.
…ion handling in Mongo adapter. Updated command for MongoDB to support replica sets and improved transaction callback handling in the adapter.
…adapter for tenant compatibility
…ecks for UTCDateTime instances and removing unused time conversion methods, improving code clarity and performance.
…tential conflicts during development.
# Conflicts: # src/Database/Database.php
…t updates; change Redis cache to Memory cache as fallback in tests; update Docker port mapping.
…into feat-mongo-2
…ns handling; update Docker configuration for client mapping; enhance tests with Redis cache integration.
…ngo-transactions # Conflicts: # src/Database/Adapter/Mongo.php
…and optimizing document ID retrieval; streamline sequence fetching logic for improved performance.
…ngo-transactions # Conflicts: # src/Database/Adapter/Mongo.php
…nd tenants; streamline upsert filter construction for improved clarity and performance.
…and adding MongoDB initialization credentials for username and password.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/e2e/Adapter/Scopes/IndexTests.php (1)
518-561: Good test structure for multiple fulltext index validation.The test properly:
- Guards with
getSupportForFulltextIndex()check- Uses try-finally for cleanup
- Validates behavior based on adapter capability
Optional: Consider using
$this->expectNotToPerformAssertions()instead ofassertTrue(true)for clearer intent, or add more descriptive assertion messages.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/e2e/Adapter/Scopes/IndexTests.php(6 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.php
🧬 Code graph analysis (1)
tests/e2e/Adapter/Scopes/IndexTests.php (4)
src/Database/Database.php (5)
getAdapter(1199-1202)createCollection(1328-1466)create(1241-1260)Database(37-8215)createIndex(3253-3413)src/Database/Adapter/Pool.php (13)
getMaxIndexLength(308-311)getInternalIndexesKeys(493-496)getSupportForIndexArray(343-346)getSupportForSpatialAttributes(433-436)getSupportForSpatialIndexNull(438-441)getSupportForSpatialIndexOrder(528-531)getSupportForAttributes(328-331)getSupportForMultipleFulltextIndexes(548-551)getSupportForIdenticalIndexes(553-556)createCollection(153-156)create(133-136)getSupportForFulltextIndex(358-361)createIndex(213-216)src/Database/Adapter.php (13)
getMaxIndexLength(876-876)getInternalIndexesKeys(1298-1298)getSupportForIndexArray(942-942)getSupportForSpatialAttributes(1064-1064)getSupportForSpatialIndexNull(1071-1071)getSupportForSpatialIndexOrder(1085-1085)getSupportForAttributes(921-921)getSupportForMultipleFulltextIndexes(1113-1113)getSupportForIdenticalIndexes(1121-1121)createCollection(525-525)create(488-488)getSupportForFulltextIndex(963-963)createIndex(670-670)src/Database/Validator/Index.php (3)
isValid(315-358)getDescription(97-100)Index(10-507)
⏰ 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/SQLite)
- GitHub Check: Adapter Tests (MySQL)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Adapter Tests (SharedTables/MongoDB)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Adapter Tests (MongoDB)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (Postgres)
- GitHub Check: Adapter Tests (SharedTables/Postgres)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (MariaDB)
- GitHub Check: Unit Test
🔇 Additional comments (2)
tests/e2e/Adapter/Scopes/IndexTests.php (2)
167-177: LGTM: Index validator constructor updated with new capability flags.The expanded constructor signature correctly propagates all adapter capability flags to the validator.
178-191: LGTM: Proper capability guard for identical index validation.The conditional block correctly skips validation when the adapter doesn't support identical indexes, preventing false test failures.
Based on learnings
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
tests/e2e/Adapter/Scopes/IndexTests.php (2)
335-338: Add guard for adapters without maximum index length.The test only checks
getSupportForAttributes()but should also verifygetMaxIndexLength() > 0. If an adapter returns 0 (no limit), the subsequent test logic becomes invalid.Apply this diff following the pattern established on Line 205:
- if (!$database->getAdapter()->getSupportForAttributes()) { + if (!$database->getAdapter()->getSupportForAttributes() || $database->getAdapter()->getMaxIndexLength() === 0) { $this->expectNotToPerformAssertions(); return; }
603-625: Fix test logic for non-identical index scenarios.The test incorrectly gates index3 (Lines 603-613) and index4 (Lines 615-625) behind
getSupportForIdenticalIndexes()checks. However:
- index3 has a different attribute order:
['age', 'name']vs. original['name', 'age']- index4 has different orders:
[DESC, ASC]vs. original[ASC, DESC]These are not identical indexes and should always succeed, regardless of
getSupportForIdenticalIndexes().Additionally:
- Line 603: Typo "faliure" → "failure"
- Line 615: Typo "orders order" → "orders"
Apply this diff:
- // Test with different attributes order - faliure + // Test with different attributes order - should succeed try { $database->createIndex($collectionId, 'index3', Database::INDEX_KEY, ['age', 'name'], [], [ Database::ORDER_ASC, Database::ORDER_DESC]); $this->assertTrue(true, 'Index with different attributes was created successfully'); } catch (Throwable $e) { - if (!$supportsIdenticalIndexes) { - $this->assertTrue(true, 'Identical indexes are not supported and exception was thrown as expected'); - } else { - $this->fail('Unexpected exception when creating identical index: ' . $e->getMessage()); - } + $this->fail('Unexpected exception when creating index with different attribute order: ' . $e->getMessage()); } - // Test with different orders order - faliure + // Test with different orders - should succeed try { $database->createIndex($collectionId, 'index4', Database::INDEX_KEY, ['age', 'name'], [], [ Database::ORDER_DESC, Database::ORDER_ASC]); $this->assertTrue(true, 'Index with different attributes was created successfully'); } catch (Throwable $e) { - if (!$supportsIdenticalIndexes) { - $this->assertTrue(true, 'Identical indexes are not supported and exception was thrown as expected'); - } else { - $this->fail('Unexpected exception when creating identical index: ' . $e->getMessage()); - } + $this->fail('Unexpected exception when creating index with different orders: ' . $e->getMessage()); }
🧹 Nitpick comments (1)
tests/e2e/Adapter/Scopes/IndexTests.php (1)
523-566: Consider more meaningful assertions.The test logic correctly handles capability-dependent behavior, but uses
assertTrue(true, 'message')which always passes. While the test will fail if an unexpected exception occurs (or doesn't occur), explicit success assertions would improve clarity.Consider this pattern for Lines 549-553:
if ($supportsMultipleFulltext) { - $this->assertTrue(true, 'Multiple fulltext indexes are supported and second index was created successfully'); + // Success: verify the index was created + $collection = $database->getCollection($collectionId); + $this->assertCount(2, array_filter($collection->getAttribute('indexes', []), fn($idx) => $idx['type'] === Database::INDEX_FULLTEXT)); } else {Similarly for Lines 555-557:
if (!$supportsMultipleFulltext) { - $this->assertTrue(true, 'Multiple fulltext indexes are not supported and exception was thrown as expected'); + $this->assertStringContainsString('fulltext', strtolower($e->getMessage())); } else {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/e2e/Adapter/Scopes/IndexTests.php(6 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.php
🧬 Code graph analysis (1)
tests/e2e/Adapter/Scopes/IndexTests.php (5)
src/Database/Database.php (3)
create(1241-1260)Database(37-8215)createIndex(3253-3413)src/Database/Adapter.php (12)
getMaxIndexLength(876-876)getInternalIndexesKeys(1298-1298)getSupportForIndexArray(942-942)getSupportForSpatialAttributes(1064-1064)getSupportForSpatialIndexNull(1071-1071)getSupportForSpatialIndexOrder(1085-1085)getSupportForAttributes(921-921)getSupportForMultipleFulltextIndexes(1113-1113)getSupportForIdenticalIndexes(1121-1121)create(488-488)getSupportForFulltextIndex(963-963)createIndex(670-670)src/Database/Adapter/Mongo.php (12)
getMaxIndexLength(3043-3046)getInternalIndexesKeys(3061-3064)getSupportForIndexArray(2581-2584)getSupportForSpatialAttributes(2838-2841)getSupportForSpatialIndexNull(2848-2851)getSupportForSpatialIndexOrder(2869-2872)getSupportForAttributes(2612-2615)getSupportForMultipleFulltextIndexes(2905-2908)getSupportForIdenticalIndexes(2915-2918)create(319-322)getSupportForFulltextIndex(2632-2635)createIndex(895-1016)src/Database/Adapter/SQL.php (10)
getMaxIndexLength(1845-1851)getInternalIndexesKeys(1903-1906)getSupportForIndexArray(1460-1463)getSupportForSpatialAttributes(1490-1493)getSupportForSpatialIndexNull(1500-1503)getSupportForSpatialIndexOrder(1510-1513)getSupportForAttributes(896-899)getSupportForMultipleFulltextIndexes(1530-1533)getSupportForIdenticalIndexes(1540-1543)getSupportForFulltextIndex(916-919)src/Database/Validator/Index.php (3)
isValid(315-358)getDescription(97-100)Index(10-507)
⏰ 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). (12)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (SharedTables/Postgres)
- GitHub Check: Adapter Tests (SharedTables/SQLite)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (MySQL)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (SharedTables/MongoDB)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Adapter Tests (MongoDB)
- GitHub Check: Adapter Tests (Postgres)
- GitHub Check: Adapter Tests (MariaDB)
🔇 Additional comments (5)
tests/e2e/Adapter/Scopes/IndexTests.php (5)
167-177: LGTM! Correct integration of capability flags.The Index validator constructor now receives the full set of capability flags from the adapter, enabling feature-gated validation. This aligns with the expanded constructor signature in
src/Database/Validator/Index.phpand the usage inDatabase::createIndex.
205-205: LGTM! Properly guards against adapters with no max index length.The guard now checks both
getSupportForAttributes()andgetMaxIndexLength() > 0, ensuring the test returns early for adapters that don't enforce a maximum index length. This addresses a previous review concern.
247-254: LGTM! Clear test setup with helpful comment.Creating a separate
$newIndexdocument with a distinct ID is correct, and the comment appropriately explains why the validator needs a different ID to perform validation.
280-285: LGTM! Proper guard for schemaless adapters.The test correctly expects an exception only when
getSupportForAttributes()is true, as schemaless adapters won't validate attribute types in fulltext indexes. Based on learnings.
297-328: LGTM! Consistent guards for attribute-dependent validation.The negative length and extra length validations are properly guarded by
getSupportForAttributes(), ensuring schemaless adapters skip these checks.
Documentsdb(Schemaless support with mongodb adapter)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/e2e/Adapter/Schemaless/MongoDBTest.php (1)
32-67: Verify attribute support configuration is correct for schemaless tests.The database initialization correctly uses
setSupportForAttributes(false)at line 54, which is appropriate for schemaless testing where MongoDB's flexible schema is used without enforced attribute definitions.Minor consistency note: There's an extra blank line at line 62 that doesn't appear in the regular MongoDBTest.php. Consider removing it for consistency.
Apply this diff to remove the extra blank line:
if ($database->exists()) { $database->delete(); } - $database->create();
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/tests.yml(1 hunks)src/Database/Adapter.php(4 hunks)src/Database/Adapter/Pool.php(4 hunks)src/Database/Adapter/SQL.php(4 hunks)tests/e2e/Adapter/MongoDBTest.php(1 hunks)tests/e2e/Adapter/Schemaless/MongoDBTest.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/tests.yml
🧰 Additional context used
🧬 Code graph analysis (5)
src/Database/Adapter.php (4)
src/Database/Adapter/Pool.php (11)
getMaxUIDLength(313-316)getSupportForMultipleFulltextIndexes(548-551)getSupportForIdenticalIndexes(553-556)getSupportForOrderRandom(558-561)getAttributeProjection(478-481)castingBefore(578-581)castingAfter(583-586)getSupportForInternalCasting(588-591)getSupportForUTCCasting(593-596)setUTCDatetime(598-601)setSupportForAttributes(603-606)src/Database/Adapter/SQL.php (11)
getMaxUIDLength(1856-1859)getSupportForMultipleFulltextIndexes(1530-1533)getSupportForIdenticalIndexes(1540-1543)getSupportForOrderRandom(1550-1553)getAttributeProjection(1955-1984)castingBefore(1565-1568)castingAfter(1570-1573)getSupportForInternalCasting(1520-1523)getSupportForUTCCasting(1555-1558)setUTCDatetime(1560-1563)setSupportForAttributes(2934-2937)src/Database/Adapter/Mongo.php (11)
getMaxUIDLength(3058-3061)getSupportForMultipleFulltextIndexes(2912-2915)getSupportForIdenticalIndexes(2922-2925)getSupportForOrderRandom(2932-2935)getAttributeProjection(2487-2512)castingBefore(1246-1298)castingAfter(1183-1237)getSupportForInternalCasting(2592-2595)getSupportForUTCCasting(2597-2600)setUTCDatetime(2602-2605)setSupportForAttributes(2618-2622)src/Database/Document.php (1)
Document(12-470)
src/Database/Adapter/SQL.php (4)
src/Database/Adapter.php (11)
getSupportForInternalCasting(1369-1369)getSupportForMultipleFulltextIndexes(1113-1113)getSupportForIdenticalIndexes(1121-1121)getSupportForOrderRandom(1128-1128)getSupportForUTCCasting(1376-1376)setUTCDatetime(1384-1384)castingBefore(1354-1354)castingAfter(1362-1362)getMaxUIDLength(883-883)getAttributeProjection(1193-1193)setSupportForAttributes(1392-1392)src/Database/Adapter/Pool.php (11)
getSupportForInternalCasting(588-591)getSupportForMultipleFulltextIndexes(548-551)getSupportForIdenticalIndexes(553-556)getSupportForOrderRandom(558-561)getSupportForUTCCasting(593-596)setUTCDatetime(598-601)castingBefore(578-581)castingAfter(583-586)getMaxUIDLength(313-316)getAttributeProjection(478-481)setSupportForAttributes(603-606)src/Database/Adapter/Mongo.php (11)
getSupportForInternalCasting(2592-2595)getSupportForMultipleFulltextIndexes(2912-2915)getSupportForIdenticalIndexes(2922-2925)getSupportForOrderRandom(2932-2935)getSupportForUTCCasting(2597-2600)setUTCDatetime(2602-2605)castingBefore(1246-1298)castingAfter(1183-1237)getMaxUIDLength(3058-3061)getAttributeProjection(2487-2512)setSupportForAttributes(2618-2622)src/Database/Document.php (1)
Document(12-470)
src/Database/Adapter/Pool.php (9)
src/Database/Adapter.php (14)
getMaxUIDLength(883-883)getAttributeProjection(1193-1193)getSupportForDistanceBetweenMultiDimensionGeometryInMeters(1106-1106)getSupportForSpatialAxisOrder(1092-1092)getSupportForOptionalSpatialAttributeWithExistingRows(1078-1078)getSupportForMultipleFulltextIndexes(1113-1113)getSupportForIdenticalIndexes(1121-1121)getSupportForOrderRandom(1128-1128)castingBefore(1354-1354)castingAfter(1362-1362)getSupportForInternalCasting(1369-1369)getSupportForUTCCasting(1376-1376)setUTCDatetime(1384-1384)setSupportForAttributes(1392-1392)src/Database/Adapter/SQL.php (12)
getMaxUIDLength(1856-1859)getAttributeProjection(1955-1984)getSupportForSpatialAxisOrder(1580-1583)getSupportForMultipleFulltextIndexes(1530-1533)getSupportForIdenticalIndexes(1540-1543)getSupportForOrderRandom(1550-1553)castingBefore(1565-1568)castingAfter(1570-1573)getSupportForInternalCasting(1520-1523)getSupportForUTCCasting(1555-1558)setUTCDatetime(1560-1563)setSupportForAttributes(2934-2937)src/Database/Adapter/Mongo.php (14)
getMaxUIDLength(3058-3061)getAttributeProjection(2487-2512)getSupportForDistanceBetweenMultiDimensionGeometryInMeters(2897-2900)getSupportForSpatialAxisOrder(2887-2890)getSupportForOptionalSpatialAttributeWithExistingRows(2902-2905)getSupportForMultipleFulltextIndexes(2912-2915)getSupportForIdenticalIndexes(2922-2925)getSupportForOrderRandom(2932-2935)castingBefore(1246-1298)castingAfter(1183-1237)getSupportForInternalCasting(2592-2595)getSupportForUTCCasting(2597-2600)setUTCDatetime(2602-2605)setSupportForAttributes(2618-2622)src/Database/Mirror.php (1)
delegate(88-103)src/Database/Adapter/MariaDB.php (3)
getSupportForDistanceBetweenMultiDimensionGeometryInMeters(1886-1889)getSupportForSpatialAxisOrder(1923-1926)getSupportForOptionalSpatialAttributeWithExistingRows(1933-1936)src/Database/Adapter/Postgres.php (3)
getSupportForDistanceBetweenMultiDimensionGeometryInMeters(1997-2000)getSupportForSpatialAxisOrder(2007-2010)getSupportForOptionalSpatialAttributeWithExistingRows(2017-2020)src/Database/Adapter/SQLite.php (3)
getSupportForDistanceBetweenMultiDimensionGeometryInMeters(1273-1276)getSupportForSpatialAxisOrder(1283-1286)getSupportForOptionalSpatialAttributeWithExistingRows(1293-1296)src/Database/Adapter/MySQL.php (3)
getSupportForDistanceBetweenMultiDimensionGeometryInMeters(189-192)getSupportForSpatialAxisOrder(242-245)getSupportForOptionalSpatialAttributeWithExistingRows(263-266)src/Database/Document.php (1)
Document(12-470)
tests/e2e/Adapter/Schemaless/MongoDBTest.php (3)
src/Database/Adapter.php (8)
Adapter(16-1394)getDatabase(160-163)setSupportForAttributes(1392-1392)setDatabase(145-150)setNamespace(92-97)exists(499-499)delete(515-515)create(488-488)src/Database/Adapter/Mongo.php (5)
Mongo(23-3150)setSupportForAttributes(2618-2622)exists(335-353)delete(380-385)create(320-323)tests/e2e/Adapter/MongoDBTest.php (2)
MongoDBTest(13-109)getDatabase(32-66)
tests/e2e/Adapter/MongoDBTest.php (4)
src/Database/Adapter/Mongo.php (6)
Mongo(23-3150)setSupportForAttributes(2618-2622)exists(335-353)delete(380-385)create(320-323)deleteIndex(1086-1093)tests/e2e/Adapter/Schemaless/MongoDBTest.php (9)
MongoDBTest(13-110)getAdapterName(23-26)getDatabase(32-67)testCreateExistsDelete(72-79)testRenameAttribute(81-84)testRenameAttributeExisting(86-89)testUpdateAttributeStructure(91-94)testKeywords(96-99)deleteColumn(101-104)tests/e2e/Adapter/SharedTables/MongoDBTest.php (10)
MongoDBTest(14-111)getAdapterName(24-27)getDatabase(33-68)testCreateExistsDelete(73-80)testRenameAttribute(82-85)testRenameAttributeExisting(87-90)testUpdateAttributeStructure(92-95)testKeywords(97-100)deleteColumn(102-105)deleteIndex(107-110)tests/e2e/Adapter/Base.php (4)
Base(20-68)getDatabase(38-38)deleteColumn(46-46)deleteIndex(54-54)
🪛 PHPMD (2.15.0)
src/Database/Adapter/SQL.php
1565-1565: Avoid unused parameters such as '$collection'. (undefined)
(UnusedFormalParameter)
1570-1570: Avoid unused parameters such as '$collection'. (undefined)
(UnusedFormalParameter)
2934-2934: Avoid unused parameters such as '$support'. (undefined)
(UnusedFormalParameter)
tests/e2e/Adapter/Schemaless/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)
tests/e2e/Adapter/MongoDBTest.php
100-100: Avoid unused parameters such as '$collection'. (undefined)
(UnusedFormalParameter)
100-100: Avoid unused parameters such as '$column'. (undefined)
(UnusedFormalParameter)
105-105: Avoid unused parameters such as '$collection'. (undefined)
(UnusedFormalParameter)
105-105: 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). (14)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Adapter Tests (SharedTables/SQLite)
- GitHub Check: Adapter Tests (SharedTables/Postgres)
- GitHub Check: Adapter Tests (Postgres)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (MariaDB)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (Schemaless/MongoDB)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (SharedTables/MongoDB)
- GitHub Check: Adapter Tests (MongoDB)
- GitHub Check: Adapter Tests (MySQL)
- GitHub Check: Unit Test
🔇 Additional comments (26)
src/Database/Adapter/SQL.php (9)
1515-1523: LGTM: Correctly indicates no internal casting support.The SQL adapter appropriately returns
falsefor internal casting support, as SQL databases handle type casting at the database level rather than in the adapter layer.
1525-1533: LGTM: Correctly indicates multiple fulltext index support.SQL databases (MySQL/MariaDB) do support multiple fulltext indexes per table, so returning
trueis appropriate.
1535-1543: LGTM: Correctly indicates identical index support.SQL databases allow creating identical indexes (though generally not recommended), so returning
trueis correct.
1545-1553: LGTM: Correctly indicates random order support.SQL databases support random ordering through functions like
RAND(), so returningtrueis appropriate.
1555-1563: LGTM: Correctly implements UTC casting as no-op.The SQL adapter doesn't require UTC conversion at the adapter level since datetime values are stored as strings. Returning the input unchanged is the correct implementation.
1565-1573: LGTM: Correctly implements casting as no-op.These methods are intentional pass-throughs since the SQL adapter returns
falsefromgetSupportForInternalCasting(). The unused$collectionparameters flagged by static analysis are required by the abstract method signatures and represent the correct no-op implementation for SQL adapters.
1853-1859: LGTM: Correct UUID length.Returning 36 is correct for standard UUID format (8-4-4-4-12 with hyphens), which is the appropriate maximum UID length for SQL adapters.
1947-1984: LGTM: Return type broadened to support adapter-specific formats.Changing the return type from
stringtomixedis appropriate, as different adapters need to return different projection formats (SQL returns a comma-separated string, while MongoDB returns an array).
2934-2937: Verify intended behavior: always returns true.This method ignores the
$supportparameter and always returnstrue. While SQL adapters always support attributes (making this behavior potentially correct), the MongoDB adapter actually stores and uses this value. Please confirm whether:
- SQL adapters should always support attributes regardless of the parameter, or
- This should store the value like the MongoDB implementation
For context, the MongoDB adapter implementation at lines 2617-2621 stores the value:
public function setSupportForAttributes(bool $support): bool { $this->supportForAttributes = $support; return $this->supportForAttributes; }If SQL always supports attributes, consider adding a comment explaining why the parameter is ignored.
src/Database/Adapter.php (4)
878-883: LGTM: Well-defined capability method.The new
getMaxUIDLength()abstract method is properly documented and allows adapters to specify their maximum UID length constraints (e.g., 36 for UUIDs in SQL, 255 for MongoDB).
1108-1128: LGTM: Clear capability declarations.The new capability methods (
getSupportForMultipleFulltextIndexes,getSupportForIdenticalIndexes,getSupportForOrderRandom) are well-documented and allow adapters to declare their specific index and ordering capabilities. The implementations show appropriate variation (e.g., SQL supports all three, MongoDB supports none).
1186-1193: LGTM: Return type appropriately broadened.Changing the return type from
stringtomixedforgetAttributeProjection()is correct, as different adapters need different projection formats (SQL uses comma-separated strings, MongoDB uses projection arrays).
1348-1392: LGTM: Well-designed casting lifecycle hooks.The new abstract methods for casting lifecycle management are well-designed:
castingBefore()andcastingAfter()provide clear hooks for document transformation before/after database operationsgetSupportForInternalCasting()andgetSupportForUTCCasting()allow adapters to declare their casting capabilitiessetUTCDatetime()andsetSupportForAttributes()provide runtime configurationThis design allows MongoDB to perform internal type conversions (integers, UTC dates) while SQL adapters can remain as pass-throughs.
src/Database/Adapter/Pool.php (4)
313-316: LGTM: Correct delegation implementation.The
getMaxUIDLength()method correctly delegates to the underlying adapter, consistent with the Pool adapter pattern.
478-481: LGTM: Correct delegation with updated return type.The
getAttributeProjection()method correctly updates its return type tomixed(matching the parent class) and properly delegates to the underlying adapter.
548-561: LGTM: Correct capability delegation.The index and order capability methods (
getSupportForMultipleFulltextIndexes,getSupportForIdenticalIndexes,getSupportForOrderRandom) correctly delegate to the underlying adapter, following the established Pool pattern.
578-606: LGTM: Correct casting lifecycle delegation.The casting-related methods (
castingBefore,castingAfter,getSupportForInternalCasting,getSupportForUTCCasting,setUTCDatetime,setSupportForAttributes) all correctly delegate to the underlying adapter, maintaining consistency with the Pool adapter pattern.tests/e2e/Adapter/MongoDBTest.php (5)
13-26: LGTM!The class structure follows the established pattern for MongoDB adapter tests, correctly extending Base and implementing the required
getAdapterName()method.
71-78: LGTM!The override of
testCreateExistsDelete()correctly accounts for MongoDB's lazy database creation semantics. The comment clearly explains why the exists check is removed.
100-108: Stub methods are acceptable for the abstract contract.The
deleteColumnanddeleteIndexhelper methods returntruewithout implementation, which is expected since they fulfill the abstract contract from Base while MongoDB likely handles cleanup differently than SQL databases. The PHPMD warnings about unused parameters are false positives in this context.
80-98: Verify MongoDB adapter support for attribute operations
TeststestRenameAttribute,testRenameAttributeExisting,testUpdateAttributeStructure, andtestKeywordsare stubbed without corresponding methods in the Mongo adapter; confirm whether these operations should be implemented, are intentionally unsupported due to schema flexibility, or if the tests need updating.
32-66: Verify MongoDB lazy-creation and namespace isolation
- Ensure tests explicitly cover MongoDB’s “create on first write” semantics around
exists()/delete()/create().- No tests reference a hard-coded
myapp_prefix; all adapters useuniqid()for isolation.tests/e2e/Adapter/Schemaless/MongoDBTest.php (4)
13-26: LGTM!The class structure correctly follows the pattern for schemaless MongoDB adapter tests, extending Base and implementing
getAdapterName().
72-79: LGTM!The override correctly handles MongoDB's lazy database creation, consistent with the regular MongoDBTest implementation.
81-99: Verify stub implementations (same concern as regular MongoDBTest).These stub test methods mirror those in the regular MongoDBTest.php. The same verification applies: confirm whether these operations are intentionally unsupported for schemaless MongoDB, are placeholders, or need different implementations.
101-109: Stub helpers fulfill the abstract contract.Like the regular MongoDBTest.php, these stubs satisfy Base's abstract requirements. The unused parameter warnings from PHPMD are expected false positives.
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 (3)
src/Database/Database.php (3)
2044-2054: Spatial defaults always throw due to missing breakThe spatial branch falls through to default and throws “Unknown attribute type” even when the type is valid.
Apply this fix:
case self::VAR_POINT: case self::VAR_LINESTRING: case self::VAR_POLYGON: // Spatial types expect arrays as default values if ($defaultType !== 'array') { throw new DatabaseException('Default value for spatial type ' . $type . ' must be an array'); } - // no break - default: - throw new DatabaseException('Unknown attribute type: ' . $type . '. Must be one of ' . self::VAR_STRING . ', ' . self::VAR_INTEGER . ', ' . self::VAR_FLOAT . ', ' . self::VAR_BOOLEAN . ', ' . self::VAR_DATETIME . ', ' . self::VAR_RELATIONSHIP . ', ' . self::VAR_POINT . ', ' . self::VAR_LINESTRING . ', ' . self::VAR_POLYGON); + break; + default: + throw new DatabaseException('Unknown attribute type: ' . $type . '. Must be one of ' . self::VAR_STRING . ', ' . self::VAR_INTEGER . ', ' . self::VAR_FLOAT . ', ' . self::VAR_BOOLEAN . ', ' . self::VAR_DATETIME . ', ' . self::VAR_RELATIONSHIP . ', ' . self::VAR_POINT . ', ' . self::VAR_LINESTRING . ', ' . self::VAR_POLYGON);
5131-5140: Fix concurrency check and cast updates before adapter call
- Concurrency check uses the merged/new document’s updatedAt instead of the persisted value.
$updatesis cast afteradapter->updateDocuments(...); it must be cast before the call to persist driver-native types (e.g., UTCDateTime).Minimal diffs:
- Use original updatedAt before overwriting
$document:- $document = $new; + // capture original updatedAt before overwriting $document + $originalUpdatedAt = $document->getUpdatedAt(); + $document = $new; - try { - $oldUpdatedAt = new \DateTime($document->getUpdatedAt()); + try { + $oldUpdatedAt = new \DateTime($originalUpdatedAt);
- Cast
$updatesbefore the adapter call; remove the post-call cast:- $this->adapter->updateDocuments( + // cast $updates to adapter-native types (e.g., UTCDateTime) before updating + $updates = $this->adapter->castingBefore($collection, $updates); + $this->adapter->updateDocuments( $collection, $updates, $batch ); - }); - - $updates = $this->adapter->castingBefore($collection, $updates); + });Also applies to: 5145-5153
7940-8169: Fix MANY_TO_MANY relationship resolution to use junction collections
- convertRelationshipQueries must receive the parent collection Document (e.g. via signature change) and, for M2M links, call getJunctionCollection(parent, related, side), query the junction with matching related IDs, extract parent IDs, and use those for Query::equal('$id', …) instead of reading twoWayKey on related docs.
- processNestedRelationshipPath must likewise detect M2M in its reverse‐lookup branch and resolve via the junction collection rather than loading twoWayKey from child documents.
- Consider replacing PHP_INT_MAX limits with chunked queries (RELATION_QUERY_CHUNK_SIZE) to avoid large scans.
🧹 Nitpick comments (1)
src/Database/Database.php (1)
2346-2346: Method casing nitUse consistent method casing for readability.
- ->setattribute('key', $newKey ?? $id) + ->setAttribute('key', $newKey ?? $id)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Database/Database.php(39 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/Database/Database.php (5)
src/Database/Adapter.php (15)
getMaxIndexLength(876-876)getInternalIndexesKeys(1298-1298)getSupportForIndexArray(942-942)getSupportForSpatialAttributes(1064-1064)getSupportForSpatialIndexNull(1071-1071)getSupportForSpatialIndexOrder(1085-1085)getSupportForAttributes(921-921)getSupportForMultipleFulltextIndexes(1113-1113)getSupportForIdenticalIndexes(1121-1121)castingAfter(1362-1362)castingBefore(1354-1354)updateDocument(725-725)sum(814-814)getSupportForUTCCasting(1376-1376)setUTCDatetime(1384-1384)src/Database/Adapter/Pool.php (15)
getMaxIndexLength(308-311)getInternalIndexesKeys(493-496)getSupportForIndexArray(343-346)getSupportForSpatialAttributes(433-436)getSupportForSpatialIndexNull(438-441)getSupportForSpatialIndexOrder(528-531)getSupportForAttributes(328-331)getSupportForMultipleFulltextIndexes(548-551)getSupportForIdenticalIndexes(553-556)castingAfter(583-586)castingBefore(578-581)updateDocument(238-241)sum(268-271)getSupportForUTCCasting(593-596)setUTCDatetime(598-601)src/Database/Adapter/Mongo.php (15)
getMaxIndexLength(3087-3090)getInternalIndexesKeys(3105-3108)getSupportForIndexArray(2614-2617)getSupportForSpatialAttributes(2877-2880)getSupportForSpatialIndexNull(2887-2890)getSupportForSpatialIndexOrder(2908-2911)getSupportForAttributes(2645-2648)getSupportForMultipleFulltextIndexes(2944-2947)getSupportForIdenticalIndexes(2954-2957)castingAfter(1194-1269)castingBefore(1278-1330)updateDocument(1432-1459)sum(2195-2237)getSupportForUTCCasting(2629-2632)setUTCDatetime(2634-2637)src/Database/Adapter/SQL.php (14)
getMaxIndexLength(1845-1851)getInternalIndexesKeys(1903-1906)getSupportForIndexArray(1460-1463)getSupportForSpatialAttributes(1490-1493)getSupportForSpatialIndexNull(1500-1503)getSupportForSpatialIndexOrder(1510-1513)getSupportForAttributes(896-899)getSupportForMultipleFulltextIndexes(1530-1533)getSupportForIdenticalIndexes(1540-1543)castingAfter(1570-1573)castingBefore(1565-1568)sum(2668-2731)getSupportForUTCCasting(1555-1558)setUTCDatetime(1560-1563)src/Database/Validator/Sequence.php (1)
isValid(38-59)
⏰ 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/SQLite)
- GitHub Check: Adapter Tests (Postgres)
- GitHub Check: Adapter Tests (Schemaless/MongoDB)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (MongoDB)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (SharedTables/Postgres)
- GitHub Check: Adapter Tests (MariaDB)
- GitHub Check: Adapter Tests (SharedTables/MongoDB)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Adapter Tests (MySQL)
🔇 Additional comments (4)
src/Database/Database.php (4)
3549-3550: Adapter casting hooks integration looks correctCasting order around read/write (before encode to driver, after read from driver) is sensible and consistent across get/create/update/batch paths.
Please ensure Mongo and SQL adapters’ casting hooks are no-ops where appropriate and fully cover datetime/objectID types for Mongo.
Also applies to: 4274-4275, 4380-4381, 4395-4396, 4939-4940, 5156-5161, 5843-5844, 5856-5859
2251-2257: IndexValidator: original index snapshot + capability flags propagation LGTM
- Cloning existing indexes before mutation prevents validator feedback from being influenced by in-flight changes.
- New flags passed to IndexValidator align with adapter capability surface.
Confirm all other IndexValidator call sites across the codebase were updated to the new signature.
Also applies to: 2426-2438, 3371-3384
5905-5925: Good: attribute existence/type checks gated by adapter capabilityValidating numeric and non-array constraints for increase/decrease when the adapter exposes attributes is appropriate.
Also applies to: 6005-6025
3490-3491: Good: DocumentValidator now aware of adapter attribute supportPassing adapter capability allows validators to adapt to schemaless adapters.
# Conflicts: # composer.lock # src/Database/Database.php # src/Database/Validator/Index.php # src/Database/Validator/Sequence.php # tests/e2e/Adapter/Base.php # tests/e2e/Adapter/Scopes/DocumentTests.php # tests/e2e/Adapter/Scopes/IndexTests.php # tests/unit/Validator/StructureTest.php
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
tests/e2e/Adapter/Scopes/DocumentTests.php (1)
217-235: Schemaless adapters still hit fail()When
getSupportForAttributes()is false, thesecreateDocument()calls now succeed, so execution runs straight into$this->fail(...). That breaks schemaless adapters (including the new Mongo flow) and contradicts the earlier guidance to assert successful creation instead of expecting exceptions. Please gate the failure expectations inside thesupportForAttributesbranch and add positive assertions for the schemaless path in all three blocks.- 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 (StructureException $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->assertInstanceOf(Document::class, $doc); + }Apply the same pattern to the subsequent validation blocks. Based on learnings
Also applies to: 238-257, 259-281
tests/e2e/Adapter/Scopes/IndexTests.php (4)
178-191: Incorrect capability guard for length validation.The guard wraps length validation inside the
getSupportForIdenticalIndexes()check. However, length validation (ensuring index length doesn't exceed attribute size) is independent of identical index support and should run whenevergetSupportForAttributes()is true.Apply this diff to fix the guard:
- if ($database->getAdapter()->getSupportForIdenticalIndexes()) { + if ($database->getAdapter()->getSupportForAttributes()) { $errorMessage = 'Index length 701 is larger than the size for title1: 700"'; $this->assertFalse($validator->isValid($indexes[0])); $this->assertEquals($errorMessage, $validator->getDescription()); try { $database->createCollection($collection->getId(), $attributes, $indexes, [ Permission::read(Role::any()), Permission::create(Role::any()), ]); $this->fail('Failed to throw exception'); } catch (Exception $e) { $this->assertEquals($errorMessage, $e->getMessage()); } }
270-276: Restructure validation assertions with proper guards.Line 270 unconditionally asserts
isValid($newIndex)returns false, but the validation logic depends on capability flags. If bothgetSupportForMultipleFulltextIndexes()andgetSupportForAttributes()are false, validation may pass, causing the assertion to fail.Apply this diff to guard the assertions:
- $this->assertFalse($validator->isValid($newIndex)); - - if (!$database->getAdapter()->getSupportForMultipleFulltextIndexes()) { + if (!$database->getAdapter()->getSupportForMultipleFulltextIndexes()) { + $this->assertFalse($validator->isValid($newIndex)); $this->assertEquals('There is already a fulltext index in the collection', $validator->getDescription()); } elseif ($database->getAdapter()->getSupportForAttributes()) { + $this->assertFalse($validator->isValid($newIndex)); $this->assertEquals('Attribute "integer" cannot be part of a fulltext index, must be of type string', $validator->getDescription()); }
335-338: Add guard for adapters without maximum index length.The test only checks
getSupportForAttributes()but should also verifygetMaxIndexLength() > 0. If an adapter returns 0 (no limit), the test logic on Lines 342-360 becomes invalid, as it attempts to exceed a non-existent maximum.Apply this diff following the pattern established on Line 205:
- if (!$database->getAdapter()->getSupportForAttributes()) { + if (!$database->getAdapter()->getSupportForAttributes() || $database->getAdapter()->getMaxIndexLength() === 0) { $this->expectNotToPerformAssertions(); return; }
603-625: Fix test logic for non-identical index scenarios.The test logic is incorrect for index3 (Lines 603-613) and index4 (Lines 615-625):
- index3 with
['age', 'name']is not identical to the original['name', 'age'](different attribute order)- index4 with
[DESC, ASC]orders is not identical to earlier indexesThese non-identical variations should always succeed regardless of
getSupportForIdenticalIndexes(). The capability flag should only govern truly identical indexes (same attributes, same order, same sort orders).Additionally, fix typos:
- Line 603: "faliure" → "failure"
- Line 615: "orders order" → "orders"
Remove the
try/catchblocks for both cases so the createIndex calls run unguarded and assert success unconditionally:- // Test with different attributes order - faliure + // Test with different attributes order - should succeed try { $database->createIndex($collectionId, 'index3', Database::INDEX_KEY, ['age', 'name'], [], [ Database::ORDER_ASC, Database::ORDER_DESC]); $this->assertTrue(true, 'Index with different attributes was created successfully'); } catch (Throwable $e) { - if (!$supportsIdenticalIndexes) { - $this->assertTrue(true, 'Identical indexes are not supported and exception was thrown as expected'); - } else { - $this->fail('Unexpected exception when creating identical index: ' . $e->getMessage()); - } + $this->fail('Unexpected exception when creating index with different attribute order: ' . $e->getMessage()); } - // Test with different orders order - faliure + // Test with different orders - should succeed try { $database->createIndex($collectionId, 'index4', Database::INDEX_KEY, ['age', 'name'], [], [ Database::ORDER_DESC, Database::ORDER_ASC]); $this->assertTrue(true, 'Index with different attributes was created successfully'); } catch (Throwable $e) { - if (!$supportsIdenticalIndexes) { - $this->assertTrue(true, 'Identical indexes are not supported and exception was thrown as expected'); - } else { - $this->fail('Unexpected exception when creating identical index: ' . $e->getMessage()); - } + $this->fail('Unexpected exception when creating index with different orders: ' . $e->getMessage()); }src/Database/Database.php (1)
8094-8299: Still missing parent context for relationship query conversion
convertRelationshipQueries()is still blind to the parent collection, so the M2M branch keeps depending on$twoWayKeybeing hydrated on the related documents. InsideskipRelationships()(where$this->resolveRelationshipsis deliberatelyfalse), those related docs no longer populate$twoWayKey, the parent-id list ends up empty, and anyQuery::equal('tags.name', …)style filter on a many-to-many collapses tonull(i.e. we incorrectly filter out every parent). This is the same junction-resolution hole flagged earlier.Please thread the parent
Document $collectionthrough every call (find/count/sum/etc.) and teachconvertRelationshipQueries()to derive parents explicitly—e.g. callgetJunctionCollection($collection, $relatedCollection, …)whenrelationType === manyToManyinstead of relying on resolved relationships. Without that propagation, many-to-many filters remain broken underskipRelationships().
🧹 Nitpick comments (2)
src/Database/Adapter/SQL.php (1)
2992-2995: Review the implementation logic.The method ignores the
$supportparameter and always returnstrue. While SQL adapters indeed always support attributes, consider:
- If the parameter is meant to be a no-op for SQL, add a comment explaining why (e.g., "SQL adapters always support attributes")
- If this should match the interface of other adapters, consider returning
$this->getSupportForAttributes()for consistencyThe current implementation may confuse callers expecting the parameter to have an effect.
public function setSupportForAttributes(bool $support): bool { + // SQL adapters always support attributes; parameter ignored for interface consistency return true; }tests/e2e/Adapter/Scopes/CollectionTests.php (1)
682-688: Consider clarifying the comment about MongoDB limitations.The change from a single-attribute to a multi-attribute index correctly addresses MongoDB's limitation (indicated by
getSupportForIdenticalIndexes()returningfalse). However, the comment "to solve the same attribute mongo issue" could be more descriptive.Consider expanding the comment to explain the specific limitation:
new Document([ - '$id' => ID::custom('idx_username_uid'), + '$id' => ID::custom('idx_username_uid'), 'type' => Database::INDEX_KEY, - 'attributes' => ['username', '$id'], // to solve the same attribute mongo issue + 'attributes' => ['username', '$id'], // MongoDB doesn't support multiple indexes on identical attributes; adding '$id' makes this index unique 'lengths' => [99, 200], 'orders' => [Database::ORDER_DESC], ]),This helps future maintainers understand why the second index requires multiple attributes.
📜 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)
src/Database/Adapter.php(4 hunks)src/Database/Adapter/Pool.php(4 hunks)src/Database/Adapter/SQL.php(4 hunks)src/Database/Database.php(39 hunks)src/Database/Validator/Index.php(5 hunks)src/Database/Validator/Query/Filter.php(4 hunks)src/Database/Validator/Sequence.php(1 hunks)src/Database/Validator/Structure.php(4 hunks)tests/e2e/Adapter/Base.php(2 hunks)tests/e2e/Adapter/Scopes/CollectionTests.php(1 hunks)tests/e2e/Adapter/Scopes/DocumentTests.php(27 hunks)tests/e2e/Adapter/Scopes/IndexTests.php(6 hunks)tests/e2e/Adapter/Scopes/PermissionTests.php(2 hunks)tests/unit/QueryTest.php(0 hunks)
💤 Files with no reviewable changes (1)
- tests/unit/QueryTest.php
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/e2e/Adapter/Base.php
- tests/e2e/Adapter/Scopes/PermissionTests.php
- src/Database/Adapter/Pool.php
- src/Database/Validator/Sequence.php
- src/Database/Validator/Index.php
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-07-30T19:17:53.630Z
Learnt from: ArnabChatterjee20k
PR: utopia-php/database#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:
src/Database/Validator/Structure.php
📚 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/DocumentTests.phptests/e2e/Adapter/Scopes/CollectionTests.php
🧬 Code graph analysis (7)
src/Database/Validator/Query/Filter.php (1)
src/Database/Database.php (1)
count(7128-7178)
tests/e2e/Adapter/Scopes/IndexTests.php (5)
src/Database/Database.php (1)
Database(37-8353)src/Database/Adapter/SQL.php (9)
getMaxIndexLength(1877-1883)getInternalIndexesKeys(1935-1938)getSupportForIndexArray(1465-1468)getSupportForSpatialIndexNull(1505-1508)getSupportForSpatialIndexOrder(1515-1518)getSupportForVectors(1595-1598)getSupportForAttributes(896-899)getSupportForMultipleFulltextIndexes(1535-1538)getSupportForIdenticalIndexes(1545-1548)src/Database/Adapter.php (9)
getMaxIndexLength(876-876)getInternalIndexesKeys(1305-1305)getSupportForIndexArray(942-942)getSupportForSpatialIndexNull(1078-1078)getSupportForSpatialIndexOrder(1092-1092)getSupportForVectors(1036-1036)getSupportForAttributes(921-921)getSupportForMultipleFulltextIndexes(1120-1120)getSupportForIdenticalIndexes(1128-1128)src/Database/Adapter/Mongo.php (9)
getMaxIndexLength(3092-3095)getInternalIndexesKeys(3110-3113)getSupportForIndexArray(2614-2617)getSupportForSpatialIndexNull(2887-2890)getSupportForSpatialIndexOrder(2908-2911)getSupportForVectors(2969-2972)getSupportForAttributes(2645-2648)getSupportForMultipleFulltextIndexes(2944-2947)getSupportForIdenticalIndexes(2954-2957)src/Database/Validator/Index.php (3)
isValid(395-428)getDescription(60-63)Index(10-453)
src/Database/Adapter.php (4)
src/Database/Adapter/SQL.php (11)
getMaxUIDLength(1888-1891)getSupportForMultipleFulltextIndexes(1535-1538)getSupportForIdenticalIndexes(1545-1548)getSupportForOrderRandom(1555-1558)getAttributeProjection(1987-2016)castingBefore(1570-1573)castingAfter(1575-1578)getSupportForInternalCasting(1525-1528)getSupportForUTCCasting(1560-1563)setUTCDatetime(1565-1568)setSupportForAttributes(2992-2995)src/Database/Adapter/Pool.php (11)
getMaxUIDLength(313-316)getSupportForMultipleFulltextIndexes(553-556)getSupportForIdenticalIndexes(558-561)getSupportForOrderRandom(563-566)getAttributeProjection(483-486)castingBefore(583-586)castingAfter(588-591)getSupportForInternalCasting(593-596)getSupportForUTCCasting(598-601)setUTCDatetime(603-606)setSupportForAttributes(608-611)src/Database/Adapter/Mongo.php (11)
getMaxUIDLength(3100-3103)getSupportForMultipleFulltextIndexes(2944-2947)getSupportForIdenticalIndexes(2954-2957)getSupportForOrderRandom(2964-2967)getAttributeProjection(2519-2544)castingBefore(1278-1330)castingAfter(1194-1269)getSupportForInternalCasting(2624-2627)getSupportForUTCCasting(2629-2632)setUTCDatetime(2634-2637)setSupportForAttributes(2650-2654)src/Database/Document.php (1)
Document(12-470)
src/Database/Adapter/SQL.php (4)
src/Database/Adapter.php (11)
getSupportForInternalCasting(1376-1376)getSupportForMultipleFulltextIndexes(1120-1120)getSupportForIdenticalIndexes(1128-1128)getSupportForOrderRandom(1135-1135)getSupportForUTCCasting(1383-1383)setUTCDatetime(1391-1391)castingBefore(1361-1361)castingAfter(1369-1369)getMaxUIDLength(883-883)getAttributeProjection(1200-1200)setSupportForAttributes(1399-1399)src/Database/Adapter/Pool.php (11)
getSupportForInternalCasting(593-596)getSupportForMultipleFulltextIndexes(553-556)getSupportForIdenticalIndexes(558-561)getSupportForOrderRandom(563-566)getSupportForUTCCasting(598-601)setUTCDatetime(603-606)castingBefore(583-586)castingAfter(588-591)getMaxUIDLength(313-316)getAttributeProjection(483-486)setSupportForAttributes(608-611)src/Database/Adapter/Mongo.php (11)
getSupportForInternalCasting(2624-2627)getSupportForMultipleFulltextIndexes(2944-2947)getSupportForIdenticalIndexes(2954-2957)getSupportForOrderRandom(2964-2967)getSupportForUTCCasting(2629-2632)setUTCDatetime(2634-2637)castingBefore(1278-1330)castingAfter(1194-1269)getMaxUIDLength(3100-3103)getAttributeProjection(2519-2544)setSupportForAttributes(2650-2654)src/Database/Document.php (1)
Document(12-470)
tests/e2e/Adapter/Scopes/DocumentTests.php (6)
src/Database/Database.php (3)
getAdapter(1250-1253)Database(37-8353)createDocument(4333-4430)src/Database/Adapter/SQL.php (4)
getIdAttributeType(1869-1872)getSupportForAttributes(896-899)getSupportForOrderRandom(1555-1558)getSupportForFulltextIndex(916-919)src/Database/Adapter.php (5)
getIdAttributeType(897-897)createDocument(701-701)getSupportForAttributes(921-921)getSupportForOrderRandom(1135-1135)getSupportForFulltextIndex(963-963)src/Database/Adapter/Pool.php (5)
getIdAttributeType(518-521)createDocument(228-231)getSupportForAttributes(328-331)getSupportForOrderRandom(563-566)getSupportForFulltextIndex(358-361)src/Database/Adapter/Mongo.php (5)
getIdAttributeType(3084-3087)createDocument(1159-1186)getSupportForAttributes(2645-2648)getSupportForOrderRandom(2964-2967)getSupportForFulltextIndex(2671-2674)src/Database/Document.php (4)
getId(63-66)getAttribute(224-231)Document(12-470)getSequence(71-80)
src/Database/Database.php (8)
src/Database/Adapter/SQL.php (14)
getMaxIndexLength(1877-1883)getInternalIndexesKeys(1935-1938)getSupportForIndexArray(1465-1468)getSupportForSpatialIndexNull(1505-1508)getSupportForSpatialIndexOrder(1515-1518)getSupportForVectors(1595-1598)getSupportForAttributes(896-899)getSupportForMultipleFulltextIndexes(1535-1538)getSupportForIdenticalIndexes(1545-1548)castingAfter(1575-1578)castingBefore(1570-1573)sum(2726-2789)getSupportForUTCCasting(1560-1563)setUTCDatetime(1565-1568)src/Database/Adapter.php (15)
getMaxIndexLength(876-876)getInternalIndexesKeys(1305-1305)getSupportForIndexArray(942-942)getSupportForSpatialIndexNull(1078-1078)getSupportForSpatialIndexOrder(1092-1092)getSupportForVectors(1036-1036)getSupportForAttributes(921-921)getSupportForMultipleFulltextIndexes(1120-1120)getSupportForIdenticalIndexes(1128-1128)castingAfter(1369-1369)castingBefore(1361-1361)updateDocument(725-725)sum(814-814)getSupportForUTCCasting(1383-1383)setUTCDatetime(1391-1391)src/Database/Adapter/Pool.php (15)
getMaxIndexLength(308-311)getInternalIndexesKeys(498-501)getSupportForIndexArray(343-346)getSupportForSpatialIndexNull(443-446)getSupportForSpatialIndexOrder(533-536)getSupportForVectors(413-416)getSupportForAttributes(328-331)getSupportForMultipleFulltextIndexes(553-556)getSupportForIdenticalIndexes(558-561)castingAfter(588-591)castingBefore(583-586)updateDocument(238-241)sum(268-271)getSupportForUTCCasting(598-601)setUTCDatetime(603-606)src/Database/Adapter/Mongo.php (15)
getMaxIndexLength(3092-3095)getInternalIndexesKeys(3110-3113)getSupportForIndexArray(2614-2617)getSupportForSpatialIndexNull(2887-2890)getSupportForSpatialIndexOrder(2908-2911)getSupportForVectors(2969-2972)getSupportForAttributes(2645-2648)getSupportForMultipleFulltextIndexes(2944-2947)getSupportForIdenticalIndexes(2954-2957)castingAfter(1194-1269)castingBefore(1278-1330)updateDocument(1432-1459)sum(2195-2237)getSupportForUTCCasting(2629-2632)setUTCDatetime(2634-2637)src/Database/Adapter/MariaDB.php (5)
getInternalIndexesKeys(1769-1772)getSupportForIndexArray(1842-1845)getSupportForSpatialIndexNull(1857-1860)getSupportForSpatialIndexOrder(1876-1879)updateDocument(948-1170)src/Database/Adapter/Postgres.php (4)
getSupportForSpatialIndexNull(2032-2035)getSupportForSpatialIndexOrder(2052-2055)getSupportForVectors(1960-1963)updateDocument(1091-1291)src/Database/Document.php (4)
getAttribute(224-231)setAttribute(244-261)Document(12-470)getArrayCopy(423-458)src/Database/Validator/Sequence.php (1)
isValid(38-59)
tests/e2e/Adapter/Scopes/CollectionTests.php (1)
src/Database/Database.php (1)
Database(37-8353)
🪛 PHPMD (2.15.0)
src/Database/Adapter/SQL.php
1570-1570: Avoid unused parameters such as '$collection'. (undefined)
(UnusedFormalParameter)
1575-1575: Avoid unused parameters such as '$collection'. (undefined)
(UnusedFormalParameter)
2992-2992: Avoid unused parameters such as '$support'. (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 (16)
src/Database/Validator/Structure.php (4)
104-111: LGTM! Capability flag correctly introduced.The
supportForAttributesparameter with a default value oftrueensures backward compatibility while enabling schemaless adapter support. The promoted constructor parameter pattern is clean and concise.
255-272: LGTM! Required attribute validation correctly gated.The early return when
supportForAttributesis false appropriately skips required attribute checks for schemaless adapters, preserving the full validation flow when true.
282-295: LGTM! Unknown attribute validation correctly gated.The early return appropriately allows arbitrary attributes for schemaless adapters while preserving validation for schema-based adapters.
371-376: LGTM! Unknown type handling correctly gated.The conditional error for unknown attribute types appropriately allows arbitrary types in schemaless mode while preserving type safety for schema-based adapters.
src/Database/Adapter/SQL.php (5)
1520-1558: LGTM! Capability methods correctly implemented.The SQL adapter appropriately returns:
falsefor internal casting (database handles type coercion)truefor multiple fulltext indexes, identical indexes, and random ordering (SQL features)These align with SQL database capabilities.
1560-1568: LGTM! UTC casting methods correctly stubbed.SQL adapters don't require UTC datetime transformations as the database handles datetime types natively. The no-op implementation is appropriate.
1570-1578: LGTM! Casting hooks correctly stubbed.The no-op implementations are appropriate for SQL adapters that don't require internal document casting. The unused
$collectionparameters are intentional for interface consistency with MongoDB's implementation.
1885-1891: LGTM! UID length correctly specified.The value of 36 correctly represents the standard UUID format length (8-4-4-4-12 with hyphens), appropriate for SQL VARCHAR constraints on ID fields.
1987-1987: LGTM! Return type correctly widened.Changing the return type from
stringtomixedappropriately accommodates different adapter implementations: SQL returns a projection string while MongoDB returns an array/object structure.tests/e2e/Adapter/Scopes/IndexTests.php (3)
165-177: LGTM! Validator instantiation updated correctly.The Index validator constructor now properly receives the three new capability flags from the adapter.
297-328: LGTM! Attribute validation tests properly guarded.The negative length and extra lengths validation tests are correctly wrapped in the
getSupportForAttributes()guard, as these validations only apply when attributes are supported.
523-566: LGTM! Multiple fulltext index validation test is well-structured.The test properly:
- Guards against adapters without fulltext support
- Checks the
getSupportForMultipleFulltextIndexes()capability- Validates expected behavior for both supporting and non-supporting adapters
- Cleans up resources in the finally block
src/Database/Adapter.php (4)
878-883: LGTM! New capability method for UID length.The
getMaxUIDLength()method provides adapter-specific maximum UID length. Implementations show SQL returns 36 (UUID standard) and Mongo returns 255.
1115-1136: LGTM! New capability flags for index and ordering features.The three new abstract methods properly expose adapter-specific capabilities:
getSupportForMultipleFulltextIndexes(): whether multiple fulltext indexes are allowedgetSupportForIdenticalIndexes(): whether duplicate/identical indexes are allowedgetSupportForOrderRandom(): whether random ordering is supportedThese align with the test usage in IndexTests.php and implementations in SQL/Mongo adapters.
1198-1200: LGTM! Return type widened to accommodate adapter variations.The return type change from
stringtomixedis necessary because:
- SQL adapter returns a string (comma-separated projection list)
- Mongo adapter returns an array (projection document)
This properly reflects the different projection formats across adapters.
1354-1400: LGTM! Casting abstraction for adapter-specific type handling.The new casting methods provide proper hooks for adapters to handle type conversions:
castingBefore()/castingAfter(): Transform documents pre-storage and post-retrievalgetSupportForInternalCasting()/getSupportForUTCCasting(): Capability flagssetUTCDatetime(): Adapter-specific datetime handling (e.g., Mongo's UTCDateTime)setSupportForAttributes(): Dynamic attribute support togglingThis abstraction allows Mongo to handle its UTCDateTime conversions while SQL can use no-op implementations.
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)
tests/e2e/Adapter/Scopes/DocumentTests.php (1)
1922-1927: Fix method name inconsistency:getSupportForFulltextWildCardIndex()→getSupportForFulltextWildcardIndex()The canonical method name is
getSupportForFulltextWildcardIndex()(single camel-case word). However, inconsistent calls exist in the test file:
- Line 1922: uses
getSupportForFulltextWildCardIndex()(uppercase 'C') — incorrect- Line 1971: uses
getSupportForFulltextWildcardIndex()— correct- Line 3410: uses
getSupportForFulltextWildCardIndex()(uppercase 'C') — incorrectFix lines 1922 and 3410 to use the canonical name
getSupportForFulltextWildcardIndex().
♻️ Duplicate comments (2)
tests/e2e/Adapter/Scopes/DocumentTests.php (2)
217-236: Schemaless adapters: stop expecting exceptions in these blocks
$this->fail('Failed to throw exception')runs even whengetSupportForAttributes() === false, breaking schemaless (e.g., MongoDB configured schemaless). Guard the expectation and use positive assertions on schemaless. Based on learningsApply this diff:
- try { - $database->createDocument('documents', new Document([ + 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) { - 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()); - } - } + ])); + $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.', $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()); + } @@ - try { + 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) { - 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()); - } - } + } catch (Throwable $e) { + $this->assertInstanceOf(StructureException::class, $e); + $this->assertStringContainsString('Attribute "bigint_unsigned" has invalid type', $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()); + } @@ - try { + 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) { - if ($database->getAdapter()->getSupportForAttributes()) { - $this->assertTrue($e instanceof StructureException); - $this->assertEquals('Invalid document structure: Attribute "$sequence" has invalid type. Invalid sequence value', $e->getMessage()); - } - } + } catch (Throwable $e) { + $this->assertInstanceOf(StructureException::class, $e); + $this->assertStringContainsString('Attribute "$sequence" has invalid type', $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()); + }Also applies to: 238-257, 259-281
964-974: Upsert mismatch: don’t expect errors on schemalessSame unconditional failure pattern here. On schemaless adapters, removing a “required” attribute should not error. Guard by capability and assert success otherwise. Based on learnings
- try { - $database->upsertDocuments(__FUNCTION__, [ - $existingDocument->removeAttribute('first'), - $newDocument - ]); - $this->fail('Failed to throw exception'); - } catch (Throwable $e) { - if ($database->getAdapter()->getSupportForAttributes()) { - $this->assertTrue($e instanceof StructureException, $e->getMessage()); - } - } + if ($database->getAdapter()->getSupportForAttributes()) { + try { + $database->upsertDocuments(__FUNCTION__, [ + $existingDocument->removeAttribute('first'), + $newDocument + ]); + $this->fail('Failed to throw exception'); + } catch (Throwable $e) { + $this->assertInstanceOf(StructureException::class, $e, $e->getMessage()); + } + } else { + $count = $database->upsertDocuments(__FUNCTION__, [ + $existingDocument->removeAttribute('first'), + $newDocument + ]); + $this->assertGreaterThan(0, $count); + }
🧹 Nitpick comments (2)
tests/e2e/Adapter/Scopes/DocumentTests.php (2)
114-117: Duplicate type assertion; likely missing bigint_unsigned type checkYou assert type for
bigint_signedtwice. Replace the second with an assertion forbigint_unsigned.- $this->assertIsInt($document->getAttribute('bigint_signed')); - $this->assertEquals(Database::MAX_BIG_INT, $document->getAttribute('bigint_unsigned')); + $this->assertIsInt($document->getAttribute('bigint_unsigned')); + $this->assertEquals(Database::MAX_BIG_INT, $document->getAttribute('bigint_unsigned'));
309-313: Normalize Query class casingUse
Query::...consistently to match project style and avoid static analysis noise.- $documentIdNull = $database->findOne('documents', [ - query::isNull('id') - ]); + $documentIdNull = $database->findOne('documents', [ + Query::isNull('id') + ]); @@ - $documentId0 = $database->findOne('documents', [ - query::equal('id', [$sequence]) - ]); + $documentId0 = $database->findOne('documents', [ + Query::equal('id', [$sequence]) + ]);Also applies to: 349-354
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/e2e/Adapter/Scopes/DocumentTests.php(27 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/DocumentTests.php
🧬 Code graph analysis (1)
tests/e2e/Adapter/Scopes/DocumentTests.php (6)
src/Database/Database.php (2)
getAdapter(1250-1253)Database(37-8353)src/Database/Adapter/SQL.php (4)
getIdAttributeType(1869-1872)getSupportForAttributes(896-899)getSupportForOrderRandom(1555-1558)getSupportForFulltextIndex(916-919)src/Database/Adapter.php (4)
getIdAttributeType(897-897)getSupportForAttributes(921-921)getSupportForOrderRandom(1135-1135)getSupportForFulltextIndex(963-963)src/Database/Adapter/Pool.php (4)
getIdAttributeType(518-521)getSupportForAttributes(328-331)getSupportForOrderRandom(563-566)getSupportForFulltextIndex(358-361)src/Database/Adapter/Mongo.php (4)
getIdAttributeType(3084-3087)getSupportForAttributes(2645-2648)getSupportForOrderRandom(2964-2967)getSupportForFulltextIndex(2671-2674)src/Database/Document.php (4)
getId(63-66)getAttribute(224-231)Document(12-470)getSequence(71-80)
⏰ 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 (Schemaless/MongoDB)
- GitHub Check: Adapter Tests (SharedTables/SQLite)
- GitHub Check: Adapter Tests (SharedTables/MongoDB)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (SharedTables/Postgres)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Adapter Tests (MySQL)
- GitHub Check: Adapter Tests (Postgres)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Adapter Tests (MariaDB)
- GitHub Check: Adapter Tests (MongoDB)
- GitHub Check: Unit Test
🔇 Additional comments (2)
tests/e2e/Adapter/Scopes/DocumentTests.php (2)
3557-3561: Good guard for adapters without orderRandomThe early return avoids false negatives on adapters that don’t support random ordering. LGTM.
699-700: Positive assertions on $id/$sequence improve test robustnessThe added
assertNotEmpty()checks on$id/$sequenceacross create/upsert/get flows are helpful and adapter-agnostic. LGTM.Also applies to: 735-737, 751-752, 1167-1167, 1206-1206, 1368-1368
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 (2)
src/Database/Validator/Index.php (2)
33-54: Initialize typed property $attributes before offset access.You're writing to $this->attributes[...] before it's initialized; typed array properties must be initialized first.
Apply this minimal fix in the constructor:
public function __construct( @@ protected bool $supportForIdenticalIndexes = true, ) { + $this->attributes = []; foreach ($attributes as $attribute) { $key = \strtolower($attribute->getAttribute('key', $attribute->getAttribute('$id'))); $this->attributes[$key] = $attribute; }Optional: also default-initialize at declaration:
protected array $attributes = [];
306-309: Typo in error message (stray quote).Remove the trailing quote.
- $this->message = 'Index length ' . $indexLength . ' is larger than the size for ' . $attributeName . ': ' . $attributeSize . '"'; + $this->message = 'Index length ' . $indexLength . ' is larger than the size for ' . $attributeName . ': ' . $attributeSize;
♻️ Duplicate comments (1)
src/Database/Validator/Index.php (1)
486-528: Identical-index guard must preserve order, include lengths, and skip self.Current logic treats attributes/orders as sets (array_diff), ignores lengths, and can match the same index by ID.
Apply this refactor:
- public function checkIdenticalIndexes(Document $index): bool + public function checkIdenticalIndexes(Document $index): bool { if ($this->supportForIdenticalIndexes) { return true; } - $indexAttributes = $index->getAttribute('attributes', []); - $indexOrders = $index->getAttribute('orders', []); - $indexType = $index->getAttribute('type', ''); + $indexAttributes = \array_map('strtolower', \array_values($index->getAttribute('attributes', []))); + $indexOrders = \array_values($index->getAttribute('orders', [])); + $indexLengths = \array_values($index->getAttribute('lengths', [])); + $indexType = $index->getAttribute('type', ''); foreach ($this->indexes as $existingIndex) { - $existingAttributes = $existingIndex->getAttribute('attributes', []); - $existingOrders = $existingIndex->getAttribute('orders', []); - $existingType = $existingIndex->getAttribute('type', ''); + if ($existingIndex->getId() === $index->getId()) { + continue; // skip self + } + $existingAttributes = \array_map('strtolower', \array_values($existingIndex->getAttribute('attributes', []))); + $existingOrders = \array_values($existingIndex->getAttribute('orders', [])); + $existingLengths = \array_values($existingIndex->getAttribute('lengths', [])); + $existingType = $existingIndex->getAttribute('type', ''); - $attributesMatch = false; - if (empty(\array_diff($existingAttributes, $indexAttributes)) && - empty(\array_diff($indexAttributes, $existingAttributes))) { - $attributesMatch = true; - } - - $ordersMatch = false; - if (empty(\array_diff($existingOrders, $indexOrders)) && - empty(\array_diff($indexOrders, $existingOrders))) { - $ordersMatch = true; - } + $attributesMatch = ($existingAttributes === $indexAttributes); + $ordersMatch = ($existingOrders === $indexOrders); if ($attributesMatch && $ordersMatch) { // Allow fulltext + key/unique combinations (different purposes) - $regularTypes = [Database::INDEX_KEY, Database::INDEX_UNIQUE]; - $isRegularIndex = \in_array($indexType, $regularTypes); - $isRegularExisting = \in_array($existingType, $regularTypes); + $regularTypes = [Database::INDEX_KEY, Database::INDEX_UNIQUE]; + $isRegularIndex = \in_array($indexType, $regularTypes, true); + $isRegularExisting = \in_array($existingType, $regularTypes, true); // Only reject if both are regular index types (key or unique) - if ($isRegularIndex && $isRegularExisting) { - $this->message = 'There is already an index with the same attributes and orders'; - return false; - } + if ($isRegularIndex && $isRegularExisting) { + $lengthsMatch = ($existingLengths === $indexLengths); + if ($lengthsMatch) { + $this->message = 'There is already an index with the same attributes, orders, and lengths'; + return false; + } + } } } return true; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Database/Validator/Index.php(10 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/Database/Validator/Index.php (1)
src/Database/Database.php (1)
Database(37-8353)
⏰ 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 (Schemaless/MongoDB)
- GitHub Check: Adapter Tests (SharedTables/SQLite)
- GitHub Check: Adapter Tests (SharedTables/Postgres)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (MariaDB)
- GitHub Check: Adapter Tests (MongoDB)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (Postgres)
- GitHub Check: Adapter Tests (MySQL)
- GitHub Check: Adapter Tests (SharedTables/MongoDB)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Unit Test
🔇 Additional comments (3)
src/Database/Validator/Index.php (3)
56-67: getType()/isArray(): LGTM.Return types align with Validator expectations (object, not array).
Also applies to: 77-88
21-31: Docblock updates: LGTM.Constructor params and flags are documented clearly.
139-148: Gated validations and specialized checks: LGTM.Attribute gating, array rules, spatial/vector constraints, and the fulltext multiplicity guard read correctly and match adapter-capability flags.
Also applies to: 188-203, 209-256, 344-382, 388-410, 417-455, 461-480
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)
src/Database/Validator/Index.php (2)
17-17: Typed property $attributes used before initialization (fatal at runtime).Initialize the typed array property before offset writes in the constructor.
- protected array $attributes; + protected array $attributes = [];
309-312: Stray double quote in error message.Removes a trailing quote.
- $this->message = 'Index length ' . $indexLength . ' is larger than the size for ' . $attributeName . ': ' . $attributeSize . '"'; + $this->message = 'Index length ' . $indexLength . ' is larger than the size for ' . $attributeName . ': ' . $attributeSize;
♻️ Duplicate comments (1)
src/Database/Validator/Index.php (1)
489-531: Identical-index comparison ignores order/duplicates and lengths; also misses same-id skip and over-restricts to key/unique.Compare attributes, orders (and lengths for regular indexes) positionally with normalized casing; skip same-id; allow only the fulltext vs key/unique combo, reject all other identical combos. This prevents false positives/negatives.
- $indexAttributes = $index->getAttribute('attributes', []); - $indexOrders = $index->getAttribute('orders', []); - $indexType = $index->getAttribute('type', ''); + $indexAttributes = \array_values(\array_map('strtolower', (array)$index->getAttribute('attributes', []))); + $indexOrders = \array_values(\array_map('strtolower', (array)$index->getAttribute('orders', []))); + $indexLengths = \array_values((array)$index->getAttribute('lengths', [])); + $indexType = $index->getAttribute('type', ''); - foreach ($this->indexes as $existingIndex) { - $existingAttributes = $existingIndex->getAttribute('attributes', []); - $existingOrders = $existingIndex->getAttribute('orders', []); - $existingType = $existingIndex->getAttribute('type', ''); + foreach ($this->indexes as $existingIndex) { + if ($existingIndex->getId() === $index->getId()) { + continue; + } + $existingAttributes = \array_values(\array_map('strtolower', (array)$existingIndex->getAttribute('attributes', []))); + $existingOrders = \array_values(\array_map('strtolower', (array)$existingIndex->getAttribute('orders', []))); + $existingLengths = \array_values((array)$existingIndex->getAttribute('lengths', [])); + $existingType = $existingIndex->getAttribute('type', ''); - $attributesMatch = false; - if (empty(\array_diff($existingAttributes, $indexAttributes)) && - empty(\array_diff($indexAttributes, $existingAttributes))) { - $attributesMatch = true; - } - - $ordersMatch = false; - if (empty(\array_diff($existingOrders, $indexOrders)) && - empty(\array_diff($indexOrders, $existingOrders))) { - $ordersMatch = true; - } - - if ($attributesMatch && $ordersMatch) { - // Allow fulltext + key/unique combinations (different purposes) - $regularTypes = [Database::INDEX_KEY, Database::INDEX_UNIQUE]; - $isRegularIndex = \in_array($indexType, $regularTypes); - $isRegularExisting = \in_array($existingType, $regularTypes); - - // Only reject if both are regular index types (key or unique) - if ($isRegularIndex && $isRegularExisting) { - $this->message = 'There is already an index with the same attributes and orders'; - return false; - } - } + $attributesMatch = ($existingAttributes === $indexAttributes); + $ordersMatch = ($existingOrders === $indexOrders); + + if ($attributesMatch && $ordersMatch) { + // Allow fulltext + key/unique combinations (different purposes) + $regularTypes = [Database::INDEX_KEY, Database::INDEX_UNIQUE]; + $isRegularIndex = \in_array($indexType, $regularTypes, true); + $isRegularExisting = \in_array($existingType, $regularTypes, true); + $oneIsFulltextAndOtherRegular = + ($indexType === Database::INDEX_FULLTEXT && $isRegularExisting) || + ($existingType === Database::INDEX_FULLTEXT && $isRegularIndex); + + if ($oneIsFulltextAndOtherRegular) { + continue; + } + + // For regular indexes, also require lengths to match exactly (positional). + if ($isRegularIndex && $isRegularExisting) { + $lengthsMatch = ($existingLengths === $indexLengths); + if ($lengthsMatch) { + $this->message = 'There is already an index with the same attributes, orders, and lengths'; + return false; + } + continue; + } + + // For all other types, identical attributes+orders are considered duplicates. + $this->message = 'There is already an index with the same attributes and orders'; + return false; + } }
🧹 Nitpick comments (1)
src/Database/Validator/Index.php (1)
176-179: Use strict comparison for duplicate attribute detection.Avoids edge cases with loose comparisons (e.g., "1" vs 1).
- if (\in_array($value, $stack)) { + if (\in_array($value, $stack, true)) {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Database/Validator/Index.php(10 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/Database/Validator/Index.php (1)
src/Database/Database.php (1)
Database(37-8353)
⏰ 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 (Schemaless/MongoDB)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (SharedTables/MongoDB)
- GitHub Check: Adapter Tests (SharedTables/Postgres)
- GitHub Check: Adapter Tests (SharedTables/SQLite)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Adapter Tests (MySQL)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (Postgres)
- GitHub Check: Adapter Tests (MariaDB)
- GitHub Check: Adapter Tests (MongoDB)
- GitHub Check: Unit Test
🔇 Additional comments (1)
src/Database/Validator/Index.php (1)
132-134: Good: identical-index check now included in the validation flow.This closes the previous gap where duplicates could slip through.
Summary by CodeRabbit
New Features
DevOps
Chores
Tests