chore(federation): rely on MongoDB client to parse db name instead of manual URL parsing #37571
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 |
|
WalkthroughRemoves URL parsing that extracted database name from the MongoDB connection string in federation-matrix setup and inlines the connection URI from env/fallback, keeping other dbConfig fields and control flow unchanged. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.{ts,tsx,js}📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
Files:
🧠 Learnings (3)📓 Common learnings📚 Learning: 2025-11-05T21:04:35.787ZApplied to files:
📚 Learning: 2025-09-19T15:15:04.642ZApplied to files:
⏰ 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). (3)
🔇 Additional comments (1)
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 |
![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
🧹 Nitpick comments (1)
ee/packages/federation-matrix/src/setup.ts (1)
109-111: Directly passingMONGO_URLtodbConfig.urifixes multi‑host parsing; confirmDATABASE_NAMEbehavior change is acceptable.Removing local URL/dbName parsing and handing the full
MONGO_URLtoinitshould avoidERR_INVALID_URLfor replica‑set / Atlas‑style strings and lets the Mongo client own db‑name parsing. However, this also means the federation‑matrix setup no longer appears to consult a separateDATABASE_NAMEoverride; environments that previously relied onMONGO_URLwithout a db segment plusDATABASE_NAMEwill now depend solely on what’s encoded inMONGO_URL(and howinithandles URIs without a db).If it’s intentional to require the DB name in
MONGO_URLfor federation, this is fine—just worth double‑checking against existing deployments and docs. If backward compatibility withDATABASE_NAMEis desired, we may want to handle that explicitly in the SDK/init layer without reintroducing manual URL parsing here.Please verify against the current
@rocket.chat/federation-sdkinitimplementation (and deployment configs) that:
- Multi‑host Mongo URIs now work end‑to‑end, and
- Any expected
DATABASE_NAMEusage is either no longer needed or handled/documented elsewhere.
📜 Review details
Configuration used: CodeRabbit 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 (3)
ee/packages/federation-matrix/src/events/room.ts(1 hunks)ee/packages/federation-matrix/src/helpers/message.parsers.ts(1 hunks)ee/packages/federation-matrix/src/setup.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
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.
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37357
File: ee/packages/federation-matrix/src/setup.ts:103-120
Timestamp: 2025-11-05T21:04:35.787Z
Learning: In Rocket.Chat's federation-matrix setup (ee/packages/federation-matrix/src/setup.ts and apps/meteor/ee/server/startup/federation.ts), configureFederationMatrixSettings does not need to be called before setupFederationMatrix. The SDK's init() establishes infrastructure (database, event handlers, APIs) first, and the configuration can be applied later via settings watchers before actual federation events are processed. The config only matters when events actually occur, at which point all infrastructure is already configured.
📚 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/src/events/room.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/src/events/room.tsee/packages/federation-matrix/src/setup.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: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings (mapping subscription documents to room IDs), never undefined, even when user has no room subscriptions.
Applied to files:
ee/packages/federation-matrix/src/events/room.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: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings by mapping subscription documents to room IDs, never undefined, even when user has no room subscriptions.
Applied to files:
ee/packages/federation-matrix/src/events/room.ts
📚 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/src/events/room.ts
📚 Learning: 2025-11-05T21:04:35.787Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37357
File: ee/packages/federation-matrix/src/setup.ts:103-120
Timestamp: 2025-11-05T21:04:35.787Z
Learning: In Rocket.Chat's federation-matrix setup (ee/packages/federation-matrix/src/setup.ts and apps/meteor/ee/server/startup/federation.ts), configureFederationMatrixSettings does not need to be called before setupFederationMatrix. The SDK's init() establishes infrastructure (database, event handlers, APIs) first, and the configuration can be applied later via settings watchers before actual federation events are processed. The config only matters when events actually occur, at which point all infrastructure is already configured.
Applied to files:
ee/packages/federation-matrix/src/setup.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). (2)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
🔇 Additional comments (2)
ee/packages/federation-matrix/src/events/room.ts (1)
9-14: Event handler updated to use nestedeventpayload – looks consistent.This aligns the
homeserver.matrix.room.namehandler with the newer{ event }payload shape and matches how other Matrix room events are handled; downstream logic is unchanged.Please confirm that the current
@rocket.chat/federation-sdktypings and emitter really pass{ event }for'homeserver.matrix.room.name'so this stays in sync with the SDK contract.ee/packages/federation-matrix/src/helpers/message.parsers.ts (1)
5-5: MatrixMessageContent now points atevent.content– aligned with new event shape.Typing this as
['homeserver.matrix.message']['event']['content']keeps the helpers consistent with the updated federation event structure; the extraformat?: stringoverlay is fine for local usage.Please double‑check against the current
HomeserverEventSignaturesdefinition in@rocket.chat/federation-sdkto ensurehomeserver.matrix.message.event.contentremains the canonical path for message content.
|
Waiting for #37570 to be approved so I can rebase this branch and keep the PR lean. Edit: done! |
ricardogarim
force-pushed
the
chore/federation-dbname
branch
from
5052820 to
f190895
Compare
ggazzo
force-pushed
the
chore/federation-dbname
branch
from
f190895 to
310dcd1
Compare
📦 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/15 22:28", "11/16 01:28", "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 15:59", "12/01 16:24 (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]
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]
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]
line "omnichannel-transcript-service" [0.14, 0.14, 0.14, 0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.14]
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]
line "queue-worker-service" [0.14, 0.14, 0.14, 0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.14]
line "rocketchat" [0.36, 0.36, 0.35, 0.35, 0.35, 0.34, 0.34, 0.34, 0.34, 0.34, 0.36]
line "stream-hub-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
Statistics (last 10 days):
ℹ️ About this reportThis report compares Docker image sizes from this build against the
Updated: Mon, 01 Dec 2025 16:24:18 GMT |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## chore/fed-regressions #37571 +/- ##
========================================================
Coverage ? 68.91%
========================================================
Files ? 3360
Lines ? 114279
Branches ? 20562
========================================================
Hits ? 78756
Misses ? 33430
Partials ? 2093
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
ggazzo
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
As per FB-72, when using MongoDB replica-set URLs (multiple hosts), the federation-matrix setup attempts to extract the database name by instantiating a URL object. Even though these MongoDB URLs are valid, the URL constructor does not support this format and throws
ERR_INVALID_URL, preventing federation from starting.This PR removes the manual URL parsing and dbName handling. We now read the database name directly from the MongoDB connection string using the MongoDB client parser. If the connection string does not contain a database name, we explicitly throw a clear error.
Works along with RocketChat/homeserver#303.
Summary by CodeRabbit
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.