Joins #763
Joins #763
Conversation
# Conflicts: # composer.lock # src/Database/Database.php # tests/e2e/Adapter/Base.php
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Database/Query.php (1)
1264-1280: Copy-paste bugs: fallback values use$limitinstead of correct variables.Lines 1270 and 1279 incorrectly fall back to
$limitinstead of$offsetand$cursorrespectively. While the null-coalescing operator makes these fallbacks rarely triggered (values should always exist for valid queries), the code is misleading and could cause subtle bugs if a malformed query slips through.Proposed fix
case Query::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 Query::TYPE_CURSOR_AFTER: case Query::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 === Query::TYPE_CURSOR_AFTER ? Database::CURSOR_AFTER : Database::CURSOR_BEFORE; break;
🤖 Fix all issues with AI agents
In `@tests/unit/Validator/QueryTest.php`:
- Around line 268-275: Replace the unsafe use of reset() and the wrong
assertion: instead of $cursor = reset($queries) (which can be false), assign
$cursor = $queries[0] after asserting the array has two elements; and change the
incorrect assertNotEquals on $queries[1]->getMethod() to
assertEquals($queries[1]->getMethod(), Query::TYPE_CURSOR_AFTER) so the second
query is asserted to be the cursor-after type; keep references to
Query::getCursorQueries, $cursor, getMethod, Query::TYPE_CURSOR_BEFORE and
Query::TYPE_CURSOR_AFTER to locate the lines to change.
🧹 Nitpick comments (1)
src/Database/Query.php (1)
350-378: UseQueryExceptioninstead of generic\Exceptionfor consistency.The
getCursorDirection()andgetOrderDirection()methods throw\Exception, but the rest of the class usesQueryExceptionfor error handling. This inconsistency could make error handling more difficult for consumers.Suggested fix
public function getCursorDirection(): string { if ($this->method === self::TYPE_CURSOR_AFTER) { return Database::CURSOR_AFTER; } if ($this->method === self::TYPE_CURSOR_BEFORE) { return Database::CURSOR_BEFORE; } - throw new \Exception('Invalid method: Get cursor direction on "'.$this->method.'" Query'); + throw new QueryException('Invalid method: Get cursor direction on "'.$this->method.'" Query'); } public function getOrderDirection(): string { if ($this->method === self::TYPE_ORDER_ASC) { return Database::ORDER_ASC; } if ($this->method === self::TYPE_ORDER_DESC) { return Database::ORDER_DESC; } if ($this->method === self::TYPE_ORDER_RANDOM) { return Database::ORDER_RANDOM; } - throw new \Exception('Invalid method: Get order direction on "'.$this->method.'" Query'); + throw new QueryException('Invalid method: Get order direction on "'.$this->method.'" Query'); }
# Conflicts: # src/Database/Query.php # tests/unit/Validator/QueryTest.php
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/Database/Query.php (1)
1069-1080: Breaking change:getByTypevisibility reduced to protected.Changing
getByTypefrompublictoprotectedis a breaking API change. External consumers and tests that call this method directly will fail. The pipeline failures confirm this affects the test suite.If this is intentional, consider:
- Providing public alternatives (which you've done with
getCursorQueries,getSelectQueries, etc.)- Documenting this breaking change in release notes
The test file needs to be updated to use the new public methods instead.
tests/unit/Validator/QueryTest.php (1)
258-324: Pipeline failure: Calling protected methodQuery::getByType().Lines 266 and 288 call
Query::getByType()which is nowprotected, causing the pipeline to fail. The test is meant to verify the behavior of filtering queries by type, but must use the public API.Since this test specifically validates the clone vs reference behavior of
getByType, you could:
- Remove the direct
getByTypecalls and rely on the specialized public methods- Keep one test using
getCursorQueriesto verify the same behaviorProposed fix using public getCursorQueries method
public function testQueryGetByType(): void { $queries = [ Query::equal('key', ['value']), Query::cursorBefore(new Document([])), Query::cursorAfter(new Document([])), ]; - $queries1 = Query::getByType($queries, [Query::TYPE_CURSOR_AFTER, Query::TYPE_CURSOR_BEFORE]); + // Test with clone (default behavior) + $queries1 = Query::getCursorQueries($queries); $this->assertCount(2, $queries1); foreach ($queries1 as $query) { $this->assertEquals(true, in_array($query->getMethod(), [Query::TYPE_CURSOR_AFTER, Query::TYPE_CURSOR_BEFORE])); } $cursor = reset($queries1); $this->assertInstanceOf(Query::class, $cursor); $cursor->setValue(new Document(['$id' => 'hello1'])); $query1 = $queries[1]; $this->assertEquals(Query::TYPE_CURSOR_BEFORE, $query1->getMethod()); $this->assertInstanceOf(Document::class, $query1->getValue()); $this->assertTrue($query1->getValue()->isEmpty()); // Cursor Document is not updated /** * Using reference $queries2 => $queries */ - $queries2 = Query::getByType($queries, [Query::TYPE_CURSOR_AFTER, Query::TYPE_CURSOR_BEFORE], false); + $queries2 = Query::getCursorQueries($queries, false); $cursor = reset($queries2); $this->assertInstanceOf(Query::class, $cursor);
🧹 Nitpick comments (2)
src/Database/Query.php (2)
350-378: Direction helpers could throw unclear exceptions.
getCursorDirection()andgetOrderDirection()throw generic\Exceptionwith a message, but the rest of the class usesQueryException. Consider usingQueryExceptionfor consistency, or at minimum add@throwsdocblocks.Suggested improvement for exception consistency
+ /** + * `@throws` QueryException + */ public function getCursorDirection(): string { if ($this->method === self::TYPE_CURSOR_AFTER) { return Database::CURSOR_AFTER; } if ($this->method === self::TYPE_CURSOR_BEFORE) { return Database::CURSOR_BEFORE; } - throw new \Exception('Invalid method: Get cursor direction on "'.$this->method.'" Query'); + throw new QueryException('Invalid method: Get cursor direction on "'.$this->method.'" Query'); } + /** + * `@throws` QueryException + */ public function getOrderDirection(): string { // ... similar change for the throw statement }
532-570: New metadata fields extracted but not type-validated.The new fields (
alias,aliasRight,as,collection,attributeRight) are extracted from the decoded array but not validated to be strings before passing to the constructor. If malformed JSON provides a non-string value (e.g.,"alias": []), it will cause aTypeErrorinstead of aQueryException, which is inconsistent with the error handling formethod,attribute, andvalues.Add type validation for new metadata fields
$alias = $query['alias'] ?? ''; $aliasRight = $query['aliasRight'] ?? ''; $as = $query['as'] ?? ''; $collection = $query['collection'] ?? ''; + foreach (['alias' => $alias, 'aliasRight' => $aliasRight, 'as' => $as, 'collection' => $collection, 'attributeRight' => $attributeRight] as $key => $value) { + if (!\is_string($value)) { + throw new QueryException("Invalid query {$key}. Must be a string, got " . \gettype($value)); + } + } + if (!\is_string($method)) {
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/Database/Validator/Queries/V2.php`:
- Around line 298-300: Update the exception message in the validation branch
that checks count($filters) (in class V2, where the code throws new
\Exception('Invalid query: '.\ucfirst($method).' queries require at least one
queries')); replace the grammatically incorrect phrase "at least one queries"
with "at least one query" so the message reads something like 'Invalid query:
'.\ucfirst($method).' queries require at least one query'.
♻️ Duplicate comments (1)
tests/e2e/Adapter/Scopes/DocumentTests.php (1)
4289-4293: Decode should use the encoded document.Right now the test decodes the raw
$document, so the encode→decode path isn’t actually exercised.🐛 Proposed fix
- $result = $database->decode($context, $document); + $result = $database->decode($context, $result);
🧹 Nitpick comments (2)
src/Database/Validator/Queries/V2.php (2)
77-79: Minor typo in comment.The comment has a grammatical error: "if original data is changes" should be "if original data changes".
/** - * Since $context includes Documents , clone if original data is changes. + * Since $context includes Documents, clone if original data changes. */
663-685: Use constant instead of string literal for type comparison.Line 663 uses the string literal
'relationship'while the switch case on line 608 usesDatabase::VAR_RELATIONSHIP. For consistency and to avoid potential bugs if the constant value ever changes, use the constant:- if ($attribute['type'] === 'relationship') { + if ($attribute['type'] === Database::VAR_RELATIONSHIP) {
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Database/Adapter/Mongo.php (1)
2094-2125: InconsistentconvertStdClassToArrayusage between batches.The first batch processing (line 2097) uses
convertStdClassToArray($record)to handle stdClass objects in results, but subsequent batch processing (line 2120) creates the Document directly from$recordwithout this conversion. This inconsistency could cause documents to contain unconverted stdClass objects when query results span multiple batches.Proposed fix
foreach ($moreResults as $result) { $record = $this->replaceChars('_', '$', (array)$result); - $doc = new Document($record); + $doc = new Document($this->convertStdClassToArray($record)); if ($removeSequence) { $doc->removeAttribute('$sequence'); } $found[] = $doc; }
🤖 Fix all issues with AI agents
In `@src/Database/Database.php`:
- Around line 4891-4896: In applySelectFiltersToDocuments(), change the
attributesToKeep construction to preserve aliases and relationship roots: for
each $selectQuery use its getAttribute() but also, if $selectQuery has an alias
(e.g., getAlias() or similar), add that alias as a key to $attributesToKeep, and
if the attribute contains a dot (e.g., "rel.field"), explode on '.' and add the
first segment as a key as well; this ensures selects like "foo AS bar" and
"rel.field" keep "bar" and "rel" so nested relationship data isn't stripped
while still using getAttribute() for normal selects.
♻️ Duplicate comments (6)
tests/e2e/Adapter/Scopes/DocumentTests.php (1)
4427-4431: Decode should use encoded output (or clarify intent).Right now decode receives the original
$document, which bypasses the encode→decode round‑trip. If the goal is to validate the pipeline, pass$resultinstead (or add a short comment if intentional).🔧 Suggested fix
- $result = $database->decode($context, $document); + $result = $database->decode($context, $result);src/Database/Adapter/Mongo.php (1)
2025-2040: Guard against unsupportedORDER_RANDOMqueries.The code calls
$order->getOrderDirection()at line 2031 without guarding againstTYPE_ORDER_RANDOM. SincegetOrderDirection()throws an exception for random-order queries and this adapter reportsgetSupportForOrderRandom() === false, anorderRandom()query reaching this code would crash with an unclear error message. Add an explicit check similar to the SQL adapter's handling.Proposed defensive guard
foreach ($orderQueries as $i => $order) { + if ($order->getMethod() === Query::TYPE_ORDER_RANDOM) { + throw new DatabaseException('Random order is not supported by the Mongo adapter'); + } + $attribute = $order->getAttribute(); $originalAttribute = $attribute;src/Database/Database.php (4)
5728-5732: Type normalization is skipped on update/upsert return paths.These paths call
decode()withoutcasting(), which leaves unnormalized scalar types for adapters without native casting and makes update/upsert inconsistent with create/get/find. Please addcasting($context, …)beforedecode()in each path.Also applies to: 5958-5963, 6784-6788
8264-8267: Throw QueryException for unknown alias context.These blocks still throw a generic
\Exception, which bypasses query-specific handling. Replace withQueryExceptionfor consistent error flow.Also applies to: 8363-8366
8322-8324: Casting guard is inverted.The early-return should skip casting when the adapter does support casting, not when it doesn’t.
8809-8811: Remove unused$idAddedfrom relationship select injection.
$idAddedis never used and triggers PHPMD. You can safely drop it while keeping the relationship-only$idinjection.🛠️ Proposed fix
- if (!empty($relationships)) { - [$queries, $idAdded] = QueryContext::addSelect($queries, Query::select('$id', system: true)); - } + if (!empty($relationships)) { + [$queries] = QueryContext::addSelect($queries, Query::select('$id', system: true)); + }Based on learnings, ...
🧹 Nitpick comments (1)
src/Database/Adapter/Mongo.php (1)
1971-1972: Unused parameters$joinsand$vectorsare intentional for interface compatibility.The static analyzer flags these as unused. This is expected since MongoDB doesn't support joins or vector operations, but the parameters are required to match the abstract
Adapterinterface. Consider adding a PHPDoc annotation to suppress the warning.* `@param` array<Query> $vectors + * + * `@SuppressWarnings`(PHPMD.UnusedFormalParameter) - Interface compliance; Mongo doesn't support joins/vectors * * `@return` array<Document>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Database/Database.php (1)
1-1: Fix PSR-12 indentation failure flagged by Pint.The linter reports a PSR-12 statement indentation error in this file (Line 1). Please run Pint or adjust indentation to satisfy PSR-12.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/e2e/Adapter/Scopes/JoinsTests.php`:
- Around line 34-35: The PHPDoc `@var` annotations incorrectly name the variable
`$database` while the actual variable is `$db`; update each docblock to read
`@var Database $db` so PHPStan recognizes the correct variable type—apply this
change to the docblock immediately above the `$db = $this->getDatabase();`
assignment and the other occurrences that reference `$database` in the same test
class (replace `@var Database $database` with `@var Database $db`).
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/Database/Query.php (2)
1271-1288:⚠️ Potential issue | 🔴 CriticalCopy-paste bug: wrong fallback variables for
$offsetand$cursorLine 1277:
$offset = $values[0] ?? $limit;— should fall back to$offset, not$limit.
Line 1286:$cursor = $values[0] ?? $limit;— should fall back to$cursor, not$limit.If
$values[0]is not set, offset and cursor will be incorrectly set to the limit value instead of preserving their current state.Proposed fix
- $offset = $values[0] ?? $limit; + $offset = $values[0] ?? $offset;- $cursor = $values[0] ?? $limit; + $cursor = $values[0] ?? $cursor;
79-127:⚠️ Potential issue | 🟡 Minor
TYPESarray is missing join and relation-equal constants
isMethod()(lines 454–457) recognizesTYPE_RELATION_EQUAL,TYPE_INNER_JOIN,TYPE_LEFT_JOIN, andTYPE_RIGHT_JOIN, but the publicTYPESarray (lines 79–127) does not include them. While the codebase uses specialized type arrays (JOINS_TYPES,FILTER_TYPES, etc.) for actual validation rather than the generalTYPESarray, the inconsistency creates confusion about the completeness ofTYPESand could introduce bugs if future code relies on it for validation.Add the missing constants to
TYPESfor consistency:Proposed fix
self::TYPE_OR, self::TYPE_ELEM_MATCH, - self::TYPE_REGEX + self::TYPE_REGEX, + self::TYPE_RELATION_EQUAL, + self::TYPE_INNER_JOIN, + self::TYPE_LEFT_JOIN, + self::TYPE_RIGHT_JOIN, ];
🤖 Fix all issues with AI agents
In `@src/Database/Query.php`:
- Line 195: The Query class declares a protected bool $system (set in the
constructor and passed from the select() factory) but never exposes it, making
it dead state; add a public accessor (e.g., isSystem() : bool or getSystem() :
bool) near the other accessors in Query so callers can read the flag, and ensure
its name matches the project's boolean getter convention and is used where
select() supplies the value.
🧹 Nitpick comments (1)
src/Database/Query.php (1)
350-378: Inconsistent exception type — useQueryExceptioninstead of\Exception
getCursorDirection()andgetOrderDirection()throw\Exception, while the rest of the class usesQueryException. This makes it harder for callers to catch query-related errors uniformly.Proposed fix
- throw new \Exception('Invalid method: Get cursor direction on "'.$this->method.'" Query'); + throw new QueryException('Invalid method: Get cursor direction on "'.$this->method.'" Query');- throw new \Exception('Invalid method: Get order direction on "'.$this->method.'" Query'); + throw new QueryException('Invalid method: Get order direction on "'.$this->method.'" Query');
| protected string $aliasRight = ''; | ||
| protected string $attributeRight = ''; | ||
| protected string $as = ''; | ||
| protected bool $system = false; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if system property is accessed anywhere in PHP files
echo "=== Searching for system property access patterns ==="
rg -n '->system\b|getSystem|isSystem' --type=php src/ -C2
echo -e "\n=== Also check for \$system assignments ==="
rg -n '\$system\s*=' --type=php src/ -C2Repository: utopia-php/database
Length of output: 1573
🏁 Script executed:
#!/bin/bash
# Search for getter methods related to system
echo "=== Searching for getSystem or isSystem methods ==="
rg 'getSystem|isSystem' --type=php src/
echo -e "\n=== Search for property access using different pattern ==="
rg '\$system' --type=php src/ -C1
echo -e "\n=== Check if system property is read anywhere ==="
rg 'this->system' --type=php src/ -C2Repository: utopia-php/database
Length of output: 1240
$system property has no public getter
The system field is declared at line 195, set in the constructor, and passed from the select() factory (line 803), but there is no getSystem() or isSystem() accessor. The property is never read after assignment, making it dead state.
Proposed fix — add a getter near the other accessors
+ public function isSystem(): bool
+ {
+ return $this->system;
+ }
+
public function getCollection(): string
{
return $this->collection;
}🤖 Prompt for AI Agents
In `@src/Database/Query.php` at line 195, The Query class declares a protected
bool $system (set in the constructor and passed from the select() factory) but
never exposes it, making it dead state; add a public accessor (e.g., isSystem()
: bool or getSystem() : bool) near the other accessors in Query so callers can
read the flag, and ensure its name matches the project's boolean getter
convention and is used where select() supplies the value.
2 participants
Summary by CodeRabbit
New Features
Refactor