Conversation
Contributor
WalkthroughDatabase::convertQueries and convertQuery are now public static methods. convertQueries recursively processes query trees, collecting returned Query objects; convertQuery returns a modified Query, applies attribute onArray flags, and converts DATETIME values' timezones (throwing on error). Tests add an e2e nested OR datetime query. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Database
participant ConvAll as Database::convertQueries
participant ConvOne as Database::convertQuery
participant AttrStore as Collection Attributes
participant TZ as DateTime TZ Ops
Client->>Database: find(collection, queries)
Database->>ConvAll: convertQueries(collection, queries)
loop each Query (including nested/compound)
ConvAll->>ConvAll: detect compound -> recurse (self::convertQueries)
ConvAll->>ConvOne: convertQuery(collection, query)
ConvOne->>AttrStore: find attribute matching query.field
alt attribute.type == DATETIME
ConvOne->>TZ: setTimezone for each value
TZ-->>ConvOne: converted values
else
ConvOne-->>ConvAll: updated onArray flag only
end
ConvOne-->>ConvAll: return modified Query
end
ConvAll-->>Database: converted queries array
Database-->>Client: execute converted queries and return results
note over ConvAll,ConvOne: static utility-style conversion (no instance state)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/e2e/Adapter/Scopes/AttributeTests.php (1)
1681-1691: Exercise both success and failure paths for nested conversions (positive OR hit + nested invalid values).Good addition to ensure nested Query::or() gets converted. To harden coverage:
- Add a positive case where one nested clause matches (equality on either $createdAt or date) so we assert OR semantics and recursive conversion on a match.
- Add a nested invalid-value case to ensure QueryException bubbles up from within logical groups.
Suggested change:
/** * Test convertQueries on nested queries */ - $docs = $database->find('datetime', [ - Query::or([ - Query::equal('$createdAt', [$date]), - Query::equal('date', [$date]) - ]), - ]); - $this->assertCount(0, $docs); + $docs = $database->find('datetime', [ + Query::or([ + Query::equal('$createdAt', [$date]), + Query::equal('date', [$date]), + ]), + ]); + $this->assertCount(0, $docs); + + // Positive case: one nested clause matches + $docs = $database->find('datetime', [ + Query::or([ + Query::equal('$createdAt', [$document->getCreatedAt()]), + Query::equal('date', ['1975-01-01T00:00:00.000+00:00']), + ]), + ]); + $this->assertCount(1, $docs); + + // Invalid nested values should raise QueryException + try { + $database->find('datetime', [ + Query::or([ + Query::equal('$createdAt', ['16/01/2024 12:00:00AM']), + Query::equal('date', ['not-a-date']), + ]), + ]); + $this->fail('Failed to throw exception'); + } catch (Throwable $e) { + $this->assertTrue($e instanceof QueryException); + }Minor nit: consider using a consistent variable name ($documents vs $docs) for readability.
src/Database/Database.php (2)
6757-6771: Recursive conversion is solid; micro-optimization: avoid per-query conversion on nested logical nodesYou already recurse into nested queries. Calling convertQuery on a logical (nested) node does nothing and costs a pass. Tiny win to skip it.
Apply this diff:
public static function convertQueries(Document $collection, array $queries): array { foreach ($queries as $index => $query) { if ($query->isNested()) { - $values = self::convertQueries($collection, $query->getValues()); - $query->setValues($values); - } - - $query = self::convertQuery($collection, $query); - - $queries[$index] = $query; + $query->setValues(self::convertQueries($collection, $query->getValues())); + $queries[$index] = $query; + continue; + } + + $queries[$index] = self::convertQuery($collection, $query); } return $queries; }Side note: since these are now static, consider switching callers to self::convertQueries/convertQuery for consistency.
6787-6812: Guard datetime conversion by value type and break early after attribute matchTwo tweaks improve robustness and a bit of performance:
- Only timezone-convert values that are strings. Queries like isNull on a DATETIME attribute often carry booleans; converting them would throw unnecessarily.
- Break after finding the matching attribute to avoid scanning the rest.
Apply this diff:
- $attribute = new Document(); - - foreach ($attributes as $attr) { - if ($attr->getId() === $query->getAttribute()) { - $attribute = $attr; - } - } + $attribute = new Document(); + foreach ($attributes as $attr) { + $attrId = $attr->getId(); + $attrKey = $attr->getAttribute('key', $attrId); + if ($attrId === $query->getAttribute() || $attrKey === $query->getAttribute()) { + $attribute = $attr; + break; + } + } - if (! $attribute->isEmpty()) { + if (!$attribute->isEmpty()) { $query->setOnArray($attribute->getAttribute('array', false)); if ($attribute->getAttribute('type') == Database::VAR_DATETIME) { - $values = $query->getValues(); - foreach ($values as $valueIndex => $value) { - try { - $values[$valueIndex] = DateTime::setTimezone($value); - } catch (\Throwable $e) { - throw new QueryException($e->getMessage(), $e->getCode(), $e); - } - } - $query->setValues($values); + $values = $query->getValues(); + foreach ($values as $valueIndex => $value) { + if (!\is_string($value)) { + // Skip non-string values (e.g., booleans from isNull, nulls) + continue; + } + try { + $values[$valueIndex] = DateTime::setTimezone($value); + } catch (\Throwable $e) { + throw new QueryException( + "Invalid datetime value for '{$query->getAttribute()}': " . $e->getMessage(), + (int)$e->getCode(), + $e + ); + } + } + $query->setValues($values); } } return $query;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/Database/Database.php(1 hunks)tests/e2e/Adapter/Scopes/AttributeTests.php(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
tests/e2e/Adapter/Scopes/AttributeTests.php (1)
src/Database/Query.php (3)
Query(8-836)or(676-679)equal(370-373)
src/Database/Database.php (3)
src/Database/Document.php (3)
Document(12-470)getAttribute(224-231)isEmpty(396-399)src/Database/Query.php (6)
isNested(811-818)getValues(138-141)setValues(184-189)Query(8-836)getAttribute(130-133)setOnArray(832-835)src/Database/DateTime.php (2)
DateTime(7-86)setTimezone(58-67)
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/Database/Database.php (2)
6782-6818: Harden DATETIME value conversion and micro‑optimize attribute lookup.
- Skip timezone conversion for null/empty/non-string values to avoid unnecessary exceptions (e.g., isNull/isNotNull scenarios).
- Use strict comparison for type check.
- Break after finding the matching attribute to avoid extra iterations.
Apply this diff:
- foreach ($attributes as $attr) { - if ($attr->getId() === $query->getAttribute()) { - $attribute = $attr; - } - } + foreach ($attributes as $attr) { + if ($attr->getId() === $query->getAttribute()) { + $attribute = $attr; + break; + } + } - if (! $attribute->isEmpty()) { + if (!$attribute->isEmpty()) { $query->setOnArray($attribute->getAttribute('array', false)); - if ($attribute->getAttribute('type') == Database::VAR_DATETIME) { + if ($attribute->getAttribute('type') === Database::VAR_DATETIME) { $values = $query->getValues(); foreach ($values as $valueIndex => $value) { - try { - $values[$valueIndex] = DateTime::setTimezone($value); - } catch (\Throwable $e) { - throw new QueryException($e->getMessage(), $e->getCode(), $e); - } + // Skip null/empty/non-string entries (e.g. isNull, placeholders) + if ($value === null || $value === '') { + continue; + } + if (!\is_string($value)) { + continue; + } + try { + $values[$valueIndex] = DateTime::setTimezone($value); + } catch (\Throwable $e) { + throw new QueryException($e->getMessage(), $e->getCode(), $e); + } } $query->setValues($values); } } return $query;
6759-6773: Prefer static calls for convertQueries (and convertQuery)I found three instance calls to the now-static method
convertQueriesinsrc/Database/Database.php—please update them to useself::for consistency and to satisfy static analysis:
- Line 6151 (in find()):
- $this->convertQueries($collection, $filters)
- self::convertQueries($collection, $filters)
- Line 6314 (in count()): ```diff - $queries = $this->convertQueries($collection, $queries); + $queries = self::convertQueries($collection, $queries);
- Line 6359 (in sum()):
- $queries = $this->convertQueries($collection, $queries);
- $queries = self::convertQueries($collection, $queries);
No direct `$this->convertQuery()` calls were found, but if you spot any elsewhere, switch them to `self::convertQuery(...)` as well. </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used: .coderabbit.yaml** **Review profile: CHILL** **Plan: Pro** **💡 Knowledge Base configuration:** - MCP integration is disabled by default for public repositories - Jira integration is disabled by default for public repositories - Linear integration is disabled by default for public repositories You can enable these sources in your CodeRabbit configuration. <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 678a8c193b75e709f6ac4f9e8d4a52fa64b9089b and f74d93a6ea56f498cfc7b3f8793515f509bac18d. </details> <details> <summary>📒 Files selected for processing (1)</summary> * `src/Database/Database.php` (1 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🧬 Code Graph Analysis (1)</summary> <details> <summary>src/Database/Database.php (3)</summary><blockquote> <details> <summary>src/Database/Document.php (3)</summary> * `Document` (12-470) * `getAttribute` (224-231) * `isEmpty` (396-399) </details> <details> <summary>src/Database/Query.php (6)</summary> * `isNested` (811-818) * `getValues` (138-141) * `setValues` (184-189) * `Query` (8-836) * `getAttribute` (130-133) * `setOnArray` (832-835) </details> <details> <summary>src/Database/DateTime.php (2)</summary> * `DateTime` (7-86) * `setTimezone` (58-67) </details> </blockquote></details> </details><details> <summary>🪛 GitHub Actions: CodeQL</summary> <details> <summary>src/Database/Database.php</summary> [error] 6759-6759: PHPStan: Method Utopia\\Database\\Database::convertQueries() return type has no value type specified in iterable type array. </details> </details> </details> <details> <summary>⏰ 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)</summary> * GitHub Check: Setup & Build Docker Image </details> <details> <summary>🔇 Additional comments (2)</summary><blockquote> <details> <summary>src/Database/Database.php (2)</summary> `6761-6773`: **Recursive nested query conversion looks good.** The recursion for nested logical queries and collecting the modified Query objects is correct and side‑effect–free outside the passed instances. --- `6775-6781`: **Docblocks updated appropriately for static usage and throws.** Signature and throws annotations are consistent with the implementation. </details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/Database/Database.php (2)
6795-6814: Nit: break after match, use strict comparison, and guard non-string datetime values
- Stop scanning attributes after a match to avoid unnecessary work.
- Use strict equality for type checks.
- Guard non-string values in datetime queries for clearer errors; nulls can be skipped (e.g., when using isNull-type semantics) while non-strings should produce a targeted error instead of relying on a lower-level exception.
Apply this diff:
- foreach ($attributes as $attr) { - if ($attr->getId() === $query->getAttribute()) { - $attribute = $attr; - } - } + foreach ($attributes as $attr) { + if ($attr->getId() === $query->getAttribute() || $attr->getAttribute('key') === $query->getAttribute()) { + $attribute = $attr; + break; + } + } - if (! $attribute->isEmpty()) { + if (!$attribute->isEmpty()) { $query->setOnArray($attribute->getAttribute('array', false)); - if ($attribute->getAttribute('type') == Database::VAR_DATETIME) { + if ($attribute->getAttribute('type') === Database::VAR_DATETIME) { $values = $query->getValues(); foreach ($values as $valueIndex => $value) { + if ($value === null) { + continue; + } + if (!\is_string($value)) { + throw new QueryException('Invalid datetime query value type: ' . \gettype($value)); + } try { $values[$valueIndex] = DateTime::setTimezone($value); } catch (\Throwable $e) { throw new QueryException($e->getMessage(), $e->getCode(), $e); } } $query->setValues($values); } }
6759-6773: Use static invocation for convertQueries within Database classTo avoid static analysis warnings and improve consistency, replace instance-style calls to the newly public static methods with
self::(orstatic::) invocations. Specifically, update the following locations insrc/Database/Database.php:
- Line 6151:
$this->convertQueries($collection, $filters)
self::convertQueries($collection, $filters)
- Line 6314:
$queries = $this->convertQueries($collection, $queries);
$queries = self::convertQueries($collection, $queries);
- Line 6359:
$queries = $this->convertQueries($collection, $queries);
$queries = self::convertQueries($collection, $queries);This ensures all internal calls to
convertQueries/convertQueryuse static invocation.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/Database/Database.php(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/Database/Database.php (4)
src/Database/Document.php (4)
Document(12-470)getAttribute(224-231)getId(63-66)isEmpty(396-399)src/Database/Validator/Queries/Document.php (1)
Document(10-43)src/Database/Query.php (6)
isNested(811-818)getValues(138-141)setValues(184-189)Query(8-836)getAttribute(130-133)setOnArray(832-835)src/Database/DateTime.php (2)
DateTime(7-86)setTimezone(58-67)
⏰ 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). (11)
- GitHub Check: Adapter Tests (MySQL)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (SharedTables/SQLite)
- GitHub Check: Adapter Tests (SharedTables/Postgres)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Adapter Tests (Postgres)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (MariaDB)
- GitHub Check: Unit Test
🔇 Additional comments (3)
src/Database/Database.php (3)
6753-6758: Docblock generics fix resolves PHPStan complaintThanks for adding precise generics to the return type. This should address the “no value type specified in iterable type array” error.
6759-6773: Recursive conversion for nested queries is correctThe recursion on nested logical queries first, followed by per-query conversion, is sound. Returning the updated array (instead of mutating an external list) simplifies the flow.
6782-6818: Good: per-attribute array-flag and timezone normalization for datetime queries
- setOnArray reflects the attribute’s array semantics into the query as intended.
- DATETIME value normalization via DateTime::setTimezone with clear QueryException surfacing is a helpful correctness improvement for cross-timezone querying.
abnegate
approved these changes
Aug 20, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Tests