Mongo ttl index support #779
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughTTL (Time-To-Live) index support is introduced across the database layer. Method signatures are extended to accept collation and TTL parameters. Adapters gain a capability check method. TTL handling logic is implemented in the Mongo adapter and validator, with parameter propagation in other adapters and core database components. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/Database/Adapter/MariaDB.php (1)
718-781: Fix PHPMD “unused parameter” warnings for createIndex().
Right now$indexAttributeTypes,$collation,$ttlare unused (and static analysis already flags this), which may fail CI.Proposed minimal fix (no behavior change)
public function createIndex(string $collection, string $id, string $type, array $attributes, array $lengths, array $orders, array $indexAttributeTypes = [], array $collation = [], int $ttl = 0): bool { + // Adapter doesn't use these yet; keep signature aligned with Adapter API. + unset($indexAttributeTypes, $collation, $ttl); + $metadataCollection = new Document(['$id' => Database::METADATA]); $collection = $this->getDocument($metadataCollection, $collection);src/Database/Mirror.php (1)
481-506: Bug: TTL value not included in document, causing data loss during mirroring.The
ttlattribute is not set when creating the document (lines 481-487), but line 506 attempts to read it. This will always return the default value0, effectively losing the TTL configuration when mirroring indexes to the destination database.🐛 Proposed fix
$document = new Document([ '$id' => $id, 'type' => $type, 'attributes' => $attributes, 'lengths' => $lengths, 'orders' => $orders, + 'ttl' => $ttl, ]);src/Database/Database.php (1)
3600-3730: Add defensive TTL validation in createIndex() even when validation is disabled.The
checkTTLIndexes()validator is only applied when$this->validateis true (line 3682), allowing invalid TTL values to bypass validation and reach the adapter. When validation is disabled, negative TTL values, TTL on non-TTL indexes, or TTL on adapters without support will only fail at the adapter level instead of being caught early.Add these checks before creating the index:
Proposed fix
public function createIndex(string $collection, string $id, string $type, array $attributes, array $lengths = [], array $orders = [], int $ttl = 0): bool { + if ($ttl < 0) { + throw new DatabaseException('TTL must be greater than or equal to 0'); + } + + if ($type === self::INDEX_TTL && !$this->adapter->getSupportTTLIndexes()) { + throw new DatabaseException('TTL indexes are not supported by the current adapter'); + } + + if ($type !== self::INDEX_TTL && $ttl !== 0) { + throw new DatabaseException('TTL can only be set for ttl indexes'); + } + if (empty($attributes)) { throw new DatabaseException('Missing attributes'); }The adapter method signature matches the call:
createIndex(..., $indexAttributesWithTypes, [], $ttl)correctly passes all 9 arguments in the expected order.
🤖 Fix all issues with AI agents
In @src/Database/Validator/Index.php:
- Around line 774-777: Fix the typo in the validation message: change the text
assigned to $this->message from 'TTL must be atleast 1 second' to 'TTL must be
at least 1 second' in the validation logic (the block that checks if ($ttl <=
0)) so the error message is grammatically correct; update the string where it's
set (reference: $this->message in Index.php's TTL validation).
- Around line 755-758: The error message set in the Index validator is
misleading: replace the current message "TTL index can be created on a single
object attribute" with one that correctly references datetime attributes (e.g.,
"TTL index can be created on a single datetime attribute") so that when the
validation in the Index validator (the block setting $this->message and
returning false) triggers, it accurately reflects that TTL indexes must target a
single datetime attribute (see the later datetime validation in this validator).
In @tests/unit/Validator/IndexTest.php:
- Around line 692-703: The assertion checks the wrong error text for the
TTL-on-non-datetime index case: update the expected substring in the test that
validates failure for the Document with 'type' => Database::INDEX_TTL (created
as $invalidIndexType) to match the TTL-specific message returned by the
validator (e.g., "TTL index can only be created on datetime attributes") by
replacing the current 'Object index can only be created on datetime attributes'
with the TTL wording when calling $this->assertStringContainsString(...,
$validator->getDescription()).
🧹 Nitpick comments (4)
tests/unit/Validator/IndexTest.php (1)
636-655: Prefer named args for Index validator construction (too many positional booleans).
This test is fragile to constructor re-ordering and hard to review.Example refactor (adjust names to actual signature)
- $validator = new Index( - $collection->getAttribute('attributes'), - $collection->getAttribute('indexes', []), - 768, - [], - false, // supportForArrayIndexes - false, // supportForSpatialIndexNull - false, // supportForSpatialIndexOrder - false, // supportForVectorIndexes - true, // supportForAttributes - true, // supportForMultipleFulltextIndexes - true, // supportForIdenticalIndexes - false, // supportForObjectIndexes - false, // supportForTrigramIndexes - false, // supportForSpatialIndexes - true, // supportForKeyIndexes - true, // supportForUniqueIndexes - true, // supportForFulltextIndexes - true // supportForTTLIndexes - ); + $validator = new Index( + attributes: $collection->getAttribute('attributes'), + indexes: $collection->getAttribute('indexes', []), + maxLength: 768, + reservedKeys: [], + supportForArrayIndexes: false, + supportForSpatialIndexNull: false, + supportForSpatialIndexOrder: false, + supportForVectorIndexes: false, + supportForAttributes: true, + supportForMultipleFulltextIndexes: true, + supportForIdenticalIndexes: true, + supportForObjectIndexes: false, + supportForTrigramIndexes: false, + supportForSpatialIndexes: false, + supportForKeyIndexes: true, + supportForUniqueIndexes: true, + supportForFulltextIndexes: true, + supportForTTLIndexes: true, + );tests/e2e/Adapter/Scopes/DocumentTests.php (2)
7431-7543: MaketestTTLIndexesdeterministic (auth state, datetime filter, and cleanup).This test currently relies on ambient Authorization state and doesn’t guarantee cleanup if an assertion fails; also the
expiresAtattribute is created without an explicitdatetimefilter (but later you do include it via the attributeDocument).Proposed hardening diff
public function testTTLIndexes(): void { /** @var Database $database */ $database = static::getDatabase(); if (!$database->getAdapter()->getSupportTTLIndexes()) { $this->expectNotToPerformAssertions(); return; } - $col = uniqid('sl_ttl'); - $database->createCollection($col); - - $database->createAttribute($col, 'expiresAt', Database::VAR_DATETIME, 0, false); + Authorization::cleanRoles(); + Authorization::setRole(Role::any()->toString()); + + $col = 'sl_ttl_' . ID::unique(); + $col2 = 'sl_ttl_collection_' . ID::unique(); + + try { + $database->createCollection($col); + + $database->createAttribute( + $col, + 'expiresAt', + Database::VAR_DATETIME, + 0, + false, + filters: ['datetime'] + ); $permissions = [ Permission::read(Role::any()), Permission::write(Role::any()), Permission::update(Role::any()), Permission::delete(Role::any()) ]; $this->assertTrue( $database->createIndex( $col, 'idx_ttl_valid', Database::INDEX_TTL, ['expiresAt'], [], [Database::ORDER_ASC], 3600 // 1 hour TTL ) ); $collection = $database->getCollection($col); $indexes = $collection->getAttribute('indexes'); $this->assertCount(1, $indexes); $ttlIndex = $indexes[0]; $this->assertEquals('idx_ttl_valid', $ttlIndex->getId()); $this->assertEquals(Database::INDEX_TTL, $ttlIndex->getAttribute('type')); $this->assertEquals(3600, $ttlIndex->getAttribute('ttl')); $now = new \DateTime(); $future1 = (clone $now)->modify('+2 hours'); $future2 = (clone $now)->modify('+1 hour'); $past = (clone $now)->modify('-1 hour'); $database->createDocuments($col, [ new Document([ '$id' => 'doc1', '$permissions' => $permissions, 'expiresAt' => $future1->format(\DateTime::ATOM), ]), new Document([ '$id' => 'doc2', '$permissions' => $permissions, 'expiresAt' => $future2->format(\DateTime::ATOM), ]), new Document([ '$id' => 'doc3', '$permissions' => $permissions, 'expiresAt' => $past->format(\DateTime::ATOM), ]) ]); $this->assertTrue($database->deleteIndex($col, 'idx_ttl_valid')); $this->assertTrue( $database->createIndex( $col, 'idx_ttl_min', Database::INDEX_TTL, ['expiresAt'], [], [Database::ORDER_ASC], 1 // Minimum TTL ) ); - $col2 = uniqid('sl_ttl_collection'); - $expiresAtAttr = new Document([ '$id' => ID::custom('expiresAt'), 'type' => Database::VAR_DATETIME, 'size' => 0, 'signed' => false, 'required' => false, 'default' => null, 'array' => false, 'filters' => ['datetime'], ]); $ttlIndexDoc = new Document([ '$id' => ID::custom('idx_ttl_collection'), 'type' => Database::INDEX_TTL, 'attributes' => ['expiresAt'], 'lengths' => [], 'orders' => [Database::ORDER_ASC], 'ttl' => 7200 // 2 hours ]); $database->createCollection($col2, [$expiresAtAttr], [$ttlIndexDoc]); $collection2 = $database->getCollection($col2); $indexes2 = $collection2->getAttribute('indexes'); $this->assertCount(1, $indexes2); $ttlIndex2 = $indexes2[0]; $this->assertEquals('idx_ttl_collection', $ttlIndex2->getId()); $this->assertEquals(7200, $ttlIndex2->getAttribute('ttl')); - $database->deleteCollection($col); - $database->deleteCollection($col2); + } finally { + // Best-effort cleanup (avoid leaking collections on assertion failures) + try { $database->deleteCollection($col); } catch (\Throwable) {} + try { $database->deleteCollection($col2); } catch (\Throwable) {} + Authorization::cleanRoles(); + } }
7545-7690: Reduce brittleness in duplicate-TTL assertions + ensure$col3can’t leak on unexpected success.The
createCollectionduplicate test hard-codes a very specific error message (“Index already exists”) and doesn’t clean up$col3if the call unexpectedly succeeds (it wouldfail()before cleanup).Suggested diff
- try { - $database->createCollection($col3, [$expiresAtAttr], [$ttlIndex1, $ttlIndex2]); - $this->fail('Expected exception for duplicate TTL indexes in createCollection'); - } catch (Exception $e) { - $this->assertInstanceOf(DatabaseException::class, $e); - // raised in the mongo level - $this->assertStringContainsString('Index already exists', $e->getMessage()); - } + try { + $database->createCollection($col3, [$expiresAtAttr], [$ttlIndex1, $ttlIndex2]); + // If we got here, make sure we don't leak the collection before failing the test. + $database->deleteCollection($col3); + $this->fail('Expected exception for duplicate TTL indexes in createCollection'); + } catch (DatabaseException $e) { + // Raised in adapter/validator layer; message may vary. + $this->assertStringContainsString('already', strtolower($e->getMessage())); + }src/Database/Database.php (1)
1633-1658: RiskyIndexValidatorctor wiring: please verify all call sites + consider named args.
Adding one more boolean at the tail is easy to mis-order across the repo; a single missed instantiation will be a runtime fatal. Also, this constructor is a prime candidate for named arguments or a small config DTO to prevent future footguns.
Based on learnings, Mongo’sgetMaxIndexLength()should be non-zero (1,048,576 bytes) so the length normalization logic/validation behaves as intended.#!/bin/bash set -euo pipefail echo "== IndexValidator constructor signature ==" rg -n "class\s+Index\b" src/Database/Validator/Index.php || true rg -n "function __construct\s*\(" src/Database/Validator/Index.php || true echo echo "== All IndexValidator instantiations (ensure extra arg was added everywhere) ==" rg -n "new\s+IndexValidator\s*\(" src -S echo echo "== Adapter capability method existence ==" rg -n "function\s+getSupportTTLIndexes\s*\(" src -S
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
src/Database/Adapter.phpsrc/Database/Adapter/MariaDB.phpsrc/Database/Adapter/Mongo.phpsrc/Database/Adapter/MySQL.phpsrc/Database/Adapter/Pool.phpsrc/Database/Adapter/Postgres.phpsrc/Database/Adapter/SQLite.phpsrc/Database/Database.phpsrc/Database/Mirror.phpsrc/Database/Validator/Index.phptests/e2e/Adapter/Scopes/DocumentTests.phptests/e2e/Adapter/Scopes/SchemalessTests.phptests/unit/Validator/IndexTest.php
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
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.
📚 Learning: 2025-10-03T02:04:17.803Z
Learnt from: abnegate
Repo: utopia-php/database PR: 721
File: tests/e2e/Adapter/Scopes/DocumentTests.php:6418-6439
Timestamp: 2025-10-03T02:04:17.803Z
Learning: In tests/e2e/Adapter/Scopes/DocumentTests::testSchemalessDocumentInvalidInteralAttributeValidation (PHP), when the adapter reports getSupportForAttributes() === false (schemaless), the test should not expect exceptions from createDocuments for “invalid” internal attributes; remove try/catch and ensure the test passes without exceptions, keeping at least one assertion.
Applied to files:
tests/e2e/Adapter/Scopes/SchemalessTests.phptests/unit/Validator/IndexTest.phptests/e2e/Adapter/Scopes/DocumentTests.php
📚 Learning: 2025-10-16T09:37:33.531Z
Learnt from: fogelito
Repo: utopia-php/database PR: 733
File: src/Database/Adapter/MariaDB.php:1801-1806
Timestamp: 2025-10-16T09:37:33.531Z
Learning: In the MariaDB adapter (src/Database/Adapter/MariaDB.php), only duplicate `_uid` violations should throw `DuplicateException`. All other unique constraint violations, including `PRIMARY` key collisions on the internal `_id` field, should throw `UniqueException`. This is the intended design to distinguish between user-facing document duplicates and internal/user-defined unique constraint violations.
Applied to files:
tests/e2e/Adapter/Scopes/SchemalessTests.php
📚 Learning: 2025-10-29T12:27:57.071Z
Learnt from: ArnabChatterjee20k
Repo: utopia-php/database PR: 747
File: src/Database/Adapter/Mongo.php:1449-1453
Timestamp: 2025-10-29T12:27:57.071Z
Learning: In src/Database/Adapter/Mongo.php, when getSupportForAttributes() returns false (schemaless mode), the updateDocument method intentionally uses a raw document without $set operator for replacement-style updates, as confirmed by the repository maintainer ArnabChatterjee20k.
Applied to files:
src/Database/Adapter/Mongo.php
📚 Learning: 2025-10-16T08:48:36.715Z
Learnt from: fogelito
Repo: utopia-php/database PR: 739
File: src/Database/Adapter/Postgres.php:154-158
Timestamp: 2025-10-16T08:48:36.715Z
Learning: For the utopia-php/database repository, no migration scripts are needed for the collation change from utf8_ci to utf8_ci_ai in the Postgres adapter because there is no existing production data.
Applied to files:
src/Database/Adapter/Postgres.php
🧬 Code graph analysis (9)
tests/e2e/Adapter/Scopes/SchemalessTests.php (4)
src/Database/Database.php (2)
Database(37-8799)createIndex(3620-3730)src/Database/Adapter.php (2)
getSupportForAttributes(925-925)createIndex(674-674)src/Database/Adapter/SQL.php (1)
getSupportForAttributes(949-952)src/Database/Adapter/SQLite.php (1)
createIndex(459-484)
src/Database/Mirror.php (3)
src/Database/Adapter/Pool.php (1)
createIndex(213-216)src/Database/Adapter.php (1)
createIndex(674-674)src/Database/Database.php (1)
createIndex(3620-3730)
src/Database/Adapter/MySQL.php (6)
src/Database/Adapter/MariaDB.php (1)
getSupportTTLIndexes(2259-2262)src/Database/Adapter/Pool.php (1)
getSupportTTLIndexes(653-656)src/Database/Adapter/Postgres.php (1)
getSupportTTLIndexes(2836-2839)src/Database/Adapter/SQLite.php (1)
getSupportTTLIndexes(1913-1916)src/Database/Adapter.php (1)
getSupportTTLIndexes(1494-1497)src/Database/Adapter/Mongo.php (1)
getSupportTTLIndexes(3418-3421)
src/Database/Adapter/Pool.php (6)
src/Database/Adapter/MariaDB.php (2)
createIndex(718-781)getSupportTTLIndexes(2259-2262)src/Database/Adapter/Postgres.php (2)
createIndex(882-945)getSupportTTLIndexes(2836-2839)src/Database/Adapter/SQLite.php (2)
createIndex(459-484)getSupportTTLIndexes(1913-1916)src/Database/Adapter.php (2)
createIndex(674-674)getSupportTTLIndexes(1494-1497)src/Database/Adapter/Mongo.php (2)
createIndex(919-1047)getSupportTTLIndexes(3418-3421)src/Database/Adapter/MySQL.php (1)
getSupportTTLIndexes(321-324)
src/Database/Adapter/Mongo.php (5)
src/Database/Adapter.php (3)
filter(1257-1266)createIndex(674-674)getSupportTTLIndexes(1494-1497)src/Database/Adapter/MariaDB.php (2)
createIndex(718-781)getSupportTTLIndexes(2259-2262)src/Database/Adapter/Pool.php (2)
createIndex(213-216)getSupportTTLIndexes(653-656)src/Database/Mirror.php (1)
createIndex(472-513)src/Database/Adapter/MySQL.php (1)
getSupportTTLIndexes(321-324)
tests/unit/Validator/IndexTest.php (2)
src/Database/Database.php (1)
Database(37-8799)src/Database/Validator/Index.php (3)
Index(10-796)isValid(110-164)getDescription(85-88)
src/Database/Validator/Index.php (1)
src/Database/Database.php (1)
Database(37-8799)
tests/e2e/Adapter/Scopes/DocumentTests.php (6)
src/Database/Adapter/MariaDB.php (1)
getSupportTTLIndexes(2259-2262)src/Database/Adapter/MySQL.php (1)
getSupportTTLIndexes(321-324)src/Database/Adapter/Pool.php (1)
getSupportTTLIndexes(653-656)src/Database/Adapter/Postgres.php (1)
getSupportTTLIndexes(2836-2839)src/Database/Adapter/SQLite.php (1)
getSupportTTLIndexes(1913-1916)src/Database/Adapter/Mongo.php (1)
getSupportTTLIndexes(3418-3421)
src/Database/Adapter/Postgres.php (2)
src/Database/Adapter.php (2)
createIndex(674-674)getSupportTTLIndexes(1494-1497)src/Database/Database.php (1)
createIndex(3620-3730)
🪛 PHPMD (2.15.0)
src/Database/Adapter/MariaDB.php
718-718: Avoid unused parameters such as '$indexAttributeTypes'. (undefined)
(UnusedFormalParameter)
718-718: Avoid unused parameters such as '$collation'. (undefined)
(UnusedFormalParameter)
718-718: Avoid unused parameters such as '$ttl'. (undefined)
(UnusedFormalParameter)
src/Database/Adapter/Mongo.php
919-919: Avoid unused parameters such as '$lengths'. (undefined)
(UnusedFormalParameter)
src/Database/Adapter/SQLite.php
459-459: Avoid unused parameters such as '$lengths'. (undefined)
(UnusedFormalParameter)
459-459: Avoid unused parameters such as '$orders'. (undefined)
(UnusedFormalParameter)
459-459: Avoid unused parameters such as '$indexAttributeTypes'. (undefined)
(UnusedFormalParameter)
459-459: Avoid unused parameters such as '$collation'. (undefined)
(UnusedFormalParameter)
459-459: Avoid unused parameters such as '$ttl'. (undefined)
(UnusedFormalParameter)
src/Database/Adapter/Postgres.php
882-882: Avoid unused parameters such as '$lengths'. (undefined)
(UnusedFormalParameter)
882-882: Avoid unused parameters such as '$indexAttributeTypes'. (undefined)
(UnusedFormalParameter)
882-882: Avoid unused parameters such as '$collation'. (undefined)
(UnusedFormalParameter)
882-882: Avoid unused parameters such as '$ttl'. (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). (8)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Adapter Tests (Postgres)
- GitHub Check: Adapter Tests (MariaDB)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (MySQL)
- GitHub Check: Adapter Tests (SharedTables/Postgres)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
🔇 Additional comments (21)
src/Database/Adapter/MySQL.php (1)
321-324: TTL capability flag is correctly exposed for MySQL (false).
This aligns with the adapter-wide API surface (Mongo true; others false).src/Database/Adapter/Pool.php (2)
213-216: Pool::createIndex correctly forwards the expanded signature.
Minor note: callers must pass TTL as the last argument (or use named args) to avoid shifting into $indexAttributeTypes/$collation.
653-656: Pool::getSupportTTLIndexes delegation looks correct.src/Database/Adapter/MariaDB.php (1)
2259-2262: TTL capability flag is correctly exposed for MariaDB (false).tests/e2e/Adapter/Scopes/SchemalessTests.php (2)
9-10: New DatabaseException import alias is fine.
1870-2130: No changes needed. The createIndex calls are correct.The Database::createIndex signature confirms
ttlis properly exposed as the 7th positional parameter (int $ttl = 0), and all calls in the test pass the TTL value (3600, 7200, etc.) in the correct position:createIndex($col, $id, Database::INDEX_TTL, $attributes, [], $orders, $ttl).tests/unit/Validator/IndexTest.php (1)
773-776: The TTL support flag is correctly targeted.The instantiation passes 13 positional arguments after
maxLength, which maps up tosupportForTrigramIndexes. The remaining parameters, includingsupportForTTLIndexes, use their default value offalse, which is the correct setting for a "no support" validator. The test assertion'TTL indexes are not supported'matches the expected error message at line 231 in the validator.However, note that the code comment on line 773 says "Validator with supportForTrigramIndexes disabled" when it should say "supportForTTLIndexes disabled"—the comment is misleading, though the test logic itself is correct.
src/Database/Adapter.php (2)
669-674: LGTM!The abstract method signature is properly extended with optional parameters at the end, maintaining backward compatibility. The PHPDoc types are correctly specified.
1488-1497: LGTM!The default implementation returning
falseis appropriate for a capability flag. Adapters that support TTL indexes (like MongoDB) can override this method to returntrue.src/Database/Adapter/Postgres.php (3)
322-336: LGTM!The TTL attribute extraction and propagation to
createIndexis correctly implemented. This ensures consistent plumbing across all adapters, even though PostgreSQL doesn't natively support TTL indexes.
882-882: Unused parameters are expected for interface compliance.The
$collationand$ttlparameters (along with pre-existing$lengthsand$indexAttributeTypes) are unused because PostgreSQL doesn't support TTL indexes natively. This is intentional - the signature must match the abstract method inAdapter.php. The static analysis warnings can be safely ignored in this context.
2835-2839: LGTM!Correctly returns
falseas PostgreSQL doesn't support TTL indexes natively.src/Database/Adapter/SQLite.php (3)
219-221: LGTM!The TTL attribute extraction and propagation follows the same pattern as other adapters, maintaining consistency across the codebase.
459-459: Unused parameters are expected for interface compliance.The
$collationand$ttlparameters (along with pre-existing$lengths,$orders, and$indexAttributeTypes) are unused because SQLite doesn't support TTL indexes. This matches the expected pattern for adapters that don't implement this feature. The static analysis warnings can be safely ignored.
1912-1916: LGTM!Correctly returns
falseas SQLite doesn't support TTL indexes.src/Database/Validator/Index.php (1)
779-792: Verify duplicate TTL index detection logic.The condition
$this->supportForAttributes && $existingType !== Database::INDEX_TTLat line 783 means duplicate detection is skipped for non-TTL indexes only whensupportForAttributesis true. This coupling seems unintentional — duplicate TTL index detection should likely occur regardless of thesupportForAttributesflag.Consider simplifying the condition to only skip non-TTL indexes:
- if ($this->supportForAttributes && $existingType !== Database::INDEX_TTL) { + if ($existingType !== Database::INDEX_TTL) { continue; }src/Database/Adapter/Mongo.php (4)
511-538: LGTM! TTL index support correctly implemented for collection creation.The TTL index handling follows MongoDB's
expireAfterSecondspattern correctly, and the case handling integrates well with existing index type processing.
915-981: TTL index creation correctly implemented.The implementation properly sets
expireAfterSecondswhen$ttl > 0for MongoDB TTL indexes.Note: The
$lengthsparameter is intentionally unused as MongoDB doesn't support index prefix lengths like SQL databases. This is consistent with the adapter's design.
1093-1098: LGTM! TTL value preserved during index rename.The
renameIndexmethod correctly retrieves and passes the existing TTL value when recreating the index with the new name.
3417-3421: LGTM! TTL support capability correctly exposed.This enables the validator to properly gate TTL index creation for MongoDB.
src/Database/Database.php (1)
79-90:INDEX_TTLconstant addition looks good (no collision with cacheTTL).
| return false; | ||
| } | ||
|
|
||
| public function getSupportTTLIndexes(): bool |
There was a problem hiding this comment.
Let's make sure method names are always consistent: getSupportForTTLIndexes
|
|
||
| // If sharedTables, always add _tenant as the first key | ||
| if ($this->sharedTables) { | ||
| if ($this->sharedTables && $index->getAttribute('type') !== Database::INDEX_TTL) { |
There was a problem hiding this comment.
Let's make this check an (adapter?) method, shouldAddTenantToIndex
| } | ||
|
|
||
| if (count($attributes) !== 1) { | ||
| $this->message = 'TTL index can be created on a single datetime attribute'; |
There was a problem hiding this comment.
| $this->message = 'TTL index can be created on a single datetime attribute'; | |
| $this->message = 'TTL indexes must be created on a single datetime attribute.'; |
| if (empty($orders)) { | ||
| $this->message = 'TTL index need explicit orders. Add the orders to create this index.'; | ||
| return false; | ||
| } |
| if ($ttl <= 0) { | ||
| $this->message = 'TTL must be at least 1 second'; | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Won't this disallow indexes without a TTL since 0 is the default?
| ); | ||
| $this->fail('Expected exception for duplicate TTL index on same attribute'); | ||
| } catch (Exception $e) { | ||
| $this->assertInstanceOf(DatabaseException::class, $e); |
There was a problem hiding this comment.
This should be an IndexException
| } | ||
| } | ||
|
|
||
| public function testTTLIndexes(): void |
There was a problem hiding this comment.
I don't see where we test that the documents were removed after TTL expiry
There was a problem hiding this comment.
Mongo doesn't directly delete after the ttl, it takes time to delete. Was getting tooo flaky
|
Keeping it in draft mode due to the datetime determination issue in schemaless |
Summary by CodeRabbit
Release Notes
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.