refactor: Isolate Parse.Object conversion behind ObjectAdapter#10487
refactor: Isolate Parse.Object conversion behind ObjectAdapter#10487dblythy wants to merge 4 commits into
Conversation
Towards parse-community#8787. Auth's session/role resolution and LiveQuery's _clearCachedRoles now always go through RestQuery instead of the Parse JS SDK's Parse.Query. The !config SDK fallback branches in Auth (getAuthForSessionToken, getRolesForUser, getRolesByIds) are removed — all real callers already pass a config, so a config is now required. Threads config into the LiveQuery getAuthForSessionToken calls. Deletes the dead SessionTokenCache (exported but never used in src). Drops the obsolete no-config Auth/Role specs and rewrites the role-based ACL LiveQuery specs to mock the auth seam instead of the removed Parse.Query.
Introduce src/cloud-code/QueryAdapter.js as the single boundary between parse-server's internal REST/JSON query format and the Parse JS SDK's Parse.Query. Core code (triggers, rest, RestQuery, LiveQuery) now calls inflateQuery/deflateQuery/isQuery/applyQueryToRest instead of constructing or inspecting Parse.Query directly, so the SDK query type lives in one place. Towards parse-community#8787.
applyQueryToRest tested option truthiness, so an explicit falsy override from a beforeFind trigger (e.g. query.limit(0)) was silently dropped. Test presence with hasOwnProperty instead. Parse.Query#toJSON only emits keys that were set, so no default values leak in. Adds a regression test.
Towards parse-community#8787. Make src/cloud-code/ObjectAdapter.js the sole owner of Parse.Object construction and inspection, mirroring QueryAdapter. The SDK pending-ops state-tracking seam is deferred to a follow-up.
|
🚀 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. |
📝 WalkthroughWalkthroughThis PR introduces cloud-code adapter modules ( ChangesCloud Code Adapter Boundary and Session Consolidation
Adapter Integration Throughout Query and Object Handling
LiveQuery Server and Publisher Adapter Integration
Test Coverage for New Adapter Behavior
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 OpenGrep (1.22.0)OpenGrep fatal error (exit code 2): [00.13][ERROR]: Error: exception Unix_error: No such file or directory stat spec/ParseLiveQueryServer.spec.js Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## alpha #10487 +/- ##
==========================================
- Coverage 92.60% 92.24% -0.36%
==========================================
Files 193 194 +1
Lines 16893 16858 -35
Branches 234 234
==========================================
- Hits 15643 15550 -93
- Misses 1227 1281 +54
- Partials 23 27 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/Controllers/UserController.js (1)
165-178: ⚡ Quick win
inflatetwo-arg form is supported; mixed conventions are a maintainability issue only.
src/cloud-code/ObjectAdapter.jsdefinesinflateObject(data, restObject), andsrc/triggers.jsre-exports it asexport const inflate = inflateObject. That means legacy calls likeinflate('_User', fetchedUser)(e.g., around lines 178 and 289) correctly mergefetchedUseronto{ className: '_User' }before callingParse.Object.fromJSON, so there’s no silent payload breakage.Still,
sendVerificationEmailuses both single-object and legacy two-arg forms—pick one convention and align the call sites for consistency/readability.🤖 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 `@src/Controllers/UserController.js` around lines 165 - 178, The codebase mixes two forms of the inflate helper (inflateObject re-exported as inflate) causing inconsistent calls in sendVerificationEmail: unify to one convention (prefer the object form) by replacing legacy two-arg calls like inflate('_User', fetchedUser) with the single-object form inflate({ className: '_User', ...fetchedUser }) (or vice-versa if you choose legacy form) across sendVerificationEmail and other sites referenced (e.g., around the second occurrence at the later call site), and ensure imports reference inflate (or inflateObject) consistently so callers and the implementation match.src/LiveQuery/ParseLiveQueryServer.ts (1)
614-655: 💤 Low valueMinor: Reuse the existing
configvariable instead of callingConfig.getagain.On line 644,
Config.get(this.config.appId)is called a second time within the map callback, but this same config was already retrieved and stored in theconfigvariable on line 616.♻️ Suggested fix
const [auth1, auth2] = await Promise.all([ authPromise, getAuthForSessionToken({ cacheController: this.cacheController, sessionToken, - config: Config.get(this.config.appId), + config, }), ]);🤖 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 `@src/LiveQuery/ParseLiveQueryServer.ts` around lines 614 - 655, In _clearCachedRoles, avoid calling Config.get(this.config.appId) again inside the map callback; reuse the already retrieved local config variable instead—pass the existing config into getAuthForSessionToken (replace Config.get(this.config.appId) with config) so the function uses the previously fetched config and avoids redundant lookups.
🤖 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 `@spec/ParseLiveQueryServer.spec.js`:
- Around line 1511-1518: Two promise chains in the tests that stub
parseLiveQueryServer.getAuthForSessionToken (the blocks that return { userId,
auth: { getUserRoles } }) are missing error handlers and can leave the spec
hanging; update each of those .then(...) chains to append .catch(done.fail) so
rejections cause the test to fail and complete (i.e., locate the tests that
spyOn parseLiveQueryServer.getAuthForSessionToken and add .catch(done.fail) to
both promise chains).
In `@src/cloud-code/ObjectAdapter.js`:
- Around line 28-30: inflateObject currently does an unconditional for (const
key in restObject) which will throw when restObject is undefined; update
inflateObject to guard the loop by checking that restObject is a non-null object
(e.g., if (restObject && typeof restObject === 'object')) before iterating, so
the copy assignment to copy[key] only runs when restObject is present; keep
references to the same symbols (inflateObject, restObject, copy) so the fix is
localized and safe for callsites that pass only the first arg.
In `@src/triggers.js`:
- Around line 983-986: The re-export triggers.inflate currently points to
inflateObject which expects two arguments and unconditionally iterates for
(const key in restObject), so one-argument call sites will throw when restObject
is undefined; fix by making inflate (or inside inflateObject) guard the second
parameter with a default empty object (e.g. restObject = restObject || {})
before the for-in loop so existing single-argument calls to inflate(...)
continue to work, ensuring inflate/inflateObject retain the same behavior for
callers like triggers.inflate and any code calling inflate(Object.assign(...))
or inflate({ className: ... }).
---
Nitpick comments:
In `@src/Controllers/UserController.js`:
- Around line 165-178: The codebase mixes two forms of the inflate helper
(inflateObject re-exported as inflate) causing inconsistent calls in
sendVerificationEmail: unify to one convention (prefer the object form) by
replacing legacy two-arg calls like inflate('_User', fetchedUser) with the
single-object form inflate({ className: '_User', ...fetchedUser }) (or
vice-versa if you choose legacy form) across sendVerificationEmail and other
sites referenced (e.g., around the second occurrence at the later call site),
and ensure imports reference inflate (or inflateObject) consistently so callers
and the implementation match.
In `@src/LiveQuery/ParseLiveQueryServer.ts`:
- Around line 614-655: In _clearCachedRoles, avoid calling
Config.get(this.config.appId) again inside the map callback; reuse the already
retrieved local config variable instead—pass the existing config into
getAuthForSessionToken (replace Config.get(this.config.appId) with config) so
the function uses the previously fetched config and avoids redundant lookups.
🪄 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: 83bc06e3-4702-4ccb-8198-6753e58c4cdd
📒 Files selected for processing (20)
spec/Auth.spec.jsspec/CloudCode.spec.jsspec/ParseLiveQueryServer.spec.jsspec/ParseRole.spec.jsspec/SessionTokenCache.spec.jssrc/Auth.jssrc/Controllers/UserController.jssrc/LiveQuery/ParseCloudCodePublisher.jssrc/LiveQuery/ParseLiveQueryServer.tssrc/LiveQuery/QueryTools.jssrc/LiveQuery/SessionTokenCache.jssrc/RestQuery.jssrc/RestWrite.jssrc/Routers/FunctionsRouter.jssrc/Routers/SessionsRouter.jssrc/Routers/UsersRouter.jssrc/cloud-code/ObjectAdapter.jssrc/cloud-code/QueryAdapter.jssrc/rest.jssrc/triggers.js
💤 Files with no reviewable changes (4)
- src/LiveQuery/SessionTokenCache.js
- spec/ParseRole.spec.js
- spec/SessionTokenCache.spec.js
- spec/Auth.spec.js
| spyOn(parseLiveQueryServer, 'getAuthForSessionToken').and.returnValue( | ||
| Promise.resolve({ | ||
| userId: 'someUserId', | ||
| auth: { | ||
| getUserRoles: () => Promise.resolve(['role:liveQueryRead']), | ||
| }, | ||
| containedIn() { | ||
| shouldReturn = false; | ||
| return this; | ||
| }, | ||
| find() { | ||
| if (!shouldReturn) { | ||
| return Promise.resolve([]); | ||
| } | ||
| //Return a role with the name "liveQueryRead" as that is what was set on the ACL | ||
| const liveQueryRole = new Parse.Role('liveQueryRead', new Parse.ACL()); | ||
| liveQueryRole.id = 'abcdef1234'; | ||
| return Promise.resolve([liveQueryRole]); | ||
| }, | ||
| }; | ||
| }); | ||
|
|
||
| parseLiveQueryServer._matchesACL(acl, client, requestId).then(function (isMatched) { | ||
| expect(isMatched).toBe(false); | ||
| done(); | ||
| }); | ||
| }) | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect getAuthForSessionToken's return shape and how _matchesACL consumes it.
fd -t f 'ParseLiveQueryServer.ts' --exec rg -nP -C4 'getAuthForSessionToken|getUserRoles|userId|_matchesACL|_matchesACLWithSessionToken' {}Repository: parse-community/parse-server
Length of output: 7730
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the exact functions that consume getAuthForSessionToken and where auth.getUserRoles()/userId are used
fd -t f 'ParseLiveQueryServer.ts' --exec rg -n --color=never 'getAuthForSessionToken\(|getUserRoles\(|_matchesACLWithSessionToken|_matchesACL\(' {} \
| head -n 50
# Print the relevant surrounding blocks (avoid whole-file dumps)
fd -t f 'ParseLiveQueryServer.ts' --exec bash -lc 'sed -n "640,740p" "$1"; echo "-----"; sed -n "760,910p" "$1"' bash {}Repository: parse-community/parse-server
Length of output: 9190
🏁 Script executed:
#!/bin/bash
set -euo pipefail
f=$(fd -t f 'ParseLiveQueryServer.ts' | head -n 1)
echo "Using: $f"
# Print the remainder of _matchesACL and the call into ACL/role verification
python3 - <<'PY'
import re, pathlib
path = pathlib.Path("".join([""])).resolve()
PY
# fallback to sed windows; adjust by printing larger region around _matchesACL start
sed -n '900,1100p' "$f"Repository: parse-community/parse-server
Length of output: 7103
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1450,1605p' spec/ParseLiveQueryServer.spec.js | nl -ba | sed -n '1,200p'Repository: parse-community/parse-server
Length of output: 115
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the exact test cases around the stubbed getAuthForSessionToken usage (avoid huge output)
python3 - <<'PY'
import itertools, pathlib
p = pathlib.Path("spec/ParseLiveQueryServer.spec.js")
start, end = 1470, 1570
lines = p.read_text().splitlines()
for i in range(start-1, min(end, len(lines))):
print(f"{i+1}: {lines[i]}")
PYRepository: parse-community/parse-server
Length of output: 3954
Update: the getAuthForSessionToken stub shape matches _matchesACL/_verifyACL role checks
The stub’s return { userId, auth: { getUserRoles } } matches _verifyACL’s usage (const { auth, userId } = await ...; roleNames = await auth.getUserRoles(); acl.getReadAccess(role)), and the returned roles use the expected role:<name> form.
The two .then(...) chains in these tests (for lines ~1511-1524 and ~1539-1552) still lack .catch(done.fail) unlike the neighboring test, so a rejection could leave the spec hanging.
🤖 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 `@spec/ParseLiveQueryServer.spec.js` around lines 1511 - 1518, Two promise
chains in the tests that stub parseLiveQueryServer.getAuthForSessionToken (the
blocks that return { userId, auth: { getUserRoles } }) are missing error
handlers and can leave the spec hanging; update each of those .then(...) chains
to append .catch(done.fail) so rejections cause the test to fail and complete
(i.e., locate the tests that spyOn parseLiveQueryServer.getAuthForSessionToken
and add .catch(done.fail) to both promise chains).
| for (const key in restObject) { | ||
| copy[key] = restObject[key]; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify callsites that pass only one argument to inflateObject (would hit this path)
rg -nP --type=js --type=ts -C2 '\binflateObject\s*\(\s*[^,\n\)]*\s*\)'
# Verify two-argument callsites too, for impact estimation
rg -nP --type=js --type=ts -C2 '\binflateObject\s*\(\s*[^,\)]*\s*,'Repository: parse-community/parse-server
Length of output: 6832
Guard restObject in inflateObject to prevent a runtime TypeError (src/cloud-code/ObjectAdapter.js, lines 28-30)
inflateObject unconditionally runs for (const key in restObject); since multiple callsites invoke inflateObject(data) with only the first argument (e.g., src/Routers/SessionsRouter.js), restObject becomes undefined and for...in will throw at runtime.
💡 Proposed fix
export function inflateObject(data, restObject) {
- const copy = typeof data === 'object' ? data : { className: data };
- for (const key in restObject) {
- copy[key] = restObject[key];
- }
+ const copy = typeof data === 'object' ? { ...data } : { className: data };
+ if (restObject && typeof restObject === 'object') {
+ for (const key in restObject) {
+ copy[key] = restObject[key];
+ }
+ }
return Parse.Object.fromJSON(copy);
}📝 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.
| for (const key in restObject) { | |
| copy[key] = restObject[key]; | |
| } | |
| export function inflateObject(data, restObject) { | |
| const copy = typeof data === 'object' ? { ...data } : { className: data }; | |
| if (restObject && typeof restObject === 'object') { | |
| for (const key in restObject) { | |
| copy[key] = restObject[key]; | |
| } | |
| } | |
| return Parse.Object.fromJSON(copy); | |
| } |
🤖 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 `@src/cloud-code/ObjectAdapter.js` around lines 28 - 30, inflateObject
currently does an unconditional for (const key in restObject) which will throw
when restObject is undefined; update inflateObject to guard the loop by checking
that restObject is a non-null object (e.g., if (restObject && typeof restObject
=== 'object')) before iterating, so the copy assignment to copy[key] only runs
when restObject is present; keep references to the same symbols (inflateObject,
restObject, copy) so the fix is localized and safe for callsites that pass only
the first arg.
| // Converts a REST-format object to a Parse.Object. | ||
| // Thin re-export of the ObjectAdapter conversion seam so existing | ||
| // `triggers.inflate` callers keep working. | ||
| export const inflate = inflateObject; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check all usages of triggers.inflate to verify they match inflateObject's expected signature
rg -n "inflate\(" --type=js -C2 | grep -v "inflateObject\|inflateQuery"Repository: parse-community/parse-server
Length of output: 2599
🏁 Script executed:
#!/bin/bash
# Check inflateObject signature in ObjectAdapter
cat src/cloud-code/ObjectAdapter.js | head -50Repository: parse-community/parse-server
Length of output: 2501
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show relevant call sites where inflate is used with 1 argument vs 2 args
sed -n '140,220p' src/Controllers/UserController.jsRepository: parse-community/parse-server
Length of output: 2683
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show triggers.js around the re-export and also search for any previous inflate implementation
sed -n '930,1020p' src/triggers.js
rg -n "export (function|const) inflate\\b|\\binflate\\s*=\\s*\\(" src/triggers.jsRepository: parse-community/parse-server
Length of output: 2825
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the inflate usage in RestWrite and UsersRouter for context
sed -n '300,380p' src/RestWrite.js
sed -n '560,630p' src/Routers/UsersRouter.jsRepository: parse-community/parse-server
Length of output: 4900
Fix inflate re-export to avoid breaking single-argument call sites
inflateObject(data, restObject)now backstriggers.inflateand unconditionally iteratesfor (const key in restObject), so whenrestObjectis omitted it will throw (restObjectisundefined).- Existing one-argument call sites would now be broken, e.g.
src/Controllers/UserController.jscallsinflate({ className: '_User', ...fetchedUser })andinflate(Object.assign({ className: '_User' }, user)). - Either guard/default
restObjectinsideinflateObject(e.g. treat missing as{}) or update those call sites to pass the second argument.
🤖 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 `@src/triggers.js` around lines 983 - 986, The re-export triggers.inflate
currently points to inflateObject which expects two arguments and
unconditionally iterates for (const key in restObject), so one-argument call
sites will throw when restObject is undefined; fix by making inflate (or inside
inflateObject) guard the second parameter with a default empty object (e.g.
restObject = restObject || {}) before the for-in loop so existing
single-argument calls to inflate(...) continue to work, ensuring
inflate/inflateObject retain the same behavior for callers like triggers.inflate
and any code calling inflate(Object.assign(...)) or inflate({ className: ... }).
1 participant
Towards #8787. Isolates
Parse.Objectconstruction/inspection behind a newsrc/cloud-code/ObjectAdapter.js, mirroring theQueryAdapterfrom #10486. The SDK pending-ops state-controller seam is deferred to a focused follow-up.Note
Stacked on top of #10486 (QueryAdapter). Until that merges, the diff here includes its commit; it will drop out once #10486 lands.
Summary by CodeRabbit
Release Notes
Tests
SessionTokenCachetest suite.Refactor
Chores
SessionTokenCachemodule in favor of integrated session handling.