chore!: change http code results for ddp over rest #38007
Conversation
…dling for rate limiting
|
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 |
|
WalkthroughIntroduces strongly typed error responses for HTTP 429 (Too Many Requests) and improves error handling in API methods by distinguishing Meteor.Error types and mapping them to appropriate HTTP status codes (429, 401, 403, 500). Updates corresponding test expectations to reflect failure cases. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 @@
## release-8.0.0 #38007 +/- ##
=================================================
- Coverage 70.64% 70.62% -0.03%
=================================================
Files 3145 3145
Lines 108718 108727 +9
Branches 19554 19534 -20
=================================================
- Hits 76808 76784 -24
- Misses 29901 29943 +42
+ Partials 2009 2000 -9
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/17 23:50", "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 05:11 (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.11]
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.11]
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.12]
line "omnichannel-transcript-service" [0.14, 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]
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.11]
line "queue-worker-service" [0.14, 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]
line "rocketchat" [0.35, 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.35]
Statistics (last 30 days):
ℹ️ About this reportThis report compares Docker image sizes from this build against the
Updated: Tue, 30 Dec 2025 05:11:38 GMT |
…perty set to false
…roperty set to false
ggazzo
changed the title
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 11 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="apps/meteor/app/api/server/v1/misc.ts">
<violation number="1" location="apps/meteor/app/api/server/v1/misc.ts:531">
P1: Missing case for `'too-many-requests'` error. The DDPRateLimiter throws `'too-many-requests'` (without 'error-' prefix) but this switch only handles `'error-too-many-requests'`. Rate limit errors from this endpoint won't return the correct 429 status code.</violation>
</file>
<file name="apps/meteor/tests/end-to-end/api/methods.ts">
<violation number="1" location="apps/meteor/tests/end-to-end/api/methods.ts:169">
P2: Inconsistent test assertion: HTTP 400 status code should correspond to `success: false`, not `success: true`. All other similar changes in this PR update both values together.</violation>
</file>
Reply to cubic to teach it or ask questions. Tag @cubic-dev-ai to re-run a review.
|
|
||
| if (err instanceof Meteor.Error) { | ||
| switch (err.error) { | ||
| case 'error-too-many-requests': |
There was a problem hiding this comment.
P1: Missing case for 'too-many-requests' error. The DDPRateLimiter throws 'too-many-requests' (without 'error-' prefix) but this switch only handles 'error-too-many-requests'. Rate limit errors from this endpoint won't return the correct 429 status code.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/app/api/server/v1/misc.ts, line 531:
<comment>Missing case for `'too-many-requests'` error. The DDPRateLimiter throws `'too-many-requests'` (without 'error-' prefix) but this switch only handles `'error-too-many-requests'`. Rate limit errors from this endpoint won't return the correct 429 status code.</comment>
<file context>
@@ -525,7 +525,22 @@ API.v1.addRoute(
+
+ if (err instanceof Meteor.Error) {
+ switch (err.error) {
+ case 'error-too-many-requests':
+ return API.v1.tooManyRequests(mountResult({ id, error: err }));
+ case 'unauthorized':
</file context>
| case 'error-too-many-requests': | |
| case 'too-many-requests': | |
| case 'error-too-many-requests': |
| }) | ||
| .expect('Content-Type', 'application/json') | ||
| .expect(200) | ||
| .expect(400) |
There was a problem hiding this comment.
P2: Inconsistent test assertion: HTTP 400 status code should correspond to success: false, not success: true. All other similar changes in this PR update both values together.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/tests/end-to-end/api/methods.ts, line 169:
<comment>Inconsistent test assertion: HTTP 400 status code should correspond to `success: false`, not `success: true`. All other similar changes in this PR update both values together.</comment>
<file context>
@@ -166,9 +166,10 @@ describe('Meteor.methods', () => {
})
.expect('Content-Type', 'application/json')
- .expect(200)
+ .expect(400)
.expect((res) => {
- expect(res.body).to.have.property('success', true);
</file context>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
apps/meteor/app/api/server/v1/misc.ts (1)
509-544: Rate‑limit error mapping misses the locally‑thrown'too-many-requests'codeThe new error mapping on
method.call/:methodis a good improvement, but there’s a subtle mismatch in the rate‑limit branch:
- When the limiter trips, this code throws
new Meteor.Error('too-many-requests', ...)(Line 513).- The
switch (err.error)only matches'error-too-many-requests'.- As a result, rate‑limited calls from this route will fall through to the default and be sent via
API.v1.failure(...)(HTTP 400) instead ofAPI.v1.tooManyRequests(...)(HTTP 429), which contradicts the intent of addingTooManyRequestsResult.To ensure rate‑limit responses are consistently surfaced as 429, handle both forms of the error code:
Proposed fix for rate‑limit error mapping
- if (err instanceof Meteor.Error) { - switch (err.error) { - case 'error-too-many-requests': - return API.v1.tooManyRequests(mountResult({ id, error: err })); + if (err instanceof Meteor.Error) { + switch (err.error) { + case 'too-many-requests': + case 'error-too-many-requests': + return API.v1.tooManyRequests(mountResult({ id, error: err })); case 'unauthorized': case 'error-unauthorized': return API.v1.unauthorized(mountResult({ id, error: err })); case 'forbidden': case 'error-forbidden': return API.v1.forbidden(mountResult({ id, error: err })); default: return API.v1.failure(mountResult({ id, error: err })); } }This keeps the rest of the mapping unchanged while making the 429 behavior reachable for the locally generated rate‑limit error.
🧹 Nitpick comments (1)
apps/meteor/tests/end-to-end/api/methods.ts (1)
2278-2311: Address skipped tests and incomplete test coverage.Two tests related to quote attachments are skipped with a TODO comment indicating "this test is not testing anything useful." Skipped tests reduce coverage and may hide regressions.
Consider either:
- Fixing the test assertions to properly validate the quote attachment behavior
- Removing the tests entirely if the functionality is deprecated
- Opening an issue to track the work if it can't be addressed immediately
The related test at line 2313 ("should replace a quote attachment") is active and passing, suggesting quote attachments are still functional.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
apps/meteor/app/api/server/ApiClass.tsapps/meteor/app/api/server/definition.tsapps/meteor/app/api/server/v1/misc.tsapps/meteor/client/meteor/overrides/ddpOverREST.tsapps/meteor/tests/end-to-end/api/abac.tsapps/meteor/tests/end-to-end/api/guest-permissions.tsapps/meteor/tests/end-to-end/api/methods.tsapps/meteor/tests/end-to-end/api/methods/2fa-enable.tsee/packages/federation-matrix/tests/end-to-end/dms.spec.tsee/packages/federation-matrix/tests/end-to-end/permissions.spec.tsee/packages/federation-matrix/tests/end-to-end/room.spec.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/client/meteor/overrides/ddpOverREST.tsee/packages/federation-matrix/tests/end-to-end/permissions.spec.tsapps/meteor/tests/end-to-end/api/guest-permissions.tsapps/meteor/tests/end-to-end/api/abac.tsapps/meteor/tests/end-to-end/api/methods/2fa-enable.tsee/packages/federation-matrix/tests/end-to-end/room.spec.tsee/packages/federation-matrix/tests/end-to-end/dms.spec.tsapps/meteor/app/api/server/definition.tsapps/meteor/app/api/server/v1/misc.tsapps/meteor/tests/end-to-end/api/methods.tsapps/meteor/app/api/server/ApiClass.ts
**/*.spec.ts
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.spec.ts: Use descriptive test names that clearly communicate expected behavior in Playwright tests
Use.spec.tsextension for test files (e.g.,login.spec.ts)
Files:
ee/packages/federation-matrix/tests/end-to-end/permissions.spec.tsee/packages/federation-matrix/tests/end-to-end/room.spec.tsee/packages/federation-matrix/tests/end-to-end/dms.spec.ts
🧠 Learnings (22)
📚 Learning: 2025-11-04T16:49:19.107Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37377
File: apps/meteor/ee/server/hooks/federation/index.ts:86-88
Timestamp: 2025-11-04T16:49:19.107Z
Learning: In Rocket.Chat's federation system (apps/meteor/ee/server/hooks/federation/), permission checks follow two distinct patterns: (1) User-initiated federation actions (creating rooms, adding users to federated rooms, joining from invites) should throw MeteorError to inform users they lack 'access-federation' permission. (2) Remote server-initiated federation events should silently skip/ignore when users lack permission. The beforeAddUserToRoom hook only executes for local user-initiated actions, so throwing an error there is correct. Remote federation events are handled separately by the federation Matrix package with silent skipping logic.
Applied to files:
ee/packages/federation-matrix/tests/end-to-end/permissions.spec.tsee/packages/federation-matrix/tests/end-to-end/room.spec.ts
📚 Learning: 2025-10-28T16:53:42.761Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37205
File: ee/packages/federation-matrix/src/FederationMatrix.ts:296-301
Timestamp: 2025-10-28T16:53:42.761Z
Learning: In the Rocket.Chat federation-matrix integration (ee/packages/federation-matrix/), the createRoom method from rocket.chat/federation-sdk will support a 4-argument signature (userId, roomName, visibility, displayName) in newer versions. Code using this 4-argument call is forward-compatible with planned library updates and should not be flagged as an error.
Applied to files:
ee/packages/federation-matrix/tests/end-to-end/permissions.spec.tsee/packages/federation-matrix/tests/end-to-end/room.spec.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: UserBridge.doGetUserRoomIds in packages/apps-engine/src/server/bridges/UserBridge.ts has a bug where it implicitly returns undefined when the app lacks read permission (missing return statement in the else case of the permission check).
Applied to files:
ee/packages/federation-matrix/tests/end-to-end/permissions.spec.ts
📚 Learning: 2025-09-19T15:15:04.642Z
Learnt from: rodrigok
Repo: RocketChat/Rocket.Chat PR: 36991
File: apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts:219-221
Timestamp: 2025-09-19T15:15:04.642Z
Learning: The Federation_Matrix_homeserver_domain setting in apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts is part of the old federation system and is being deprecated/removed, so configuration issues with this setting should not be flagged for improvement.
Applied to files:
ee/packages/federation-matrix/tests/end-to-end/permissions.spec.ts
📚 Learning: 2025-12-09T20:01:00.324Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37532
File: ee/packages/federation-matrix/src/FederationMatrix.ts:920-927
Timestamp: 2025-12-09T20:01:00.324Z
Learning: When reviewing federation invite handling in Rocket.Chat (specifically under ee/packages/federation-matrix), understand that rejecting an invite via federationSDK.rejectInvite() triggers an event-driven cleanup: a leave event is emitted and handled by handleLeave() in ee/packages/federation-matrix/src/events/member.ts, which calls Room.performUserRemoval() to remove the subscription. Do not add explicit cleanup in the reject branch of handleInvite(); rely on the existing leave-event flow for cleanup. If making changes, ensure this invariant remains and that any related paths still funnel cleanup through the leave event to avoid duplicate or missing removals.
Applied to files:
ee/packages/federation-matrix/tests/end-to-end/permissions.spec.tsee/packages/federation-matrix/tests/end-to-end/room.spec.tsee/packages/federation-matrix/tests/end-to-end/dms.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `expect` matchers for assertions (`toEqual`, `toContain`, `toBeTruthy`, `toHaveLength`, etc.) instead of `assert` statements in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/api/guest-permissions.tsapps/meteor/tests/end-to-end/api/abac.tsapps/meteor/tests/end-to-end/api/methods/2fa-enable.tsee/packages/federation-matrix/tests/end-to-end/room.spec.tsee/packages/federation-matrix/tests/end-to-end/dms.spec.tsapps/meteor/tests/end-to-end/api/methods.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
apps/meteor/tests/end-to-end/api/guest-permissions.tsapps/meteor/tests/end-to-end/api/abac.tsapps/meteor/tests/end-to-end/api/methods/2fa-enable.tsee/packages/federation-matrix/tests/end-to-end/room.spec.tsee/packages/federation-matrix/tests/end-to-end/dms.spec.tsapps/meteor/tests/end-to-end/api/methods.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Utilize Playwright fixtures (`test`, `page`, `expect`) for consistency in test files
Applied to files:
apps/meteor/tests/end-to-end/api/guest-permissions.tsapps/meteor/tests/end-to-end/api/methods/2fa-enable.tsee/packages/federation-matrix/tests/end-to-end/room.spec.tsapps/meteor/tests/end-to-end/api/methods.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : All test files must be created in `apps/meteor/tests/e2e/` directory
Applied to files:
apps/meteor/tests/end-to-end/api/guest-permissions.tsapps/meteor/tests/end-to-end/api/methods/2fa-enable.tsapps/meteor/tests/end-to-end/api/methods.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Prefer web-first assertions (`toBeVisible`, `toHaveText`, etc.) in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/api/guest-permissions.tsapps/meteor/tests/end-to-end/api/methods/2fa-enable.tsapps/meteor/tests/end-to-end/api/methods.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,spec.ts} : Follow Page Object Model pattern consistently in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/api/guest-permissions.tsapps/meteor/tests/end-to-end/api/methods.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Group related tests in the same file
Applied to files:
apps/meteor/tests/end-to-end/api/guest-permissions.tsapps/meteor/tests/end-to-end/api/methods/2fa-enable.tsee/packages/federation-matrix/tests/end-to-end/room.spec.tsapps/meteor/tests/end-to-end/api/methods.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Maintain test isolation between test cases in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/api/guest-permissions.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure clean state for each test execution in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/api/guest-permissions.ts
📚 Learning: 2025-12-16T17:29:45.163Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37834
File: apps/meteor/tests/e2e/page-objects/fragments/admin-flextab-emoji.ts:12-22
Timestamp: 2025-12-16T17:29:45.163Z
Learning: In page object files under `apps/meteor/tests/e2e/page-objects/`, always import `expect` from `../../utils/test` (Playwright's async expect), not from Jest. Jest's `expect` has a synchronous signature and will cause TypeScript errors when used with web-first assertions like `toBeVisible()`.
Applied to files:
apps/meteor/tests/end-to-end/api/guest-permissions.ts
📚 Learning: 2025-11-27T17:56:26.050Z
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37557
File: apps/meteor/client/views/admin/ABAC/AdminABACRooms.tsx:115-116
Timestamp: 2025-11-27T17:56:26.050Z
Learning: In Rocket.Chat, the GET /v1/abac/rooms endpoint (implemented in ee/packages/abac/src/index.ts) only returns rooms where abacAttributes exists and is not an empty array (query: { abacAttributes: { $exists: true, $ne: [] } }). Therefore, in components consuming this endpoint (like AdminABACRooms.tsx), room.abacAttributes is guaranteed to be defined for all returned rooms, and optional chaining before calling array methods like .join() is sufficient without additional null coalescing.
Applied to files:
apps/meteor/tests/end-to-end/api/abac.ts
📚 Learning: 2025-10-27T14:38:46.994Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37303
File: apps/meteor/tests/end-to-end/api/abac.ts:1125-1137
Timestamp: 2025-10-27T14:38:46.994Z
Learning: In Rocket.Chat ABAC feature, when ABAC is disabled globally (ABAC_Enabled setting is false), room-level ABAC attributes are not evaluated when changing room types. This means converting a private room to public will succeed even if the room has ABAC attributes, as long as the global ABAC setting is disabled.
Applied to files:
apps/meteor/tests/end-to-end/api/abac.ts
📚 Learning: 2025-11-07T14:50:33.544Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37423
File: packages/i18n/src/locales/en.i18n.json:18-18
Timestamp: 2025-11-07T14:50:33.544Z
Learning: Rocket.Chat settings: in apps/meteor/ee/server/settings/abac.ts, the Abac_Cache_Decision_Time_Seconds setting uses invalidValue: 0 as the fallback when ABAC is unlicensed. With a valid license, admins can still set the value to 0 to intentionally disable the ABAC decision cache.
Applied to files:
apps/meteor/tests/end-to-end/api/abac.ts
📚 Learning: 2025-10-24T17:32:05.348Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37299
File: apps/meteor/ee/server/lib/ldap/Manager.ts:438-454
Timestamp: 2025-10-24T17:32:05.348Z
Learning: In Rocket.Chat, ABAC attributes can only be set on private rooms and teams (type 'p'), not on public rooms (type 'c'). Therefore, when checking for ABAC-protected rooms/teams during LDAP sync or similar operations, it's sufficient to query only private rooms using methods like `findPrivateRoomsByIdsWithAbacAttributes`.
Applied to files:
apps/meteor/tests/end-to-end/api/abac.ts
📚 Learning: 2025-10-06T20:30:45.540Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37152
File: packages/apps-engine/tests/test-data/storage/storage.ts:101-122
Timestamp: 2025-10-06T20:30:45.540Z
Learning: In `packages/apps-engine/tests/test-data/storage/storage.ts`, the stub methods (updatePartialAndReturnDocument, updateStatus, updateSetting, updateAppInfo, updateMarketplaceInfo) intentionally throw "Method not implemented." Tests using these methods must stub them using `SpyOn` from the test library rather than relying on actual implementations.
Applied to files:
apps/meteor/tests/end-to-end/api/methods/2fa-enable.tsapps/meteor/tests/end-to-end/api/methods.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/page-objects/**/*.ts : Utilize existing page objects pattern from `apps/meteor/tests/e2e/page-objects/`
Applied to files:
apps/meteor/tests/end-to-end/api/methods/2fa-enable.tsapps/meteor/tests/end-to-end/api/methods.ts
📚 Learning: 2025-12-10T21:00:54.909Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:54.909Z
Learning: Rocket.Chat monorepo: Jest testMatch pattern '<rootDir>/src/**/*.spec.(ts|js|mjs)' is valid in this repo and used across multiple packages (e.g., packages/tools, ee/packages/omnichannel-services). Do not flag it as invalid in future reviews.
Applied to files:
ee/packages/federation-matrix/tests/end-to-end/room.spec.tsee/packages/federation-matrix/tests/end-to-end/dms.spec.tsapps/meteor/tests/end-to-end/api/methods.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: cubic · AI code reviewer
- GitHub Check: cubic · AI code reviewer
- GitHub Check: cubic · AI code reviewer
- GitHub Check: cubic · AI code reviewer
- GitHub Check: cubic · AI code reviewer
- GitHub Check: cubic · AI code reviewer
- GitHub Check: cubic · AI code reviewer
- GitHub Check: cubic · AI code reviewer
- GitHub Check: cubic · AI code reviewer
🔇 Additional comments (13)
apps/meteor/tests/end-to-end/api/abac.ts (1)
390-417: Audit ABAC room now correctly treated as an error over RESTSwitching this auditGetMessages call to expect HTTP 400 and a DDP error payload is consistent with the new method.call error mapping and ABAC behavior for managed rooms. No issues spotted.
ee/packages/federation-matrix/tests/end-to-end/permissions.spec.ts (1)
182-192: Local add-user permission test aligned with new failure semanticsExpecting status 400 and
success: falsewhen adding a user withoutaccess-federationto a federated room matches the updated authorization behavior for local, user-initiated federation actions. Looks correct.ee/packages/federation-matrix/tests/end-to-end/room.spec.ts (1)
225-255: Explicitly asserting failure when inviting federated user to non‑federated roomAdding the
success: falseassertion here makes the test accurately reflect the forbidden operation, on top of checking the DDP error payload. This is consistent with the strengthened API error handling.ee/packages/federation-matrix/tests/end-to-end/dms.spec.ts (3)
641-656: Group DM permission failure now assertssuccess: falseFor the RC user attempting to add another user to the group DM, asserting
success: false(while still checking the DDPerror-not-allowedpayload) is consistent with the new error-handling contract. This looks good.
1108-1123: RC-side add‑user permission failure correctly marked as an errorHere as well, flipping the expectation to
success: falsewhile validating theerror-not-allowedDDP error matches the intended behavior for disallowed membership changes in group DMs. No issues.
1661-1674: Blocking conversion of non‑federated 1:1 DM into group DM via external inviteThe new
success: falseassertion anderror-cant-invite-for-direct-roomcheck correctly encode the policy that you cannot add an external user to a non‑federated direct room. This aligns with the updated API semantics.apps/meteor/tests/end-to-end/api/methods/2fa-enable.ts (1)
142-161: Re‑enabling 2FA now correctly treated as a failing method callUpdating this test to expect HTTP 400,
success: false, and an error in the DDP result when 2FA is already enabled matches the intended contract of2fa:enable. The assertions are coherent with the new method.call failure behavior.apps/meteor/client/meteor/overrides/ddpOverREST.ts (1)
56-96: Propagating REST errors back into the DDP method pipelineWiring
error.messagethroughprocessResultin the catch branch ensures that DDP method invokers see a terminal result even when the REST call fails with a 4xx/5xx, matching the newmethod.call*error responses instead of silently dropping them. The async catch signature is fine here, and the change is consistent with the DDP-over-REST design.apps/meteor/app/api/server/v1/misc.ts (1)
579-599: Anonymous method calls now return proper failure responses on errorsSwitching the
method.callAnon/:methodcatch branch fromAPI.v1.success(mountResult({ id, error }))toAPI.v1.failure(mountResult({ id, error }))makes anonymous DDP-over-REST calls behave consistently with authenticated ones: HTTP 4xx status andsuccess: falseon error while still returning the DDPmessage. This matches the updated test expectations and is a solid improvement.apps/meteor/tests/end-to-end/api/guest-permissions.ts (1)
96-116: Guest non‑whitelisted permission now correctly asserted as a 400 failureUpdating this test to require HTTP 400,
success: false, and a DDP error with reason"Permission is restricted"matches the tightened server-side guard around guest permissions and the new method.call failure contract.apps/meteor/app/api/server/definition.ts (1)
59-65: LGTM: Well-structured error type for rate limiting.The new
TooManyRequestsResult<T>type follows the established pattern of other error result types and provides proper typing for HTTP 429 responses with strongly-typed error payloads.apps/meteor/tests/end-to-end/api/methods.ts (1)
169-171: Consistent test updates align with new error handling.The test expectations have been systematically updated to reflect the new error handling behavior where validation and business logic errors return HTTP 400 with
success: false. This aligns with the PR's objective to improve error handling for DDP over REST.The changes are consistent across all affected test cases while correctly preserving HTTP 401 for authentication errors.
Also applies to: 580-582, 738-740, 907-909, 1125-1127, 1305-1307, 1566-1568, 1587-1589, 2026-2028, 2057-2059, 2245-2247, 2268-2270, 2539-2541, 3103-3103, 3206-3206, 3230-3230, 3338-3338, 3416-3418
apps/meteor/app/api/server/ApiClass.ts (1)
40-40: LGTM: Enhanced type safety for rate limit errors.Adding the explicit return type
TooManyRequestsResult<T>to thetooManyRequestsmethod improves type safety and developer experience without changing runtime behavior. This brings the method signature in line with other error helpers likeunauthorized,forbidden, etc.Also applies to: 387-395
There was a problem hiding this comment.
Pull request overview
This PR changes the HTTP status code behavior for DDP (Distributed Data Protocol) over REST API endpoints, transforming error responses from returning HTTP 200 with success: true to returning appropriate error status codes (400, 401, 403, 429) with success: false. This breaking change improves API semantics by making error responses distinguishable at the HTTP level rather than requiring clients to parse response bodies.
Key Changes
- Error responses from DDP over REST now return proper HTTP error codes (400 for general failures, 401 for unauthorized, 403 for forbidden, 429 for rate limiting) instead of always returning 200
- Error response bodies now have
success: falseinstead ofsuccess: true - Added specialized error handling for rate limiting, unauthorized, and forbidden errors in the authenticated method call endpoint
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/meteor/app/api/server/v1/misc.ts | Updated error handling in method.call endpoint to return proper HTTP status codes based on error type; updated method.callAnon endpoint to use failure response |
| apps/meteor/app/api/server/ApiClass.ts | Added type parameter to tooManyRequests method for better type safety |
| apps/meteor/app/api/server/definition.ts | Added TooManyRequestsResult type definition for 429 status code responses |
| apps/meteor/client/meteor/overrides/ddpOverREST.ts | Updated error handling to process error messages from failed API calls |
| apps/meteor/tests/end-to-end/api/methods.ts | Updated test expectations to match new HTTP status codes (400 instead of 200) and success field values (false instead of true) for error responses; skipped two tests with TODO comments |
| apps/meteor/tests/end-to-end/api/methods/2fa-enable.ts | Updated test to expect 400 status code and success: false for error responses |
| apps/meteor/tests/end-to-end/api/guest-permissions.ts | Updated test to expect 400 status code and success: false for error responses |
| apps/meteor/tests/end-to-end/api/abac.ts | Updated test to expect 400 status code for error responses |
| ee/packages/federation-matrix/tests/end-to-end/room.spec.ts | Updated test to expect success: false for error responses |
| ee/packages/federation-matrix/tests/end-to-end/permissions.spec.ts | Updated test to expect 400 status code and success: false for error responses |
| ee/packages/federation-matrix/tests/end-to-end/dms.spec.ts | Updated test expectations to expect success: false for error responses in multiple test cases |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| .expect(200) | ||
| .expect(400) | ||
| .expect((res) => { | ||
| expect(res.body).to.have.property('success', true); |
There was a problem hiding this comment.
Test expectation is inconsistent with the HTTP status code. The test expects HTTP status 400 (error response) but still checks for success: true. When an error response is returned with status 400, the success field should be false to indicate the request failed.
| expect(res.body).to.have.property('success', true); | |
| expect(res.body).to.have.property('success', false); |
| .expect(200) | ||
| .expect(400) | ||
| .expect((res) => { | ||
| expect(res.body).to.have.property('success', true); |
There was a problem hiding this comment.
Test expectations are inconsistent. The test expects HTTP status 400 (error response) but checks for success: true. When an error response is returned with status 400, the success field should be false to indicate the request failed.
| expect(res.body).to.have.property('success', true); | |
| expect(res.body).to.have.property('success', false); |
| .expect(200) | ||
| .expect(400) | ||
| .expect((res) => { | ||
| expect(res.body).to.have.property('success', true); |
There was a problem hiding this comment.
Test expectations are inconsistent. The test expects HTTP status 400 (error response) but checks for success: true. When an error response is returned with status 400, the success field should be false to indicate the request failed.
| expect(res.body).to.have.property('success', true); | |
| expect(res.body).to.have.property('success', false); |
| .catch(async (error) => { | ||
| if ('message' in error && error.message) { | ||
| processResult(error.message); |
There was a problem hiding this comment.
The error handling logic has a potential issue. The code checks for 'message' in error and accesses error.message, but according to the new API response structure for errors, the message field contains a JSON string that needs to be processed. The current implementation treats it as a regular string without considering it might be a stringified DDP message. Additionally, the async keyword is added to the catch callback but the function doesn't use await, making it unnecessarily async.
| .catch(async (error) => { | |
| if ('message' in error && error.message) { | |
| processResult(error.message); | |
| .catch((error) => { | |
| try { | |
| if (error && typeof error === 'object' && 'message' in error && (error as { message?: unknown }).message) { | |
| const rawMessage = (error as { message?: unknown }).message; | |
| if (typeof rawMessage === 'string') { | |
| let processedMessage = rawMessage; | |
| try { | |
| const parsed = JSON.parse(rawMessage); | |
| if (parsed && typeof parsed === 'object') { | |
| if ('message' in parsed && typeof (parsed as { message?: unknown }).message === 'string') { | |
| processedMessage = (parsed as { message: string }).message; | |
| } else { | |
| processedMessage = DDPCommon.stringifyDDP(parsed as Record<string, unknown>); | |
| } | |
| } | |
| } catch { | |
| // rawMessage is not JSON; use it as-is | |
| } | |
| processResult(processedMessage); | |
| } | |
| } | |
| } catch (parsingError) { | |
| console.error(parsingError); |
| .expect(200) | ||
| .expect((res) => { | ||
| expect(res.body).to.have.a.property('success', true); | ||
| // TODO: this test is not testing anything useful |
There was a problem hiding this comment.
The TODO comment indicates this test doesn't test anything useful, but the test is being skipped rather than removed or fixed. Consider either removing this test entirely or updating it to properly validate the expected behavior with meaningful assertions.
| Meteor._debug(`Exception while invoking method ${method}`, err); | ||
| } | ||
| return API.v1.success(mountResult({ id, error: err })); | ||
| return API.v1.failure(mountResult({ id, error: err })); |
There was a problem hiding this comment.
The error handling logic for the anonymous method call endpoint (method.callAnon) is incomplete. Unlike the authenticated endpoint (method.call), this endpoint doesn't differentiate between different error types (rate limiting, unauthorized, forbidden) and always returns a generic failure response. This creates inconsistent behavior between the two endpoints when handling errors.
…dling for rate limiting
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.