Open
Conversation
Contributor
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe pull request adds the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/Database/Query.php (1)
62-76: Align cursor helper docblocks with the actual parameter type.Lines 62 and 73 document
Document, but the signature ismixed $value(Lines 65 and 76). Please update phpdoc to match actual behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Query.php` around lines 62 - 76, The PHPDoc for Query::cursorAfter and Query::cursorBefore incorrectly types the parameter as Document while the method signatures accept mixed $value; update the `@param` annotations in both docblocks to "@param mixed $value" (and keep "@return Query" unchanged) so the docblocks match the actual signatures for the cursorAfter and cursorBefore methods on the Query class.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@composer.json`:
- Around line 62-66: Replace the SSH Git URL in the composer.json "repositories"
entry with its HTTPS equivalent to avoid CI SSH auth issues; locate the
"repositories" array and update the url value currently set to
"git@github.com:utopia-php/query.git" to use
"https://github.com/utopia-php/query.git" so installs succeed in non-interactive
CI environments.
In `@src/Database/Query.php`:
- Around line 167-183: The code is using $limit as a fallback for both $offset
and $cursor which is incorrect; update the assignments in the TYPE_OFFSET and
TYPE_CURSOR_AFTER/TYPE_CURSOR_BEFORE branches so they use the actual first value
or null instead of $limit (i.e. change $values[0] ?? $limit to $values[0] ??
null) while keeping the existing checks that ignore subsequent offsets/cursors
and preserving $cursorDirection assignment (references: TYPE_OFFSET,
TYPE_CURSOR_AFTER, TYPE_CURSOR_BEFORE, $offset, $cursor, $values, $limit,
$cursorDirection, Database::CURSOR_AFTER, Database::CURSOR_BEFORE).
---
Nitpick comments:
In `@src/Database/Query.php`:
- Around line 62-76: The PHPDoc for Query::cursorAfter and Query::cursorBefore
incorrectly types the parameter as Document while the method signatures accept
mixed $value; update the `@param` annotations in both docblocks to "@param mixed
$value" (and keep "@return Query" unchanged) so the docblocks match the actual
signatures for the cursorAfter and cursorBefore methods on the Query class.
ℹ️ Review info
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 (2)
composer.jsonsrc/Database/Query.php
Comment on lines
+62
to
+66
| "repositories": [ | ||
| { | ||
| "type": "vcs", | ||
| "url": "git@github.com:utopia-php/query.git" | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n composer.json | head -70Repository: utopia-php/database
Length of output: 2871
🏁 Script executed:
python - <<'PY'
import json
from pathlib import Path
data = json.loads(Path("composer.json").read_text())
repos = data.get("repositories", [])
for idx, repo in enumerate(repos):
url = repo.get("url", "")
if url.startswith("git@"):
print(f"Repository[{idx}] uses SSH URL: {url}")
else:
print(f"Repository[{idx}] URL type: {url}")
PYRepository: utopia-php/database
Length of output: 126
Use HTTPS for the VCS repository URL to prevent install failures in CI.
Line 65 uses an SSH URL (git@github.com:utopia-php/query.git), which fails in non-interactive CI environments that lack SSH key configuration. Change to the HTTPS equivalent:
🔧 Proposed fix
"repositories": [
{
"type": "vcs",
- "url": "git@github.com:utopia-php/query.git"
+ "url": "https://github.com/utopia-php/query.git"
}
],📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "repositories": [ | |
| { | |
| "type": "vcs", | |
| "url": "git@github.com:utopia-php/query.git" | |
| } | |
| "repositories": [ | |
| { | |
| "type": "vcs", | |
| "url": "https://github.com/utopia-php/query.git" | |
| } | |
| ], |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@composer.json` around lines 62 - 66, Replace the SSH Git URL in the
composer.json "repositories" entry with its HTTPS equivalent to avoid CI SSH
auth issues; locate the "repositories" array and update the url value currently
set to "git@github.com:utopia-php/query.git" to use
"https://github.com/utopia-php/query.git" so installs succeed in non-interactive
CI environments.
Comment on lines
+167
to
+183
| case self::TYPE_OFFSET: | ||
| // Keep the 1st offset encountered and ignore the rest | ||
| if ($offset !== null) { | ||
| break; | ||
| } | ||
|
|
||
| $offset = $values[0] ?? $limit; | ||
| break; | ||
| case Query::TYPE_CURSOR_AFTER: | ||
| case Query::TYPE_CURSOR_BEFORE: | ||
| case self::TYPE_CURSOR_AFTER: | ||
| case self::TYPE_CURSOR_BEFORE: | ||
| // Keep the 1st cursor encountered and ignore the rest | ||
| if ($cursor !== null) { | ||
| break; | ||
| } | ||
|
|
||
| $cursor = $values[0] ?? $limit; | ||
| $cursorDirection = $method === Query::TYPE_CURSOR_AFTER ? Database::CURSOR_AFTER : Database::CURSOR_BEFORE; | ||
| $cursorDirection = $method === self::TYPE_CURSOR_AFTER ? Database::CURSOR_AFTER : Database::CURSOR_BEFORE; |
Contributor
There was a problem hiding this comment.
Fix incorrect fallback variable usage for offset and cursor.
Line 173 and Line 182 fall back to $limit, which is unrelated and can silently corrupt grouped query state.
🔧 Proposed fix
case self::TYPE_OFFSET:
@@
- $offset = $values[0] ?? $limit;
+ $offset = $values[0] ?? $offset;
break;
@@
case self::TYPE_CURSOR_AFTER:
case self::TYPE_CURSOR_BEFORE:
@@
- $cursor = $values[0] ?? $limit;
+ $cursor = $values[0] ?? $cursor;
$cursorDirection = $method === self::TYPE_CURSOR_AFTER ? Database::CURSOR_AFTER : Database::CURSOR_BEFORE;
break;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Database/Query.php` around lines 167 - 183, The code is using $limit as a
fallback for both $offset and $cursor which is incorrect; update the assignments
in the TYPE_OFFSET and TYPE_CURSOR_AFTER/TYPE_CURSOR_BEFORE branches so they use
the actual first value or null instead of $limit (i.e. change $values[0] ??
$limit to $values[0] ?? null) while keeping the existing checks that ignore
subsequent offsets/cursors and preserving $cursorDirection assignment
(references: TYPE_OFFSET, TYPE_CURSOR_AFTER, TYPE_CURSOR_BEFORE, $offset,
$cursor, $values, $limit, $cursorDirection, Database::CURSOR_AFTER,
Database::CURSOR_BEFORE).
Add a PHPStan stub for Utopia\Query\Query that declares `@return static` on all factory methods, so PHPStan correctly resolves return types when called via the Utopia\Database\Query subclass. Also fix groupByType() param type and remove dead instanceof checks in parse/parseQuery. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/Database/Query.php (1)
161-177:⚠️ Potential issue | 🟠 MajorFix fallback leakage from
limitinto offset/cursor state.Line 167 and Line 176 incorrectly use
$limitas fallback, which can corrupt grouped query output when offset/cursor values are missing.🔧 Proposed fix
case self::TYPE_OFFSET: @@ - $offset = $values[0] ?? $limit; + $offset = $values[0] ?? $offset; break; @@ case self::TYPE_CURSOR_AFTER: case self::TYPE_CURSOR_BEFORE: @@ - $cursor = $values[0] ?? $limit; + $cursor = $values[0] ?? $cursor; $cursorDirection = $method === self::TYPE_CURSOR_AFTER ? Database::CURSOR_AFTER : Database::CURSOR_BEFORE; break;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Query.php` around lines 161 - 177, In Query:: (switch handling TYPE_OFFSET / TYPE_CURSOR_AFTER / TYPE_CURSOR_BEFORE) remove the incorrect fallback to $limit so $offset and $cursor aren't polluted: instead of assigning $values[0] ?? $limit, only assign when an explicit value exists (e.g. check isset($values[0]) or array_key_exists) and otherwise leave $offset/$cursor as null; keep the first-occurrence guards intact and preserve setting $cursorDirection when handling TYPE_CURSOR_AFTER vs TYPE_CURSOR_BEFORE (Database::CURSOR_AFTER / Database::CURSOR_BEFORE).
🧹 Nitpick comments (1)
src/Database/Query.php (1)
53-73: Align cursor helper PHPDoc with the actualmixedparameter type.Both docblocks still document
@param Document $value, but the signatures now acceptmixed.📝 Suggested doc fix
- * `@param` Document $value + * `@param` mixed $value @@ - * `@param` Document $value + * `@param` mixed $value🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Query.php` around lines 53 - 73, Update the PHPDoc for the Query helper methods to match the actual parameter type: in the Query class update the docblocks for cursorAfter and cursorBefore so the `@param` annotation is "@param mixed $value" (instead of "@param Document $value") and keep the `@return` annotation as "Query"; ensure these docblocks sit immediately above the corresponding methods cursorAfter and cursorBefore to maintain accurate IDE/typehinting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/Database/Query.php`:
- Around line 161-177: In Query:: (switch handling TYPE_OFFSET /
TYPE_CURSOR_AFTER / TYPE_CURSOR_BEFORE) remove the incorrect fallback to $limit
so $offset and $cursor aren't polluted: instead of assigning $values[0] ??
$limit, only assign when an explicit value exists (e.g. check isset($values[0])
or array_key_exists) and otherwise leave $offset/$cursor as null; keep the
first-occurrence guards intact and preserve setting $cursorDirection when
handling TYPE_CURSOR_AFTER vs TYPE_CURSOR_BEFORE (Database::CURSOR_AFTER /
Database::CURSOR_BEFORE).
---
Nitpick comments:
In `@src/Database/Query.php`:
- Around line 53-73: Update the PHPDoc for the Query helper methods to match the
actual parameter type: in the Query class update the docblocks for cursorAfter
and cursorBefore so the `@param` annotation is "@param mixed $value" (instead of
"@param Document $value") and keep the `@return` annotation as "Query"; ensure
these docblocks sit immediately above the corresponding methods cursorAfter and
cursorBefore to maintain accurate IDE/typehinting.
Update Query overrides to use `: static` return types matching the base query package. Remove the phpstan.neon and stubs workaround since the query package now uses `: static` natively. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/Database/Query.php (1)
159-169:⚠️ Potential issue | 🟠 MajorFix incorrect fallback variable usage for offset and cursor.
Lines 159 and 168 incorrectly fall back to
$limitinstead of the appropriate variable. This can silently corrupt the grouped query state.
- Line 159:
$offset = $values[0] ?? $limitshould use$offset(ornull)- Line 168:
$cursor = $values[0] ?? $limitshould use$cursor(ornull)🔧 Proposed fix
case self::TYPE_OFFSET: // Keep the 1st offset encountered and ignore the rest if ($offset !== null) { break; } - $offset = $values[0] ?? $limit; + $offset = $values[0] ?? null; break; case self::TYPE_CURSOR_AFTER: case self::TYPE_CURSOR_BEFORE: // Keep the 1st cursor encountered and ignore the rest if ($cursor !== null) { break; } - $cursor = $values[0] ?? $limit; + $cursor = $values[0] ?? null; $cursorDirection = $method === self::TYPE_CURSOR_AFTER ? Database::CURSOR_AFTER : Database::CURSOR_BEFORE; break;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Query.php` around lines 159 - 169, The switch handling in Query.php incorrectly falls back to $limit when assigning $offset and $cursor, corrupting grouped query state; update the assignments in the cases that set $offset and $cursor (the branches that assign $offset = $values[0] ?? $limit and $cursor = $values[0] ?? $limit) to use a proper null fallback instead (e.g. $offset = $values[0] ?? null and $cursor = $values[0] ?? null), keeping the surrounding logic for cursorDirection (Database::CURSOR_AFTER / Database::CURSOR_BEFORE) intact.
🧹 Nitpick comments (1)
src/Database/Query.php (1)
54-65: Consider tightening parameter type hint.The PHPDoc indicates
@param Document $value, but the signature usesmixed. Consider usingDocumentas the type hint for better type safety, or update the PHPDoc to reflect the actual accepted types.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Query.php` around lines 54 - 65, The PHPDoc for cursorBefore indicates the parameter is a Document but the method signature (and cursorAfter) use mixed; update the signatures to use Document instead of mixed for cursorBefore and cursorAfter (i.e., change the parameter type from mixed to Document) to match the PHPDoc and improve type safety, or alternatively update the PHPDoc to reflect mixed if these methods truly accept other types—adjust the declarations in the Query class (cursorBefore, cursorAfter) so the docblock and method signatures are consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Database/Query.php`:
- Around line 42-49: The method parseQuery currently declares a return type of
static but calls parent::parseQuery() which returns BaseQuery; change the method
signature to return BaseQuery instead of static and keep the try/catch that
wraps BaseQueryException into QueryException (i.e., update the return type on
parseQuery to BaseQuery to match parent::parseQuery and ensure the thrown
QueryException still wraps the original BaseQueryException).
---
Duplicate comments:
In `@src/Database/Query.php`:
- Around line 159-169: The switch handling in Query.php incorrectly falls back
to $limit when assigning $offset and $cursor, corrupting grouped query state;
update the assignments in the cases that set $offset and $cursor (the branches
that assign $offset = $values[0] ?? $limit and $cursor = $values[0] ?? $limit)
to use a proper null fallback instead (e.g. $offset = $values[0] ?? null and
$cursor = $values[0] ?? null), keeping the surrounding logic for cursorDirection
(Database::CURSOR_AFTER / Database::CURSOR_BEFORE) intact.
---
Nitpick comments:
In `@src/Database/Query.php`:
- Around line 54-65: The PHPDoc for cursorBefore indicates the parameter is a
Document but the method signature (and cursorAfter) use mixed; update the
signatures to use Document instead of mixed for cursorBefore and cursorAfter
(i.e., change the parameter type from mixed to Document) to match the PHPDoc and
improve type safety, or alternatively update the PHPDoc to reflect mixed if
these methods truly accept other types—adjust the declarations in the Query
class (cursorBefore, cursorAfter) so the docblock and method signatures are
consistent.
Comment on lines
+42
to
49
| public static function parseQuery(array $query): static | ||
| { | ||
| try { | ||
| $query = \json_decode($query, true, flags: JSON_THROW_ON_ERROR); | ||
| } catch (\JsonException $e) { | ||
| throw new QueryException('Invalid query: ' . $e->getMessage()); | ||
| } | ||
|
|
||
| if (!\is_array($query)) { | ||
| throw new QueryException('Invalid query. Must be an array, got ' . \gettype($query)); | ||
| return parent::parseQuery($query); | ||
| } catch (BaseQueryException $e) { | ||
| throw new QueryException($e->getMessage(), $e->getCode(), $e); | ||
| } | ||
|
|
||
| return self::parseQuery($query); | ||
| } |
Contributor
There was a problem hiding this comment.
Same return type issue as parse().
parent::parseQuery() will also return BaseQuery, not static. Apply the same fix pattern.
🔧 Proposed fix
public static function parseQuery(array $query): static
{
try {
- return parent::parseQuery($query);
+ $parsed = parent::parseQuery($query);
+ return new static($parsed->getMethod(), $parsed->getAttribute(), $parsed->getValues());
} catch (BaseQueryException $e) {
throw new QueryException($e->getMessage(), $e->getCode(), $e);
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public static function parseQuery(array $query): static | |
| { | |
| try { | |
| $query = \json_decode($query, true, flags: JSON_THROW_ON_ERROR); | |
| } catch (\JsonException $e) { | |
| throw new QueryException('Invalid query: ' . $e->getMessage()); | |
| } | |
| if (!\is_array($query)) { | |
| throw new QueryException('Invalid query. Must be an array, got ' . \gettype($query)); | |
| return parent::parseQuery($query); | |
| } catch (BaseQueryException $e) { | |
| throw new QueryException($e->getMessage(), $e->getCode(), $e); | |
| } | |
| return self::parseQuery($query); | |
| } | |
| public static function parseQuery(array $query): static | |
| { | |
| try { | |
| $parsed = parent::parseQuery($query); | |
| return new static($parsed->getMethod(), $parsed->getAttribute(), $parsed->getValues()); | |
| } catch (BaseQueryException $e) { | |
| throw new QueryException($e->getMessage(), $e->getCode(), $e); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Database/Query.php` around lines 42 - 49, The method parseQuery currently
declares a return type of static but calls parent::parseQuery() which returns
BaseQuery; change the method signature to return BaseQuery instead of static and
keep the try/catch that wraps BaseQueryException into QueryException (i.e.,
update the return type on parseQuery to BaseQuery to match parent::parseQuery
and ensure the thrown QueryException still wraps the original
BaseQueryException).
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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
1 participant
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