fix: Restore direct pointer match in equalTo for array value queries#527
fix: Restore direct pointer match in equalTo for array value queries#527dacdoyx wants to merge 3 commits into
Conversation
The 2.3.1 equalTo fix always wrapped values in $eq, which broke queries on array values (e.g. users => ParseUser in ParseRole queries). When the value is a ParseObject (Pointer), set it directly in the where clause instead of using $eq.
|
I will reformat the title to use the proper commit message syntax. |
parse-github-assistant
Bot
changed the title
|
🚀 Thanks for opening this pull request! We appreciate your effort in improving the project. Please let us know once your pull request is ready for review. Tip
Note Please respond to review comments from AI agents just like you would to comments from a human reviewer. Let the reviewer resolve their own comments, unless they have reviewed and accepted your commit, or agreed with your explanation for why the feedback was incorrect. Caution Pull requests must be written using an AI agent with human supervision. Pull requests written entirely by a human will likely be rejected, because of lower code quality, higher review effort and the higher risk of introducing bugs. Please note that AI review comments on this pull request alone do not satisfy this requirement. Our CI and AI review are safeguards, not development tools. If many issues are flagged, rethink your development approach. Invest more effort in planning and design rather than using review cycles to fix low-quality code. |
📝 WalkthroughWalkthrough
ChangesQuery equality pointer handling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 7✅ Passed checks (7 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Parse/ParseQuery.php`:
- Around line 143-149: The direct assignment in ParseQuery::equalTo() bypasses
addCondition(), making same-key constraints order-dependent and breaking the
usual per-key operator map behavior. Update equalTo() so ParseObject values are
stored through addCondition() under '$eq' instead of writing directly into
$this->where, and only collapse a single '$eq' pointer to a raw pointer during
serialization when it is the only condition for that key.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bd508ab6-e06e-458d-ab6c-7420d02103ef
📒 Files selected for processing (1)
src/Parse/ParseQuery.php
…ew feedback) Instead of directly assigning to $this->where (which drops same-key constraints), store ParseObject values via addCondition under a special $eq_pointer key, then collapse to raw pointer at query build time when it is the only condition for that key. This preserves the per-key operator map contract while still supporting array value queries.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
verify_fix.php (1)
39-61: 📐 Maintainability & Code Quality | 🔵 TrivialVerification gap:
getWhere()may not reflect final request serialization.This test validates
getWhere()output, but per the PR stack the$eq_pointermarker is only collapsed in_getOptions()anddistinct(). IfgetWhere()returns the internal state before collapse, these assertions may pass or fail depending on implementation details, but they don't verify the actual request payload sent to the server. Consider adding a test that exercises the full serialization path.Also, there's no test for the multi-constraint scenario (e.g.,
equalTo('users', $user)->notEqualTo('users', $otherUser)) that motivated preserving the per-key operator map.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@verify_fix.php` around lines 39 - 61, `verify_fix.php` is asserting `ParseQuery::getWhere()` internal state instead of the final serialized payload, so it may miss the actual `$eq_pointer` collapse behavior. Update the test to exercise the full request serialization path used by `ParseQuery::_getOptions()` (and `distinct()` if relevant) so the pointer is verified in the form sent to the server, not just in `getWhere()`. Also add a multi-constraint case on the same key, such as chaining `equalTo('users', $user)` with `notEqualTo('users', $otherUser)`, to confirm the per-key operator map is preserved correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Parse/ParseQuery.php`:
- Around line 510-517: The current `where` serialization in
`ParseQuery::_getOptions()` and `distinct()` only handles top-level
`$eq_pointer` entries, so nested `orQueries()/andQueries()/norQueries()` and
mixed field operators can still leak invalid sentinel values. Add a single
recursive normalization helper in `ParseQuery` that walks the full condition
tree, collapses a sole `$eq_pointer` to the raw pointer value, and rewrites
mixed pointer-equality cases to `'$eq'` before either method serializes `where`.
Make sure both `_getOptions()` and `distinct()` call this helper on
`$this->where` so all query shapes are normalized consistently.
In `@verify_equalto.php`:
- Around line 2-8: The autoloader in the verify_equalto.php bootstrap is
hardcoded to an absolute checkout path, which breaks portability. Update the
spl_autoload_register callback to build the Parse source base directory relative
to the script location using __DIR__ instead of the fixed
/root/parse-php-sdk/src/Parse/ path, keeping the rest of the class-to-file
mapping logic unchanged.
- Around line 61-80: Test 5 in verify_equalto.php is checking different keys, so
it does not cover the mixed same-key serialization case that can emit
$eq_pointer. Update the ParseQuery setup in the Test 5 block so both constraints
target the same field using ParseQuery::greaterThan and ParseQuery::equalTo,
then assert the encoded where payload still contains the expected constraint
markers and never contains $eq_pointer. Use the existing Test 4 and Test 5
ParseQuery logic as the place to make this change.
---
Nitpick comments:
In `@verify_fix.php`:
- Around line 39-61: `verify_fix.php` is asserting `ParseQuery::getWhere()`
internal state instead of the final serialized payload, so it may miss the
actual `$eq_pointer` collapse behavior. Update the test to exercise the full
request serialization path used by `ParseQuery::_getOptions()` (and `distinct()`
if relevant) so the pointer is verified in the form sent to the server, not just
in `getWhere()`. Also add a multi-constraint case on the same key, such as
chaining `equalTo('users', $user)` with `notEqualTo('users', $otherUser)`, to
confirm the per-key operator map is preserved correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0e099bba-04f8-457f-8227-6be3240b0d0b
📒 Files selected for processing (3)
src/Parse/ParseQuery.phpverify_equalto.phpverify_fix.php
- Add normalizeWhere() recursive helper that walks full condition tree - Collapses sole $eq_pointer to raw pointer, rewrites mixed cases to $eq - Both _getOptions() and distinct() now use normalizeWhere() - Fix verify_equalto.php autoloader to use __DIR__ (portable) - Update Test 5 to test same-key mixed constraint case
There was a problem hiding this comment.
🧹 Nitpick comments (1)
verify_equalto.php (1)
71-81: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd one nested boolean-query case.
normalizeWhere()was added to rewrite pointer equality inside nested$or/$and/$nortrees too, but this verifier still only covers top-level shapes. A single nested-query assertion here would protect the main regression-prone path.Example addition
+echo "\nTest 6: Nested OR query with ParseObject equality\n"; +$left = new ParseQuery('TestClass'); +$left->equalTo('users', $user); +$right = new ParseQuery('TestClass'); +$right->equalTo('name', 'alice'); +$query6 = ParseQuery::orQueries([$left, $right]); +$encoded6 = json_encode($query6->_getOptions()['where'] ?? []); +echo " where = $encoded6\n"; +check('nested query should NOT have $eq_pointer', strpos($encoded6, '$eq_pointer') === false); +check('nested query should still contain the pointer payload', strpos($encoded6, '"__type":"Pointer"') !== false);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@verify_equalto.php` around lines 71 - 81, Add a nested boolean-query test to verify normalizeWhere() rewrites pointer equality inside a tree like $or, $and, or $nor, not just top-level constraints. Update the ParseQuery-based verifier in verify_equalto.php by creating a nested query case, inspecting _getOptions()['where'], and asserting the pointer payload is preserved while $eq_pointer does not appear inside the nested structure. Use ParseQuery and normalizeWhere-related behavior as the key references when adding the assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@verify_equalto.php`:
- Around line 71-81: Add a nested boolean-query test to verify normalizeWhere()
rewrites pointer equality inside a tree like $or, $and, or $nor, not just
top-level constraints. Update the ParseQuery-based verifier in
verify_equalto.php by creating a nested query case, inspecting
_getOptions()['where'], and asserting the pointer payload is preserved while
$eq_pointer does not appear inside the nested structure. Use ParseQuery and
normalizeWhere-related behavior as the key references when adding the assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 27bbbb31-c01e-4e5a-8537-149d832ee169
📒 Files selected for processing (2)
src/Parse/ParseQuery.phpverify_equalto.php
1 participant
Summary
equalToto use direct pointer matching for ParseObject valuesusers => ParseUserin ParseRole queries)$eqconstraint as beforeIssue
Closes #516
Root Cause
The 2.3.1
equalTofix always wrapped values in$eq, which meant pointer/array queries like:sent
{"users": {"$eq": {"__type": "Pointer", ...}}}instead of{"users": {"__type": "Pointer", ...}}.Fix
When the value is a
ParseObject(Pointer), set it directly in the where clause instead of wrapping in$eq.Files Changed
src/Parse/ParseQuery.phpSummary by CodeRabbit
equalToquery encoding for pointer-style object comparisons, ensuring the backend receives the correct pointer representation instead of a generic equality constraint.whereclause handling so chained/same-key operators (includingdistinct) correctly preserve pointer equality semantics alongside other conditions.whereserialization for pointer vs non-pointer values, chaining behavior, and mixed constraints.