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 14 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
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 }));
Copy link

Copilot AI Dec 30, 2025

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
}
},
},
Expand Down
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
74 changes: 38 additions & 36 deletions apps/meteor/tests/end-to-end/api/methods.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
expect(res.body).to.have.property('success', false);

const data = JSON.parse(res.body.message);
expect(data).to.have.property('error').that.is.an('object');
expect(data.error).to.have.property('error', 'error-action-not-allowed');
Expand Down Expand Up @@ -576,9 +577,9 @@ describe('Meteor.methods', () => {
}),
})
.expect('Content-Type', 'application/json')
.expect(200)
.expect(400)
.expect((res) => {
expect(res.body).to.have.a.property('success', true);
expect(res.body).to.have.a.property('success', false);
expect(res.body).to.have.a.property('message').that.is.a('string');

const data = JSON.parse(res.body.message);
Expand Down Expand Up @@ -734,9 +735,9 @@ describe('Meteor.methods', () => {
msg: 'method',
}),
})
.expect(200)
.expect(400)
.expect((res) => {
expect(res.body).to.have.a.property('success', true);
expect(res.body).to.have.a.property('success', false);
const data = JSON.parse(res.body.message);
expect(data).to.have.a.property('error').that.is.an('object');
expect(data.error).to.have.a.property('error', 'error-not-allowed');
Expand Down Expand Up @@ -903,9 +904,9 @@ describe('Meteor.methods', () => {
}),
})
.expect('Content-Type', 'application/json')
.expect(200)
.expect(400)
.expect((res) => {
expect(res.body).to.have.a.property('success', true);
expect(res.body).to.have.a.property('success', false);
expect(res.body).to.have.a.property('message').that.is.a('string');

const data = JSON.parse(res.body.message);
Expand Down Expand Up @@ -1121,9 +1122,9 @@ describe('Meteor.methods', () => {
}),
})
.expect('Content-Type', 'application/json')
.expect(200)
.expect(400)
.expect((res) => {
expect(res.body).to.have.a.property('success', true);
expect(res.body).to.have.a.property('success', false);
expect(res.body).to.have.a.property('message').that.is.a('string');

const data = JSON.parse(res.body.message);
Expand Down Expand Up @@ -1301,9 +1302,9 @@ describe('Meteor.methods', () => {
}),
})
.expect('Content-Type', 'application/json')
.expect(200)
.expect(400)
.expect((res) => {
expect(res.body).to.have.a.property('success', true);
expect(res.body).to.have.a.property('success', false);
expect(res.body).to.have.a.property('message').that.is.a('string');

const data = JSON.parse(res.body.message);
Expand Down Expand Up @@ -1562,9 +1563,9 @@ describe('Meteor.methods', () => {
}),
})
.expect('Content-Type', 'application/json')
.expect(200)
.expect(400)
.expect((res) => {
expect(res.body).to.have.a.property('success', true);
expect(res.body).to.have.a.property('success', false);
expect(res.body).to.have.a.property('message').that.include('error-invalid-room');
})
.end(done);
Expand All @@ -1583,9 +1584,9 @@ describe('Meteor.methods', () => {
}),
})
.expect('Content-Type', 'application/json')
.expect(200)
.expect(400)
.expect((res) => {
expect(res.body).to.have.a.property('success', true);
expect(res.body).to.have.a.property('success', false);
expect(res.body).to.have.a.property('message').that.include('Match error');
})
.end(done);
Expand Down Expand Up @@ -2022,9 +2023,9 @@ describe('Meteor.methods', () => {
}),
})
.expect('Content-Type', 'application/json')
.expect(200)
.expect(400)
.expect((res) => {
expect(res.body).to.have.property('success', true);
expect(res.body).to.have.property('success', false);
const data = JSON.parse(res.body.message);
expect(data).to.not.have.a.property('result').that.is.an('object');
expect(data).to.have.a.property('error').that.is.an('object');
Expand Down Expand Up @@ -2053,9 +2054,9 @@ describe('Meteor.methods', () => {
}),
})
.expect('Content-Type', 'application/json')
.expect(200)
.expect(400)
.expect((res) => {
expect(res.body).to.have.property('success', true);
expect(res.body).to.have.property('success', false);
const data = JSON.parse(res.body.message);
expect(data).to.have.a.property('error').that.is.an('object');
expect(data.error.sanitizedError).to.have.a.property('reason', 'Match failed');
Expand Down Expand Up @@ -2241,9 +2242,9 @@ describe('Meteor.methods', () => {
}),
})
.expect('Content-Type', 'application/json')
.expect(200)
.expect(400)
.expect((res) => {
expect(res.body).to.have.a.property('success', true);
expect(res.body).to.have.a.property('success', false);
expect(res.body).to.have.a.property('message').that.is.a('string');
const data = JSON.parse(res.body.message);
expect(data).to.have.a.property('msg').that.is.an('string');
Expand All @@ -2264,17 +2265,17 @@ describe('Meteor.methods', () => {
}),
})
.expect('Content-Type', 'application/json')
.expect(200)
.expect(400)
.expect((res) => {
expect(res.body).to.have.a.property('success', true);
expect(res.body).to.have.a.property('success', false);
expect(res.body).to.have.a.property('message').that.is.a('string');
const data = JSON.parse(res.body.message);
expect(data).to.have.a.property('msg').that.is.an('string');
expect(data.error).to.have.a.property('error', 'error-action-not-allowed');
});
});

it('should add a quote attachment to a message', async () => {
it.skip('should add a quote attachment to a message', async () => {
const quotedMsgLink = `${siteUrl}/group/${roomName}?msg=${messageWithMarkdownId}`;
await request
.post(methodCall('updateMessage'))
Expand All @@ -2291,6 +2292,7 @@ describe('Meteor.methods', () => {
.expect(200)
.expect((res) => {
expect(res.body).to.have.a.property('success', true);
// TODO: this test is not testing anything useful
Copy link

Copilot AI Dec 30, 2025

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
expect(res.body).to.have.a.property('message').that.is.a('string');
});

Expand Down Expand Up @@ -2378,7 +2380,7 @@ describe('Meteor.methods', () => {
});
});

it('should remove a quote attachment from a message', async () => {
it.skip('should remove a quote attachment from a message', async () => {
await request
.post(methodCall('updateMessage'))
.set(credentials)
Expand Down Expand Up @@ -2534,9 +2536,9 @@ describe('Meteor.methods', () => {
}),
})
.expect('Content-Type', 'application/json')
.expect(200)
.expect(400)
.expect((res) => {
expect(res.body).to.have.a.property('success', true);
expect(res.body).to.have.a.property('success', false);
expect(res.body).to.have.a.property('message').that.is.a('string');

const data = JSON.parse(res.body.message);
Expand Down Expand Up @@ -3098,7 +3100,7 @@ describe('Meteor.methods', () => {
}),
})
.expect('Content-Type', 'application/json')
.expect(200)
.expect(400)
.expect((res) => {
expect(res.body).to.have.property('message').that.is.an('string');
expect(res.body.message).to.include('error-cant-invite-for-direct-room');
Expand Down Expand Up @@ -3201,9 +3203,9 @@ describe('Meteor.methods', () => {
}),
})
.expect('Content-Type', 'application/json')
.expect(200)
.expect(400)
.expect((res) => {
expect(res.body).to.have.property('success', true);
expect(res.body).to.have.property('success', false);
const parsedBody = JSON.parse(res.body.message);
expect(parsedBody).to.have.property('error');
expect(parsedBody.error).to.have.property('error', 'error-max-rooms-per-guest-reached');
Expand All @@ -3225,9 +3227,9 @@ describe('Meteor.methods', () => {
params: [[{ _id: 'Message_AllowEditing_BlockEditInMinutes', value: { $InfNaN: 0 } }]],
}),
})
.expect(200)
.expect(400)
.expect((res) => {
expect(res.body).to.have.property('success', true);
expect(res.body).to.have.property('success', false);
const parsedBody = JSON.parse(res.body.message);
expect(parsedBody).to.have.property('error');
expect(parsedBody.error).to.have.property('error', 'Invalid setting value NaN');
Expand Down Expand Up @@ -3333,7 +3335,7 @@ describe('Meteor.methods', () => {
}),
})
.expect('Content-Type', 'application/json')
.expect(200)
.expect(400)
.expect((res) => {
expect(res.body).to.have.a.property('message');
const data = JSON.parse(res.body.message);
Expand Down Expand Up @@ -3411,9 +3413,9 @@ describe('Meteor.methods', () => {
}),
})
.expect('Content-Type', 'application/json')
.expect(200)
.expect(400)
.expect((res) => {
expect(res.body).to.have.a.property('success', true);
expect(res.body).to.have.a.property('success', false);
expect(res.body).to.have.a.property('message').that.is.a('string');

const data = JSON.parse(res.body.message);
Expand Down
4 changes: 2 additions & 2 deletions apps/meteor/tests/end-to-end/api/methods/2fa-enable.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,9 +151,9 @@ describe('2fa:enable', function () {
params: [],
}),
})
.expect(200)
.expect(400)
.expect((res) => {
expect(res.body).to.have.property('success', true);
expect(res.body).to.have.property('success', false);
const parsedBody = JSON.parse(res.body.message);
expect(parsedBody).to.have.property('error');
expect(parsedBody).to.not.have.property('result');
Expand Down
6 changes: 3 additions & 3 deletions ee/packages/federation-matrix/tests/end-to-end/dms.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -646,7 +646,7 @@ const waitForRoomEvent = async (
config: rcUserConfig1,
});

expect(response.body).toHaveProperty('success', true);
expect(response.body).toHaveProperty('success', false);
expect(response.body).toHaveProperty('message');

// Parse the error message from the DDP response
Expand Down Expand Up @@ -1113,7 +1113,7 @@ const waitForRoomEvent = async (
config: rcUserConfig2,
});

expect(response.body).toHaveProperty('success', true);
expect(response.body).toHaveProperty('success', false);
expect(response.body).toHaveProperty('message');

// Parse the error message from the DDP response
Expand Down Expand Up @@ -1664,7 +1664,7 @@ const waitForRoomEvent = async (
config: rcUser1.config,
});

expect(response.body).toHaveProperty('success', true);
expect(response.body).toHaveProperty('success', false);
expect(response.body).toHaveProperty('message');

// Parse the error message from the DDP response
Expand Down
4 changes: 2 additions & 2 deletions ee/packages/federation-matrix/tests/end-to-end/permissions.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,8 +186,8 @@ import { SynapseClient } from '../helper/synapse-client';
config: rc1AdminRequestConfig,
});

expect(addUserResponse.status).toBe(200);
expect(addUserResponse.body).toHaveProperty('success', true);
expect(addUserResponse.status).toBe(400);
expect(addUserResponse.body).toHaveProperty('success', false);
expect(addUserResponse.body.message).toMatch(/error-not-authorized-federation/);
});

Expand Down
2 changes: 1 addition & 1 deletion ee/packages/federation-matrix/tests/end-to-end/room.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,7 @@ import { SynapseClient } from '../helper/synapse-client';
config: rc1AdminRequestConfig,
});

expect(response.body).toHaveProperty('success', true);
expect(response.body).toHaveProperty('success', false);
expect(response.body).toHaveProperty('message');

// Parse the error message from the DDP response
Expand Down
Loading