Skip to content

chore!: change http code results for ddp over rest #38007

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ggazzo merged 16 commits into from
Dec 30, 2025
Merged

chore!: change http code results for ddp over rest #38007

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
4a3b4b1
feat(api): implement TooManyRequestsResult type and enhance error han…
ggazzo Dec 29, 2025
47def35
Merge branch 'release-8.0.0' into chore/ddp-over-rest
ggazzo Dec 29, 2025
574aed8
test(api): update method tests to expect 400 status and error responses
ggazzo Dec 29, 2025
ea7e639
fix(ddpOverREST): enhance error handling to process error messages co…
ggazzo Dec 29, 2025
44bc0b6
fix(tests): update API method test to expect success response
ggazzo Dec 30, 2025
a2257f2
fix(tests): update end-to-end tests to expect failure responses
ggazzo Dec 30, 2025
d03ff97
fix(tests): adjust end-to-end test to expect failure with 400 status
ggazzo Dec 30, 2025
50dc670
fix(tests): conditionally skip getReadReceipts test for non-enterpris…
ggazzo Dec 30, 2025
1c767e0
fix(tests): update end-to-end test to expect failure with success pro…
ggazzo Dec 30, 2025
f747ccd
fix(tests): update 2fa-enable test to expect 400 status and success p…
ggazzo Dec 30, 2025
676fab9
Apply suggestions from code review
ggazzo Dec 30, 2025
f2dac2d
fix(api): improve error handling by throwing errors directly instead …
ggazzo Dec 30, 2025
dec5ffb
fix(api): update HTTP response codes for internal errors in method calls
ggazzo Dec 30, 2025
c828108
fix(api): return structured error responses instead of throwing excep…
ggazzo Dec 30, 2025
fe128b7
refactor(api): update starMessage method to accept user object instea…
ggazzo Dec 30, 2025
d50a169
refactor(api): remove wrong user validation for getChannelHistory
ggazzo Dec 30, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/long-swans-sin.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@rocket.chat/meteor': patch
---

Changes the HTTP code of `/api/v1/method.call` and `/api/v1/method.callAnon` in case of internal errors
Copy link
Contributor

@coderabbitai coderabbitai bot Dec 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Clarify the new HTTP response codes in the changeset description.

The description lacks specificity about what HTTP codes are now returned. Based on the PR objectives and summary, the endpoints should return 400 (and possibly other specific 40x codes) instead of 200, but this is not clear from the current text.

🔎 Suggested improvement 
-Changes the HTTP code of `/api/v1/method.call` and `/api/v1/method.callAnon` in case of internal errors
+Changes the HTTP code of `/api/v1/method.call` and `/api/v1/method.callAnon` from 200 to appropriate error codes (400+) in case of internal errors

Or, if more specificity is needed:

-Changes the HTTP code of `/api/v1/method.call` and `/api/v1/method.callAnon` in case of internal errors
+Changes the HTTP code of `/api/v1/method.call` and `/api/v1/method.callAnon` from 200 to 400/429 in case of internal/rate-limit errors

This will make the changelog entry clearer for users reviewing release notes.

📝 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.

Suggested change
Changes the HTTP code of `/api/v1/method.call` and `/api/v1/method.callAnon` in case of internal errors
Changes the HTTP code of `/api/v1/method.call` and `/api/v1/method.callAnon` from 200 to appropriate error codes (400+) in case of internal errors
🤖 Prompt for AI Agents 
In .changeset/long-swans-sin.md around line 5, the changeset message is vague
about which HTTP status codes the /api/v1/method.call and
/api/v1/method.callAnon endpoints now return; update the single-line description
to explicitly list the new status codes (e.g., "Return 400 Bad Request for
client-side validation errors instead of 200; preserve/return other appropriate
4xx codes for specific client errors") or provide a short table of endpoint ->
status mapping, and include a brief example of the most common replacement (200
-> 400) so release readers know the precise behavior change.

3 changes: 2 additions & 1 deletion apps/meteor/app/api/server/ApiClass.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ import type {
RedirectResult,
UnavailableResult,
GenericRouteExecutionContext,
TooManyRequestsResult,
} from './definition';
import { getUserInfo } from './helpers/getUserInfo';
import { parseJsonQuery } from './helpers/parseJsonQuery';
Expand Down Expand Up @@ -383,7 +384,7 @@ export class APIClass<TBasePath extends string = '', TOperations extends Record<
};
}

public tooManyRequests(msg?: string): { statusCode: number; body: Record<string, any> & { success?: boolean } } {
public tooManyRequests<T>(msg?: T): TooManyRequestsResult<T> {
return {
statusCode: 429,
body: {
Expand Down
8 changes: 8 additions & 0 deletions apps/meteor/app/api/server/definition.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,14 @@ export type ForbiddenResult<T> = {
};
};

export type TooManyRequestsResult<T> = {
statusCode: 429;
body: {
success: false;
error: T | 'Too many requests';
};
};

export type InternalError<T, StatusCode extends ErrorStatusCodes = 500, D = 'Internal server error'> = {
statusCode: StatusCode;
body: {
Expand Down
4 changes: 2 additions & 2 deletions apps/meteor/app/api/server/v1/chat.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -456,7 +456,7 @@ API.v1.addRoute(
throw new Meteor.Error('error-message-not-found', 'The provided "messageId" does not match any existing message.');
}

await starMessage(this.userId, {
await starMessage(this.user, {
_id: msg._id,
rid: msg.rid,
starred: true,
Expand All @@ -478,7 +478,7 @@ API.v1.addRoute(
throw new Meteor.Error('error-message-not-found', 'The provided "messageId" does not match any existing message.');
}

await starMessage(this.userId, {
await starMessage(this.user, {
_id: msg._id,
rid: msg.rid,
starred: false,
Expand Down
5 changes: 3 additions & 2 deletions apps/meteor/app/api/server/v1/misc.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -525,7 +525,8 @@ API.v1.addRoute(
if (settings.get('Log_Level') === '2') {
Meteor._debug(`Exception while invoking method ${method}`, err);
}
return API.v1.success(mountResult({ id, error: err }));

return API.v1.failure(mountResult({ id, error: err }));
}
},
},
Expand Down Expand Up @@ -580,7 +581,7 @@ API.v1.addRoute(
if (settings.get('Log_Level') === '2') {
Meteor._debug(`Exception while invoking method ${method}`, err);
}
return API.v1.success(mountResult({ id, error: err }));
return API.v1.failure(mountResult({ id, error: err }));
}
},
},
Expand Down
4 changes: 0 additions & 4 deletions apps/meteor/app/lib/server/methods/getChannelHistory.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,10 +48,6 @@ export const getChannelHistory = async ({
}): Promise<false | IMessage[] | { messages: IMessage[]; firstUnread?: any; unreadNotLoaded?: number }> => {
check(rid, String);

if (!Meteor.userId()) {
throw new Meteor.Error('error-invalid-user', 'Invalid user', { method: 'getChannelHistory' });
}

if (!fromUserId) {
return false;
}
Expand Down
22 changes: 12 additions & 10 deletions apps/meteor/app/message-star/server/starMessage.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
import { Apps, AppEvents } from '@rocket.chat/apps';
import type { IMessage } from '@rocket.chat/core-typings';
import type { IMessage, IUser } from '@rocket.chat/core-typings';
import type { ServerMethods } from '@rocket.chat/ddp-client';
import { Messages, Subscriptions, Rooms } from '@rocket.chat/models';
import { Meteor } from 'meteor/meteor';

import { canAccessRoomAsync, roomAccessAttributes } from '../../authorization/server';
import { isTheLastMessage } from '../../lib/server/functions/isTheLastMessage';
import { methodDeprecationLogger } from '../../lib/server/lib/deprecationWarningLogger';
import { notifyOnRoomChangedById, notifyOnMessageChange } from '../../lib/server/lib/notifyListener';
import { settings } from '../../settings/server';

Expand All @@ -16,15 +17,15 @@ declare module '@rocket.chat/ddp-client' {
}
}

export const starMessage = async (userId: string, message: Pick<IMessage, 'rid' | '_id'> & { starred: boolean }): Promise<boolean> => {
export const starMessage = async (user: IUser, message: Pick<IMessage, 'rid' | '_id'> & { starred: boolean }): Promise<boolean> => {
if (!settings.get('Message_AllowStarring')) {
throw new Meteor.Error('error-action-not-allowed', 'Message starring not allowed', {
method: 'starMessage',
action: 'Message_starring',
});
}

const subscription = await Subscriptions.findOneByRoomIdAndUserId(message.rid, userId, {
const subscription = await Subscriptions.findOneByRoomIdAndUserId(message.rid, user._id, {
projection: { _id: 1 },
});
if (!subscription) {
Expand All @@ -40,18 +41,18 @@ export const starMessage = async (userId: string, message: Pick<IMessage, 'rid'
throw new Meteor.Error('error-not-allowed', 'Not allowed', { method: 'starMessage' });
}

if (!(await canAccessRoomAsync(room, { _id: userId }))) {
if (!(await canAccessRoomAsync(room, { _id: user._id }))) {
throw new Meteor.Error('not-authorized', 'Not Authorized', { method: 'starMessage' });
}

if (isTheLastMessage(room, message)) {
await Rooms.updateLastMessageStar(room._id, userId, message.starred);
await Rooms.updateLastMessageStar(room._id, user._id, message.starred);
void notifyOnRoomChangedById(room._id);
}

await Apps.self?.triggerEvent(AppEvents.IPostMessageStarred, message, await Meteor.userAsync(), message.starred);
await Apps.self?.triggerEvent(AppEvents.IPostMessageStarred, message, user, message.starred);

await Messages.updateUserStarById(message._id, userId, message.starred);
await Messages.updateUserStarById(message._id, user._id, message.starred);

void notifyOnMessageChange({
id: message._id,
Expand All @@ -62,14 +63,15 @@ export const starMessage = async (userId: string, message: Pick<IMessage, 'rid'

Meteor.methods<ServerMethods>({
async starMessage(message) {
const uid = Meteor.userId();
methodDeprecationLogger.method('starMessage', '9.0.0', '/v1/chat.starMessage');
const user = (await Meteor.userAsync()) as IUser;

if (!uid) {
if (!user) {
throw new Meteor.Error('error-invalid-user', 'Invalid user', {
method: 'starMessage',
});
}

return starMessage(uid, message);
return starMessage(user, message);
},
});
5 changes: 4 additions & 1 deletion apps/meteor/client/meteor/overrides/ddpOverREST.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,10 @@ const withDDPOverREST = (_send: (this: Meteor.IMeteorConnection, message: Meteor

processResult(_message);
})
.catch((error) => {
.catch(async (error) => {
if ('message' in error && error.message) {
processResult(error.message);
}
console.error(error);
});
};
Expand Down
2 changes: 1 addition & 1 deletion apps/meteor/tests/end-to-end/api/abac.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -408,7 +408,7 @@ const addAbacAttributesToUserDirectly = async (userId: string, abacAttributes: I
msg: 'method',
}),
})
.expect(200)
.expect(400)
.expect((res) => {
const result = JSON.parse(res.body.message);
expect(result).to.have.property('error');
Expand Down
4 changes: 2 additions & 2 deletions apps/meteor/tests/end-to-end/api/guest-permissions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,9 +106,9 @@ import { IS_EE } from '../../e2e/config/constants';
}),
})
.expect('Content-Type', 'application/json')
.expect(200);
.expect(400);

expect(res.body).to.have.property('success', true);
expect(res.body).to.have.property('success', false);
expect(res.body).to.have.property('message');
const message = JSON.parse(res.body.message);
expect(message).to.have.property('error');
Expand Down
Loading
Loading