feat: Add Cloud Code Adapter system for multi-language cloud code#10218
Open
dblythy wants to merge 16 commits intoparse-community:alphafrom
Open
feat: Add Cloud Code Adapter system for multi-language cloud code#10218dblythy wants to merge 16 commits intoparse-community:alphafrom
dblythy wants to merge 16 commits intoparse-community:alphafrom
Conversation
Defines the architecture for a next-generation Parse.Cloud system that replaces triggers.js with a CloudCodeManager, supports BYO adapters, and enables multi-language cloud code via three built-in adapter types (Legacy, InProcess, ExternalProcess).
- Add beforePasswordResetRequest to TriggerName - Replace file/config trigger names with virtual className pattern (@file, @config, @connect) matching triggers.js internals - Add applicationId scoping (one CloudCodeManager per app on Config) - Add GlobalConfigRouter.js to consumer migration table - Add missing API methods (triggerExists, getJobs, runQueryTrigger, runFileTrigger, runGlobalConfigTrigger, runLiveQueryEventHandlers) - Clarify validators/rate-limiting only for LegacyAdapter - Document crash recovery ownership (manager calls unregisterAll)
- Rename getValidator param to key (covers both functions and triggers) - Note runTrigger subsumes maybeRunAfterFindTrigger - Make runLiveQueryEventHandlers synchronous to match existing behavior - Add defineTrigger validation rules (enforced for all adapters) - Add getRequestFileObject to utility function list
Comprehensive implementation plan addressing all review feedback: - LegacyAdapter loads files only (no Parse.Cloud patching) - triggers.js facade delegates reads AND writes with correct signatures - AppCache persistence survives Config.put() overwrites - _unregisterAll uses synchronous clearAll() for test cleanup - getJobsObject() for facade backwards compatibility - @parse-lite/cloud usage example in plan header
|
🚀 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. |
|
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:
📝 WalkthroughWalkthroughAdds a pluggable Cloud Code system: CloudCodeManager, type definitions, adapter implementations (in-process, external-process, legacy), adapter resolution and ParseServer integration, webhook bridge utilities, README, and new tests for registration, dispatch, lifecycle, health, and shutdown flows. Changes
Sequence DiagramsequenceDiagram
participant PS as ParseServer
participant RA as resolveAdapters
participant CCM as CloudCodeManager
participant Reg as Registry
participant IPA as InProcessAdapter
participant EPA as ExternalProcessAdapter
participant Ext as ExternalProcess
PS->>RA: resolveAdapters(options)
RA-->>PS: adapters[]
PS->>CCM: new CloudCodeManager()
PS->>CCM: initialize(adapters, config)
CCM->>Reg: createRegistry(adapter: in-process)
CCM->>IPA: initialize(registry, config)
IPA->>Reg: defineFunction/defineTrigger/defineJob
CCM->>Reg: createRegistry(adapter: external-process)
CCM->>EPA: initialize(registry, config)
EPA->>Ext: spawn process (env: PARSE_CLOUD_PORT...)
Ext-->>EPA: stdout "PARSE_CLOUD_READY:<port>"
EPA->>Ext: GET / -> manifest
EPA->>Reg: defineFunction/defineTrigger/defineJob
loop runtime invocation
PS->>CCM: invoke function/trigger/job (with appId)
CCM->>Reg: lookup entry
Reg-->>PS: execute via adapter bridge (router or HTTP)
end
PS->>CCM: shutdown()
CCM->>IPA: shutdown()
CCM->>EPA: shutdown()
EPA->>Ext: SIGTERM -> (timeout) SIGKILL
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Contributor
✅ 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. |
- Resolve merge conflicts in triggers.js and Definitions.js - Fix ESLint curly rule in InProcessAdapter spec - Fix TypeScript: import * as http, import Parse default - Fix Options/index.js: use ?Object instead of inline types (buildConfigDefinitions.js cannot parse Array<Object> or inline object type annotations)
dblythy
changed the title
- Regenerate Definitions.js and docs.js via npm run definitions - Fix lint: replace instanceof Map with toBeInstanceOf (no-restricted-syntax) - Fix cloud type validation: reject non-string/function/object cloud values to preserve existing "cloud code must be valid type" test - Docker build failure is QEMU/arm64 infra flake (not code-related)
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## alpha #10218 +/- ##
==========================================
- Coverage 92.60% 92.34% -0.26%
==========================================
Files 192 198 +6
Lines 16324 16730 +406
Branches 200 271 +71
==========================================
+ Hits 15117 15450 +333
- Misses 1190 1259 +69
- Partials 17 21 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (3)
src/cloud-code/resolveAdapters.ts (1)
18-19: Inconsistent error throwing pattern.Line 19 throws a string literal while line 25 throws an
Errorobject. For consistency and better stack traces, consider usingErrorobjects throughout.🔧 Suggested fix
} else { - throw "argument 'cloud' must either be a string or a function"; + throw new Error("argument 'cloud' must either be a string or a function"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cloud-code/resolveAdapters.ts` around lines 18 - 19, The code in resolveAdapters.ts throws a string for the invalid 'cloud' argument; change it to throw an Error object for consistency and proper stack traces (e.g., replace the string throw with throw new Error("argument 'cloud' must either be a string or a function")). Also scan the surrounding function (the cloud adapter resolution logic where other errors use new Error(...)) and ensure all thrown values (including the existing throw on line with Error object) use Error instances rather than string literals.src/ParseServer.ts (1)
198-199: RedundantaddParseCloud()call.
addParseCloud()is already called unconditionally at line 54 during module initialization. The call here at line 199 is redundant sinceaddParseCloud()only assigns properties and doesn't check if they're already set. Consider removing this call or adding a comment if there's a specific reason for re-initialization at this point.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ParseServer.ts` around lines 198 - 199, The call to addParseCloud() inside the adapters-length conditional is redundant because addParseCloud() is already invoked during module initialization; remove the addParseCloud() invocation from the block that checks adapters.length (or, if re-initialization is intentional, make addParseCloud() idempotent and add a clear comment explaining why it must run twice). Update the code around the adapters check to either drop the addParseCloud() call or wrap it with an explicit guard (or adjust addParseCloud() to noop when already initialized) so properties are not blindly reassigned.src/Options/docs.js (1)
30-30: Type annotation inconsistency:cloudCodeAdaptersis an array, not an object.The documentation shows
@property {Object} cloudCodeAdaptersbut the description says "Array of CloudCodeAdapter instances". Consider updating the type to{Array}or{CloudCodeAdapter[]}insrc/Options/index.jsand regenerating withnpm run definitions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Options/docs.js` at line 30, Update the JSDoc type for the cloudCodeAdapters property to reflect that it's an array (e.g., use {Array} or {CloudCodeAdapter[]} instead of {Object}) in the Options documentation and source definition where cloudCodeAdapters is declared so generated types match the description; after changing the JSDoc/type in the Options definition (the symbol cloudCodeAdapters in your options module), regenerate the definitions with npm run definitions to update docs and type artifacts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@spec/ExternalProcessAdapter.spec.js`:
- Around line 6-28: createMockCloudServer currently hardcodes port 19876, never
rejects on listen errors, and the test only shuts down the adapter on the happy
path; change createMockCloudServer to listen on an ephemeral port by passing 0
to server.listen, reject the Promise on server 'error' and resolve with both the
server instance and the actual bound port (server.address().port), and update
the test teardown to always run in an awaited finally block that closes the mock
server (await a Promise-wrapped server.close) and shuts down the
ExternalProcessAdapter (await adapter.shutdown) so no handles are left if
initialization or assertions fail; apply the same fixes to the other occurrence
of createMockCloudServer used in lines ~46-68.
In `@src/cloud-code/adapters/ExternalProcessAdapter.ts`:
- Around line 183-197: The beforeSave branch currently calls
applyBeforeSaveResponse(request, response) which mutates request.object, but
file triggers use a different request shape (request.file and request.fileSize);
add a special-case when triggerName === 'beforeSave' and the incoming request is
a file trigger (e.g., detect presence of request.file or className === 'File')
to translate the webhook response into the file-shaped result instead of
mutating request.object: parse the response to extract the new file URL/path and
size, set request.file and request.fileSize (or construct the appropriate file
response shape) and return; otherwise keep the existing applyBeforeSaveResponse
flow. Reference manifest.hooks.triggers, registry.defineTrigger,
requestToWebhookBody, applyBeforeSaveResponse, and webhookResponseToResult to
locate and modify the logic.
- Around line 80-88: initialize() currently spawns the child via
spawnAndWaitForReady() but if fetchManifest() or registerFromManifest() throws
the child is left running; wrap the post-spawn work in a try/catch (after
calling spawnAndWaitForReady()) and on any error ensure you terminate/cleanup
the spawned process and any healthInterval (call the adapter's
shutdown/stop/child-kill routine or clearInterval on healthInterval and kill the
child created by spawnAndWaitForReady()), then rethrow the error so callers see
the failure; update initialize() to perform this cleanup path referencing
initialize, spawnAndWaitForReady, fetchManifest, registerFromManifest,
healthInterval and options.healthCheckInterval.
- Around line 21-29: httpGet and httpPost must enforce request timeouts and
validate HTTP status codes to avoid hangs: update the httpGet and httpPost
implementations to set a socket/request timeout (e.g., via setTimeout on the
request and res.socket) that rejects the promise on timeout, and when the
response arrives reject for any non-2xx status (include statusCode and
statusMessage in the error). Also adjust the health check logic (the health
check that inspects the child process HTTP response) so it only treats responses
with a successful 2xx status as healthy and validates the body (e.g., exact "ok"
or trimmed equality) rather than substring matching. Ensure fetchManifest usage
continues to handle the promise rejections from these functions so startup fails
fast on unreachable/unhealthy child processes.
In `@src/cloud-code/adapters/InProcessAdapter.ts`:
- Around line 29-39: The beforeSave branch assumes request.object always exists
and calls applyBeforeSaveResponse, which breaks for `@File` beforeSave hooks;
update the registry.defineTrigger handler (the async request => { ... } block in
InProcessAdapter.ts where requestToWebhookBody, router.dispatchTrigger,
applyBeforeSaveResponse and webhookResponseToResult are used) to check whether
request.object is present before calling applyBeforeSaveResponse — if
request.object is missing (file hooks) then convert the router response via
webhookResponseToResult and return that instead; otherwise keep the existing
applyBeforeSaveResponse path.
In `@src/cloud-code/adapters/LegacyAdapter.ts`:
- Around line 22-34: The code in LegacyAdapter currently tries to infer ESM from
package.json and falls back from import() to require() in a broad catch; change
the loader to attempt require(resolved) first inside a try and catch only errors
with code === 'ERR_REQUIRE_ESM', rethrow any other errors so real evaluation
problems surface; when falling back to import use the URL-safe form (convert
resolved with url.pathToFileURL(resolved).href) and await import(...) so dynamic
import works for filesystem paths. Reference the string-path handling around
this.cloud and the module-loading logic in LegacyAdapter to locate and replace
the current if/try/catch block.
In `@src/cloud-code/CloudCodeManager.ts`:
- Around line 270-274: CloudCodeManager.shutdown currently awaits each
adapter.shutdown() directly so the first rejection aborts the loop; change
shutdown to perform best-effort shutdown by invoking adapter.shutdown() for each
adapter while catching and logging individual errors (so one failure doesn't
stop others), then clear/reset manager state by emptying this.adapters and any
registries (e.g., service/instance registries held on the manager) and resetting
any "started" flags so the manager can be reused; reference
CloudCodeManager.shutdown, this.adapters, adapter.shutdown and the registries to
locate where to add per-adapter try/catch and state-clearing logic.
- Around line 252-267: If adapter.initialize can register hooks and then throw,
wrap the await adapter.initialize(registry, config) in a try/catch and on error
roll back any partial registrations for that adapter by using the registry
returned from createRegistry(adapter.name) (e.g., call
registry.rollback/registry.dispose/registry.clearRegistrations or
removeRegisteredHooks), and also call adapter.shutdown()/adapter.dispose() if
the adapter exposes a teardown method (to stop any started child processes like
ExternalProcessAdapter). Only push the adapter into this.adapters after
successful initialization; in the catch rethrow the error after cleanup so
startup fails but leaves no orphaned registrations or processes.
In `@src/cloud-code/README.md`:
- Around line 277-282: The Go example's import block is missing the "net"
package needed by net.Listen and net.TCPAddr; update the import list in the
file's example to include "net" so that net.Listen() and net.TCPAddr compile,
ensuring the import block contains "encoding/json", "fmt", "net", "net/http",
and "os" (or equivalent ordering) to satisfy references to net.Listen and
net.TCPAddr used later.
In `@src/Options/index.js`:
- Around line 142-151: The Options definitions expose outdated types: update the
ParseServerOptions entries for cloud, cloudCodeAdapters and cloudCodeOptions in
src/Options/index.js so they reflect the new cloud adapter contract (e.g., allow
cloud to accept either a filesystem path string or a router/object, and change
cloudCodeAdapters from ?Object to the new union type accepting a Router/object
or an array of CloudCodeAdapter instances; also tighten cloudCodeOptions to the
expected options shape instead of a generic Object). After updating the type
signatures for cloud, cloudCodeAdapters and cloudCodeOptions, regenerate
src/Options/Definitions.js and src/Options/docs.js so the changes propagate to
the runtime definitions and docs.
In `@src/triggers.js`:
- Around line 156-163: addFunction stores per-app validators on the manager via
getManager(applicationId), but maybeRunValidator still resolves validators with
the global Parse.applicationId; update maybeRunValidator (and the other
occurrences around 352-356) to pass the correct applicationId (e.g., the
request's/application context's applicationId or the manager derived via
getManager(applicationId)) into getValidator instead of using
Parse.applicationId so the validator lookup uses the same per-app manager as the
handler; alternatively call manager.getValidator(functionName) when a manager is
available to ensure per-application resolution.
---
Nitpick comments:
In `@src/cloud-code/resolveAdapters.ts`:
- Around line 18-19: The code in resolveAdapters.ts throws a string for the
invalid 'cloud' argument; change it to throw an Error object for consistency and
proper stack traces (e.g., replace the string throw with throw new
Error("argument 'cloud' must either be a string or a function")). Also scan the
surrounding function (the cloud adapter resolution logic where other errors use
new Error(...)) and ensure all thrown values (including the existing throw on
line with Error object) use Error instances rather than string literals.
In `@src/Options/docs.js`:
- Line 30: Update the JSDoc type for the cloudCodeAdapters property to reflect
that it's an array (e.g., use {Array} or {CloudCodeAdapter[]} instead of
{Object}) in the Options documentation and source definition where
cloudCodeAdapters is declared so generated types match the description; after
changing the JSDoc/type in the Options definition (the symbol cloudCodeAdapters
in your options module), regenerate the definitions with npm run definitions to
update docs and type artifacts.
In `@src/ParseServer.ts`:
- Around line 198-199: The call to addParseCloud() inside the adapters-length
conditional is redundant because addParseCloud() is already invoked during
module initialization; remove the addParseCloud() invocation from the block that
checks adapters.length (or, if re-initialization is intentional, make
addParseCloud() idempotent and add a clear comment explaining why it must run
twice). Update the code around the adapters check to either drop the
addParseCloud() call or wrap it with an explicit guard (or adjust
addParseCloud() to noop when already initialized) so properties are not blindly
reassigned.
🪄 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: 672c337b-83b3-4790-b361-24179dad9283
📒 Files selected for processing (17)
spec/CloudCodeAdapter.integration.spec.jsspec/CloudCodeManager.spec.jsspec/ExternalProcessAdapter.spec.jsspec/InProcessAdapter.spec.jssrc/Options/Definitions.jssrc/Options/docs.jssrc/Options/index.jssrc/ParseServer.tssrc/cloud-code/CloudCodeManager.tssrc/cloud-code/README.mdsrc/cloud-code/adapters/ExternalProcessAdapter.tssrc/cloud-code/adapters/InProcessAdapter.tssrc/cloud-code/adapters/LegacyAdapter.tssrc/cloud-code/adapters/webhook-bridge.tssrc/cloud-code/resolveAdapters.tssrc/cloud-code/types.tssrc/triggers.js
- Use ephemeral ports and proper teardown in ExternalProcessAdapter specs - Add HTTP timeout/status validation and file trigger handling in ExternalProcessAdapter - Guard request.object for file hooks in InProcessAdapter beforeSave - Fix ESM/CJS loading order in LegacyAdapter (require-first, catch ERR_REQUIRE_ESM) - Best-effort shutdown and initialize rollback in CloudCodeManager - Add missing "net" import in README Go example - Update Options types for cloud, cloudCodeAdapters, cloudCodeOptions - Fix validator applicationId lookup in triggers.js and ParseLiveQueryServer - Throw Error objects instead of strings in resolveAdapters - Update JSDoc types in docs.js - Clarify intentional addParseCloud re-invocation in ParseServer
- Use ephemeral ports and proper teardown in ExternalProcessAdapter specs - Add HTTP timeout/status validation and file trigger handling in ExternalProcessAdapter - Guard request.object for file hooks in InProcessAdapter beforeSave - Fix ESM/CJS loading order in LegacyAdapter (require-first, catch ERR_REQUIRE_ESM) - Best-effort shutdown and initialize rollback in CloudCodeManager - Add missing "net" import in README Go example - Update Options types for cloud, cloudCodeAdapters, cloudCodeOptions - Fix validator applicationId lookup in triggers.js and ParseLiveQueryServer - Throw Error objects instead of strings in resolveAdapters - Update JSDoc types in docs.js - Clarify intentional addParseCloud re-invocation in ParseServer
dblythy
force-pushed
the
feature/cloud-adapter
branch
from
7cdf253 to
a1c9162
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/triggers.js (1)
266-275:⚠️ Potential issue | 🔴 CriticalDon’t break callers that still omit
applicationId.
src/Routers/HooksRouter.js:51-53still doesgetTrigger(req.params.className, req.params.triggerName)with two arguments. Throwing here turns that route into a 500 instead of preserving the old fallback or updating that caller first.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/triggers.js` around lines 266 - 275, The current getTrigger function throws when applicationId is falsy which breaks older callers; instead remove the throw and preserve the legacy fallback path: if applicationId is falsy, skip getManager and return get(Category.Triggers, `${triggerType}.${className}`, applicationId) so callers that pass only className and triggerType continue to work; otherwise, keep the existing logic that calls getManager(applicationId) and returns manager.getTrigger(...).handler or the get(...) fallback when manager is absent.src/LiveQuery/ParseLiveQueryServer.ts (1)
235-245:⚠️ Potential issue | 🟠 MajorFinish propagating
this.config.appIdthrough LiveQuery.These trigger lookups are now app-scoped, but
_createSubscribers()still subscribes and routesafterSave,afterDelete, andclearCachewithParse.applicationId, and the websocket lifecycle paths still callrunLiveQueryEventHandlers()without an app id. In a multi-app process, the first LiveQuery server will start missing its own pubsub messages once another app reinitializes Parse.🔧 Places that still need the same treatment
- if (channel === Parse.applicationId + 'clearCache') { + if (channel === `${this.config.appId}clearCache`) { this._clearCachedRoles(message.userId); return; } - if (channel === Parse.applicationId + 'afterSave') { + if (channel === `${this.config.appId}afterSave`) { this._onAfterSave(message); - } else if (channel === Parse.applicationId + 'afterDelete') { + } else if (channel === `${this.config.appId}afterDelete`) { this._onAfterDelete(message); } ... - const channel = `${Parse.applicationId}${field}`; + const channel = `${this.config.appId}${field}`; this.subscriber.subscribe(channel, messageStr => messageRecieved(channel, messageStr));- runLiveQueryEventHandlers({ + runLiveQueryEventHandlers({ event: 'ws_connect', clients: this.clients.size, subscriptions: this.subscriptions.size, - }); + }, this.config.appId);Also applies to: 391-403, 848-855, 911-922
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/LiveQuery/ParseLiveQueryServer.ts` around lines 235 - 245, The LiveQuery code must pass the current server instance app id into all trigger lookups and event routing instead of using the global Parse.applicationId; update _createSubscribers, runLiveQueryEventHandlers, and any websocket lifecycle handlers to accept and forward this.config.appId (e.g., change calls that use Parse.applicationId to pass this.config.appId and add an appId parameter to runLiveQueryEventHandlers and subscriber callbacks), ensure runTrigger invocations (like the shown afterEvent block) receive this.config.appId consistently, and propagate that appId through any auth/subscription resolution paths so each app's pubsub messages are isolated.
♻️ Duplicate comments (3)
src/Options/index.js (1)
154-155:⚠️ Potential issue | 🟠 MajorNarrow
cloudto the supported object shape.At runtime,
resolveAdapters()only accepts strings, functions, or objects withgetRouter(), but the public type here still allows anyObject. That means arrays/plain objects can look valid in the option contract and then fail at startup.♻️ Suggested type tightening
- cloud: ?(string | Object); + cloud: ?(string | ((parse: any) => void) | { getRouter: () => any });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Options/index.js` around lines 154 - 155, The public Options type currently declares cloud: ?(string | Object) which allows arbitrary objects; tighten this to only the supported shapes accepted by resolveAdapters() by changing the cloud option type to allow string, Function, or an object with a getRouter method (e.g., (string | Function | { getRouter: Function })). Update the cloud declaration in src/Options/index.js and any related exported types so arrays/plain objects are rejected at type-check time and only strings, functions, or objects implementing getRouter() are allowed.src/cloud-code/CloudCodeManager.ts (1)
263-279:⚠️ Potential issue | 🔴 CriticalRollback earlier adapters if a later
initialize()fails.This catch only unwinds the adapter that just threw. If adapter B fails after adapter A succeeded, A stays in
this.adapterswith its hooks/resources live, andParseServer.start()does not clean it up on the error path.🔁 Expected rollback shape
async initialize(adapters: CloudCodeAdapter[], config: ParseServerConfig): Promise<void> { const seen = new Set<string>(); + const initialized: CloudCodeAdapter[] = []; for (const adapter of adapters) { if (seen.has(adapter.name)) { throw new Error( `Duplicate adapter name "${adapter.name}". Each adapter must have a unique name.` ); @@ for (const adapter of adapters) { const registry = this.createRegistry(adapter.name); try { await adapter.initialize(registry, config); + initialized.push(adapter); + this.adapters.push(adapter); } catch (error) { // Roll back any partial registrations from this adapter this.unregisterAll(adapter.name); // Attempt graceful shutdown of the failed adapter try { await adapter.shutdown(); } catch { // Ignore shutdown errors during initialization rollback } + for (const previous of [...initialized].reverse()) { + this.unregisterAll(previous.name); + try { + await previous.shutdown(); + } catch { + // Ignore rollback shutdown errors + } + } + this.adapters.length = 0; + this.clearAll(); throw error; } - this.adapters.push(adapter); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cloud-code/CloudCodeManager.ts` around lines 263 - 279, When an adapter's initialize() throws, only that adapter is currently rolled back; previously-initialized adapters remain active in this.adapters. In the catch block inside the adapters loop in CloudCodeManager (around createRegistry, initialize, unregisterAll, shutdown, and this.adapters), iterate the already-added adapters (this.adapters) in reverse order, call this.unregisterAll(prevAdapter.name) for each, await prevAdapter.shutdown() inside a try/catch to ignore shutdown errors, and remove/clear them from this.adapters before rethrowing the original error so no prior adapters remain active after a failed initialize().src/cloud-code/adapters/InProcessAdapter.ts (1)
34-39:⚠️ Potential issue | 🟠 MajorTreat empty file
beforeSaveresponses as no-op.
applyBeforeSaveResponse()already treats the normal no-op webhook payload (success: {}) as “no changes.” On the file path we return that raw{}instead, andsrc/triggers.jslater treats it as a real replacement because{}is truthy, so a no-opbeforeSaveFilecan return{}instead of the original{ file, fileSize }.🩹 Minimal fix
if (triggerName === 'beforeSave') { if (request.object) { applyBeforeSaveResponse(request, response); return; } - return webhookResponseToResult(response); + const result = webhookResponseToResult(response); + if ( + result && + typeof result === 'object' && + !Array.isArray(result) && + Object.keys(result).length === 0 + ) { + return; + } + return result; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cloud-code/adapters/InProcessAdapter.ts` around lines 34 - 39, When handling beforeSave for the file path (triggerName === 'beforeSave' and request.object is falsy) we currently return webhookResponseToResult(response) directly which treats an empty success payload ({}) as a real replacement; instead call webhookResponseToResult(response) into a variable, and if that value is a plain object with no own properties return undefined (or null) to represent a no-op; otherwise return the computed result. Keep the existing branch that calls applyBeforeSaveResponse(request, response) when request.object exists unchanged.
🤖 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/cloud-code/adapters/ExternalProcessAdapter.ts`:
- Around line 81-96: The adapter currently hard-codes readonly name =
'external-process' in ExternalProcessAdapter, which prevents registering
multiple adapters; change the constructor signature of ExternalProcessAdapter to
accept an optional name (e.g., constructor(command: string, webhookKey: string,
options?: CloudCodeOptions, name?: string)) and set the instance name to that
value or to a generated unique name (for example derived from webhookKey or
command) instead of the constant; update any references to the class
instantiation to pass a distinct name when creating multiple adapters so
CloudCodeManager.initialize can distinguish them.
In `@src/ParseServer.ts`:
- Around line 198-223: When stopping Parse Server, ensure any long-lived
CloudCodeManager started in start() is shut down: update the shutdown path
(handleShutdown / stop / close method on the ParseServer class) to look up the
manager (this.config.cloudCodeManager or AppCache.get(appId)?.cloudCodeManager)
and invoke its teardown method (e.g., cloudManager.shutdown() or
cloudManager.stop()), awaiting its completion before process exit so
ExternalProcessAdapter child processes are terminated; add null checks and error
logging around CloudCodeManager shutdown to avoid throwing during shutdown.
---
Outside diff comments:
In `@src/LiveQuery/ParseLiveQueryServer.ts`:
- Around line 235-245: The LiveQuery code must pass the current server instance
app id into all trigger lookups and event routing instead of using the global
Parse.applicationId; update _createSubscribers, runLiveQueryEventHandlers, and
any websocket lifecycle handlers to accept and forward this.config.appId (e.g.,
change calls that use Parse.applicationId to pass this.config.appId and add an
appId parameter to runLiveQueryEventHandlers and subscriber callbacks), ensure
runTrigger invocations (like the shown afterEvent block) receive
this.config.appId consistently, and propagate that appId through any
auth/subscription resolution paths so each app's pubsub messages are isolated.
In `@src/triggers.js`:
- Around line 266-275: The current getTrigger function throws when applicationId
is falsy which breaks older callers; instead remove the throw and preserve the
legacy fallback path: if applicationId is falsy, skip getManager and return
get(Category.Triggers, `${triggerType}.${className}`, applicationId) so callers
that pass only className and triggerType continue to work; otherwise, keep the
existing logic that calls getManager(applicationId) and returns
manager.getTrigger(...).handler or the get(...) fallback when manager is absent.
---
Duplicate comments:
In `@src/cloud-code/adapters/InProcessAdapter.ts`:
- Around line 34-39: When handling beforeSave for the file path (triggerName ===
'beforeSave' and request.object is falsy) we currently return
webhookResponseToResult(response) directly which treats an empty success payload
({}) as a real replacement; instead call webhookResponseToResult(response) into
a variable, and if that value is a plain object with no own properties return
undefined (or null) to represent a no-op; otherwise return the computed result.
Keep the existing branch that calls applyBeforeSaveResponse(request, response)
when request.object exists unchanged.
In `@src/cloud-code/CloudCodeManager.ts`:
- Around line 263-279: When an adapter's initialize() throws, only that adapter
is currently rolled back; previously-initialized adapters remain active in
this.adapters. In the catch block inside the adapters loop in CloudCodeManager
(around createRegistry, initialize, unregisterAll, shutdown, and this.adapters),
iterate the already-added adapters (this.adapters) in reverse order, call
this.unregisterAll(prevAdapter.name) for each, await prevAdapter.shutdown()
inside a try/catch to ignore shutdown errors, and remove/clear them from
this.adapters before rethrowing the original error so no prior adapters remain
active after a failed initialize().
In `@src/Options/index.js`:
- Around line 154-155: The public Options type currently declares cloud:
?(string | Object) which allows arbitrary objects; tighten this to only the
supported shapes accepted by resolveAdapters() by changing the cloud option type
to allow string, Function, or an object with a getRouter method (e.g., (string |
Function | { getRouter: Function })). Update the cloud declaration in
src/Options/index.js and any related exported types so arrays/plain objects are
rejected at type-check time and only strings, functions, or objects implementing
getRouter() are allowed.
🪄 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: b9dbb464-3d1a-4df8-bca0-0ce1e4def094
📒 Files selected for processing (13)
spec/ExternalProcessAdapter.spec.jssrc/LiveQuery/ParseLiveQueryServer.tssrc/Options/Definitions.jssrc/Options/docs.jssrc/Options/index.jssrc/ParseServer.tssrc/cloud-code/CloudCodeManager.tssrc/cloud-code/README.mdsrc/cloud-code/adapters/ExternalProcessAdapter.tssrc/cloud-code/adapters/InProcessAdapter.tssrc/cloud-code/adapters/LegacyAdapter.tssrc/cloud-code/resolveAdapters.tssrc/triggers.js
🚧 Files skipped from review as they are similar to previous changes (5)
- src/cloud-code/adapters/LegacyAdapter.ts
- src/Options/Definitions.js
- spec/ExternalProcessAdapter.spec.js
- src/Options/docs.js
- src/cloud-code/README.md
Comment on lines
+198
to
223
| if (adapters.length > 0) { | ||
| // Re-invoke addParseCloud with the concrete appId so that Parse.Cloud | ||
| // methods (define, beforeSave, etc.) bind the correct appId in their | ||
| // closure. The module-level call (no appId) uses a lazy fallback to | ||
| // Parse.applicationId; this call upgrades it to a fixed binding before | ||
| // cloud code adapters run. | ||
| addParseCloud(this.config.appId); | ||
| const cloudManager = new CloudCodeManager(); | ||
|
|
||
| // CRITICAL: Store on this.config BEFORE adapter initialization. | ||
| // this.config flows into AppCache via Config.put() later in start(). | ||
| // We must also store it on AppCache NOW so the facade can find it | ||
| // during LegacyAdapter.initialize() → Parse.Cloud.define() → triggers.addFunction(). | ||
| this.config.cloudCodeManager = cloudManager; | ||
| const appId = this.config.appId; | ||
| const cached = AppCache.get(appId); | ||
| if (cached) { | ||
| cached.cloudCodeManager = cloudManager; | ||
| } | ||
| await new Promise(resolve => setTimeout(resolve, 10)); | ||
|
|
||
| await cloudManager.initialize(adapters, { | ||
| appId, | ||
| masterKey: this.config.masterKey, | ||
| serverURL: this.config.serverURL || `http://localhost:${this.config.port}${this.config.mountPath || '/parse'}`, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Shut cloud code adapters down when Parse Server stops.
start() now creates a long-lived CloudCodeManager, but handleShutdown() never invokes it. With ExternalProcessAdapter, the child process survives server shutdown and keeps adapter-owned state alive after the main server is gone.
🧹 Suggested teardown hook
if (this.liveQueryServer) {
promises.push(this.liveQueryServer.shutdown());
}
+ if (this.config.cloudCodeManager) {
+ promises.push(this.config.cloudCodeManager.shutdown());
+ }
await Promise.all(promises);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ParseServer.ts` around lines 198 - 223, When stopping Parse Server,
ensure any long-lived CloudCodeManager started in start() is shut down: update
the shutdown path (handleShutdown / stop / close method on the ParseServer
class) to look up the manager (this.config.cloudCodeManager or
AppCache.get(appId)?.cloudCodeManager) and invoke its teardown method (e.g.,
cloudManager.shutdown() or cloudManager.stop()), awaiting its completion before
process exit so ExternalProcessAdapter child processes are terminated; add null
checks and error logging around CloudCodeManager shutdown to avoid throwing
during shutdown.
There was a problem hiding this comment.
♻️ Duplicate comments (3)
spec/ExternalProcessAdapter.spec.js (1)
64-69:⚠️ Potential issue | 🟡 MinorAwait mock server shutdown in teardown.
At Line 68,
server.close()is still fire-and-forget. This can leave open handles when the test exits early or under CI load.💡 Suggested fix
} finally { if (adapter) { await adapter.shutdown(); } - server.close(); + await new Promise(resolve => server.close(resolve)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/ExternalProcessAdapter.spec.js` around lines 64 - 69, The teardown currently calls server.close() without awaiting it, which can leave open handles; update the teardown to await the mock server shutdown by using a promise-based close (e.g., wrap server.close in a Promise or use server.closeAsync) and replace the fire-and-forget call with await server.close() after awaiting adapter.shutdown() so the test waits for the server to fully close before exiting.src/ParseServer.ts (1)
289-292:⚠️ Potential issue | 🟠 MajorShutdown path still misses CloudCodeManager teardown.
start()now creates a long-lived manager, buthandleShutdown()does not shut it down. That can leave external adapter processes running after server stop.💡 Suggested fix
if (this.liveQueryServer) { promises.push(this.liveQueryServer.shutdown()); } + if (this.config.cloudCodeManager) { + promises.push(this.config.cloudCodeManager.shutdown()); + } await Promise.all(promises);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ParseServer.ts` around lines 289 - 292, handleShutdown() currently shuts down liveQueryServer but does not tear down the long-lived CloudCodeManager created in start(); update handleShutdown() to also stop the CloudCodeManager instance (the same instance created in start(), e.g. this.cloudCodeManager) by invoking its shutdown/stop/teardown method (e.g. this.cloudCodeManager.shutdown()) and include that promise in the promises array before awaiting Promise.all(promises).src/cloud-code/adapters/ExternalProcessAdapter.ts (1)
81-82:⚠️ Potential issue | 🟠 MajorLet each external adapter expose a distinct
name.
CloudCodeManager.initialize()rejects duplicate adapter names, so hard-coding'external-process'here makescloudCodeAdaptersunable to host two external processes. That blocks the natural one-process-per-runtime setup for multi-language cloud code.♻️ One possible direction
export class ExternalProcessAdapter implements CloudCodeAdapter { - readonly name = 'external-process'; + readonly name: string; private command: string; private webhookKey: string; private options: Required<CloudCodeOptions>; private process: ChildProcess | null = null; private port: number = 0; private healthInterval: ReturnType<typeof setInterval> | null = null; - constructor(command: string, webhookKey: string, options?: CloudCodeOptions) { + constructor( + command: string, + webhookKey: string, + options?: CloudCodeOptions, + name = `external-process:${command.split(/\s+/)[0]}` + ) { if (!webhookKey) { throw new Error('webhookKey is required for ExternalProcessAdapter'); } + this.name = name; this.command = command; this.webhookKey = webhookKey; this.options = { ...DEFAULT_OPTIONS, ...options }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cloud-code/adapters/ExternalProcessAdapter.ts` around lines 81 - 82, The adapter currently hard-codes readonly name = 'external-process' which prevents multiple instances with different names; change ExternalProcessAdapter to accept a name in its constructor (e.g., constructor(name: string, ...)) and assign it to an instance property (this.name = name) instead of the fixed literal, providing a sensible default like 'external-process' if none is passed; update any constructor callers to pass a unique identifier when creating multiple adapters so CloudCodeManager.initialize() can detect distinct adapters by the instance name.
🧹 Nitpick comments (1)
src/cloud-code/CloudCodeManager.ts (1)
298-306: Consider parallelizing health checks for faster feedback.The sequential
await adapter.isHealthy()calls could be slow with many adapters. UsingPromise.allwould provide faster aggregate health status.♻️ Optional: Parallel health checks
async healthCheck(): Promise<boolean> { - for (const adapter of this.adapters) { - const healthy = await adapter.isHealthy(); - if (!healthy) { - return false; - } - } - return true; + const results = await Promise.all( + this.adapters.map(adapter => adapter.isHealthy()) + ); + return results.every(healthy => healthy); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cloud-code/CloudCodeManager.ts` around lines 298 - 306, The healthCheck method currently awaits each adapter.isHealthy() sequentially; update CloudCodeManager.healthCheck to run all adapters' isHealthy calls in parallel by mapping this.adapters to an array of Promises (adapter.isHealthy()) and await Promise.all or Promise.allSettled, then aggregate results (e.g., return results.every(Boolean)), and treat any rejected promise as unhealthy (convert rejections to false) so a failure doesn't throw but causes healthCheck to return false; use the existing symbols healthCheck, this.adapters, and adapter.isHealthy to locate the code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@spec/ExternalProcessAdapter.spec.js`:
- Around line 64-69: The teardown currently calls server.close() without
awaiting it, which can leave open handles; update the teardown to await the mock
server shutdown by using a promise-based close (e.g., wrap server.close in a
Promise or use server.closeAsync) and replace the fire-and-forget call with
await server.close() after awaiting adapter.shutdown() so the test waits for the
server to fully close before exiting.
In `@src/cloud-code/adapters/ExternalProcessAdapter.ts`:
- Around line 81-82: The adapter currently hard-codes readonly name =
'external-process' which prevents multiple instances with different names;
change ExternalProcessAdapter to accept a name in its constructor (e.g.,
constructor(name: string, ...)) and assign it to an instance property (this.name
= name) instead of the fixed literal, providing a sensible default like
'external-process' if none is passed; update any constructor callers to pass a
unique identifier when creating multiple adapters so
CloudCodeManager.initialize() can detect distinct adapters by the instance name.
In `@src/ParseServer.ts`:
- Around line 289-292: handleShutdown() currently shuts down liveQueryServer but
does not tear down the long-lived CloudCodeManager created in start(); update
handleShutdown() to also stop the CloudCodeManager instance (the same instance
created in start(), e.g. this.cloudCodeManager) by invoking its
shutdown/stop/teardown method (e.g. this.cloudCodeManager.shutdown()) and
include that promise in the promises array before awaiting
Promise.all(promises).
---
Nitpick comments:
In `@src/cloud-code/CloudCodeManager.ts`:
- Around line 298-306: The healthCheck method currently awaits each
adapter.isHealthy() sequentially; update CloudCodeManager.healthCheck to run all
adapters' isHealthy calls in parallel by mapping this.adapters to an array of
Promises (adapter.isHealthy()) and await Promise.all or Promise.allSettled, then
aggregate results (e.g., return results.every(Boolean)), and treat any rejected
promise as unhealthy (convert rejections to false) so a failure doesn't throw
but causes healthCheck to return false; use the existing symbols healthCheck,
this.adapters, and adapter.isHealthy to locate the code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d95ab334-8b79-445a-9ba3-cb78a2405152
📒 Files selected for processing (13)
spec/ExternalProcessAdapter.spec.jssrc/LiveQuery/ParseLiveQueryServer.tssrc/Options/Definitions.jssrc/Options/docs.jssrc/Options/index.jssrc/ParseServer.tssrc/cloud-code/CloudCodeManager.tssrc/cloud-code/README.mdsrc/cloud-code/adapters/ExternalProcessAdapter.tssrc/cloud-code/adapters/InProcessAdapter.tssrc/cloud-code/adapters/LegacyAdapter.tssrc/cloud-code/resolveAdapters.tssrc/triggers.js
🚧 Files skipped from review as they are similar to previous changes (4)
- src/cloud-code/adapters/InProcessAdapter.ts
- src/cloud-code/adapters/LegacyAdapter.ts
- src/cloud-code/resolveAdapters.ts
- src/cloud-code/README.md
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/cloud-code/CloudCodeManager.ts (1)
164-166: Return a defensive copy to prevent external mutation.
getJobs()returns the internal Map directly, allowing callers to mutate manager state viagetJobs().set()ordelete(). Consider returning a new Map or making this read-only.♻️ Proposed fix
getJobs(): Map<string, JobEntry> { - return this.jobs; + return new Map(this.jobs); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cloud-code/CloudCodeManager.ts` around lines 164 - 166, getJobs() currently returns the internal Map "jobs" allowing external callers to mutate manager state; change getJobs() to return a defensive copy (e.g., return new Map(this.jobs)) or a ReadonlyMap to prevent external mutation, ensuring callers cannot call set/delete on the manager's internal "jobs" Map while preserving existing external 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/cloud-code/CloudCodeManager.ts`:
- Around line 182-186: The runLiveQueryEventHandlers method currently invokes
each entry.handler(data) directly, so a thrown exception aborts the loop; update
runLiveQueryEventHandlers to call each handler inside a try-catch so one failing
handler doesn't prevent subsequent handlers from running, catching and logging
the error (including context such as the entry or handler identity) via the
existing logger or error reporting mechanism; ensure you still pass the original
data to every handler and do not rethrow inside the loop.
---
Nitpick comments:
In `@src/cloud-code/CloudCodeManager.ts`:
- Around line 164-166: getJobs() currently returns the internal Map "jobs"
allowing external callers to mutate manager state; change getJobs() to return a
defensive copy (e.g., return new Map(this.jobs)) or a ReadonlyMap to prevent
external mutation, ensuring callers cannot call set/delete on the manager's
internal "jobs" Map while preserving existing external 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: 7b132fd0-da7f-4c2c-910f-594385d533ae
📒 Files selected for processing (2)
spec/ExternalProcessAdapter.spec.jssrc/cloud-code/CloudCodeManager.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- spec/ExternalProcessAdapter.spec.js
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Introduces a pluggable Cloud Code Adapter system that decouples cloud code from the Parse JS SDK, enabling:
InProcessCloudCoderouter interfacecloud: './main.js'configurations work identicallyNew config options
cloud(extended)getRouter()(in-process adapter)cloudCodeCommandwebhookKeycloudCodeCommandcloudCodeOptionscloudCodeAdaptersCloudCodeAdapterinstancesArchitecture
CloudCodeManagerreplacestriggers.jsas the hook registry (triggers.js becomes a facade that delegates to the manager)LegacyAdapter,InProcessAdapter,ExternalProcessAdapterCloudCodeManagerperapplicationId, stored onAppCacheNew files
src/cloud-code/types.ts— TypeScript interfacessrc/cloud-code/CloudCodeManager.ts— Core managersrc/cloud-code/resolveAdapters.ts— Config → adapter resolutionsrc/cloud-code/adapters/LegacyAdapter.ts— Backwards-compatible loadersrc/cloud-code/adapters/InProcessAdapter.ts— Manifest-based in-process adaptersrc/cloud-code/adapters/ExternalProcessAdapter.ts— Child process lifecyclesrc/cloud-code/adapters/webhook-bridge.ts— Request/response conversionsrc/cloud-code/README.md— Documentation for building custom adaptersTest plan
npm run buildcompiles cleanlynpm run definitionspassesSummary by CodeRabbit