Joins #763
Joins #763
Conversation
# Conflicts: # composer.lock # src/Database/Database.php # tests/e2e/Adapter/Base.php
# Conflicts: # src/Database/Database.php # src/Database/Validator/Queries/Documents.php # src/Database/Validator/Query/Filter.php # src/Database/Validator/Query/Order.php # tests/unit/Validator/Query/FilterTest.php
# Conflicts: # composer.lock # src/Database/Adapter.php # src/Database/Adapter/Pool.php # src/Database/Adapter/SQL.php # src/Database/Database.php # src/Database/Query.php # src/Database/Validator/IndexedQueries.php # src/Database/Validator/Queries.php # src/Database/Validator/Queries/Document.php # src/Database/Validator/Queries/Documents.php # src/Database/Validator/Query/Filter.php # src/Database/Validator/Query/Order.php # src/Database/Validator/Query/Select.php # tests/e2e/Adapter/Base.php # tests/e2e/Adapter/Scopes/DocumentTests.php
# Conflicts: # src/Database/Database.php # src/Database/Query.php # tests/e2e/Adapter/Base.php
WalkthroughReplaces document-centric query handling with a QueryContext-driven model: Query gains join/relation primitives; Adapter.find and getAttributeProjection signatures change; Database and adapters adopt QueryContext; legacy validators commented out and replaced by a new Queries V2 validator; tests updated to per-field selects and QueryContext usage. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant QC as QueryContext
participant DB as Database
participant Adapter
rect rgb(240,250,240)
note right of Client: New context-driven read flow
Client->>QC: create(), add(collection, alias), set skipAuth...
Client->>DB: find(QC, limit, offset, selects, filters, joins, vectors, orderQueries)
DB->>Adapter: find(QC, limit, offset, cursor..., selects, filters, joins, vectors, orderQueries)
Adapter->>QC: getCollectionByAlias(alias) / skipAuth(...)
Adapter-->>DB: rows[]
DB-->>Client: documents[]
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Areas requiring extra attention:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (9)
src/Database/Adapter/Postgres.php (1)
1725-1787: Sanitize right-hand alias consistently inTYPE_RELATION_EQUALIn
getSQLCondition(), the left alias goes throughfilter()andquote(), but the right-hand alias inTYPE_RELATION_EQUALis only passed toquote():$alias = $query->getAlias(); $alias = $this->filter($alias); $alias = $this->quote($alias); // ... case Query::TYPE_RELATION_EQUAL: $attributeRight = $this->quote($this->filter($query->getAttributeRight())); $aliasRight = $this->quote($query->getRightAlias()); return "{$alias}.{$attribute}={$aliasRight}.{$attributeRight}";For symmetry and to keep alias handling as defensive as attribute and left-alias handling, consider filtering
aliasRightas well:- 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}";This keeps both sides aligned with the existing filtering rules and avoids any surprises if aliases ever become more dynamic.
tests/e2e/Adapter/Scopes/Relationships/ManyToManyTests.php (1)
111-160: Remove debuggingvar_dumpfrom testThe new
Query::selectcalls look correct and aligned with the updated API. However, thevar_dump($playlist);on Line 149 is a leftover debug statement and will pollute test output.Please remove it:
- if ($playlist->isEmpty()) { - throw new Exception('Playlist not found'); - } - var_dump($playlist); + if ($playlist->isEmpty()) { + throw new Exception('Playlist not found'); + }tests/unit/Validator/Query/FilterTest.php (2)
21-61: DocumentsValidator constructed without requiredidAttributeTypeAs with
OrderTest,DocumentsValidator (V2)’s constructor requires a secondstring $idAttributeTypeparameter, but here it’s omitted. This will cause anArgumentCountError.Recommend:
- $context = new QueryContext(); - - $context->add($collection); - - $this->validator = new DocumentsValidator($context); + $context = new QueryContext(); + $context->add($collection); + + $this->validator = new DocumentsValidator( + $context, + idAttributeType: Database::VAR_STRING, + );
123-145: Wrap all Query objects in arrays in the test methods; update error message expectations to include "Invalid query:" prefixThe V2 validator's
isValid()method requires an array parameter and immediately rejects non-arrays with "Queries must be an array". The affected tests pass unwrapped Query objects, so they will fail before exercising any validation logic.Tests requiring fixes (all with unwrapped Query objects):
testMaxValuesCount(line 131)testNotContains(lines 138, 139, 140, 143)testNotSearch(lines 150, 153, 157)testNotStartsWith(lines 164, 167, 171)testNotEndsWith(lines 178, 181, 185)testNotBetween(lines 192, 195, 199, 202)Additionally, error message expectations must be updated to include the "Invalid query:" prefix that V2 adds. For example, line 132 expects
'Query on attribute has greater than '.$max.' values: integer'but will receive'Invalid query: Query on attribute has greater than '.$max.' values: integer'.src/Database/Adapter/SQL.php (2)
203-208: Remove debugvar_dump()calls from adapter code
var_dump($document)inexists()andvar_dump($sql)in bothgetDocument()andfind()will emit raw data to STDOUT in production, corrupt responses, and leak internal details. These should be removed or replaced with proper logging hooks before release.Also applies to: 375-387, 3140-3149
3010-3071: Fix ORDER BY cursor logic:ORDER_RANDOMand alias handling for tie-breaksTwo issues in the order/cursor block:
getOrderDirection()is called for every$order, but it only supportsTYPE_ORDER_ASC/TYPE_ORDER_DESC. If aQuery::orderRandom()(methodTYPE_ORDER_RANDOM) ever reaches this code,getOrderDirection()will throw before theif ($direction === Database::ORDER_RANDOM)guard is reached, breaking random ordering.When building cursor tie-break conditions for previous order attributes, you use
$orderAlias(the current order’s alias) instead of each previous order query’s own alias:$prevQuery = $orderQueries[$j]; $prevOriginal = $prevQuery->getAttribute(); // ... $conditions[] = "{$this->quote($orderAlias)}.{$this->quote($prevAttr)} = {$bindName}";For multi-table joins where different
ORDER BYclauses use different aliases, this will generate incorrect or invalid SQL.Consider:
- Handling
TYPE_ORDER_RANDOMseparately based on$order->getMethod()(and skippinggetOrderDirection()for that case), and- Using
$prevQuery->getAlias()instead of$orderAliaswhen building equality conditions for previous order attributes.src/Database/Database.php (3)
4316-4410: Ensure system$permissionsselect is stripped consistently on cache hitsWhen
getDocument()auto-injects a systemQuery::select('$permissions', system: true),$permissionsAddedis only used to remove$permissionson the non‑cached code path (after adapter fetch and caching). On the cached path (if ($cached)), the document is returned without re‑applying the same removal, so callers can observe$permissionsonly when a cache hit occurs, even if they never selected it.This is an inconsistent projection and can surprise consumers that rely on selects to control the visible shape.
Consider also applying the
$permissionsAddedcleanup on the cached branch:try { $cached = $this->cache->load($documentKey, self::TTL, $hashKey); } catch (Exception $e) { Console::warning('Warning: Failed to get document from cache: ' . $e->getMessage()); $cached = null; } if ($cached) { $document = $this->createDocumentInstance($collection->getId(), $cached); if ($collection->getId() !== self::METADATA) { if (!$this->authorization->isValid(new Input(self::PERMISSION_READ, [ ...$collection->getRead(), ...($documentSecurity ? $document->getRead() : []) ]))) { return $this->createDocumentInstance($collection->getId(), []); } } + + if ($permissionsAdded) { + $document->removeAttribute('$permissions'); + } $this->trigger(self::EVENT_DOCUMENT_READ, $document); return $document; } @@ - if ($permissionsAdded) { // Or remove all queries added by system + if ($permissionsAdded) { // Or remove all queries added by system $document->removeAttribute('$permissions'); }
6705-6723: Avoid double-encoding documents inupsertDocumentsWithIncreaseIn
upsertDocumentsWithIncrease(), each document is passed throughencode($collection, $document)twice in succession (once beforeStructurevalidation and once again at line 6722). For built‑in filters this is mostly idempotent, but it’s wasted work and could break custom filters that don’t expect to be run twice.You can safely drop the second encode:
$document = $this->encode($collection, $document); if ($this->validate) { $validator = new Structure( @@ if (!$validator->isValid($document)) { throw new StructureException($validator->getDescription()); } } - - $document = $this->encode($collection, $document);
8823-8970: Nested many‑to‑many relationship filters likely broken byskipRelationships()
processNestedRelationshipPath()andconvertRelationshipFiltersToSubqueries()currently use:
skipRelationships()for the reverse‑lookup step inprocessNestedRelationshipPath(inside the$needsReverseLookupbranch), andfind()withoutskipRelationships()for top‑level many‑to‑many filters.For many‑to‑many, the relationship cardinality is materialized via junction collections and populated relationship attributes (including the two‑way key) during relationship resolution. The retrieved learning explicitly notes that many‑to‑many filtering must rely on
find()withoutskipRelationships()so that relationship attributes are populated and parent IDs can be derived from them. Based on learnings, usingskipRelationships()on the nested path step means$doc->getAttribute($twoWayKey)will be empty for many‑to‑many links in multi‑hop paths, so nested filters likeproject.manager.skills.category.namewill fail to resolve matching parent IDs.You can preserve the intended behavior by special‑casing many‑to‑many in
processNestedRelationshipPath()so it doesn’t useskipRelationships()when extracting parent IDs:- $needsReverseLookup = ( + $needsReverseLookup = ( ($relationType === self::RELATION_ONE_TO_MANY && $side === self::RELATION_SIDE_PARENT) || ($relationType === self::RELATION_MANY_TO_ONE && $side === self::RELATION_SIDE_CHILD) || ($relationType === self::RELATION_MANY_TO_MANY) ); if ($needsReverseLookup) { - // Need to find parents by querying children and extracting parent IDs - $childDocs = $this->silent(fn () => $this->skipRelationships(fn () => $this->find( - $link['toCollection'], - [ - Query::equal('$id', $matchingIds), - Query::select('$id'), - Query::select($link['twoWayKey']), - Query::limit(PHP_INT_MAX), - ] - ))); + // Need to find parents by querying children and extracting parent IDs + $fetchChildren = fn () => $this->find( + $link['toCollection'], + [ + Query::equal('$id', $matchingIds), + Query::select('$id'), + Query::select($link['twoWayKey']), + Query::limit(PHP_INT_MAX), + ] + ); + + // For many-to-many we must NOT skip relationships so that the two-way key + // is populated from the junction collection (see convertRelationshipQueries learning). + $childDocs = $relationType === self::RELATION_MANY_TO_MANY + ? $this->silent($fetchChildren) + : $this->silent(fn () => $this->skipRelationships($fetchChildren));This keeps the current optimization for one‑to‑many / many‑to‑one, while restoring the required behavior for many‑to‑many nested paths. Based on learnings.
Also applies to: 8994-9197
🧹 Nitpick comments (26)
tests/e2e/Adapter/Scopes/RelationshipTests.php (1)
1415-1692: Internal attribute expectations intestSelectRelationshipAttributesmay be brittleThese assertions thoroughly document the behavior of system fields (
$id,$sequence,$collection,$createdAt,$updatedAt,$permissions) under many select combinations, which is valuable coverage.However, this also turns internal/meta attribute behavior into a strict external contract. If internal projection rules change (e.g. always including
$createdAtor$permissionsfor caching/auth reasons), many of these negativeassertArrayNotHasKeychecks will start failing.If the intent is to keep these rules stable long-term, this is fine. Otherwise, consider relaxing some of the negative assertions on internal keys to reduce coupling to implementation details, while still checking the core functional behavior (which user-visible fields are present, and when relationships are populated).
tests/unit/Validator/QueriesTest.php (1)
3-80: Fully commented-out test file – consider deleting or replacing with new testsAll code in this test file is commented out, so it no longer exercises anything at runtime. If the old
Queriesvalidator is truly replaced by newer, context-driven validators, it would be cleaner to either:
- remove this file entirely, or
- replace it with tests targeting the new validator/API,
rather than leaving a dead, commented-out test class.
src/Database/Adapter/MariaDB.php (1)
1547-1556:getSQLConditionalias/right-attribute handling andrelationEquallook correct; minor consistency nit
- Normalizing both sides via
getInternalKeyForAttribute()and then quoting via$this->quote()yields safe, engine-appropriate identifiers like`main`.`_uid`.- The new
TYPE_RELATION_EQUALbranch:$attributeRight = $this->quote($this->filter($query->getAttributeRight())); $aliasRight = $this->quote($query->getRightAlias()); return "{$alias}.{$attribute}={$aliasRight}.{$attributeRight}";generates the expected join-style predicate between two aliases.
For consistency/defense-in-depth, consider also passing
getRightAlias()through$this->filter()before quoting, mirroring how the left alias is handled, even if aliases are already validated upstream.Also applies to: 1597-1602
tests/unit/Validator/DocumentsQueriesTest.php (1)
131-171: AlignidAttributeTypewith the schema’sidattributeHere the validator is constructed as:
$validator = new DocumentsValidator( $this->context, Database::VAR_INTEGER );but the schema above defines:
new Document([ '$id' => 'id', 'key' => 'id', 'type' => Database::VAR_ID, // ... ])If
V2uses the$idAttributeTypeto validate queries onid(e.g.Query::notEqual('id', '1000000')), passingDatabase::VAR_INTEGERwhile the declared type isDatabase::VAR_IDmay lead to inconsistent behavior or mask type issues.Consider either:
- Switching this second argument to
Database::VAR_ID(or whatever the real$idtype is meant to be), or- Adding a short comment explaining why
VAR_INTEGERis intentional here (e.g. if you’re simulating a numeric ID schema on purpose).Also, the updated limit error expectation:
$this->assertEquals( 'Invalid limit: Value must be a valid range between 1 and ' . number_format(PHP_INT_MAX), $validator->getDescription() );looks consistent with the
V2message format (no longer prefixed with “Invalid query:”), so just ensure this matches the actual message inV2::__construct/Limitvalidation.Also applies to: 185-188
tests/unit/Validator/DocumentQueriesTest.php (1)
23-57: Clarify intent oftestInvalidQueries(currently asserting valid)This test class has:
public function testInvalidQueries(): void { $validator = new DocumentsValidator($this->context); $queries = [ Query::limit(1) ]; /** * Think what to do about this? */ //$this->assertEquals(false, $validator->isValid($queries)); $this->assertEquals(true, $validator->isValid($queries)); }As written:
- The method name suggests it should cover invalid queries.
- The inline comment indicates prior intent to assert
isValid(...) === false.- The current assertion explicitly expects the query to be valid.
This is confusing and leaves you without any negative coverage for document-scoped queries. It’d be better to do one of:
- Decide that
Query::limit(1)is genuinely invalid in the “document” scope, restore thefalseassertion, and adjustV2accordingly; or- Decide that limits are allowed here, rename the test (or add a new one) to reflect that, and add a separate test that clearly exercises an actually invalid query.
Either way, cleaning up the commented assertion and making the test name + expectation consistent will help future readers understand the intended behavior.
Also applies to: 66-96
src/Database/Validator/Queries/Documents.php (1)
3-81: File is entirely commented-out; consider removal or explicit deprecationThis file now consists only of commented legacy code for the
Documentsvalidator. Since the runtime implementation has moved toQueries\V2, keeping this large commented block adds noise without value.Consider either:
- Removing the file entirely, or
- Replacing it with a minimal stub that clearly marks the old class as removed/deprecated and points to
Queries\V2.tests/e2e/Adapter/Scopes/JoinsTests.php (1)
62-110: Clean up unused session variables in join tests
$sessionNoPermissions(Line 62) and$session3(Line 102) are created but never used, which static analysis rightfully flags as unused locals.Either use them in assertions (e.g., to clarify which sessions participate in joins), or drop the variables and keep only the
createDocumentcalls:- $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, ]));tests/unit/Validator/Query/FilterTest.php (1)
135-204: Align not tests with V2’s error-message format*The tests for
notSearch,notStartsWith,notEndsWith, andnotBetweenstill expect bare messages like:
Cannot query notSearch on attribute "string_array" because it is an array.NotBetween queries require exactly two values.V2 typically prefixes messages with
Invalid query: ...(as already updated intestEmptyValuesand type-mismatch tests). After switching toDocumentsValidator, these expectations are likely stale.Once you wrap the queries in arrays, please re-run the tests and update the
assertEqualsstrings to whatever V2 actually returns, probably in the form:
Invalid query: Cannot query notSearch on attribute "string_array" because it is an array.Invalid query: NotBetween queries require exactly two values.src/Database/QueryContext.php (2)
60-68: Preferarray_key_exists()overempty()for alias collision detection.The ambiguous alias check uses
! empty($this->aliases[$alias]), which treats empty strings, false, 0, etc. as non-existent. While this works in practice (collection IDs are never empty), it's clearer to use explicit existence checks.Apply this diff to improve clarity:
- if (! empty($this->aliases[$alias])) { + if (array_key_exists($alias, $this->aliases)) { throw new QueryException('Ambiguous alias for collection "'.$collection->getId().'".'); }
75-86: SimplifyskipAuthlogic for clarity.The current implementation uses
empty()to check both existence and truthiness, which is non-obvious. The logic can be simplified using the null coalescing operator.Apply this diff to improve readability:
public function skipAuth(string $collection, string $permission, Authorization $authorization): bool { if (!$authorization->getStatus()) { // for Authorization::disable(); return true; } - if (empty($this->skipAuthCollections[$collection][$permission])) { - return false; - } - - return true; + return $this->skipAuthCollections[$collection][$permission] ?? false; }tests/e2e/Adapter/Scopes/DocumentTests.php (1)
3702-3843:testFindSelectthoroughly covers system-field projection; consider unifyinggetDatabase()usageThe expanded cases for
Query::select('name')/('year')combined with selects for$id,$sequence,$collection,$createdAt,$updatedAt, and$permissionsprovide strong coverage that:
$idand$collectionare always present, and- other internal fields only appear when explicitly selected.
This matches the new projection contract well. The only minor nit is mixing
$database = $this->getDatabase();with calls tostatic::getDatabase()within the same test; keeping a single style (preferably the instance method) would avoid confusion about whethergetDatabaseis intended to be static.- $documents = static::getDatabase()->find('movies', [ + $documents = $database->find('movies', [ Query::select('name'), Query::select('year'), Query::select('$createdAt') ]); ... - $documents = static::getDatabase()->find('movies', [ + $documents = $database->find('movies', [ Query::select('name'), Query::select('year'), Query::select('$permissions') ]);src/Database/Validator/Queries/V2.php (1)
585-595: Clarify distance query expected value shapeFor distance-related methods you validate with:
if (count($query->getValues()) !== 1 || !is_array($query->getValues()[0]) || count($query->getValues()[0]) !== 3 ) { throw new \Exception('Distance query requires [[geometry, distance]] parameters'); }The code expects an inner array of length 3, but the error message refers to
[[geometry, distance]](2 elements). Either the message or the count check is inconsistent. Please align the check and message with the actual expected structure so callers know what to send.src/Database/Adapter/Mongo.php (1)
1876-1887: Silence static-analysis noise for unused$joinsand$vectors(optional)
Mongo::find()intentionally ignores$joinsand$vectorsbecause this adapter does not support relationships or vector operations, but PHPMD flags them as unused formal parameters.If you want to satisfy static analysis without changing behaviour, you can add:
unset($joins, $vectors);near the top of the method.
src/Database/Query.php (12)
135-170: Filter type list contains a duplicate entry and includes logical operators
FILTER_TYPEScurrently containsself::TYPE_GREATER_EQUALtwice. This doesn’t break behavior (becausein_arrayignores duplicates) but is noisy and could confuse readers.Also note that
FILTER_TYPESincludesTYPE_ANDandTYPE_OR. That makesisFilter()returntruefor logical queries and causesgetFilterQueries()to return logical wrappers rather than only “leaf” filters. If that’s intentional, it’s fine; if callers expect only atomic filters, you may want to:
- Drop
TYPE_AND/TYPE_ORfromFILTER_TYPES, or- Rename these utilities (
getFilterRootsvsgetLeafFilters) to make semantics explicit.At minimum, consider removing the duplicate constant:
- self::TYPE_GREATER_EQUAL, - self::TYPE_GREATER_EQUAL, + self::TYPE_GREATER_EQUAL,
195-230: Constructor defaults and docblock driftThe extended constructor signature and DEFAULT_ALIAS fallback logic look coherent (everything without an explicit alias/right‑alias becomes
'main'). Two nits:
- The docblock above still documents only
$method,$attribute,$values. It should be updated to match the new parameters, especially since this is now the central factory entrypoint.- If there is ever a use‑case for an “unaliased” query, the unconditional fallback to
DEFAULT_ALIASmakes that impossible. If that’s not desired, all good; otherwise, you might want an explicit “no alias” sentinel instead ofempty($alias).These are documentation / design clarity issues rather than correctness problems.
335-359: Cursor / order direction helpers: behavior OK, but exception type is generic
getCursorDirection()andgetOrderDirection()correctly map the internal methods toDatabase::CURSOR_*andDatabase::ORDER_*constants and throw on invalid usage.You may want to:
- Throw a domain‑specific exception (
QueryException) rather than a bare\Exceptionto make error handling more consistent.
This is non‑blocking and can be deferred.
441-471: Type hints forisSpatialQuery/isVectorQueryBoth helpers accept
$methodas an untyped parameter and the docblocks don’t specify a concrete type. Everywhere else, methods are treated as strings, so for consistency and static‑analysis friendliness it would be better to explicitly type these:- public static function isSpatialQuery($method): bool + public static function isSpatialQuery(string $method): bool - public static function isVectorQuery($method): bool + public static function isVectorQuery(string $method): boolAlso consider using
in_array(..., true)inisVectorQueryto keep comparisons strictly string‑based.
600-667: Alias support in scalar filter factories is partial
equal,notEqual,lessThan*,greaterThan*, andbetweennow take an optional$aliasand correctly pass it into the constructor. Other helpers likenotBetween,search,isNull, etc., still don’t accept an alias and therefore always default toDEFAULT_ALIAS.If you intend aliasing to be usable across all filter types, you probably want to extend the remaining factories as well so callers don’t need to special‑case some operators. If aliasing is only meant for a subset, this is fine but should be clearly documented.
743-746: Unused$functionparameter onselect()
select()accepts$functionbut never uses it in the constructor call, and it’s flagged by PHPMD. This suggests either:
- The parameter is planned for future aggregate behavior and is currently dead, or
- It should have been threaded into the query (e.g. into
$valuesor a dedicated property).To avoid confusion and static‑analysis noise, consider:
- Wiring
$functioninto the query representation if you actually need it, or- Dropping it from the signature (or annotating as deprecated) until you implement function‑based selects.
1045-1105: Limit/offset helpers: fine, but duplicate traversal pattern
getLimitQueries/getOffsetQueriesand their scalar wrappers work as expected: they return at most one query and extract its first value, with a sensible$defaultfallback.You may at some point want to DRY the traversal logic (limit and offset are structurally identical), but that’s purely a cleanliness refactor.
1111-1148: Cursor helpers andgetCursorDocumentdesign
getOrderQueries()is a straightforward reuse ofgetByType()and looks good.getCursorQueries()correctly returns the first cursor query (already cloned bygetByType()).For
getCursorDocument(?Query $query): Document:
- The method doesn’t use
$thisat all, so it is effectively a static helper but is defined as an instance method.- The docblock says
@param Query $querywhile the signature is?Query, so the annotations don’t match.- It returns a fresh
Documentwhen$queryis null or not a cursor, which aligns with thecursor: Document|nullsemantics ingroupByType.To make intent clearer, I’d suggest:
- public function getCursorDocument(?Query $query): Document + public static function getCursorDocument(?Query $query): Documentand updating the docblock accordingly. Behavior can stay as‑is.
1183-1265:groupByTypeignores joins and has an unused$joinsvariableInside
groupByType():
$joins = [];is declared but never populated or returned, which is exactly what PHPMD flagged. This is dead state and should either be:
- Removed, or
- Fully wired so joins are grouped and exposed in the returned array (e.g. return key
'joins' => $joinsplus acasearm for join methods).Join queries currently fall into the
default:branch, so they are classified as “filters” alongside ordinary conditions. If your downstream consumers now distinguish filters vs joins via the newer helpers (getJoinQueries,isJoin()), that may be OK. But if any existing code still relies solely ongroupByType()for top‑level classification, you may want to add explicit cases for join types and surface them in the returned structure.Given the new join capabilities, I’d recommend either:
- Extending
groupByType()’s return contract to addjoins, or- Removing
$joinshere and steering all join‑specific grouping through the dedicated helpers to avoid confusion.
1283-1299:isJoin/isFilterhelpers are useful but could be stricter
isJoin()correctly checks the method against the three join types.
isFilter()usesin_array($method, self::FILTER_TYPES)without strict comparison. Since methods are always strings, consider adding strict mode to guard against accidental int/enum values:- return in_array($method, self::FILTER_TYPES); + return in_array($method, self::FILTER_TYPES, true);Otherwise, these helpers are fine.
1185-1187: PHPMD unused local$joinshintPHPMD correctly pointed out that
$joins(initialized at Line 1186) is never used. Depending on your plan forgroupByType():
- If you’re not going to expose joins here, remove
$joinsentirely.- If you do plan to expose them, wire them into the switch and the return structure.
See the earlier
groupByTypecomment for details.
743-743: PHPMD unused$functionparameter hintThe PHPMD hint about
$functioninselect()is valid: the parameter is never used. Addressing it as suggested in theselect()comment (either wiring it into the query or removing/deprecating it) will clean up static‑analysis noise.src/Database/Database.php (1)
8483-8532:validateSelections()now mixesQueryobjects and strings
validateSelections()was adapted to takearray<Query>but still treats$selectionsas if it were a list of strings:
$selectionsand$relationshipSelectionsare arrays ofQueryinstances.- The check
array_diff($selections, $keys)comparesQueryobjects to strings, which is not meaningful and will mark every selection as “invalid”.- Later, string literals like
'$id','$sequence', etc., are appended into the same$selectionsarray, mixing types.It looks like this method is currently unused, but if it’s reintroduced it will misbehave.
Recommend either:
- Refactoring it to operate on attribute names (
$query->getAttribute()/$query->getValue()), returning an array ofQueryobjects, or- Removing it entirely until you actually need selection validation in the Query‑object world.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (46)
phpunit.xml(2 hunks)src/Database/Adapter.php(2 hunks)src/Database/Adapter/MariaDB.php(3 hunks)src/Database/Adapter/Mongo.php(11 hunks)src/Database/Adapter/Pool.php(3 hunks)src/Database/Adapter/Postgres.php(3 hunks)src/Database/Adapter/SQL.php(9 hunks)src/Database/Database.php(60 hunks)src/Database/Query.php(24 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/MirrorTest.php(1 hunks)tests/e2e/Adapter/Scopes/AttributeTests.php(3 hunks)tests/e2e/Adapter/Scopes/CollectionTests.php(1 hunks)tests/e2e/Adapter/Scopes/DocumentTests.php(17 hunks)tests/e2e/Adapter/Scopes/JoinsTests.php(1 hunks)tests/e2e/Adapter/Scopes/ObjectAttributeTests.php(7 hunks)tests/e2e/Adapter/Scopes/PermissionTests.php(1 hunks)tests/e2e/Adapter/Scopes/RelationshipTests.php(24 hunks)tests/e2e/Adapter/Scopes/Relationships/ManyToManyTests.php(6 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/e2e/Adapter/Scopes/SpatialTests.php(3 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(2 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
🧠 Learnings (5)
📚 Learning: 2025-10-03T02:04:17.803Z
Learnt from: abnegate
Repo: utopia-php/database PR: 721
File: tests/e2e/Adapter/Scopes/DocumentTests.php:6418-6439
Timestamp: 2025-10-03T02:04:17.803Z
Learning: In tests/e2e/Adapter/Scopes/DocumentTests::testSchemalessDocumentInvalidInteralAttributeValidation (PHP), when the adapter reports getSupportForAttributes() === false (schemaless), the test should not expect exceptions from createDocuments for “invalid” internal attributes; remove try/catch and ensure the test passes without exceptions, keeping at least one assertion.
Applied to files:
tests/e2e/Adapter/MirrorTest.phptests/e2e/Adapter/Scopes/PermissionTests.phptests/unit/Validator/QueriesTest.phptests/e2e/Adapter/Scopes/AttributeTests.phptests/unit/Validator/Query/OrderTest.phptests/e2e/Adapter/Base.phptests/e2e/Adapter/Scopes/Relationships/ManyToManyTests.phptests/unit/Validator/Query/FilterTest.phptests/e2e/Adapter/Scopes/JoinsTests.phptests/e2e/Adapter/Scopes/ObjectAttributeTests.phptests/unit/Validator/DocumentsQueriesTest.phptests/e2e/Adapter/Scopes/Relationships/OneToOneTests.phptests/e2e/Adapter/Scopes/SpatialTests.phptests/unit/Validator/DocumentQueriesTest.phptests/e2e/Adapter/Scopes/Relationships/OneToManyTests.phpsrc/Database/Validator/Queries/Documents.phptests/unit/Validator/Query/SelectTest.phpsrc/Database/Validator/Queries/Document.phptests/unit/Validator/QueryTest.phptests/e2e/Adapter/Scopes/CollectionTests.phptests/unit/Validator/IndexedQueriesTest.phptests/e2e/Adapter/Scopes/DocumentTests.phptests/unit/Validator/Query/CursorTest.phpsrc/Database/Adapter/Mongo.phpsrc/Database/Database.phptests/e2e/Adapter/Scopes/RelationshipTests.phptests/e2e/Adapter/Scopes/Relationships/ManyToOneTests.php
📚 Learning: 2025-10-29T12:27:57.071Z
Learnt from: ArnabChatterjee20k
Repo: utopia-php/database PR: 747
File: src/Database/Adapter/Mongo.php:1449-1453
Timestamp: 2025-10-29T12:27:57.071Z
Learning: In src/Database/Adapter/Mongo.php, when getSupportForAttributes() returns false (schemaless mode), the updateDocument method intentionally uses a raw document without $set operator for replacement-style updates, as confirmed by the repository maintainer ArnabChatterjee20k.
Applied to files:
tests/e2e/Adapter/Scopes/AttributeTests.phptests/e2e/Adapter/Scopes/CollectionTests.phptests/e2e/Adapter/Scopes/DocumentTests.phpsrc/Database/Adapter/Mongo.php
📚 Learning: 2025-10-20T05:29:29.487Z
Learnt from: abnegate
Repo: utopia-php/database PR: 731
File: src/Database/Database.php:6987-6988
Timestamp: 2025-10-20T05:29:29.487Z
Learning: In Utopia\Database\Database::convertRelationshipQueries, Many-to-Many filtering does not need the parent collection or a direct junction query: it calls find() on the related collection without skipping relationships, which populates relationship attributes (including the two-way key). Parent IDs are derived from that populated attribute. Therefore, calls should remain convertRelationshipQueries($relationships, $queries).
Applied to files:
tests/e2e/Adapter/Scopes/Relationships/ManyToManyTests.phptests/e2e/Adapter/Scopes/JoinsTests.phptests/e2e/Adapter/Scopes/Relationships/OneToOneTests.phptests/e2e/Adapter/Scopes/Relationships/OneToManyTests.phpsrc/Database/Database.phptests/e2e/Adapter/Scopes/RelationshipTests.phptests/e2e/Adapter/Scopes/Relationships/ManyToOneTests.php
📚 Learning: 2025-08-14T06:35:30.429Z
Learnt from: ArnabChatterjee20k
Repo: utopia-php/database PR: 661
File: tests/e2e/Adapter/Scopes/SpatialTests.php:180-186
Timestamp: 2025-08-14T06:35:30.429Z
Learning: Query::distance method in Utopia\Database\Query expects an array of values parameter, where each value is a [geometry, distance] pair. So the correct format is Query::distance('attribute', [[[lat, lng], distance]]) where the outer array contains the values and each value is [geometry, distance].
Applied to files:
tests/e2e/Adapter/Scopes/SpatialTests.phpsrc/Database/Query.php
📚 Learning: 2025-07-30T19:17:53.630Z
Learnt from: ArnabChatterjee20k
Repo: utopia-php/database PR: 642
File: src/Database/Validator/PartialStructure.php:43-52
Timestamp: 2025-07-30T19:17:53.630Z
Learning: In PartialStructure validator, when filtering for required attributes validation using the $requiredAttributes parameter, $this->attributes should be used instead of the merged $attributes array because this validation is specifically for internal attributes like $createdAt and $updatedAt that are defined in the base Structure class, not collection-specific attributes.
Applied to files:
tests/unit/Validator/IndexedQueriesTest.phpsrc/Database/Database.php
🧬 Code graph analysis (33)
tests/e2e/Adapter/Scopes/AttributeTests.php (2)
src/Database/Database.php (1)
getCollection(1812-1832)src/Database/Query.php (3)
getCollection(294-297)Query(8-1578)select(743-746)
tests/unit/Validator/Query/OrderTest.php (3)
src/Database/QueryContext.php (2)
QueryContext(8-89)add(60-68)src/Database/Validator/Queries/V2.php (3)
V2(23-791)isValid(534-790)getDescription(130-133)src/Database/Query.php (6)
Query(8-1578)orderAsc(765-768)orderDesc(754-757)limit(786-789)offset(797-800)equal(600-603)
src/Database/Validator/Alias.php (1)
src/Database/Validator/Queries/V2.php (4)
getDescription(130-133)isValid(534-790)isArray(140-143)getType(150-153)
src/Database/Adapter/Postgres.php (3)
src/Database/Query.php (6)
setAttributeRight(328-333)getAttributeRight(284-287)getAttribute(252-255)getAlias(274-277)Query(8-1578)getRightAlias(279-282)src/Database/Adapter/SQL.php (1)
getInternalKeyForAttribute(2387-2399)src/Database/Adapter.php (2)
filter(1259-1268)quote(491-491)
tests/e2e/Adapter/Scopes/Relationships/ManyToManyTests.php (2)
tests/e2e/Adapter/Base.php (1)
getDatabase(52-52)src/Database/Query.php (3)
Query(8-1578)select(743-746)getAttribute(252-255)
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-791)isValid(534-790)getDescription(130-133)
tests/e2e/Adapter/Scopes/JoinsTests.php (4)
src/Database/Adapter.php (3)
Adapter(17-1456)getSupportForRelationships(1038-1038)find(829-841)src/Database/Database.php (2)
Database(38-9428)find(7741-7893)src/Database/Document.php (2)
Document(12-470)getId(63-66)src/Database/Query.php (8)
Query(8-1578)join(958-961)relationEqual(996-999)equal(600-603)orderAsc(765-768)orderDesc(754-757)select(743-746)getAttribute(252-255)
tests/e2e/Adapter/Scopes/ObjectAttributeTests.php (1)
src/Database/Query.php (2)
Query(8-1578)select(743-746)
src/Database/QueryContext.php (3)
src/Database/Database.php (1)
Database(38-9428)src/Database/Query.php (1)
Query(8-1578)src/Database/Validator/Authorization.php (2)
Authorization(8-236)getStatus(157-160)
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-791)getDescription(130-133)tests/unit/Validator/Query/FilterTest.php (1)
setUp(19-61)
tests/e2e/Adapter/Scopes/Relationships/OneToOneTests.php (4)
src/Database/Query.php (2)
Query(8-1578)select(743-746)src/Database/Adapter.php (1)
getDocument(715-715)src/Database/Adapter/SQL.php (1)
getDocument(365-430)src/Database/Database.php (1)
getDocument(4275-4414)
tests/e2e/Adapter/Scopes/SpatialTests.php (2)
src/Database/QueryContext.php (2)
QueryContext(8-89)add(60-68)src/Database/Query.php (2)
Query(8-1578)select(743-746)
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-791)isValid(534-790)src/Database/Query.php (3)
Query(8-1578)select(743-746)limit(786-789)
src/Database/Adapter.php (5)
src/Database/Adapter/Mongo.php (2)
find(1876-2066)getAttributeProjection(2564-2597)src/Database/Adapter/Pool.php (2)
find(271-285)getAttributeProjection(507-510)src/Database/Adapter/SQL.php (2)
find(2983-3204)getAttributeProjection(2338-2385)src/Database/Database.php (2)
find(7741-7893)Database(38-9428)src/Database/QueryContext.php (1)
QueryContext(8-89)
tests/e2e/Adapter/Scopes/Relationships/OneToManyTests.php (1)
src/Database/Query.php (2)
Query(8-1578)select(743-746)
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-791)isValid(534-790)getDescription(130-133)src/Database/Query.php (3)
Query(8-1578)select(743-746)limit(786-789)
tests/unit/QueryTest.php (1)
src/Database/Query.php (16)
Query(8-1578)equal(600-603)getMethod(244-247)getAttribute(252-255)getValues(260-263)getAlias(274-277)orderDesc(754-757)limit(786-789)parse(480-493)select(743-746)getAs(289-292)join(958-961)relationEqual(996-999)getCollection(294-297)getRightAlias(279-282)getAttributeRight(284-287)
src/Database/Adapter/SQL.php (3)
src/Database/Database.php (3)
Database(38-9428)find(7741-7893)getCollection(1812-1832)src/Database/QueryContext.php (3)
QueryContext(8-89)getCollections(32-35)skipAuth(75-86)src/Database/Adapter.php (6)
getAttributeProjection(1250-1250)quote(491-491)getTenantQuery(1350-1350)getSupportForUpdateLock(1040-1040)filter(1259-1268)find(829-841)
src/Database/Adapter/MariaDB.php (4)
src/Database/Query.php (6)
setAttributeRight(328-333)getAttributeRight(284-287)getAttribute(252-255)getAlias(274-277)Query(8-1578)getRightAlias(279-282)src/Database/Adapter/SQL.php (1)
getInternalKeyForAttribute(2387-2399)src/Database/Adapter.php (2)
filter(1259-1268)quote(491-491)src/Database/Adapter/Postgres.php (1)
quote(2169-2172)
src/Database/Adapter/Pool.php (5)
src/Database/Database.php (2)
Database(38-9428)find(7741-7893)src/Database/QueryContext.php (1)
QueryContext(8-89)src/Database/Adapter.php (2)
find(829-841)getAttributeProjection(1250-1250)src/Database/Adapter/Mongo.php (2)
find(1876-2066)getAttributeProjection(2564-2597)src/Database/Adapter/SQL.php (2)
find(2983-3204)getAttributeProjection(2338-2385)
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-791)isValid(534-790)src/Database/Query.php (4)
Query(8-1578)select(743-746)getCursorQueries(1123-1135)getMethod(244-247)
tests/e2e/Adapter/Scopes/CollectionTests.php (5)
src/Database/Adapter.php (1)
getSupportForSchemas(953-953)src/Database/Adapter/Mongo.php (1)
getSupportForSchemas(2652-2655)src/Database/Adapter/Pool.php (1)
getSupportForSchemas(342-345)src/Database/Adapter/SQL.php (1)
getSupportForSchemas(929-932)src/Database/Adapter/SQLite.php (1)
getSupportForSchemas(896-899)
tests/unit/Validator/IndexedQueriesTest.php (2)
src/Database/QueryContext.php (2)
QueryContext(8-89)add(60-68)src/Database/Validator/Queries/V2.php (2)
V2(23-791)isValid(534-790)
tests/e2e/Adapter/Scopes/DocumentTests.php (3)
src/Database/Database.php (2)
Database(38-9428)decode(8201-8285)src/Database/QueryContext.php (2)
QueryContext(8-89)add(60-68)src/Database/Query.php (2)
Query(8-1578)select(743-746)
src/Database/Validator/Queries/V2.php (10)
src/Database/Database.php (2)
Database(38-9428)__construct(447-676)src/Database/Query.php (14)
Query(8-1578)__construct(195-230)getAttribute(252-255)isNested(1273-1280)getAlias(274-277)getMethod(244-247)getRightAlias(279-282)getFilterQueries(1154-1157)getValues(260-263)isSpatialQuery(445-462)isVectorQuery(468-471)getAs(289-292)parse(480-493)getAttributeRight(284-287)src/Database/QueryContext.php (4)
QueryContext(8-89)__construct(25-27)getCollections(32-35)getCollectionByAlias(37-55)src/Database/Validator/Alias.php (3)
Alias(7-70)getDescription(15-18)isValid(26-45)src/Database/Validator/AsQuery.php (4)
AsQuery(7-84)__construct(14-18)getDescription(24-27)isValid(35-59)src/Database/Validator/Datetime.php (1)
Datetime(7-152)src/Database/Validator/Query/Cursor.php (1)
Cursor(10-56)src/Database/Validator/Query/Limit.php (1)
Limit(9-63)src/Database/Validator/Query/Offset.php (3)
Offset(9-59)__construct(16-19)isValid(25-53)src/Database/Validator/Sequence.php (1)
Sequence(9-60)
src/Database/Validator/Query/Offset.php (1)
src/Database/Validator/Query/Base.php (1)
getDescription(26-29)
tests/unit/Validator/Query/CursorTest.php (4)
src/Database/Document.php (1)
Document(12-470)src/Database/Exception.php (1)
Exception(7-21)src/Database/Query.php (3)
Query(8-1578)cursorAfter(808-811)cursorBefore(819-822)src/Database/Validator/Query/Cursor.php (1)
Cursor(10-56)
src/Database/Adapter/Mongo.php (5)
src/Database/QueryContext.php (3)
QueryContext(8-89)getCollections(32-35)skipAuth(75-86)src/Database/Adapter.php (4)
renameAttribute(628-628)filter(1259-1268)getAttributeProjection(1250-1250)find(829-841)src/Database/Adapter/Pool.php (3)
renameAttribute(196-199)getAttributeProjection(507-510)find(271-285)src/Database/Document.php (3)
removeAttribute(287-293)getId(63-66)Document(12-470)src/Database/Query.php (7)
Query(8-1578)equal(600-603)limit(786-789)offset(797-800)getAttribute(252-255)getOrderDirection(348-359)select(743-746)
src/Database/Query.php (2)
src/Database/Database.php (3)
__construct(447-676)getCollection(1812-1832)Database(38-9428)src/Database/Document.php (4)
__construct(28-58)getCollection(85-88)Document(12-470)getAttribute(224-231)
src/Database/Database.php (4)
src/Database/Validator/Queries/V2.php (1)
V2(23-791)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 (9)
getIdAttributeType(936-936)getSupportForAttributes(960-960)getMinDateTime(929-929)getMaxDateTime(943-946)getMaxUIDLength(922-922)getDocument(715-715)filter(1259-1268)find(829-841)getSupportForCasting(1017-1017)src/Database/Query.php (20)
Query(8-1578)getSelectQueries(1025-1030)addSelect(1544-1577)select(743-746)getAttribute(252-255)getJoinQueries(1036-1043)join(958-961)getCollection(294-297)getAlias(274-277)limit(786-789)getLimitQuery(1065-1074)offset(797-800)getOffsetQuery(1096-1105)getOrderQueries(1111-1117)getVectorQueries(1163-1166)getFilterQueries(1154-1157)getAs(289-292)getValue(269-272)getMethod(244-247)parseQuery(502-531)
tests/e2e/Adapter/Scopes/RelationshipTests.php (2)
src/Database/Query.php (2)
Query(8-1578)select(743-746)src/Database/Database.php (1)
getDocument(4275-4414)
tests/e2e/Adapter/Scopes/Relationships/ManyToOneTests.php (1)
src/Database/Query.php (2)
Query(8-1578)select(743-746)
src/Database/Validator/AsQuery.php (1)
src/Database/Validator/Queries/V2.php (2)
getDescription(130-133)isValid(534-790)
🪛 PHPMD (2.15.0)
tests/e2e/Adapter/Scopes/JoinsTests.php
62-62: Avoid unused local variables such as '$sessionNoPermissions'. (undefined)
(UnusedLocalVariable)
102-102: Avoid unused local variables such as '$session3'. (undefined)
(UnusedLocalVariable)
src/Database/Validator/Queries/V2.php
292-292: Avoid unused local variables such as '$filters'. (undefined)
(UnusedLocalVariable)
src/Database/Adapter/Mongo.php
1885-1885: Avoid unused parameters such as '$joins'. (undefined)
(UnusedFormalParameter)
1886-1886: Avoid unused parameters such as '$vectors'. (undefined)
(UnusedFormalParameter)
src/Database/Query.php
743-743: Avoid unused parameters such as '$function'. (undefined)
(UnusedFormalParameter)
1186-1186: Avoid unused local variables such as '$joins'. (undefined)
(UnusedLocalVariable)
src/Database/Database.php
8259-8259: Avoid unused local variables such as '$type'. (undefined)
(UnusedLocalVariable)
8801-8801: Avoid unused local variables such as '$idAdded'. (undefined)
(UnusedLocalVariable)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Setup & Build Docker Image
| public function find( | ||
| QueryContext $context, | ||
| ?int $limit = 25, | ||
| ?int $offset = null, | ||
| array $cursor = [], | ||
| string $cursorDirection = Database::CURSOR_AFTER, | ||
| string $forPermission = Database::PERMISSION_READ, | ||
| array $selects = [], | ||
| array $filters = [], | ||
| array $joins = [], | ||
| array $vectors = [], | ||
| array $orderQueries = [] | ||
| ): array { |
There was a problem hiding this comment.
🧩 Analysis chain
Guard against unsupported ORDER_RANDOM and confirm cursor logic assumptions
In find():
-
You call
$order->getOrderDirection()for every$order. As in the SQL adapter,getOrderDirection()only supports ASC/DESC and will throw if aTYPE_ORDER_RANDOMquery ever reaches this code. Since this adapter reportsgetSupportForOrderRandom(): boolasfalse, it would be safer for the higher-level layer to never pass random-order queries here, or to explicitly guard against them and throw a clearer “not supported by Mongo adapter” error. -
The cursor pagination conditions assume all
ORDER BYattributes live on the same document (which is true for Mongo, sincegetSupportForRelationships()is false and there are no SQL-style joins). Please ensureDatabase::find()does not construct multi-collection order queries for this adapter.
If you want to make the adapter more robust, you can add a defensive check to reject TYPE_ORDER_RANDOM order queries up front.
Also applies to: 1939-1989
---
🏁 Script executed:
```shell
#!/bin/bash
# Find getOrderDirection() implementation and check TYPE_ORDER_RANDOM handling
rg -n "getOrderDirection" -A 10 -B 2 --type=php
Length of output: 2689
🏁 Script executed:
#!/bin/bash
# Check TYPE_ORDER_RANDOM constant definition
rg -n "TYPE_ORDER_RANDOM" --type=phpLength of output: 1059
🏁 Script executed:
#!/bin/bash
# Check Mongo adapter getSupportForOrderRandom and getSupportForRelationships
rg -n "getSupportFor(OrderRandom|Relationships)" -A 2 --type=php src/Database/Adapter/Mongo.phpLength of output: 196
🏁 Script executed:
#!/bin/bash
# Inspect the find() method lines around 1876-1888 and 1939-1989
sed -n '1850,1900p' src/Database/Adapter/Mongo.php
sed -n '1920,2000p' src/Database/Adapter/Mongo.phpLength of output: 4225
🏁 Script executed:
#!/bin/bash
# Check if Database::find() has guards for unsupported order types
rg -n "find\(" -A 30 --type=php src/Database/Database.php | head -100Length of output: 3465
🏁 Script executed:
#!/bin/bash
# Search for ORDER_RANDOM related guards or exceptions
rg -n "ORDER_RANDOM|TYPE_ORDER_RANDOM" -B 3 -A 3 --type=phpLength of output: 6760
---
🏁 Script executed:
```shell
#!/bin/bash
# Find Database::find() public method to see if it validates order queries
sed -n '1350,1550p' src/Database/Database.php | grep -A 100 "public function find"
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for getSupportForOrderRandom usage - check if validation happens
rg -n "getSupportForOrderRandom" -B 5 -A 5 --type=phpLength of output: 3407
🏁 Script executed:
#!/bin/bash
# Check if Database validates queries against adapter capabilities
rg -n "getOrderDirection|TYPE_ORDER_RANDOM" -B 10 -A 10 --type=php src/Database/Database.php | head -80Length of output: 0
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for Database find method more broadly, looking at the entire signature and initial logic
rg -n "public function find" src/Database/Database.php -A 50 | head -120
Length of output: 4310
🏁 Script executed:
#!/bin/bash
# Look for any validation or filtering of order queries in Database class
rg -n "orderRandom|Order|ORDER_RANDOM" src/Database/Database.php -B 2 -A 2Length of output: 3052
🏁 Script executed:
#!/bin/bash
# Check if there's adapter capability checking anywhere for order operations
rg -n "getSupportFor" src/Database/Database.php -B 2 -A 2Length of output: 13506
---
🏁 Script executed:
```shell
#!/bin/bash
# Check DocumentsValidator to see if it validates TYPE_ORDER_RANDOM support
rg -n "TYPE_ORDER_RANDOM|OrderRandom|getSupportForOrderRandom" src/Database/Validator/ --type=php -B 2 -A 2
Length of output: 1111
Guard against unsupported ORDER_RANDOM in Mongo adapter's find() method
The find() method calls $order->getOrderDirection() at line 1945 without guarding against TYPE_ORDER_RANDOM. Since getOrderDirection() (Query.php:348) throws an exception for random-order queries, and the Mongo adapter reports getSupportForOrderRandom() === false, an orderRandom() query reaching this code will crash with "Invalid method: Get order direction on "orderRandom" Query".
The SQL adapter handles this at line 3020 with an explicit check. Add similar protection in Mongo.find() before calling getOrderDirection(), or ensure the higher-level Database::find() validates order query types against adapter capabilities before delegation.
🧰 Tools
🪛 PHPMD (2.15.0)
1885-1885: Avoid unused parameters such as '$joins'. (undefined)
(UnusedFormalParameter)
1886-1886: Avoid unused parameters such as '$vectors'. (undefined)
(UnusedFormalParameter)
🤖 Prompt for AI Agents
In src/Database/Adapter/Mongo.php around lines 1876–1888 (and the
order-processing block near line ~1945), the code calls
$order->getOrderDirection() without guarding against TYPE_ORDER_RANDOM even
though this adapter reports getSupportForOrderRandom() === false; add a guard
before calling getOrderDirection(): check the order query type (e.g.,
$order->getType()) and if it equals TYPE_ORDER_RANDOM and the adapter does not
support random ordering, throw a clear, descriptive exception (or otherwise
reject/skip the order) similar to the SQL adapter’s explicit check; ensure no
call to getOrderDirection() occurs for random-order queries (or add a capability
check earlier in Database::find() if preferred) so random orders cannot reach
code paths that will raise "Invalid method: Get order direction on 'orderRandom'
Query".
| $type = $attribute['type'] ?? ''; | ||
| $array = $attribute['array'] ?? false; | ||
| $filters = $attribute['filters'] ?? []; | ||
|
|
There was a problem hiding this comment.
Clean up unused locals reported by static analysis
Two locals are currently unused and can be safely removed:
$typeindecode()(Line 8259) is assigned but never referenced.$idAddedinprocessRelationshipQueries()(Line 8801) is captured fromQuery::addSelect()but never used.
These are harmless but noisy and may confuse future maintainers. Suggested minimal cleanups:
- $type = $attribute['type'] ?? '';
- $array = $attribute['array'] ?? false;
+ $array = $attribute['array'] ?? false;
$filters = $attribute['filters'] ?? [];- 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));
+ }Also applies to: 8800-8803
🧰 Tools
🪛 PHPMD (2.15.0)
8259-8259: Avoid unused local variables such as '$type'. (undefined)
(UnusedLocalVariable)
🤖 Prompt for AI Agents
In src/Database/Database.php around lines 8259-8262 and 8800-8803, two local
variables are assigned but never used: $type in decode() (line ~8259) and
$idAdded from Query::addSelect() in processRelationshipQueries() (around line
~8801). Remove the unused $type assignment (just read the needed attributes
directly or omit the assignment) and change the addSelect call to not capture
its return value (call $query->addSelect(...) without assigning to $idAdded) so
there are no unused locals left.
| $context = new QueryContext(); | ||
| $context->add($collection); | ||
|
|
||
| $result = $database->decode($context, $document); | ||
|
|
There was a problem hiding this comment.
Decode likely should operate on the encoded $result, not the original $document
Here you build a QueryContext and then call:
$context = new QueryContext();
$context->add($collection);
$result = $database->decode($context, $document);Given the prior $result = $database->encode($collection, $document);, it seems more consistent (and a better end‑to‑end test of the encode/decode pipeline) to decode the encoded $result rather than the original $document.
- $context = new QueryContext();
- $context->add($collection);
-
- $result = $database->decode($context, $document);
+ $context = new QueryContext();
+ $context->add($collection);
+
+ $result = $database->decode($context, $result);If this was intentional (to test decode independently of encode), a short inline comment would help clarify the intent.
🤖 Prompt for AI Agents
In tests/e2e/Adapter/Scopes/DocumentTests.php around lines 4193-4197, the test
builds a QueryContext then calls decode using the original $document; change it
to decode the encoded output instead so the test exercises the encode→decode
pipeline: after you call $database->encode($collection, $document) use that
encoded variable as the input to $database->decode($context, ...). If the
current use of the raw $document was intentional, add a short inline comment
explaining that decode is being tested independently.
| { | ||
| $this->validator = new Order( | ||
| attributes: [ | ||
| new Document([ | ||
| '$id' => 'attr', | ||
| 'key' => 'attr', | ||
| 'type' => Database::VAR_STRING, | ||
| 'array' => false, | ||
| ]), | ||
| new Document([ | ||
| '$id' => '$sequence', | ||
| 'key' => '$sequence', | ||
| 'type' => Database::VAR_STRING, | ||
| 'array' => false, | ||
| ]), | ||
| ], | ||
| ); | ||
| $collection = new Document([ | ||
| '$id' => Database::METADATA, | ||
| '$collection' => Database::METADATA, | ||
| 'name' => 'movies', | ||
| 'attributes' => [], | ||
| 'indexes' => [], | ||
| ]); | ||
|
|
||
| $collection->setAttribute('attributes', [ | ||
| new Document([ | ||
| '$id' => 'attr', | ||
| 'key' => 'attr', | ||
| 'type' => Database::VAR_STRING, | ||
| 'array' => false, | ||
| ]), | ||
| new Document([ | ||
| '$id' => '$sequence', | ||
| 'key' => '$sequence', | ||
| 'type' => Database::VAR_STRING, | ||
| 'array' => false, | ||
| ]), | ||
| ]); | ||
|
|
||
| $context = new QueryContext(); | ||
|
|
||
| $context->add($collection); | ||
|
|
||
| $this->validator = new DocumentsValidator($context); | ||
| } | ||
|
|
||
| public function testValueSuccess(): void | ||
| { | ||
| $this->assertTrue($this->validator->isValid(Query::orderAsc('attr'))); | ||
| $this->assertTrue($this->validator->isValid(Query::orderAsc())); | ||
| $this->assertTrue($this->validator->isValid(Query::orderDesc('attr'))); | ||
| $this->assertTrue($this->validator->isValid(Query::orderDesc())); | ||
| $this->assertTrue($this->validator->isValid([Query::orderAsc('attr')])); | ||
| $this->assertTrue($this->validator->isValid([Query::orderAsc()])); | ||
| $this->assertTrue($this->validator->isValid([Query::orderDesc('attr')])); | ||
| $this->assertTrue($this->validator->isValid([Query::orderDesc()])); | ||
| $this->assertTrue($this->validator->isValid([Query::limit(101)])); | ||
| $this->assertTrue($this->validator->isValid([Query::offset(5001)])); | ||
| $this->assertTrue($this->validator->isValid([Query::equal('attr', ['v'])])); |
There was a problem hiding this comment.
DocumentsValidator instantiated without required idAttributeType argument
DocumentsValidator (V2)’s constructor takes QueryContext $context, string $idAttributeType, ..., but here it’s called with only the context. This will raise an ArgumentCountError and prevent the tests from running.
Recommend passing the ID attribute type explicitly (string IDs in this metadata collection):
- $context->add($collection);
-
- $this->validator = new DocumentsValidator($context);
+ $context->add($collection);
+
+ $this->validator = new DocumentsValidator(
+ $context,
+ idAttributeType: 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 Order( | |
| attributes: [ | |
| new Document([ | |
| '$id' => 'attr', | |
| 'key' => 'attr', | |
| 'type' => Database::VAR_STRING, | |
| 'array' => false, | |
| ]), | |
| new Document([ | |
| '$id' => '$sequence', | |
| 'key' => '$sequence', | |
| 'type' => Database::VAR_STRING, | |
| 'array' => false, | |
| ]), | |
| ], | |
| ); | |
| $collection = new Document([ | |
| '$id' => Database::METADATA, | |
| '$collection' => Database::METADATA, | |
| 'name' => 'movies', | |
| 'attributes' => [], | |
| 'indexes' => [], | |
| ]); | |
| $collection->setAttribute('attributes', [ | |
| new Document([ | |
| '$id' => 'attr', | |
| 'key' => 'attr', | |
| 'type' => Database::VAR_STRING, | |
| 'array' => false, | |
| ]), | |
| new Document([ | |
| '$id' => '$sequence', | |
| 'key' => '$sequence', | |
| 'type' => Database::VAR_STRING, | |
| 'array' => false, | |
| ]), | |
| ]); | |
| $context = new QueryContext(); | |
| $context->add($collection); | |
| $this->validator = new DocumentsValidator($context); | |
| } | |
| public function testValueSuccess(): void | |
| { | |
| $this->assertTrue($this->validator->isValid(Query::orderAsc('attr'))); | |
| $this->assertTrue($this->validator->isValid(Query::orderAsc())); | |
| $this->assertTrue($this->validator->isValid(Query::orderDesc('attr'))); | |
| $this->assertTrue($this->validator->isValid(Query::orderDesc())); | |
| $this->assertTrue($this->validator->isValid([Query::orderAsc('attr')])); | |
| $this->assertTrue($this->validator->isValid([Query::orderAsc()])); | |
| $this->assertTrue($this->validator->isValid([Query::orderDesc('attr')])); | |
| $this->assertTrue($this->validator->isValid([Query::orderDesc()])); | |
| $this->assertTrue($this->validator->isValid([Query::limit(101)])); | |
| $this->assertTrue($this->validator->isValid([Query::offset(5001)])); | |
| $this->assertTrue($this->validator->isValid([Query::equal('attr', ['v'])])); | |
| { | |
| $collection = new Document([ | |
| '$id' => Database::METADATA, | |
| '$collection' => Database::METADATA, | |
| 'name' => 'movies', | |
| 'attributes' => [], | |
| 'indexes' => [], | |
| ]); | |
| $collection->setAttribute('attributes', [ | |
| new Document([ | |
| '$id' => 'attr', | |
| 'key' => 'attr', | |
| 'type' => Database::VAR_STRING, | |
| 'array' => false, | |
| ]), | |
| new Document([ | |
| '$id' => '$sequence', | |
| 'key' => '$sequence', | |
| 'type' => Database::VAR_STRING, | |
| 'array' => false, | |
| ]), | |
| ]); | |
| $context = new QueryContext(); | |
| $context->add($collection); | |
| $this->validator = new DocumentsValidator( | |
| $context, | |
| idAttributeType: Database::VAR_STRING, | |
| ); | |
| } | |
| public function testValueSuccess(): void | |
| { | |
| $this->assertTrue($this->validator->isValid([Query::orderAsc('attr')])); | |
| $this->assertTrue($this->validator->isValid([Query::orderAsc()])); | |
| $this->assertTrue($this->validator->isValid([Query::orderDesc('attr')])); | |
| $this->assertTrue($this->validator->isValid([Query::orderDesc()])); | |
| $this->assertTrue($this->validator->isValid([Query::limit(101)])); | |
| $this->assertTrue($this->validator->isValid([Query::offset(5001)])); | |
| $this->assertTrue($this->validator->isValid([Query::equal('attr', ['v'])])); |
🤖 Prompt for AI Agents
In tests/unit/Validator/Query/OrderTest.php around lines 21 to 60, the
DocumentsValidator is being constructed with only the QueryContext but the V2
constructor requires a second parameter idAttributeType; update the
instantiation to pass the ID attribute type for this metadata collection (use
Database::VAR_STRING) as the second argument so the validator is created with
both the context and the correct id attribute type.
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 (4)
src/Database/Database.php (2)
4295-4414: Cached getDocument responses can expose system-added$permissionswhile uncached responses hide themIn
getDocument(), when$permissionsis auto-added to$selectsviaQuery::addSelect(...), the non‑cached path strips$permissionsagain using$permissionsAddedbefore returning the document. However, the cached path (if ($cached) { ... return $document; }) never performs this cleanup, and the document is cached before$permissionsis removed. This means:
- First uncached read: client sees the document without
$permissions(if it was system-added).- Subsequent cached read: client sees the same document with
$permissionspresent.That’s an observable inconsistency and may leak internal/system-added fields depending on how callers use selects.
To keep behaviour consistent across cached and non‑cached paths, strip
$permissionsafter the auth check in the cached branch as well, mirroring the logic used later on the cold path. For example:if ($cached) { $document = $this->createDocumentInstance($collection->getId(), $cached); if ($collection->getId() !== self::METADATA) { if (!$this->authorization->isValid(new Input(self::PERMISSION_READ, [ ...$collection->getRead(), ...($documentSecurity ? $document->getRead() : []) ]))) { return $this->createDocumentInstance($collection->getId(), []); } } + + if ($permissionsAdded) { + $document->removeAttribute('$permissions'); + } $this->trigger(self::EVENT_DOCUMENT_READ, $document); return $document; }
8827-8974: Many-to-many nested relationship filters are broken due to relationship population being skippedThe review comment is correct. The code has a critical inconsistency:
In
convertRelationshipFiltersToSubqueries()(lines 9122-9127), for many-to-many relationships with$needsParentResolution, the code callsfind()withoutskipRelationships(), allowing relationship population to run and populate thetwoWayKeyattribute.In
processNestedRelationshipPath()(line 8920), for the same many-to-many case with$needsReverseLookup, the code callsfind()withskipRelationships(), which preventspopulateDocumentsRelationships()from running. SinceresolveRelationshipsis forced tofalse, explicitQuery::select()alone cannot populate many-to-many relationships via junction tables. This causes$doc->getAttribute($link['twoWayKey'])to return empty, making parent ID extraction fail and the entire nested filter chain returnnull.The proposed fix is correct: conditional application of
skipRelationships()to allow relationship population for many-to-many while maintaining optimization for one-to-many/many-to-one paths. This aligns the nested behavior with the working simple-case pattern.Any nested filter crossing a many-to-many relationship (e.g.,
tags.category.name) is currently non-functional.src/Database/Query.php (2)
502-543:collectionandsystemfields are not serializedWhile
parseQuery()now handlesalias,aliasRight,attributeRight, andas, it doesn't readcollectionorsystemfrom the query array. Similarly,toArray()(lines 567-606) doesn't emit these fields. This means join queries with collection metadata cannot survive JSON serialization round-trips.If joins are expected to be serialized (e.g., for caching, logging, or HTTP transport), extend both methods to include these fields.
Apply this diff to
toArray():if (!empty($this->as)) { $array['as'] = $this->as; } + + if (!empty($this->collection)) { + $array['collection'] = $this->collection; + } + + if ($this->system) { + $array['system'] = $this->system; + }And update
parseQuery():$alias = $query['alias'] ?? ''; $aliasRight = $query['aliasRight'] ?? ''; $as = $query['as'] ?? ''; + $collection = $query['collection'] ?? ''; + $system = $query['system'] ?? false;return new self( $method, $attribute, $values, alias: $alias, attributeRight: $attributeRight, aliasRight: $aliasRight, as: $as, + collection: $collection, + system: $system, );
1211-1294: Unused variable$joinsingroupByType()Line 1214 declares
$joins = []but never populates or uses it. This is likely leftover from refactoring.Apply this diff:
$filters = []; - $joins = []; $selections = [];
♻️ Duplicate comments (8)
tests/e2e/Adapter/Scopes/DocumentTests.php (1)
4229-4233: Decode still uses the original $document instead of the encoded resultHere you build the
QueryContextand then calldecodeon the original$documentrather than the encoded$resultfromencode. That means this section validatesdecodein isolation instead of exercising the full encode → decode round-trip.If the goal is an end-to-end pipeline test, consider decoding the encoded result instead:
- $context = new QueryContext(); - $context->add($collection); - - $result = $database->decode($context, $document); + $context = new QueryContext(); + $context->add($collection); + + $result = $database->decode($context, $result);If you intentionally want to test
decodeindependently ofencode, a brief inline comment stating that intent would help future readers understand why the pre-encoded document is used here.src/Database/Validator/Queries/V2.php (2)
292-292: Fix Sequence validator logic and remove unused variable.Three issues in
validateValues():
- Line 311 (Critical bug): The Sequence validator compares the entire
$attributearray to the string'$sequence', which always evaluates to false. The primary flag is never set correctly. Compare$attributeIdinstead:- $validator = new Sequence($this->idAttributeType, $attribute === '$sequence'); + $validator = new Sequence($this->idAttributeType, $attributeId === '$sequence');
- Line 292: The
$filtersvariable is assigned but never used in this method. It's only referenced in the commented-out code at lines 428-430. Remove it:$array = $attribute['array'] ?? false; - $filters = $attribute['filters'] ?? []; $size = $attribute['size'] ?? 0;
- Lines 428-430: This commented encryption check is redundant because
validateAttributeExist()already enforces it at lines 209-211. Remove the commented code.Based on past review comments.
Also applies to: 311-311, 428-430
779-785: Remove debug var_dump() calls from validator.The
var_dump()calls at lines 781-782 will leak internal error details to logs or API responses whenever validation fails. You already store the message in$this->message.} catch (\Throwable $e) { $this->message = $e->getMessage(); - var_dump($this->message); - var_dump($e); - return false; }Based on past review comments.
src/Database/Database.php (2)
8205-8289: Remove unused$typelocal indecode()Inside
decode(QueryContext $context, Document $document, array $selects = []),$typeis read from$attributebut never used, which is exactly what PHPMD is reporting:$type = $attribute['type'] ?? ''; // unused $array = $attribute['array'] ?? false; $filters = $attribute['filters'] ?? [];You can safely drop the
$typeassignment:- $type = $attribute['type'] ?? ''; - $array = $attribute['array'] ?? false; - $filters = $attribute['filters'] ?? []; + $array = $attribute['array'] ?? false; + $filters = $attribute['filters'] ?? [];
8804-8807: Clean up unused$idAddedfromprocessRelationshipQueries()
processRelationshipQueries()callsQuery::addSelect()but never uses the$idAddedflag, matching the static-analysis warning:if (\count($queries) > 0) { [$queries, $idAdded] = Query::addSelect($queries, Query::select('$id', system: true)); }Since the boolean is unused, just capture the updated query list:
- 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)); + }This keeps behaviour identical while removing noise.
src/Database/Query.php (3)
44-76: Join and relation-equal types remain absent fromTYPESandisMethod()The join constants (
TYPE_INNER_JOIN,TYPE_LEFT_JOIN,TYPE_RIGHT_JOIN) andTYPE_RELATION_EQUALare still not included in theTYPESarray (lines 78-122) or recognized byisMethod()(lines 391-439). This prevents serialization/deserialization viaparse()/parseQuery()and breaks validation for any consumers relying on these methods.
391-439:isMethod()still missing join and relation-equal typesThe
isMethod()validation excludesTYPE_RELATION_EQUAL,TYPE_INNER_JOIN,TYPE_LEFT_JOIN, andTYPE_RIGHT_JOIN, causingparseQuery()(line 516) to reject these methods. This prevents serialization round-trips for join queries.
1566-1605:addSelect()silently drops the first select queryWhen
$queriescontains no existingTYPE_SELECT,$foundremainsfalse, causing the method to return without appending the new select (line 1604). This contradicts the expected behavior of a method namedaddSelect.Apply this diff to fix:
- if ($found && $merge) { + if ($merge) { $queries = [ ...$queries, $query ]; return [$queries, true]; } return [$queries, false];
🧹 Nitpick comments (9)
tests/e2e/Adapter/Scopes/DocumentTests.php (2)
3147-3164: Select in testOrNested nicely exercises projection; optional stronger assertionAdding
Query::select('director')ensures this nested OR test now also covers interaction between projections and compound conditions, and theassertArrayNotHasKey('name', $documents[0]);checks that non-selected fields are dropped.You might optionally add an assertion that
'director'is present (e.g.assertArrayHasKey('director', $documents[0]);) to fully validate the projection outcome.
3743-3879: testFindSelect comprehensively covers per-field selects for internal attributesThe refactored
testFindSelectnow uses multipleQuery::select('...')calls instead of array-based selects and systematically checks combinations of user attributes with$id,$sequence,$collection,$createdAt,$updatedAt, and$permissions. This gives strong coverage of which internal fields appear by default and which require explicit selection under the new decode logic.The mix of
$database->find(...)andstatic::getDatabase()->find(...)is slightly stylistically inconsistent but functionally equivalent in this trait; you can standardize on one style later if desired.src/Database/Validator/Queries/V2.php (4)
72-80: Remove commented-out code.This commented validator list serves no purpose and should be removed to improve code clarity.
- // $validators = [ - // new Limit(), - // new Offset(), - // new Cursor(), - // new Filter($collections), - // new Order($collections), - // new Select($collections), - // new Join($collections), - // ]; -
454-469: Clean up commented code in validateSelect.Two blocks of commented code should be addressed:
- Line 455: Empty
//return;serves no purpose- Lines 460-469: Large commented block for handling dotted attributes
Either remove these entirely if the logic is no longer needed, or implement them if they're required for nested attribute validation.
if (\in_array($attribute, $internalKeys)) { - //return; } $alias = $query->getAlias(); - // if (\str_contains($attribute, '.')) { - // try { - // // Handle attributes containing dots (e.g., relationships or special symbols) - // $this->validateAttributeExist($attribute, $alias); - // } catch (\Throwable $e) { - // // For relationships, validate only the top-level attribute - // $attribute = \explode('.', $attribute)[0]; - // $this->validateAttributeExist($attribute, $alias); - // } - // } - $this->validateAttributeExist($attribute, $alias);
757-766: Remove obsolete commented code.This commented block appears to be an older approach to vector query validation that's no longer needed, as the validation is now handled earlier in the switch statement (lines 752-773).
$this->validateAttributeExist($query->getAttribute(), $query->getAlias()); - // Handle dotted attributes (relationships) - // $attributeKey = $attribute; - // if (\str_contains($attributeKey, '.') && !isset($this->schema[$attributeKey])) { - // $attributeKey = \explode('.', $attributeKey)[0]; - // } - // - // $attributeSchema = $this->schema[$attributeKey]; - // if ($attributeSchema['type'] !== Database::VAR_VECTOR) { - // throw new \Exception('Vector queries can only be used on vector attributes'); - // } - if (count($query->getValues()) != 1) {
670-672: Track TODO for alias scope validation.This TODO highlights missing validation: ensuring that aliases used in queries are actually available in the context scope, not just that they exist.
Do you want me to open an issue to track this validation enhancement, or is it planned for a follow-up PR?
src/Database/Database.php (1)
8487-8536:validateSelections()mixesQueryobjects with string keys
validateSelections()was updated to acceptarray<Query>but still does:$selections = []; ... if ($query->getMethod() == Query::TYPE_SELECT) { ... $selections[] = $query; } ... if ($this->adapter->getSupportForAttributes()) { $invalid = \array_diff($selections, $keys); if (!empty($invalid) && !\in_array('*', $invalid)) { throw new QueryException('Cannot select attributes: ' . \implode(', ', $invalid)); } }Here
$selectionsis an array ofQueryobjects, while$keysis an array of strings, so:
array_diff()will always return allQueryobjects as “invalid”.implode(', ', $invalid)on objects will trigger a runtime error if this code path is ever executed.If you plan to use this helper with
Queryobjects, extract attribute names first:- $selections = []; + $selections = []; + $selectionAttributes = []; ... if ($query->getMethod() == Query::TYPE_SELECT) { if (\str_contains($query->getValue(), '.')) { $relationshipSelections[] = $query; continue; } $selections[] = $query; + $selectionAttributes[] = $query->getValue(); } ... if ($this->adapter->getSupportForAttributes()) { - $invalid = \array_diff($selections, $keys); + $invalid = \array_diff($selectionAttributes, $keys); if (!empty($invalid) && !\in_array('*', $invalid)) { throw new QueryException('Cannot select attributes: ' . \implode(', ', $invalid)); } }(or equivalently, use
getAttribute()depending on the intended semantics).src/Database/Query.php (2)
335-359: Use specific exception types instead of generic\ExceptionBoth
getCursorDirection()andgetOrderDirection()throw generic\Exception. Consider usingQueryException(which already exists in this namespace) for consistency with other query-related errors likeparse()andparseQuery().Apply this diff:
- 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');
1049-1194: Helper methods are well-designed, butgetCursorDocument()should be staticThe new static helper methods (
getSelectQueries(),getJoinQueries(),getLimitQuery(), etc.) provide a clean API for filtering queries by type. However,getCursorDocument()(line 1169) is an instance method that doesn't use$this, and its signature suggests it should be static for consistency with the other helpers.Apply this diff:
- public function getCursorDocument(?Query $query): Document + public static function getCursorDocument(?Query $query): Document
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/Database/Database.php(60 hunks)src/Database/Query.php(27 hunks)src/Database/Validator/Queries/V2.php(1 hunks)tests/e2e/Adapter/Base.php(2 hunks)tests/e2e/Adapter/Scopes/DocumentTests.php(17 hunks)tests/unit/Validator/DocumentQueriesTest.php(4 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-10-03T02:04:17.803Z
Learnt from: abnegate
Repo: utopia-php/database PR: 721
File: tests/e2e/Adapter/Scopes/DocumentTests.php:6418-6439
Timestamp: 2025-10-03T02:04:17.803Z
Learning: In tests/e2e/Adapter/Scopes/DocumentTests::testSchemalessDocumentInvalidInteralAttributeValidation (PHP), when the adapter reports getSupportForAttributes() === false (schemaless), the test should not expect exceptions from createDocuments for “invalid” internal attributes; remove try/catch and ensure the test passes without exceptions, keeping at least one assertion.
Applied to files:
tests/unit/Validator/DocumentQueriesTest.phptests/e2e/Adapter/Base.phpsrc/Database/Validator/Queries/V2.phptests/e2e/Adapter/Scopes/DocumentTests.php
📚 Learning: 2025-07-30T19:17:53.630Z
Learnt from: ArnabChatterjee20k
Repo: utopia-php/database PR: 642
File: src/Database/Validator/PartialStructure.php:43-52
Timestamp: 2025-07-30T19:17:53.630Z
Learning: In PartialStructure validator, when filtering for required attributes validation using the $requiredAttributes parameter, $this->attributes should be used instead of the merged $attributes array because this validation is specifically for internal attributes like $createdAt and $updatedAt that are defined in the base Structure class, not collection-specific attributes.
Applied to files:
src/Database/Validator/Queries/V2.phpsrc/Database/Database.php
📚 Learning: 2025-10-29T12:27:57.071Z
Learnt from: ArnabChatterjee20k
Repo: utopia-php/database PR: 747
File: src/Database/Adapter/Mongo.php:1449-1453
Timestamp: 2025-10-29T12:27:57.071Z
Learning: In src/Database/Adapter/Mongo.php, when getSupportForAttributes() returns false (schemaless mode), the updateDocument method intentionally uses a raw document without $set operator for replacement-style updates, as confirmed by the repository maintainer ArnabChatterjee20k.
Applied to files:
tests/e2e/Adapter/Scopes/DocumentTests.php
📚 Learning: 2025-08-14T06:35:30.429Z
Learnt from: ArnabChatterjee20k
Repo: utopia-php/database PR: 661
File: tests/e2e/Adapter/Scopes/SpatialTests.php:180-186
Timestamp: 2025-08-14T06:35:30.429Z
Learning: Query::distance method in Utopia\Database\Query expects an array of values parameter, where each value is a [geometry, distance] pair. So the correct format is Query::distance('attribute', [[[lat, lng], distance]]) where the outer array contains the values and each value is [geometry, distance].
Applied to files:
src/Database/Query.php
📚 Learning: 2025-10-20T05:29:29.487Z
Learnt from: abnegate
Repo: utopia-php/database PR: 731
File: src/Database/Database.php:6987-6988
Timestamp: 2025-10-20T05:29:29.487Z
Learning: In Utopia\Database\Database::convertRelationshipQueries, Many-to-Many filtering does not need the parent collection or a direct junction query: it calls find() on the related collection without skipping relationships, which populates relationship attributes (including the two-way key). Parent IDs are derived from that populated attribute. Therefore, calls should remain convertRelationshipQueries($relationships, $queries).
Applied to files:
src/Database/Database.php
🧬 Code graph analysis (6)
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-789)isValid(534-788)src/Database/Query.php (3)
Query(8-1606)select(771-774)limit(814-817)
tests/e2e/Adapter/Base.php (1)
src/Database/Adapter.php (1)
Adapter(17-1456)
src/Database/Validator/Queries/V2.php (11)
src/Database/Database.php (2)
Database(38-9432)__construct(447-676)src/Database/Document.php (3)
Document(12-470)getId(63-66)getArrayCopy(423-458)src/Database/Query.php (14)
Query(8-1606)__construct(195-230)getAttribute(252-255)isNested(1301-1308)getAlias(274-277)getMethod(244-247)getRightAlias(279-282)getFilterQueries(1182-1185)getValues(260-263)isSpatialQuery(445-462)isVectorQuery(468-471)getAs(289-292)parse(480-493)getAttributeRight(284-287)src/Database/QueryContext.php (4)
QueryContext(8-89)__construct(25-27)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 (6)
AsQuery(7-84)__construct(14-18)getDescription(24-27)isArray(68-71)getType(80-83)isValid(35-59)src/Database/Validator/Datetime.php (1)
Datetime(7-152)src/Database/Validator/Query/Cursor.php (1)
Cursor(10-56)src/Database/Validator/Query/Limit.php (1)
Limit(9-63)src/Database/Validator/Query/Offset.php (3)
Offset(9-59)__construct(16-19)isValid(25-53)src/Database/Validator/Sequence.php (1)
Sequence(9-60)
tests/e2e/Adapter/Scopes/DocumentTests.php (3)
src/Database/Database.php (3)
Database(38-9432)getDocument(4275-4418)decode(8205-8289)src/Database/QueryContext.php (2)
QueryContext(8-89)add(60-68)src/Database/Query.php (2)
Query(8-1606)select(771-774)
src/Database/Query.php (2)
src/Database/Database.php (3)
__construct(447-676)getCollection(1812-1832)Database(38-9432)src/Database/QueryContext.php (1)
__construct(25-27)
src/Database/Database.php (8)
src/Database/Validator/Queries/V2.php (1)
V2(23-789)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/Query.php (18)
Query(8-1606)getSelectQueries(1053-1058)addSelect(1572-1605)select(771-774)getAttribute(252-255)getJoinQueries(1064-1071)join(986-989)getCollection(294-297)limit(814-817)getLimitQuery(1093-1102)offset(825-828)getOffsetQuery(1124-1133)getOrderQueries(1139-1145)getVectorQueries(1191-1194)getFilterQueries(1182-1185)setAttribute(318-323)getValue(269-272)parseQuery(502-543)src/Database/Adapter.php (9)
getIdAttributeType(936-936)getSupportForAttributes(960-960)getMinDateTime(929-929)getMaxDateTime(943-946)getMaxUIDLength(922-922)getDocument(715-715)filter(1259-1268)find(829-841)getSupportForCasting(1017-1017)src/Database/Adapter/Pool.php (7)
getIdAttributeType(542-545)getSupportForAttributes(347-350)getMinDateTime(337-340)getMaxUIDLength(332-335)getDocument(231-234)find(271-285)getSupportForCasting(387-390)src/Database/Adapter/Mongo.php (7)
getIdAttributeType(3162-3165)getSupportForAttributes(2698-2701)getMinDateTime(2642-2645)getMaxUIDLength(3178-3181)getDocument(1113-1158)find(1876-2066)getSupportForCasting(2925-2928)src/Database/Adapter/SQL.php (6)
getIdAttributeType(2221-2224)getSupportForAttributes(949-952)getMaxUIDLength(2240-2243)getDocument(365-430)find(2983-3204)getSupportForCasting(1499-1502)src/Database/Document.php (9)
getAttribute(224-231)getId(63-66)removeAttribute(287-293)Document(12-470)getCollection(85-88)getPermissionsByType(145-157)find(304-322)getArrayCopy(423-458)setAttribute(244-261)
🪛 PHPMD (2.15.0)
tests/unit/Validator/DocumentQueriesTest.php
88-88: Avoid unused local variables such as '$validator'. (undefined)
(UnusedLocalVariable)
93-93: Avoid unused local variables such as '$queries'. (undefined)
(UnusedLocalVariable)
src/Database/Validator/Queries/V2.php
292-292: Avoid unused local variables such as '$filters'. (undefined)
(UnusedLocalVariable)
src/Database/Query.php
1214-1214: Avoid unused local variables such as '$joins'. (undefined)
(UnusedLocalVariable)
src/Database/Database.php
8263-8263: Avoid unused local variables such as '$type'. (undefined)
(UnusedLocalVariable)
8805-8805: Avoid unused local variables such as '$idAdded'. (undefined)
(UnusedLocalVariable)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Adapter Tests (SharedTables/SQLite)
- GitHub Check: Adapter Tests (SharedTables/Postgres)
- GitHub Check: Adapter Tests (Schemaless/MongoDB)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (SharedTables/MongoDB)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (MariaDB)
- GitHub Check: Adapter Tests (MySQL)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Adapter Tests (Postgres)
- GitHub Check: Adapter Tests (MongoDB)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Unit Test
🔇 Additional comments (13)
tests/e2e/Adapter/Base.php (2)
12-12: LGTM!The import statement follows the existing pattern and is properly placed alphabetically among other test trait imports.
39-39: JoinsTests trait properly checks adapter support—no changes needed.Verification confirms that JoinsTests correctly checks adapter capability using
getSupportForRelationships()at the beginning oftestJoin(). When joins are unsupported, the test skips assertions and returns early. The trait usage in Base.php line 39 is safe and follows the established pattern.tests/e2e/Adapter/Scopes/DocumentTests.php (6)
22-22: QueryContext import is appropriateBringing in
Utopia\Database\QueryContexthere is necessary for the new context-awaredecodeusage later in the file and matches the updated Database API.
1397-1433: Per-field selects and internal-field expectations for getDocument look correctUsing individual
Query::select('string')/Query::select('integer_signed')and then asserting that only those user attributes plus$id/$collectionare present (while$sequence,$createdAt,$updatedAt,$permissionsstay hidden) lines up with the newgetDocumentcontract and gives good coverage of select behavior for single-document reads.
1437-1470: testGetDocumentOnlySelectQueries correctly enforces the new getDocument constraintThis test verifies that passing non-select queries (
equal,limit) intogetDocumentnow throws aDatabaseExceptionwith the messageOnly Select queries are permitted, matching the updated guard inDatabase::getDocument. The assertions on message and exception type are tight and appropriate.
4975-4982: Including select queries in deleteDocuments pagination tests aligns with new projection rulesIn
testDeleteBulkDocuments, addingQuery::select('$createdAt')together with...array_map(fn ($f) => Query::select($f), $selects)ensures that the documents participating in cursor-based pagination and deletion carry the same internal fields ($sequence,$id,$collection,$permissions,$updatedAt,$createdAt) that the logic relies on. This keeps the bulk-delete pagination tests robust under the new select-aware decode behavior.
5184-5185: Callback-based bulk delete tests correctly mirror select usage from the non-callback pathIn
testDeleteBulkDocumentsWithCallbackSupport, reusing the samearray_map(fn ($f) => Query::select($f), $selects)pattern and addingQuery::select('$createdAt')in both the error-path and success-path invocations keeps the callback variants behaviorally aligned with the earlierdeleteDocumentstests. That symmetry is helpful for ensuring projections and cursor/order logic behave consistently whether or not callbacks are supplied.Also applies to: 5207-5208
5461-5461: Using Query::select('*') in the shared-tables branch keeps intent explicitFor adapters with
getSharedTables()enabled, callingfind('documents', [Query::select('*')])makes it explicit that the test wants all fields back while working around the Mongo integer UID issue noted in the comment. This keepstestEmptyTenantcompatible with the per-field select API without changing the test’s semantics.src/Database/Validator/Queries/V2.php (1)
706-733: Address the intentionally-disabled duplicate/ambiguous column validations.Three validation exceptions have been deliberately commented out since commit 735e0d3 (May 15, 2025), with corresponding test cases also disabled. While the logic to track duplications and ambiguous columns remains intact, the actual validation checks are bypassed:
- Line 711: Ambiguous column check when using
*- Line 718: Duplicate select check
- Line 730: Ambiguous column name check
These checks were intentionally disabled (not accidental), but no explanation was provided in the code. To proceed, either:
- Add documentation: Include a TODO comment or issue reference explaining why these validations must remain disabled
- Enable the checks: Uncomment the three
throwstatements and verify/fix any test failures they surfaceWithout clarification, it's unclear whether this is deferred work, a known limitation, or unfinished validation logic. Developer must explicitly resolve this ambiguity.
src/Database/Query.php (4)
628-796: LGTM: Factory method updates maintain backward compatibilityThe factory method signatures have been extended with optional
alias,as, andsystemparameters, maintaining backward compatibility through default values. The changes consistently support the new alias-aware query model.
980-1027: LGTM: Join factory methods are well-structuredThe join factory methods (
join(),innerJoin(),leftJoin(),rightJoin(),relationEqual()) correctly instantiate join queries with appropriate metadata. Note thatjoin()andinnerJoin()are functionally identical (both createTYPE_INNER_JOIN), which provides convenient API aliasing.
1313-1327: LGTM:isJoin()andisFilter()methods are correctly implementedBoth methods provide clear boolean checks for join and filter query types, consistent with the existing
isNested()pattern.
1036-1047: Review comment is incorrect—getByType()is entirely internalVerification found zero external calls to
getByType()anywhere in the codebase. The method is used only internally viaself::getByType()within Query.php by six public wrapper methods. Making an internal implementation detailprotectedis proper encapsulation, not a breaking change. No documentation is needed.Likely an incorrect or invalid review comment.
| protected const FILTER_TYPES = [ | ||
| self::TYPE_EQUAL, | ||
| self::TYPE_NOT_EQUAL, | ||
| self::TYPE_LESSER, | ||
| self::TYPE_LESSER_EQUAL, | ||
| self::TYPE_GREATER, | ||
| self::TYPE_GREATER_EQUAL, | ||
| self::TYPE_GREATER_EQUAL, | ||
| self::TYPE_CONTAINS, | ||
| self::TYPE_NOT_CONTAINS, | ||
| self::TYPE_SEARCH, | ||
| self::TYPE_NOT_SEARCH, | ||
| self::TYPE_IS_NULL, | ||
| self::TYPE_IS_NOT_NULL, | ||
| self::TYPE_BETWEEN, | ||
| self::TYPE_NOT_BETWEEN, | ||
| self::TYPE_STARTS_WITH, | ||
| self::TYPE_NOT_STARTS_WITH, | ||
| self::TYPE_ENDS_WITH, | ||
| self::TYPE_NOT_ENDS_WITH, | ||
| self::TYPE_CROSSES, | ||
| self::TYPE_NOT_CROSSES, | ||
| self::TYPE_DISTANCE_EQUAL, | ||
| self::TYPE_DISTANCE_NOT_EQUAL, | ||
| self::TYPE_DISTANCE_GREATER_THAN, | ||
| self::TYPE_DISTANCE_LESS_THAN, | ||
| self::TYPE_INTERSECTS, | ||
| self::TYPE_NOT_INTERSECTS, | ||
| self::TYPE_OVERLAPS, | ||
| self::TYPE_NOT_OVERLAPS, | ||
| self::TYPE_TOUCHES, | ||
| self::TYPE_NOT_TOUCHES, | ||
| self::TYPE_AND, | ||
| self::TYPE_OR, | ||
| self::TYPE_RELATION_EQUAL, | ||
| ]; |
There was a problem hiding this comment.
Duplicate entry in FILTER_TYPES
TYPE_GREATER_EQUAL appears twice (lines 141-142). While not harmful in PHP, it's redundant.
Apply this diff to remove the duplicate:
self::TYPE_GREATER,
self::TYPE_GREATER_EQUAL,
- self::TYPE_GREATER_EQUAL,
self::TYPE_CONTAINS,🤖 Prompt for AI Agents
In src/Database/Query.php around lines 135 to 170, the FILTER_TYPES array
contains a duplicate entry of self::TYPE_GREATER_EQUAL (appearing twice around
lines 141-142); remove the redundant second occurrence so each constant appears
only once, preserving the array formatting and trailing commas.
| public function testInvalidQueries(): void | ||
| { | ||
| $validator = new DocumentQueries($this->collection['attributes']); | ||
| $queries = [Query::limit(1)]; | ||
| $this->assertEquals(false, $validator->isValid($queries)); | ||
| $validator = new DocumentsValidator( | ||
| $this->context, | ||
| Database::VAR_INTEGER | ||
| ); | ||
|
|
||
| $queries = [ | ||
| Query::limit(1) | ||
| ]; | ||
|
|
||
| /** | ||
| * Remove this tests | ||
| * Think what to do about this? originally we had DocumentValidator which only allow select queires | ||
| * Added tests this test to check it out testGetDocumentOnlySelectQueries | ||
| */ | ||
| //$this->assertEquals(false, $validator->isValid($queries)); | ||
| } |
There was a problem hiding this comment.
Remove incomplete test or implement validation.
This test method creates a validator and queries but performs no assertions. The commented-out code and TODO indicate incomplete refactoring work.
Either:
- Complete the test with proper assertions for the new DocumentsValidator behavior, or
- Remove this test method entirely if it's no longer applicable
Leaving non-functional test methods reduces test suite clarity and maintenance.
Do you want me to help determine what validation behavior should be tested here, or would you prefer to remove this test?
🧰 Tools
🪛 PHPMD (2.15.0)
88-88: Avoid unused local variables such as '$validator'. (undefined)
(UnusedLocalVariable)
93-93: Avoid unused local variables such as '$queries'. (undefined)
(UnusedLocalVariable)
🤖 Prompt for AI Agents
In tests/unit/Validator/DocumentQueriesTest.php around lines 86 to 103, the test
method testInvalidQueries is incomplete: it constructs a DocumentsValidator and
a Query::limit(1) but contains no active assertions and has commented-out code
and a TODO. Either remove the entire test method if DocumentsValidator no longer
applies, or implement a real assertion reflecting the validator's intended
behavior (for example, if DocumentsValidator should reject non-select queries
assert false/expectation that isValid($queries) returns false, or assert true if
the new validator allows limit-only queries), and remove the inline
TODO/comment; ensure the test name and any docblock match the chosen behavior.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/Database/Adapter/SQL.php (1)
3078-3093: Derive join type from Query method instead of hardcoding INNER JOIN.Line 3088 hardcodes
INNER JOINfor all joins, ignoring the actual join type specified in the Query object. This changes the semantics of LEFT JOIN and RIGHT JOIN queries by forcing them to become INNER JOINs, which can incorrectly filter out rows that should be preserved.According to the Query class constants (
TYPE_INNER_JOIN,TYPE_LEFT_JOIN,TYPE_RIGHT_JOIN) and the past review comment that was marked as addressed, the join type should be derived from$join->getMethod().Apply this diff:
$sqlJoin = ''; foreach ($joins as $join) { $permissions = ''; $collection = $join->getCollection(); $collection = $this->filter($collection); $skipAuth = $context->skipAuth($collection, $forPermission, $this->authorization); if (! $skipAuth) { $permissions = 'AND '.$this->getSQLPermissionsCondition($collection, $roles, $join->getAlias(), $forPermission); } - $sqlJoin .= "INNER JOIN {$this->getSQLTable($collection)} AS {$this->quote($join->getAlias())} + $joinKeyword = match($join->getMethod()) { + Query::TYPE_LEFT_JOIN => 'LEFT JOIN', + Query::TYPE_RIGHT_JOIN => 'RIGHT JOIN', + default => 'INNER JOIN', + }; + + $sqlJoin .= "{$joinKeyword} {$this->getSQLTable($collection)} AS {$this->quote($join->getAlias())} ON {$this->getSQLConditions($join->getValues(), $binds)} {$permissions} {$this->getTenantQuery($collection, $join->getAlias())} "; }
🧹 Nitpick comments (1)
tests/e2e/Adapter/Scopes/Relationships/ManyToManyTests.php (1)
111-114: Use the local$databasevariable for consistency.Line 111 uses
static::getDatabase()directly, which is inconsistent with the established pattern in this method (and throughout the file) where$databaseis assigned at line 20.Apply this diff:
- $documents = static::getDatabase()->find('playlist', [ + $documents = $database->find('playlist', [ Query::select('name'), Query::limit(1) ]);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Database/Adapter/MariaDB.php(2 hunks)src/Database/Adapter/SQL.php(8 hunks)tests/e2e/Adapter/Scopes/RelationshipTests.php(24 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/Scopes/RelationshipTests.php
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-20T05:29:29.487Z
Learnt from: abnegate
Repo: utopia-php/database PR: 731
File: src/Database/Database.php:6987-6988
Timestamp: 2025-10-20T05:29:29.487Z
Learning: In Utopia\Database\Database::convertRelationshipQueries, Many-to-Many filtering does not need the parent collection or a direct junction query: it calls find() on the related collection without skipping relationships, which populates relationship attributes (including the two-way key). Parent IDs are derived from that populated attribute. Therefore, calls should remain convertRelationshipQueries($relationships, $queries).
Applied to files:
tests/e2e/Adapter/Scopes/Relationships/ManyToManyTests.php
📚 Learning: 2025-10-03T02:04:17.803Z
Learnt from: abnegate
Repo: utopia-php/database PR: 721
File: tests/e2e/Adapter/Scopes/DocumentTests.php:6418-6439
Timestamp: 2025-10-03T02:04:17.803Z
Learning: In tests/e2e/Adapter/Scopes/DocumentTests::testSchemalessDocumentInvalidInteralAttributeValidation (PHP), when the adapter reports getSupportForAttributes() === false (schemaless), the test should not expect exceptions from createDocuments for “invalid” internal attributes; remove try/catch and ensure the test passes without exceptions, keeping at least one assertion.
Applied to files:
tests/e2e/Adapter/Scopes/Relationships/ManyToManyTests.php
🧬 Code graph analysis (3)
tests/e2e/Adapter/Scopes/Relationships/ManyToManyTests.php (2)
tests/e2e/Adapter/Base.php (1)
getDatabase(52-52)src/Database/Query.php (2)
Query(8-1606)select(771-774)
src/Database/Adapter/MariaDB.php (4)
src/Database/Query.php (5)
getAttributeRight(284-287)getAttribute(252-255)getAlias(274-277)Query(8-1606)getRightAlias(279-282)src/Database/Adapter/SQL.php (1)
getInternalKeyForAttribute(2386-2398)src/Database/Adapter.php (2)
filter(1259-1268)quote(491-491)src/Database/Adapter/Postgres.php (1)
quote(2169-2172)
src/Database/Adapter/SQL.php (5)
src/Database/QueryContext.php (3)
QueryContext(8-89)getCollections(32-35)skipAuth(75-86)src/Database/Adapter.php (6)
getAttributeProjection(1250-1250)quote(491-491)filter(1259-1268)find(829-841)count(864-864)getTenantQuery(1350-1350)src/Database/Adapter/Pool.php (5)
getAttributeProjection(507-510)quote(126-129)find(271-285)count(292-295)getTenantQuery(532-535)src/Database/Adapter/Mongo.php (6)
getAttributeProjection(2564-2597)quote(3145-3148)find(1876-2066)getInternalKeyForAttribute(1839-1851)count(2138-2225)getTenantQuery(3266-3269)src/Database/Query.php (10)
Query(8-1606)select(771-774)getAttribute(252-255)getAlias(274-277)getAs(289-292)limit(814-817)offset(825-828)join(986-989)getCollection(294-297)getValues(260-263)
🪛 GitHub Actions: CodeQL
src/Database/Adapter/SQL.php
[error] 2982-2982: Method Utopia\Database\Adapter\SQL::find() has parameter $vectors with no value type specified in iterable type array.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Adapter Tests (MySQL)
🔇 Additional comments (2)
tests/e2e/Adapter/Scopes/Relationships/ManyToManyTests.php (2)
142-143: LGTM! Split projection pattern aligns with new Query API.The change from array-based projections to individual
Query::select()calls correctly reflects the new API design where each selection is specified separately.
1691-1695: LGTM! Excellent test coverage for nested many-to-many projections.The split projection pattern correctly handles multiple nesting levels (artists → albums → tracks), demonstrating that the new API properly supports deep relationship traversal with per-field selection.
| 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.
Filter the right-hand alias before quoting.
On line 1599, $aliasRight is quoted but not filtered, unlike the left alias which is both filtered (line 1554) and quoted (line 1555). This inconsistency could cause issues if the alias contains characters that filter() would reject.
Apply this diff to filter the right alias:
case Query::TYPE_RELATION_EQUAL:
$attributeRight = $this->quote($this->filter($query->getAttributeRight()));
- $aliasRight = $this->quote($query->getRightAlias());
+ $aliasRight = $this->filter($query->getRightAlias());
+ $aliasRight = $this->quote($aliasRight);
return "{$alias}.{$attribute}={$aliasRight}.{$attributeRight}";🤖 Prompt for AI Agents
In src/Database/Adapter/MariaDB.php around lines 1597 to 1601, the right-hand
alias ($aliasRight) is quoted but not passed through filter(), unlike the left
alias; update the code to call filter() on $query->getRightAlias() before
quoting (i.e., $aliasRight =
$this->quote($this->filter($query->getRightAlias()))) so both aliases are
consistently filtered then quoted.
| protected function getAttributeProjection(array $selects): string | ||
| { | ||
| if (empty($selections) || \in_array('*', $selections)) { | ||
| return "{$this->quote($prefix)}.*"; | ||
| //todo: fix this $spatialAttributes | ||
|
|
||
| if (empty($selects)) { | ||
| return Query::DEFAULT_ALIAS.'.*'; | ||
| } | ||
|
|
||
| // Handle specific selections with spatial conversion where needed | ||
| $internalKeys = [ | ||
| '$id', | ||
| '$sequence', | ||
| '$permissions', | ||
| '$createdAt', | ||
| '$updatedAt', | ||
| ]; | ||
| $string = ''; | ||
| foreach ($selects as $select) { | ||
| if ($select->getAttribute() === '$collection') { | ||
| continue; | ||
| } | ||
|
|
||
| $alias = $select->getAlias(); | ||
| $alias = $this->filter($alias); | ||
| $attribute = $select->getAttribute(); | ||
|
|
||
| $attribute = match ($attribute) { | ||
| '$id' => '_uid', | ||
| '$sequence' => '_id', | ||
| '$tenant' => '_tenant', | ||
| '$createdAt' => '_createdAt', | ||
| '$updatedAt' => '_updatedAt', | ||
| '$permissions' => '_permissions', | ||
| default => $attribute | ||
| }; | ||
|
|
||
| if ($attribute !== '*') { | ||
| $attribute = $this->filter($attribute); | ||
| $attribute = $this->quote($attribute); | ||
| } | ||
|
|
||
| $selections = \array_diff($selections, [...$internalKeys, '$collection']); | ||
| $as = $select->getAs(); | ||
|
|
||
| foreach ($internalKeys as $internalKey) { | ||
| $selections[] = $this->getInternalKeyForAttribute($internalKey); | ||
| } | ||
| if (!empty($as)) { | ||
| $as = ' as '.$this->quote($this->filter($as)); | ||
| } | ||
|
|
||
| $projections = []; | ||
| foreach ($selections as $selection) { | ||
| $filteredSelection = $this->filter($selection); | ||
| $quotedSelection = $this->quote($filteredSelection); | ||
| $projections[] = "{$this->quote($prefix)}.{$quotedSelection}"; | ||
| if (!empty($string)) { | ||
| $string .= ', '; | ||
| } | ||
|
|
||
| $string .= "{$this->quote($alias)}.{$attribute}{$as}"; | ||
| } | ||
|
|
||
| return \implode(',', $projections); | ||
| return $string; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Quote the default alias in the projection.
Line 2342 returns Query::DEFAULT_ALIAS.'.*' without quoting the alias. While DEFAULT_ALIAS is a known constant ('main'), this is inconsistent with the rest of the method where aliases are filtered and quoted (lines 2352, 2380). If the constant value ever changes or contains special characters, this could cause SQL syntax errors.
Apply this diff:
if (empty($selects)) {
- return Query::DEFAULT_ALIAS.'.*';
+ return $this->quote(Query::DEFAULT_ALIAS).'.*';
}🤖 Prompt for AI Agents
In src/Database/Adapter/SQL.php around lines 2337 to 2384, the early return uses
Query::DEFAULT_ALIAS.'.*' unquoted which is inconsistent with the rest of the
method and can break SQL if the alias changes; replace that return with a
quoted, filtered alias followed by .*, e.g. build the alias via
$this->quote($this->filter(Query::DEFAULT_ALIAS)) and return that concatenated
with '.*' so the default projection matches how other aliases are handled.
| public function find( | ||
| QueryContext $context, | ||
| ?int $limit = 25, | ||
| ?int $offset = null, | ||
| array $cursor = [], | ||
| string $cursorDirection = Database::CURSOR_AFTER, | ||
| string $forPermission = Database::PERMISSION_READ, | ||
| array $selects = [], | ||
| array $filters = [], | ||
| array $joins = [], | ||
| array $vectors = [], | ||
| array $orderQueries = [] | ||
| ): array { |
There was a problem hiding this comment.
Add missing PHPDoc type hint for $vectors parameter.
The static analysis pipeline reports that parameter $vectors on line 2992 lacks a value type specification for the iterable. Based on the method signature and usage context (vector queries are Query objects), this should be documented as array<Query>.
Add the proper PHPDoc above the method signature:
/**
* Find Documents
*
* @param QueryContext $context
* @param int|null $limit
* @param int|null $offset
* @param array<string, mixed> $cursor
* @param string $cursorDirection
* @param string $forPermission
* @param array<Query> $selects
* @param array<Query> $filters
* @param array<Query> $joins
+ * @param array<Query> $vectors
* @param array<Query> $orderQueries
* @return array<Document>
* @throws DatabaseException
* @throws TimeoutException
* @throws Exception
*/Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Actions: CodeQL
[error] 2982-2982: Method Utopia\Database\Adapter\SQL::find() has parameter $vectors with no value type specified in iterable type array.
🤖 Prompt for AI Agents
In src/Database/Adapter/SQL.php around lines 2982 to 2994, the method find is
missing a PHPDoc type hint for the $vectors parameter; add a @param annotation
for $vectors specifying it is an array of Query objects (e.g. @param
array<Query> $vectors) in the method's docblock immediately above the signature
(or add/extend the existing docblock) so static analysis recognizes the iterable
element type.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/Database/Adapter/Mongo.php (1)
1859-1887:find()integration withQueryContext, auth, projection, and cursor pagination looks solid; consider defensive guards for unsupported featuresThe refactored
find():
- Derives the root collection from
QueryContextand usesskipAuth()for permission checks, matching the adapter’s lack of relationship support.- Clones
$filtersbeforebuildFilters()so upstream queries aren’t mutated.- Applies
getAttributeProjection($selects)with the$removeSequencehack so_idbeing always returned by Mongo does not leak$sequencewhen not selected.- Mirrors SQL’s cursor pagination over
$orderQueries, including the$sequencesingle-attribute fast path and correct operator choice forCURSOR_BEFORE.- Wraps each record as
Documentand conditionally drops$sequencein both the first and subsequent batches.- Properly kills the server cursor in
finallyto avoid resource leaks.Given Mongo reports
getSupportForRelationships() === falseandgetSupportForVectors() === false,$joinsand$vectorsbeing unused here is expected. To make this more explicit (and quiet PHPMD), you might optionally:
- Add early guards like
if (!empty($joins)) { throw new DatabaseException('Joins are not supported by Mongo adapter'); }and similarly for$vectors, or- Add inline comments / suppression to indicate they are intentionally unused for this adapter.
If random ordering is still unsupported for this adapter, a defensive check on
$orderQueriesbefore calling$order->getOrderDirection()would also make behavior clearer if such a query somehow slips past higher-level validation.Also applies to: 1888-1899, 1902-1905, 1920-1935, 1938-1987, 1997-2005, 2007-2016, 2021-2042
🧹 Nitpick comments (1)
src/Database/Adapter/Mongo.php (1)
1125-1136: Projection ingetDocument()is wired correctly; ensure callers pass select-style queriesUsing
$projections = $this->getAttributeProjection($queries)and the$removeSequenceflag alignsgetDocument()with the new projection semantics: if callers explicitly project fields but omit$sequence, you strip the derived$sequencethat Mongo always returns via_id. This is consistent withfind(). Just make sure the$queriesargument is now used strictly for select-styleQueryobjects; if filter queries are ever passed here, they will unintentionally double as projections and could drop attributes from the result.Also applies to: 1153-1155
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Database/Adapter.php(2 hunks)src/Database/Adapter/Mongo.php(11 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-29T12:27:57.071Z
Learnt from: ArnabChatterjee20k
Repo: utopia-php/database PR: 747
File: src/Database/Adapter/Mongo.php:1449-1453
Timestamp: 2025-10-29T12:27:57.071Z
Learning: In src/Database/Adapter/Mongo.php, when getSupportForAttributes() returns false (schemaless mode), the updateDocument method intentionally uses a raw document without $set operator for replacement-style updates, as confirmed by the repository maintainer ArnabChatterjee20k.
Applied to files:
src/Database/Adapter/Mongo.php
📚 Learning: 2025-10-03T02:04:17.803Z
Learnt from: abnegate
Repo: utopia-php/database PR: 721
File: tests/e2e/Adapter/Scopes/DocumentTests.php:6418-6439
Timestamp: 2025-10-03T02:04:17.803Z
Learning: In tests/e2e/Adapter/Scopes/DocumentTests::testSchemalessDocumentInvalidInteralAttributeValidation (PHP), when the adapter reports getSupportForAttributes() === false (schemaless), the test should not expect exceptions from createDocuments for “invalid” internal attributes; remove try/catch and ensure the test passes without exceptions, keeping at least one assertion.
Applied to files:
src/Database/Adapter/Mongo.php
🧬 Code graph analysis (2)
src/Database/Adapter.php (5)
src/Database/Adapter/Mongo.php (2)
find(1875-2065)getAttributeProjection(2563-2596)src/Database/Adapter/SQL.php (2)
find(2982-3203)getAttributeProjection(2337-2384)src/Database/Adapter/Pool.php (2)
find(271-285)getAttributeProjection(507-510)src/Database/Database.php (2)
find(7745-7897)Database(38-9432)src/Database/QueryContext.php (1)
QueryContext(8-89)
src/Database/Adapter/Mongo.php (4)
src/Database/QueryContext.php (3)
QueryContext(8-89)getCollections(32-35)skipAuth(75-86)src/Database/Adapter.php (4)
renameAttribute(628-628)filter(1259-1268)getAttributeProjection(1250-1250)find(829-841)src/Database/Adapter/SQL.php (4)
renameAttribute(310-327)getInternalKeyForAttribute(2386-2398)getAttributeProjection(2337-2384)find(2982-3203)src/Database/Document.php (3)
removeAttribute(287-293)getId(63-66)Document(12-470)
🪛 PHPMD (2.15.0)
src/Database/Adapter/Mongo.php
1884-1884: Avoid unused parameters such as '$joins'. (undefined)
(UnusedFormalParameter)
1885-1885: Avoid unused parameters such as '$vectors'. (undefined)
(UnusedFormalParameter)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Adapter Tests (SharedTables/SQLite)
- GitHub Check: Adapter Tests (SharedTables/MongoDB)
- GitHub Check: Adapter Tests (SharedTables/Postgres)
- GitHub Check: Adapter Tests (Schemaless/MongoDB)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Adapter Tests (MongoDB)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (Postgres)
- GitHub Check: Adapter Tests (MySQL)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (MariaDB)
- GitHub Check: Unit Test
🔇 Additional comments (4)
src/Database/Adapter/Mongo.php (3)
707-733:renameAttribute()now correctly respects internal key mappingThe new
renameAttribute(string $collection, string $old, string $new)that runs both names throughgetInternalKeyForAttribute()before$renamekeeps internal fields ($id,$sequence,$tenant, timestamps, permissions) consistent with the SQL adapter’s mapping helper. This also makesupdateAttribute()’s delegation (renameAttribute($collection, $id, $newKey)) safer in the face of special attributes.Also applies to: 1839-1851, 1826-1832
1790-1799:deleteDocuments()now reuses the query pipeline for$sequencedeletesSwitching to
buildFilters([Query::equal('$sequence', $sequences)])and thenreplaceInternalIdsKeys()brings the bulk-delete path in line with the rest of the query system and with$sequence→_idmapping. This is a good consistency improvement and should behave identically across adapters when deleting by sequence IDs.
2559-2595:getAttributeProjection()behavior matches the new selection semanticsThe new
getAttributeProjection(array $selects): arrayimplementation:
- Returns
[](no projection) when there are no selects or when*is selected, so Mongo returns full documents.- Skips
$collectionas a non-stored attribute.- Correctly maps
$id,$sequence,$tenant, timestamps, and$permissionsto their internal_uid,_id,_tenant,_createdAt,_updatedAt,_permissionscounterparts and marks them with1for inclusion.This is consistent with the SQL adapter’s mapping logic and with the
$removeSequencehandling ingetDocument()/find(), and should give predictable projections to callers.src/Database/Adapter.php (1)
815-825: All adapters match; review comment is accurate and approvedVerification confirms all three adapter implementations (SQL, Mongo, Pool) have identical
find()parameter names and order, matching the abstract signature in Adapter.php. The parameter names forselects,filters,joins,vectors, andorderQueriesare consistently standardized across implementations, validating the review's point that these names are now part of the public contract due to named argument usage inDatabase::find().
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 (1)
tests/unit/Validator/Query/FilterTest.php (1)
151-208: FixisValid()argument type and avoid using the protectedQueryconstructorStatic analysis errors here stem from (a) calling
isValid()with a singleQueryinstead of an array, and (b) instantiatingQuerydirectly via its protected constructor. You can fix both by always passing an array of queries and usingQuery::parse()for the “invalid arity” cases:151-163 - // Test valid notSearch queries - $this->assertTrue($this->validator->isValid([Query::notSearch('string', 'unwanted')])); + // Test valid notSearch queries + $this->assertTrue($this->validator->isValid([Query::notSearch('string', 'unwanted')])); @@ - // Test that arrays cannot use notSearch - $this->assertFalse($this->validator->isValid([Query::notSearch('string_array', 'unwanted')])); + // Test that arrays cannot use notSearch + $this->assertFalse($this->validator->isValid([Query::notSearch('string_array', 'unwanted')])); @@ - // Test multiple values not allowed - $this->assertFalse($this->validator->isValid([new Query(Query::TYPE_NOT_SEARCH, 'string', ['word1', 'word2'])])); + // Test multiple values not allowed + $this->assertFalse($this->validator->isValid([ + Query::parse('{"method":"notSearch","attribute":"string","values":["word1","word2"]}') + ])); 165-177 - // Test valid notStartsWith queries - $this->assertTrue($this->validator->isValid(Query::notStartsWith('string', 'temp'))); + // Test valid notStartsWith queries + $this->assertTrue($this->validator->isValid([Query::notStartsWith('string', 'temp')])); @@ - // Test that arrays cannot use notStartsWith - $this->assertFalse($this->validator->isValid(Query::notStartsWith('string_array', 'temp'))); + // Test that arrays cannot use notStartsWith + $this->assertFalse($this->validator->isValid([Query::notStartsWith('string_array', 'temp')])); @@ - // Test multiple values not allowed - $this->assertFalse($this->validator->isValid(new Query(Query::TYPE_NOT_STARTS_WITH, 'string', ['prefix1', 'prefix2']))); + // Test multiple values not allowed + $this->assertFalse($this->validator->isValid([ + Query::parse('{"method":"notStartsWith","attribute":"string","values":["prefix1","prefix2"]}') + ])); 179-191 - // Test valid notEndsWith queries - $this->assertTrue($this->validator->isValid(Query::notEndsWith('string', '.tmp'))); + // Test valid notEndsWith queries + $this->assertTrue($this->validator->isValid([Query::notEndsWith('string', '.tmp')])); @@ - // Test that arrays cannot use notEndsWith - $this->assertFalse($this->validator->isValid(Query::notEndsWith('string_array', '.tmp'))); + // Test that arrays cannot use notEndsWith + $this->assertFalse($this->validator->isValid([Query::notEndsWith('string_array', '.tmp')])); @@ - // Test multiple values not allowed - $this->assertFalse($this->validator->isValid(new Query(Query::TYPE_NOT_ENDS_WITH, 'string', ['suffix1', 'suffix2']))); + // Test multiple values not allowed + $this->assertFalse($this->validator->isValid([ + Query::parse('{"method":"notEndsWith","attribute":"string","values":["suffix1","suffix2"]}') + ])); 193-208 - // Test valid notBetween queries - $this->assertTrue($this->validator->isValid(Query::notBetween('integer', 0, 50))); + // Test valid notBetween queries + $this->assertTrue($this->validator->isValid([Query::notBetween('integer', 0, 50)])); @@ - // Test that arrays cannot use notBetween - $this->assertFalse($this->validator->isValid(Query::notBetween('integer_array', 1, 10))); + // Test that arrays cannot use notBetween + $this->assertFalse($this->validator->isValid([Query::notBetween('integer_array', 1, 10)])); @@ - // Test wrong number of values - $this->assertFalse($this->validator->isValid(new Query(Query::TYPE_NOT_BETWEEN, 'integer', [10]))); + // Test wrong number of values + $this->assertFalse($this->validator->isValid([ + Query::parse('{"method":"notBetween","attribute":"integer","values":[10]}') + ])); @@ - $this->assertFalse($this->validator->isValid(new Query(Query::TYPE_NOT_BETWEEN, 'integer', [10, 20, 30]))); + $this->assertFalse($this->validator->isValid([ + Query::parse('{"method":"notBetween","attribute":"integer","values":[10,20,30]}') + ]));This removes all direct constructor calls and ensures
isValid()always receivesarray<string|Query>as intended.
♻️ Duplicate comments (1)
tests/unit/Validator/DocumentQueriesTest.php (1)
83-103: Remove the fully commented‑outtestInvalidQueriesmethodThis method is entirely commented out and still sits at class scope, which (a) contributes to the PSR‑12
statement_indentationfailure and (b) keeps around an obsolete, incomplete test that no longer reflectsDocumentsValidatorbehavior.Recommend deleting this block altogether; if you still need negative‑case coverage for non‑select queries, add a fresh, focused test that asserts the intended V2 behavior instead of reviving this commented code.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tests/unit/Validator/DocumentQueriesTest.php(3 hunks)tests/unit/Validator/IndexedQueriesTest.php(6 hunks)tests/unit/Validator/Query/FilterTest.php(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-03T02:04:17.803Z
Learnt from: abnegate
Repo: utopia-php/database PR: 721
File: tests/e2e/Adapter/Scopes/DocumentTests.php:6418-6439
Timestamp: 2025-10-03T02:04:17.803Z
Learning: In tests/e2e/Adapter/Scopes/DocumentTests::testSchemalessDocumentInvalidInteralAttributeValidation (PHP), when the adapter reports getSupportForAttributes() === false (schemaless), the test should not expect exceptions from createDocuments for “invalid” internal attributes; remove try/catch and ensure the test passes without exceptions, keeping at least one assertion.
Applied to files:
tests/unit/Validator/DocumentQueriesTest.phptests/unit/Validator/IndexedQueriesTest.phptests/unit/Validator/Query/FilterTest.php
📚 Learning: 2025-07-30T19:17:53.630Z
Learnt from: ArnabChatterjee20k
Repo: utopia-php/database PR: 642
File: src/Database/Validator/PartialStructure.php:43-52
Timestamp: 2025-07-30T19:17:53.630Z
Learning: In PartialStructure validator, when filtering for required attributes validation using the $requiredAttributes parameter, $this->attributes should be used instead of the merged $attributes array because this validation is specifically for internal attributes like $createdAt and $updatedAt that are defined in the base Structure class, not collection-specific attributes.
Applied to files:
tests/unit/Validator/IndexedQueriesTest.php
🧬 Code graph analysis (3)
tests/unit/Validator/DocumentQueriesTest.php (3)
src/Database/QueryContext.php (2)
QueryContext(8-89)add(60-68)src/Database/Validator/Queries/V2.php (1)
V2(23-789)src/Database/Query.php (2)
Query(8-1606)select(771-774)
tests/unit/Validator/IndexedQueriesTest.php (2)
src/Database/QueryContext.php (2)
QueryContext(8-89)add(60-68)src/Database/Validator/Queries/V2.php (2)
V2(23-789)isValid(534-788)
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-789)isValid(534-788)getDescription(130-133)
🪛 GitHub Actions: CodeQL
tests/unit/Validator/DocumentQueriesTest.php
[error] 1-1: PHPStan analysis reported 30 errors.
tests/unit/Validator/IndexedQueriesTest.php
[error] 1-1: PHPStan analysis reported 30 errors.
tests/unit/Validator/Query/FilterTest.php
[error] 161-161: Cannot instantiate class Utopia\Database\Query via protected constructor Utopia\Database\Query::__construct().
[error] 168-168: Parameter #1 $value of method Utopia\Database\Validator\Queries\V2::isValid() expects array<string|Utopia\Database\Query>, Utopia\Database\Query given.
[error] 171-171: Parameter #1 $value of method Utopia\Database\Validator\Queries\V2::isValid() expects array<string|Utopia\Database\Query>, Utopia\Database\Query given.
[error] 175-175: Cannot instantiate class Utopia\Database\Query via protected constructor Utopia\Database\Query::__construct().
[error] 175-175: Parameter #1 $value of method Utopia\Database\Validator\Queries\V2::isValid() expects array<string|Utopia\Database\Query>, Utopia\Database\Query given.
[error] 182-182: Parameter #1 $value of method Utopia\Database\Validator\Queries\V2::isValid() expects array<string|Utopia\Database\Query>, Utopia\Database\Query given.
[error] 185-185: Cannot instantiate class Utopia\Database\Query via protected constructor Utopia\Database\Query::__construct().
[error] 189-189: Parameter #1 $value of method Utopia\Database\Validator\Queries\V2::isValid() expects array<string|Utopia\Database\Query>, Utopia\Database\Query given.
[error] 196-196: Parameter #1 $value of method Utopia\Database\Validator\Queries\V2::isValid() expects array<string|Utopia\Database\Query>, Utopia\Database\Query given.
[error] 199-199: Parameter #1 $value of method Utopia\Database\Validator\Queries\V2::isValid() expects array<string|Utopia\Database\Query>, Utopia\Database\Query given.
[error] 203-203: Cannot instantiate class Utopia\Database\Query via protected constructor Utopia\Database\Query::__construct().
[error] 203-203: Parameter #1 $value of method Utopia\Database\Validator\Queries\V2::isValid() expects array<string|Utopia\Database\Query>, Utopia\Database\Query given.
[error] 206-206: Cannot instantiate class Utopia\Database\Query via protected constructor Utopia\Database\Query::__construct().
[error] 206-206: Parameter #1 $value of method Utopia\Database\Validator\Queries\V2::isValid() expects array<string|Utopia\Database\Query>, Utopia\Database\Query given.
[error] 209-209: Cannot instantiate class Utopia\Database\Query via protected constructor Utopia\Database\Query::__construct().
[error] 209-209: Parameter #1 $value of method Utopia\Database\Validator\Queries\V2::isValid() expects array<string|Utopia\Database\Query>, Utopia\Database\Query given.
[error] 212-212: Cannot instantiate class Utopia\Database\Query via protected constructor Utopia\Database\Query::__construct().
[error] 212-212: Parameter #1 $value of method Utopia\Database\Validator\Queries\V2::isValid() expects array<string|Utopia\Database\Query>, Utopia\Database\Query given.
[error] 215-215: Cannot instantiate class Utopia\Database\Query via protected constructor Utopia\Database\Query::__construct().
[error] 215-215: Parameter #1 $value of method Utopia\Database\Validator\Queries\V2::isValid() expects array<string|Utopia\Database\Query>, Utopia\Database\Query given.
[error] 218-218: Cannot instantiate class Utopia\Database\Query via protected constructor Utopia\Database\Query::__construct().
[error] 218-218: Parameter #1 $value of method Utopia\Database\Validator\Queries\V2::isValid() expects array<string|Utopia\Database\Query>, Utopia\Database\Query given.
[error] 221-221: Cannot instantiate class Utopia\Database\Query via protected constructor Utopia\Database\Query::__construct().
[error] 221-221: Parameter #1 $value of method Utopia\Database\Validator\Queries\V2::isValid() expects array<string|Utopia\Database\Query>, Utopia\Database\Query given.
[error] 224-224: Cannot instantiate class Utopia\Database\Query via protected constructor Utopia\Database\Query::__construct().
[error] 224-224: Parameter #1 $value of method Utopia\Database\Validator\Queries\V2::isValid() expects array<string|Utopia\Database\Query>, Utopia\Database\Query given.
[error] 227-227: Cannot instantiate class Utopia\Database\Query via protected constructor Utopia\Database\Query::__construct().
[error] 227-227: Parameter #1 $value of method Utopia\Database\Validator\Queries\V2::isValid() expects array<string|Utopia\Database\Query>, Utopia\Database\Query given.
[error] 230-230: Cannot instantiate class Utopia\Database\Query via protected constructor Utopia\Database\Query::__construct().
[error] 230-230: Parameter #1 $value of method Utopia\Database\Validator\Queries\V2::isValid() expects array<string|Utopia\Database\Query>, Utopia\Database\Query given.
[error] 233-233: Cannot instantiate class Utopia\Database\Query via protected constructor Utopia\Database\Query::__construct().
[error] 233-233: Parameter #1 $value of method Utopia\Database\Validator\Queries\V2::isValid() expects array<string|Utopia\Database\Query>, Utopia\Database\Query given.
🪛 GitHub Actions: Linter
tests/unit/Validator/DocumentQueriesTest.php
[error] 1-1: PSR-12 lint error: statement_indentation.
tests/unit/Validator/IndexedQueriesTest.php
[error] 1-1: PSR-12 lint error: no_unused_imports.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Adapter Tests (SharedTables/Postgres)
- GitHub Check: Adapter Tests (Schemaless/MongoDB)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Adapter Tests (Postgres)
- GitHub Check: Adapter Tests (SharedTables/SQLite)
- GitHub Check: Adapter Tests (SharedTables/MongoDB)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (MariaDB)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Adapter Tests (MongoDB)
- GitHub Check: Adapter Tests (MySQL)
- GitHub Check: Unit Test
🔇 Additional comments (1)
tests/unit/Validator/Query/FilterTest.php (1)
14-65: V2 + QueryContext wiring insetUplooks consistentThe move to
DocumentsValidatorwith aQueryContext-backed collection document is coherent and matches the V2 constructor contract; the per-testmaxValuesCountproperty is also a clean way to reuse the limit in tests.
| use PHPUnit\Framework\TestCase; | ||
| use Swoole\FastCGI\Record\Data; | ||
| use Utopia\Database\Database; | ||
| use Utopia\Database\Document; | ||
| use Utopia\Database\Exception; | ||
| use Utopia\Database\Query; | ||
| use Utopia\Database\Validator\IndexedQueries; | ||
| use Utopia\Database\Validator\Query\Cursor; | ||
| use Utopia\Database\Validator\Query\Filter; | ||
| use Utopia\Database\Validator\Query\Limit; | ||
| use Utopia\Database\Validator\Query\Offset; | ||
| use Utopia\Database\Validator\Query\Order; | ||
| use Utopia\Database\QueryContext; | ||
| use Utopia\Database\Validator\Queries\V2 as DocumentsValidator; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove unused Swoole\FastCGI\Record\Data import
Data isn’t used anywhere in this test class and triggers the PSR‑12 no_unused_imports linter error. It’s safe to drop the import:
-use Swoole\FastCGI\Record\Data;
use Utopia\Database\Database;🤖 Prompt for AI Agents
In tests/unit/Validator/IndexedQueriesTest.php around lines 5 to 13, the import
"use Swoole\FastCGI\Record\Data;" is unused and triggers the PSR-12
no_unused_imports linter error; remove that import line so only the necessary
classes (PHPUnit\Framework\TestCase, Utopia\Database\Database,
Utopia\Database\Document, Utopia\Database\Exception, Utopia\Database\Query,
Utopia\Database\QueryContext, Utopia\Database\Validator\Queries\V2 as
DocumentsValidator) remain.
| public function testMaxValuesCount(): void | ||
| { | ||
| $max = $this->validator->getMaxValuesCount(); | ||
| $max = $this->maxValuesCount; | ||
| $values = []; | ||
| for ($i = 1; $i <= $max + 1; $i++) { | ||
| $values[] = $i; | ||
| } | ||
|
|
||
| $this->assertFalse($this->validator->isValid(Query::equal('integer', $values))); | ||
| $this->assertFalse($this->validator->isValid([Query::equal('integer', $values)])); | ||
| $this->assertEquals('Query on attribute has greater than '.$max.' values: integer', $this->validator->getDescription()); | ||
| } | ||
|
|
||
| public function testNotContains(): void | ||
| { | ||
| // Test valid notContains queries | ||
| $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])])); | ||
|
|
||
| // Test invalid notContains queries (empty values) | ||
| $this->assertFalse($this->validator->isValid(Query::notContains('string', []))); | ||
| $this->assertFalse($this->validator->isValid([Query::notContains('string', [])])); | ||
| $this->assertEquals('NotContains queries require at least one value.', $this->validator->getDescription()); |
There was a problem hiding this comment.
Align expected error messages with V2’s validateValues behavior
V2 now prefixes some validation messages with Invalid query:, and validateValues() for too-many values and empty NotContains uses those exact strings. The current expectations in testMaxValuesCount() and testNotContains() are missing that prefix.
Consider updating the assertions like this:
129-137
- $max = $this->maxValuesCount;
+ $max = $this->maxValuesCount;
@@
- $this->assertFalse($this->validator->isValid([Query::equal('integer', $values)]));
- $this->assertEquals('Query on attribute has greater than '.$max.' values: integer', $this->validator->getDescription());
+ $this->assertFalse($this->validator->isValid([Query::equal('integer', $values)]));
+ $this->assertEquals('Invalid query: Query on attribute has greater than '.$max.' values: integer', $this->validator->getDescription());
139-148
@@
- $this->assertFalse($this->validator->isValid([Query::notContains('string', [])]));
- $this->assertEquals('NotContains queries require at least one value.', $this->validator->getDescription());
+ $this->assertFalse($this->validator->isValid([Query::notContains('string', [])]));
+ $this->assertEquals('Invalid query: NotContains queries require at least one value.', $this->validator->getDescription());📝 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.
| public function testMaxValuesCount(): void | |
| { | |
| $max = $this->validator->getMaxValuesCount(); | |
| $max = $this->maxValuesCount; | |
| $values = []; | |
| for ($i = 1; $i <= $max + 1; $i++) { | |
| $values[] = $i; | |
| } | |
| $this->assertFalse($this->validator->isValid(Query::equal('integer', $values))); | |
| $this->assertFalse($this->validator->isValid([Query::equal('integer', $values)])); | |
| $this->assertEquals('Query on attribute has greater than '.$max.' values: integer', $this->validator->getDescription()); | |
| } | |
| public function testNotContains(): void | |
| { | |
| // Test valid notContains queries | |
| $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])])); | |
| // Test invalid notContains queries (empty values) | |
| $this->assertFalse($this->validator->isValid(Query::notContains('string', []))); | |
| $this->assertFalse($this->validator->isValid([Query::notContains('string', [])])); | |
| $this->assertEquals('NotContains queries require at least one value.', $this->validator->getDescription()); | |
| public function testMaxValuesCount(): void | |
| { | |
| $max = $this->maxValuesCount; | |
| $values = []; | |
| for ($i = 1; $i <= $max + 1; $i++) { | |
| $values[] = $i; | |
| } | |
| $this->assertFalse($this->validator->isValid([Query::equal('integer', $values)])); | |
| $this->assertEquals('Invalid query: Query on attribute has greater than '.$max.' values: integer', $this->validator->getDescription()); | |
| } | |
| public function testNotContains(): void | |
| { | |
| // Test valid notContains queries | |
| $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])])); | |
| // Test invalid notContains queries (empty values) | |
| $this->assertFalse($this->validator->isValid([Query::notContains('string', [])])); | |
| $this->assertEquals('Invalid query: NotContains queries require at least one value.', $this->validator->getDescription()); |
🤖 Prompt for AI Agents
In tests/unit/Validator/Query/FilterTest.php around lines 127 to 148, the
assertions expect old error messages but V2's validateValues() now prefixes
messages with "Invalid query:"; update the two failing assertions to include
that prefix so they match the validator output — change the expected string in
testMaxValuesCount() to "Invalid query: Query on attribute has greater than
{max} values: integer" (or concatenate the existing message with "Invalid query:
"), and change the expected string in testNotContains() to "Invalid query:
NotContains queries require at least one value." ensuring the dynamic $max
variable is preserved in the first assertion.
Summary by CodeRabbit
New Features
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.