Skip to content

chore: migrate rooms.hide and rooms.open to OpenAPI-compliant pattern #39072

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

Closed
Verifieddanny wants to merge 21 commits into from
Closed

chore: migrate rooms.hide and rooms.open to OpenAPI-compliant pattern #39072

Show file tree
Hide file tree
Changes from 20 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
7144ed7
feat: migrate rooms.leave to openAPI format
Verifieddanny Feb 23, 2026
7a9f808
Merge branch 'develop' of github.com:Verifieddanny/Rocket.Chat into d…
Verifieddanny Feb 23, 2026
eefea4e
Merge branch 'develop' into refactor/migrate-rooms-leave-endpoint
Verifieddanny Feb 24, 2026
4b3767f
Merge branch 'develop' into refactor/migrate-rooms-leave-endpoint
Verifieddanny Feb 24, 2026
5fc1d43
ix: Added rest typing
Verifieddanny Feb 24, 2026
00bcd97
Merge branch 'refactor/migrate-rooms-leave-endpoint' of github.com:Ve…
Verifieddanny Feb 24, 2026
c260039
Merge branch 'develop' into refactor/migrate-rooms-leave-endpoint
Verifieddanny Feb 24, 2026
651dd01
Apply suggestion from @ggazzo
ggazzo Feb 24, 2026
5f70334
Fix: applied changeset suggestion
Verifieddanny Feb 24, 2026
16a23c5
Merge branch 'refactor/migrate-rooms-leave-endpoint' of github.com:Ve…
Verifieddanny Feb 24, 2026
9fa1c10
Merge branch 'develop' of github.com:Verifieddanny/Rocket.Chat into r…
Verifieddanny Feb 25, 2026
96c0f83
Merge branch 'develop' of github.com:Verifieddanny/Rocket.Chat into r…
Verifieddanny Feb 25, 2026
e05c7cb
chore: migrate rooms.hide and rooms.open to OpenAPI-compliant pattern
Verifieddanny Feb 26, 2026
7591ff7
Merge branch 'develop' into refactor/api-migration
Verifieddanny Feb 26, 2026
a025ef0
Merge branch 'develop' into refactor/api-migration
Verifieddanny Feb 26, 2026
7aa1adc
Merge branch 'develop' into refactor/api-migration
Verifieddanny Mar 1, 2026
4277e92
Fix: test bug not sending roomId to run test
Verifieddanny Mar 2, 2026
4150fad
Merge branch 'refactor/api-migration' of github.com:Verifieddanny/Roc…
Verifieddanny Mar 2, 2026
eb263ba
Fix: moved RoomsOpenProps and RoomsHideProps from rest-typing
Verifieddanny Mar 3, 2026
f4a4021
Fix: RoomsOpenProps and RoomsHideProps from rest-typings
Verifieddanny Mar 4, 2026
cb7d370
Fix: removed unnecessary export keyword
Verifieddanny Mar 4, 2026
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
8 changes: 8 additions & 0 deletions .changeset/chore-mirgrate-room-hide-room-open.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
---
'@rocket.chat/meteor': minor
'@rocket.chat/rest-typings': minor
---

Migrated `rooms.hide` and `rooms.open` endpoints to new OpenAPI-compliant pattern with AJV validation and response schemas.

Tracking PR: https://github.com/RocketChat/Rocket.Chat-Open-API/pull/150
144 changes: 100 additions & 44 deletions apps/meteor/app/api/server/v1/rooms.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,8 @@ import {
isRoomsExportProps,
isRoomsIsMemberProps,
isRoomsCleanHistoryProps,
isRoomsOpenProps,
isRoomsMembersOrderedByRoleProps,
isRoomsChangeArchivationStateProps,
isRoomsHideProps,
isRoomsInviteProps,
validateBadRequestErrorResponse,
validateUnauthorizedErrorResponse,
Expand Down Expand Up @@ -887,48 +885,6 @@ API.v1.addRoute(
},
);

API.v1.addRoute(
'rooms.open',
{ authRequired: true, validateParams: isRoomsOpenProps },
{
async post() {
const { roomId } = this.bodyParams;

await openRoom(this.userId, roomId);

return API.v1.success();
},
},
);

API.v1.addRoute(
'rooms.hide',
{ authRequired: true, validateParams: isRoomsHideProps },
{
async post() {
const { roomId } = this.bodyParams;

if (!(await canAccessRoomIdAsync(roomId, this.userId))) {
return API.v1.unauthorized();
}

const user = await Users.findOneById(this.userId, { projections: { _id: 1 } });

if (!user) {
return API.v1.failure('error-invalid-user');
}

const modCount = await hideRoomMethod(this.userId, roomId);

if (!modCount) {
return API.v1.failure('error-room-already-hidden');
}

return API.v1.success();
},
},
);

type RoomsFavorite =
| {
roomId: string;
Expand All @@ -947,6 +903,14 @@ type RoomsLeave =
roomName: string;
};

type RoomsHideProps = {
roomId: string;
};

type RoomsOpenProps = {
roomId: string;
};

const isRoomGetRolesPropsSchema = {
type: 'object',
properties: {
Expand Down Expand Up @@ -1000,8 +964,34 @@ const isRoomsLeavePropsSchema = {
],
};

const roomsHideSchema = {
type: 'object',
properties: {
roomId: {
type: 'string',
minLength: 1,
},
},
required: ['roomId'],
additionalProperties: false,
};

const roomsOpenSchema = {
type: 'object',
properties: {
roomId: {
type: 'string',
minLength: 1,
},
},
required: ['roomId'],
additionalProperties: false,
};

const isRoomsFavoriteProps = ajv.compile<RoomsFavorite>(RoomsFavoriteSchema);
const isRoomsLeaveProps = ajv.compile<RoomsLeave>(isRoomsLeavePropsSchema);
const isRoomsOpenProps = ajv.compile<RoomsOpenProps>(roomsOpenSchema);
export const isRoomsHideProps = ajv.compile<RoomsHideProps>(roomsHideSchema);
Copy link
Copy Markdown
Contributor

@ahmed-n-abdeltwab ahmed-n-abdeltwab Mar 4, 2026

Choose a reason for hiding this comment

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

why do you export isRoomsHideProps? is it necessary?

Copy link
Copy Markdown
Contributor Author

@Verifieddanny Verifieddanny Mar 4, 2026

Choose a reason for hiding this comment

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

That was an oversight

Updating it now


export const roomEndpoints = API.v1
.get(
Expand Down Expand Up @@ -1207,6 +1197,72 @@ export const roomEndpoints = API.v1

await leaveRoomMethod(user, room._id);

return API.v1.success();
},
)
.post(
'rooms.hide',
{
authRequired: true,
body: isRoomsHideProps,
response: {
200: ajv.compile<void>({
type: 'object',
properties: {
success: { type: 'boolean', enum: [true] },
},
required: ['success'],
additionalProperties: false,
}),
400: validateBadRequestErrorResponse,
401: validateUnauthorizedErrorResponse,
},
},
async function action() {
const { roomId } = this.bodyParams;

if (!(await canAccessRoomIdAsync(roomId, this.userId))) {
return API.v1.unauthorized('Unauthorized');
}

const user = await Users.findOneById(this.userId, { projections: { _id: 1 } });
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Feb 26, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Typo in projection option: projections should be projection.

MongoDB/Mongoose uses projection (singular) as the option name. This typo will cause the projection to be ignored, returning the full user document instead of just { _id: 1 }.

Proposed fix 
-		const user = await Users.findOneById(this.userId, { projections: { _id: 1 } });
+		const user = await Users.findOneById(this.userId, { projection: { _id: 1 } });
📝 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
const user = await Users.findOneById(this.userId, { projections: { _id: 1 } });
const user = await Users.findOneById(this.userId, { projection: { _id: 1 } });
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@apps/meteor/app/api/server/v1/rooms.ts` at line 1175, Fix the typo in the
query options for Users.findOneById: change the option key from "projections" to
the correct Mongo/Mongoose key "projection" so the call
Users.findOneById(this.userId, { projection: { _id: 1 } }) actually returns only
the _id; update the invocation in the surrounding function to use "projection"
wherever "projections" appears.

Copy link
Copy Markdown
Contributor

@ahmed-n-abdeltwab ahmed-n-abdeltwab Feb 26, 2026

Choose a reason for hiding this comment

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

isnt this outoff the scoop for this PR?

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Feb 26, 2026

Choose a reason for hiding this comment

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

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Search for the old implementation of rooms.hide endpoint to check if the typo existed before
# Look for the legacy API.v1.addRoute pattern for rooms.hide
git log -p --all -S "rooms.hide" -- "apps/meteor/app/api/server/v1/rooms.ts" | grep -A 20 -B 5 "projections\|projection" | head -100

Length of output: 59

🏁 Script executed:

#!/bin/bash
# Also search the current state of develop/main branch to see the original implementation
git show origin/develop:apps/meteor/app/api/server/v1/rooms.ts | grep -A 30 "rooms.hide" | grep -A 20 "Users.findOneById"

Length of output: 408

🏁 Script executed:

#!/bin/bash
# Search for other usages of Users.findOneById in the file to see the correct pattern
rg -A 2 "Users\.findOneById" apps/meteor/app/api/server/v1/rooms.ts

Length of output: 354

@ahmed-n-abdeltwab You're absolutely right—this is out of scope for this migration PR.

I checked the original implementation on the develop branch, and the projections typo already existed in the old code. Since this PR is focused on migrating to the OpenAPI pattern while preserving existing behavior, fixing the typo would constitute a logic change (the current code returns the full user document, fixing it would return only _id).

Per the migration guidelines, this PR should maintain behavioral parity with the original implementation, even if that includes pre-existing bugs. The typo should be addressed in a separate bug-fix PR.

My apologies for flagging this during a migration-focused review. Would you like me to open a follow-up issue to track fixing this typo across the codebase?

🧠 Learnings used 
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38219
File: packages/core-typings/src/cloud/Announcement.ts:5-6
Timestamp: 2026-01-17T01:51:47.764Z
Learning: In packages/core-typings/src/cloud/Announcement.ts, the AnnouncementSchema.createdBy field intentionally overrides IBannerSchema.createdBy (object with _id and optional username) with a string enum ['cloud', 'system'] to match existing runtime behavior. This is documented as technical debt with a FIXME comment at apps/meteor/app/cloud/server/functions/syncWorkspace/handleCommsSync.ts:53 and should not be flagged as an error until the runtime behavior is corrected.

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

Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:01.522Z
Learning: In Rocket.Chat OpenAPI migration PRs for endpoints under apps/meteor/app/api/server/v1, avoid introducing logic changes. Only perform scope-tight changes that preserve behavior; style-only cleanups (e.g., removing inline comments) may be deferred to follow-ups to keep the migration PR focused.

Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 35995
File: apps/meteor/app/api/server/v1/rooms.ts:1107-1112
Timestamp: 2026-02-23T17:53:06.802Z
Learning: During PR reviews that touch endpoint files under apps/meteor/app/api/server/v1, enforce strict scope: if a PR targets a specific endpoint (e.g., rooms.favorite), do not propose changes to unrelated endpoints (e.g., rooms.invite) unless maintainers explicitly request them. Focus feedback on the touched endpoint's behavior, API surface, and related tests; avoid broad cross-endpoint changes in the same PR unless requested.


if (!user) {
return API.v1.failure('error-invalid-user');
}

const modCount = await hideRoomMethod(this.userId, roomId);

if (!modCount) {
return API.v1.failure('error-room-already-hidden');
}

return API.v1.success();
},
)
.post(
'rooms.open',
{
authRequired: true,
body: isRoomsOpenProps,
response: {
200: ajv.compile<void>({
type: 'object',
properties: {
success: { type: 'boolean', enum: [true] },
},
required: ['success'],
additionalProperties: false,
}),
400: validateBadRequestErrorResponse,
401: validateUnauthorizedErrorResponse,
},
},
async function action() {
const { roomId } = this.bodyParams;

await openRoom(this.userId, roomId);

return API.v1.success();
},
);
Expand Down
1 change: 1 addition & 0 deletions apps/meteor/tests/end-to-end/api/rooms.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4215,6 +4215,7 @@ describe('[Rooms]', () => {
it('should return 401 if user is not logged in', async () => {
await request
.post(api('rooms.hide'))
.send({ roomId: roomA._id })
Copy link
Copy Markdown
Contributor

@ahmed-n-abdeltwab ahmed-n-abdeltwab Mar 4, 2026

Choose a reason for hiding this comment

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

why?

Copy link
Copy Markdown
Contributor Author

@Verifieddanny Verifieddanny Mar 4, 2026

Choose a reason for hiding this comment

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

The test was sending a request with no body and no credentials, which caused AJV validation to run first and return 400 (missing roomId) before the auth middleware could return 401. Adding .send({ roomId: roomA._id }) ensures the request body is valid so that authentication is the only failing condition, making the test correctly assert the 401 behavior.

Copy link
Copy Markdown
Contributor

@ahmed-n-abdeltwab ahmed-n-abdeltwab Mar 4, 2026

Choose a reason for hiding this comment

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

The test was sending a request with no body and no credentials, which caused AJV validation to run first and return 400 (missing roomId) before the auth middleware could return 401. Adding .send({ roomId: roomA._id }) ensures the request body is valid so that authentication is the only failing condition, making the test correctly assert the 401 behavior.

make sense

.expect('Content-Type', 'application/json')
.expect(401)
.expect((res) => {
Expand Down
44 changes: 0 additions & 44 deletions packages/rest-typings/src/v1/rooms.ts
View file
Open in desktop
Copy link
Copy Markdown
Contributor

@ahmed-n-abdeltwab ahmed-n-abdeltwab Feb 26, 2026

Choose a reason for hiding this comment

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

you forget to remove RoomsOpenProps and RoomsHideProps from rest-typings and move them into the corresponding API file apps/meteor/app/api/server/v1/rooms.ts

Copy link
Copy Markdown
Contributor Author

@Verifieddanny Verifieddanny Mar 3, 2026

Choose a reason for hiding this comment

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

Just seeing this now, I've immediately done this and pushed

Original file line number Diff line number Diff line change
Expand Up @@ -609,24 +609,6 @@ const roomsCleanHistorySchema = {

export const isRoomsCleanHistoryProps = ajv.compile<RoomsCleanHistoryProps>(roomsCleanHistorySchema);

type RoomsOpenProps = {
roomId: string;
};

const roomsOpenSchema = {
type: 'object',
properties: {
roomId: {
type: 'string',
minLength: 1,
},
},
required: ['roomId'],
additionalProperties: false,
};

export const isRoomsOpenProps = ajv.compile<RoomsOpenProps>(roomsOpenSchema);

type MembersOrderedByRoleProps = {
roomId?: IRoom['_id'];
roomName?: IRoom['name'];
Expand Down Expand Up @@ -669,24 +651,6 @@ const membersOrderedByRoleRolePropsSchema = {

export const isRoomsMembersOrderedByRoleProps = ajv.compile<RoomsMembersOrderedByRoleProps>(membersOrderedByRoleRolePropsSchema);

type RoomsHideProps = {
roomId: string;
};

const roomsHideSchema = {
type: 'object',
properties: {
roomId: {
type: 'string',
minLength: 1,
},
},
required: ['roomId'],
additionalProperties: false,
};

export const isRoomsHideProps = ajv.compile<RoomsHideProps>(roomsHideSchema);

type RoomsInviteProps = {
roomId: string;
action: 'accept' | 'reject';
Expand Down Expand Up @@ -841,20 +805,12 @@ export type RoomsEndpoints = {
}>;
};

'/v1/rooms.open': {
POST: (params: RoomsOpenProps) => void;
};

'/v1/rooms.membersOrderedByRole': {
GET: (params: RoomsMembersOrderedByRoleProps) => PaginatedResult<{
members: (IUser & { subscription: Pick<ISubscription, '_id' | 'status' | 'ts' | 'roles'> })[];
}>;
};

'/v1/rooms.hide': {
POST: (params: RoomsHideProps) => void;
};

'/v1/rooms.invite': {
POST: (params: RoomsInviteProps) => void;
};
Expand Down