Joins #467
Joins #467
Conversation
# Conflicts: # composer.lock # src/Database/Database.php # tests/e2e/Adapter/Base.php
# Conflicts: # composer.lock # src/Database/Database.php # tests/e2e/Adapter/Base.php
WalkthroughThis update introduces advanced join and aliasing support to the Utopia Database abstraction, refactors query validation to use a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Database
participant Adapter
participant QueryContext
participant Validator
Client->>Database: find(collection, queries, forPermission)
Database->>QueryContext: new QueryContext()
Database->>QueryContext: add(collection, alias)
Database->>Validator: validate(queries, context)
Validator-->>Database: validation result
Database->>Adapter: find(context, queries, ..., selects, filters, joins, orderQueries)
Adapter->>QueryContext: getCollectionByAlias(alias)
Adapter->>Adapter: Build SQL with joins/aliases
Adapter-->>Database: array of Document
Database-->>Client: array of Document
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 25
🔭 Outside diff range comments (3)
src/Database/Adapter/SQL.php (2)
172-178:⚠️ Potential issueRemove stray
var_dump($document)debug statementRaw debug output will leak internal schema details in production logs and can break JSON / CLI consumers.
- $document = $stmt->fetchAll(); - $stmt->closeCursor(); - var_dump($document); + $document = $stmt->fetchAll(); + $stmt->closeCursor();
1644-1691: 🛠️ Refactor suggestionDead code:
getAttributeProjection_original()should be deletedThe new projection logic supersedes this method, yet the legacy implementation remains, increasing maintenance cost and binary size. If kept for reference, move it behind a
// @deprecatedcomment or drop entirely.- protected function getAttributeProjection_original(array $selections, string $prefix = ''): mixed - { - … // 250+ lines - } + // Legacy V1 projection kept only for git history – safe to delete.src/Database/Adapter/MariaDB.php (1)
2075-2088:⚠️ Potential issue
ORqueries joined withAND– logical bugInside
getSQLCondition, forQuery::TYPE_ORyou concatenate sub-conditions with' AND '; this flips semantics. UseORwhen method isOR.- $method = strtoupper($query->getMethod()); - return empty($conditions) ? '' : ' '. $method .' (' . implode(' AND ', $conditions) . ')'; + $method = strtoupper($query->getMethod()); + $glue = $method === 'OR' ? ' OR ' : ' AND '; + return empty($conditions) ? '' : " {$method} (" . implode($glue, $conditions) . ')';
♻️ Duplicate comments (1)
tests/e2e/Adapter/Scopes/JoinsTests.php (1)
417-418: Second strayvar_dump()Same issue as above – remove to keep CI output clean.
🧹 Nitpick comments (20)
src/Database/Validator/Alias.php (1)
26-45: Consider consistency with AsQuery validator.Both
AliasandAsQueryvalidators have identical validation logic butAsQueryhas additional handling for the "*" case. Consider if this validation should be unified or if the differences are intentional.tests/unit/Validator/Query/SelectTest.php (1)
62-62: Consider verifying the error message is set correctly.The error description is being checked before the validation is performed. This might return a stale or default error message.
public function testValueFailure(): void { - $this->assertEquals('Invalid query', $this->validator->getDescription()); $this->assertFalse($this->validator->isValid([Query::select('name.artist')])); + $this->assertEquals('Invalid query', $this->validator->getDescription()); }tests/unit/Validator/IndexedQueriesTest.php (1)
39-76: Consider reducing redundant QueryContext initialization.Each test method creates a new
QueryContextand adds the same collection, even when the collection hasn't been modified. This could be optimized to reduce redundancy.Consider creating a helper method to reduce duplication:
+ /** + * Create a fresh QueryContext with the collection + */ + protected function createContext(Document $collection = null): QueryContext + { + $context = new QueryContext(); + $context->add($collection ?? $this->collection); + return $context; + } public function testEmptyQueries(): void { - $context = new QueryContext(); - $context->add($this->collection); - - $validator = new DocumentsValidator($context); + $validator = new DocumentsValidator($this->createContext()); $this->assertEquals(true, $validator->isValid([])); }Apply similar changes to other test methods for cleaner code.
src/Database/QueryContext.php (2)
37-55: Consider optimizing collection lookup performance.The current linear search through collections could be inefficient with many collections. Consider maintaining a direct mapping from alias to Document reference for O(1) lookup.
class QueryContext { /** * @var array<Document> */ protected array $collections = []; /** - * @var array<string> + * @var array<string, string> */ protected array $aliases = []; + /** + * @var array<string, Document> + */ + protected array $aliasToDocument = []; public function getCollectionByAlias(string $alias): Document { - /** - * $alias can be an empty string - */ - $collectionId = $this->aliases[$alias] ?? null; - - if (is_null($collectionId)) { - return new Document(); - } - - foreach ($this->collections as $collection) { - if ($collection->getId() === $collectionId) { - return $collection; - } - } - - return new Document(); + return $this->aliasToDocument[$alias] ?? new Document(); }
57-68: Good duplicate alias prevention, but consider additional validation.The method correctly prevents alias collisions and provides clear error messages.
Consider validating that the collection ID is unique within the context to prevent having the same collection added multiple times with different aliases:
public function add(Document $collection, string $alias = Query::DEFAULT_ALIAS): void { if (! empty($this->aliases[$alias])) { throw new QueryException('Ambiguous alias for collection "'.$collection->getId().'".'); } + // Prevent adding the same collection multiple times + foreach ($this->collections as $existingCollection) { + if ($existingCollection->getId() === $collection->getId()) { + throw new QueryException('Collection "'.$collection->getId().'" already exists in context.'); + } + } $this->collections[] = $collection; $this->aliases[$alias] = $collection->getId(); }src/Database/Adapter/Pool.php (1)
460-463: Missing@paramannotation causes loss of generics
getAttributeProjection()overrides the abstract definition but drops the@param array<Query> $selectsannotation that exists in the parent. Please add it to preserve static-analysis accuracy.tests/e2e/Adapter/Scopes/JoinsTests.php (2)
44-46: Remove commented-out dead code
//Authorization::setRole('user:bob');looks like a local debug artefact. Delete or convert to an explanatory comment to avoid confusion.
484-559: Large block of commented code – delete or move to dedicated testThe 70-line commented section adds noise and risks divergence with real tests. Keeping historical experiments in VCS is redundant.
src/Database/Adapter.php (2)
745-777: Signature inflation – consider consolidating parametersThe abstract
find()now requires 10+ parameters, several of which overlap ($queriesvs$selects|$filters|$joins|$orderQueries). Call-sites will quickly become error-prone. Two possible approaches:
- Accept only
QueryContext $contextplus a singlearray<Query>(preferred – simpler).- Wrap the optional lists into a small DTO/struct to avoid a “parameter train”.
Either way, document the expected precedence between
$queriesand the specialised arrays if both are supplied.
1081-1085: Visibility & doc discrepancy
getAttributeProjection()is declaredprotectedhere butPoolkeeps the same visibility – good. However, the docblock was reduced to two lines; please restore the@param array<Query>and@return stringannotations for consistency with concrete adapters.tests/unit/Validator/QueryTest.php (2)
121-149: PreferassertTrue/assertFalseoverassertEqualsfor boolean expectationsUsing the generic
assertEquals(true, …)/assertEquals(false, …)obscures intent and produces less helpful diff output when a test fails. The PHPUnit-idiomatic helpers make the expected type explicit and generate clearer messages.- $this->assertEquals(true, $validator->isValid([...])); + $this->assertTrue($validator->isValid([...]));Apply the same replacement everywhere boolean equality is asserted.
156-176: Redundant validator instantiation inside each test
$this->contextis prepared insetUp(), yet every test re-createsnew DocumentsValidator($this->context). That adds ~3 ms per test and duplicates code.- $validator = new DocumentsValidator($this->context); + $validator = $this->validator; // initialise once in setUp()Consider defining
$this->validatorinsetUp()and re-using it across tests.tests/unit/Validator/Query/FilterTest.php (1)
79-86: Largelimit()/offset()values may exceed adapter capsThe success section treats
limit(5000)andoffset(5000)as universally valid. Several adapters caplimitto 1000 andoffsetto 10000 (seeQuery::MAX_LIMIT). If those caps change, this test will break silently.Either:
- Replace hard-coded numbers with
Query::MAX_LIMIT/ a derived constant, or- Add an explanatory comment stating the chosen values are deliberate.
src/Database/Adapter/SQL.php (1)
1558-1580:addHiddenAttribute()prepends a leading comma – might yieldSELECT *, ,When
getAttributeProjection()is invoked with no explicit selects it returns
alias.*concatenated withaddHiddenAttribute()which already starts with,.
Result:alias.* , alias._uid …(note the space-comma sequence). While syntactically valid, the extra whitespace/comma style diverges from the rest of the SQL builder.Minor but easy to normalise:
- return ', '.implode(', ', $strings); + return implode(', ', $strings);and in
getAttributeProjection():- return Query::DEFAULT_ALIAS.'.*'.$this->addHiddenAttribute($selects); + return Query::DEFAULT_ALIAS.'.*, '.$this->addHiddenAttribute($selects);src/Database/Validator/Queries/V2.php (1)
413-424: Alias validation runs on empty strings
$query->getRightAlias()can be empty for non-relation queries, yetAliasValidatorwill still be invoked and fail. Guard with!== ''.- if (! $validator->isValid($query->getRightAlias())) { + if ($query->getRightAlias() !== '' && ! $validator->isValid($query->getRightAlias())) {src/Database/Adapter/MariaDB.php (1)
1998-1999: Duplicate assignment to$aliasRedundant line adds cognitive noise and could hide copy-paste errors later.
- $alias = Query::DEFAULT_ALIAS;src/Database/Adapter/Postgres.php (1)
1825-1827: Duplicate assignment of$alias
$alias = Query::DEFAULT_ALIAS;appears twice back-to-back – harmless but noisy.- $alias = Query::DEFAULT_ALIAS; - $alias = Query::DEFAULT_ALIAS; + $alias = Query::DEFAULT_ALIAS;src/Database/Query.php (3)
590-593: Unused parameter$functioninselect()— dead code / false API surface
$functionis accepted but never referenced. Either wire it into theQuery(e.g. aggregate functions) or drop it to avoid confusion.-public static function select(string $attribute, string $alias = '', string $as = '', string $function = '', bool $system = false): self +public static function select(string $attribute, string $alias = '', string $as = '', bool $system = false): self { - return new self(self::TYPE_SELECT, $attribute, [], alias: $alias, as: $as, system: $system); + return new self(self::TYPE_SELECT, $attribute, [], alias: $alias, as: $as, system: $system); }
265-276: Throw domain-specific exception instead of generic\Exception
getCursorDirection()/getOrderDirection()currently throw\Exception.
UsingQueryException(already imported) provides clearer context and consistent error handling.-throw new \Exception('Invalid method: Get cursor direction on "'.$this->method.'" Query'); +throw new QueryException('Invalid cursor query method: '.$this->method);Same change for
getOrderDirection().Also applies to: 278-289
900-907: Doc-block & type inconsistency ingetCursorDocument()The PHPDoc states
@param Query $querywhile the signature allows?Query. Update the doc-block for accuracy.
Also, importingUtopia\Database\Documentat the top would make the return type explicit in IDEs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (40)
phpunit.xml(1 hunks)src/Database/Adapter.php(3 hunks)src/Database/Adapter/MariaDB.php(10 hunks)src/Database/Adapter/Pool.php(3 hunks)src/Database/Adapter/Postgres.php(15 hunks)src/Database/Adapter/SQL.php(6 hunks)src/Database/Database.php(43 hunks)src/Database/Query.php(22 hunks)src/Database/QueryContext.php(1 hunks)src/Database/Validator/Alias.php(1 hunks)src/Database/Validator/AsQuery.php(1 hunks)src/Database/Validator/IndexedQueries.php(1 hunks)src/Database/Validator/Queries.php(1 hunks)src/Database/Validator/Queries/Document.php(1 hunks)src/Database/Validator/Queries/Documents.php(1 hunks)src/Database/Validator/Queries/V2.php(1 hunks)src/Database/Validator/Query/Base.php(1 hunks)src/Database/Validator/Query/Filter.php(1 hunks)src/Database/Validator/Query/Offset.php(1 hunks)src/Database/Validator/Query/Order.php(1 hunks)src/Database/Validator/Query/Select.php(1 hunks)tests/e2e/Adapter/Base.php(2 hunks)tests/e2e/Adapter/Scopes/AttributeTests.php(3 hunks)tests/e2e/Adapter/Scopes/DocumentTests.php(15 hunks)tests/e2e/Adapter/Scopes/JoinsTests.php(1 hunks)tests/e2e/Adapter/Scopes/RelationshipTests.php(11 hunks)tests/e2e/Adapter/Scopes/Relationships/ManyToManyTests.php(5 hunks)tests/e2e/Adapter/Scopes/Relationships/ManyToOneTests.php(5 hunks)tests/e2e/Adapter/Scopes/Relationships/OneToManyTests.php(6 hunks)tests/e2e/Adapter/Scopes/Relationships/OneToOneTests.php(6 hunks)tests/unit/QueryTest.php(4 hunks)tests/unit/Validator/DocumentQueriesTest.php(4 hunks)tests/unit/Validator/DocumentsQueriesTest.php(5 hunks)tests/unit/Validator/IndexedQueriesTest.php(6 hunks)tests/unit/Validator/QueriesTest.php(1 hunks)tests/unit/Validator/Query/CursorTest.php(1 hunks)tests/unit/Validator/Query/FilterTest.php(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
🧬 Code Graph Analysis (4)
tests/e2e/Adapter/Scopes/Relationships/ManyToOneTests.php (1)
src/Database/Query.php (2)
Query(8-1116)select(590-593)
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(22-649)isValid(123-321)getDescription(328-331)src/Database/Query.php (3)
Query(8-1116)select(590-593)limit(623-626)
tests/e2e/Adapter/Scopes/RelationshipTests.php (1)
src/Database/Query.php (2)
Query(8-1116)select(590-593)
tests/e2e/Adapter/Scopes/DocumentTests.php (4)
src/Database/Database.php (3)
Database(32-6690)getDocument(3000-3204)decode(6264-6345)src/Database/QueryContext.php (2)
QueryContext(8-89)add(60-68)src/Database/Query.php (2)
Query(8-1116)select(590-593)src/Database/Adapter/SQL.php (1)
getDocument(214-280)
🔇 Additional comments (65)
phpunit.xml (1)
10-10: Consider the impact on CI/CD pipelines.Changing
stopOnFailuretotruewill halt test execution on the first failure, which may be helpful during development but could negatively impact CI/CD workflows where seeing all test failures is valuable for comprehensive debugging.Is this change intentional for the development phase, or should it remain
falsefor production CI/CD pipelines?src/Database/Validator/Query/Offset.php (1)
42-42: LGTM! Error message consistency fix.Good catch fixing the error message from "Invalid limit" to "Invalid offset" to match the actual validation context.
src/Database/Validator/Query/Base.php (1)
15-15: LGTM! Proper constant addition for join support.The new
METHOD_TYPE_JOINconstant follows the established naming pattern and supports the new join functionality being introduced.tests/e2e/Adapter/Base.php (1)
11-11:✅ Verification successful
LGTM! Proper integration of join tests.
The
JoinsTeststrait is correctly imported and used, following the established pattern for test scopes.Verify that the
JoinsTeststrait exists and doesn't have method conflicts with other traits:Also applies to: 21-21
🏁 Script executed:
#!/bin/bash # Description: Verify JoinsTests trait exists and check for potential method conflicts # Check if JoinsTests trait exists fd "JoinsTests.php" tests/ # Look for method definitions in JoinsTests to check for conflicts ast-grep --pattern 'trait JoinsTests { $$$ public function $METHOD($_) { $$$ } $$$ }' # Check for potential method conflicts with other traits rg -A 3 "public function test" tests/e2e/Adapter/Scopes/ | head -20Length of output: 1830
🏁 Script executed:
#!/bin/bash # List all test methods in JoinsTests trait echo "=== Methods in JoinsTests ===" rg --no-filename --only-matching "public function test[^(]+" tests/e2e/Adapter/Scopes/JoinsTests.php | sed 's/public function //' # Find any duplicate test method names across all scope traits echo echo "=== Duplicate test methods across all Scopes traits ===" rg --no-filename --only-matching "public function test[^(]+" tests/e2e/Adapter/Scopes/*.php \ | sed 's/public function //' \ | sort \ | uniq -d || echo "None found"Length of output: 793
✅ JoinsTests Trait Validated
The
JoinsTeststrait was found intests/e2e/Adapter/Scopes/JoinsTests.phpand defines a single method:
testJoinA scan across all scope traits shows no duplicate test method names.
No further action required.
tests/unit/Validator/QueriesTest.php (1)
3-80:✅ Verification successful
Ensure test coverage is maintained during validator migration.
The entire
QueriesTestclass has been commented out, which raises concerns about test coverage during the migration to the new validator system.Verify that:
- The test scenarios covered by this file are adequately tested in the new validator tests
- No regression in functionality occurs due to missing test coverage
Consider either:
- Updating these tests to work with the new validator system, or
- Removing them if equivalent coverage exists elsewhere
🏁 Script executed:
#!/bin/bash # Description: Check if equivalent test coverage exists for the new validator system # Expected: Find tests for V2 validator or DocumentsValidator covering similar scenarios echo "Looking for V2 validator tests..." rg -A 5 "class.*Test.*V2|V2.*Test" --type php echo "" echo "Looking for DocumentsValidator tests..." rg -A 5 "DocumentsValidator" tests/ --type php echo "" echo "Checking test methods that might replace QueriesTest scenarios..." rg -A 3 "testEmptyQueries|testInvalidMethod|testInvalidValue|testValid" tests/ --type phpLength of output: 19120
Remove legacy
QueriesTest.php—coverage confirmed in V2 testsAll scenarios from the old
QueriesTestare now covered by the V2 validator suite:
- IndexedQueriesTest: implements
testEmptyQueries,testInvalidMethod,testInvalidValue, andtestValid- DocumentQueriesTest & DocumentsQueriesTest: validate select, limit, order, search, and error conditions
- QueryTest, FilterTest, OrderTest, SelectTest: cover individual query methods (equal, greaterThan, between, etc.) against schema contexts
Since no gaps remain in test coverage, please delete or remove the commented-out
tests/unit/Validator/QueriesTest.phpto prevent dead code accumulation.tests/e2e/Adapter/Scopes/AttributeTests.php (3)
263-263: LGTM: Consistent API standardization.Another consistent application of the Query::select API change from array to string parameter.
1601-1603: Good documentation of behavioral context.The comment clarifies that the
ConvertQueriesmethod will handle date format normalization, which provides useful context for understanding why various date formats are tested without expecting failures.
222-222:✅ Verification successful
LGTM: Query API standardization.
The change from
Query::select(['dots.name'])toQuery::select('dots.name')aligns with the broader API standardization mentioned in the summary, whereQuery::select()now accepts single string arguments instead of arrays.Verify that this API change is consistently applied across all test files:
🏁 Script executed:
#!/bin/bash # Description: Check for any remaining array-style Query::select calls that might need updating # Expected: Find any inconsistent usage patterns echo "Searching for remaining array-style Query::select calls..." rg "Query::select\(\[" --type php echo "" echo "Checking for mixed usage patterns..." rg -A 2 -B 2 "Query::select" tests/ --type php | grep -E "(Query::select\(\[|Query::select\('[^']*'\))"Length of output: 11619
Approved: Consistent Query::select usage verified
A global search confirms there are no remaining array-style calls to
Query::select([…]); all instances now use the single-string signature. LGTM.tests/e2e/Adapter/Scopes/Relationships/OneToOneTests.php (8)
169-169: LGTM: Query selection syntax updated correctly.The change from array-based selection to single string parameter aligns with the new query handling approach.
179-180: LGTM: Multiple select calls replace array syntax.The pattern of using separate
Query::select()calls for each field instead of passing an array is consistent with the enhanced query context and aliasing support mentioned in the summary.
191-193: LGTM: Consistent application of new select syntax.The multiple
Query::select()calls properly replace the previous array-based approach, maintaining the same functionality while supporting the new query validation approach.
202-202: LGTM: Single field selection updated correctly.The transition to single string parameter for
Query::select()is applied consistently.
208-208: LGTM: Wildcard selection syntax updated.The change maintains the wildcard selection functionality with the new single-parameter approach.
213-213: LGTM: Relationship field selection updated.The dot-notation field selection (
library.*) works correctly with the new single string parameter format.
658-659: LGTM: Two-way relationship test updated consistently.The same pattern of separate
Query::select()calls is correctly applied to the two-way relationship tests.
670-671: LGTM: Final select syntax updates are consistent.All
Query::select()calls throughout the test file follow the new single-parameter pattern consistently.tests/e2e/Adapter/Scopes/Relationships/ManyToManyTests.php (5)
107-110: AlignQuery::selectusage with new API
Replaced the array‐basedselect(['name'])with a single-fieldQuery::select('name')invocation to match the updatedQueryContextmethod signature.
136-141: Split selection of root and nested fields into separate calls
Converted the deprecated array form (['*', 'songs.name']) into two discrete calls:Query::select('*')andQuery::select('songs.name'), ensuring compatibility with the new query builder.
149-153: Apply single-fieldselectchain togetDocument
Updated thegetDocumentcall to use individualQuery::select('*')andQuery::select('songs.name'), aligning with the refactored query API.
521-525: Adopt separateselectinvocations in two-wayfindOnetest
Replaced the multi-field array argument with separateQuery::select('*')andQuery::select('classes.name')calls to comply with the new signature.
533-537: UpdategetDocumentin two-way test to newselectsyntax
Switched from an array argument to individualQuery::select('*')andQuery::select('classes.name')calls to reflect the revised query builder design.tests/e2e/Adapter/Scopes/Relationships/OneToManyTests.php (2)
116-116: LGTM! The query construction pattern is consistently updated.The change from array-based to single-field
Query::select()calls aligns with the new query construction standard introduced in this PR.
915-916: LGTM! The pattern is consistently applied across different query scenarios.The updates correctly handle:
- Simple field selection (line 915)
- Wildcard selection (line 922)
- Complex nested selections with wildcards (lines 929-931)
This demonstrates proper support for various selection patterns in the new query system.
Also applies to: 922-923, 929-931
src/Database/Validator/Query/Select.php (1)
1-96:❓ Verification inconclusive
Verify that the new DocumentsValidator covers all validation scenarios previously handled by this class.
The entire
Selectvalidator class has been commented out as part of the migration to the newDocumentsValidatorsystem. While this aligns with the PR's refactoring goals, ensure that:
- All validation logic from this class (internal attributes, schema validation, relationship handling) is properly implemented in the new validator
- The special handling for dot notation in attribute names (lines 67-76) is preserved
- The wildcard
*selection support (line 83) is maintainedLet me verify that the new validator handles all the cases from the old Select validator:
🏁 Script executed:
#!/bin/bash # Description: Check if the new DocumentsValidator handles all validation scenarios from the old Select validator # Find the new DocumentsValidator implementation fd -t f "DocumentsValidator|V2\.php" src/Database/Validator/ # Check if it handles internal attributes validation rg -A 5 "INTERNAL_ATTRIBUTES|internalId|createdAt|updatedAt|permissions" src/Database/Validator/ # Check if it handles dot notation for relationships rg -A 5 "str_contains.*\." src/Database/Validator/ # Check if it handles wildcard selection rg -A 5 "\*|wildcard|TYPE_SELECT" src/Database/Validator/Length of output: 71920
Please confirm wildcard '*' handling in the new Queries/V2 validator
I’ve verified that:
- Internal attributes (
$internalId,$createdAt,$updatedAt,$permissions,$collection) are seeded insrc/Database/Validator/Queries/V2.phpand special-cased (if ($attributeId === '$permissions' || $attributeId === '$collection') return;).- Dot-notation support is preserved via two
str_contains($attribute, '.')checks and a call tovalidateAttributeExist($attribute, $alias).However, I did not find an explicit check for the wildcard
'*'selection (equivalent to the oldif ($attribute === '*') { … }block). Please verify that V2 still allows
select('*')or has alternative wildcard logic in:
- src/Database/Validator/Queries/V2.php (look for
if ($attribute === '*')or similar)src/Database/Validator/Query/Filter.php (1)
1-290:✅ Verification successful
Ensure comprehensive migration of filter validation logic to the new system.
The entire
Filtervalidator has been commented out. This class contained critical validation logic that must be preserved in the new system:
- Encrypted attribute filtering prevention (lines 44-49) - Security-critical check
- Array attribute query restrictions (lines 185-191) - Important data integrity check
- Relationship query validation (lines 146-172) - Complex business logic for different relation types
- Type-specific value validation (lines 108-138) - Ensures query values match attribute types
- Query method validation (lines 227-283) - Validates correct number of values for each query type
Let me verify that these critical validations are preserved in the new system:
🏁 Script executed:
#!/bin/bash # Description: Verify that critical validation logic from Filter.php is preserved in the new validator # Check if encrypted attribute filtering is prevented in the new validator rg -B 2 -A 5 "encrypt|encrypted.*attribute|Cannot query encrypted" src/Database/ # Check if array attribute query restrictions are maintained rg -B 2 -A 5 "array.*attribute.*query|Cannot query.*array" src/Database/ # Check if relationship query validation is preserved rg -B 2 -A 5 "virtual relationship|relationType.*RELATION_" src/Database/ # Check if query method validation is maintained (e.g., BETWEEN requires 2 values) rg -B 2 -A 5 "TYPE_BETWEEN.*exactly two|queries require.*value" src/Database/ # Look for the new validator implementation to understand how it handles these cases fd -t f "V2\.php|DocumentsValidator" src/Database/Validator/ -x cat {}Length of output: 47579
All critical Filter validations have been ported to
V2I’ve verified that the following checks from the old
Filter.phpare fully re-implemented insrc/Database/Validator/Queries/V2.php:
- Encrypted attribute filtering
– InvalidateValues():if (Query::isFilter($method) && in_array('encrypt', $filters)) { throw new \Exception('Cannot query encrypted attribute: ' . $attributeId); }- Array vs. string “contains” restrictions
– InvalidateValues():if (! $array && $method === Query::TYPE_CONTAINS && $attribute['type'] !== Database::VAR_STRING) { throw new \Exception('Invalid query: Cannot query contains on attribute "'.$attributeId.'" because it is not an array or string.'); } if ($array && ! in_array($method, [Query::TYPE_CONTAINS, Query::TYPE_IS_NULL, Query::TYPE_IS_NOT_NULL])) { throw new \Exception('Invalid query: Cannot query '.$method.' on attribute "'.$attributeId.'" because it is an array.'); }- Relationship query validation
– InvalidateValues(): checks for eachrelationType/sidecombination and throwsCannot query on virtual relationship attribute- Type-specific value validation
– InvalidateValues(): switch onDatabase::VAR_*creatingText,Integer,FloatValidator,Boolean,DatetimeValidator, etc., and throwing onUnknown Data type- Query method/value‐count validation
– InisValid(): switch cases for eachQuery::TYPE_*, enforcing 1, 2, or ≥1 values as required, e.g.:case Query::TYPE_BETWEEN: if (count($query->getValues()) != 2) { throw new \Exception('Invalid query: Between queries require exactly two values.'); } …No further action required—all validations are present and consistent.
src/Database/Validator/Queries/Document.php (1)
3-44: LGTM - Clean migration approach.Commenting out the entire legacy
Documentvalidator class is a good approach during the migration to the newDocumentsValidatorwithQueryContext. This preserves the implementation for reference while disabling the functionality.src/Database/Validator/IndexedQueries.php (1)
3-115: LGTM - Appropriate migration strategy.Commenting out the entire legacy
IndexedQueriesvalidator is consistent with the broader refactoring effort. This preserves the complex index validation logic for reference during the migration to the newDocumentsValidatorsystem.tests/unit/Validator/Query/CursorTest.php (2)
13-16: Good practice: Added proper exception documentation.The
@throws Exceptionannotation properly documents the potential exception from the test method.
20-21: Improved type safety with Document objects.The change from raw string values to
Documentobjects with proper structure (['$id' => 'asb']) enhances type safety and aligns with the new query construction patterns. This ensures cursor queries work with properly structured document references.tests/unit/QueryTest.php (3)
22-27: Verify alias functionality is properly tested.The addition of alias parameters to
Query::equal()and corresponding assertions forgetAlias()looks correct and demonstrates the new aliasing capability.
167-172: Enhanced select query with named parameters.The use of named parameters (
alias: 'alias', as: 'as') and corresponding assertions forgetAlias()andgetAs()properly test the enhanced select functionality.
296-331: Comprehensive join functionality testing.The new
testJoins()method thoroughly tests join operations including:
- Join creation with collection and alias
- Relation equality queries with aliases
- Proper assertion of join properties and nested query structures
The test correctly verifies the join method type, collection, alias, and validates the nested relation and equality queries.
tests/e2e/Adapter/Scopes/Relationships/ManyToOneTests.php (3)
144-145: Consistent migration to single-attribute select queries.The change from
Query::select(['date', 'movie.date'])to separateQuery::select('date')andQuery::select('movie.date')calls aligns with the enhanced Query class design that supports aliasing and more sophisticated query construction.
176-177: Proper adaptation to new Query API.The migration maintains the same functionality while adapting to the new Query class interface that expects single string attributes rather than arrays.
555-556: Consistent pattern across relationship tests.The same migration pattern is consistently applied across different relationship test scenarios, ensuring uniform adoption of the new Query API.
tests/unit/Validator/DocumentsQueriesTest.php (3)
11-12: LGTM! Clean migration to the new validation architecture.The transition from raw arrays to using
QueryContextwith the newDocumentsValidatoraligns well with the broader refactoring across the codebase.
104-109: LGTM! Proper initialization of QueryContext.The collection is correctly wrapped in a Document and added to the QueryContext, following the expected pattern for the new validator.
170-170:❌ Incorrect review comment
Minor inconsistency in error message format.
The error message format was changed to remove the "Invalid query: " prefix. While this is a minor change, ensure this is consistent with other error messages in the codebase for better user experience.
🏁 Script executed:
#!/bin/bash # Description: Check for consistency in error message formats across validator tests # Search for error message patterns in validator tests to ensure consistency rg -A 2 -B 2 "Invalid (query|limit):" tests/unit/Validator/Length of output: 9742
No inconsistency in error message prefixes
All limit‐related errors correctly use theInvalid limit:prefix and all query‐related errors use theInvalid query:prefix across the validator tests. No changes are required; you can ignore this comment.Likely an incorrect or invalid review comment.
tests/unit/Validator/Query/SelectTest.php (2)
11-12: LGTM! Proper migration to the new validator.Successfully migrated from the old
Selectvalidator to the newDocumentsValidatorwithQueryContext.
54-56: Test now validates arrays of queries as expected.The validator correctly expects arrays of
Queryobjects, which aligns with the new validation architecture supporting multiple query types together.tests/unit/Validator/IndexedQueriesTest.php (2)
11-12: LGTM! Consistent migration pattern.Successfully migrated to use
QueryContextandDocumentsValidator, maintaining consistency with other test files.
82-89: LGTM! Proper collection attribute setup.The test correctly modifies the collection's attributes and indexes before validation, demonstrating the flexibility of the new approach.
Also applies to: 91-100
src/Database/QueryContext.php (2)
8-27: LGTM! Clean class structure with well-documented properties.The class provides a clear separation of concerns for managing collections, aliases, and authorization context.
70-86: Authorization skip logic is sound.The implementation correctly prioritizes global authorization status and provides granular collection-permission control.
Note: If this code runs in a multi-threaded environment, consider that
Authorization::$statuscould change between calls, though this is likely not an issue in typical PHP web request contexts.tests/unit/Validator/Query/OrderTest.php (3)
10-15: Correct imports for the new validation architecture.The transition from individual query validators to the unified DocumentsValidator approach is well-implemented.
20-50: Excellent refactoring to use realistic collection schema.The new setUp method creates a proper Document structure with collection metadata, making the tests more representative of actual usage. The QueryContext integration is well-implemented.
52-73: Comprehensive test coverage with improved validation.The tests now validate realistic query arrays and include broader coverage of query types. The updated error message assertions correctly reflect the new validator's behavior.
tests/e2e/Adapter/Scopes/RelationshipTests.php (1)
951-953: Correct adoption of new Query::select API pattern.The changes properly align with the updated
Query::select()method signature that accepts single string arguments. The pattern of using multipleQuery::select()calls for multiple attributes is consistent and correct.The addition of
Query::select('*')ensures comprehensive attribute selection for test validation.Also applies to: 975-976, 991-992, 1007-1008, 1023-1024, 1039-1040, 1055-1056, 1072-1073, 1088-1090, 1107-1107, 1123-1123
tests/unit/Validator/DocumentQueriesTest.php (4)
11-16: Consistent adoption of new validation architecture.The imports and property changes align with the broader refactoring to use QueryContext-based validation.
21-57: Well-structured test setup using QueryContext.The setUp method correctly creates a Document schema and integrates it with QueryContext, maintaining consistency with the new validation approach.
71-71: Correct usage of updated Query::select API.The tests properly use single string arguments with
Query::select(), aligning with the new API signature.Also applies to: 76-76
87-96:✅ Verification successful
Resolve the commented assertion and clarify expected behavior.
The commented assertion with the question "Think what to do about this?" indicates uncertainty about the expected validation behavior. This should be resolved before merging.
Please clarify whether
Query::limit(1)should be considered valid or invalid by the new DocumentsValidator, and update the test accordingly. The current state with a commented assertion and contradictory behavior could lead to incorrect test validation.
🏁 Script executed:
#!/bin/bash # Search for other limit query validations to understand expected behavior rg -A 5 -B 5 "Query::limit" --type phpLength of output: 76084
🏁 Script executed:
#!/bin/bash # Search for DocumentsValidator class definition rg --pretty --heading --line-number "class DocumentsValidator" -A 5 -B 5 --type phpLength of output: 85
🏁 Script executed:
#!/bin/bash # List all PHP files under src that include 'Validator' in their name find src -type f -iname '*Validator.php'Length of output: 42
Limit queries are valid by design
PositiveQuery::limit(...)values are accepted byDocumentsValidator(seetestQueryLimitintests/unit/Validator/QueryTest.php). The existing assertion$this->assertEquals(true, $validator->isValid($queries));correctly reflects this behavior. You can safely remove the commented-out assertion and merge as is.
src/Database/Adapter/Pool.php (1)
259-271: Add a PHPDoc block and reconsider redundant parameters
find()now exposes both the generic$queriesarray and four specialised arrays ($selects,$filters,$joins,$orderQueries).
- With the new signature the specialised arrays fully cover what used to be inside
$queries; keeping both encourages misuse and complicates call-sites.- Unlike the parent abstract method, this override has no PHPDoc – IDEs and static analysers will lose the type information (e.g.
array<Query>generics).Diff sketch:
+ /** + * {@inheritDoc} + * + * @param array<Query> $queries + * @param array<Query> $selects + * @param array<Query> $filters + * @param array<Query> $joins + * @param array<Query> $orderQueries + * @return array<Document> + */ public function find( - QueryContext $context, - array $queries = [], + QueryContext $context, + array $queries = [], // ⚠️ consider deprecating / removing ... ): array {[SUGGEST_ESSENTIAL_REFACTOR]
tests/unit/Validator/QueryTest.php (1)
223-236:orderAsc('')andorderAsc()silently allow an empty attribute – verify this is intentionalAllowing ordering with no column (empty string or
null) may mask configuration mistakes and can generate vendor-specific SQL errors (ORDER BY ""). If the validator intentionally permits it, add an explicit assertion in the validator or a doc-block comment explaining why this is safe; otherwise, mark these two assertions as failures.tests/unit/Validator/Query/FilterTest.php (2)
21-61:$collectionloses required attribute metadataAttributes added with
setAttribute('attributes', …)omitsize,required,signed, etc., even though the validator uses those fields for type checks (e.g. integer bounds). Tests might pass but diverge from production data.Ensure each mock attribute contains the full schema or use the helper that the production code employs to avoid false-positive test results.
92-103: Cursor tests use non-standard$uidkey
Query::cursorAfter/Before()expect aDocumentcarrying the primary key in$id, yet the failure-case assertions here build a document with$uid. If the intent is to fail due to wrong key, add a comment; otherwise, switch to$idto accurately test cursor validation logic.tests/e2e/Adapter/Scopes/DocumentTests.php (7)
19-19: LGTM: Required import for new QueryContext functionality.The import is necessary for the QueryContext usage in the
testEncodeDecodemethod and aligns with the database abstraction refactor.
776-777: Approve the query construction standardization.The change from array-based to individual
Query::select()calls improves API consistency and aligns with the new query construction pattern mentioned in the summary.
794-795: Correct assertion updates for new metadata behavior.The assertions now properly expect
$permissionsand$collectionfields to be present, reflecting the changes in the database adapter that includes these metadata fields by default in selected documents.
2886-2889: Proper implementation of new QueryContext pattern.The code correctly creates a QueryContext, adds the collection metadata, and passes it to the decode method. This aligns with the broader refactor to use QueryContext for managing collections and aliases throughout the database layer.
1062-1063: Consistent query construction standardization.The change maintains consistency with the new query construction pattern using individual
Query::select()calls for each attribute.
1071-1074: Appropriate test assertions for new document structure.The assertions correctly reflect the new document structure where
$collectionis included by default while other fields like$permissionsmay be absent depending on the selection.
3498-3498: Clean implementation for mandatory field selection.The use of
array_mapto createQuery::select()calls for mandatory fields is an elegant functional programming approach that maintains consistency with the new query construction pattern.Also applies to: 3503-3504
src/Database/Adapter/Postgres.php (1)
1157-1166: Verify existence ofgetInternalIds()
createDocuments()calls$this->getInternalIds(...), but the method is not defined in this class. Ensure it exists (likely in the parentSQLclass) and returns the expected mapping, otherwise a fatal error will occur when inserting documents without preset internal IDs.src/Database/Database.php (1)
5826-5836:addSkipAuth()receives filtered id, not the alias
$context->addSkipAuth($this->adapter->filter($_collection->getId()), …)passes a
sanitised collection id whereasaddSkipAuth()is later looked-up through
Query::getAlias()(default alias is_).
If a query uses the default alias, the skip-auth entry will never match, causing
unnecessary permission checks on the main collection.Recommend passing the alias actually used in the query:
- $context->addSkipAuth($this->adapter->filter($_collection->getId()), $forPermission, $skipAuth); + $context->addSkipAuth(Query::DEFAULT_ALIAS, $forPermission, $skipAuth);(or extend
addSkipAuthto accept both).Please review to ensure permission-skipping works as intended for the root collection.
| // | ||
| //namespace Utopia\Database\Validator\Queries; | ||
| // | ||
| //use Exception; | ||
| //use Utopia\Database\Database; | ||
| //use Utopia\Database\Document; | ||
| //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\Validator\Query\Select; | ||
| // | ||
| //class Documents extends IndexedQueries | ||
| //{ | ||
| // /** | ||
| // * Expression constructor | ||
| // * | ||
| // * @param array<mixed> $attributes | ||
| // * @param array<mixed> $indexes | ||
| // * @throws Exception | ||
| // */ | ||
| // public function __construct( | ||
| // array $attributes, | ||
| // array $indexes, | ||
| // int $maxValuesCount = 100, | ||
| // \DateTime $minAllowedDate = new \DateTime('0000-01-01'), | ||
| // \DateTime $maxAllowedDate = new \DateTime('9999-12-31'), | ||
| // ) { | ||
| // $attributes[] = new Document([ | ||
| // '$id' => '$id', | ||
| // 'key' => '$id', | ||
| // 'type' => Database::VAR_STRING, | ||
| // 'array' => false, | ||
| // ]); | ||
| // $attributes[] = new Document([ | ||
| // '$id' => '$internalId', | ||
| // 'key' => '$internalId', | ||
| // 'type' => Database::VAR_STRING, | ||
| // 'array' => false, | ||
| // ]); | ||
| // $attributes[] = new Document([ | ||
| // '$id' => '$createdAt', | ||
| // 'key' => '$createdAt', | ||
| // 'type' => Database::VAR_DATETIME, | ||
| // 'array' => false, | ||
| // ]); | ||
| // $attributes[] = new Document([ | ||
| // '$id' => '$updatedAt', | ||
| // 'key' => '$updatedAt', | ||
| // 'type' => Database::VAR_DATETIME, | ||
| // 'array' => false, | ||
| // ]); | ||
| // | ||
| // $validators = [ | ||
| // new Limit(), | ||
| // new Offset(), | ||
| // new Cursor(), | ||
| // new Filter( | ||
| // $attributes, | ||
| // $maxValuesCount, | ||
| // $minAllowedDate, | ||
| // $maxAllowedDate, | ||
| // ), | ||
| // new Order($attributes), | ||
| // new Select($attributes), | ||
| // ]; | ||
| // | ||
| // parent::__construct($attributes, $indexes, $validators); | ||
| // } | ||
| //} |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider removing commented code instead of keeping it.
The entire Documents validator class has been commented out as part of the migration to the new V2 validator system. While this approach might be useful during development, keeping large blocks of commented code can lead to:
- Code bloat and reduced readability
- Confusion about what code is actually active
- Maintenance overhead
Consider removing this commented code if the migration is complete and stable. If you need to keep it temporarily, add a clear comment explaining the timeline for removal.
🏁 Script executed:
#!/bin/bash
# Description: Verify that the Documents validator is no longer used anywhere in the codebase
# Expected: Only references should be in tests and the new V2 validator
echo "Searching for remaining usage of Documents validator..."
rg -A 3 "use.*Validator.*Documents" --type php
echo ""
echo "Searching for 'new Documents' instantiations..."
rg -A 3 "new Documents" --type php
echo ""
echo "Checking if V2 validator is properly replacing Documents..."
rg -A 3 "DocumentsValidator.*V2" --type phpLength of output: 12282
Remove the commented-out Documents validator class
The src/Database/Validator/Queries/Documents.php file is fully commented out and no longer referenced—tests and runtime use the V2 validator (aliased as DocumentsValidator). Keeping it only adds dead code and potential confusion.
• Delete src/Database/Validator/Queries/Documents.php from the repository
• If you need a temporary fallback during migration, add a clear deprecation comment at the top with a removal timeline (e.g. “to be deleted in vX.Y”)
🤖 Prompt for AI Agents
In src/Database/Validator/Queries/Documents.php from lines 3 to 74, the entire
Documents validator class is commented out and no longer used, which adds dead
code and reduces clarity. Remove this file completely from the repository to
eliminate unused code. If you need to keep it temporarily during migration, add
a clear deprecation comment at the top specifying when it will be removed.
| // | ||
| //namespace Utopia\Database\Validator\Query; | ||
| // | ||
| //use Utopia\Database\Document; | ||
| //use Utopia\Database\Query; | ||
| // | ||
| //class Order extends Base | ||
| //{ | ||
| // /** | ||
| // * @var array<int|string, mixed> | ||
| // */ | ||
| // protected array $schema = []; | ||
| // | ||
| // /** | ||
| // * @param array<Document> $attributes | ||
| // */ | ||
| // public function __construct(array $attributes = []) | ||
| // { | ||
| // foreach ($attributes as $attribute) { | ||
| // $this->schema[$attribute->getAttribute('key', $attribute->getAttribute('$id'))] = $attribute->getArrayCopy(); | ||
| // } | ||
| // } | ||
| // | ||
| // /** | ||
| // * @param string $attribute | ||
| // * @return bool | ||
| // */ | ||
| // protected function isValidAttribute(string $attribute): bool | ||
| // { | ||
| // // Search for attribute in schema | ||
| // if (!isset($this->schema[$attribute])) { | ||
| // $this->message = 'Attribute not found in schema: ' . $attribute; | ||
| // return false; | ||
| // } | ||
| // | ||
| // return true; | ||
| // } | ||
| // | ||
| // /** | ||
| // * Is valid. | ||
| // * | ||
| // * Returns true if method is ORDER_ASC or ORDER_DESC and attributes are valid | ||
| // * | ||
| // * Otherwise, returns false | ||
| // * | ||
| // * @param Query $value | ||
| // * @return bool | ||
| // */ | ||
| // public function isValid($value): bool | ||
| // { | ||
| // if (!$value instanceof Query) { | ||
| // return false; | ||
| // } | ||
| // | ||
| // $method = $value->getMethod(); | ||
| // $attribute = $value->getAttribute(); | ||
| // | ||
| // if ($method === Query::TYPE_ORDER_ASC || $method === Query::TYPE_ORDER_DESC) { | ||
| // if ($attribute === '') { | ||
| // return true; | ||
| // } | ||
| // return $this->isValidAttribute($attribute); | ||
| // } | ||
| // | ||
| // return false; | ||
| // } | ||
| // | ||
| // public function getMethodType(): string | ||
| // { | ||
| // return self::METHOD_TYPE_ORDER; | ||
| // } | ||
| //} |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify that the new DocumentsValidator provides equivalent order validation functionality.
The entire Order validator class has been commented out as part of the migration to the new DocumentsValidator and QueryContext system. Ensure that:
- Order query validation is properly handled by the new
DocumentsValidatorclass - All existing order validation logic (schema attribute checking, order type validation) is preserved
- Error messages for invalid order queries remain informative
- All tests pass with the new validation system
🏁 Script executed:
#!/bin/bash
# Description: Verify that order query validation is handled by the new DocumentsValidator
# Search for DocumentsValidator usage and order validation logic
ast-grep --pattern $'class DocumentsValidator {
$$$
}'
# Check for order-related validation in the new validator
rg -A 10 -B 5 "order|Order|ORDER_ASC|ORDER_DESC" src/Database/Validator/Queries/V2.php
# Look for tests that verify order query validation still works
rg -A 5 -B 5 "order.*valid" tests/Length of output: 2318
Let’s inspect how validateAttributeExist handles empty attributes and find any new tests for order queries:
🏁 Script executed:
#!/bin/bash
# Locate validateAttributeExist implementation
rg -n "function validateAttributeExist" -n src/Database/Validator/Queries
# Show its full context
rg -A 10 -B 2 "function validateAttributeExist" -n src/Database/Validator/Queries
# Search for any tests covering Query::orderAsc or ::orderDesc
rg -n "orderAsc" -n tests
rg -n "orderDesc" -n testsLength of output: 10672
Inspecting the full validateAttributeExist implementation and class context:
🏁 Script executed:
#!/bin/bash
# Show class declaration and context
sed -n '1,80p' src/Database/Validator/Queries/V2.php
# Print the complete validateAttributeExist method
rg -A 20 -B 2 "protected function validateAttributeExist" -n src/Database/Validator/Queries/V2.phpLength of output: 3434
Implement missing attribute-existence checks in V2.validateAttributeExist
The new validateAttributeExist method in src/Database/Validator/Queries/V2.php is currently a stub (with only a var_dump and commented‐out logic) and does not throw on unknown attributes—breaking the old Order validator’s schema checks.
Please:
- Complete
validateAttributeExist(string $attributeId, string $alias): void(around lines 372–383):- Remove the
var_dump - Look up
$attributeIdin the cloned schema from$this->context - Throw an
Exceptionwith a clear message when the attribute isn’t found
- Remove the
- Mirror the original
isValidAttributebehavior (including empty‐string handling) - Ensure the following tests still pass or are updated:
tests/unit/Validator/Query/OrderTest.php- Any e2e Document/Joins tests covering
Query::orderAscandQuery::orderDesc
- Verify error messages remain informative for invalid order queries
🤖 Prompt for AI Agents
In src/Database/Validator/Queries/V2.php around lines 372 to 383, the
validateAttributeExist method is incomplete and currently only contains a
var_dump. Remove the var_dump and implement logic to check if the given
attributeId exists in the cloned schema from $this->context. If the attribute is
not found or is an empty string, throw an Exception with a clear message
indicating the attribute is not found, replicating the original isValidAttribute
behavior from the old Order validator. After implementing this, ensure that all
existing tests related to order validation, including
tests/unit/Validator/Query/OrderTest.php and any e2e tests covering
Query::orderAsc and Query::orderDesc, pass without errors and that error
messages for invalid order queries remain informative.
| Query::select('*'), | ||
| Query::select('albums.name') |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify that multi-field selections work correctly with the new pattern.
The change from Query::select(['*', 'albums.name']) to separate Query::select('*') and Query::select('albums.name') calls should preserve the same functionality. Ensure that the order of these calls doesn't matter for the query execution.
Let me verify that the new query pattern maintains the same behavior:
🏁 Script executed:
#!/bin/bash
# Description: Check if there are any tests that validate the order independence of Query::select() calls
# Search for test cases that use multiple Query::select() calls to ensure order doesn't matter
rg -B 5 -A 10 "Query::select\('.*'\).*Query::select\('.*'\)" tests/Length of output: 1365
Add tests to verify order independence of multi-field Query::select calls
I didn’t find any existing tests that exercise calling Query::select('albums.name') before Query::select('*'). To ensure the new pattern truly preserves functionality regardless of call order, please:
- In
tests/e2e/Adapter/Scopes/Relationships/OneToManyTests.php(or a new e2e file), add a scenario where you reverse the selects:and assert that the resulting SQL (or fetched data) matches the original order.Query::select('albums.name'), Query::select('*'),
- In
tests/unit/Validator/Query/SelectTest.php(or a new unit test), add a case for:to mirror the reversed ordering in the validator.$this->assertTrue($this->validator->isValid([Query::select('attr'), Query::select('*')]));
These additions will confirm that splitting the array into separate select() calls does not introduce any order-dependent behavior.
🤖 Prompt for AI Agents
In tests/e2e/Adapter/Scopes/Relationships/OneToManyTests.php around lines
147-148, add a new test scenario that reverses the order of Query::select calls
to Query::select('albums.name') followed by Query::select('*') and assert that
the resulting SQL or fetched data matches the original order to verify order
independence. Additionally, in tests/unit/Validator/Query/SelectTest.php, add a
unit test case asserting that the validator accepts the reversed order of
selects, such as Query::select('attr') followed by Query::select('*'), to ensure
the validator also treats the order as irrelevant.
|
|
||
| class AsQuery extends Validator | ||
| { | ||
| protected string $message = '"as" must contain at most 64 chars. Valid chars are a-z, A-Z, 0-9, and underscore.'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix the inconsistency between error message and validation logic.
The error message claims "at most 64 chars" but the validation logic mb_strlen($value) >= 64 actually enforces a 63-character limit.
- protected string $message = '"as" must contain at most 64 chars. Valid chars are a-z, A-Z, 0-9, and underscore.';
+ protected string $message = '"as" must contain at most 63 chars. Valid chars are a-z, A-Z, 0-9, and underscore.';Or if 64 characters should be allowed:
- if (\mb_strlen($value) >= 64) {
+ if (\mb_strlen($value) > 64) {
return false;
}Also applies to: 50-52
🤖 Prompt for AI Agents
In src/Database/Validator/AsQuery.php at line 9 and also lines 50-52, the error
message states that the "as" value must contain at most 64 characters, but the
validation logic uses a condition that enforces a maximum length of 63
characters (mb_strlen($value) >= 64). To fix this inconsistency, either update
the validation logic to allow 64 characters by changing the condition to
mb_strlen($value) > 64, or revise the error message to say "at most 63 chars" to
match the current validation. Ensure both the message and validation logic are
aligned.
| if($this->attribute === '*'){ | ||
| $this->message = 'Invalid "as" on attribute "*"'; | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Fix the logic error in the wildcard attribute check.
The condition checks $this->attribute === '*' but should check $value === '*' since you're validating the alias value, not the attribute name.
- if($this->attribute === '*'){
+ if($value === '*'){
$this->message = 'Invalid "as" on attribute "*"';
return false;
}🤖 Prompt for AI Agents
In src/Database/Validator/AsQuery.php around lines 54 to 57, the condition
incorrectly checks if $this->attribute equals '*', but it should check if $value
equals '*' since the validation is for the alias value. Update the condition to
compare $value to '*' instead of $this->attribute.
| 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/validate getRightAlias() to prevent SQL-injection via aliases
$aliasRight = $this->quote($query->getRightAlias()); quotes but does not pass through $this->filter() unlike the left alias. A malicious alias ("; DROP TABLE …) injected upstream would still be accepted inside the quotes.
-$aliasRight = $this->quote($query->getRightAlias());
+$aliasRight = $this->quote($this->filter($query->getRightAlias()));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| case Query::TYPE_RELATION_EQUAL: | |
| $attributeRight = $this->quote($this->filter($query->getAttributeRight())); | |
| $aliasRight = $this->quote($query->getRightAlias()); | |
| return "{$alias}.{$attribute}={$aliasRight}.{$attributeRight}"; | |
| case Query::TYPE_RELATION_EQUAL: | |
| $attributeRight = $this->quote($this->filter($query->getAttributeRight())); | |
| $aliasRight = $this->quote($this->filter($query->getRightAlias())); | |
| return "{$alias}.{$attribute}={$aliasRight}.{$attributeRight}"; |
🤖 Prompt for AI Agents
In src/Database/Adapter/Postgres.php around lines 1934 to 1939, the alias
obtained from getRightAlias() is only quoted but not filtered, which risks SQL
injection via malicious aliases. To fix this, pass the result of getRightAlias()
through the filter() method before quoting it, similar to how the left alias is
handled. This ensures the alias is properly validated and sanitized before use
in the SQL statement.
| $values = $query->getValues(); | ||
| foreach ($values as $valueIndex => $value) { | ||
| try { | ||
| $values[$valueIndex] = DateTime::setTimezone($value); | ||
| } catch (\Throwable $e) { | ||
| throw new QueryException($e->getMessage(), $e->getCode(), $e); |
There was a problem hiding this comment.
Potential undefined class reference – qualify DateTime::setTimezone()
DateTime::setTimezone() is invoked without a leading back-slash or use import.
Inside the Utopia\Database namespace this resolves to Utopia\Database\DateTime.
If the helper lives in Utopia\Database\Helpers\DateTime, this call will fatally error
(Class "Utopia\Database\DateTime" not found) at runtime.
- $values[$valueIndex] = DateTime::setTimezone($value);
+ $values[$valueIndex] = \Utopia\Database\Helpers\DateTime::setTimezone($value);Either add an explicit use Utopia\Database\Helpers\DateTime; at the top of the file
or prefix the call with its fully-qualified name.
Please verify the correct namespace before shipping.
🤖 Prompt for AI Agents
In src/Database/Database.php around lines 6606 to 6611, the call to
DateTime::setTimezone() is unqualified and resolves incorrectly to
Utopia\Database\DateTime, causing a fatal error. Fix this by either adding a
'use Utopia\Database\Helpers\DateTime;' statement at the top of the file or
prefixing the call with the fully qualified class name
'\Utopia\Database\Helpers\DateTime::setTimezone()' to ensure the correct class
is referenced.
| /** | ||
| * Auth requires $permissions | ||
| */ | ||
| //$selects[] = Query::select('$id'); // Do we need this? | ||
| //$selects[] = Query::select('$permissions', system: true); | ||
| $queries = Query::addSelect($queries, Query::select('$permissions', system: true)); | ||
| // $queries = Query::add($queries, Query::select('$id')); |
There was a problem hiding this comment.
$id must always be included in the projection
populateDocumentRelationships() and several other downstream code paths rely on Document::getId() being populated.
Currently we only force–add $permissions; $id (and $collection) are commented-out.
If the caller supplies a selective projection without $id, the returned Document
will have an empty id → broken relationship resolution, cache keys, conflict detection, etc.
-//$selects[] = Query::select('$id'); // Do we need this?
-//$selects[] = Query::select('$permissions', system: true);
-$queries = Query::addSelect($queries, Query::select('$permissions', system: true));
+// Always project internal columns required by the runtime
+$queries = Query::addSelect($queries, Query::select('$id', system: true));
+$queries = Query::addSelect($queries, Query::select('$permissions', system: true));
+// Optionally expose $collection as well – several callers assume its presence.
+// $queries = Query::addSelect($queries, Query::select('$collection', system: true));Without this, any call such as getDocument('posts', 'abc', [Query::select('title')])
returns a Document whose $id === '', silently breaking logic.
This is a correctness issue – please restore the mandatory $id select.
📝 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.
| /** | |
| * Auth requires $permissions | |
| */ | |
| //$selects[] = Query::select('$id'); // Do we need this? | |
| //$selects[] = Query::select('$permissions', system: true); | |
| $queries = Query::addSelect($queries, Query::select('$permissions', system: true)); | |
| // $queries = Query::add($queries, Query::select('$id')); | |
| /** | |
| * Auth requires $permissions | |
| */ | |
| // Always project internal columns required by the runtime | |
| $queries = Query::addSelect($queries, Query::select('$id', system: true)); | |
| $queries = Query::addSelect($queries, Query::select('$permissions', system: true)); | |
| // Optionally expose $collection as well – several callers assume its presence. | |
| // $queries = Query::addSelect($queries, Query::select('$collection', system: true)); | |
| // $queries = Query::add($queries, Query::select('$id')); |
🤖 Prompt for AI Agents
In src/Database/Database.php around lines 3020 to 3026, the code currently only
forces the addition of the $permissions field in the query projection, while $id
is commented out. To fix the issue where Document::getId() is empty and breaks
downstream logic, ensure that $id is always included in the projection by
uncommenting and restoring the line that adds $id to the selects. This
guarantees that $id is present in all query results, maintaining correct
relationship resolution and other dependent functionality.
| public const TYPE_RELATION_EQUAL = 'relationEqual'; | ||
|
|
There was a problem hiding this comment.
New query types are missing from TYPES and isMethod() – breaks JSON parsing/validation
TYPE_RELATION_EQUAL, TYPE_INNER_JOIN, TYPE_LEFT_JOIN, and TYPE_RIGHT_JOIN were added as constants, but
- they are not appended to the
TYPESwhitelist, and isMethod()still returnsfalsefor them.
As a result, any join / relation-equality query coming from JSON (or being re-serialised) will be rejected with “Invalid query method”. This blocks round-tripping through Query::parse() / toString() and can surface as runtime 400 errors from the SDK.
@@ public const TYPES = [
self::TYPE_ENDS_WITH,
+ self::TYPE_RELATION_EQUAL,
+ self::TYPE_INNER_JOIN,
+ self::TYPE_LEFT_JOIN,
+ self::TYPE_RIGHT_JOIN,
@@
];
...
@@ public static function isMethod(string $value): bool
self::TYPE_SELECT => true,
+ self::TYPE_RELATION_EQUAL,
+ self::TYPE_INNER_JOIN,
+ self::TYPE_LEFT_JOIN,
+ self::TYPE_RIGHT_JOIN => true,
default => false,Also applies to: 45-51, 54-77, 321-348
🤖 Prompt for AI Agents
In src/Database/Query.php around lines 25-26, 45-51, 54-77, and 321-348, the new
query type constants TYPE_RELATION_EQUAL, TYPE_INNER_JOIN, TYPE_LEFT_JOIN, and
TYPE_RIGHT_JOIN are missing from the TYPES whitelist array and the isMethod()
function. To fix this, add these constants to the TYPES array to whitelist them
and update isMethod() to return true for these new types, ensuring JSON parsing
and validation accept these query methods without errors.
| public static function addSelect(array $queries, Query $query): array | ||
| { | ||
| $merge = true; | ||
| $found = false; | ||
|
|
||
| foreach ($queries as $q) { | ||
| if ($q->getMethod() === self::TYPE_SELECT){ | ||
| $found = true; | ||
|
|
||
| if ($q->getAlias() === $query->getAlias()){ | ||
| if ($q->getAttribute() === '*'){ | ||
| $merge = false; | ||
| } | ||
|
|
||
| if ($q->getAttribute() === $query->getAttribute()){ | ||
| if ($q->getAs() === $query->getAs()){ | ||
| $merge = false; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if ($found && $merge){ | ||
| $queries = [ | ||
| ...$queries, | ||
| $query | ||
| ]; | ||
| } | ||
|
|
||
| return $queries; | ||
| } |
There was a problem hiding this comment.
addSelect() never inserts when the list is empty
When $queries contains no existing TYPE_SELECT, $found stays false, so the new select is silently discarded. This prevents callers from building the initial projection list.
-if ($found && $merge){
+if ((! $found) || $merge) {
$queries = [
...$queries,
$query
];
}Also consider early-returning if $merge is false to avoid needless array spread.
📝 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 static function addSelect(array $queries, Query $query): array | |
| { | |
| $merge = true; | |
| $found = false; | |
| foreach ($queries as $q) { | |
| if ($q->getMethod() === self::TYPE_SELECT){ | |
| $found = true; | |
| if ($q->getAlias() === $query->getAlias()){ | |
| if ($q->getAttribute() === '*'){ | |
| $merge = false; | |
| } | |
| if ($q->getAttribute() === $query->getAttribute()){ | |
| if ($q->getAs() === $query->getAs()){ | |
| $merge = false; | |
| } | |
| } | |
| } | |
| } | |
| } | |
| if ($found && $merge){ | |
| $queries = [ | |
| ...$queries, | |
| $query | |
| ]; | |
| } | |
| return $queries; | |
| } | |
| public static function addSelect(array $queries, Query $query): array | |
| { | |
| $merge = true; | |
| $found = false; | |
| foreach ($queries as $q) { | |
| if ($q->getMethod() === self::TYPE_SELECT){ | |
| $found = true; | |
| if ($q->getAlias() === $query->getAlias()){ | |
| if ($q->getAttribute() === '*'){ | |
| $merge = false; | |
| } | |
| if ($q->getAttribute() === $query->getAttribute()){ | |
| if ($q->getAs() === $query->getAs()){ | |
| $merge = false; | |
| } | |
| } | |
| } | |
| } | |
| } | |
| // add when there are no existing selects, or when merge is still true | |
| if ((! $found) || $merge) { | |
| $queries = [ | |
| ...$queries, | |
| $query | |
| ]; | |
| } | |
| return $queries; | |
| } |
🤖 Prompt for AI Agents
In src/Database/Query.php around lines 1083 to 1114, the addSelect() method
fails to add the new query if the $queries array has no existing TYPE_SELECT
queries because $found remains false, causing the new select to be discarded.
Fix this by adding the new query when $found is false, ensuring the initial
projection list can be built. Also, implement an early return when $merge is
false to avoid unnecessary array spreading and improve performance.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
tests/e2e/Adapter/Scopes/JoinsTests.php (2)
332-334: Debugvar_dump()left in committed tests
var_dump($document);will spam test output on every run. Please remove.
375-377: Intentionally failing assertion & strayvar_dump()
Both thevar_dump($document);and$this->assertEquals('dsdsd', 'ds');should be deleted or replaced with meaningful checks – they break the suite on purpose.
🧹 Nitpick comments (1)
tests/e2e/Adapter/Scopes/JoinsTests.php (1)
489-564: Large block of commented-out test code clutters the file
The ~75 lines of commented tests make maintenance harder and hide the actual coverage. Either restore them as active tests (fixed) or drop them entirely.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Database/Adapter/SQL.php(6 hunks)tests/e2e/Adapter/Scopes/JoinsTests.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Database/Adapter/SQL.php
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (7)
src/Database/Adapter/MariaDB.php (2)
63-63: Debug output still present after previous review.This debug statement was already flagged in previous reviews but remains in the code. Remove the
var_dump($sql)call to prevent leaking internal DDL statements in production.
1848-1849: Debug output still present after previous review.These debug statements were already flagged in previous reviews but remain in the code. Remove the
echoandvar_dumpcalls to prevent exposing query strings and bindings in production.src/Database/Database.php (2)
6602-6607: Still unresolved: Qualify the DateTime class referenceThe previous review comment about
DateTime::setTimezone()requiring proper namespace qualification remains unaddressed. This will cause a fatal error at runtime.Apply this fix:
- $values[$valueIndex] = DateTime::setTimezone($value); + $values[$valueIndex] = \Utopia\Database\Helpers\DateTime::setTimezone($value);
3021-3027: Critical: $id is still commented out despite past reviewThe previous review comment correctly identified that
$idmust always be included in projections, but it remains commented out. This will break downstream functionality that relies onDocument::getId().Restore the mandatory
$idselect:-//$selects[] = Query::select('$id'); // Do we need this? -//$selects[] = Query::select('$permissions', system: true); -$queries = Query::addSelect($queries, Query::select('$permissions', system: true)); +// Always project internal columns required by the runtime +$queries = Query::addSelect($queries, Query::select('$id', system: true)); +$queries = Query::addSelect($queries, Query::select('$permissions', system: true));src/Database/Query.php (3)
54-77: Critical: Missing new query types in TYPES array.The new query types (
TYPE_RELATION_EQUAL,TYPE_INNER_JOIN,TYPE_LEFT_JOIN,TYPE_RIGHT_JOIN) are missing from theTYPESwhitelist array, which will cause JSON parsing and validation to fail.public const TYPES = [ self::TYPE_EQUAL, self::TYPE_NOT_EQUAL, self::TYPE_LESSER, self::TYPE_LESSER_EQUAL, self::TYPE_GREATER, self::TYPE_GREATER_EQUAL, self::TYPE_CONTAINS, self::TYPE_SEARCH, self::TYPE_IS_NULL, self::TYPE_IS_NOT_NULL, self::TYPE_BETWEEN, self::TYPE_STARTS_WITH, self::TYPE_ENDS_WITH, + self::TYPE_RELATION_EQUAL, self::TYPE_SELECT, self::TYPE_ORDER_DESC, self::TYPE_ORDER_ASC, self::TYPE_LIMIT, self::TYPE_OFFSET, self::TYPE_CURSOR_AFTER, self::TYPE_CURSOR_BEFORE, self::TYPE_AND, self::TYPE_OR, + self::TYPE_INNER_JOIN, + self::TYPE_LEFT_JOIN, + self::TYPE_RIGHT_JOIN, ];
321-348: Critical: Missing new query types in isMethod() function.The
isMethod()function is missing the new query types, which will cause validation failures when these query types are used.public static function isMethod(string $value): bool { return match ($value) { self::TYPE_EQUAL, self::TYPE_NOT_EQUAL, self::TYPE_LESSER, self::TYPE_LESSER_EQUAL, self::TYPE_GREATER, self::TYPE_GREATER_EQUAL, self::TYPE_CONTAINS, self::TYPE_SEARCH, self::TYPE_ORDER_ASC, self::TYPE_ORDER_DESC, self::TYPE_LIMIT, self::TYPE_OFFSET, self::TYPE_CURSOR_AFTER, self::TYPE_CURSOR_BEFORE, self::TYPE_IS_NULL, self::TYPE_IS_NOT_NULL, self::TYPE_BETWEEN, self::TYPE_STARTS_WITH, self::TYPE_ENDS_WITH, self::TYPE_OR, self::TYPE_AND, + self::TYPE_RELATION_EQUAL, + self::TYPE_INNER_JOIN, + self::TYPE_LEFT_JOIN, + self::TYPE_RIGHT_JOIN, self::TYPE_SELECT => true, default => false, }; }
1076-1107: Critical: addSelect() method fails to add queries when list is empty.The method has a logic error where it only adds the new query if an existing
TYPE_SELECTis found AND$mergeis true. This prevents adding the first select query to an empty list.-if ($found && $merge){ +if ((!$found) || $merge) { $queries = [ ...$queries, $query ]; }
🧹 Nitpick comments (5)
src/Database/Adapter/MariaDB.php (1)
1983-1983: Remove duplicate variable assignment.The
$aliasvariable is assigned twice consecutively. Remove the duplicate assignment for cleaner code.- $alias = Query::DEFAULT_ALIAS;src/Database/Database.php (1)
6614-6635: Clean separation in query conversion logicThe separation between collections and context provides good organization, though the exception throwing could be more informative about which alias is unknown.
Consider improving the error message:
- throw new \Exception('Unknown Alias context'); + throw new \Exception("Unknown alias '{$query->getAlias()}' in query context");src/Database/Query.php (3)
29-29: Remove the commented out code.The commented
TYPE_SELECTIONconstant should be removed to keep the code clean.- //public const TYPE_SELECTION = 'selection';
590-593: Remove unused parameter from select method.The
$functionparameter is declared but never used in the method.-public static function select(string $attribute, string $alias = '', string $as = '', string $function = '', bool $system = false): self +public static function select(string $attribute, string $alias = '', string $as = '', bool $system = false): self
936-937: Remove unused variable.The
$joinsvariable is declared but never used in thegroupByTypemethod.public static function groupByType(array $queries): array { $filters = []; - $joins = []; $selections = [];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
src/Database/Adapter.php(2 hunks)src/Database/Adapter/MariaDB.php(9 hunks)src/Database/Database.php(44 hunks)src/Database/Query.php(20 hunks)tests/e2e/Adapter/Base.php(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/e2e/Adapter/Base.php
- src/Database/Adapter.php
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/Database/Adapter/MariaDB.php (7)
src/Database/Database.php (3)
Database(33-6686)find(5804-5962)getCollection(1363-1379)src/Database/QueryContext.php (3)
QueryContext(8-89)getCollections(32-35)skipAuth(75-86)src/Database/Adapter/Pool.php (4)
quote(119-122)find(259-273)getTenantQuery(485-488)getAttributeProjection(460-463)src/Database/Query.php (8)
limit(623-626)offset(634-637)Query(8-1109)getAlias(204-207)getAttribute(182-185)join(717-720)getCollection(224-227)getValues(190-193)src/Database/Validator/Authorization.php (2)
Authorization(7-225)getRoles(101-104)src/Database/Adapter/SQL.php (7)
getInternalKeyForAttribute(1698-1710)getSQLOperator(1304-1330)getSQLPermissionsCondition(1370-1390)getSQLTable(1399-1402)getSQLConditions(1482-1497)getTenantQuery(1517-1549)getAttributeProjection(1595-1640)src/Database/Adapter/SQLite.php (2)
getSQLPermissionsCondition(1032-1042)getSQLTable(1050-1053)
src/Database/Query.php (3)
src/Database/QueryContext.php (1)
__construct(25-27)src/Database/Database.php (3)
__construct(380-457)getCollection(1363-1379)Database(33-6686)src/Database/Exception/Query.php (1)
Query(7-9)
🪛 PHPMD (2.15.0)
src/Database/Query.php
590-590: Avoid unused parameters such as '$function'. (Unused Code Rules)
(UnusedFormalParameter)
936-936: Avoid unused local variables such as '$joins'. (Unused Code Rules)
(UnusedLocalVariable)
🔇 Additional comments (34)
src/Database/Adapter/MariaDB.php (6)
18-18: LGTM: Clean import addition for QueryContext.The QueryContext import is properly added and necessary for the updated find() method signature.
1014-1014: Good: Standardized column quoting.Using the
quote()method instead of manual backticks improves consistency and security across the adapter.
1676-1704: Excellent: Major architectural improvement to support QueryContext and joins.The updated method signature properly separates different query types (selects, filters, joins, orders) and introduces QueryContext for better alias and collection management. This enables more sophisticated query construction with join support.
1721-1777: Sophisticated cursor pagination implementation.The logic correctly handles multi-attribute cursor pagination with tie-break conditions and includes a performance optimization for single unique key scenarios. The direction reversal for CURSOR_BEFORE is properly implemented.
1783-1799: Robust JOIN implementation with proper security checks.The INNER JOIN logic correctly implements per-join permission checks, respects skipAuth flags from QueryContext, and maintains tenant isolation. The use of getSQLConditions for join conditions ensures consistency with filter handling.
2051-2089: Good: Added support for relation queries in JOIN conditions.The updates properly handle
TYPE_RELATION_EQUALqueries for generating JOIN ON clauses. The use of query-specific aliases and proper attribute mapping enables flexible join conditions between different table aliases.src/Database/Database.php (17)
3037-3038: Good architectural improvement with QueryContextThe introduction of
QueryContextprovides a clean abstraction for managing collections and aliases, improving the separation of concerns.
3041-3041: Consistent validator usage with new V2 systemThe migration to
DocumentsValidatorwithQueryContextprovides better schema-aware validation compared to the previous approach.
3054-3082: Robust handling of nested selections in relationshipsThe logic for processing nested selections based on relationship types (many-to-many, one-to-many vs many-to-one, one-to-one) is well thought out and handles the different projection requirements correctly.
3148-3149: Consistent parameter passing to decode methodsThe updated method signatures for
casting()anddecode()now properly acceptQueryContextand selects, enabling attribute filtering and alias resolution.
3533-3536: Verify QueryContext initialization in createDocumentThe QueryContext creation and population looks correct for single collection operations.
4215-4217: Improved validation with schema awarenessThe new validator accepts QueryContext and supports complex nested queries, which is a significant improvement over the previous validation approach.
4232-4240: Enhanced cursor handling with proper validationThe cursor document validation ensuring it belongs to the same collection adds important data integrity checks.
5812-5835: Comprehensive authorization handling for joinsThe enhanced authorization logic that considers document security and permissions per collection within the context is well-implemented and maintains proper access control.
5853-5853: Verify convertQueries method implementationThe modularization of query conversion into static methods improves code organization and reusability.
5860-5942: Solid adapter integration with new query structureThe adapter method signature changes to accept QueryContext and explicit arrays of selects, filters, joins, and orders provide better separation of concerns and enable more sophisticated query processing.
5976-5982: Appropriate restriction on cursor directionDisallowing "before" direction in
foreachis a reasonable limitation that simplifies the implementation while covering the most common use cases.
6074-6075: QueryContext properly initialized for count operationsThe QueryContext setup for count operations follows the established pattern correctly.
6097-6097: Clean separation of concerns in query filteringExtracting only filter queries for count operations maintains the method's focus while working with the new query infrastructure.
6675-6676: Improved cache key generationSerializing the selects array before hashing provides more reliable cache key generation compared to simpler approaches.
6254-6341: Enhanced decode method with alias supportThe decode method now properly handles aliased attributes and QueryContext, enabling correct attribute resolution in join scenarios. The logic for handling
$permsand attribute mapping is well-structured.
6352-6458: Consistent casting implementationThe casting method mirrors the decode method's approach with QueryContext and alias handling, maintaining consistency across the codebase.
3031-3035: No changes needed:getDocumentis intentionally restricted to select-only queries
ThegetDocumentmethod is designed for projecting specific fields viaQuery::selectonly. All existing call sites pass onlyQuery::select(...), and any filtering or sorting should be done throughfind(). You can safely ignore the suggestion to allow other query types here.Likely an incorrect or invalid review comment.
src/Database/Query.php (11)
25-26: New query type constant added correctly.The
TYPE_RELATION_EQUALconstant is properly defined and will be used for join conditions.
45-51: Join type constants added correctly.The join type constants are properly defined and follow the existing naming convention.
84-102: FILTER_TYPES array correctly includes TYPE_RELATION_EQUAL.The
FILTER_TYPESarray properly includes the newTYPE_RELATION_EQUALconstant along with existing filter types.
104-110: New properties added correctly.The new properties support the enhanced functionality for joins and aliasing. The property declarations are properly typed and follow the existing conventions.
125-160: Enhanced constructor with proper parameter handling.The constructor correctly handles the new parameters with appropriate default values and fallback logic for aliases.
204-228: New getter methods implemented correctly.The getter methods follow the existing pattern and provide access to the new properties.
255-289: Direction getter methods with proper validation.The
getCursorDirection()andgetOrderDirection()methods correctly validate the query type before returning the direction, throwing exceptions for invalid usage.
477-615: Static factory methods enhanced with alias support.The existing static factory methods have been properly enhanced to accept optional alias parameters while maintaining backward compatibility.
711-759: New join and relation factory methods implemented correctly.The join factory methods (
join,innerJoin,leftJoin,rightJoin) andrelationEqualmethod are properly implemented with correct parameter handling.
767-916: Query filtering methods implemented correctly.The new static methods to filter queries by type are well-implemented and follow consistent patterns. The methods properly handle cloning to avoid side effects.
1032-1069: New utility methods implemented correctly.The
isJoin(),isFilter(), andisSystem()methods are properly implemented with correct logic and type checking.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
src/Database/Database.php (1)
6260-6458: Significant code duplication between decode() and casting() methodsBoth
decode()andcasting()methods share nearly identical logic for:
- Building internals and schema arrays
- Iterating through document attributes
- Resolving aliases from selects
- Looking up attributes in collections
This violates the DRY principle and makes maintenance harder.
Consider extracting the common logic into a shared helper method:
private function processDocumentWithContext( QueryContext $context, Document $document, array $selects, callable $attributeProcessor ): Document { $internals = []; $schema = []; foreach (Database::INTERNAL_ATTRIBUTES as $attribute) { $internals[$attribute['$id']] = $attribute; } foreach ($context->getCollections() as $collection) { foreach ($collection->getAttribute('attributes', []) as $attribute) { $key = $attribute->getAttribute('key', $attribute->getAttribute('$id')); $key = $this->adapter->filter($key); $schema[$collection->getId()][$key] = $attribute->getArrayCopy(); } } $new = new Document(); foreach ($document as $key => $value) { if($key === '$perms'){ $new->setAttribute($key, $value); continue; } // Alias resolution logic... $result = $this->resolveAttributeContext($key, $selects, $context, $internals, $schema); if ($result === null) { continue; } [$attributeKey, $attribute, $alias] = $result; // Process the attribute value $processedValue = $attributeProcessor($attribute, $value, $attributeKey); $new->setAttribute($attributeKey, $processedValue); } return $new; }Then refactor both methods to use this shared logic with specific attribute processors.
♻️ Duplicate comments (2)
src/Database/Database.php (2)
3024-3026: Missing $id in projection needs to be addressedThe
$idattribute should always be included in the projection to ensure proper document functionality. Uncommenting the line that adds$idto selects is critical for relationship resolution and other downstream operations.
6605-6606: Unqualified DateTime::setTimezone() call needs namespace qualificationThe
DateTime::setTimezone()call is unqualified and will resolve incorrectly within theUtopia\Databasenamespace.
🧹 Nitpick comments (2)
src/Database/Database.php (2)
3037-3038: Consider extracting QueryContext initialization into a helper methodThe pattern of creating and populating
QueryContextis repeated across multiple methods. Consider extracting this into a helper method to ensure consistency and reduce code duplication.Example helper method:
private function createQueryContext(Document|string $collection): QueryContext { $context = new QueryContext(); if (is_string($collection)) { $collection = $this->silent(fn () => $this->getCollection($collection)); } $context->add($collection); return $context; }Also applies to: 3533-3535, 3579-3581, 4160-4162, 4215-4217
4233-4240: Extract duplicated cursor validation logicThe cursor validation logic checking if the cursor document belongs to the same collection is duplicated. Consider extracting this into a reusable method.
+private function validateCursor(?Query $cursorQuery, Document $collection): Document { + if (is_null($cursorQuery)) { + return new Document(); + } + + $cursor = $cursorQuery->getCursorDocument($cursorQuery); + if ($cursor->getCollection() !== $collection->getId()) { + throw new DatabaseException("cursor Document must be from the same Collection."); + } + + return $cursor; +}Then use it:
-$cursor = new Document(); -$cursorQuery = Query::getCursorQueries($queries); -if (! is_null($cursorQuery)) { - $cursor = $cursorQuery->getCursorDocument($cursorQuery); - if ($cursor->getCollection() !== $collection->getId()) { - throw new DatabaseException("cursor Document must be from the same Collection."); - } -} +$cursorQuery = Query::getCursorQueries($queries); +$cursor = $this->validateCursor($cursorQuery, $collection);Also applies to: 5639-5646
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Database/Database.php(44 hunks)
🔇 Additional comments (2)
src/Database/Database.php (2)
6676-6676: Cache key generation change may impact performanceChanging from
implode()toserialize()for generating cache keys is more robust for complex query structures but could impact performance for frequently accessed documents.Consider the performance implications and ensure cache invalidation works correctly with the new key format.
5824-5835: Well-structured authorization handling for joined collectionsThe implementation properly handles authorization checks for each collection in the join context, tracking skipAuth status independently per collection. This ensures security is maintained across complex queries.
| public static function convertQuery(QueryContext $context, Query $query): Query | ||
| { | ||
| $collection = clone $context->getCollectionByAlias($query->getAlias()); | ||
|
|
||
| if ($collection->isEmpty()) { | ||
| throw new \Exception('Unknown Alias context'); |
There was a problem hiding this comment.
Generic exception type reduces debuggability
Using generic \Exception makes it harder to handle specific error cases and reduces API clarity.
-if ($collection->isEmpty()) {
- throw new \Exception('Unknown Alias context');
-}
+if ($collection->isEmpty()) {
+ throw new QueryException('Unknown alias "' . $query->getAlias() . '" in query context');
+}📝 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 static function convertQuery(QueryContext $context, Query $query): Query | |
| { | |
| $collection = clone $context->getCollectionByAlias($query->getAlias()); | |
| if ($collection->isEmpty()) { | |
| throw new \Exception('Unknown Alias context'); | |
| public static function convertQuery(QueryContext $context, Query $query): Query | |
| { | |
| $collection = clone $context->getCollectionByAlias($query->getAlias()); | |
| if ($collection->isEmpty()) { | |
| throw new QueryException('Unknown alias "' . $query->getAlias() . '" in query context'); | |
| } |
🤖 Prompt for AI Agents
In src/Database/Database.php around lines 6573 to 6578, replace the generic
\Exception thrown when the collection is empty with a more specific custom
exception class that clearly indicates the error condition, such as
UnknownAliasException. Define this exception if it does not exist, and throw it
instead to improve error handling and API clarity.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
Chores