Mongo ttl index support #779
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds per-index TTL support across the database layer: expands Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant Database
participant Validator as IndexValidator
participant Adapter
participant Storage as StorageEngine
Client->>Database: createIndex(collection, id, type="ttl", attributes..., ttl)
Database->>Validator: validate index (includes TTL)
Validator->>Validator: checkTTLIndexes()
alt invalid TTL
Validator-->>Database: validation error
Database-->>Client: IndexException
else valid TTL
Database->>Adapter: createIndex(..., collation, ttl)
Adapter->>Storage: configure index (maybe expireAfterSeconds)
Storage-->>Adapter: success
Adapter-->>Database: success
Database->>Database: persist index metadata (includes ttl)
Database-->>Client: OK
end
sequenceDiagram
actor Client
participant Database
participant Adapter
participant Cache
Client->>Database: getDocument(collection, id)
Database->>Database: detect TTL index for collection
alt TTL index present
Database->>Adapter: fetch document
Adapter-->>Database: document
Database->>Database: read ttl attribute & seconds
Database->>Database: parse datetime & compute expiry
alt expired
Database-->>Cache: remove cached entry
Database-->>Client: empty document
else not expired
Database->>Cache: store/update
Database-->>Client: document
end
else no TTL
Database->>Adapter: fetch document
Adapter-->>Database: document
Database-->>Client: document
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai 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).
src/Database/Adapter/MariaDB.php
Outdated
| return false; | ||
| } | ||
|
|
||
| public function getSupportTTLIndexes(): bool |
There was a problem hiding this comment.
Let's make sure method names are always consistent: getSupportForTTLIndexes
src/Database/Adapter/Mongo.php
Outdated
|
|
||
| // 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
src/Database/Validator/Index.php
Outdated
| if (empty($orders)) { | ||
| $this->message = 'TTL index need explicit orders. Add the orders to create this index.'; | ||
| return false; | ||
| } |
src/Database/Validator/Index.php
Outdated
| 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?
There was a problem hiding this comment.
changed the default to 1
| ); | ||
| $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
There was a problem hiding this comment.
In IndexTests everyplace we are using DatabaseException
| } | ||
| } | ||
|
|
||
| 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
There was a problem hiding this comment.
updated with retries due to flakiness
- Updated assertions in existing tests to compare datetime values by parsing them into DateTime objects, ensuring accurate comparisons with MongoDB's format. - Added new test `testSchemalessDatetimeCreationAndFetching` to verify creation, fetching, and updating of ISO 8601 datetime strings. - Introduced `testSchemalessTTLExpiry` to validate TTL functionality with mixed document types, ensuring expired documents are correctly removed. - Implemented `testStringAndDatetime` and `testStringAndDateWithTTL` to check behavior of string and datetime fields, including handling of TTL indexes.
Co-authored-by: Jake Barnby <jakeb994@gmail.com>
| } | ||
|
|
||
| // Check if existing index is also a TTL index | ||
| if ($existingIndex->getAttribute('type') === Database::INDEX_TTL) { |
There was a problem hiding this comment.
@abnegate restricting to a single ttl index per collection for deterministic expiration on the cache
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@composer.json`:
- Line 42: The composer.json dependency "utopia-php/pools": "1.*" is invalid;
update the constraint to a real published version (for example change
"utopia-php/pools": "0.8.*") if the code is compatible, or revert this change
and ensure the PR is held until utopia-php/pools v1.0 is officially released;
modify the dependency string in composer.json accordingly and run composer
validate/install to confirm the fix.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/Database/Database.php`:
- Around line 4284-4311: The TTL enforcement currently only runs when the TTL
attribute is present in the returned/cached document; fix this by extracting the
TTL-check logic into a reusable helper (e.g., checkAndExpireDocument(collection,
Document $document) that uses $this->adapter->getSupportForTTLIndexes(),
self::INDEX_TTL, $collection->getAttribute('indexes') and checks the TTL
attribute/ttl seconds and returns an empty document via
$this->createDocumentInstance(...) when expired), call that helper both on cache
hits and immediately after fetching from the adapter (before caching/returning),
and ensure the TTL attribute is always loaded for the internal check by
temporarily augmenting the requested selection to include the TTL attribute when
fetching (then strip it from the returned payload if it was not originally
requested).
…ted assertions to check for empty state of expired documents and removed redundant TTL deletion checks, streamlining the test logic.
2 participants
Summary by CodeRabbit
New Features
Tests