chore(apps): refactor jsonrpc request handlers parameter passing #38322
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
WalkthroughRefactors Deno runtime handlers to accept a consolidated Changes
Sequence Diagram(s)(omitted — changes are a cross-cutting refactor to handler input shapes and tests; no single new multi-component sequential flow to visualize) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 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)
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #38322 +/- ##
===========================================
+ Coverage 70.77% 70.83% +0.06%
===========================================
Files 3159 3159
Lines 109384 109364 -20
Branches 19644 19693 +49
===========================================
+ Hits 77412 77469 +57
+ Misses 29939 29868 -71
+ Partials 2033 2027 -6
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
📦 Docker Image Size Report➡️ Changes
📊 Historical Trend---
config:
theme: "dark"
xyChart:
width: 900
height: 400
---
xychart
title "Image Size Evolution by Service (Last 30 Days + This PR)"
x-axis ["11/18 22:53", "11/19 23:02", "11/21 16:49", "11/24 17:34", "11/27 22:32", "11/28 19:05", "12/01 23:01", "12/02 21:57", "12/03 21:00", "12/04 18:17", "12/05 21:56", "12/08 20:15", "12/09 22:17", "12/10 23:26", "12/11 21:56", "12/12 22:45", "12/13 01:34", "12/15 22:31", "12/16 22:18", "12/17 21:04", "12/18 23:12", "12/19 23:27", "12/20 21:03", "12/22 18:54", "12/23 16:16", "12/24 19:38", "12/25 17:51", "12/26 13:18", "12/29 19:01", "12/30 20:52", "01/26 21:33 (PR)"]
y-axis "Size (GB)" 0 --> 0.5
line "account-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.00]
line "authorization-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.00]
line "ddp-streamer-service" [0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.00]
line "omnichannel-transcript-service" [0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.00]
line "presence-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.00]
line "queue-worker-service" [0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.00]
line "rocketchat" [0.35, 0.35, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.00]
Statistics (last 30 days):
ℹ️ About this reportThis report compares Docker image sizes from this build against the
Updated: Mon, 26 Jan 2026 21:33:48 GMT |
d-gubert
force-pushed
the
feat/apps-logs-redirect
branch
3 times, most recently
from
05a53ac to
8d51b92
Compare
d-gubert
changed the title
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
packages/apps-engine/deno-runtime/handlers/app/handleOnPreSettingUpdate.ts (1)
17-23: Validate params length and first element shape before destructuring.
Array.isArray(params)allows[], which yieldssetting === undefinedand can surface as a harder-to-debug runtime error inside the app handler. Consider guarding for length and a non-null object before calling the hook.🔧 Suggested fix
- if (!Array.isArray(params)) { + if (!Array.isArray(params) || params.length < 1 || typeof params[0] !== 'object' || params[0] === null) { throw new Error('Invalid params', { cause: 'invalid_param_type' }); } - const [setting] = params as [Record<string, unknown>]; + const [setting] = params as [Record<string, unknown>];packages/apps-engine/deno-runtime/handlers/app/handleOnSettingUpdated.ts (1)
17-21: Missing validation for empty params array.If
paramsis an empty array,settingwill beundefinedafter destructuring, andapp.onSettingUpdatedwill be called with an undefined setting. Consider validating that the setting exists.Proposed fix
if (!Array.isArray(params)) { throw new Error('Invalid params', { cause: 'invalid_param_type' }); } const [setting] = params as [Record<string, unknown>]; +if (!setting) { + throw new Error('Setting is required', { cause: 'invalid_param_type' }); +} + await app.onSettingUpdated(setting, AppAccessorsInstance.getConfigurationModify(), AppAccessorsInstance.getReader(), AppAccessorsInstance.getHttp());packages/apps-engine/deno-runtime/handlers/outboundcomms-handler.ts (1)
8-18: Add validation forparamsshape before spreading.
In JSON-RPC 2.0,paramscan be either an Array (positional) or an Object (named). The current code castsparamstoArray<unknown>without validation, which silently fails whenparamsis an object—the spread operator produces an empty array, causing the provider method to receive incorrect arguments. Add a guard to validateparamsis array-shaped and return a controlled error for invalid shapes, consistent with patterns already used inscheduler-handler.tsandconstruct.ts.Proposed fix
export default async function outboundMessageHandler(request: RequestObject): Promise<JsonRpcError | Defined> { const { method: call, params } = request; const [, providerName, methodName] = call.split(':'); + if (params !== undefined && !Array.isArray(params)) { + return new JsonRpcError('error-invalid-params', -32000); + } const provider = AppObjectRegistry.get<IOutboundMessageProviders>(`outboundCommunication:${providerName}`); if (!provider) { return new JsonRpcError('error-invalid-provider', -32000); } const method = provider[methodName as keyof IOutboundMessageProviders]; const logger = AppObjectRegistry.get<Logger>('logger'); const args = (params as Array<unknown>) ?? [];packages/apps-engine/deno-runtime/handlers/videoconference-handler.ts (1)
27-27: Consider handling undefinedparams.In
jsonrpc-lite,RequestObject.paramsis optional. If a request arrives without params, this line will throw when attempting to destructureundefined. Consider defaulting to an empty array.Suggested fix
- const [videoconf, user, options] = params as Array<unknown>; + const [videoconf, user, options] = (params ?? []) as Array<unknown>;packages/apps-engine/deno-runtime/handlers/api-handler.ts (1)
2-32: Validate params shape before destructuring.If
paramsis missing or not an array,requestData/endpointInfobecomeundefinedand the endpoint call can fail with an internal error instead of a JSON‑RPC invalid‑params response. Other handlers in this codebase validate this consistently.🔧 Proposed fix
- const [requestData, endpointInfo] = params as Array<unknown>; + if (!Array.isArray(params) || params.length < 2) { + return JsonRpcError.invalidParams(null); + } + const [requestData, endpointInfo] = params as [unknown, unknown];
🤖 Fix all issues with AI agents
In `@packages/apps-engine/deno-runtime/handlers/app/handleUploadEvents.ts`:
- Around line 28-33: The destructuring of params happens outside the try and can
throw if params is missing or malformed; move parsing of params into the try
block inside handleUploadEvents, validate that request.params is an array and
has at least one element (and that the first element is an object) before doing
const [{ file, path }] = params, and if validation fails return a JsonRpcError
or appropriate error object; keep the rawMethod -> method parsing (rawMethod and
method derived from uploadEvents) but perform the params checks first to ensure
safe destructuring and proper JSON-RPC error flow.
packages/apps-engine/deno-runtime/handlers/app/handleUploadEvents.ts
Outdated
Show resolved
Hide resolved
d-gubert
force-pushed
the
feat/apps-logs-redirect
branch
from
1ca4ed8 to
7307eb6
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/apps-engine/deno-runtime/handlers/api-handler.ts (1)
8-27: Add params validation before destructuring to prevent uncaught exceptions.Line 25 destructures
paramswithout guarding againstundefined,null, or non-array values. Ifparamsis invalid, destructuring throws and bypasses error handling. Add an early Array.isArray check, consistent withscheduler-handler.tsandlistener/handler.ts:Suggested fix
export default async function apiHandler(request: RequestObject): Promise<JsonRpcError | Defined> { const { method: call, params } = request; const [, path, httpMethod] = call.split(':'); const endpoint = AppObjectRegistry.get<IApiEndpoint>(`api:${path}`); const logger = AppObjectRegistry.get<Logger>('logger'); if (!endpoint) { return new JsonRpcError(`Endpoint ${path} not found`, -32000); } const method = endpoint[httpMethod as keyof IApiEndpoint]; if (typeof method !== 'function') { return new JsonRpcError(`${path}'s ${httpMethod} not exists`, -32000); } + if (!Array.isArray(params)) { + return JsonRpcError.invalidParams(null); + } + const [requestData, endpointInfo] = params as Array<unknown>;
Proposed changes (including videos or screenshots)
This refactor allows each handler to have access to the request object reference and consequently its ID. This is an enabler to fix an issue with logs in "nested" requests.
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
Refactor
Style / Errors
Tests
✏️ Tip: You can customize this high-level summary in your review settings.