-
Notifications
You must be signed in to change notification settings - Fork 55
Add support for exists and notExists query types in Mongo #776
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 3.x
Are you sure you want to change the base?
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis PR introduces support for EXISTS and NOT_EXISTS query types across the database system, enabling field existence checks in MongoDB while explicitly rejecting them in SQL adapters. The changes extend the Query class with new constants and helper methods, update validators to recognize the new types, and implement adapter-specific logic for handling existence checks. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Database/Validator/Query/Filter.php (1)
80-99: EXISTS/NOT_EXISTS validation may bypass relationship guards and accepts stray valuesThe new handling for
exists/notExistsmostly makes sense, but there are two behavior changes worth tightening:
- Virtual relationship guard is skipped for EXISTS/NOT_EXISTS
isValidAttributeAndValues()now returns early forTYPE_EXISTS/TYPE_NOT_EXISTSafterisValidAttribute($attribute).- That means the later block that rejects queries on virtual relationship attributes (child side of 1‑1, certain 1‑N / N‑1, and M‑M) is never reached for these methods.
- Previously, all filter methods (including null checks) respected those guards. Allowing
exists('virtualRelation')while other filters are forbidden is likely inconsistent with the “cannot query virtual relationship attribute” rule.Consider removing the early
returnand instead:
- Let
isValidAttributeAndValues()run as usual for EXISTS/NOT_EXISTS, but- Skip only the per‑value type validation, e.g. by short‑circuiting the
foreach ($values as $value)block when the method is EXISTS/NOT_EXISTS.
- EXISTS/NOT_EXISTS currently allow non‑empty
valuesarrays
isValid()routes EXISTS/NOT_EXISTS throughisValidAttributeAndValues()but does not assert thatvaluesis empty.- If someone sends raw JSON like
{"method":"exists","attribute":"x","values":[1,2]}, it will be silently accepted even though the values are ignored.You may want to explicitly enforce that no values are supplied, e.g.:
- In
isValid()branch forTYPE_EXISTS/TYPE_NOT_EXISTS, return false ifcount($value->getValues()) !== 0, then callisValidAttributeAndValues().This preserves your new allowance for array attributes while keeping behavior consistent for relationship attributes and catching malformed queries.
Also applies to: 215-263, 359-364
🧹 Nitpick comments (3)
src/Database/Adapter/SQL.php (1)
1773-1804: Explicitly rejecting EXISTS/NOT_EXISTS in SQL looks consistentMapping
Query::TYPE_EXISTS/Query::TYPE_NOT_EXISTSto aDatabaseExceptioningetSQLOperator()mirrors how vector queries are rejected and makes the limitation explicit at the adapter boundary. If you ever plan per‑adapter support, you might want a more specific message (e.g. include the actual$method), but as‑is this is clear and safe.src/Database/Adapter/Mongo.php (1)
28-46: Mongo$existswiring for exists/notExists queries is coherent
- Adding
'$exists'to$this->operatorsand mappingTYPE_EXISTS/TYPE_NOT_EXISTSto'$exists'ingetQueryOperator()plustrue/falseinbuildFilter()correctly yields filters of the form['field' => ['$exists' => true|false]].- Including
'$exists'in the exclusion list forreplaceInternalIdsKeys()avoids mangling the operator name when rewriting$‑prefixed attribute keys.- The dedicated
$operator === '$exists'branch inbuildFilter()currently does the same as the finalelseand is redundant, but harmless if you prefer to keep it for clarity.Also applies to: 2371-2385, 2440-2444, 2457-2483
tests/e2e/Adapter/Scopes/SchemalessTests.php (1)
1159-1219: Consider adding a combination query test for consistency.The
testSchemalessNotExistsmethod includes a combination test with bothexistsandnotExistsqueries (lines 1277-1281). Consider adding a similar combination test here for symmetry and completeness, such as combiningQuery::exists('optionalField')withQuery::equal('name', ['doc1']).🔎 Example combination test
Add before line 1218:
+ // Test combination of exists with other query + $documents = $database->find($colName, [ + Query::exists('optionalField'), + Query::equal('name', ['doc1']), + ]); + $this->assertEquals(1, count($documents)); // Only doc1 + $this->assertEquals('doc1', $documents[0]->getId()); + $database->deleteCollection($colName);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/Database/Adapter/Mongo.phpsrc/Database/Adapter/SQL.phpsrc/Database/Query.phpsrc/Database/Validator/Queries.phpsrc/Database/Validator/Query/Filter.phptests/e2e/Adapter/Scopes/SchemalessTests.php
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
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.
📚 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/Validator/Query/Filter.phptests/e2e/Adapter/Scopes/SchemalessTests.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:
src/Database/Adapter/Mongo.phptests/e2e/Adapter/Scopes/SchemalessTests.php
🧬 Code graph analysis (5)
src/Database/Adapter/SQL.php (1)
src/Database/Query.php (1)
Query(8-1207)
src/Database/Validator/Query/Filter.php (2)
src/Database/Query.php (1)
Query(8-1207)src/Database/Validator/Query/Order.php (1)
isValidAttribute(30-55)
src/Database/Adapter/Mongo.php (1)
src/Database/Query.php (1)
Query(8-1207)
tests/e2e/Adapter/Scopes/SchemalessTests.php (4)
src/Database/Adapter/Mongo.php (2)
getSupportForAttributes(2654-2657)exists(344-362)src/Database/Adapter/SQL.php (2)
getSupportForAttributes(949-952)exists(180-222)src/Database/Query.php (3)
Query(8-1207)exists(1192-1195)notExists(1203-1206)src/Database/Document.php (2)
getId(63-66)getAttributes(194-212)
src/Database/Query.php (6)
src/Database/Adapter/Mongo.php (1)
exists(344-362)src/Database/Adapter/SQL.php (1)
exists(180-222)src/Database/Database.php (1)
exists(1496-1501)src/Database/Adapter.php (1)
exists(501-501)src/Database/Adapter/Pool.php (1)
exists(138-141)src/Database/Adapter/SQLite.php (1)
exists(76-106)
⏰ 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
🔇 Additional comments (2)
src/Database/Validator/Queries.php (1)
80-127: Routing EXISTS/NOT_EXISTS to FILTER validators is correctAdding
Query::TYPE_EXISTSandQuery::TYPE_NOT_EXISTSto theBase::METHOD_TYPE_FILTERbranch aligns them with other filter methods, so they get processed by the sameFiltervalidator chain as the rest. This wiring looks correct and complete.tests/e2e/Adapter/Scopes/SchemalessTests.php (1)
1221-1284: LGTM! Comprehensive test coverage for notExists.The test correctly verifies the MongoDB semantics where a field set to
nullis considered "existing" and therefore excluded fromnotExistsresults (line 1262). The combination test at lines 1277-1281 provides excellent coverage for real-world query scenarios.
|
LGTM lets wait for @abnegate review as well |
Query added
Query::exists,
Query::notExists
Adapter
Only mongodb
Summary by CodeRabbit
New Features
exists()andnotExists()query methods to check for field presence or absence in MongoDB queries.Tests
✏️ Tip: You can customize this high-level summary in your review settings.