chore: log deprecation for unused DDP methods without a REST replacement#40657
Draft
ggazzo wants to merge 9 commits into
Draft
chore: log deprecation for unused DDP methods without a REST replacement#40657ggazzo wants to merge 9 commits into
ggazzo wants to merge 9 commits into
Conversation
scripts/audit-ddp-methods.mjs walks the monorepo to enumerate every
Meteor.methods registration, find their callers across server/client/
ee, and cross-reference against REST endpoints registered in
apps/meteor/app/api/server. Produces docs/ddp-audit.{json,md} with
three buckets: used methods, orphans (no caller), and used methods
without a REST replacement.
scripts/add-ddp-deprecation.mjs consumes that audit and inserts
methodDeprecationLogger.method(name, '9.0.0', replacement) into the
body of every orphan and every used-method-with-REST-replacement,
adding the import in the relative-imports group. Skips methods that
already log a deprecation. Supports shorthand, colon-function, arrow,
and twoFactorRequired-wrapped registration shapes.
Adds methodDeprecationLogger.method() calls to 148 Meteor methods that are either orphan (no caller found in the repo) or already have a REST endpoint replacement. Methods with the call now warn that they will be removed in 9.0.0, and when applicable point to the replacement endpoint. Methods still in use without a REST equivalent are intentionally left untouched until a replacement endpoint exists. Already-deprecated methods (sendFileMessage, starMessage, insertOrUpdateSound, uploadCustomSound) are kept as-is. Generated by scripts/add-ddp-deprecation.mjs from docs/ddp-audit.json.
The audit script picked up 'settings' and 'context' as method names from inside an object-literal return value and a JSDoc `@param` block. Those two methodDeprecationLogger calls were injected into the bodies of rocketchatSearch.getProvider and rocketchatSearch.search by mistake. Neither method is a deprecation target.
Initial pass shipped 18 deprecation-message replacements that pointed
developers at REST endpoints with materially different semantics or at
paths that did not exist. Independent review classified them as WRONG,
MISSING, or PARTIAL with silent-break risk:
WRONG (drop replacement, keep deprecation)
- 2fa:enable (TOTP enable vs email-2fa enable)
- 2fa:disable (TOTP disable vs send email code)
- deleteFileMessage (single-msg vs rooms.cleanHistory range purge)
- getMessages (batched IDs vs single chat.getMessage)
- loadNextMessages, loadSurroundingMessages
(forward/window fetch vs chat.syncMessages delta)
- readThreads (one thread vs whole-room read)
- getSetupWizardParameters (wizard subset vs settings.public)
- banner/dismiss (per-user set vs Banner.dismiss broadcast)
MISSING
- saveSettings (/v1/settings does not exist; only /v1/settings/:_id)
PARTIAL (room-type or shape mismatch — silent break for callers)
- loadHistory, joinRoom, leaveRoom, addUsersToRoom,
getRoomByTypeAndName (channels.* fails on p/d/l rooms)
- loadMissedMessages (chat.syncMessages has incompatible response)
- spotlight, slashCommand (REST drops required params)
For all 18, the replacement argument is now an empty array so the
deprecation logger still reports the removal version without pointing
to a wrong endpoint.
Also fills four mappings that the previous heuristic missed because of
case/separator differences or an unscanned EE api directory:
- autoTranslate.getSupportedLanguages
-> /v1/autotranslate.getSupportedLanguages
- autoTranslate.translateMessage
-> /v1/autotranslate.translateMessage
- subscriptions/get
-> /v1/subscriptions.get
- getReadReceipts (EE)
-> /v1/chat.getMessageReadReceipts
scripts/audit-ddp-methods.mjs gains:
- REST_DENYLIST that hard-blocks the 18 misleading mappings from being
re-derived if the heuristic runs again,
- lower-case and `/`-vs-`.` separator candidate paths so similarly
misspelled mappings get matched,
- apps/meteor/ee/server/api/ added to REST_DIRS so EE endpoints are
visible to the audit.
docs/ddp-replacement-review.md captures the per-method review notes.
Contributor
|
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 |
|
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #40657 +/- ##
===========================================
- Coverage 69.64% 69.63% -0.01%
===========================================
Files 3338 3338
Lines 123232 123237 +5
Branches 21989 21974 -15
===========================================
- Hits 85830 85822 -8
- Misses 34049 34057 +8
- Partials 3353 3358 +5
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Adds a JSDoc `@deprecated` block above every Meteor.methods({...})
entry that the audit flagged as an orphan (no caller found in the
repository). The message also explains the rationale — these methods
remain registered for external DDP consumers (mobile SDK, custom
integrations) but should not be picked up as new dependencies inside
the monorepo.
110 entries annotated across 101 files. 2 entries already had a
`@deprecated` block from earlier and were left untouched.
scripts/add-orphan-jsdoc.mjs reads docs/ddp-audit.json and walks every
`Meteor.methods({...})` body (skipping the `declare module
'@rocket.chat/ddp-client'` interface blocks above) so the comment
lands on the implementation, not the type augmentation.
The deprecation logger throws in TEST_MODE
(apps/meteor/app/lib/server/lib/deprecationWarningLogger.ts:118), so a
deprecation log on a method that's still invoked by integration or
unit tests turns those tests into failures with messages like:
Error: The method "addUsersToRoom" is deprecated and will be
removed on version 9.0.0
That's the intended dev-loop guard rail, but it means this PR can
only carry deprecation calls on methods with **no callers**.
Removes the 83 logger calls (and their now-unused imports) that
targeted methods listed in audit.used. They will be revisited as part
of the caller-migration PR #40659 — once each callsite moves to the
matching REST endpoint, the DDP method can either get a deprecation
log here or be removed outright in #40656.
What remains in this PR: 71 deprecation calls, all on orphan methods
without a REST replacement.
ggazzo
changed the title
2 tasks
The deprecation logger throws under TEST_MODE so accidental usage of a
deprecated method or endpoint surfaces immediately. That guard rail is
correct for unit tests, but the API end-to-end suite intentionally
exercises DDP methods through the `/v1/method.call/:method` proxy and
through streamer paths to keep coverage on those code paths until the
methods are actually removed.
Introduces a second env var, `TEST_MODE_API`, that suppresses the throw
while keeping the warn log:
- apps/meteor/app/lib/server/lib/deprecationWarningLogger.ts: a
`shouldThrowOnDeprecation()` helper replaces the seven inline
`if (process.env.TEST_MODE === 'true') throw` blocks, gated on
both TEST_MODE being true and TEST_MODE_API NOT being true.
- docker-compose-ci.yml: the rocketchat and omnichannel-transcript
services now pass `TEST_MODE_API` through to the container so the
Node process sees it.
- .github/workflows/ci-test-e2e.yml: the `E2E Test API` and
`E2E Test API (Livechat)` jobs set `TEST_MODE_API: 'true'` so
the suppression only kicks in for those runs.
UI and Unit test runs still throw on accidental deprecated usage.
The previous patch set TEST_MODE_API on the `E2E Test API` step, which runs *after* docker compose has already started the rocketchat container — so the env var never reached the Node process. Move the export to the `Start containers` steps (CE + EE) gated on `inputs.type == 'api' || inputs.type == 'api-livechat'` so the var exists in the shell when docker compose substitutes it into the container's environment from `docker-compose-ci.yml`.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds
methodDeprecationLogger.method(name, '9.0.0', [])to every orphan Meteor (DDP) method that does not have a REST endpoint to migrate callers to. The log line warns external clients that the method will be removed in 9.0.0 without pointing at a substitute.Companion PR layout:
/v1/...).release-9.0.0.useEndpoint/sdk.rest.*.Used methods without a REST replacement are not in this PR. The deprecation logger throws under
TEST_MODE, so deprecating a live method breaks the test suite. Those methods either need a new REST endpoint first or stay registered until 9.0.0 cleanup.Scope
71 deprecation calls across 62 files. Each call sits above a
// @deprecatedJSDoc on the implementation so editors strike through the method name. Files already had their imports pruned and were not deleted (the implementations still ship — only the log line is new).Caveats
useMethod(value)(adminMethodActionInput.tsx) or the/v1/method.call/:methodREST proxy can look orphan but are still reachable. Their entries are on the skip list and not deprecated here.Test plan