feat: Throw error if deprecation endpoint deprecated should be removed #36647
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 |
|
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release-8.0.0 #36647 +/- ##
=================================================
+ Coverage 66.43% 68.42% +1.98%
=================================================
Files 3276 2877 -399
Lines 109596 101934 -7662
Branches 20860 17729 -3131
=================================================
- Hits 72814 69747 -3067
+ Misses 34116 30538 -3578
+ Partials 2666 1649 -1017
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
ggazzo
force-pushed
the
release-8.0.0
branch
2 times, most recently
from
33ea06e to
9505de9
Compare
ggazzo
force-pushed
the
chore/break-deprecations
branch
2 times, most recently
from
9588c6c to
c9f8809
Compare
ggazzo
force-pushed
the
release-8.0.0
branch
from
9e4dba8 to
9aacd32
Compare
ggazzo
force-pushed
the
release-8.0.0
branch
3 times, most recently
from
e19fae0 to
0d1c2a5
Compare
juliajforesti
force-pushed
the
chore/break-deprecations
branch
from
78a372d to
5a6c610
Compare
WalkthroughAdds a pre-registration deprecation guard in ApiClass.addRoute that throws when an endpoint is marked deprecated and should break in the current version. Also narrows Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Dev / Startup
participant API as ApiClass.addRoute
participant Dep as shouldBreakInVersion
participant Sys as Router/Registry
Dev->>API: addRoute(route, options, originalAction)
Note over API: capture originalAction
API->>API: if options.deprecation?
alt deprecation present
API->>Dep: shouldBreakInVersion(options.deprecation.version)
Dep-->>API: true / false
alt true (breaking)
API-->>Dev: throw Meteor.Error('error-deprecated', "The endpoint {route} should be removed")
Note over API: registration aborted
else false
API->>Sys: register wrapped action (auth, rate-limit, perms, errors)
Sys-->>Dev: route registered
end
else no deprecation
API->>Sys: register wrapped action
Sys-->>Dev: route registered
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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 (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.{ts,tsx,js}📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
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). (2)
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)
apps/meteor/app/api/server/ApiClass.ts (1)
808-810: Fail-fast removal guard: enrich error and confirm startup-crash intentThrowing during route registration will fail server startup once the planned version is reached. If that’s the goal, keep it—otherwise gate behind an env flag or only enforce in CI/TEST.
Minimal improvement: include method and planned version to aid triage.
- if (options.deprecation && shouldBreakInVersion(options.deprecation.version)) { - throw new Meteor.Error('error-deprecated', `The endpoint ${route} should be removed`); - } + if (options.deprecation && shouldBreakInVersion(options.deprecation.version)) { + const planned = options.deprecation.version; + throw new Meteor.Error( + 'error-deprecated', + `The endpoint ${this.getFullRouteName(route, method)} is scheduled for removal in ${planned} and must be removed`, + { endpoint: route, method, planned }, + ); + }If you prefer a softer rollout aligned with the logger’s env knob, consider:
// near other consts const hardFailDeprecatedRoutes = process.env.ROCKET_CHAT_DEPRECATION_HARD_FAIL !== 'false';- if (options.deprecation && shouldBreakInVersion(options.deprecation.version)) { + if (options.deprecation && shouldBreakInVersion(options.deprecation.version) && hardFailDeprecatedRoutes) { // throw... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/meteor/app/api/server/ApiClass.ts(1 hunks)apps/meteor/app/lib/server/lib/deprecationWarningLogger.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/meteor/app/api/server/ApiClass.ts (1)
apps/meteor/server/lib/shouldBreakInVersion.ts (1)
shouldBreakInVersion(5-5)
⏰ 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)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (1)
apps/meteor/app/lib/server/lib/deprecationWarningLogger.ts (1)
27-27: Type narrowed to '9.0.0' — repo still contains '7.0.0' callers; update callers or centralizeScan found these literal occurrences — update them to match the new planned version or centralize the version constant:
- apps/meteor/client/views/admin/workspace/VersionCard/getVersionStatus.spec.ts:175,183
- apps/meteor/ee/server/api/engagementDashboard/channels.ts:63
- apps/meteor/app/authorization/server/methods/removeUserFromRole.ts:44
- apps/meteor/app/authorization/server/methods/addUserToRole.ts:44
- apps/meteor/app/api/server/v1/oauthapps.ts:296 (apiDeprecationLogger.parameter(..., '7.0.0', ...))
Optional refactor (centralize the version):
-export type DeprecationLoggerNextPlannedVersion = '9.0.0'; +export const NEXT_BREAKING_VERSION = '9.0.0' as const; +export type DeprecationLoggerNextPlannedVersion = typeof NEXT_BREAKING_VERSION;
ggazzo
force-pushed
the
release-8.0.0
branch
from
f6eaabb to
70fdc50
Compare
ggazzo
force-pushed
the
chore/break-deprecations
branch
from
97cd0ad to
7188097
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/meteor/app/api/server/ApiClass.ts (1)
810-812: LGTM! Consider enhancing the error message with version info.The pre-registration deprecation guard correctly prevents deprecated endpoints from being registered when they should break in the current version. This fail-fast approach ensures deprecated endpoints are removed before release.
Consider including the deprecation version in the error message for better developer experience:
- if (options.deprecation && shouldBreakInVersion(options.deprecation.version)) { - throw new Meteor.Error('error-deprecated', `The endpoint ${route} should be removed`); - } + if (options.deprecation && shouldBreakInVersion(options.deprecation.version)) { + throw new Meteor.Error('error-deprecated', `The endpoint ${route} should be removed (deprecated since ${options.deprecation.version})`); + }
📜 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 (2)
apps/meteor/app/api/server/ApiClass.ts(1 hunks)apps/meteor/app/lib/server/lib/deprecationWarningLogger.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/meteor/app/api/server/ApiClass.ts (1)
apps/meteor/app/livechat/server/methods/getAnalyticsChartData.ts (1)
options(18-38)
⏰ 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). (4)
- GitHub Check: 📦 Build Packages
- GitHub Check: Builds matrix rust bindings against alpine
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (1)
apps/meteor/app/lib/server/lib/deprecationWarningLogger.ts (1)
27-27: Type alias matches the new removal target.Locking
DeprecationLoggerNextPlannedVersionto'9.0.0'enforces the cleanup of anything slated for ≤8.x before this release, which is exactly what we want here. 👍
ggazzo
force-pushed
the
chore/break-deprecations
branch
from
7188097 to
ddb885e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/meteor/app/api/server/ApiClass.ts (1)
810-812: Good enforcement strategy; consider enriching the error message.Throwing at registration time (during startup) is the right approach to prevent deprecated endpoints from being used past their removal deadline. This ensures teams can't accidentally deploy code with expired deprecations.
Consider including the deprecation version in the error message for better debugging:
-if (options.deprecation && shouldBreakInVersion(options.deprecation.version)) { - throw new Meteor.Error('error-deprecated', `The endpoint ${route} should be removed`); -} +if (options.deprecation && shouldBreakInVersion(options.deprecation.version)) { + throw new Meteor.Error('error-deprecated', `The endpoint ${route} is deprecated for version ${options.deprecation.version} and should be removed`); +}
📜 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 (2)
apps/meteor/app/api/server/ApiClass.ts(1 hunks)apps/meteor/app/lib/server/lib/deprecationWarningLogger.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/meteor/app/api/server/ApiClass.ts (1)
apps/meteor/app/livechat/server/methods/getAnalyticsChartData.ts (1)
options(18-38)
| }; | ||
|
|
||
| export type DeprecationLoggerNextPlannedVersion = '7.0.0' | '8.0.0'; | ||
| export type DeprecationLoggerNextPlannedVersion = '9.0.0'; |
There was a problem hiding this comment.
🧩 Analysis chain
Verify all call sites updated to use '9.0.0'.
The type has been narrowed from '7.0.0' | '8.0.0' to the single literal '9.0.0', which will cause type errors at any call sites still passing the old versions.
The relevant code snippet shows an example that would break:
// apps/meteor/app/livechat/server/methods/getAnalyticsChartData.ts:18
methodDeprecationLogger.method('livechat:getAnalyticsChartData', '8.0.0', '/v1/livechat/analytics/dashboards/charts-data');Run the following script to find all call sites that may need updating:
🏁 Script executed:
#!/bin/bash
# Description: Find all usages of deprecation loggers with version strings to verify they use '9.0.0'
# Search for apiDeprecationLogger calls with version parameters
echo "=== apiDeprecationLogger usage ==="
rg -nP --type=ts -A2 "apiDeprecationLogger\.(endpoint|parameter|deprecatedParameterUsage)\(" -g '!**/test/**'
echo -e "\n=== methodDeprecationLogger usage ==="
rg -nP --type=ts -A2 "methodDeprecationLogger\.(method|parameter|deprecatedParameterUsage)\(" -g '!**/test/**'
echo -e "\n=== Instances still using old versions ==="
rg -nP --type=ts "('7\.0\.0'|'8\.0\.0')" -g '!**/test/**' -C2Length of output: 23928
Update all deprecation logger call sites from '8.0.0' to '9.0.0'.
The type narrowing is correct but incomplete. Found 26+ active call sites that still pass '8.0.0':
- 18
methodDeprecationLogger.method()calls across livechat, e2e, and canned-responses - 2
apiDeprecationLogger.parameter()calls in parseJsonQuery.ts - 5 API deprecation version fields in route definitions
- 2
shouldBreakInVersion('8.0.0')feature flag calls
All will fail type checking with the narrowed type. Update systematically:
- apps/meteor/app/livechat/server/methods/* (13 files)
- apps/meteor/ee/app/livechat-enterprise/server/methods/* (2 files)
- apps/meteor/ee/app/canned-responses/server/methods/* (2 files)
- apps/meteor/app/api/server/helpers/parseJsonQuery.ts (2 parameter calls)
- apps/meteor/app/lib/server/methods/setAdminStatus.ts
- apps/meteor/app/e2e/server/methods/updateGroupKey.ts
- apps/meteor/app/lib/server/functions/saveUser/saveUser.ts
- apps/meteor/app/api/server/ApiClass.ts
- apps/meteor/app/api/server/v1/users.ts
- apps/meteor/app/api/server/v1/e2e.ts
- ee/app/api-enterprise/server/voip-freeswitch.ts
- ee/app/api-enterprise/server/canned-responses.ts
Co-authored-by: Douglas Fabris <[email protected]>
Co-authored-by: Tasso Evangelista <[email protected]> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
ggazzo
force-pushed
the
chore/break-deprecations
branch
from
923b1fb to
e791e69
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 23:01", "12/02 21:57", "12/03 21:00", "12/04 18:17", "12/05 21:56", "12/08 20:15", "12/09 13:34", "12/09 14:15 (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]
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]
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]
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.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]
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.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13]
line "rocketchat" [0.36, 0.36, 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.35]
Statistics (last 16 days):
ℹ️ About this reportThis report compares Docker image sizes from this build against the
Updated: Tue, 09 Dec 2025 14:15:06 GMT |
ggazzo
force-pushed
the
release-8.0.0
branch
2 times, most recently
from
d5bff46 to
21d6796
Compare
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.