chore: api http router improvements #38227
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 |
|
WalkthroughCentralizes request parsing into Router, makes payload extraction asynchronous and request-centric, integrates password redaction in logging, removes runtime Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Router
participant LoggerMiddleware as Logger
participant PayloadSvc as getRestPayload
participant Handler
Client->>Router: HTTP request
Router->>Router: async parse query + body → c.set('queryParams','bodyParams')
Router->>Logger: invoke logger middleware
Logger->>PayloadSvc: await getRestPayload(request)
PayloadSvc-->>Logger: payload | "{ payload: '[multipart/form-data]' }" | null
Logger->>Handler: call handler (context includes body/query and logs with redacted payload)
Handler-->>Client: response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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 |
d-gubert
force-pushed
the
chore/router-improvements
branch
from
12a4f8f to
b1b33d3
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #38227 +/- ##
===========================================
+ Coverage 70.73% 70.84% +0.10%
===========================================
Files 3158 3159 +1
Lines 109359 109383 +24
Branches 19695 19703 +8
===========================================
+ Hits 77358 77489 +131
+ Misses 29966 29869 -97
+ Partials 2035 2025 -10
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/23 21:51 (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: Fri, 23 Jan 2026 21:51:49 GMT |
packages/http-router/src/Router.ts
Outdated
| if (options.query) { | ||
| const validatorFn = options.query; | ||
| if (typeof options.query === 'function' && !validatorFn(queryParams)) { | ||
| if (typeof options.query === 'function' && !validatorFn(structuredClone(queryParams))) { |
There was a problem hiding this comment.
validatorFn mutates its input, which causes some weird behaviors that break tests. Cloning the input "solves" the issues, but defeats the purpose of removing the second calls to parseBodyParams and parseQueryParams.
I think it is valuable to actually take a look at the breaking tests and make the system adapt to the mutatons that happen in those validatorFn calls
d-gubert
force-pushed
the
chore/router-improvements
branch
from
dd78dad to
536fd45
Compare
|
@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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/rest-typings/src/v1/omnichannel.ts (1)
4502-4505: Type guard does not match schema nullability forcustomFieldId.The schema defines
customFieldIdwithnullable: trueand norequiredarray (making it optional), but the type guard narrows tocustomFieldId: string. After validation,customFieldIdcould benullor omitted (undefined), violating the type guard. Align the generic type tocustomFieldId?: string | nullto match the schema.Suggested fix
export const isPOSTLivechatSaveCustomFieldsParams = ajv.compile<{ - customFieldId: string; + customFieldId?: string | null; customFieldData: Omit<ILivechatCustomField, '_id' | '_updatedAt'> & { field: string }; }>(POSTLivechatSaveCustomFieldsSchema);
🤖 Fix all issues with AI agents
In `@packages/rest-typings/src/v1/omnichannel.ts`:
- Around line 4446-4457: Update the regex character classes to move the hyphen
to the end so they read `[0-9a-zA-Z_-]` for both customFieldId and
customFieldData.field to avoid creating an unintended range; also reconcile the
nullable mismatch by either removing `nullable: true` from the `customFieldId`
schema or updating the type guard that references `customFieldId` (the check
near the type guard around line ~4502) to accept `string | null` so schema and
runtime types match.
🧹 Nitpick comments (1)
packages/http-router/src/Router.ts (1)
157-175: AlignbodyParamstyping with possible JSON shapes.
request.raw.clone().json()can return arrays or primitives, but the context types areRecord<string, unknown>. Consider normalizing non‑object bodies to{}or widening the type so downstream code doesn’t rely on an incorrect shape.
d-gubert
force-pushed
the
chore/router-improvements
branch
from
536fd45 to
2186968
Compare
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
|
While reviewing the Created CORE-1741 to track this separately. |
|
@ricardogarim are they really broken? I've found this test to validate it (should) be working: Rocket.Chat/apps/meteor/tests/end-to-end/api/incoming-integrations.ts Lines 343 to 366 in 0580cfc |
|
@d-gubert maybe the Livechat changes could be in another PR? they look like regular changes not necessarily related to "api http router improments" |
We need to modify that endpoint, otherwise tests fail because of this #38227 (comment) I can make a much more contrived change just to make sure the tests are passing, then open a new PR with the appropriate changes to schemas and all. But the endpoint can't go unscathed 😛 |
|
Forgot to mention @sampaiodiego #38227 (comment) 😬 |
d-gubert
force-pushed
the
chore/router-improvements
branch
from
2186968 to
5ffad11
Compare
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 (1)
packages/models/src/models/LivechatCustomField.ts (1)
52-75: Return value missing_idin update path.When updating an existing record (
_idis truthy), the returnedrecordobject lacks the_idproperty, which violates theILivechatCustomFieldreturn type. The insert path correctly assignsrecord._id = field, but the update path does not preserve the ID on the returned object.🐛 Proposed fix
async createOrUpdateCustomField( _id: string | null, field: string, label: ILivechatCustomField['label'], scope: ILivechatCustomField['scope'], visibility: ILivechatCustomField['visibility'], extraData: any, ) { const record = { label, scope, visibility, ...extraData, }; if (_id) { await this.updateOne({ _id }, { $set: record }); + record._id = _id; } else { record._id = field; await this.insertOne(record); } return record; }
🤖 Fix all issues with AI agents
In `@apps/meteor/app/api/server/router.ts`:
- Around line 44-49: The APIActionContext construction should defensively
default potentially-undefined context entries; update the code that builds
APIActionContext (the object with requestIp, urlParams, queryParams, bodyParams,
request) to use c.get('bodyParams') ?? {} and c.get('queryParams') ?? {} so
bodyParams and queryParams are always objects even if c.get(...) returns
undefined, keeping the existing fields (e.g., requestIp: c.get('remoteAddress')
and urlParams: req.param()) unchanged.
| const context: APIActionContext = { | ||
| requestIp: c.get('remoteAddress'), | ||
| urlParams: req.param(), | ||
| queryParams, | ||
| bodyParams, | ||
| queryParams: c.get('queryParams'), | ||
| bodyParams: c.get('bodyParams'), | ||
| request, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's look at the router.ts file around lines 44-49
cat -n apps/meteor/app/api/server/router.ts | sed -n '35,60p'Repository: RocketChat/Rocket.Chat
Length of output: 982
🏁 Script executed:
#!/bin/bash
# Search for middleware/handlers that set bodyParams and queryParams
rg -n 'bodyParams|queryParams' apps/meteor/app/api/server/ -B2 -A2 -t tsRepository: RocketChat/Rocket.Chat
Length of output: 50379
🏁 Script executed:
#!/bin/bash
# Search for middleware or handlers related to parameter parsing
rg -n 'application/x-www-form-urlencoded|payload' apps/meteor/app/api/server/ -B3 -A3 -t tsRepository: RocketChat/Rocket.Chat
Length of output: 1039
🏁 Script executed:
#!/bin/bash
# Search for where middleware sets bodyParams and queryParams
rg -n 'c\.set.*bodyParams|c\.set.*queryParams|set\(.*bodyParams|set\(.*queryParams' apps/meteor/app/api/server/ -B3 -A3 -t tsRepository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
#!/bin/bash
# Look for middleware setup in router or main API files
rg -n 'middleware|use\(|hook|Hono' apps/meteor/app/api/server/router.ts -B2 -A5 -t tsRepository: RocketChat/Rocket.Chat
Length of output: 754
🏁 Script executed:
#!/bin/bash
# Search for where request bodies are parsed or params are extracted
rg -n 'parseBody|parseForm|urlencoded|form-data|bodyParser|parseRequest' apps/meteor/app/api/server/ -B2 -A3 -t ts | head -100Repository: RocketChat/Rocket.Chat
Length of output: 2725
🏁 Script executed:
#!/bin/bash
# Find Router class import and definition
rg -n 'class Router|import.*Router' apps/meteor/app/api/server/router.ts -B1 -A1 -t tsRepository: RocketChat/Rocket.Chat
Length of output: 223
🏁 Script executed:
#!/bin/bash
# Look for the definition.ts file and Router class
find apps/meteor/app/api/server -name "*.ts" -type f | xargs rg -l 'class Router|interface APIActionContext' | head -5Repository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
#!/bin/bash
# Search for where bodyParams is populated - likely in middleware or parent setup
rg -n 'bodyParams.*=|queryParams.*=' apps/meteor/app/api/server -t ts --max-count=20Repository: RocketChat/Rocket.Chat
Length of output: 4158
🏁 Script executed:
#!/bin/bash
# Search for middleware, hooks, or setup files in api/server
fd -a '\.ts$' apps/meteor/app/api/server | grep -E '(middleware|setup|init|hook)' | head -20Repository: RocketChat/Rocket.Chat
Length of output: 792
🏁 Script executed:
#!/bin/bash
# Check definition.ts for Router setup
cat -n apps/meteor/app/api/server/definition.ts | head -100Repository: RocketChat/Rocket.Chat
Length of output: 3585
🏁 Script executed:
#!/bin/bash
# Look for API setup and middleware registration
rg -n 'app\.use|middleware|bodyParams|queryParams' apps/meteor/app/api/server/ApiClass.ts -B2 -A3 -t ts | head -150Repository: RocketChat/Rocket.Chat
Length of output: 1964
🏁 Script executed:
#!/bin/bash
# Check how the HTTP router is initialized
rg -n 'new Router|createRouter|setupRouter' apps/meteor/app/api/server/ -B2 -A5 -t tsRepository: RocketChat/Rocket.Chat
Length of output: 5860
🏁 Script executed:
#!/bin/bash
# Search for where RocketChatAPIRouter is instantiated
rg -n 'new RocketChatAPIRouter|RocketChatAPIRouter' apps/meteor/app/api/server/ -B2 -A5 -t ts | head -80Repository: RocketChat/Rocket.Chat
Length of output: 4629
🏁 Script executed:
#!/bin/bash
# Look at the full ApiClass initialization
rg -n 'constructor|initRoutes|initialize' apps/meteor/app/api/server/ApiClass.ts -B1 -A8 -t ts | head -150Repository: RocketChat/Rocket.Chat
Length of output: 1018
🏁 Script executed:
#!/bin/bash
# Search for where body/query parsing happens - check imports and setup
head -100 apps/meteor/app/api/server/ApiClass.tsRepository: RocketChat/Rocket.Chat
Length of output: 4155
🏁 Script executed:
#!/bin/bash
# Look at api.ts to see middleware registration
cat -n apps/meteor/app/api/server/api.ts | head -150Repository: RocketChat/Rocket.Chat
Length of output: 4643
🏁 Script executed:
#!/bin/bash
# Search for where body/query params are extracted or middleware that sets them
rg -n 'use\(|middleware|setBodyParams|setQueryParams|c\.set\(' apps/meteor/app/api/server/ -t ts | grep -v spec | grep -v test | head -40Repository: RocketChat/Rocket.Chat
Length of output: 1748
🏁 Script executed:
#!/bin/bash
# Look for the http-router package to understand how it works
rg -n 'bodyParams|queryParams|c\.get' apps/meteor/app/api/server/middlewares/ -t ts -B2 -A2Repository: RocketChat/Rocket.Chat
Length of output: 540
🏁 Script executed:
#!/bin/bash
# Look at remoteAddressMiddleware to understand the pattern
cat -n apps/meteor/app/api/server/middlewares/remoteAddressMiddleware.tsRepository: RocketChat/Rocket.Chat
Length of output: 1553
🏁 Script executed:
#!/bin/bash
# Search for where bodyParams and queryParams are actually set
rg -n "set\('bodyParams'|set\('queryParams'|bodyParams.*=|queryParams.*=" apps/meteor/app/api/server/ -B3 -A3 -t tsRepository: RocketChat/Rocket.Chat
Length of output: 18487
🏁 Script executed:
#!/bin/bash
# Check if http-router package might be handling this - search for getBodyParams or similar
rg -n "getBodyParams|getQueryParams|body.*parse|query.*parse" apps/meteor/app/api/server/ -t tsRepository: RocketChat/Rocket.Chat
Length of output: 4187
🏁 Script executed:
#!/bin/bash
# Look at the router spec to see if it shows how params are set
cat -n apps/meteor/app/api/server/router.spec.ts | head -120Repository: RocketChat/Rocket.Chat
Length of output: 1935
🏁 Script executed:
#!/bin/bash
# Search for any use method that might set params
rg -n '\.use\(' apps/meteor/app/api/server/router.ts apps/meteor/app/api/server/ApiClass.ts -B3 -A8 -t tsRepository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
#!/bin/bash
# Look for where body or query might be extracted - check the Router parent class behavior
rg -n 'req\.(body|query)' apps/meteor/app/api/server/ -t ts | head -20Repository: RocketChat/Rocket.Chat
Length of output: 185
🏁 Script executed:
#!/bin/bash
# Check http-router imports and documentation or check if there's a way params get set
find . -name "*.json" -path "*/node_modules/@rocket.chat/http-router/*" 2>/dev/null | head -5Repository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
#!/bin/bash
# Look for explicit middleware that parses request bodies
rg -n 'bodyParams.*set|set.*bodyParams|queryParams.*set|set.*queryParams' apps/meteor/app/ -t ts -B3 -A3Repository: RocketChat/Rocket.Chat
Length of output: 50378
🏁 Script executed:
#!/bin/bash
# Look at authentication middleware
cat -n apps/meteor/app/api/server/middlewares/authentication.tsRepository: RocketChat/Rocket.Chat
Length of output: 2545
🏁 Script executed:
#!/bin/bash
# Search for where `@rocket.chat/http-router` might be used to understand param parsing
rg -n "req.query|req.body|c.req" apps/meteor/app/api/server/ -t ts | head -30Repository: RocketChat/Rocket.Chat
Length of output: 2068
Add defensive defaults for potentially undefined bodyParams and queryParams.
The HonoContext type system declares bodyParams and queryParams as non-optional Record<string, unknown>, but c.get() can return undefined if these values are not set upstream. While the codebase assumes they're always objects (evidenced by widespread destructuring without null checks), adding ?? {} guards against potential runtime errors if the upstream middleware in @rocket.chat/http-router fails to set these values.
✅ Safer default for missing params
- queryParams: c.get('queryParams'),
- bodyParams: c.get('bodyParams'),
+ queryParams: c.get('queryParams') ?? {},
+ bodyParams: c.get('bodyParams') ?? {},Regarding application/x-www-form-urlencoded requests with payload={...} (Slack/GitHub style): This unwrapping logic is not visible in the codebase and is likely handled by the @rocket.chat/http-router package. There is no evidence that this handling is missing or broken by the refactoring.
📝 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.
| const context: APIActionContext = { | |
| requestIp: c.get('remoteAddress'), | |
| urlParams: req.param(), | |
| queryParams, | |
| bodyParams, | |
| queryParams: c.get('queryParams'), | |
| bodyParams: c.get('bodyParams'), | |
| request, | |
| const context: APIActionContext = { | |
| requestIp: c.get('remoteAddress'), | |
| urlParams: req.param(), | |
| queryParams: c.get('queryParams') ?? {}, | |
| bodyParams: c.get('bodyParams') ?? {}, | |
| request, |
🤖 Prompt for AI Agents
In `@apps/meteor/app/api/server/router.ts` around lines 44 - 49, The
APIActionContext construction should defensively default potentially-undefined
context entries; update the code that builds APIActionContext (the object with
requestIp, urlParams, queryParams, bodyParams, request) to use
c.get('bodyParams') ?? {} and c.get('queryParams') ?? {} so bodyParams and
queryParams are always objects even if c.get(...) returns undefined, keeping the
existing fields (e.g., requestIp: c.get('remoteAddress') and urlParams:
req.param()) unchanged.
ricardogarim
force-pushed
the
chore/router-improvements
branch
2 times, most recently
from
3ad9b41 to
5ffad11
Compare
Proposed changes (including videos or screenshots)
parseBodyParamsandparseQueryParamsinRocketChatAPIRouterpasswordfield when logging requestsIssue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
Security & Privacy
Bug Fixes & Improvements
✏️ Tip: You can customize this high-level summary in your review settings.