fix: Undefined values not stripped in directAccess mode - #10269
fix: Undefined values not stripped in directAccess mode#10269yog27ray wants to merge 30 commits into
directAccess mode#10269Conversation
…TTP mode behavior When `directAccess: true` is enabled, `ParseServerRESTController` passes request data directly to Parse Server internals without JSON serialization. In HTTP mode, `JSON.stringify()` naturally strips `undefined` values from payloads. With directAccess, `undefined` values are preserved, passed to MongoDB's `$set` operator, and converted to `null` by the BSON driver. This causes fields that should be absent to be stored as `null`, breaking `doesNotExist` queries and changing data semantics. The fix adds a `stripUndefined()` helper that removes keys with `undefined` values from the request body, making directAccess behave identically to HTTP mode. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughAdds recursive ChangesUndefined value normalization
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This aligns directAccess behavior with HTTP serialization: undefined object properties are omitted and undefined array elements become null. The targeted persistence, query, and response behavior is covered without an active merge-blocking risk. 🚥 Pre-merge checks | ✅ 6 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (6 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
![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
🧹 Nitpick comments (1)
spec/ParseServerRESTController.spec.js (1)
679-695: Add one nested undefined regression case to close the parity gap.This test is good for top-level fields. Please add a case like
{ nested: { absentField: undefined } }to ensure nested updates also match HTTP behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/ParseServerRESTController.spec.js` around lines 679 - 695, Add a nested-undefined regression case to the existing test "should not convert undefined values to null on update with directAccess" in ParseServerRESTController.spec.js: after the current PUT that includes absentField: undefined, also perform a PUT (or include in the same update) with a nested object like { nested: { absentField: undefined } } and then GET the object and assert that getRes.nested is defined, getRes.nested.absentField is undefined, and that 'absentField' is not an own property of getRes.nested; update expectations in the test to cover both top-level and nested undefined behavior so nested updates mirror HTTP behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ParseServerRESTController.js`:
- Around line 32-43: The stripUndefined function currently only removes
undefined at the top level; update stripUndefined to recursively traverse
objects and arrays so its behavior matches JSON.stringify: for objects (function
stripUndefined) iterate keys and if a value is undefined omit the key, otherwise
set key to stripUndefined(value); for arrays return a new array mapping each
element to stripUndefined(element) but convert undefined elements to null (i.e.,
if element === undefined return null); preserve primitives and null as-is and
avoid mutating the original input. Ensure the function handles nested
objects/arrays and returns a fresh structure.
---
Nitpick comments:
In `@spec/ParseServerRESTController.spec.js`:
- Around line 679-695: Add a nested-undefined regression case to the existing
test "should not convert undefined values to null on update with directAccess"
in ParseServerRESTController.spec.js: after the current PUT that includes
absentField: undefined, also perform a PUT (or include in the same update) with
a nested object like { nested: { absentField: undefined } } and then GET the
object and assert that getRes.nested is defined, getRes.nested.absentField is
undefined, and that 'absentField' is not an own property of getRes.nested;
update expectations in the test to cover both top-level and nested undefined
behavior so nested updates mirror HTTP behavior.
🪄 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: 40b804d3-ed21-4a29-b323-4dff6e0f2cc2
📒 Files selected for processing (2)
spec/ParseServerRESTController.spec.jssrc/ParseServerRESTController.js
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## alpha #10269 +/- ##
==========================================
- Coverage 93.80% 93.41% -0.40%
==========================================
Files 192 192
Lines 16863 16877 +14
Branches 252 252
==========================================
- Hits 15818 15765 -53
- Misses 1023 1086 +63
- Partials 22 26 +4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
JSON.parse(JSON.stringify(data)) perfectly replicates HTTP mode's serialization roundtrip — stripping undefined at all nesting levels, not just top-level keys. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ingify()) JSON.parse(JSON.stringify()) is expensive. Replace with a recursive stripUndefined that walks the object tree efficiently — strips undefined keys from objects at all nesting levels and converts undefined array elements to null, matching JSON.stringify behavior. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
mtrezza
left a comment
mtrezza
left a comment
There was a problem hiding this comment.
Can you add a test to compare that with behavior when directAccess is false? It should yield the same result.
Adds a companion test that verifies the same behavior without directAccess (HTTP mode) to confirm both paths yield identical results, as requested in review. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@mtrezza Added a companion test that performs the same create/update/fetch via HTTP (non-directAccess) to confirm both paths yield identical results. The test lets the HTTP layer handle JSON serialization naturally (via |
Remove explicit JSON.stringify/JSON.parse calls from the HTTP mode test, letting the request helper's encodeBody handle serialization naturally. This better demonstrates that HTTP mode inherently strips undefined values. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…behavior Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@coderabbitai Always review the PR title as well. If the commit prefix is |
|
Tip For best results, initiate chat on the files or code changes.
The current title "fix: Strip undefined values in ParseServerRESTController to match HTTP mode" describes the fix. Here's a suggested revision that describes the broken behavior instead:
Or alternatively:
I'll record this convention for future reviews. ✏️ Learnings added
🧠 Learnings used |
|
@mtrezza Regarding the perf regression on Query.find (concurrent, GC pressure): The benchmark suite uses the Parse JS SDK over HTTP ( The latest CI run shows:
All other benchmarks are within normal variance (< 7%). The concurrent GC pressure test is inherently noisy since it depends on V8 garbage collection timing across 10 simultaneous queries each fetching 3,000 large (~8KB) documents. The fluctuation between runs (41.1% in the first run, 53.8% in the re-run) also suggests CI environment noise rather than a code-related regression. That said, to address the performance concern, I can replace |
|
A GC perf test can be flaky, but not fail on every CI run. That's suspicious. Also, the perf arguments don't make much sense. I suggest you try another solution. |
|
@mtrezza Replaced Both approaches produce identical results in The performance advantage: when no |
Avoid full deep-copy on every request/response by mutating in-place. This eliminates unnecessary allocations and GC pressure for the common case where no undefined values exist.
f6ea238 to
0e56fdf
Compare
|
Perf job passes now.
What is this convention based on? |
|
This matches
For example: JSON.stringify({ a: undefined, b: 1 }) // '{"b":1}' — key omitted
JSON.stringify([1, undefined, 3]) // '[1,null,3]' — undefined → nullSince the goal of |
|
@mtrezza any further changes required. |
|
@mtrezza any update on this |
|
@mtrezza is there any changes required? |
|
Requires further review... the issue is that directAccess is lacking proper test coverage, so we're operating in the dark here. |
@mtrezza, thanks for the review. On the coverage concern — I've now added test cases covering this path in For what it's worth, we've also been running this fix in production via |
|
Thanks for the details. I'm referring to the whole directAccess feature; it's lacking proper coverage; virtually all tests run with directAccess disabled. So directAcess related changes take some time to merge. |
|
@mtrezza any thing that i can help with to make this PR merge. |
|
Merged alpha to check CI state |
|
@mtrezza All checks have passed on the latest run against |
|
@mtrezza can we get this PR merged ? |
|
@coderabbitai full review |
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
4 participants
Issue
When
directAccess: trueis enabled,ParseServerRESTControllerbypasses the HTTP layer and routes requests internally. This causes two categories of behavior divergence from the HTTP code path:1. Undefined values in request body stored as
nullundefinedvalues in update (PUT) payloads are converted tonulland stored in the database. This breaksdoesNotExist/existsqueries and changes data semantics.Root cause:
ParseServerRESTControllerpasses request data directly to Parse Server internals without JSON serialization. In HTTP mode,JSON.stringify()naturally stripsundefinedvalues from payloads. With directAccess,undefinedvalues are preserved all the way totransformUpdate()inMongoTransform.js, which passes them into MongoDB's$setoperator. The MongoDB BSON driver then convertsundefinedtonull.2. Undefined values in response leak to the client
Cloud function responses containing
undefinedvalues (e.g.,{ questionId: undefined }) are passed directly in-memory to the caller without JSON serialization. In HTTP mode,JSON.stringifynaturally strips these undefined properties. With directAccess, the caller receives the raw response object withundefinedproperties present, causingdeep.equalassertions and object key checks to behave differently.Reproduction (response issue):
Approach
The
stripUndefined()helper inParseServerRESTController.jsis now applied to both the request body AND the response:Request body :
body: stripUndefined(data)— stripsundefinedvalues before passing to the router, matching HTTP'sJSON.stringifybehavior on the request path.Response :
stripUndefined(response)— stripsundefinedvalues from the response before resolving to the caller, matching HTTP's JSON serialization behavior on the response path.This ensures directAccess behaves identically to HTTP mode in both directions. The fix is applied at the
ParseServerRESTControllerlevel — the component added whendirectAccessis enabled — addressing the root cause at the right layer.Tasks
Summary by CodeRabbit
nullin arrays.null.