feat: refactor ddp method registration into a MethodRegistry#1783
Open
Julusian wants to merge 1 commit into
Open
feat: refactor ddp method registration into a MethodRegistry#1783Julusian wants to merge 1 commit into
Julusian wants to merge 1 commit into
Conversation
Julusian
added
the
Contribution from SuperFly.tv
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (40)
🚧 Files skipped from review as they are similar to previous changes (37)
WalkthroughThis PR replaces import-time Meteor method registration with a registry-driven startup flow. Server API classes are exported, debug methods become exported maps, REST/security wiring reads from the registry, and tests switch to a shared helper for explicit method setup. ChangesMethodRegistry Centralization
Sequence Diagram(s)sequenceDiagram
participant main.ts
participant MethodRegistry
participant methodRegistrations
participant bindRestApiRouter
participant startupVerifyAllMethods
participant Meteor
main.ts->>MethodRegistry: new MethodRegistry()
main.ts->>methodRegistrations: registerAllApiMethods(registry)
methodRegistrations->>MethodRegistry: registerApi(...)
methodRegistrations->>MethodRegistry: registerDebugMethods(...)
main.ts->>MethodRegistry: applyToMeteor()
main.ts->>Meteor: Meteor.startup(...)
main.ts->>bindRestApiRouter: bindRestApiRouter(registry)
bindRestApiRouter->>MethodRegistry: getSignatures()
main.ts->>startupVerifyAllMethods: startupVerifyAllMethods(registry)
startupVerifyAllMethods->>MethodRegistry: getAllMethodNames()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
meteor/server/api/studio/api.ts (1)
149-186: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAdd runtime checks before using method arguments as Mongo selectors.
studioId,configId, and especiallydeviceIdare client-controlled Meteor method arguments; TypeScript types are erased at runtime, and a non-string object can become the selector passed toPeripheralDevices.updateAsync(deviceId, ...).🛡️ Proposed fix
async assignConfigToPeripheralDevice( studioId: StudioId, configId: string, deviceId: PeripheralDeviceId | null ): Promise<void> { + check(studioId, String) + check(configId, String) + check(deviceId, Match.OneOf(String, null)) + assertConnectionHasOneOfPermissions(this.connection, ...PERMISSIONS_FOR_MANAGE_STUDIOS)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@meteor/server/api/studio/api.ts` around lines 149 - 186, The Mongo selectors in assignConfigToPeripheralDevice currently rely on method arguments that are only typed at compile time, so add runtime validation for studioId, configId, and especially deviceId before passing them into PeripheralDevices.updateAsync. Ensure deviceId is verified to be a valid PeripheralDeviceId/string (or null) and reject any non-scalar/object input before using it as the selector, while keeping the existing permission check and unassign/reassign flow in assignConfigToPeripheralDevice.
🧹 Nitpick comments (4)
meteor/server/api/rest/v0/__tests__/rest.test.ts (2)
7-13: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the now-redundant
import '../index'.
createLegacyApiRouteris already imported from'..'(the sameindexmodule) on line 7, so the bare side-effect import on line 13 is a duplicate. With the registry refactor it no longer populatesMeteorMethodSignaturesat import time, so it serves no purpose.♻️ Proposed cleanup
jest.mock('../../../deviceTriggers/observer') - -import '../index' - describe('REST API', () => {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@meteor/server/api/rest/v0/__tests__/rest.test.ts` around lines 7 - 13, The test file has a redundant side-effect import of the same index module already brought in via createLegacyApiRouter, so remove the bare import '../index' from the rest test setup. Keep the existing imports for createLegacyApiRouter and ServerUserActionAPI, and ensure the test still relies only on the explicit module imports needed for the assertions.
21-28: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDouble cast on the registration object can mask a real shape mismatch.
as unknown as AnyMethodApiRegistrationbypasses type checking entirely. Sincesecret/wrapperare optional, a plain object literal typed asMethodApiRegistration<typeof UserActionAPIMethods>(or the matching generic) should normally assign without theas unknownescape hatch; the double cast suggests the enum/class generics don't line up. Worth confirming the direct typing works so this test still catches contract drift.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@meteor/server/api/rest/v0/__tests__/rest.test.ts` around lines 21 - 28, The test setup in methodRegistry.registerApi is using a double cast that bypasses type safety and can hide a mismatch between UserActionAPIMethods and ServerUserActionAPI. Replace the as unknown as AnyMethodApiRegistration escape hatch with the correct concrete registration type for registerApi, using the generic MethodApiRegistration shape if needed, so the object literal is checked directly and the test still fails on contract drift.meteor/server/methodRegistry.ts (1)
115-124: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winPrevent registry mutations after
applyToMeteor().After Line 152, later
registerApi(),registerMethod(), orregisterDebugMethods()calls still changethis.methodsbut do not update Meteor’s registered method table. Guardadd()so the registry and Meteor DDP cannot diverge.Proposed fix
private add(name: string, wrappedInner: MeteorMethod, original: (...args: any[]) => any, secret: boolean): void { + if (this.applied) { + throw new Meteor.Error(500, `MethodRegistry: cannot register "${name}" after applyToMeteor()`) + } if (this.methods.has(name)) { throw new Meteor.Error(500, `MethodRegistry: A method called "${name}" is already registered.`) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@meteor/server/methodRegistry.ts` around lines 115 - 124, Prevent method registry mutations after applyToMeteor() so Meteor’s method table cannot diverge from this.methods. Update add() in MethodRegistry to reject any registerApi(), registerMethod(), or registerDebugMethods() call once applyToMeteor() has already been run, using the existing MethodRegistry state and the add()/applyToMeteor() flow to gate further additions. Keep the duplicate-name check, but add an explicit guard before this.methods.set(...) so late registrations fail fast instead of silently mutating only the local registry.meteor/server/api/rest/v0/index.ts (1)
70-71: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winFail fast when a user-action signature is missing from the registry.
getSignatures()only contains registered non-secret methods, so|| []turns registration/signature drift into a zero-argument/action/*route. Preserve real zero-argument methods by checking forundefined.Proposed guard
for (const [methodName, methodValue] of Object.entries<any>(UserActionAPIMethods)) { - const signature = methodSignatures[methodValue] || [] + const signature = methodSignatures[methodValue] + if (!signature) { + throw new Error(`Missing MethodRegistry signature for UserActionAPIMethods.${methodName} (${methodValue})`) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@meteor/server/api/rest/v0/index.ts` around lines 70 - 71, The signature lookup in the UserActionAPIMethods loop is swallowing missing registry entries by defaulting to an empty array, which can hide registration/signature drift. Update the getSignatures() lookup so it explicitly checks for undefined before building the route signature, preserving real zero-argument methods while failing fast when a methodValue is not registered; use the methodSignatures map and the surrounding route construction logic in meteor/server/api/rest/v0/index.ts to locate the fix.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@meteor/server/api/blueprints/api.ts`:
- Around line 413-415: The Blueprints API wrapper method insertBlueprint is
discarding caller-provided arguments, so the helper never receives the
client-supplied blueprint type/name. Update the insertBlueprint method to accept
and forward the same arguments supported by insertBlueprint(this.connection),
preserving the existing BlueprintApi entry point while passing those values
through to the helper unchanged.
In `@meteor/server/methodRegistry.ts`:
- Around line 65-92: Fail startup when API method registration fails instead of
swallowing errors in registerApi. In MethodRegistry.registerApi, the catch
around getAllClassMethods(orgClass), methodEnum lookup, and this.add() should
rethrow after logging (or otherwise propagate) so applyToMeteor() cannot
complete with a partial registry. Keep the existing logger.error with
stringifyError(e), but ensure missing wire names or duplicate registrations
abort registration of the whole API class and surface the failure through
registerApi/applyToMeteor.
In `@meteor/server/security/securityVerify.ts`:
- Around line 60-63: The startup verification in verifyAllMethods is incorrectly
including developer-only debug methods, which causes false auth failures when
verifyMethod runs without a real connection. Update the method iteration in
verifyAllMethods, using the MethodRegistry entries registered by
registerAllApiMethods, to skip or explicitly special-case playoutDebugMethods
and ingestDebugMethods before calling verifyMethod, while leaving the rest of
the verification flow unchanged.
---
Outside diff comments:
In `@meteor/server/api/studio/api.ts`:
- Around line 149-186: The Mongo selectors in assignConfigToPeripheralDevice
currently rely on method arguments that are only typed at compile time, so add
runtime validation for studioId, configId, and especially deviceId before
passing them into PeripheralDevices.updateAsync. Ensure deviceId is verified to
be a valid PeripheralDeviceId/string (or null) and reject any non-scalar/object
input before using it as the selector, while keeping the existing permission
check and unassign/reassign flow in assignConfigToPeripheralDevice.
---
Nitpick comments:
In `@meteor/server/api/rest/v0/__tests__/rest.test.ts`:
- Around line 7-13: The test file has a redundant side-effect import of the same
index module already brought in via createLegacyApiRouter, so remove the bare
import '../index' from the rest test setup. Keep the existing imports for
createLegacyApiRouter and ServerUserActionAPI, and ensure the test still relies
only on the explicit module imports needed for the assertions.
- Around line 21-28: The test setup in methodRegistry.registerApi is using a
double cast that bypasses type safety and can hide a mismatch between
UserActionAPIMethods and ServerUserActionAPI. Replace the as unknown as
AnyMethodApiRegistration escape hatch with the correct concrete registration
type for registerApi, using the generic MethodApiRegistration shape if needed,
so the object literal is checked directly and the test still fails on contract
drift.
In `@meteor/server/api/rest/v0/index.ts`:
- Around line 70-71: The signature lookup in the UserActionAPIMethods loop is
swallowing missing registry entries by defaulting to an empty array, which can
hide registration/signature drift. Update the getSignatures() lookup so it
explicitly checks for undefined before building the route signature, preserving
real zero-argument methods while failing fast when a methodValue is not
registered; use the methodSignatures map and the surrounding route construction
logic in meteor/server/api/rest/v0/index.ts to locate the fix.
In `@meteor/server/methodRegistry.ts`:
- Around line 115-124: Prevent method registry mutations after applyToMeteor()
so Meteor’s method table cannot diverge from this.methods. Update add() in
MethodRegistry to reject any registerApi(), registerMethod(), or
registerDebugMethods() call once applyToMeteor() has already been run, using the
existing MethodRegistry state and the add()/applyToMeteor() flow to gate further
additions. Keep the duplicate-name check, but add an explicit guard before
this.methods.set(...) so late registrations fail fast instead of silently
mutating only the local registry.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 91ff1d22-6ecc-4b07-9f35-909b74c9860e
⛔ Files ignored due to path filters (1)
meteor/server/__tests__/__snapshots__/methodRegistry.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (40)
meteor/__mocks__/helpers/methods.tsmeteor/server/__tests__/cronjobs.test.tsmeteor/server/__tests__/methodRegistry.test.tsmeteor/server/api/ExternalMessageQueue.tsmeteor/server/api/__tests__/client.test.tsmeteor/server/api/__tests__/externalMessageQueue.test.tsmeteor/server/api/__tests__/peripheralDevice.test.tsmeteor/server/api/__tests__/rundownLayouts.test.tsmeteor/server/api/__tests__/userActions/general.test.tsmeteor/server/api/__tests__/userActions/system.test.tsmeteor/server/api/blueprints/__tests__/api.test.tsmeteor/server/api/blueprints/api.tsmeteor/server/api/client.tsmeteor/server/api/ingest/debug.tsmeteor/server/api/mongo.tsmeteor/server/api/peripheralDevice.tsmeteor/server/api/playout/api.tsmeteor/server/api/playout/debug.tsmeteor/server/api/rest/api.tsmeteor/server/api/rest/v0/__tests__/rest.test.tsmeteor/server/api/rest/v0/index.tsmeteor/server/api/rundown.tsmeteor/server/api/rundownLayouts.tsmeteor/server/api/showStyles.tsmeteor/server/api/snapshot.tsmeteor/server/api/studio/api.tsmeteor/server/api/system.tsmeteor/server/api/triggeredActions.tsmeteor/server/api/user.tsmeteor/server/api/userActions.tsmeteor/server/main.tsmeteor/server/methodRegistrations.tsmeteor/server/methodRegistry.tsmeteor/server/methods.tsmeteor/server/migration/__tests__/migrations.test.tsmeteor/server/migration/api.tsmeteor/server/security/securityVerify.tsmeteor/server/systemStatus/__tests__/api.test.tsmeteor/server/systemStatus/__tests__/systemStatus.test.tsmeteor/server/systemStatus/api.ts
Julusian
force-pushed
the
refactor/ddp-method-registry
branch
from
ebedcbf to
44e9d24
Compare
7 tasks
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.
About the Contributor
This pull request is posted on behalf of Superfly
Type of Contribution
This is a: Code improvement
This is part of a series of PRs aiming to replace the meteor ddp server.
Current Behavior
The registering of DDP/meteor methods is currently a bit hard to follow. Over time it has migrated away from directly calling
Meteor.methods(...)and is instead using a custom wrapper which manipulates the methods before passing them on.Additionally, we also store references or information about these methods for use elsewhere.
All of these mutate global state, which can make it hard to intercept or mock these, or to ensure they are isolated between different tests when needed.
For now, many tests are using a global
MeteorCallto call these methods which has been left to minimise the sprawl of this change. In the future that could change so that each test file constructs a subset of what they need instead.New Behavior
This starts down the path of removing that global state from being spread around, instead storing all this state in a MethodRegistry instance. This then gets passed onto the areas of the code which want to be able to query what methods have been registered, as well as passing those methods on to meteor.
It is worth noting that in the main.ts, there is now a startup sequencing chunk forming. The intention being that all the imports above will eventually be replaced with explicit invocations of startup logic.
Testing
Affected areas
Time Frame
Not urgent, but we would like to get this merged into the in-development release.
Other Information
Status