Joins3 #686
Joins3 #686
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: 31
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (15)
tests/e2e/Adapter/Scopes/Relationships/OneToManyTests.php (1)
800-803: Bug: wrong variable asserted; should reference$account1, not$account2.After deleting
customer1, the null check must be onaccount1’snewCustomer. This currently tests the unrelatedaccount2and will fail or mask the intended behavior.Apply this diff:
- $account1 = $database->getDocument('account', 'account1'); - $this->assertEquals(null, $account2->getAttribute('newCustomer', '')); + $account1 = $database->getDocument('account', 'account1'); + $this->assertEquals(null, $account1->getAttribute('newCustomer', ''));src/Database/Validator/Query/Order.php (1)
3-75: Delete dead validator file
This file only contains commented-out code and no live references toUtopia\Database\Validator\Query\Order; removesrc/Database/Validator/Query/Order.php.tests/e2e/Adapter/Scopes/Relationships/ManyToManyTests.php (2)
1691-1693: Same issue: array-form select should be split.Apply:
- $artists = $database->find('artists', [ - Query::select(['name', 'albums.name', 'albums.tracks.title']) - ]); + $artists = $database->find('artists', [ + Query::select('name'), + Query::select('albums.name'), + Query::select('albums.tracks.title'), + ]);
1587-1589: Refactor all array-form Query::select() calls to string-only arguments
PHP 8+ throws a TypeError forQuery::select([...]); search forQuery::select([and replace each with individualQuery::select('field')calls in tests (SpatialTests, RelationshipTests, ManyToManyTests, DocumentTests) and in README.md.tests/e2e/Adapter/Scopes/DocumentTests.php (2)
4887-4899: Fix:Query::select()now accepts a single attribute, not an array.Passing
[...]will break with the new API. Mirror the approach used above witharray_map.- Query::select([...$selects, '$createdAt']), + Query::select('$createdAt'), + ...array_map(fn ($f) => Query::select($f), $selects), Query::lessThan('$createdAt', '1800-01-01'), Query::orderAsc('$createdAt'), Query::orderAsc(), Query::limit(1),
5152-5155: Replace array-styleQuery::selectcalls
- In tests/e2e/Adapter/Scopes/DocumentTests.php (around line 5143), change
to[Query::select(['*'])] // Mongo bug with Integer UID[Query::select('*')] // Mongo bug with Integer UID- Likewise, update any
Query::select([...])in SpatialTests.php, RelationshipTests.php, etc., to either multiple per‐field calls (Query::select('name'),Query::select('location'), …) or a single wildcard string.- To find all remaining array-style selects, run:
rg -nP "Query::select\(\s*\[" -g '!vendor/**' -C2tests/unit/Validator/QueryTest.php (1)
176-176: Remove debug output from validatorThere's a var_dump statement in the V2 validator that should be removed.
This debug statement appears to be from the V2.php file based on the relevant code snippets provided. It should be removed from production code.
tests/unit/Validator/Query/FilterTest.php (3)
137-138: Incorrect validator usage for notContainsThe validator is being called incorrectly - the
isValid()method expects an array of Query objects, but a single Query is being passed.Apply this diff to fix the validation calls:
- $this->assertTrue($this->validator->isValid(Query::notContains('string', ['unwanted']))); - $this->assertTrue($this->validator->isValid(Query::notContains('string_array', ['spam', 'unwanted']))); - $this->assertTrue($this->validator->isValid(Query::notContains('integer_array', [100, 200]))); + $this->assertTrue($this->validator->isValid([Query::notContains('string', ['unwanted'])])); + $this->assertTrue($this->validator->isValid([Query::notContains('string_array', ['spam', 'unwanted'])])); + $this->assertTrue($this->validator->isValid([Query::notContains('integer_array', [100, 200])]));
142-143: Inconsistent validator usage patternThe validator is being called with a single Query instead of an array, which is inconsistent with the expected signature.
Apply this diff:
- $this->assertFalse($this->validator->isValid(Query::notContains('string', []))); + $this->assertFalse($this->validator->isValid([Query::notContains('string', [])]));
149-149: Inconsistent validator method signature usage throughout test methodsMultiple test methods are passing single Query objects instead of arrays to the
isValid()method.Apply this diff to fix all occurrences:
- $this->assertTrue($this->validator->isValid(Query::notSearch('string', 'unwanted'))); + $this->assertTrue($this->validator->isValid([Query::notSearch('string', 'unwanted')])); - $this->assertFalse($this->validator->isValid(Query::notSearch('string_array', 'unwanted'))); + $this->assertFalse($this->validator->isValid([Query::notSearch('string_array', 'unwanted')])); - $this->assertTrue($this->validator->isValid(Query::notStartsWith('string', 'temp'))); + $this->assertTrue($this->validator->isValid([Query::notStartsWith('string', 'temp')])); - $this->assertFalse($this->validator->isValid(Query::notStartsWith('string_array', 'temp'))); + $this->assertFalse($this->validator->isValid([Query::notStartsWith('string_array', 'temp')])); - $this->assertTrue($this->validator->isValid(Query::notEndsWith('string', '.tmp'))); + $this->assertTrue($this->validator->isValid([Query::notEndsWith('string', '.tmp')])); - $this->assertFalse($this->validator->isValid(Query::notEndsWith('string_array', '.tmp'))); + $this->assertFalse($this->validator->isValid([Query::notEndsWith('string_array', '.tmp')])); - $this->assertTrue($this->validator->isValid(Query::notBetween('integer', 0, 50))); + $this->assertTrue($this->validator->isValid([Query::notBetween('integer', 0, 50)])); - $this->assertFalse($this->validator->isValid(Query::notBetween('integer_array', 1, 10))); + $this->assertFalse($this->validator->isValid([Query::notBetween('integer_array', 1, 10)]));Also applies to: 163-163, 177-177, 191-191
src/Database/Database.php (5)
3465-3474: Fix caching condition — don’t disable cache for all collections with relationships.Current check disables cache whenever the collection has any relationship attribute. Only skip cache when this fetch is part of a relationship traversal.
- // Don't save to cache if it's part of a relationship - if (empty($relationships)) { + // Don't save to cache if this fetch is part of a relationship traversal + if (empty($this->relationshipFetchStack)) { try { $this->cache->save($documentKey, $document->getArrayCopy(), $hashKey); $this->cache->save($collectionKey, 'empty', $documentKey); } catch (Exception $e) { Console::warning('Failed to save document to cache: ' . $e->getMessage()); } }
6365-6373: Avoid leaking internal $perms in find().
decode()/casting()preserve “$perms”. Remove it from the final payload, like in getDocument().$node = $this->casting($context, $node, $selects); $node = $this->decode($context, $node, $selects); + $node->removeAttribute('$perms');
4617-4621: Batch update requires $permissions and $sequence — auto-inject selects.Instead of failing later, add the required selects here to make the API resilient.
- $batch = $this->silent(fn () => $this->find( + $requiredSelects = [Query::select('$permissions'), Query::select('$sequence')]; + $batch = $this->silent(fn () => $this->find( $collection->getId(), - array_merge($new, $queries), + array_merge($requiredSelects, $new, $queries), forPermission: Database::PERMISSION_UPDATE ));
6096-6101: Batch delete requires $permissions and $sequence — auto-inject selects.Same robustness fix for deletes.
- $batch = $this->silent(fn () => $this->find( + $requiredSelects = [Query::select('$permissions'), Query::select('$sequence')]; + $batch = $this->silent(fn () => $this->find( $collection->getId(), - array_merge($new, $queries), + array_merge($requiredSelects, $new, $queries), forPermission: Database::PERMISSION_DELETE ));
6723-6743: decodeOriginal(): undefined variable and unused parameter.
$selectionsis undefined and$selectsis unused, producing incorrect decoding and static-analysis noise. Use$selectsto build the selected names and gate assignments.- foreach ($value as $index => $node) { + foreach ($value as $index => $node) { if (is_string($node) && in_array($type, Database::SPATIAL_TYPES)) { $node = $this->decodeSpatialData($node); } foreach (array_reverse($filters) as $filter) { $node = $this->decodeAttribute($filter, $node, $document, $key); } $value[$index] = $node; } - - if ( - empty($selections) - || \in_array($key, $selections) - || \in_array('*', $selections) - ) { - $document->setAttribute($key, ($array) ? $value : $value[0]); - } + $selectedNames = array_map( + fn (Query $q) => $q->getAs() !== '' ? $q->getAs() : $q->getAttribute(), + $selects + ); + if (empty($selects) || \in_array($key, $selectedNames, true) || \in_array('*', $selectedNames, true)) { + $document->setAttribute($key, $array ? $value : $value[0]); + }
🧹 Nitpick comments (28)
phpunit.xml (1)
10-10: Avoid committing stop-on-first-failure to the shared configFlipping stopOnFailure="true" is great for local debugging but hides subsequent failures in CI and slows down triage. Prefer keeping it false in the repo and using the CLI flag ad hoc or a CI-specific config.
Apply this diff if you agree:
- stopOnFailure="true" + stopOnFailure="false"src/Database/Validator/Query/Base.php (1)
7-16: Consider hardening method types (optional)To avoid stringly-typed drift, consider centralizing method types via a dedicated final class or native enum (if PHP ≥ 8.1), and referencing it across validators.
src/Database/Validator/Query/Offset.php (1)
40-48: Optional: enforce integer-only offsetsNumeric() may accept numeric strings/floats. If offsets must be non-negative integers, validate with FILTER_VALIDATE_INT (and range) or an Integer validator to prevent “10.5” or "007" slipping through.
Example:
- $validator = new Numeric(); - if (!$validator->isValid($offset)) { - $this->message = 'Invalid offset: ' . $validator->getDescription(); - return false; - } + if (filter_var($offset, FILTER_VALIDATE_INT) === false) { + $this->message = 'Invalid offset: must be an integer.'; + return false; + }src/Database/Validator/AsQuery.php (3)
46-47: Tighten identifier rules to avoid SQL identifier pitfalls (optional).Allowing a leading digit can force quoting in SQL identifiers across adapters. Consider enforcing a leading letter/underscore and validating length in one pass.
- if (! preg_match('/^[a-zA-Z0-9_]+$/', $value)) { + if (! preg_match('/^[A-Za-z_][A-Za-z0-9_]*$/', $value)) { return false; } - - if (\mb_strlen($value) > 64) { - return false; - } + if (\mb_strlen($value) > 64) { // keep for clarity or drop since regex already permits any length; up to you + return false; + }Also applies to: 50-52
38-44: Non-string path should set a descriptive message.Right now a non-string returns false but the message remains about length/charset. Set a clearer message for this path.
- if (! \is_string($value)) { - return false; - } + if (! \is_string($value)) { + $this->message = '"as" must be a string.'; + return false; + }
54-57: Confirm spec: disallowing alias on "*"Blocking
ASwith*seems intentional. If so, consider harmonizing the default description with this specific message or elevate it earlier in docs/errors for clarity.src/Database/Query.php (4)
704-708: Unused$functionparameter in select
$functionisn’t used. Remove it now or persist it as a new property to avoid PHPMD noise.Minimal removal:
- public static function select(string $attribute, string $alias = '', string $as = '', string $function = '', bool $system = false): self + public static function select(string $attribute, string $alias = '', string $as = '', bool $system = false): self { - return new self(self::TYPE_SELECT, $attribute, [], alias: $alias, as: $as, system: $system); + return new self(self::TYPE_SELECT, $attribute, [], alias: $alias, as: $as, system: $system); }
1104-1104: Remove unused$joinslocalDeclared but never used in
groupByType.- $joins = [];
301-326: Throw QueryException instead of generic ExceptionAligns with the rest of the query error surface.
- 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');
403-409: Type-hint the parameter
isSpatialQueryshould accept astring $method.- public static function isSpatialQuery($method): bool + public static function isSpatialQuery(string $method): boolsrc/Database/Adapter/Postgres.php (1)
1565-1568: Alias handling improved; consider defensive default.Filtering and quoting alias is correct. Optional: default empty alias to Query::DEFAULT_ALIAS to avoid "" if ever unset.
Apply locally within this block:
- $alias = $query->getAlias(); + $alias = $query->getAlias() ?: Query::DEFAULT_ALIAS; $alias = $this->filter($alias); $alias = $this->quote($alias);tests/unit/Validator/QueriesTest.php (1)
3-80: Don’t leave an entire test suite commented out—either remove it or turn it into an explicit skipped placeholder.Commented code silently reduces coverage and confuses contributors. Replace with a tiny skipped test to document the deprecation/migration, or delete the file.
Apply this replacement:
-// -//namespace Tests\Unit\Validator; -// ... -//class QueriesTest extends TestCase -//{ -// ... -//} +<?php +namespace Tests\Unit\Validator; + +use PHPUnit\Framework\TestCase; + +final class QueriesTest extends TestCase +{ + public function testLegacyQueriesValidatorDeprecated(): void + { + $this->markTestSkipped('Legacy Queries validator removed; coverage migrated to V2/QueryContext tests.'); + } +}tests/e2e/Adapter/Base.php (1)
11-11: Avoid importing disabled traits; either enable JoinsTests (recommended for this PR) or drop the import.Having JoinsTests imported but commented reduces clarity and leaves join coverage out of CI, which is risky for a joins-focused PR.
Apply to enable Joins tests now:
-use Tests\E2E\Adapter\Scopes\JoinsTests; +use Tests\E2E\Adapter\Scopes\JoinsTests; @@ - //use JoinsTests; + use JoinsTests; - //use CollectionTests; + //use CollectionTests;If joins are still unstable on some adapters, consider guarding via an env flag:
if (getenv('RUN_JOINS_TESTS')) { class_alias(\Tests\E2E\Adapter\Scopes\JoinsTests::class, 'JoinTestsEnabler'); }Also applies to: 22-23
src/Database/Adapter/MariaDB.php (1)
16-18: Drop unused imports.QueryContext and Authorization aren’t used in this adapter file.
Apply:
use Utopia\Database\Query; -use Utopia\Database\QueryContext; -use Utopia\Database\Validator\Authorization;tests/unit/Validator/Query/SelectTest.php (1)
22-29: Collection metadata doc id looks off.Consider setting '$id' to the collection id ('movies') and '$collection' to Database::METADATA for clarity.
- $collection = new Document([ - '$id' => Database::METADATA, + $collection = new Document([ + '$id' => 'movies', '$collection' => Database::METADATA, 'name' => 'movies', 'attributes' => [], 'indexes' => [], ]);tests/unit/Validator/DocumentQueriesTest.php (1)
24-26: Prefer the constant for$collectionoverID::custom()for consistency.Minor consistency tweak; both work, but other tests use the constant directly.
- '$collection' => ID::custom(Database::METADATA), + '$collection' => Database::METADATA,tests/unit/Validator/DocumentsQueriesTest.php (1)
154-158: Order with no attribute may be invalid in V2 — verify or specify field.If V2 doesn’t default to
$sequence, these will fail. Prefer explicit fields to reduce ambiguity.- Query::orderAsc('title'), + Query::orderAsc('title'), Query::limit(10), Query::offset(10), - Query::orderDesc(), + Query::orderDesc('$sequence'),src/Database/QueryContext.php (2)
48-55: Avoid O(n) scans on every alias lookup.Maintain a map of collectionId ➝ Document to make
getCollectionByAlias()O(1).class QueryContext { + /** + * @var array<string, Document> + */ + protected array $collectionsById = []; @@ public function add(Document $collection, string $alias = Query::DEFAULT_ALIAS): void { if (! empty($this->aliases[$alias])) { throw new QueryException('Ambiguous alias for collection "'.$collection->getId().'".'); } $this->collections[] = $collection; $this->aliases[$alias] = $collection->getId(); + $this->collectionsById[$collection->getId()] = $collection; } @@ - foreach ($this->collections as $collection) { - if ($collection->getId() === $collectionId) { - return $collection; - } - } - - return new Document(); + return $this->collectionsById[$collectionId] ?? new Document();
75-86: Use explicit existence check instead ofempty()for boolean flags.
empty()conflates “unset” withfalse. Useisset()to read the stored boolean.public function skipAuth(string $collection, string $permission): bool { if (!Authorization::$status) { // for Authorization::disable(); return true; } - if (empty($this->skipAuthCollections[$collection][$permission])) { - return false; - } - - return true; + if (!isset($this->skipAuthCollections[$collection][$permission])) { + return false; + } + return (bool) $this->skipAuthCollections[$collection][$permission]; }tests/unit/Validator/Query/OrderTest.php (1)
55-58: Unqualified order should likely specify a field — verify or target$sequence.If V2 doesn’t default an empty attribute, make these explicit to avoid flaky expectations.
- $this->assertTrue($this->validator->isValid([Query::orderAsc()])); + $this->assertTrue($this->validator->isValid([Query::orderAsc('$sequence')])); @@ - $this->assertTrue($this->validator->isValid([Query::orderDesc()])); + $this->assertTrue($this->validator->isValid([Query::orderDesc('$sequence')]));tests/unit/Validator/IndexedQueriesTest.php (1)
23-31: Optional: make the collection document’s $id the logical name.
Using'$id' => Database::METADATAcan confuse validators/readers; prefer'$id' => 'movies'and keep'$collection' => Database::METADATA.- $collection = new Document([ - '$id' => Database::METADATA, + $collection = new Document([ + '$id' => 'movies', '$collection' => Database::METADATA, 'name' => 'movies', 'attributes' => [], 'indexes' => [], ]);src/Database/Adapter.php (1)
791-802: Docblock matches the new context-based API; flag$queriesas legacy.
Since adapters now consume explicitselects/filters/joins/orderQueries, consider marking$queriesas deprecated in the doc to steer users away from it.tests/e2e/Adapter/Scopes/JoinsTests.php (1)
62-66: Silence PHPMD: drop or underscore unused variables.
$sessionNoPermissionsand$session3are not used after creation. Create without assigning or use a leading underscore to indicate intentional discard.- $sessionNoPermissions = $db->createDocument('__sessions', new Document([ + $db->createDocument('__sessions', new Document([ 'user_id' => $user1->getId(), '$permissions' => [], ])); ... - $session3 = $db->createDocument('__sessions', new Document([ + $db->createDocument('__sessions', new Document([ 'user_id' => $user2->getId(), '$permissions' => [ Permission::read(Role::any()), ], 'boolean' => true, 'float' => 5.5, ]));Also applies to: 100-107
tests/e2e/Adapter/Scopes/DocumentTests.php (2)
3532-3536: Minor: prefer consistent$databaseusage.This block uses
static::getDatabase()while surrounding code uses$database. Consider consistency for readability.- $documents = static::getDatabase()->find('movies', [ + $documents = $database->find('movies', [ Query::select('name'), Query::select('year'), Query::select('$createdAt') ]);
2929-2938: Optional: add a positive assertion on selected field.Since only
directoris selected, assert it exists to tighten the test.$documents = $database->find('movies', $queries); $this->assertCount(1, $documents); + $this->assertArrayHasKey('director', $documents[0]); $this->assertArrayNotHasKey('name', $documents[0]);src/Database/Database.php (3)
3381-3385: Duplicate computation of selects.
$selectsis computed twice. Keep the earlier one and drop this block.- //$selects = Query::groupByType($queries)['selections']; - $selects = Query::getSelectQueries($queries); + // $selects already computed above
6300-6315: Prefer one path for grouping queries.You compute groups via
groupByType()and then recompute withQuery::get*()calls. Keep one approach to avoid drift.
7249-7251: Cache-key stability.
md5(serialize($selects))couples to object internals and order. Consider hashing stable tuples like[alias, attribute, as]for each select to avoid accidental cache fragmentation.
📜 Review details
Configuration used: Path: .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 ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (39)
phpunit.xml(2 hunks)src/Database/Adapter.php(2 hunks)src/Database/Adapter/MariaDB.php(4 hunks)src/Database/Adapter/Pool.php(3 hunks)src/Database/Adapter/Postgres.php(4 hunks)src/Database/Adapter/SQL.php(11 hunks)src/Database/Database.php(49 hunks)src/Database/Query.php(20 hunks)src/Database/QueryContext.php(1 hunks)src/Database/Validator/Alias.php(1 hunks)src/Database/Validator/AsQuery.php(1 hunks)src/Database/Validator/IndexedQueries.php(1 hunks)src/Database/Validator/Queries.php(1 hunks)src/Database/Validator/Queries/Document.php(1 hunks)src/Database/Validator/Queries/Documents.php(1 hunks)src/Database/Validator/Queries/V2.php(1 hunks)src/Database/Validator/Query/Base.php(1 hunks)src/Database/Validator/Query/Filter.php(1 hunks)src/Database/Validator/Query/Offset.php(1 hunks)src/Database/Validator/Query/Order.php(1 hunks)src/Database/Validator/Query/Select.php(1 hunks)tests/e2e/Adapter/Base.php(2 hunks)tests/e2e/Adapter/Scopes/AttributeTests.php(3 hunks)tests/e2e/Adapter/Scopes/DocumentTests.php(13 hunks)tests/e2e/Adapter/Scopes/JoinsTests.php(1 hunks)tests/e2e/Adapter/Scopes/Relationships/ManyToManyTests.php(5 hunks)tests/e2e/Adapter/Scopes/Relationships/ManyToOneTests.php(5 hunks)tests/e2e/Adapter/Scopes/Relationships/OneToManyTests.php(6 hunks)tests/e2e/Adapter/Scopes/Relationships/OneToOneTests.php(6 hunks)tests/unit/QueryTest.php(4 hunks)tests/unit/Validator/DocumentQueriesTest.php(4 hunks)tests/unit/Validator/DocumentsQueriesTest.php(5 hunks)tests/unit/Validator/IndexedQueriesTest.php(6 hunks)tests/unit/Validator/QueriesTest.php(1 hunks)tests/unit/Validator/Query/CursorTest.php(1 hunks)tests/unit/Validator/Query/FilterTest.php(3 hunks)tests/unit/Validator/Query/OrderTest.php(1 hunks)tests/unit/Validator/Query/SelectTest.php(1 hunks)tests/unit/Validator/QueryTest.php(14 hunks)
🧰 Additional context used
🧬 Code graph analysis (28)
tests/e2e/Adapter/Scopes/Relationships/ManyToOneTests.php (1)
src/Database/Query.php (2)
Query(8-1379)select(704-707)
src/Database/Validator/Query/Offset.php (1)
src/Database/Validator/Query/Base.php (1)
getDescription(26-29)
tests/e2e/Adapter/Scopes/Relationships/OneToManyTests.php (2)
src/Database/Query.php (3)
Query(8-1379)select(704-707)limit(737-740)src/Database/Database.php (1)
find(6243-6378)
tests/e2e/Adapter/Scopes/AttributeTests.php (2)
src/Database/Database.php (1)
getCollection(1413-1429)src/Database/Query.php (3)
getCollection(260-263)Query(8-1379)select(704-707)
tests/e2e/Adapter/Scopes/JoinsTests.php (4)
src/Database/Database.php (2)
Database(35-7466)find(6243-6378)src/Database/Query.php (8)
Query(8-1379)join(885-888)relationEqual(923-926)equal(554-557)orderAsc(726-729)orderDesc(715-718)select(704-707)getAttribute(218-221)src/Database/Helpers/Permission.php (2)
Permission(9-264)read(186-195)src/Database/Helpers/Role.php (3)
Role(5-178)any(159-162)user(115-118)
tests/unit/Validator/Query/CursorTest.php (3)
src/Database/Document.php (1)
Document(12-470)src/Database/Exception.php (1)
Exception(7-21)src/Database/Query.php (3)
Query(8-1379)cursorAfter(759-762)cursorBefore(770-773)
src/Database/QueryContext.php (2)
src/Database/Database.php (1)
Database(35-7466)src/Database/Validator/Authorization.php (1)
Authorization(7-225)
tests/e2e/Adapter/Scopes/Relationships/ManyToManyTests.php (3)
tests/e2e/Adapter/Base.php (1)
getDatabase(38-38)src/Database/Database.php (2)
getDatabase(687-690)find(6243-6378)src/Database/Query.php (2)
Query(8-1379)select(704-707)
src/Database/Validator/Alias.php (1)
src/Database/Validator/Queries/V2.php (3)
getDescription(127-130)isArray(137-140)getType(147-150)
tests/unit/Validator/Query/SelectTest.php (3)
src/Database/QueryContext.php (2)
QueryContext(8-89)add(60-68)src/Database/Validator/Queries/V2.php (3)
V2(23-696)isValid(472-695)getDescription(127-130)src/Database/Query.php (3)
Query(8-1379)select(704-707)limit(737-740)
src/Database/Adapter/MariaDB.php (5)
src/Database/QueryContext.php (1)
QueryContext(8-89)src/Database/Validator/Authorization.php (1)
Authorization(7-225)src/Database/Query.php (5)
getAttributeRight(250-253)getAttribute(218-221)getAlias(240-243)Query(8-1379)getRightAlias(245-248)src/Database/Adapter/SQL.php (1)
getInternalKeyForAttribute(1891-1903)src/Database/Adapter.php (2)
filter(1164-1173)quote(468-468)
tests/unit/Validator/Query/OrderTest.php (4)
src/Database/Database.php (1)
Database(35-7466)src/Database/QueryContext.php (2)
QueryContext(8-89)add(60-68)src/Database/Validator/Queries/V2.php (3)
V2(23-696)isValid(472-695)getDescription(127-130)src/Database/Query.php (6)
Query(8-1379)orderAsc(726-729)orderDesc(715-718)limit(737-740)offset(748-751)equal(554-557)
src/Database/Adapter/Pool.php (3)
src/Database/QueryContext.php (1)
QueryContext(8-89)src/Database/Adapter/SQL.php (2)
find(2330-2532)getAttributeProjection(1842-1889)src/Database/Adapter.php (2)
find(805-817)getAttributeProjection(1155-1155)
tests/unit/Validator/DocumentQueriesTest.php (3)
src/Database/QueryContext.php (2)
QueryContext(8-89)add(60-68)src/Database/Validator/Queries/V2.php (2)
V2(23-696)isValid(472-695)src/Database/Query.php (2)
Query(8-1379)select(704-707)
tests/unit/Validator/DocumentsQueriesTest.php (3)
src/Database/QueryContext.php (2)
QueryContext(8-89)add(60-68)src/Database/Validator/Queries/V2.php (2)
V2(23-696)getDescription(127-130)tests/unit/Validator/DocumentQueriesTest.php (1)
setUp(21-57)
tests/e2e/Adapter/Scopes/Relationships/OneToOneTests.php (3)
src/Database/Query.php (2)
Query(8-1379)select(704-707)src/Database/Database.php (1)
getDocument(3335-3480)src/Database/Adapter.php (1)
getDocument(691-691)
src/Database/Validator/Queries/V2.php (12)
src/Database/Database.php (1)
Database(35-7466)src/Database/Document.php (3)
Document(12-470)getId(63-66)getArrayCopy(423-458)src/Database/Exception.php (1)
Exception(7-21)src/Database/Query.php (11)
Query(8-1379)getAttribute(218-221)getAlias(240-243)getMethod(210-213)getRightAlias(245-248)getFilterQueries(1081-1084)getValues(226-229)isSpatialQuery(408-425)isFilter(1211-1214)getAs(255-258)parse(434-447)src/Database/QueryContext.php (3)
QueryContext(8-89)getCollections(32-35)getCollectionByAlias(37-55)src/Database/Validator/Alias.php (5)
Alias(7-70)getDescription(15-18)isArray(54-57)getType(66-69)isValid(26-45)src/Database/Validator/AsQuery.php (5)
AsQuery(7-85)getDescription(25-28)isArray(69-72)getType(81-84)isValid(36-60)src/Database/Validator/Datetime.php (1)
Datetime(7-152)src/Database/Validator/Query/Cursor.php (1)
Cursor(9-51)src/Database/Validator/Query/Limit.php (1)
Limit(9-63)src/Database/Validator/Query/Offset.php (2)
Offset(9-59)isValid(25-53)src/Database/Validator/Sequence.php (1)
Sequence(9-61)
tests/unit/Validator/Query/FilterTest.php (2)
src/Database/QueryContext.php (2)
QueryContext(8-89)add(60-68)src/Database/Validator/Queries/V2.php (3)
V2(23-696)isValid(472-695)getDescription(127-130)
src/Database/Adapter/Postgres.php (5)
src/Database/QueryContext.php (1)
QueryContext(8-89)src/Database/Validator/Authorization.php (1)
Authorization(7-225)src/Database/Query.php (6)
setAttributeRight(294-299)getAttributeRight(250-253)getAttribute(218-221)getAlias(240-243)Query(8-1379)getRightAlias(245-248)src/Database/Adapter/SQL.php (1)
getInternalKeyForAttribute(1891-1903)src/Database/Adapter.php (2)
filter(1164-1173)quote(468-468)
src/Database/Validator/AsQuery.php (1)
src/Database/Validator/Queries/V2.php (4)
getDescription(127-130)isValid(472-695)isArray(137-140)getType(147-150)
src/Database/Adapter.php (4)
src/Database/Adapter/SQL.php (2)
find(2330-2532)getAttributeProjection(1842-1889)src/Database/Database.php (2)
find(6243-6378)Database(35-7466)src/Database/Adapter/Pool.php (2)
find(264-279)getAttributeProjection(486-489)src/Database/QueryContext.php (1)
QueryContext(8-89)
src/Database/Adapter/SQL.php (4)
src/Database/Database.php (3)
Database(35-7466)find(6243-6378)getCollection(1413-1429)src/Database/QueryContext.php (3)
QueryContext(8-89)getCollections(32-35)skipAuth(75-86)src/Database/Adapter.php (3)
getAttributeProjection(1155-1155)filter(1164-1173)find(805-817)src/Database/Query.php (10)
Query(8-1379)getFilterQueries(1081-1084)select(704-707)getAttribute(218-221)getAlias(240-243)getAs(255-258)limit(737-740)offset(748-751)join(885-888)getCollection(260-263)
tests/unit/Validator/QueryTest.php (3)
src/Database/QueryContext.php (2)
QueryContext(8-89)add(60-68)src/Database/Validator/Queries/V2.php (2)
V2(23-696)isValid(472-695)src/Database/Query.php (4)
Query(8-1379)select(704-707)getCursorQueries(1050-1062)getMethod(210-213)
src/Database/Database.php (6)
src/Database/Validator/Queries/V2.php (2)
V2(23-696)isValid(472-695)src/Database/Query.php (20)
getAttribute(218-221)Query(8-1379)getSelectQueries(952-957)select(704-707)getJoinQueries(963-970)join(885-888)getCollection(260-263)getAlias(240-243)getFilterQueries(1081-1084)limit(737-740)getLimitQuery(992-1001)offset(748-751)getOffsetQuery(1023-1032)getOrderQueries(1038-1044)orderAsc(726-729)getAs(255-258)isNested(1188-1195)isJoin(1200-1209)getValues(226-229)setValues(332-337)src/Database/Document.php (9)
getAttribute(224-231)Document(12-470)removeAttribute(287-293)getId(63-66)getRead(101-104)getCollection(85-88)getPermissionsByType(145-157)find(304-322)getArrayCopy(423-458)src/Database/Adapter/SQL.php (6)
count(2544-2606)getIdAttributeType(1733-1736)getDocument(353-421)find(2330-2532)sum(2619-2682)getSupportForCasting(1436-1439)src/Database/Adapter.php (9)
count(840-840)getIdAttributeType(905-905)getMinDateTime(898-898)getMaxDateTime(912-915)getDocument(691-691)filter(1164-1173)find(805-817)sum(829-829)getSupportForCasting(986-986)src/Database/QueryContext.php (6)
QueryContext(8-89)add(60-68)getCollections(32-35)skipAuth(75-86)addSkipAuth(70-73)getCollectionByAlias(37-55)
tests/unit/Validator/IndexedQueriesTest.php (3)
src/Database/QueryContext.php (2)
QueryContext(8-89)add(60-68)src/Database/Validator/Queries/V2.php (2)
V2(23-696)isValid(472-695)src/Database/Document.php (2)
Document(12-470)setAttribute(244-261)
src/Database/Query.php (2)
src/Database/Database.php (3)
__construct(398-475)getCollection(1413-1429)Database(35-7466)src/Database/QueryContext.php (1)
__construct(25-27)
tests/e2e/Adapter/Scopes/DocumentTests.php (4)
src/Database/Database.php (4)
Database(35-7466)getDocument(3335-3480)find(6243-6378)decode(6757-6845)src/Database/QueryContext.php (2)
QueryContext(8-89)add(60-68)src/Database/Query.php (2)
Query(8-1379)select(704-707)src/Database/Adapter.php (2)
getDocument(691-691)find(805-817)
tests/unit/QueryTest.php (1)
src/Database/Query.php (17)
Query(8-1379)equal(554-557)getMethod(210-213)getAttribute(218-221)getValues(226-229)getAlias(240-243)orderDesc(715-718)limit(737-740)parse(434-447)select(704-707)toString(538-545)getAs(255-258)join(885-888)relationEqual(923-926)getCollection(260-263)getRightAlias(245-248)getAttributeRight(250-253)
🪛 PHPMD (2.15.0)
tests/e2e/Adapter/Scopes/JoinsTests.php
62-62: Avoid unused local variables such as '$sessionNoPermissions'. (Unused Code Rules)
(UnusedLocalVariable)
100-100: Avoid unused local variables such as '$session3'. (Unused Code Rules)
(UnusedLocalVariable)
src/Database/Adapter/SQL.php
1842-1842: Avoid unused parameters such as '$spatialAttributes'. (Unused Code Rules)
(UnusedFormalParameter)
src/Database/Database.php
6673-6673: Avoid unused parameters such as '$selects'. (Unused Code Rules)
(UnusedFormalParameter)
src/Database/Query.php
704-704: Avoid unused parameters such as '$function'. (Unused Code Rules)
(UnusedFormalParameter)
1104-1104: Avoid unused local variables such as '$joins'. (Unused Code Rules)
(UnusedLocalVariable)
🔇 Additional comments (58)
src/Database/Validator/Query/Base.php (1)
15-15: LGTM: add join method type constantThe constant aligns validators with the new join flow. Ensure all join validators’ getMethodType() return this value and that any switch/dispatch in V2 includes it.
src/Database/Validator/Query/Offset.php (1)
42-42: LGTM: corrected error messageMessage now correctly references “offset”, matching the validator’s purpose and the later range check.
src/Database/Validator/IndexedQueries.php (1)
3-116: Remove IndexedQueries file and document BC break
No active references remain (only commented-out mentions), so deletesrc/Database/Validator/IndexedQueries.phpand add a CHANGELOG entry noting its removal as a breaking change.tests/e2e/Adapter/Scopes/Relationships/OneToManyTests.php (8)
119-122: Switch to string-based select API: looks good.
Query::select('name')with separateQuery::limit(1)aligns with the new API and expected projection behavior (no relationships).
151-153: Composed selections are correct.Using
Query::select('*')plusQuery::select('albums.name')to project nested attributes while excluding others (e.g., price) matches intent.
163-165: Duplicate selection pattern.Same pattern as above; no additional feedback.
589-591: Nested projection usage is correct.
Query::select('*')+Query::select('accounts.name')is consistent with the new selection semantics.
600-603: Duplicate selection pattern.Same as prior block; no additional feedback.
925-927: Root-only projection with name: OK.Ensures relationships aren’t included unintentionally.
932-934: Using*to select root attributes only: OK.Expectation that relationships are excluded is clear and tested.
939-942: Multi-level wildcard projections: OK.
'*','cities.*', and'cities.mayor.*'together validate deep selection behavior.src/Database/Validator/Alias.php (2)
32-34: Confirm empty alias is intended.Returning true for empty string implies “no alias” is valid. Verify all call sites treat '' as absence of alias and won’t serialize it as an actual alias token.
7-70: Overall validator implementation is solid.Good regex constraint and type reporting; minimal surface for injection.
src/Database/Validator/AsQuery.php (1)
15-16: composer.json already enforces PHP >= 8.1—readonly properties are supported, no changes needed.src/Database/Validator/Query/Filter.php (1)
2-289: Safe to remove Filter: no active references
Only commented-out imports/instantiations of Utopia\Database\Validator\Query\Filter remain (in tests/unit/Validator/QueriesTest.php, src/Database/Validator/Queries/Documents.php, src/Database/Validator/Queries/V2.php). Removing this class causes no BC breaks.Likely an incorrect or invalid review comment.
tests/unit/QueryTest.php (2)
22-37: LGTM: alias-aware builders coveredGood additions validating alias handling for
equal,orderDesc, andlimit.Also applies to: 29-35
424-459: Joins test shape looks goodAssertions validate join method, collection, aliases, and nested queries correctly (left defaults to DEFAULT_ALIAS; right preserves 'u').
Ensure adapters (MariaDB/Postgres) consume
TYPE_RELATION_EQUALconsistently with these alias semantics.src/Database/Query.php (1)
176-186: Good defaulting of aliasesFalling back to
DEFAULT_ALIASfor empty left/right aliases prevents surprising empty-context bugs.tests/e2e/Adapter/Scopes/AttributeTests.php (3)
66-70: Good addition: assert initial collection metadata.These checks help catch regressions in defaults after createCollection.
246-247: API-aligned select usage.Switching to string-based Query::select matches the new signature.
287-288: API-aligned select usage.Using Query::select('*') as a scalar is correct with the new API.
src/Database/Adapter/Postgres.php (2)
18-20: Imports look fine.Adding QueryContext and Authorization is OK even if future-facing.
1561-1561: Right-hand attribute internal-key mapping: LGTM.Normalizing the right attribute with getInternalKeyForAttribute keeps parity with the left side.
tests/e2e/Adapter/Scopes/Relationships/ManyToOneTests.php (5)
148-150: Use multiple string-based selects.This aligns with the updated Query::select API and keeps projections explicit.
179-182: Projection update: OK.Explicit selects for '*' and movie.name are correct.
191-194: Projection update in getDocument: OK.Matches new API.
562-564: Projection update in findOne: OK.Matches new API.
573-576: Projection update in getDocument: OK.Matches new API.
src/Database/Adapter/MariaDB.php (2)
1459-1467: Good: consistent internal-key mapping and safe alias quoting.Mapping both sides via getInternalKeyForAttribute and quoting + filtering the left alias reduces SQL injection surface and aligns with internal column names.
1456-1456: Ensure getAttributeRight() never returns null
Non-relation queries may leaveattributeRightunset, causinggetAttributeRight()to return null and pass null intogetInternalKeyForAttribute(). Confirm thatgetAttributeRight()always returns a string (e.g.'') or add a fallback before callinggetInternalKeyForAttribute().tests/unit/Validator/Query/CursorTest.php (2)
20-21: Good switch to the new cursor API.Using Query::cursorAfter/Before with a Document aligns with the updated builder methods.
24-36: Negative cases remain valid.The validator rejecting non-cursor queries is appropriate here.
src/Database/Validator/Queries/Document.php (1)
3-44: Remove src/Database/Validator/Queries/Document.php — this file only contains commented-out code and the class it would define is never referenced; it can be deleted.tests/e2e/Adapter/Scopes/Relationships/ManyToManyTests.php (3)
111-114: Consistent database accessor and string-based select.static::getDatabase() and Query::select('name') look good.
142-144: Projection style LGTM.Splitting selects into multiple string-based Query::select calls matches the new API.
Also applies to: 153-156
528-531: Projection style LGTM.Same here—string-based selects per attribute are correct.
Also applies to: 540-543
tests/unit/Validator/Query/SelectTest.php (2)
54-57: Happy-path coverage LGTM.Covers wildcard, scalar, relationship dot-path, and limit.
62-63: Failure-path LGTM.Invalid dot order ('name.artist') should be rejected.
tests/unit/Validator/DocumentQueriesTest.php (1)
76-77: Selectingprice.relationon a non-relation attribute — confirm intent.If dot-path is intended only for relations, switch to a relation field or just
'price'. Otherwise this masks invalid usage.src/Database/Adapter/Pool.php (3)
9-9: Import added matches the new API surface.
TheQueryContextimport is correct and consistent with the updated abstract signature.
486-489: Delegation signature is safe—no subclass requires a non-optional parameter. SQL adapter’s extra$spatialAttributesparameter has a default value, so the proxy’s single-argument call won’t break it.
264-276: Deprecate$queriesand re-verify call sites
Mark the$queriesparameter as deprecated in thefind()phpdoc and confirm no callers pass a non-default value. The previous ripgrep failed because the pattern begins with-; please re-run with:rg -nP -- '->find\(\s*\$?context[^,]*,\s*\$?queries' -C2 src/—or simply list all invocations—
rg -n '->find\(' -C2 src/to manually ensure no non-empty
$queriesusage.tests/unit/Validator/IndexedQueriesTest.php (1)
78-143: LGTM: schema and index setup cover key paths.
Creating KEY and FULLTEXT indexes and asserting cursor/order/limit/offset/search cases matches the V2 validator behavior.src/Database/Adapter.php (2)
1152-1156: Projection API signatures unified and compliant
All adapters match the new abstract signature—Pool uses onlyarray $selects, and SQL’s extraarray $spatialAttributes = []is optional, preserving LSP. No changes needed.
805-817: Ensurefind()overrides match the new abstract signature.
Confirmed both Pool.php and SQL.php declarepublic function find(with the full parameter list—including?int $limit = 25,?int $offset = null,array $cursor = [],string $cursorDirection = Database::CURSOR_AFTER,string $forPermission = Database::PERMISSION_READ, and the remainingarray…parameters—and returnarray. No further updates needed.tests/e2e/Adapter/Scopes/JoinsTests.php (1)
318-329: Confirm aliasing ofmain.*.You're selecting
'*'with alias'main'. Ensure the adapter defines/uses'main'as the base alias; otherwise, prefer default alias ('') for the root selection to avoid “Unknown Alias context” during decode.tests/e2e/Adapter/Scopes/DocumentTests.php (5)
1308-1310: Migration to per-field selects looks good.
1321-1327: Assertion updates align with selective getDocument behavior.
3453-3569: Select semantics for find(): assertions look inconsistent.
- With only
nameandyearselected (Lines 3453–3469), the test expects all internal fields present.- With
name,year, and$updatedAtselected (Lines 3552–3570), the test expects most internals absent.Please verify intended behavior for find() when any select is provided: do we always include all internals, or only selected internals? Align both blocks accordingly.
Would you like a patch once you confirm the intended contract?
4678-4685: Nice: per-field select expansion for bulk delete queries.
3940-3944: Decode via QueryContext: LGTM.The context-based decode matches the new API.
src/Database/Validator/Queries/Documents.php (1)
3-75: Add backward‐compatibility shim or alias for external consumers
- Internal ripgrep search found no references to
Utopia\Database\Validator\Queries\Documentsin the repo.- To avoid fatal errors for any external users still relying on the legacy class, provide either:
- a small proxy that throws a clear deprecation error with upgrade guidance (Option A), or
- a
class_aliasto your new V2 validator if it’s fully compatible (Option B).Example proxy implementation (Option A):
-// -//class Documents extends IndexedQueries -//{ -// ... -//} +namespace Utopia\Database\Validator\Queries; + +final class Documents +{ + public function __construct() + { + throw new \Error('Validator\Queries\Documents has been removed. Use the context-based validator (V2) instead.'); + } +}tests/unit/Validator/Query/FilterTest.php (1)
155-157: Valid usage of Query constructor with array for isValidThese lines correctly wrap the Query objects in arrays when calling
isValid(). This is the correct pattern that should be used consistently throughout the test file.Also applies to: 170-171, 184-185, 198-202
src/Database/Adapter/SQL.php (4)
2369-2370: Cursor direction logic reversalThe cursor direction reversal logic for ORDER BY looks correct - when using CURSOR_BEFORE, the sort direction is inverted to fetch previous results.
The implementation correctly handles bidirectional cursor pagination.
2421-2437: Well-implemented JOIN support with permission checksThe new JOIN implementation properly:
- Applies permission checks per joined table when authorization is enabled
- Handles tenant queries for multi-tenant scenarios
- Uses proper ON conditions from the join query values
This is a solid implementation of the context-driven join functionality.
1768-1769: Pre-filtering queries before building conditionsGood practice to filter queries using
Query::getFilterQueries()before building SQL conditions, ensuring only filter-type queries are processed.
365-369: The script to inspectgetDocumentin SQL.php has been dispatched. Once it runs, we can determine if consumers correctly handle the new$permsfield.src/Database/Database.php (1)
3355-3360: getDocument(): rejecting non-select queries — verify callers.Enforcing “only select queries” is sensible, but it’s breaking if any caller still passes filters/limits. Please confirm tests and public docs reflect this stricter contract.
|
|
||
| $sql = $this->trigger(Database::EVENT_DATABASE_DELETE, $sql); | ||
|
|
||
| var_dump($sql); |
There was a problem hiding this comment.
Remove debug var_dump in production path.
Leaking SQL to stdout/stderr from delete() is a correctness and security issue; it also pollutes test output.
Apply:
- var_dump($sql);📝 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.
| var_dump($sql); |
🤖 Prompt for AI Agents
In src/Database/Adapter/MariaDB.php around line 61 there is a leftover
var_dump($sql) used for debugging; remove that var_dump call from the delete()
path and, if visibility is required for diagnostics, replace it with a
conditional debug log via the project's logger at debug level (or nothing) so no
SQL is printed to stdout/stderr in production; run relevant tests to ensure no
test output is polluted.
| case Query::TYPE_RELATION_EQUAL: | ||
| $attributeRight = $this->quote($this->filter($query->getAttributeRight())); | ||
| $aliasRight = $this->quote($query->getRightAlias()); | ||
|
|
||
| return "{$alias}.{$attribute}={$aliasRight}.{$attributeRight}"; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Sanitize right-hand alias in relation-equality SQL.
aliasRight is quoted but not filtered; filter it like the left alias to prevent malformed aliases or injection.
Apply:
- case Query::TYPE_RELATION_EQUAL:
- $attributeRight = $this->quote($this->filter($query->getAttributeRight()));
- $aliasRight = $this->quote($query->getRightAlias());
- return "{$alias}.{$attribute}={$aliasRight}.{$attributeRight}";
+ case Query::TYPE_RELATION_EQUAL:
+ $attributeRight = $this->quote($this->filter($query->getAttributeRight()));
+ $aliasRight = $this->quote($this->filter($query->getRightAlias()));
+ return "{$alias}.{$attribute}={$aliasRight}.{$attributeRight}";🤖 Prompt for AI Agents
In src/Database/Adapter/MariaDB.php around lines 1510 to 1515 the right-hand
relation alias ($aliasRight) is quoted but not passed through the filter method
like the left alias, leaving a risk of malformed aliases or injection; update
the code to first filter the right alias (e.g.,
$this->filter($query->getRightAlias())) and then quote it, mirroring the
left-alias handling so the returned SQL uses the filtered-and-quoted right
alias.
| case Query::TYPE_RELATION_EQUAL: | ||
| $attributeRight = $this->quote($this->filter($query->getAttributeRight())); | ||
| $aliasRight = $this->quote($query->getRightAlias()); | ||
|
|
||
| return "{$alias}.{$attribute}={$aliasRight}.{$attributeRight}"; | ||
|
|
There was a problem hiding this comment.
Sanitize right alias to prevent malformed identifiers.
getRightAlias() is quoted but not filtered; filter() should precede quote() as with the left alias.
Apply this fix:
case Query::TYPE_RELATION_EQUAL:
$attributeRight = $this->quote($this->filter($query->getAttributeRight()));
- $aliasRight = $this->quote($query->getRightAlias());
+ $aliasRight = $this->quote($this->filter($query->getRightAlias()));
return "{$alias}.{$attribute}={$aliasRight}.{$attributeRight}";📝 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.
| case Query::TYPE_RELATION_EQUAL: | |
| $attributeRight = $this->quote($this->filter($query->getAttributeRight())); | |
| $aliasRight = $this->quote($query->getRightAlias()); | |
| return "{$alias}.{$attribute}={$aliasRight}.{$attributeRight}"; | |
| case Query::TYPE_RELATION_EQUAL: | |
| $attributeRight = $this->quote($this->filter($query->getAttributeRight())); | |
| $aliasRight = $this->quote($this->filter($query->getRightAlias())); | |
| return "{$alias}.{$attribute}={$aliasRight}.{$attributeRight}"; |
🤖 Prompt for AI Agents
In src/Database/Adapter/Postgres.php around lines 1607-1612, the right alias is
quoted but not filtered which can allow malformed identifiers; change the
assignment to filter the right alias before quoting it (i.e. call
$this->filter($query->getRightAlias()) and then $this->quote(...) around that)
so it mirrors the left-alias handling and prevents unsafe identifiers.
|
|
||
| class Alias extends Validator | ||
| { | ||
| protected string $message = 'Alias must contain at most 64 chars. Valid chars are a-z, A-Z, 0-9, and underscore.'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Length limit mismatch: message says 64 chars, code enforces max 63.
Current check mb_strlen($value) >= 64 rejects 64-char aliases. Align code with message.
Apply this diff:
- protected string $message = 'Alias must contain at most 64 chars. Valid chars are a-z, A-Z, 0-9, and underscore.';
+ protected string $message = 'Alias must contain at most 64 chars. Valid chars are a-z, A-Z, 0-9, and underscore.';
@@
- if (\mb_strlen($value) >= 64) {
+ if (\mb_strlen($value) > 64) {
return false;
}Also applies to: 40-41
🤖 Prompt for AI Agents
In src/Database/Validator/Alias.php around line 9 (and also at lines 40-41), the
validation currently rejects 64-character aliases even though the error message
states "at most 64 chars"; change the length check to allow 64 characters by
replacing the condition that uses mb_strlen($value) >= 64 with a check that
fails only when mb_strlen($value) > 64 (or equivalently >= 65), and apply the
same fix to the check at lines 40-41 so the code behavior matches the message.
| $validator = new DocumentsValidator( | ||
| $this->context, | ||
| Database::VAR_INTEGER | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Same here: pass Database::VAR_STRING as $id type.
- $validator = new DocumentsValidator(
- $this->context,
- Database::VAR_INTEGER
- );
+ $validator = new DocumentsValidator(
+ $this->context,
+ Database::VAR_STRING
+ );📝 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.
| $validator = new DocumentsValidator( | |
| $this->context, | |
| Database::VAR_INTEGER | |
| ); | |
| $validator = new DocumentsValidator( | |
| $this->context, | |
| Database::VAR_STRING | |
| ); |
🤖 Prompt for AI Agents
In tests/unit/Validator/DocumentsQueriesTest.php around lines 168 to 171, the
DocumentsValidator is being constructed with Database::VAR_INTEGER for the $id
type but the test expects string IDs; change the second argument to
Database::VAR_STRING so the validator treats IDs as strings; update the
instantiation to pass Database::VAR_STRING and run the tests to confirm
behavior.
| $context = new QueryContext(); | ||
| $context->add($this->collection); | ||
|
|
||
| $validator = new DocumentsValidator($context); | ||
|
|
There was a problem hiding this comment.
Fix: pass required id-attribute type to DocumentsValidator constructor.
V2::__construct(QueryContext $context, string $idAttributeType, ...) requires the second argument; these calls would fatally error. Use a consistent default (string) in tests that don’t care about PK type.
Apply:
- $validator = new DocumentsValidator($context);
+ $validator = new DocumentsValidator($context, Database::VAR_STRING);
@@
- $validator = new DocumentsValidator($context);
+ $validator = new DocumentsValidator($context, Database::VAR_STRING);
@@
- $validator = new DocumentsValidator($context);
+ $validator = new DocumentsValidator($context, Database::VAR_STRING);
@@
- $validator = new DocumentsValidator($context);
+ $validator = new DocumentsValidator($context, Database::VAR_STRING);
@@
- $validator = new DocumentsValidator($context);
+ $validator = new DocumentsValidator($context, Database::VAR_STRING);
@@
- $validator = new DocumentsValidator($context);
+ $validator = new DocumentsValidator($context, Database::VAR_STRING);Also applies to: 50-54, 60-64, 70-74, 164-169, 213-218
🤖 Prompt for AI Agents
In tests/unit/Validator/IndexedQueriesTest.php around lines 40-44 (and also at
50-54, 60-64, 70-74, 164-169, 213-218), the DocumentsValidator is being
constructed without the required second parameter id-attribute type
(V2::__construct requires string $idAttributeType). Update each
DocumentsValidator instantiation to pass a consistent default id attribute type
(e.g., 'string') as the second argument so the constructor receives the required
parameter.
|
|
||
| $context->add($collection); | ||
|
|
||
| $this->validator = new DocumentsValidator($context); |
There was a problem hiding this comment.
Fix constructor call: missing required $idAttributeType.
- $this->validator = new DocumentsValidator($context);
+ $this->validator = new DocumentsValidator($context, Database::VAR_STRING);📝 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.
| $this->validator = new DocumentsValidator($context); | |
| $this->validator = new DocumentsValidator($context, Database::VAR_STRING); |
🤖 Prompt for AI Agents
In tests/unit/Validator/Query/OrderTest.php at line 49, the DocumentsValidator
constructor is called with only $context but the required second parameter
$idAttributeType is missing; update the call to pass the required
$idAttributeType (for example the test-suite's expected constant or string such
as the AttributeType/ID constant used elsewhere in tests) — e.g. new
DocumentsValidator($context, $idAttributeType) or inline the appropriate
constant so the validator is constructed with both $context and the correct id
attribute type.
| $context = new QueryContext(); | ||
|
|
||
| $context->add($collection); | ||
|
|
||
| $this->validator = new DocumentsValidator($context); |
There was a problem hiding this comment.
Validator construction missing required id-attribute type.
V2::__construct requires $idAttributeType (e.g., Database::VAR_STRING). Without it, this will fatal.
- $this->validator = new DocumentsValidator($context);
+ $this->validator = new DocumentsValidator($context, Database::VAR_STRING);📝 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.
| $context = new QueryContext(); | |
| $context->add($collection); | |
| $this->validator = new DocumentsValidator($context); | |
| $context = new QueryContext(); | |
| $context->add($collection); | |
| $this->validator = new DocumentsValidator($context, Database::VAR_STRING); |
🤖 Prompt for AI Agents
In tests/unit/Validator/Query/SelectTest.php around lines 45 to 49, the
DocumentsValidator is being constructed without the required $idAttributeType
parameter (V2::__construct requires it), causing a fatal error; update the
constructor call to pass the appropriate id attribute type (e.g.,
Database::VAR_STRING or the constant used in your test suite) when creating new
DocumentsValidator($context, $idAttributeType) so the validator is initialized
with the correct id type.
| public function testQueryLimit(): void | ||
| { | ||
| $validator = new Documents($this->attributes, [], Database::VAR_INTEGER); | ||
| $validator = new DocumentsValidator($this->context, Database::VAR_INTEGER); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Debug output left in production code
There's a var_dump() statement that should be removed before merging.
Apply this diff to remove the debug statement:
- var_dump($document);🏁 Script executed:
#!/bin/bash
# Search for other var_dump statements in the codebase that might have been added
rg -n "var_dump\(" --type phpLength of output: 1369
Remove all debug var_dump calls
Multiple active var_dump statements remain in both production and test code. Remove them before merging:
- src/Database/Database.php:1523
- src/Database/Validator/Queries/V2.php:180, 688–689
- src/Database/Adapter/SQL.php:195, 374, 2476
- src/Database/Adapter/MariaDB.php:61
- tests/e2e/Adapter/Scopes/JoinsTests.php:333, 375
There was a problem hiding this comment.
Actionable comments posted: 7
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/SQL.php (2)
2357-2413: ORDER BY is fragile: missing alias, no natural-order handling, and breaks when orders are empty.
- Qualify columns with the correct alias to avoid ambiguity in joins.
- Treat empty attribute ('') as natural order by primary key (_id).
- Use each prior sort’s alias when building cursor tie-breakers.
- Guard against empty $orders when composing SQL.
- foreach ($orderQueries as $i => $order) { - $orderAlias = $order->getAlias(); - $attribute = $order->getAttribute(); - $originalAttribute = $attribute; + foreach ($orderQueries as $i => $order) { + $orderAlias = $order->getAlias() ?: Query::DEFAULT_ALIAS; + $attribute = $order->getAttribute(); + $originalAttribute = $attribute; + + // Natural order -> primary key + if ($originalAttribute === '' || $originalAttribute === null) { + $originalAttribute = '$sequence'; + $attribute = '_id'; + } $attribute = $this->getInternalKeyForAttribute($originalAttribute); $attribute = $this->filter($attribute); $direction = $order->getOrderDirection(); if ($cursorDirection === Database::CURSOR_BEFORE) { $direction = ($direction === Database::ORDER_ASC) ? Database::ORDER_DESC : Database::ORDER_ASC; } - $orders[] = "{$this->quote($attribute)} {$direction}"; + $orders[] = "{$this->quote($orderAlias)}.{$this->quote($attribute)} {$direction}"; // Build pagination WHERE clause only if we have a cursor if (!empty($cursor)) { // Special case: No tie breaks. only 1 attribute and it's a unique primary key - if (count($orderQueries) === 1 && $i === 0 && $originalAttribute === '$sequence') { + if (count($orderQueries) === 1 && $i === 0 && $originalAttribute === '$sequence') { $operator = ($direction === Database::ORDER_DESC) ? Query::TYPE_LESSER : Query::TYPE_GREATER; $bindName = ":cursor_pk"; - $binds[$bindName] = $cursor[$originalAttribute]; + $binds[$bindName] = $cursor['$sequence']; - $cursorWhere[] = "{$this->quote($orderAlias)}.{$this->quote($attribute)} {$this->getSQLOperator($operator)} {$bindName}"; + $cursorWhere[] = "{$this->quote($orderAlias)}.{$this->quote($attribute)} {$this->getSQLOperator($operator)} {$bindName}"; break; } $conditions = []; // Add equality conditions for previous attributes for ($j = 0; $j < $i; $j++) { - $prevQuery = $orderQueries[$j]; - $prevOriginal = $prevQuery->getAttribute(); - $prevAttr = $this->filter($this->getInternalKeyForAttribute($prevOriginal)); + $prevQuery = $orderQueries[$j]; + $prevAlias = $prevQuery->getAlias() ?: Query::DEFAULT_ALIAS; + $prevOriginal = $prevQuery->getAttribute(); + if ($prevOriginal === '' || $prevOriginal === null) { + $prevOriginal = '$sequence'; + } + $prevAttr = $this->filter($this->getInternalKeyForAttribute($prevOriginal)); $bindName = ":cursor_{$j}"; - $binds[$bindName] = $cursor[$prevOriginal]; + $binds[$bindName] = $cursor[$prevOriginal]; - $conditions[] = "{$this->quote($orderAlias)}.{$this->quote($prevAttr)} = {$bindName}"; + $conditions[] = "{$this->quote($prevAlias)}.{$this->quote($prevAttr)} = {$bindName}"; } // Add comparison for current attribute $operator = ($direction === Database::ORDER_DESC) ? Query::TYPE_LESSER : Query::TYPE_GREATER; $bindName = ":cursor_{$i}"; - $binds[$bindName] = $cursor[$originalAttribute]; + $binds[$bindName] = $cursor[$originalAttribute]; - $conditions[] = "{$this->quote($orderAlias)}.{$this->quote($attribute)} {$this->getSQLOperator($operator)} {$bindName}"; + $conditions[] = "{$this->quote($orderAlias)}.{$this->quote($attribute)} {$this->getSQLOperator($operator)} {$bindName}"; $cursorWhere[] = '(' . implode(' AND ', $conditions) . ')'; } } @@ - $sqlOrder = 'ORDER BY ' . implode(', ', $orders); + $sqlOrder = empty($orders) ? '' : 'ORDER BY ' . implode(', ', $orders);Also applies to: 2452-2454
521-525: Spatial check bug when binding update values.isset on a numeric-keyed array always fails here. Use in_array to avoid incorrectly JSON-encoding spatial values.
- foreach ($attributes as $attributeName => $value) { - if (!isset($spatialAttributes[$attributeName]) && is_array($value)) { + foreach ($attributes as $attributeName => $value) { + if (!in_array($attributeName, $spatialAttributes, true) && is_array($value)) { $value = json_encode($value); }src/Database/Database.php (1)
6721-6727: Fix undefined variable in decodeOriginal().Uses
$selectionswhich is undefined; should use$selects.- if ( - empty($selections) - || \in_array($key, $selections) - || \in_array('*', $selections) - ) { + if ( + empty($selects) + || \in_array($key, $selects) + || \in_array('*', $selects) + ) {
♻️ Duplicate comments (9)
src/Database/Adapter/SQL.php (1)
1840-1847: $spatialAttributes parameter remains unused.This was flagged previously; either remove it or implement spatial projection handling.
src/Database/Database.php (1)
1523-1523: Remove debug output from production path (duplicate of prior review).
var_dump()will corrupt API responses and leak internals.Apply:
- var_dump($collection->getAttribute('attributes')); + // no-opsrc/Database/Validator/Queries/V2.php (6)
385-387: Allow selecting internal attributes without extra checks (duplicate of prior review).Return early for internal keys.
- if (\in_array($attribute, $internalKeys)) { - //return; - } + if (\in_array($attribute, $internalKeys, true)) { + return; + }
180-180: Remove debug var_dump (duplicate of prior review).Debug prints break output and leak internals.
- var_dump('=== validateAttributeExist'); + // no-op
503-506: Call instance method, not static.Use
$this->isValid(...)for recursion.- if (! self::isValid($query->getValues(), $scope)) { + if (! $this->isValid($query->getValues(), $scope)) { throw new \Exception($this->message); } @@ - if (! self::isValid($query->getValues(), 'joins')) { + if (! $this->isValid($query->getValues(), 'joins')) { throw new \Exception($this->message); }Also applies to: 586-588
674-683: Fix default branch: wrong variables used and missing spatial handling.Reference the current
$queryand validate properly.- default: - if ($value->isSpatialQuery()) { - if ($this->isEmpty($value->getValues())) { - $this->message = \ucfirst($method) . ' queries require at least one value.'; - return false; - } - return $this->isValidAttributeAndValues($attribute, $value->getValues(), $method); - } - - throw new \Exception('Invalid query: Method not found '); + default: + if (Query::isSpatialQuery($method)) { + if ($this->isEmpty($query->getValues())) { + throw new \Exception(\ucfirst($method).' queries require at least one value.'); + } + $this->validateAttributeExist($query->getAttribute(), $query->getAlias()); + $this->validateValues($query->getAttribute(), $query->getAlias(), $query->getValues(), $method); + break; + } + throw new \Exception('Invalid query: Method not found');
688-690: Remove debug var_dump in catch (duplicate of prior review).Do not echo exceptions.
- var_dump($this->message); - var_dump($e); + // logged upstream
271-301: Fix ID/sequence detection and add ObjectId support.
- Wrong comparison when flagging sequence.
- Missing
VAR_OBJECT_IDhandling.- case Database::VAR_ID: - $validator = new Sequence($this->idAttributeType, $attribute === '$sequence'); + case Database::VAR_ID: + $validator = new Sequence($this->idAttributeType, $attributeId === '$sequence'); break; + case Database::VAR_OBJECT_ID: + $validator = new Sequence(Database::VAR_OBJECT_ID, false); + break; @@ - case Database::VAR_RELATIONSHIP: - $validator = new Text(255, 0); // The query is always on uid + case Database::VAR_RELATIONSHIP: + $validator = new Text(0, 255); // The query is always on uid break;src/Database/Query.php (1)
44-45: Broken round‑trip for joins/alias metadata; missing TYPES/isMethod entriesJoins and relationEqual can’t be parsed/serialized; alias/as/collection/aliasRight/system are dropped; join values aren’t recursively serialized. Add types, persist metadata, and recurse for joins.
@@ public const TYPES = [ @@ - self::TYPE_SELECT, + self::TYPE_SELECT, + self::TYPE_RELATION_EQUAL, + self::TYPE_INNER_JOIN, + self::TYPE_LEFT_JOIN, + self::TYPE_RIGHT_JOIN, @@ public static function isMethod(string $value): bool { return match ($value) { @@ - self::TYPE_SELECT => true, + self::TYPE_SELECT, + self::TYPE_RELATION_EQUAL, + self::TYPE_INNER_JOIN, + self::TYPE_LEFT_JOIN, + self::TYPE_RIGHT_JOIN => true, default => false, }; } @@ public function toArray(): array - if (\in_array($array['method'], self::LOGICAL_TYPES)) { + // Persist extended metadata (omit defaults) + if ($this->alias !== self::DEFAULT_ALIAS) { + $array['alias'] = $this->alias; + } + if ($this->as !== '') { + $array['as'] = $this->as; + } + if ($this->collection !== '') { + $array['collection'] = $this->collection; + } + if ($this->attributeRight !== '') { + $array['attributeRight'] = $this->attributeRight; + } + if ($this->aliasRight !== self::DEFAULT_ALIAS) { + $array['aliasRight'] = $this->aliasRight; + } + if ($this->system === true) { + $array['system'] = true; + } + + // Logical and Join queries nest other queries + if (\in_array($array['method'], self::LOGICAL_TYPES, true) || $this->isJoin()) { foreach ($this->values as $index => $value) { $array['values'][$index] = $value->toArray(); } } else { @@ public static function parseQuery(array $query): self - $method = $query['method'] ?? ''; - $attribute = $query['attribute'] ?? ''; - $values = $query['values'] ?? []; + $method = $query['method'] ?? ''; + $attribute = $query['attribute'] ?? ''; + $values = $query['values'] ?? []; + $alias = $query['alias'] ?? ''; + $as = $query['as'] ?? ''; + $collection = $query['collection'] ?? ''; + $attributeRight = $query['attributeRight'] ?? ''; + $aliasRight = $query['aliasRight'] ?? ''; + $system = $query['system'] ?? false; @@ - if (\in_array($method, self::LOGICAL_TYPES)) { + if (\in_array($method, self::LOGICAL_TYPES, true) || \in_array($method, [self::TYPE_INNER_JOIN, self::TYPE_LEFT_JOIN, self::TYPE_RIGHT_JOIN], true)) { foreach ($values as $index => $value) { $values[$index] = self::parseQuery($value); } } - return new self($method, $attribute, $values); + if (!\is_string($alias)) { throw new QueryException('Invalid query alias. Must be a string, got ' . \gettype($alias)); } + if (!\is_string($as)) { throw new QueryException('Invalid query as. Must be a string, got ' . \gettype($as)); } + if (!\is_string($collection)) { throw new QueryException('Invalid query collection. Must be a string, got ' . \gettype($collection)); } + if (!\is_string($attributeRight)) { throw new QueryException('Invalid query attributeRight. Must be a string, got ' . \gettype($attributeRight)); } + if (!\is_string($aliasRight)) { throw new QueryException('Invalid query aliasRight. Must be a string, got ' . \gettype($aliasRight)); } + if (!\is_bool($system)) { throw new QueryException('Invalid query system. Must be a boolean, got ' . \gettype($system)); } + + return new self( + $method, + $attribute, + $values, + alias: $alias, + attributeRight: $attributeRight, + aliasRight: $aliasRight, + collection: $collection, + as: $as, + system: $system, + );Also applies to: 64-70, 73-113, 363-407, 462-491, 512-538
🧹 Nitpick comments (13)
src/Database/Adapter/SQL.php (3)
1854-1856: Default alias safeguard in projection.Avoid generating ````.col`` when alias is empty.
- $alias = $select->getAlias(); - $alias = $this->filter($alias); + $alias = $select->getAlias() ?: Query::DEFAULT_ALIAS; + $alias = $this->filter($alias);
2341-2344: Nit: unsetting an unused parameter is unnecessary.Drop the unset; it adds noise.
- ): array { - unset($queries); // remove this since we pass explicit queries + ): array {
393-416: Optional: map _collection to $collection for symmetry.Results already map other internals; consider mapping _collection too for consistency.
if (\array_key_exists('_permissions', $document)) { $document['$permissions'] = json_decode($document['_permissions'] ?? '[]', true); unset($document['_permissions']); } + if (\array_key_exists('_collection', $document)) { + $document['$collection'] = $document['_collection']; + unset($document['_collection']); + }And inside find() loop:
if (\array_key_exists('_permissions', $document)) { $results[$index]['$permissions'] = \json_decode($document['_permissions'] ?? '[]', true); unset($results[$index]['_permissions']); } + if (\array_key_exists('_collection', $document)) { + $results[$index]['$collection'] = $document['_collection']; + unset($results[$index]['_collection']); + }Also applies to: 2496-2523
src/Database/Database.php (3)
6285-6300: Avoid recomputing grouped data; pick one source of truth.Values from
groupByType()are immediately overwritten by manual getters; keep one path to reduce drift.- $grouped = Query::groupByType($queries); - $filters = $grouped['filters']; - $selects = $grouped['selections']; - $limit = $grouped['limit']; - $offset = $grouped['offset']; - $orderAttributes = $grouped['orderAttributes']; - $orderTypes = $grouped['orderTypes']; - $cursor = $grouped['cursor']; - $cursorDirection = $grouped['cursorDirection'] ?? Database::CURSOR_AFTER; - - $filters = Query::getFilterQueries($queries); - $selects = Query::getSelectQueries($queries); - $limit = Query::getLimitQuery($queries, 25); - $offset = Query::getOffsetQuery($queries, 0); - $orders = Query::getOrderQueries($queries); + $grouped = Query::groupByType($queries); + $filters = $grouped['filters']; + $selects = $grouped['selections']; + $limit = $grouped['limit'] ?? 25; + $offset = $grouped['offset'] ?? 0; + $orders = $grouped['orderQueries'] ?? Query::getOrderQueries($queries); + $cursor = $grouped['cursor'] ?? []; + $cursorDirection = $grouped['cursorDirection'] ?? Database::CURSOR_AFTER;
4471-4475: Return pipeline consistency: cast before decode in updateDocument().Other paths cast then decode; do the same here for consistent types.
- $context = new QueryContext(); - $context->add($collection); - - $document = $this->decode($context, $document); + $context = new QueryContext(); + $context->add($collection); + $document = $this->casting($context, $document); + $document = $this->decode($context, $document);
7229-7231: Stabilize cache key hashing for selects.
md5(serialize($selects))can be brittle across PHP versions/object state. Normalize to a deterministic string (method|alias|attr|as).- if (!empty($selects)) { - $documentHashKey = $documentKey . ':' . \md5(\serialize($selects)); - } + if (!empty($selects)) { + $norm = array_map(fn($q) => implode('|', [ + $q->getMethod(), $q->getAlias(), $q->getAttribute(), $q->getAs() ?? '' + ]), $selects); + $documentHashKey = $documentKey . ':' . \md5(\implode(';', $norm)); + }src/Database/Validator/Queries/V2.php (2)
263-265: Fix spatial error message to reference the right attribute.Use
$attributeIdand include actual type for clarity.- if (Query::isSpatialQuery($method) && !in_array($attribute['type'], Database::SPATIAL_TYPES, true)) { - throw new \Exception('Invalid query: Spatial query "' . $method . '" cannot be applied on non-spatial attribute: ' . $attribute); + if (Query::isSpatialQuery($method) && !in_array($attribute['type'], Database::SPATIAL_TYPES, true)) { + throw new \Exception( + 'Invalid query: Spatial query "'.$method.'" cannot be applied on non-spatial attribute "'.$attributeId.'" (type '.$attribute['type'].').' + ); }
391-407: Tighten dotted-attribute handling.Remove duplicated
str_containsand keep the try/catch logic.- if (\str_contains($attribute, '.')) { - if (\str_contains($attribute, '.')) { + if (\str_contains($attribute, '.')) {src/Database/Query.php (5)
710-714: select(): unused $function parameter$function is accepted but ignored. Either persist it (e.g., into values/metadata) or remove it for now to avoid dead API.
Do you want me to wire this into toArray()/parseQuery as a "function" field?
1194-1201: Make isNested honor join queriesCallers expecting “container” queries will likely want joins considered nested.
- if (in_array($this->getMethod(), self::LOGICAL_TYPES)) { + if (in_array($this->getMethod(), self::LOGICAL_TYPES, true) || $this->isJoin()) { return true; }Also applies to: 1204-1215
1217-1221: Use strict in_array for method checksAvoid accidental truthiness collisions.
- return in_array($method, self::FILTER_TYPES); + return in_array($method, self::FILTER_TYPES, true);
307-331: Throw QueryException, not generic ExceptionAligns with the rest of the class’ error handling.
- 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');
409-416: Tighten param type for isSpatialQuerySignature/docblock mismatch; accept string explicitly.
- /** - * Check if method is a spatial-only query method - * @param $method - * @return bool - */ - public static function isSpatialQuery($method): bool + /** + * Check if method is a spatial-only query method + * @param string $method + * @return bool + */ + public static function isSpatialQuery(string $method): bool
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Database/Adapter/SQL.php(10 hunks)src/Database/Database.php(50 hunks)src/Database/Query.php(21 hunks)src/Database/Validator/Queries/V2.php(1 hunks)tests/e2e/Adapter/Scopes/DocumentTests.php(16 hunks)
🔇 Additional comments (8)
tests/e2e/Adapter/Scopes/DocumentTests.php (6)
21-21: QueryContext import aligns with new decode/selection API.Import looks correct and consistent with the new flow.
1308-1339: Per-field select assertions are clear and correct.Good verification of inclusion/exclusion for user and internal fields with/without selecting $id.
2929-2946: Scoped projection before nested OR reduces payload and matches new exposure rules.Selecting only 'director' here is appropriate given the assertions.
3454-3590: Comprehensive coverage of select() for special fields.Nice matrix of $id/$sequence/$collection/$createdAt/$updatedAt/$permissions variants—this will catch regressions in projection and internal-field exposure.
3940-3944: Decode now uses QueryContext.Looks good; ensure all call sites are updated to pass QueryContext consistently.
4678-4685: Short-selects in bulk delete are well-formed.Using array_map to build Query::select() list is concise and readable.
Also applies to: 4887-4917
src/Database/Query.php (2)
560-575: API surface additions look goodAlias-aware builders and join/relation helpers are consistent and useful. Once round-trip is fixed, these will integrate cleanly.
Also applies to: 584-623, 657-660, 721-735, 885-927, 929-933
120-144: ```shell
#!/bin/bashVerify duplicate TYPE_GREATER_EQUAL, FILTER_TYPES, getFilterQueries, and TYPE_AND/TYPE_OR usage
Show occurrences of TYPE_GREATER_EQUAL in Query.php
rg -n "TYPE_GREATER_EQUAL" -g "src/Database/Query.php"
Display FILTER_TYPES definition
rg -n "protected const FILTER_TYPES" -g "src/Database/Query.php" -C3
Locate getFilterQueries method to see how FILTER_TYPES is used
rg -nP --type=php "function getFilterQueries" -g "src/Database/Query.php" -C5
Check where TYPE_AND and TYPE_OR constants are referenced
rg -n "TYPE_AND" -g "src/Database/Query.php"
rg -n "TYPE_OR" -g "src/Database/Query.php"</blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| var_dump($document); | ||
| } catch (PDOException $e) { |
There was a problem hiding this comment.
Remove debug output in exists().
Leftover var_dump will leak internals to stdout and break consumers.
Apply this diff:
- $stmt->closeCursor();
- var_dump($document);
+ $stmt->closeCursor();📝 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.
| var_dump($document); | |
| } catch (PDOException $e) { | |
| $stmt->closeCursor(); | |
| } catch (PDOException $e) { |
🤖 Prompt for AI Agents
In src/Database/Adapter/SQL.php around lines 195-196 there is a leftover
var_dump($document) inside the exists() method; remove that debug output so
internals are not printed to stdout and instead either do nothing or log via the
project's logger (e.g. $this->logger->debug(...)) if diagnostic information is
required, ensuring no var_dump or echo statements remain in production code.
| var_dump($sql); | ||
| $stmt = $this->getPDO()->prepare($sql); |
There was a problem hiding this comment.
Remove debug output in getDocument().
var_dump($sql) should not ship.
- if ($this->getSupportForUpdateLock()) {
- $sql .= " {$forUpdate}";
- }
-var_dump($sql);
+ if ($this->getSupportForUpdateLock()) {
+ $sql .= " {$forUpdate}";
+ }📝 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.
| var_dump($sql); | |
| $stmt = $this->getPDO()->prepare($sql); | |
| if ($this->getSupportForUpdateLock()) { | |
| $sql .= " {$forUpdate}"; | |
| } | |
| $stmt = $this->getPDO()->prepare($sql); |
🤖 Prompt for AI Agents
In src/Database/Adapter/SQL.php around lines 374 to 375, remove the debug
var_dump($sql) left in getDocument() so SQL is not printed to output; simply
delete that var_dump call so the code proceeds directly to preparing the
statement (keep the $stmt = $this->getPDO()->prepare($sql); line intact).
| var_dump($sql); | ||
| $sql = $this->trigger(Database::EVENT_DOCUMENT_FIND, $sql); |
There was a problem hiding this comment.
Remove debug output in find().
Same issue as above.
- ";
-var_dump($sql);
+ ";Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/Database/Adapter/SQL.php around lines 2474-2475, remove the leftover
debug var_dump($sql) call in find(); replace it by simply proceeding to trigger
the event and return as intended. Ensure no debug output remains (remove the
var_dump line) so the method does not print SQL to stdout, and keep the
subsequent $sql = $this->trigger(Database::EVENT_DOCUMENT_FIND, $sql); line
intact.
| $filters = []; | ||
| $joins = []; | ||
| $selections = []; |
There was a problem hiding this comment.
groupByType bugs: wrong fallbacks; joins misclassified as filters
- $offset uses $limit as fallback; should use $offset.
- $cursor uses $limit as fallback; should use $cursor.
- Join methods fall into default and are added to filters.
case Query::TYPE_OFFSET:
@@
- $offset = $values[0] ?? $limit;
+ $offset = $values[0] ?? $offset;
break;
case Query::TYPE_CURSOR_AFTER:
case Query::TYPE_CURSOR_BEFORE:
@@
- $cursor = $values[0] ?? $limit;
+ $cursor = $values[0] ?? $cursor;
$cursorDirection = $method === Query::TYPE_CURSOR_AFTER ? Database::CURSOR_AFTER : Database::CURSOR_BEFORE;
break;
+ case Query::TYPE_INNER_JOIN:
+ case Query::TYPE_LEFT_JOIN:
+ case Query::TYPE_RIGHT_JOIN:
+ // Do not mix joins into filters; consumers can use Query::getJoinQueries($queries)
+ break;Also applies to: 1128-1175
🤖 Prompt for AI Agents
In src/Database/Query.php around lines 1109-1111 (and similarly 1128-1175), the
groupByType logic wrongly uses $limit as the fallback for $offset and $cursor
and it treats join-related methods as default filters; update the switch/case
(or mapping) so $offset falls back to $offset (not $limit) and $cursor falls
back to $cursor, and explicitly handle join method names so they are appended to
the $joins array instead of being placed in $filters; ensure default still adds
true filters only and adjust any variable names used in fallbacks and
assignments accordingly.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/e2e/Adapter/Scopes/RelationshipTests.php (1)
334-337: Nit: collapse consecutive selects into a single variadic call (if supported).
Minor readability win; same behavior with fewer Query objects.- Query::select('*'), - Query::select('votes.*'), - Query::select('votes.animals.*'), + Query::select('*', 'votes.*', 'votes.animals.*'),tests/e2e/Adapter/Base.php (2)
11-11: Unused import: JoinsTests.
Either enable the trait below or drop this import to avoid dead code.-use Tests\E2E\Adapter\Scopes\JoinsTests; +// use Tests\E2E\Adapter\Scopes\JoinsTests;
22-27: Big test coverage drop — confirm intent and timeline to re-enable.
Commenting out core suites (Collection/Document/Attribute/Index/Permission) hides regressions. If this is temporary for Joins/QueryContext work, add a TODO and an issue link, or gate with @group to keep code visible in-base.I can propose a plan to re-enable incrementally or annotate with @group; want me to open a tracking ticket and prep diffs?
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/e2e/Adapter/Base.php(2 hunks)tests/e2e/Adapter/Scopes/RelationshipTests.php(3 hunks)
🔇 Additional comments (2)
tests/e2e/Adapter/Scopes/RelationshipTests.php (2)
323-325: LGTM: additive Query::select() usage is clear and aligns with intent.
Keeps base fields while resolving votes; assertions match expected shape.
349-349: LGTM: '*' alone should not resolve relations.
The following assertions correctly verify non-resolution of nested relations.
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)
6685-6685: Fix undefined variable in decodeOriginal(): use $selects (Query[]) not $selections.Current code references
$selections, causing notices and incorrect filtering.foreach ($this->getInternalAttributes() as $attribute) { $attributes[] = $attribute; } + // Normalize selects (support both Query[] and string[] for backward compatibility) + $selected = \array_map( + fn ($s) => $s instanceof Query ? $s->getAttribute() : $s, + $selects + ); + foreach ($attributes as $attribute) { $key = $attribute['$id'] ?? ''; @@ - if ( - empty($selections) - || \in_array($key, $selections) - || \in_array('*', $selections) - ) { + if ( + empty($selected) + || \in_array($key, $selected, true) + || \in_array('*', $selected, true) + ) { $document->setAttribute($key, ($array) ? $value : $value[0]); }Also applies to: 6722-6729
♻️ Duplicate comments (3)
src/Database/Database.php (3)
1523-1523: Remove debug output from production path.Leftover var_dump leaks internals and corrupts responses. Delete it.
- var_dump($collection->getAttribute('attributes')); + // removed debug output
7157-7157: Remove stray var_dump in convertQuery().Debug print in hot path breaks responses and logs sensitive data.
- var_dump($values); + // removed debug output
6303-6311: Cursor safety: require unique order or fallback to $id; avoid empty ORDER BY.
Query::orderAsc()without an attribute is invalid and will also make cursor paging unsafe. Enforce unique order when a cursor is present; otherwise, add a deterministic fallback on$idonly when not using joins.- if ($uniqueOrderBy === false) { - $orders[] = Query::orderAsc(); // In joins we should not add a default order, we should validate when using a cursor we should have a unique order - } + if ($uniqueOrderBy === false) { + if (!empty($cursor)) { + throw new OrderException("Cursor requires unique order by '\$id' or '\$sequence'."); + } + if (empty($joins)) { + $orders[] = Query::orderAsc('$id'); + } + } if (!empty($cursor)) { foreach ($orders as $order) { if ($cursor->getAttribute($order->getAttribute()) === null) { throw new OrderException( - message: "Order attribute '{$order->getAttribute()}' is empty", + message: "Order attribute '{$order->getAttribute()}' is empty", attribute: $order->getAttribute() ); } } }Also applies to: 6313-6321
🧹 Nitpick comments (5)
src/Database/Database.php (5)
6285-6300: Avoid double-parsing queries; use one source of truth.You compute grouped fields via
groupByType()then immediately re-derive them viaQuery::get*(); this risks divergence and increases maintenance.- $filters = Query::getFilterQueries($queries); - $selects = Query::getSelectQueries($queries); - $limit = Query::getLimitQuery($queries, 25); - $offset = Query::getOffsetQuery($queries, 0); - $orders = Query::getOrderQueries($queries); + // Prefer using either $grouped[...] or the Query::get* helpers, not both. + $orders = Query::getOrderQueries($queries);
6784-6786: Throw QueryException for unknown alias context.Use the project’s query exception type for consistency and better handling.
- if ($collection->isEmpty()) { - throw new \Exception('Invalid query: Unknown Alias context'); - } + if ($collection->isEmpty()) { + throw new QueryException('Invalid query: Unknown alias context'); + }Also applies to: 6881-6883
7230-7232: Stabilize cache key hashing for selects.
md5(serialize($selects))on objects can be brittle. Hash a normalized tuple of (alias|attribute|as) to avoid serialization quirks.- if (!empty($selects)) { - $documentHashKey = $documentKey . ':' . \md5(\serialize($selects)); - } + if (!empty($selects)) { + $selParts = \array_map( + fn ($s) => $s instanceof Query + ? ($s->getAlias() . '|' . $s->getAttribute() . '|' . ($s->getAs() ?? '')) + : (string)$s, + $selects + ); + $documentHashKey = $documentKey . ':' . \md5(\implode(',', $selParts)); + }
4471-4475: Consistency: apply casting before decode in updateDocument().Other paths call
casting()thendecode(). Do the same here to keep output typing consistent.- $context = new QueryContext(); - $context->add($collection); - - $document = $this->decode($context, $document); + $context = new QueryContext(); + $context->add($collection); + $document = $this->casting($context, $document); + $document = $this->decode($context, $document);
7035-7041: validateSelections() currently mixes Query objects with strings.If this helper will be revived, normalize inputs to strings before diffing, or operate directly on Query instances.
- foreach ($queries as $query) { + foreach ($queries as $query) { if ($query->getMethod() == Query::TYPE_SELECT) { - if (\str_contains($query->getValue(), '.')) { + $values = $query->getValues(); + if (\count($values) === 1 && \str_contains($values[0], '.')) { $relationshipSelections[] = $query; continue; } - $selections[] = $query; + $selections = \array_merge($selections, $values); } } @@ - $invalid = \array_diff($selections, $keys); + $invalid = \array_diff($selections, $keys);Also applies to: 7057-7071
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Database/Database.php(51 hunks)tests/e2e/Adapter/Scopes/RelationshipTests.php(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/e2e/Adapter/Scopes/RelationshipTests.php
🔇 Additional comments (2)
src/Database/Database.php (2)
3355-3365: QueryContext threading and V2 validator integration look solid.Context-aware validation, casting/decoding, and join-aware selections are wired coherently across get/find/create/update/batch ops.
Also applies to: 3361-3365, 3375-3381, 3436-3438, 3823-3828, 3870-3872, 3929-3931, 4528-4536, 4676-4676, 5153-5155, 6033-6045, 6237-6261, 6351-6353, 6526-6554, 6465-6487
4613-4621: Batch ops: required attribute presence checks are good.Enforcing
$permissionsand$sequencein selects prevents downstream errors and permission drift.Also applies to: 6092-6100
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/Database.php (3)
6659-6732: Fix undefined variable and selection handling in decodeOriginal().Uses undefined $selections and ignores Query objects. This will trigger notices and incorrect filtering.
- public function decodeOriginal(Document $collection, Document $document, array $selects = []): Document + public function decodeOriginal(Document $collection, Document $document, array $selects = []): Document { + // Normalize selected attributes from Query[] or string[] + $selectedAttributes = []; + foreach ($selects as $s) { + if ($s instanceof Query && $s->getMethod() === Query::TYPE_SELECT) { + $selectedAttributes[] = $s->getAttribute(); + } elseif (is_string($s)) { + $selectedAttributes[] = $s; + } + } ... - if ( - empty($selections) - || \in_array($key, $selections) - || \in_array('*', $selections) - ) { + if ( + empty($selectedAttributes) + || \in_array($key, $selectedAttributes, true) + || \in_array('*', $selectedAttributes, true) + ) { $document->setAttribute($key, ($array) ? $value : $value[0]); }
4595-4607: Batch update requires $permissions and $sequence in select; add them automatically.Avoids surprising QueryException when callers don't specify these internals.
- $batch = $this->silent(fn () => $this->find( + // Ensure required internals are selected + [$queries] = Query::addSelect($queries, Query::select('$permissions', system: true)); + [$queries] = Query::addSelect($queries, Query::select('$sequence', system: true)); + + $batch = $this->silent(fn () => $this->find( $collection->getId(), array_merge($new, $queries), forPermission: Database::PERMISSION_UPDATE ));
6079-6086: Batch delete requires $permissions and $sequence in select; add them automatically.Same requirement as updates.
- $batch = $this->silent(fn () => $this->find( + // Ensure required internals are selected + [$queries] = Query::addSelect($queries, Query::select('$permissions', system: true)); + [$queries] = Query::addSelect($queries, Query::select('$sequence', system: true)); + + $batch = $this->silent(fn () => $this->find( $collection->getId(), array_merge($new, $queries), forPermission: Database::PERMISSION_DELETE ));
♻️ Duplicate comments (2)
src/Database/Database.php (2)
1523-1523: Remove debug var_dump from production path.This dumps internals and corrupts responses.
- var_dump($collection->getAttribute('attributes')); + // removed debug output
7155-7158: Remove stray var_dump($values) in convertQuery().Leaking values during query conversion breaks API responses.
- if ($attribute->getAttribute('type') == Database::VAR_DATETIME) { - $values = $query->getValues(); - var_dump($values); + if ($attribute->getAttribute('type') == Database::VAR_DATETIME) { + $values = $query->getValues();
🧹 Nitpick comments (7)
tests/e2e/Adapter/Scopes/RelationshipTests.php (2)
360-361: Make the test deterministic by filtering the subject document.Without a filter,
findOne('veterinarians', [...])depends on insertion order. Add an$idfilter to avoid ordering-related flakes.Apply:
- [ - Query::select('animals.*') - ] + [ + Query::select('animals.*'), + Query::equal('$id', ['dr.pol']), + ]
1342-1345: Clean up stray commented code and migrate legacy Query::select calls
- Remove the commented line in tests/e2e/Adapter/Scopes/RelationshipTests.php (around lines 1342–1345):
- Query::select('name'), - //Query::select('*'), - Query::select('models.name'), + Query::select('name'), + Query::select('models.name'),
- Migrate the 22 remaining
Query::select([...])usages (per yourrgoutput) to the additive API (->select('a')->select('b')) or wrap them inQuery::select_old([...])during transition.src/Database/Database.php (5)
7228-7233: Make cache key hashing stable across PHP versions.serialize($selects) on objects can be brittle. Normalize selects (alias|attribute|as|function|system) before hashing.
- if (!empty($selects)) { - $documentHashKey = $documentKey . ':' . \md5(\serialize($selects)); - } + if (!empty($selects)) { + $documentHashKey = $documentKey . ':' . $this->hashSelects($selects); + }Add helper:
private function hashSelects(array $selects): string { $parts = []; foreach ($selects as $s) { if ($s instanceof Query && $s->getMethod() === Query::TYPE_SELECT) { $parts[] = implode('|', [ $s->getAlias(), $s->getAttribute(), $s->getAs(), $s->getFunction(), $s->isSystem() ? '1' : '0', ]); } } sort($parts); return md5(implode(';', $parts)); }
6285-6294: Avoid duplicated query parsing.You compute grouped and then re-derive filters/selects/limit/offset/orders. Keep one code path.
Also applies to: 6295-6300
6492-6497: Initialize $skipAuth for clarity.Prevents reliance on null coalescing and improves readability.
- $authorization = new Authorization(self::PERMISSION_READ); + $skipAuth = false; + $authorization = new Authorization(self::PERMISSION_READ); if ($authorization->isValid($collection->getRead())) { - $skipAuth = true; + $skipAuth = true; }Also applies to: 6529-6533
6766-6781: Avoid O(n×m) scanning of selects during decode/casting.Pre-index selects by as/attribute to remove per-field scans.
Example:
$selectIndex = []; foreach ($selects as $s) { if ($s->getMethod() === Query::TYPE_SELECT) { $selectIndex[$s->getAs() ?: $this->adapter->filter($s->getAttribute())] = $s; } } // then look up $selectIndex[$key] instead of looping $selects each timeAlso applies to: 6864-6878
7261-7266: Docblock mismatch: not passed by reference.Update the comment or change signature; currently, $queries is not by reference.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Database/Database.php(51 hunks)tests/e2e/Adapter/Scopes/RelationshipTests.php(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/e2e/Adapter/Scopes/RelationshipTests.php (1)
src/Database/Query.php (2)
Query(8-1426)select(710-713)
src/Database/Database.php (7)
src/Database/Validator/Queries/V2.php (3)
V2(23-696)isEmpty(155-166)isValid(472-695)src/Database/Query.php (15)
getAttribute(224-227)Query(8-1426)getSelectQueries(958-963)addSelect(1392-1425)select(710-713)getJoinQueries(969-976)join(891-894)getCollection(266-269)getFilterQueries(1087-1090)limit(743-746)getLimitQuery(998-1007)offset(754-757)getOffsetQuery(1029-1038)getOrderQueries(1044-1050)orderAsc(732-735)src/Database/Document.php (9)
getAttribute(224-231)getId(63-66)isEmpty(396-399)removeAttribute(287-293)Document(12-470)getCollection(85-88)getPermissionsByType(145-157)find(304-322)getArrayCopy(423-458)src/Database/QueryContext.php (6)
QueryContext(8-89)add(60-68)getCollections(32-35)skipAuth(75-86)addSkipAuth(70-73)getCollectionByAlias(37-55)src/Database/Adapter/SQL.php (5)
getIdAttributeType(1731-1734)getDocument(353-419)find(2328-2530)getSupportForCasting(1434-1437)count(2542-2604)src/Database/Adapter.php (6)
getIdAttributeType(905-905)getDocument(691-691)filter(1164-1173)find(805-817)getSupportForCasting(986-986)count(840-840)src/Database/Validator/Authorization.php (3)
Authorization(7-225)isValid(55-77)skip(160-170)
🪛 PHPMD (2.15.0)
src/Database/Database.php
6659-6659: Avoid unused parameters such as '$selects'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (3)
tests/e2e/Adapter/Scopes/RelationshipTests.php (2)
349-351: Confirm semantics:select('*')implies no relationship resolving.The test relies on adapters consistently treating
Query::select('*')as "no resolving." Please verify behavior across all adapters (MySQL, MariaDB, etc.) to avoid adapter-specific flakes.
378-381: LGTM: additive, nested selects are clear and precise.This sequence cleanly expresses required expansions.
src/Database/Database.php (1)
3361-3365: Good upgrade to V2 validator with context.Stronger, context-aware validation path for getDocument().
| $collection = $context->getCollectionByAlias($alias); | ||
| if ($collection->isEmpty()) { | ||
| throw new \Exception('Invalid query: Unknown Alias context'); | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Throw QueryException for unknown alias context.
Aligns with database/query error semantics and avoids leaking generic \Exception.
- if ($collection->isEmpty()) {
- throw new \Exception('Invalid query: Unknown Alias context');
- }
+ if ($collection->isEmpty()) {
+ throw new QueryException('Unknown alias context in query.');
+ }Also applies to: 6881-6884
🤖 Prompt for AI Agents
In src/Database/Database.php around lines 6782-6786, the code throws a generic
\Exception when the alias context is unknown; replace that with throwing the
appropriate QueryException (use the existing QueryException class/namespace used
elsewhere) and provide a clear message like "Unknown alias context" so it fits
database/query error semantics; also make the identical change at the other
occurrence around lines 6881-6884 to ensure consistent error typing across both
sites.
| Query::select('*'), | ||
| Query::select('votes.*'), | ||
| Query::select('votes.animals.*'), | ||
| Query::equal('$id', ['trump']) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Same ordering hazard here; place most-specific first.
To ensure 'votes.animals.*' is honored regardless of addSelect() behavior, order from most- to least-specific.
Apply:
- Query::select('*'),
- Query::select('votes.*'),
- Query::select('votes.animals.*'),
+ Query::select('votes.animals.*'),
+ Query::select('votes.*'),
+ Query::select('*'),🤖 Prompt for AI Agents
In tests/e2e/Adapter/Scopes/RelationshipTests.php around lines 334 to 337, the
Query::select items are ordered least- to most-specific so 'votes.animals.*' can
be overridden by addSelect behavior; reorder the selects so the most-specific
comes first (e.g., Query::select('votes.animals.*'), then
Query::select('votes.*'), then Query::select('*')), leaving the
Query::equal('$id', ['trump']) unchanged, to ensure 'votes.animals.*' is
honored.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/Database/Database.php (2)
1-7447: Remove all debug prints and replace generic Exceptions
- Remove
var_dumpcalls at lines 1523, 3489, 6351–6353, and 7163, as well as theecho PHP_EOL…at line 6350.- Replace the three occurrences of
at lines 6790, 6888, and 7138 withthrow new \Exception('Invalid query: Unknown Alias context');throw new QueryException('Invalid query: Unknown Alias context');.
3355-3462: Ensure system fields are auto-included for batch ops
- In updateDocuments() and deleteDocuments(), you validate that each batch entry has ‘$permissions’ and ‘$sequence’, but the internal find() call uses Query::getSelectQueries() without adding those system selects. As a result, calls without explicit selects will throw at runtime.
- Fix by auto-including Query::select('$permissions', system: true) and Query::select('$sequence', system: true) in the find() invocation for update/delete, or centralize in find() when forPermission ≠ PERMISSION_READ. Document this policy clearly.
♻️ Duplicate comments (3)
src/Database/Database.php (3)
1523-1523: Remove debug output from production path.Leftover var_dump will break responses and leak internals.
- var_dump($collection->getAttribute('attributes')); + // no-op
7163-7164: Delete leftover var_dump in convertQuery().Leaks internal data and breaks output.
- var_dump($values);
6310-6312: Cursor + default order: require unique order when using a cursor; default only when no cursor.Prevents unstable pagination and invalid cursor checks.
- if ($uniqueOrderBy === false) { - $orders[] = Query::orderAsc(); // In joins we should not add a default order, we should validate when using a cursor we should have a unique order - } + if ($uniqueOrderBy === false && empty($cursor)) { + $orders[] = Query::orderAsc('$id'); + } - if (!empty($cursor)) { + if (!empty($cursor)) { + if ($uniqueOrderBy === false) { + throw new OrderException("A unique order by '\$id' or '\$sequence' is required when using a cursor."); + } foreach ($orders as $order) { if ($cursor->getAttribute($order->getAttribute()) === null) { throw new OrderException( message: "Order attribute '{$order->getAttribute()}' is empty", attribute: $order->getAttribute() ); } } }Also applies to: 6314-6323
🧹 Nitpick comments (2)
src/Database/Database.php (2)
6285-6303: Redundant query grouping vs. getters.You compute grouped values then recompute filters/selects/limit/offset/orders from $queries. Prefer one source to avoid drift; keep $grouped (needed for cursor) and drop the re-derivations.
If you agree, remove the re-derivation block and use $grouped['filters'], $grouped['selections'], etc., or keep the getters and only use $grouped for $cursor/$cursorDirection.
6749-6951: DRY up decode() and casting() pipelines.Both duplicate alias resolution, schema lookup, and iteration. Extract a shared internal helper to reduce maintenance surface.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Database/Database.php(52 hunks)tests/e2e/Adapter/Scopes/RelationshipTests.php(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/e2e/Adapter/Scopes/RelationshipTests.php
🧰 Additional context used
🧬 Code graph analysis (1)
src/Database/Database.php (5)
src/Database/Validator/Queries/V2.php (3)
V2(23-696)isEmpty(155-166)isValid(472-695)src/Database/Query.php (20)
getAttribute(224-227)Query(8-1426)getSelectQueries(958-963)addSelect(1392-1425)select(710-713)getJoinQueries(969-976)join(891-894)getCollection(266-269)getAlias(246-249)getFilterQueries(1087-1090)limit(743-746)getLimitQuery(998-1007)offset(754-757)getOffsetQuery(1029-1038)getOrderQueries(1044-1050)getAs(261-264)getMethod(216-219)getValue(241-244)isNested(1194-1201)getValues(232-235)src/Database/QueryContext.php (6)
QueryContext(8-89)add(60-68)getCollections(32-35)skipAuth(75-86)addSkipAuth(70-73)getCollectionByAlias(37-55)src/Database/Adapter/SQL.php (4)
getIdAttributeType(1731-1734)getDocument(353-419)find(2328-2530)getSupportForCasting(1434-1437)src/Database/Adapter.php (5)
getIdAttributeType(905-905)getDocument(691-691)filter(1164-1173)find(805-817)getSupportForCasting(986-986)
🪛 PHPMD (2.15.0)
src/Database/Database.php
6665-6665: Avoid unused parameters such as '$selects'. (undefined)
(UnusedFormalParameter)
| * @throws DatabaseException | ||
| */ | ||
| public function decode(Document $collection, Document $document, array $selections = []): Document | ||
| public function decodeOriginal(Document $collection, Document $document, array $selects = []): Document |
There was a problem hiding this comment.
Fix undefined variable in decodeOriginal() and actually honor $selects.
$selections undefined => notices and wrong filtering; also resolves PHPMD UnusedFormalParameter.
- public function decodeOriginal(Document $collection, Document $document, array $selects = []): Document
+ public function decodeOriginal(Document $collection, Document $document, array $selects = []): Document
{
+ // Normalize selects -> attribute names
+ $selections = array_map(
+ fn ($q) => $q instanceof Query ? $q->getAttribute() : (string)$q,
+ $selects
+ );
@@
- if (
- empty($selections)
- || \in_array($key, $selections)
- || \in_array('*', $selections)
- ) {
+ if (
+ empty($selections)
+ || \in_array($key, $selections, true)
+ || \in_array('*', $selections, true)
+ ) {
$document->setAttribute($key, ($array) ? $value : $value[0]);
}Also applies to: 6728-6735
🧰 Tools
🪛 PHPMD (2.15.0)
6665-6665: Avoid unused parameters such as '$selects'. (undefined)
(UnusedFormalParameter)
🤖 Prompt for AI Agents
In src/Database/Database.php around lines 6665 and 6728-6735, the
decodeOriginal() implementation uses an undefined $selections variable and
ignores the $selects parameter, causing notices and incorrect filtering; replace
the undefined variable with the provided $selects and actually apply it when
building the returned Document: if $selects is empty return the original
Document unchanged, otherwise convert $selects to a lookup (e.g. array_flip) and
construct a new Document that only includes keys present in the lookup (or
remove keys not selected) so no undefined-variable notices occur and the method
honors the selection list; also remove any leftover unused-parameter
workarounds.
| if ($collection->isEmpty()) { | ||
| throw new \Exception('Invalid query: Unknown Alias context'); | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use QueryException instead of generic \Exception for unknown alias context.
Aligns with error semantics and improves catchability.
@@
- if ($collection->isEmpty()) {
- throw new \Exception('Invalid query: Unknown Alias context');
- }
+ if ($collection->isEmpty()) {
+ throw new QueryException('Unknown alias context in query.');
+ }
@@
- if ($collection->isEmpty()) {
- throw new \Exception('Invalid query: Unknown Alias context');
- }
+ if ($collection->isEmpty()) {
+ throw new QueryException('Unknown alias context in query.');
+ }
@@
- if ($collection->isEmpty()) {
- throw new \Exception('Unknown Alias context');
- }
+ if ($collection->isEmpty()) {
+ throw new QueryException('Unknown alias context in query.');
+ }Also applies to: 6887-6889, 7135-7139
🤖 Prompt for AI Agents
In src/Database/Database.php around lines 6789-6792 (and likewise at 6887-6889
and 7135-7139), replace the thrown generic \Exception('Invalid query: Unknown
Alias context') with a QueryException so callers can catch DB-specific errors;
update the file to import or fully-qualify the QueryException class (e.g. add a
use statement or prefix with its namespace), and throw new QueryException with
the same message (and include any existing SQL/bindings/context parameters if
the QueryException constructor requires them).
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/Database/Database.php (4)
3401-3415: Strip auto-added system attributes on cache hit before returningCurrently only non-cached path removes $permissions; ensure consistency on cached reads as well.
Apply:
if ($cached) { $document = new Document($cached); @@ if ($collection->getId() !== self::METADATA) { if (!$validator->isValid([ ...$collection->getRead(), ...($documentSecurity ? $document->getRead() : []) ])) { return new Document(); } } + + if (isset($permissionsAdded) && $permissionsAdded) { + $document->removeAttribute('$permissions'); + } + if (isset($idAdded) && $idAdded) { + $document->removeAttribute('$id'); + } + if (isset($sequenceAdded) && $sequenceAdded) { + $document->removeAttribute('$sequence'); + } $this->trigger(self::EVENT_DOCUMENT_READ, $document); return $document; }
6295-6359: find(): auto-select required system attributes and clean them from outputs when not requestedGuarantees update/delete batch checks, relationship population, and consistent behavior.
Apply:
- $result = $this->processRelationshipQueries($relationships, $selects); + $result = $this->processRelationshipQueries($relationships, $selects); $selects = $result['queries']; $nestedSelections = $result['nestedSelections']; + + // Ensure required system attributes are projected + [$selects, $permissionsAdded] = Query::addSelect($selects, Query::select('$permissions', system: true)); + [$selects, $idAdded] = Query::addSelect($selects, Query::select('$id', system: true)); + [$selects, $sequenceAdded] = Query::addSelect($selects, Query::select('$sequence', system: true)); @@ $results = $this->adapter->find( @@ orderQueries: $orders ); @@ - $node = $this->casting($context, $node, $selects); - $node = $this->decode($context, $node, $selects); + $node = $this->casting($context, $node, $selects); + $node = $this->decode($context, $node, $selects); @@ if (!$node->isEmpty()) { $node->setAttribute('$collection', $collection->getId()); } + + // Strip auto-added attributes if caller didn't request them + if ($permissionsAdded) { + $node->removeAttribute('$permissions'); + } + if ($idAdded) { + $node->removeAttribute('$id'); + } + if ($sequenceAdded) { + $node->removeAttribute('$sequence'); + }
6678-6751: Fix undefined $selections in decodeOriginal() and honor $selectsCurrently uses an undefined variable, causing notices and incorrect filtering.
Apply:
- public function decodeOriginal(Document $collection, Document $document, array $selects = []): Document + public function decodeOriginal(Document $collection, Document $document, array $selects = []): Document { + // Normalize selects -> attribute names + $selections = array_map( + fn ($q) => $q instanceof Query ? $q->getAttribute() : (string)$q, + $selects + ); @@ - if ( - empty($selections) - || \in_array($key, $selections) - || \in_array('*', $selections) - ) { + if ( + empty($selections) + || \in_array($key, $selections, true) + || \in_array('*', $selections, true) + ) { $document->setAttribute($key, ($array) ? $value : $value[0]); }
1-7463: Remove all debug prints (var_dump/echo) from Database.php
- src/Database/Database.php:1523 →
var_dump($collection->getAttribute('attributes'));- src/Database/Database.php:3611–3613 →
var_dump($relationships);,var_dump($side);,var_dump($document);- src/Database/Database.php:6363–6367 →
echo PHP_EOL…;var_dump('populateDocumentRelationships');,var_dump($collection->getId());,var_dump($nestedSelections);- src/Database/Database.php:7176 →
var_dump($values);
♻️ Duplicate comments (5)
src/Database/Database.php (5)
1523-1523: Remove debug var_dump from deleteCollection()This will corrupt responses and leak internals.
Apply:
- var_dump($collection->getAttribute('attributes')); + // no-op
3611-3616: Remove stray debug output in relationship fetchLeaking internals and breaking responses.
Apply:
- var_dump($relationships); - var_dump($side); - var_dump($document); - /** - * How to force $document->getId() , not to be empty? - */ + // no-op
6363-6366: Remove stray echo/var_dump in find() loopPollutes output/logs.
Apply:
- echo PHP_EOL.PHP_EOL.PHP_EOL.PHP_EOL.PHP_EOL.PHP_EOL.PHP_EOL.PHP_EOL.PHP_EOL.PHP_EOL; - var_dump('populateDocumentRelationships'); - var_dump($collection->getId()); - var_dump($nestedSelections); + // no-op
6801-6805: Use QueryException instead of generic \Exception for unknown alias contextsAligns with DB error semantics and improves catchability.
Apply:
- if ($collection->isEmpty()) { - throw new \Exception('Invalid query: Unknown Alias context'); - } + if ($collection->isEmpty()) { + throw new QueryException('Unknown alias context in query.'); + }- if ($collection->isEmpty()) { - throw new \Exception('Invalid query: Unknown Alias context'); - } + if ($collection->isEmpty()) { + throw new QueryException('Unknown alias context in query.'); + }- if ($collection->isEmpty()) { - throw new \Exception('Unknown Alias context'); - } + if ($collection->isEmpty()) { + throw new QueryException('Unknown alias context in query.'); + }Also applies to: 6901-6902, 7150-7153
7174-7178: Remove stray var_dump in convertQuery()Leaks internals and breaks responses.
Apply:
- var_dump($values); foreach ($values as $valueIndex => $value) {
🧹 Nitpick comments (5)
src/Database/Database.php (5)
6321-6322: Avoid empty orderBy; use a deterministic unique orderEmpty attribute in orderAsc() is ambiguous. Prefer ordering by $id (or $sequence if that’s your canonical unique).
Apply (if acceptable):
- $orders[] = Query::orderAsc(); // In joins we should not add a default order, we should validate when using a cursor we should have a unique order + $orders[] = Query::orderAsc('$id'); // Ensures deterministic ordering for cursor paginationIf you rely on $sequence for uniqueness, use Query::orderAsc('$sequence') instead.
7223-7251: Cache key hashing for selects: good improvementHashing by serialize($selects) differentiates projections. Works as long as Query serialization is deterministic.
If you hit cache misses due to non-deterministic object serialization, consider hashing a normalized tuple like [$alias,$attribute,$as,$function,$system].
7292-7292: Remove unused local variable$count is computed but never used.
Apply:
- $count = \count($queries); + // no-op(or remove the line entirely)
6361-6379: Position of casting/decode is correct; minor after-cleanup suggestedAfter stripping auto-added system attributes (see earlier comment), the rest of the loop is fine.
6250-6360: Optional: remove duplicated grouping logicYou compute groupByType earlier, then re-derive filters/selects/limit/offset/orders again. Prefer one source to avoid drift.
Apply:
- $filters = Query::getFilterQueries($queries); - $selects = Query::getSelectQueries($queries); - $limit = Query::getLimitQuery($queries, 25); - $offset = Query::getOffsetQuery($queries, 0); - $orders = Query::getOrderQueries($queries); + // use values from $grouped calculated above
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Database/Database.php(53 hunks)tests/e2e/Adapter/Scopes/RelationshipTests.php(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/e2e/Adapter/Scopes/RelationshipTests.php
🧰 Additional context used
🧬 Code graph analysis (1)
src/Database/Database.php (6)
src/Database/Validator/Queries/V2.php (2)
V2(23-696)isValid(472-695)src/Database/Query.php (16)
getAttribute(224-227)Query(8-1426)getSelectQueries(958-963)addSelect(1392-1425)select(710-713)getJoinQueries(969-976)join(891-894)getCollection(266-269)getAlias(246-249)getFilterQueries(1087-1090)limit(743-746)getLimitQuery(998-1007)offset(754-757)getOffsetQuery(1029-1038)getOrderQueries(1044-1050)orderAsc(732-735)src/Database/Document.php (6)
getAttribute(224-231)getId(63-66)removeAttribute(287-293)Document(12-470)getCollection(85-88)find(304-322)src/Database/QueryContext.php (6)
QueryContext(8-89)add(60-68)getCollections(32-35)skipAuth(75-86)addSkipAuth(70-73)getCollectionByAlias(37-55)src/Database/Adapter.php (7)
getIdAttributeType(905-905)getMaxDateTime(912-915)getDocument(691-691)filter(1164-1173)find(805-817)getSupportForCasting(986-986)count(840-840)src/Database/Adapter/SQL.php (5)
getIdAttributeType(1731-1734)getDocument(353-419)find(2328-2530)getSupportForCasting(1434-1437)count(2542-2604)
🪛 PHPMD (2.15.0)
src/Database/Database.php
6678-6678: Avoid unused parameters such as '$selects'. (undefined)
(UnusedFormalParameter)
7292-7292: Avoid unused local variables such as '$count'. (undefined)
(UnusedLocalVariable)
🔇 Additional comments (6)
src/Database/Database.php (6)
32-32: LGTM: V2 validator alias importImporting Queries\V2 as DocumentsValidator is correct and aligns with the new context-aware validation.
3480-3483: Relationship population requires $id; avoid brittle throws by ensuring upstream selects include itGiven the upstream fix to auto-select $id, this guard should never trip in normal flows. Keeping it is fine, but if you see this tripping in tests, the issue is missing $id in the adapter projection.
Do you want me to add an assertion in tests to verify $id presence whenever nested relationship selections exist?
4625-4633: Batch update requires $permissions and $sequence — ensure find() always selects themThis throws if callers didn’t request these attributes. The find() fix below auto-adds them and removes from outputs when not explicitly requested.
6250-6272: Join context initialization looks goodAdding joined collections to QueryContext and auth flags is appropriate.
6484-6495: Count/Sum: context-aware conversion + validation looks correctUse of convertQueries with QueryContext and V2 validator is consistent.
Also applies to: 6539-6573
7174-7189: DateTime value normalization in convertQuery(): good, but avoid side-effectsOnce var_dump is removed (above), this block is solid.
| $selects = Query::getSelectQueries($queries); | ||
| [$selects, $permissionsAdded] = Query::addSelect($selects, Query::select('$permissions', system: true)); | ||
|
|
||
| //$selects = $this->validateSelections($collection, $selects); | ||
|
|
||
| $result = $this->processRelationshipQueries($relationships, $selects); | ||
| $selects = $result['queries']; | ||
| $nestedSelections = $result['nestedSelections']; | ||
|
|
||
| $validator = new Authorization(self::PERMISSION_READ); | ||
| $documentSecurity = $collection->getAttribute('documentSecurity', false); | ||
|
|
||
| [$collectionKey, $documentKey, $hashKey] = $this->getCacheKeys( | ||
| $collection->getId(), | ||
| $id, | ||
| $selections | ||
| $selects | ||
| ); |
There was a problem hiding this comment.
Always include system selects needed for relationships, caching, and batch ops
Populate + caching paths require $id/$sequence/$permissions even if callers didn’t select them. Add them here and track what was auto-added.
Apply:
$selects = Query::getSelectQueries($queries);
- [$selects, $permissionsAdded] = Query::addSelect($selects, Query::select('$permissions', system: true));
+ [$selects, $permissionsAdded] = Query::addSelect($selects, Query::select('$permissions', system: true));
+ [$selects, $idAdded] = Query::addSelect($selects, Query::select('$id', system: true));
+ [$selects, $sequenceAdded] = Query::addSelect($selects, Query::select('$sequence', system: true));📝 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.
| $selects = Query::getSelectQueries($queries); | |
| [$selects, $permissionsAdded] = Query::addSelect($selects, Query::select('$permissions', system: true)); | |
| //$selects = $this->validateSelections($collection, $selects); | |
| $result = $this->processRelationshipQueries($relationships, $selects); | |
| $selects = $result['queries']; | |
| $nestedSelections = $result['nestedSelections']; | |
| $validator = new Authorization(self::PERMISSION_READ); | |
| $documentSecurity = $collection->getAttribute('documentSecurity', false); | |
| [$collectionKey, $documentKey, $hashKey] = $this->getCacheKeys( | |
| $collection->getId(), | |
| $id, | |
| $selections | |
| $selects | |
| ); | |
| $selects = Query::getSelectQueries($queries); | |
| [$selects, $permissionsAdded] = Query::addSelect($selects, Query::select('$permissions', system: true)); | |
| [$selects, $idAdded] = Query::addSelect($selects, Query::select('$id', system: true)); | |
| [$selects, $sequenceAdded] = Query::addSelect($selects, Query::select('$sequence', system: true)); | |
| //$selects = $this->validateSelections($collection, $selects); | |
| $result = $this->processRelationshipQueries($relationships, $selects); | |
| $selects = $result['queries']; | |
| $nestedSelections = $result['nestedSelections']; | |
| $validator = new Authorization(self::PERMISSION_READ); | |
| $documentSecurity = $collection->getAttribute('documentSecurity', false); | |
| [$collectionKey, $documentKey, $hashKey] = $this->getCacheKeys( | |
| $collection->getId(), | |
| $id, | |
| $selects | |
| ); |
🤖 Prompt for AI Agents
In src/Database/Database.php around lines 3375-3391, ensure the code always
injects system selects for $id, $sequence and $permissions (system: true) into
$selects even when callers didn't request them, and record which selects were
auto-added; call Query::addSelect for each required system field, merge the
returned selects and a flag/list of auto-added fields into
$selects/$permissionsAdded before passing $selects into
processRelationshipQueries and getCacheKeys, so caching, relationship processing
and batch/populate flows have $id/$sequence/$permissions available and you can
later strip or account for auto-added fields when forming the final response.
src/Database/Database.php
Outdated
| if($permissionsAdded){ | ||
| $document->removeAttribute('$permissions'); | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Also remove auto-added $id and $sequence on non-cached path
Mirror the cache-hit behavior.
Apply:
- if($permissionsAdded){
- $document->removeAttribute('$permissions');
- }
+ if ($permissionsAdded) {
+ $document->removeAttribute('$permissions');
+ }
+ if (isset($idAdded) && $idAdded) {
+ $document->removeAttribute('$id');
+ }
+ if (isset($sequenceAdded) && $sequenceAdded) {
+ $document->removeAttribute('$sequence');
+ }📝 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.
| if($permissionsAdded){ | |
| $document->removeAttribute('$permissions'); | |
| } | |
| if ($permissionsAdded) { | |
| $document->removeAttribute('$permissions'); | |
| } | |
| if (isset($idAdded) && $idAdded) { | |
| $document->removeAttribute('$id'); | |
| } | |
| if (isset($sequenceAdded) && $sequenceAdded) { | |
| $document->removeAttribute('$sequence'); | |
| } |
🤖 Prompt for AI Agents
In src/Database/Database.php around lines 3462-3465, the non-cached response
path currently only removes the auto-added $permissions attribute; update it to
mirror the cache-hit behavior by also removing the auto-added $id and $sequence
attributes when permissionsAdded is true (or the same condition used for
$permissions removal). Ensure you call removeAttribute for '$id' and '$sequence'
in the same block so the non-cached output matches the cached output.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/e2e/Adapter/Scopes/DocumentTests.php (1)
5145-5146: Replace array-basedQuery::selectcalls with string arguments
AllQuery::select([...])usages must be updated to pass a string (or separate calls) per the new signature. Update the following:
- tests/e2e/Adapter/Scopes/DocumentTests.php:5145
- tests/e2e/Adapter/Scopes/SpatialTests.php:1521, 1526, 1531
src/Database/Database.php (1)
6665-6735: Fix undefined $selections in decodeOriginal() and honor $selects.Prevents notices and ensures proper filtering.
- public function decodeOriginal(Document $collection, Document $document, array $selects = []): Document + public function decodeOriginal(Document $collection, Document $document, array $selects = []): Document { + // Normalize selects to attribute names (supports Query instances or raw strings) + $selections = array_map( + fn ($q) => $q instanceof Query ? $q->getAttribute() : (string)$q, + $selects + ); @@ - if ( - empty($selections) - || \in_array($key, $selections) - || \in_array('*', $selections) - ) { + if ( + empty($selections) + || \in_array($key, $selections, true) + || \in_array('*', $selections, true) + ) { $document->setAttribute($key, ($array) ? $value : $value[0]); }
♻️ Duplicate comments (4)
tests/e2e/Adapter/Scopes/RelationshipTests.php (2)
322-326: Reorder selects: placing “*” first swallows later specific selects (duplicate).Known issue: Query::addSelect blocks merging when a prior select is “”, so subsequent “votes.”/“votes.animals.*” may be ignored. Put most-specific first to make tests robust.
Apply:
@@ - Query::select('*'), - Query::select('votes.*'), + Query::select('votes.*'), + Query::select('*'), @@ - Query::select('*'), - Query::select('votes.*'), - Query::select('votes.animals.*'), + Query::select('votes.animals.*'), + Query::select('votes.*'), + Query::select('*'),Also applies to: 334-338
1466-1470: Refactor addSelect merge logic to allow specific selects after “*”
In src/Database/Query.php ::addSelect (around line 1397), change:- if ($q->getAttribute() === '*'){ - $merge = false; - } + if ($q->getAttribute() === '*' && $query->getAttribute() === '*'){ + $merge = false; + }This blocks only duplicate “*” entries while permitting child-specific selects regardless of order, so tests no longer need to swap their Query::select calls.
src/Database/Database.php (2)
6789-6792: Use QueryException instead of generic \Exception for unknown alias.Keeps error semantics consistent and improves catchability.
- if ($collection->isEmpty()) { - throw new \Exception('Invalid query: Unknown Alias context'); - } + if ($collection->isEmpty()) { + throw new QueryException('Unknown alias context in query.'); + }- if ($collection->isEmpty()) { - throw new \Exception('Invalid query: Unknown Alias context'); - } + if ($collection->isEmpty()) { + throw new QueryException('Unknown alias context in query.'); + }- if ($collection->isEmpty()) { - throw new \Exception('Unknown Alias context'); - } + if ($collection->isEmpty()) { + throw new QueryException('Unknown alias context in query.'); + }Also applies to: 6887-6890, 7135-7139
7162-7164: Remove stray var_dump() in convertQuery().Leaks data and corrupts responses.
- $values = $query->getValues(); - var_dump($values); + $values = $query->getValues();
🧹 Nitpick comments (4)
tests/e2e/Adapter/Scopes/Relationships/ManyToManyTests.php (1)
111-113: Prefer using the existing $database instance for consistencyAvoid mixing static::getDatabase() and the local $database within the same test.
- $documents = static::getDatabase()->find('playlist', [ + $documents = $database->find('playlist', [ Query::select('name'), Query::limit(1) ]);tests/e2e/Adapter/Scopes/DocumentTests.php (1)
3532-3536: Use the local $database for consistencyMinor style nit to avoid mixing static::getDatabase() amid an existing $database.
- $documents = static::getDatabase()->find('movies', [ + $documents = $database->find('movies', [ Query::select('name'), Query::select('year'), Query::select('$createdAt') ]);src/Database/Database.php (2)
6291-6305: Remove duplicate grouping code after convertQueries().You compute groupByType and then re-derive filters/limits below; keep one path to avoid drift.
- $grouped = Query::groupByType($queries); - $filters = $grouped['filters']; - $selects = $grouped['selections']; - $limit = $grouped['limit']; - $offset = $grouped['offset']; - $orderAttributes = $grouped['orderAttributes']; - $orderTypes = $grouped['orderTypes']; - $cursor = $grouped['cursor']; - $cursorDirection = $grouped['cursorDirection'] ?? Database::CURSOR_AFTER; - - $filters = Query::getFilterQueries($queries); - $selects = Query::getSelectQueries($queries); - $limit = Query::getLimitQuery($queries, 25); - $offset = Query::getOffsetQuery($queries, 0); - $orders = Query::getOrderQueries($queries); + $grouped = Query::groupByType($queries); + $filters = $grouped['filters']; + $selects = $grouped['selections']; + $limit = $grouped['limit'] ?? 25; + $offset = $grouped['offset'] ?? 0; + $orders = $grouped['orders'] ?? Query::getOrderQueries($queries); + $cursor = $grouped['cursor']; + $cursorDirection = $grouped['cursorDirection'] ?? Database::CURSOR_AFTER;
7341-7344: Drop unused $idAdded in processRelationshipQueries().Cleans up static analysis warning without behavior change.
- if (\count($queries) > 0) { - [$queries, $idAdded] = Query::addSelect($queries, Query::select('$id', system: true)); - } + if (\count($queries) > 0) { + [$queries] = Query::addSelect($queries, Query::select('$id', system: true)); + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Database/Database.php(51 hunks)tests/e2e/Adapter/Base.php(2 hunks)tests/e2e/Adapter/Scopes/DocumentTests.php(16 hunks)tests/e2e/Adapter/Scopes/RelationshipTests.php(22 hunks)tests/e2e/Adapter/Scopes/Relationships/ManyToManyTests.php(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/e2e/Adapter/Base.php
🧰 Additional context used
🧬 Code graph analysis (4)
src/Database/Database.php (4)
src/Database/Validator/Queries/V2.php (2)
V2(23-696)isEmpty(155-166)src/Database/QueryContext.php (6)
QueryContext(8-89)add(60-68)getCollections(32-35)skipAuth(75-86)addSkipAuth(70-73)getCollectionByAlias(37-55)src/Database/Adapter.php (7)
getIdAttributeType(905-905)getDocument(691-691)filter(1164-1173)find(805-817)sum(829-829)getSupportForCasting(986-986)count(840-840)src/Database/Query.php (16)
Query(8-1426)getSelectQueries(958-963)addSelect(1392-1425)select(710-713)getAttribute(224-227)join(891-894)getCollection(266-269)getAlias(246-249)getFilterQueries(1087-1090)limit(743-746)getLimitQuery(998-1007)offset(754-757)getOrderQueries(1044-1050)getAs(261-264)getMethod(216-219)getValue(241-244)
tests/e2e/Adapter/Scopes/DocumentTests.php (3)
src/Database/Database.php (3)
Database(35-7448)getDocument(3334-3467)decode(6749-6832)src/Database/QueryContext.php (2)
QueryContext(8-89)add(60-68)src/Database/Query.php (2)
Query(8-1426)select(710-713)
tests/e2e/Adapter/Scopes/RelationshipTests.php (3)
src/Database/Query.php (2)
Query(8-1426)select(710-713)src/Database/Database.php (1)
getDocument(3334-3467)src/Database/Adapter/SQL.php (1)
getDocument(353-419)
tests/e2e/Adapter/Scopes/Relationships/ManyToManyTests.php (3)
src/Database/Database.php (1)
getDatabase(687-690)tests/e2e/Adapter/Base.php (1)
getDatabase(38-38)src/Database/Query.php (3)
Query(8-1426)select(710-713)getAttribute(224-227)
🪛 PHPMD (2.15.0)
src/Database/Database.php
6665-6665: Avoid unused parameters such as '$selects'. (undefined)
(UnusedFormalParameter)
7341-7341: Avoid unused local variables such as '$idAdded'. (undefined)
(UnusedLocalVariable)
🔇 Additional comments (30)
tests/e2e/Adapter/Scopes/Relationships/ManyToManyTests.php (6)
142-144: Per-field select refactor: OKUsing multiple Query::select(...) entries aligns with the new API.
154-156: Per-field select refactor: OKConsistent with new selection semantics.
529-531: Per-field select refactor: OKMatches new API for nested selections.
541-543: Per-field select refactor: OK
1588-1590: Per-field select refactor: OK
1693-1696: Per-field select refactor: OKtests/e2e/Adapter/Scopes/DocumentTests.php (21)
21-21: Import QueryContext: OKRequired for the new decode signature.
1308-1310: Per-field select refactor: OK
1323-1326: Updated internal-attribute expectations: OKAsserting absence of $sequence/$createdAt/$updatedAt/$permissions matches new exposure rules.
1329-1332: Explicit $id selection: OK
1335-1339: Internal attributes gating: OK
3453-3456: Per-field select in find: OK
3465-3470: Internal attributes: assertions match new behavior
3473-3476: Selecting $id explicitly: OK
3485-3490: Assertions for internal fields: OK
3493-3496: Selecting $sequence explicitly: OK
3513-3516: Selecting $collection explicitly: OK
3545-3550: Assertions with $createdAt: OK
3553-3556: Selecting $updatedAt explicitly: OK
3565-3570: Assertions with $updatedAt: OK
3572-3576: Selecting $permissions explicitly: OK
3585-3590: Assertions with $permissions: OK
2929-2929: Per-field select inside nested OR query: OK
3940-3944: Switch to context-aware decode: OKConstructing QueryContext and calling Database::decode($context, ...) matches the new API.
4678-4685: Short-selects in bulk delete: OKSelecting internals for callbacks is aligned with the new projection model.
4887-4893: Short-selects with callback error path: OK
4910-4917: Short-selects + pagination in delete: OKsrc/Database/Database.php (3)
6308-6315: Verify default order invocation.Query::orderAsc() without an attribute may be invalid or non-unique for cursor use. If unsupported, default to '$id'.
- $orders[] = Query::orderAsc(); // In joins we should not add a default order, we should validate when using a cursor we should have a unique order + $orders[] = Query::orderAsc('$id'); // ensure deterministic orderingWould you like me to adjust based on the actual Query API?
3478-3480: Good guard: require $id when populating relationships.Prevents ambiguous lookups and unexpected cascades.
7212-7239: Cache hash improvement LGTM.Switching to md5(serialize($selects)) correctly disambiguates select variants.
| $selects = Query::getSelectQueries($queries); | ||
|
|
||
| //$selects = $this->validateSelections($collection, $selects); | ||
|
|
||
| [$selects, $nestedSelections] = $this->processRelationshipQueries($relationships, $selects); | ||
|
|
||
| [$selects, $permissionsAdded] = Query::addSelect($selects, Query::select('$permissions', system: true)); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Always include and later strip system selects ($permissions, $id, $sequence) in getDocument().
Needed for auth checks, relationship population, and cache hashing; avoid leaking them to callers when auto-added.
- [$selects, $permissionsAdded] = Query::addSelect($selects, Query::select('$permissions', system: true));
+ [$selects, $permissionsAdded] = Query::addSelect($selects, Query::select('$permissions', system: true));
+ [$selects, $idAdded] = Query::addSelect($selects, Query::select('$id', system: true));
+ [$selects, $sequenceAdded] = Query::addSelect($selects, Query::select('$sequence', system: true)); if ($cached) {
$document = new Document($cached);
@@
- $this->trigger(self::EVENT_DOCUMENT_READ, $document);
- return $document;
+ // Strip auto-added system attributes before returning
+ if (!empty($permissionsAdded)) { $document->removeAttribute('$permissions'); }
+ if (!empty($idAdded)) { $document->removeAttribute('$id'); }
+ if (!empty($sequenceAdded)) { $document->removeAttribute('$sequence'); }
+
+ $this->trigger(self::EVENT_DOCUMENT_READ, $document);
+ return $document;
}- if($permissionsAdded){ // Or remove all queries added by system
- $document->removeAttribute('$permissions');
- }
+ // Remove auto-added system attributes on non-cached path
+ if (!empty($permissionsAdded)) { $document->removeAttribute('$permissions'); }
+ if (!empty($idAdded)) { $document->removeAttribute('$id'); }
+ if (!empty($sequenceAdded)) { $document->removeAttribute('$sequence'); }Also applies to: 3398-3413, 3459-3463
| $context = new QueryContext(); | ||
| $context->add($collection); | ||
|
|
||
| $joins = Query::getJoinQueries($queries); | ||
|
|
||
| foreach ($joins as $join) { | ||
| $context->add( | ||
| $this->silent(fn () => $this->getCollection($join->getCollection())), | ||
| $join->getAlias() | ||
| ); | ||
| } | ||
|
|
||
| $authorization = new Authorization($forPermission); | ||
|
|
||
| foreach ($context->getCollections() as $_collection) { | ||
| $documentSecurity = $_collection->getAttribute('documentSecurity', false); | ||
| $skipAuth = $authorization->isValid($_collection->getPermissionsByType($forPermission)); | ||
|
|
||
| if (!$skipAuth && !$documentSecurity && $_collection->getId() !== self::METADATA) { | ||
| throw new AuthorizationException($authorization->getDescription()); | ||
| } | ||
|
|
||
| $context->addSkipAuth($this->adapter->filter($_collection->getId()), $forPermission, $skipAuth); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure update/delete batch flows receive required system fields via find().
updateDocuments/deleteDocuments require $permissions and $sequence; inject them when forPermission != read. Avoid exposing them to read callers.
- $queries = self::convertQueries($context, $queries);
+ $queries = self::convertQueries($context, $queries);
@@
- $filters = Query::getFilterQueries($queries);
- $selects = Query::getSelectQueries($queries);
+ $filters = Query::getFilterQueries($queries);
+ $selects = Query::getSelectQueries($queries);
+ // Auto-include system selects for non-read operations
+ $permissionsAdded = $idAdded = $sequenceAdded = false;
+ if ($forPermission !== Database::PERMISSION_READ) {
+ [$selects, $permissionsAdded] = Query::addSelect($selects, Query::select('$permissions', system: true));
+ [$selects, $idAdded] = Query::addSelect($selects, Query::select('$id', system: true));
+ [$selects, $sequenceAdded] = Query::addSelect($selects, Query::select('$sequence', system: true));
+ }
@@
- $node = $this->casting($context, $node, $selects);
- $node = $this->decode($context, $node, $selects);
+ $node = $this->casting($context, $node, $selects);
+ $node = $this->decode($context, $node, $selects);
+ // Hide auto-added attributes on read responses
+ if ($forPermission === Database::PERMISSION_READ) {
+ if (!empty($permissionsAdded)) { $node->removeAttribute('$permissions'); }
+ if (!empty($idAdded)) { $node->removeAttribute('$id'); }
+ if (!empty($sequenceAdded)) { $node->removeAttribute('$sequence'); }
+ }Also applies to: 6336-6350, 6357-6359
| throw new Exception('Playlist not found'); | ||
| } | ||
|
|
||
| var_dump($playlist); |
There was a problem hiding this comment.
Remove stray debug output
var_dump in tests is noisy and may break CI output.
-var_dump($playlist);📝 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.
| var_dump($playlist); |
🤖 Prompt for AI Agents
In tests/e2e/Adapter/Scopes/Relationships/ManyToManyTests.php around line 149,
there is a stray debugging statement var_dump($playlist) that produces noisy
output and can break CI; remove that var_dump call and, if needed, replace it
with an appropriate assertion or logging via the test framework (e.g.,
assertNotNull/assertEquals) so the test remains silent on success and only
reports failures.
| $make = $database->getDocument('make', 'ford', [ | ||
| Query::select('models.*'), | ||
| ]); | ||
| var_dump($make); | ||
| if ($make->isEmpty()) { | ||
| throw new Exception('Make not found'); |
There was a problem hiding this comment.
Remove debug output from tests (and adapter).
var_dump in tests will pollute PHPUnit output; drop it. Also remove var_dump($sql) from SQL adapter.
@@
- $make = $database->getDocument('make', 'ford', [
- Query::select('models.*'),
- ]);
-var_dump($make);
+ $make = $database->getDocument('make', 'ford', [
+ Query::select('models.*'),
+ ]);And in src/Database/Adapter/SQL.php:
@@
- var_dump($sql);
+ // debug print removed📝 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.
| $make = $database->getDocument('make', 'ford', [ | |
| Query::select('models.*'), | |
| ]); | |
| var_dump($make); | |
| if ($make->isEmpty()) { | |
| throw new Exception('Make not found'); | |
| // File: tests/e2e/Adapter/Scopes/RelationshipTests.php | |
| $make = $database->getDocument('make', 'ford', [ | |
| Query::select('models.*'), | |
| ]); | |
| if ($make->isEmpty()) { | |
| throw new Exception('Make not found'); |
| $make = $database->getDocument('make', 'ford', [ | |
| Query::select('models.*'), | |
| ]); | |
| var_dump($make); | |
| if ($make->isEmpty()) { | |
| throw new Exception('Make not found'); | |
| // File: src/Database/Adapter/SQL.php | |
| // debug print removed |
🤖 Prompt for AI Agents
In tests/e2e/Adapter/Scopes/RelationshipTests.php around lines 1508–1513 and in
src/Database/Adapter/SQL.php (search for var_dump($sql)), there are stray
var_dump debug calls; remove the var_dump($make) in the test and any
var_dump($sql) in the SQL adapter so PHPUnit output is not polluted, replace
with proper logging if needed, and run the test suite to ensure no leftover
debug statements remain.
There was a problem hiding this comment.
Actionable comments posted: 3
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)
6666-6736: Fix undefined$selectionsand honor$selectsin decodeOriginal().
$selectionsis undefined;$selectsis ignored. Causes notices and wrong filtering.Apply:
- public function decodeOriginal(Document $collection, Document $document, array $selects = []): Document + public function decodeOriginal(Document $collection, Document $document, array $selects = []): Document { + // Normalize selects -> attribute names + $selections = array_map( + fn ($q) => $q instanceof Query ? $q->getAttribute() : (string)$q, + $selects + ); @@ - if ( - empty($selections) - || \in_array($key, $selections) - || \in_array('*', $selections) - ) { + if ( + empty($selections) + || \in_array($key, $selections, true) + || \in_array('*', $selections, true) + ) { $document->setAttribute($key, ($array) ? $value : $value[0]); }
♻️ Duplicate comments (3)
src/Database/Database.php (3)
7136-7176: Remove stray var_dump and throw QueryException in convertQuery().
var_dump($values)leaks data and breaks responses; generic Exception harms error typing.- $collection = clone $context->getCollectionByAlias($query->getAlias()); + $collection = clone $context->getCollectionByAlias($query->getAlias()); if ($collection->isEmpty()) { - throw new \Exception('Unknown Alias context'); + throw new QueryException('Unknown alias context in query.'); } @@ if ($attribute->getAttribute('type') == Database::VAR_DATETIME) { $values = $query->getValues(); - var_dump($values); foreach ($values as $valueIndex => $value) { try { $values[$valueIndex] = DateTime::setTimezone($value);
6789-6793: Throw QueryException instead of generic Exception (decode).
Aligns with DB/query error semantics and improves catchability.- if ($collection->isEmpty()) { - throw new \Exception('Invalid query: Unknown Alias context'); - } + if ($collection->isEmpty()) { + throw new QueryException('Unknown alias context in query.'); + }
6887-6890: Throw QueryException instead of generic Exception (casting).
Same issue as decode().- if ($collection->isEmpty()) { - throw new \Exception('Invalid query: Unknown Alias context'); - } + if ($collection->isEmpty()) { + throw new QueryException('Unknown alias context in query.'); + }
🧹 Nitpick comments (6)
tests/e2e/Adapter/Base.php (1)
11-11: Unused import (JoinsTests) if the trait remains disabledIf you don’t enable the trait, drop this import to keep the file clean.
-use Tests\E2E\Adapter\Scopes\JoinsTests; +// use Tests\E2E\Adapter\Scopes\JoinsTests;(Prefer enabling the trait per the previous comment.)
src/Database/Database.php (5)
6291-6306: Avoid recomputing grouped pieces; use one source of truth.You compute via groupByType, then recompute filters/selects/limit/offset/orders again. Simplify to use the grouped result only.
- $grouped = Query::groupByType($queries); - $filters = $grouped['filters']; - $selects = $grouped['selections']; - $limit = $grouped['limit']; - $offset = $grouped['offset']; - $orderAttributes = $grouped['orderAttributes']; - $orderTypes = $grouped['orderTypes']; - $cursor = $grouped['cursor']; - $cursorDirection = $grouped['cursorDirection'] ?? Database::CURSOR_AFTER; - - $filters = Query::getFilterQueries($queries); - $selects = Query::getSelectQueries($queries); - $limit = Query::getLimitQuery($queries, 25); - $offset = Query::getOffsetQuery($queries, 0); - $orders = Query::getOrderQueries($queries); + $grouped = Query::groupByType($queries); + $filters = $grouped['filters']; + $selects = $grouped['selections']; + $limit = $grouped['limit'] ?? 25; + $offset = $grouped['offset'] ?? 0; + $orders = $grouped['orders'] ?? Query::getOrderQueries($queries); + $cursor = $grouped['cursor']; + $cursorDirection = $grouped['cursorDirection'] ?? Database::CURSOR_AFTER;
7339-7346: Silence unused$idAddedfrom addSelect().PHPMD warning (unused local).
- if (\count($queries) > 0) { - [$queries, $idAdded] = Query::addSelect($queries, Query::select('$id', system: true)); - } + if (\count($queries) > 0) { + [$queries, $idAdded] = Query::addSelect($queries, Query::select('$id', system: true)); + unset($idAdded); + }
6330-6331: Nit: error text.
“Cursor document must be from the same collection.” (lowercase “collection”).- throw new DatabaseException("cursor Document must be from the same Collection."); + throw new DatabaseException('Cursor document must be from the same collection.');
7030-7079: Dead code: validateSelections() isn’t used.Either remove or mark @deprecated to avoid drift.
6309-6316: Specify a unique attribute in default orderAsc call
Query::orderAsc() defaults to an empty attribute (''), which can produce an invalid sort order for cursor queries. Pass'$id'to guarantee a unique sort key.- if ($uniqueOrderBy === false) { - $orders[] = Query::orderAsc(); // In joins we should not add a default order, we should validate when using a cursor we should have a unique order - } + if ($uniqueOrderBy === false) { + $orders[] = Query::orderAsc('$id'); + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Database/Database.php(51 hunks)tests/e2e/Adapter/Base.php(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/Database/Database.php (6)
src/Database/Validator/Queries/V2.php (3)
V2(23-696)isEmpty(155-166)isValid(472-695)src/Database/QueryContext.php (6)
QueryContext(8-89)add(60-68)getCollections(32-35)skipAuth(75-86)addSkipAuth(70-73)getCollectionByAlias(37-55)src/Database/Adapter/SQL.php (6)
getIdAttributeType(1731-1734)getDocument(353-419)find(2328-2530)sum(2617-2680)getSupportForCasting(1434-1437)count(2542-2604)src/Database/Adapter.php (9)
getIdAttributeType(905-905)getMinDateTime(898-898)getMaxDateTime(912-915)getDocument(691-691)filter(1164-1173)find(805-817)sum(829-829)getSupportForCasting(986-986)count(840-840)src/Database/Query.php (17)
Query(8-1426)getSelectQueries(958-963)addSelect(1392-1425)select(710-713)getAttribute(224-227)getJoinQueries(969-976)join(891-894)getCollection(266-269)getFilterQueries(1087-1090)limit(743-746)getLimitQuery(998-1007)offset(754-757)getOffsetQuery(1029-1038)getOrderQueries(1044-1050)orderAsc(732-735)isNested(1194-1201)isJoin(1206-1215)src/Database/Document.php (8)
getAttribute(224-231)getId(63-66)isEmpty(396-399)removeAttribute(287-293)Document(12-470)getCollection(85-88)find(304-322)getArrayCopy(423-458)
🪛 PHPMD (2.15.0)
src/Database/Database.php
6666-6666: Avoid unused parameters such as '$selects'. (undefined)
(UnusedFormalParameter)
7342-7342: Avoid unused local variables such as '$idAdded'. (undefined)
(UnusedLocalVariable)
🔇 Additional comments (2)
src/Database/Database.php (2)
3375-3383: Always include and later strip system selects ($permissions, $id, $sequence) in getDocument().Needed for auth checks, relationship population, and cache hashing; avoid leaking them when auto-added.
[ suggest_essential_refactor ]$selects = Query::getSelectQueries($queries); @@ - [$selects, $nestedSelections] = $this->processRelationshipQueries($relationships, $selects); - [$selects, $permissionsAdded] = Query::addSelect($selects, Query::select('$permissions', system: true)); + [$selects, $nestedSelections] = $this->processRelationshipQueries($relationships, $selects); + [$selects, $permissionsAdded] = Query::addSelect($selects, Query::select('$permissions', system: true)); + [$selects, $idAdded] = Query::addSelect($selects, Query::select('$id', system: true)); + [$selects, $sequenceAdded] = Query::addSelect($selects, Query::select('$sequence', system: true)); @@ if ($cached) { $document = new Document($cached); @@ - $this->trigger(self::EVENT_DOCUMENT_READ, $document); - return $document; + // Strip auto-added system attributes before returning + if (!empty($permissionsAdded)) { $document->removeAttribute('$permissions'); } + if (!empty($idAdded)) { $document->removeAttribute('$id'); } + if (!empty($sequenceAdded)) { $document->removeAttribute('$sequence'); } + $this->trigger(self::EVENT_DOCUMENT_READ, $document); + return $document; } @@ - if($permissionsAdded){ // Or remove all queries added by system - $document->removeAttribute('$permissions'); - } + // Remove auto-added system attributes on non-cached path + if (!empty($permissionsAdded)) { $document->removeAttribute('$permissions'); } + if (!empty($idAdded)) { $document->removeAttribute('$id'); } + if (!empty($sequenceAdded)) { $document->removeAttribute('$sequence'); }Also applies to: 3400-3414, 3461-3463
3830-3834: LGTM: context-aware casting/decode after mutations.Good consistency using QueryContext in create/createDocuments/upsert paths.
Also applies to: 3935-3937, 5330-5331
| //$selects = $this->validateSelections($collection, $selects); | ||
|
|
||
| $selections = $this->validateSelections($collection, $selects); | ||
| $nestedSelections = $this->processRelationshipQueries($relationships, $queries); | ||
| [$selects, $nestedSelections] = $this->processRelationshipQueries($relationships, $selects); | ||
|
|
||
| $getResults = fn () => $this->adapter->find( | ||
| $collection, | ||
| $results = $this->adapter->find( | ||
| $context, | ||
| $queries, | ||
| $limit ?? 25, | ||
| $offset ?? 0, | ||
| $orderAttributes, | ||
| $orderTypes, | ||
| $cursor, | ||
| $cursorDirection, | ||
| $forPermission | ||
| $forPermission, | ||
| selects: $selects, | ||
| filters: $filters, | ||
| joins: $joins, | ||
| orderQueries: $orders | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inject required system selects for non-read flows (find → update/delete batches).
updateDocuments/deleteDocuments require $permissions and $sequence but find() doesn’t auto-select them, causing immediate exceptions. Add them when $forPermission !== read.
- [$selects, $nestedSelections] = $this->processRelationshipQueries($relationships, $selects);
+ [$selects, $nestedSelections] = $this->processRelationshipQueries($relationships, $selects);
+ // Ensure batch ops receive required system fields
+ if ($forPermission !== Database::PERMISSION_READ) {
+ [$selects, $pAdded] = Query::addSelect($selects, Query::select('$permissions', system: true)); unset($pAdded);
+ [$selects, $iAdded] = Query::addSelect($selects, Query::select('$id', system: true)); unset($iAdded);
+ [$selects, $sAdded] = Query::addSelect($selects, Query::select('$sequence', system: true)); unset($sAdded);
+ }This change satisfies the required-attributes checks at:
- updateDocuments: Lines 4622-4626,
- deleteDocuments: Lines 6101-6106.
Also applies to: 4619-4626, 6101-6106
🤖 Prompt for AI Agents
In src/Database/Database.php around lines 6335 to 6351, find() is called with
$selects produced by processRelationshipQueries but does not include required
system fields used by non-read flows; when $forPermission !== 'read' inject the
system selects (at minimum 'permissions' and 'sequence') into the $selects array
before calling adapter->find so updateDocuments/deleteDocuments will have those
attributes available; ensure you preserve any existing selects and
nestedSelections and only add these system selects when not already present.
| var_dump($values); | ||
| foreach ($values as $valueIndex => $value) { |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Scan for remaining debug output in repo.
To ensure no stdout leaks remain.
🏁 Script executed:
#!/bin/bash
rg -nP "var_dump\s*\(|print_r\s*\(|echo\s+[^\$]" -g '!**/vendor/**' -C2Length of output: 148
🏁 Script executed:
#!/bin/bash
# Search for any remaining debug output in all PHP files (excluding vendor)
rg -nP "var_dump\s*\(|print_r\s*\(|echo\s+[^\$]" -C2 --type phpLength of output: 5804
Remove all debug output statements
Debug calls (var_dump, print_r, raw echo) remain across source and test files (e.g., src/Database/Database.php:7164, Validator/Queries/V2.php, Adapter/SQL.php, bin scripts, tests). Remove them to prevent unintended stdout leaks.
🤖 Prompt for AI Agents
In src/Database/Database.php around lines 7164-7165, remove the debug output
call var_dump($values) (and any adjacent raw echo/print_r) so no debugging data
is written to stdout; if structured debug is required replace it with a proper
logger call (e.g., $this->logger->debug(...)) or remove entirely, and then grep
the repository for var_dump, print_r, echo usages in source and test files and
remove/convert them to proper logging where appropriate to prevent unintended
stdout leaks.
| // use JoinsTests; | ||
| // use CollectionTests; | ||
| // use DocumentTests; | ||
| // use AttributeTests; | ||
| // use IndexTests; | ||
| // use PermissionTests; | ||
| // use RelationshipTests; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Join e2e tests are not running — trait usage is commented out
JoinsTests is imported but disabled here, so the new join suite won’t execute in CI. Enable it at least, even if other suites remain temporarily disabled.
Apply:
-// use JoinsTests;
+ use JoinsTests;Optional: add a TODO above the remaining commented traits with an issue link and expected re-enable date to avoid silently losing coverage.
To verify current state and test discovery:
🏁 Script executed:
#!/bin/bash
# Show commented trait usage in Base
rg -n '^\s*//\s*use\s+.*Tests;' tests/e2e/Adapter/Base.php -n -C0
# Ensure the JoinsTests trait exists and has tests
rg -n 'trait\s+JoinsTests' tests/e2e/Adapter/Scopes/JoinsTests.php
rg -nP 'function\s+test\w+\s*\(' tests/e2e/Adapter/Scopes/JoinsTests.phpLength of output: 258
Enable Joins e2e tests
- Uncomment
use JoinsTests;in tests/e2e/Adapter/Base.php so the join suite runs in CI. - Optional: add a TODO above the other commented traits with an issue link and target re-enable date to avoid losing coverage.
-// use JoinsTests;
+ use JoinsTests;📝 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.
| // use JoinsTests; | |
| // use CollectionTests; | |
| // use DocumentTests; | |
| // use AttributeTests; | |
| // use IndexTests; | |
| // use PermissionTests; | |
| // use RelationshipTests; | |
| use JoinsTests; | |
| // use CollectionTests; | |
| // use DocumentTests; | |
| // use AttributeTests; | |
| // use IndexTests; | |
| // use PermissionTests; | |
| // use RelationshipTests; |
🤖 Prompt for AI Agents
In tests/e2e/Adapter/Base.php around lines 22 to 28, the Joins e2e tests are
disabled; uncomment the line "use JoinsTests;" so the join test suite runs in
CI, and (optionally) add a single-line TODO comment above the remaining
commented trait uses with a link to the tracking issue and a target re-enable
date to preserve coverage context.
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)
tests/e2e/Adapter/Scopes/SpatialTests.php (1)
231-232: Re-enable cleanup to avoid leaking collections between tests.Commented deletion can cause “collection already exists” on subsequent runs and cross-test flakiness.
- // $database->deleteCollection($collectionName); + $database->deleteCollection($collectionName);
🧹 Nitpick comments (3)
tests/e2e/Adapter/Scopes/SpatialTests.php (3)
167-169: Normalize class casing: use Query:: not query::Class names should be consistently cased (PSR-1/PSR-12). While PHP resolves class names case-insensitively, keeping the canonical casing avoids surprises.
- 'equals' => query::equal('lineAttr', [[[1.0, 2.0], [3.0, 4.0]]]), // Exact same linestring + 'equals' => Query::equal('lineAttr', [[[1.0, 2.0], [3.0, 4.0]]]), // Exact same linestring - 'notEquals' => query::notEqual('lineAttr', [[[5.0, 6.0], [7.0, 8.0]]]), // Different linestring + 'notEquals' => Query::notEqual('lineAttr', [[[5.0, 6.0], [7.0, 8.0]]]), // Different linestring- 'equals' => query::equal('polyAttr', [[[[0.0, 0.0], [0.0, 10.0], [10.0, 10.0], [0.0, 0.0]]]]), // Exact same polygon + 'equals' => Query::equal('polyAttr', [[[[0.0, 0.0], [0.0, 10.0], [10.0, 10.0], [0.0, 0.0]]]]), // Exact same polygon - 'notEquals' => query::notEqual('polyAttr', [[[[20.0, 20.0], [20.0, 30.0], [30.0, 30.0], [20.0, 20.0]]]]), // Different polygon + 'notEquals' => Query::notEqual('polyAttr', [[[[20.0, 20.0], [20.0, 30.0], [30.0, 30.0], [20.0, 20.0]]]]), // Different polygon- $differentSquare = $database->find($collectionName, [ - query::notEqual('square', [[[0, 0], [0, 10], [10, 10], [10, 0], [0, 0]]]) // Different square + $differentSquare = $database->find($collectionName, [ + Query::notEqual('square', [[[0, 0], [0, 10], [10, 10], [10, 0], [0, 0]]]) // Different square ], Database::PERMISSION_READ);Also applies to: 203-204, 1068-1069
1712-1712: Fix test name typo: testSptialAggregation → testSpatialAggregationImproves readability and discoverability.
- public function testSptialAggregation(): void + public function testSpatialAggregation(): void
89-96: Also strip global debug prints from the runtime (SQL adapter).The adapter currently var_dump’s built SQL (see src/Database/Adapter/SQL.php in this PR). That will spam CI logs when these tests run. Please remove or gate behind a debug flag/env.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/e2e/Adapter/Scopes/SpatialTests.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/e2e/Adapter/Scopes/SpatialTests.php (2)
src/Database/Adapter/SQL.php (1)
find(2328-2530)src/Database/Database.php (1)
find(6235-6371)
| $result = $database->find($collectionName); | ||
|
|
||
| function decodePoint(string $wkb): array { | ||
| // WKB format: 1-byte byte order + 4-byte type + 8-byte X + 8-byte Y | ||
| // Skip first 5 bytes (1 byte byte order + 4 bytes type) | ||
| $coords = unpack('dX/dY', substr($wkb, 5, 16)); | ||
| return ['lon' => $coords['X'], 'lat' => $coords['Y']]; | ||
| } | ||
|
|
||
| // Example usage: | ||
| $pointBinary = "\000\000\000\000\000\000\000\000\000\000\000\000\000@\000\000\000\000\000\000@"; | ||
| $coords = decodePoint($pointBinary); | ||
| print_r($coords); | ||
|
|
||
| var_dump($result); | ||
| $this->assertEquals(999,888); |
There was a problem hiding this comment.
Remove debug scaffolding and the deliberate failing assertion.
Unconditional find/printouts and the hardcoded failing assertion will halt the suite (stop-on-failure) and pollute output. The nested named function can also trigger redeclaration if this test runs more than once.
Apply:
- $result = $database->find($collectionName);
-
- function decodePoint(string $wkb): array {
- // WKB format: 1-byte byte order + 4-byte type + 8-byte X + 8-byte Y
- // Skip first 5 bytes (1 byte byte order + 4 bytes type)
- $coords = unpack('dX/dY', substr($wkb, 5, 16));
- return ['lon' => $coords['X'], 'lat' => $coords['Y']];
- }
-
-// Example usage:
- $pointBinary = "\000\000\000\000\000\000\000\000\000\000\000\000\000@\000\000\000\000\000\000@";
- $coords = decodePoint($pointBinary);
- print_r($coords);
-
- var_dump($result);
- $this->assertEquals(999,888);
+ // Debug scaffolding removed.📝 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.
| $result = $database->find($collectionName); | |
| function decodePoint(string $wkb): array { | |
| // WKB format: 1-byte byte order + 4-byte type + 8-byte X + 8-byte Y | |
| // Skip first 5 bytes (1 byte byte order + 4 bytes type) | |
| $coords = unpack('dX/dY', substr($wkb, 5, 16)); | |
| return ['lon' => $coords['X'], 'lat' => $coords['Y']]; | |
| } | |
| // Example usage: | |
| $pointBinary = "\000\000\000\000\000\000\000\000\000\000\000\000\000@\000\000\000\000\000\000@"; | |
| $coords = decodePoint($pointBinary); | |
| print_r($coords); | |
| var_dump($result); | |
| $this->assertEquals(999,888); | |
| // Debug scaffolding removed. |
🤖 Prompt for AI Agents
In tests/e2e/Adapter/Scopes/SpatialTests.php around lines 141 to 156, remove the
temporary debug scaffolding: delete the nested named function decodePoint (or
convert it to a private helper method on the test class or an anonymous closure
to avoid redeclaration), remove the example WKB usage, print_r and var_dump
calls, and remove the hardcoded failing assertion
($this->assertEquals(999,888)); instead, either assert the actual expected
values from $result or omit the ad-hoc check until a proper assertion is
implemented so the test suite won’t be polluted or halted.
Summary by CodeRabbit
New Features
Breaking Changes
Improvements
Tests
Bug Fixes / Regressions