Normalize extended ISO 8601 datetime strings in query values for Mong…#814
Normalize extended ISO 8601 datetime strings in query values for Mong…#814ArnabChatterjee20k wants to merge 5 commits intomainfrom
Conversation
…oDB compatibility
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdds ISO 8601 datetime normalization to the MongoDB adapter's buildFilter to convert matching query values into MongoDB UTCDateTime objects when attributes support is disabled or when filtering on $createdAt/$updatedAt; adds an end-to-end test exercising datetime queries across multiple comparison operators. Changes
Sequence Diagram(s)(omitted — changes are localized and do not introduce multi-component sequential flows warranting a diagram) Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/Database/Adapter/Mongo.php`:
- Around line 2476-2488: The current loop in the Mongo adapter unconditionally
converts ISO-like strings to UTCDateTime (using isExtendedISODatetime and
toMongoDatetime) which breaks schemaful queries; change the logic to only
normalize values when the query is schemaless or when the corresponding
attribute is known to be a datetime type—e.g. call a metadata accessor on the
Query (or buildFilter) to get the attribute type for key $k and skip conversion
unless the type === 'datetime' (or equivalent) or $query->isSchemaless() returns
true; keep the try/catch around to leave unparsable values unchanged and then
call $query->setValues($values) as before.
In `@tests/e2e/Adapter/Scopes/SchemalessTests.php`:
- Around line 3371-3372: Re-enable the collection cleanup by restoring the call
to $database->deleteCollection($col) and ensure it runs regardless of test
outcome by moving it into a finally block; locate the test that currently leaves
the collection (references $database and $col around the commented line) and
wrap the test logic in try { ... } finally { $database->deleteCollection($col);
} so the collection is always removed after the test.
src/Database/Adapter/Mongo.php
Outdated
| // Normalize extended ISO 8601 datetime strings in query values to UTCDateTime | ||
| // so they can be correctly compared against datetime fields stored in MongoDB. | ||
| $values = $query->getValues(); | ||
| foreach ($values as $k => $value) { | ||
| if (is_string($value) && $this->isExtendedISODatetime($value)) { | ||
| try { | ||
| $values[$k] = $this->toMongoDatetime($value); | ||
| } catch (\Throwable $th) { | ||
| // Leave value as-is if it cannot be parsed as a datetime | ||
| } | ||
| } | ||
| } | ||
| $query->setValues($values); |
There was a problem hiding this comment.
Guard datetime normalization by attribute type to avoid schemaful regressions.
Line 2479 converts all ISO-like strings to UTCDateTime. In schemaful mode, string attributes can store ISO-looking values; coercing the query value will stop those rows from matching (type mismatch). Please only normalize when the attribute is known to be a datetime (or when running schemaless). Otherwise this is a query correctness regression.
💡 Possible guard (stopgap)
- $values = $query->getValues();
- foreach ($values as $k => $value) {
- if (is_string($value) && $this->isExtendedISODatetime($value)) {
- try {
- $values[$k] = $this->toMongoDatetime($value);
- } catch (\Throwable $th) {
- // Leave value as-is if it cannot be parsed as a datetime
- }
- }
- }
- $query->setValues($values);
+ // Only normalize when we can be sure the attribute represents a datetime
+ if (!$this->getSupportForAttributes() || \in_array($query->getAttribute(), ['$createdAt', '$updatedAt'], true)) {
+ $values = $query->getValues();
+ foreach ($values as $k => $value) {
+ if (is_string($value) && $this->isExtendedISODatetime($value)) {
+ try {
+ $values[$k] = $this->toMongoDatetime($value);
+ } catch (\Throwable $th) {
+ // Leave value as-is if it cannot be parsed as a datetime
+ }
+ }
+ }
+ $query->setValues($values);
+ }Consider wiring attribute type metadata into buildFilter/Query so custom datetime attributes are safely normalized as well.
🤖 Prompt for AI Agents
In `@src/Database/Adapter/Mongo.php` around lines 2476 - 2488, The current loop in
the Mongo adapter unconditionally converts ISO-like strings to UTCDateTime
(using isExtendedISODatetime and toMongoDatetime) which breaks schemaful
queries; change the logic to only normalize values when the query is schemaless
or when the corresponding attribute is known to be a datetime type—e.g. call a
metadata accessor on the Query (or buildFilter) to get the attribute type for
key $k and skip conversion unless the type === 'datetime' (or equivalent) or
$query->isSchemaless() returns true; keep the try/catch around to leave
unparsable values unchanged and then call $query->setValues($values) as before.
There was a problem hiding this comment.
in schemaless we dont have support for attribute types, but in queries it will not be an issue reason being if string the check would be o(1) as the normalization doing it and if you see the logic you will get it
There was a problem hiding this comment.
@ArnabChatterjee20k, I understand the focus on schemaless mode, but I'd like to clarify the concern about schemaful mode:
In schemaful mode (when getSupportForAttributes() returns true), a collection could have a regular string attribute that legitimately stores ISO-formatted datetime strings as text data (e.g., event logs, API responses, human-readable timestamps).
The issue:
- Database stores:
{ "logMessage": "2024-01-01T00:00:00Z" }(as a string) - User queries:
Query::equal("logMessage", "2024-01-01T00:00:00Z") - Your code converts the query value to
UTCDateTimeobject - MongoDB compares:
string(in DB) vsUTCDateTime(in query) → no match ❌
Pattern in your own codebase:
In castingBefore (lines 1398-1413), you already guard datetime normalization:
if (!$this->getSupportForAttributes()) { // ← Only in schemaless mode
foreach ($document->getArrayCopy() as $key => $value) {
if (is_string($value) && ... && $this->isExtendedISODatetime($value)) {
// normalize to UTCDateTime
}
}
}Suggestion:
Add the same guard to buildFilter to maintain consistency and avoid breaking schemaful string queries:
// Only normalize in schemaless mode or for internal datetime attributes
if (!$this->getSupportForAttributes() || \in_array($query->getAttribute(), ['$createdAt', '$updatedAt'], true)) {
$values = $query->getValues();
foreach ($values as $k => $value) {
if (is_string($value) && $this->isExtendedISODatetime($value)) {
try {
$values[$k] = $this->toMongoDatetime($value);
} catch (\Throwable $th) {
// Leave value as-is
}
}
}
$query->setValues($values);
}Could you confirm whether schemaful collections with string attributes containing ISO-formatted text are a supported use case? If so, the guard would prevent regressions. 🤔
🧠 Learnings used
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.
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.
| // $database->deleteCollection($col); | ||
| } |
There was a problem hiding this comment.
Re-enable collection cleanup in the new test.
Line 3371 leaves the collection behind. This can leak test data and impact later runs. Please delete the collection at the end (ideally in a finally).
🧹 Suggested fix
- // $database->deleteCollection($col);
+ $database->deleteCollection($col);🤖 Prompt for AI Agents
In `@tests/e2e/Adapter/Scopes/SchemalessTests.php` around lines 3371 - 3372,
Re-enable the collection cleanup by restoring the call to
$database->deleteCollection($col) and ensure it runs regardless of test outcome
by moving it into a finally block; locate the test that currently leaves the
collection (references $database and $col around the commented line) and wrap
the test logic in try { ... } finally { $database->deleteCollection($col); } so
the collection is always removed after the test.
1 participant
…oDB compatibility
Summary by CodeRabbit
New Features
Tests