fix: Parse.Query.containedIn and matchesQuery do not work with nested objects
#9738
Conversation
|
The branch Pull requests are usually opened against the default branch |
|
🚀 Thanks for opening this pull request! |
📝 Walkthrough""" WalkthroughThree new test cases were added to the Additionally, the Changes
Sequence Diagram(s)sequenceDiagram
participant TestRunner
participant MongoDB
participant ParseServer
TestRunner->>ParseServer: Create and save Child object
ParseServer->>MongoDB: Insert Child
MongoDB-->>ParseServer: Ack
TestRunner->>ParseServer: Create Parent object with nested Child pointer and save
ParseServer->>MongoDB: Insert Parent (with nested pointer)
MongoDB-->>ParseServer: Ack
TestRunner->>ParseServer: Query Parent with constraint on nested pointer
ParseServer->>MongoDB: Query Parent (with nested pointer constraint)
MongoDB-->>ParseServer: Return matching Parent
ParseServer-->>TestRunner: Return query results
Assessment against linked issues
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🪛 ESLintspec/MongoStorageAdapter.spec.js[error] 398-398: 'it' is not defined. (no-undef) [error] 417-417: 'expect' is not defined. (no-undef) [error] 420-420: 'it' is not defined. (no-undef) [error] 439-439: 'expect' is not defined. (no-undef) [error] 442-442: 'it' is not defined. (no-undef) [error] 461-461: 'expect' is not defined. (no-undef) 🔇 Additional comments (4)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 0
🧹 Nitpick comments (1)
spec/MongoStorageAdapter.spec.js (1)
398-462: Consider adding negative test casesWhile the current tests verify that the query constraints work correctly when there should be a match, it would be beneficial to also test cases where no match should be found.
For example, you could add a test like:
it('Parse query with containedIn returns no results for non-matching object', async () => { const child = new Parse.Object('Child') child.set('key','value') await child.save(); const nonMatchingChild = new Parse.Object('Child') nonMatchingChild.set('key','different_value') await nonMatchingChild.save(); const parent = new Parse.Object('Parent'); parent.set('some', { nested: { key: { child } } }) await parent.save(); const query = await new Parse.Query('Parent') .containedIn('some.nested.key.child', [nonMatchingChild]) .find(); expect(query.length).toEqual(0); })🧰 Tools
🪛 ESLint
[error] 398-398: 'it' is not defined.
(no-undef)
[error] 417-417: 'expect' is not defined.
(no-undef)
[error] 420-420: 'it' is not defined.
(no-undef)
[error] 439-439: 'expect' is not defined.
(no-undef)
[error] 442-442: 'it' is not defined.
(no-undef)
[error] 461-461: 'expect' is not defined.
(no-undef)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
spec/MongoStorageAdapter.spec.js(1 hunks)
🧰 Additional context used
🪛 ESLint
spec/MongoStorageAdapter.spec.js
[error] 398-398: 'it' is not defined.
(no-undef)
[error] 417-417: 'expect' is not defined.
(no-undef)
[error] 420-420: 'it' is not defined.
(no-undef)
[error] 439-439: 'expect' is not defined.
(no-undef)
[error] 442-442: 'it' is not defined.
(no-undef)
[error] 461-461: 'expect' is not defined.
(no-undef)
🔇 Additional comments (4)
spec/MongoStorageAdapter.spec.js (4)
393-397: Good documentationThe comment clearly explains the issue being fixed - that nested pointer queries work with
equalTobut not withcontainedInormatchesQuery. This provides valuable context for understanding the tests that follow.
398-418: Well-structured baseline testThis test establishes a baseline by confirming that
equalToworks correctly with nested pointers. The test creates the necessary objects, sets up the nested structure properly, and makes a clear assertion.🧰 Tools
🪛 ESLint
[error] 398-398: 'it' is not defined.
(no-undef)
[error] 417-417: 'expect' is not defined.
(no-undef)
420-440: Good test for thecontainedInfixThis test properly verifies that using
containedInon a nested pointer field works correctly, which addresses part of the issue mentioned in the PR objectives. The structure mirrors the previous test for consistency.🧰 Tools
🪛 ESLint
[error] 420-420: 'it' is not defined.
(no-undef)
[error] 439-439: 'expect' is not defined.
(no-undef)
442-462: Comprehensive test formatchesQueryfunctionalityThis test effectively verifies that the
matchesQueryconstraint works properly with nested pointers, completing the coverage of the fix. The comment noting that this internally usescontainedInhelps explain the relationship between the tests.🧰 Tools
🪛 ESLint
[error] 442-442: 'it' is not defined.
(no-undef)
[error] 461-461: 'expect' is not defined.
(no-undef)
🎉 Snyk checks have passed. No issues have been found so far.✅ security/snyk check is complete. No issues have been found. (View Details) |
|
It cancelled the failing test case action -_- |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/Adapters/Storage/Mongo/MongoTransform.js (5)
654-657: Fix for nested object query support in constraintsThis change is addressing the core issue in the PR. By accepting the
keyparameter and checking if it's a nested field (contains a dot), the function can now properly handle query constraints on nested objects.Note: There is a small typo in the comment: "wether" should be "whether".
- // Check wether the given key has `.` + // Check whether the given key has `.`🧰 Tools
🪛 Biome (1.9.4)
[error] 655-655: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
739-742: Consistently renamed variables in $all handlerThe variable renaming is applied throughout the $all constraint handling.
As highlighted by static analysis, consider using
Array.isArray()instead ofinstanceof Arrayfor better compatibility:- const arr = constraint[constraintKey]; - if (!(arr instanceof Array)) { + const arr = constraint[constraintKey]; + if (!Array.isArray(arr)) {🧰 Tools
🪛 Biome (1.9.4)
[error] 740-740: Use Array.isArray() instead of instanceof Array.
instanceof Array returns false for array-like objects and arrays from other execution contexts.
Unsafe fix: Use Array.isArray() instead.(lint/suspicious/useIsArray)
756-757: Renamed variables in regex constraint handlingVariable renamed in the regex constraint handling section.
As noted by static analysis, the variable declaration in a switch case could be improved:
- var s = constraint[constraintKey]; + { + const s = constraint[constraintKey]; + if (typeof s !== 'string') { + throw new Parse.Error(Parse.Error.INVALID_JSON, 'bad regex: ' + s); + } + answer[constraintKey] = s; + } - if (typeof s !== 'string') { - throw new Parse.Error(Parse.Error.INVALID_JSON, 'bad regex: ' + s); - } - answer[constraintKey] = s;Also applies to: 760-761
🧰 Tools
🪛 Biome (1.9.4)
[error] 756-756: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
764-765: Renamed variables in $containedBy constraintVariable renaming consistently applied to the $containedBy constraint.
Similar to previous suggestions, consider using Array.isArray():
- const arr = constraint[constraintKey]; - if (!(arr instanceof Array)) { + const arr = constraint[constraintKey]; + if (!Array.isArray(arr)) {🧰 Tools
🪛 Biome (1.9.4)
[error] 765-765: Use Array.isArray() instead of instanceof Array.
instanceof Array returns false for array-like objects and arrays from other execution contexts.
Unsafe fix: Use Array.isArray() instead.(lint/suspicious/useIsArray)
850-851: Renamed variables in $within constraintVariable renaming applied to the $within constraint handler.
As noted by static analysis, this switch clause variable declaration could be wrapped in a block:
- var box = constraint[constraintKey]['$box']; + { + const box = constraint[constraintKey]['$box']; + if (!box || box.length != 2) { + throw new Parse.Error(Parse.Error.INVALID_JSON, 'malformatted $within arg'); + } + answer[constraintKey] = { + $box: [ + [box[0].longitude, box[0].latitude], + [box[1].longitude, box[1].latitude], + ], + }; + } - if (!box || box.length != 2) { - throw new Parse.Error(Parse.Error.INVALID_JSON, 'malformatted $within arg'); - } - answer[constraintKey] = { - $box: [ - [box[0].longitude, box[0].latitude], - [box[1].longitude, box[1].latitude], - ], - };Also applies to: 854-855
🧰 Tools
🪛 Biome (1.9.4)
[error] 850-850: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Adapters/Storage/Mongo/MongoTransform.js(13 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/Adapters/Storage/Mongo/MongoTransform.js
[error] 655-655: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 724-724: Use Array.isArray() instead of instanceof Array.
instanceof Array returns false for array-like objects and arrays from other execution contexts.
Unsafe fix: Use Array.isArray() instead.
(lint/suspicious/useIsArray)
[error] 740-740: Use Array.isArray() instead of instanceof Array.
instanceof Array returns false for array-like objects and arrays from other execution contexts.
Unsafe fix: Use Array.isArray() instead.
(lint/suspicious/useIsArray)
[error] 756-756: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 850-850: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: PostgreSQL 15, PostGIS 3.5
- GitHub Check: PostgreSQL 17, PostGIS 3.5
- GitHub Check: MongoDB 7, ReplicaSet
- GitHub Check: PostgreSQL 16, PostGIS 3.5
- GitHub Check: MongoDB 6, ReplicaSet
- GitHub Check: Node 18
- GitHub Check: Node 20
- GitHub Check: Redis Cache
- GitHub Check: PostgreSQL 15, PostGIS 3.4
- GitHub Check: PostgreSQL 15, PostGIS 3.3
- GitHub Check: MongoDB 8, ReplicaSet
- GitHub Check: Docker Build
- GitHub Check: Code Analysis (javascript)
🔇 Additional comments (24)
src/Adapters/Storage/Mongo/MongoTransform.js (24)
329-330: Properly passing the key parameter to transformConstraintThe key is now correctly passed to the transformConstraint function from the transformQueryKeyValue function, which is essential for the fix to work.
661-662: Essential logic to handle nested fields correctlyThis is the critical change that fixes the issue with
containedInandmatchesQueryconstraints for nested objects. By choosing the appropriate transformation function based on whether the field is an array OR the key is nested, nested pointer fields are now properly transformed.
674-675: Variable renaming for clarityRenaming
keystoconstraintKeysimproves code clarity and avoids confusion with the outerkeyparameter.
676-677: Variable renaming to prevent shadowingGood change to avoid shadowing the
keyparameter with a loop variable. This improves code maintainability and prevents potential bugs.
685-686: Consistent variable renamingContinuing the variable renaming pattern to use
constraintKeyinstead ofkeyfor consistency within the function.
693-694: Consistent variable renamingFurther applied the variable renaming scheme throughout the switch statement.
706-707: Consistent variable renamingVariable renaming applied to
answer[constraintKey].
713-714: Variable renaming in error messageConsistent renaming in the error message string.
717-718: Final value transformation using the appropriate transformerThis line uses the dynamically selected transformer (which now correctly handles nested fields) to transform the value.
745-746: Variable renaming in relationship extractionConsistent variable renaming applied to the values extraction.
774-775: Renaming for $options constraintVariable renaming applied to the $options constraint case.
778-779: Renamed variables in $text constraintVariable renaming applied in the $text constraint handling section.
Also applies to: 785-786
792-793: Renamed variables in $text optionsConsistently renamed variables for additional $text search options.
Also applies to: 800-801, 808-809
813-814: Renamed variables in $nearSphere constraintVariable renaming applied to the $nearSphere constraint.
Also applies to: 819-820
827-828: Renamed variables in $maxDistanceVariable renaming in the $maxDistance constraint handler.
833-834: Renamed variables in distance-related constraintsVariable renaming consistently applied to the distance-related constraints.
Also applies to: 836-837, 839-840
846-847: Renamed variable in error messageVariable renaming applied to the error message for unsupported constraints.
863-865: Renamed variables in $geoWithin constraintVariable renaming applied to the $geoWithin constraint handler.
901-903: Renamed variables in polygon handlingVariable renaming applied to the polygon portion of $geoWithin.
930-932: Renamed variables in centerSphere handlingVariable renaming applied to the centerSphere portion of $geoWithin.
937-938: Renamed variables in $geoIntersects constraintVariable renaming applied to the $geoIntersects constraint handler.
Also applies to: 946-952
955-957: Renamed variables in the default caseVariable renaming applied to the default case that handles custom constraints.
654-962: Overall assessment of the changes to fix nested objects in queriesThis is a solid, targeted fix for the issue with querying nested objects. The changes:
- Add proper detection of nested fields (containing dots)
- Use the correct transformation function for nested fields
- Consistently rename variables to avoid shadowing and improve clarity
These changes ensure that constraints like
containedInandmatchesQuerywork properly with nested objects by correctly transforming the query conditions. The thorough renaming of variables also prevents potential bugs that could arise from variable shadowing.🧰 Tools
🪛 Biome (1.9.4)
[error] 655-655: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 756-756: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 850-850: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 724-724: Use Array.isArray() instead of instanceof Array.
instanceof Array returns false for array-like objects and arrays from other execution contexts.
Unsafe fix: Use Array.isArray() instead.(lint/suspicious/useIsArray)
[error] 740-740: Use Array.isArray() instead of instanceof Array.
instanceof Array returns false for array-like objects and arrays from other execution contexts.
Unsafe fix: Use Array.isArray() instead.(lint/suspicious/useIsArray)
[error] 765-765: Use Array.isArray() instead of instanceof Array.
instanceof Array returns false for array-like objects and arrays from other execution contexts.
Unsafe fix: Use Array.isArray() instead.(lint/suspicious/useIsArray)
[error] 875-875: Use Array.isArray() instead of instanceof Array.
instanceof Array returns false for array-like objects and arrays from other execution contexts.
Unsafe fix: Use Array.isArray() instead.(lint/suspicious/useIsArray)
[error] 890-890: Use Array.isArray() instead of instanceof Array.
instanceof Array returns false for array-like objects and arrays from other execution contexts.
Unsafe fix: Use Array.isArray() instead.(lint/suspicious/useIsArray)
[error] 905-905: Use Array.isArray() instead of instanceof Array.
instanceof Array returns false for array-like objects and arrays from other execution contexts.
Unsafe fix: Use Array.isArray() instead.(lint/suspicious/useIsArray)
[error] 913-913: Use Array.isArray() instead of instanceof Array.
instanceof Array returns false for array-like objects and arrays from other execution contexts.
Unsafe fix: Use Array.isArray() instead.(lint/suspicious/useIsArray)
723-726:✅ Verification successful
Variable renaming for consistency
The renaming is consistently applied throughout the function, including in array value extractions and error messages.
🏁 Script executed:
#!/bin/bash # Check if there are any remaining instances of unintended 'key' variables that should be 'constraintKey' # This will help ensure the renaming was comprehensive rg -A 2 -B 2 "for\s*\(.+\s+key\s+.+\)\s*\{" src/Adapters/Storage/Mongo/MongoTransform.jsLength of output: 181
Variable renaming verified and correct
All instances of
constraintKeyare used consistently in array access and error messages. Thefor (var key in mongoObject)loop correctly useskeyto iterate overmongoObjectproperties and does not require renaming.🧰 Tools
🪛 Biome (1.9.4)
[error] 724-724: Use Array.isArray() instead of instanceof Array.
instanceof Array returns false for array-like objects and arrays from other execution contexts.
Unsafe fix: Use Array.isArray() instead.(lint/suspicious/useIsArray)
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## alpha #9738 +/- ##
==========================================
- Coverage 93.01% 93.00% -0.02%
==========================================
Files 187 187
Lines 15081 15082 +1
Branches 174 174
==========================================
- Hits 14028 14027 -1
- Misses 1041 1043 +2
Partials 12 12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Tests should be running now |
i meant the failing test cases were skipped since I pushed a commit before the actions were complete. We need that to see the current behaviour right? Is there a way to retry actions at that specific commit so we can see the failing test cases |
|
If you say they failed for you when you ran them locally, then that's fine. I see that all tests are passing now as well, nice! |
Signed-off-by: Manuel <[email protected]>
Signed-off-by: Manuel <[email protected]>
| var keys = Object.keys(constraint).sort().reverse(); | ||
| var constraintKeys = Object.keys(constraint).sort().reverse(); |
There was a problem hiding this comment.
It seems that most changes are just a var renaming form keys to constraintKeys. We aim at minimal code changes unless there is a justified benefit (beyond semantics) to rename the var. If you feel the var needs an explanation, then please add a code comment directly to the var which will also be picked up by the IDE:
/** Explain var. */
var keys = ...| * If we use equalTo to comparse the nested pointer it works | ||
| * But it does not work with contained in or matchesQuery | ||
| */ | ||
| it('queries nested objects using equalTo', async () => { |
There was a problem hiding this comment.
I doesn't seem like these tests belong in the MongoStorageAdapter.spec.js file, which is testing the adapter itself. These are just Parse.Query tests and there should be a dedicated file for that.
mtrezza
changed the title
Parse.Query.containedIn and matchesQuery do not work with nested objects
|
Note: it seems that your PR is based on the |
Pull Request
Issue
#7414
Closes: #7414
Approach
Previously nested objects were converted to
Child$ObjectIDformat before making a query to mongodb. Which was incorrect since nested objects are stored in their object format. Current change handles this edge case. Originally even nested objects should have been stored inChild$ObjectIDformat. But this is a fix we can provide for now.Tasks
Summary by CodeRabbit
Tests
Refactor