feat: refactor ddp publication registration into a PublicationRegistry#1785
Open
Julusian wants to merge 2 commits into
Open
feat: refactor ddp publication registration into a PublicationRegistry#1785Julusian wants to merge 2 commits into
Julusian wants to merge 2 commits 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 (2)
📒 Files selected for processing (76)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (73)
WalkthroughIntroduces registry-based Meteor method and publication registration, exports server API classes with explicit ChangesRegistry-based Meteor method and publication registration
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 OpenGrep (1.23.0)meteor/__mocks__/helpers/methods.ts┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.13][ERROR]: unable to find a config; path meteor/server/__tests__/publicationRegistry.test.ts┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.16][ERROR]: unable to find a config; path meteor/server/api/ExternalMessageQueue.ts┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.18][ERROR]: unable to find a config; path
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: 12
🧹 Nitpick comments (1)
meteor/server/api/client.ts (1)
442-449: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep this API method return type explicit.
This exported registry surface still suppresses explicit boundary typing while neighboring API methods declare their
Promise<...>return types.Proposed cleanup
- // eslint-disable-next-line `@typescript-eslint/explicit-module-boundary-types` async callPeripheralDeviceFunction( context: string, deviceId: PeripheralDeviceId, timeoutTime: number | undefined, functionName: string, ...args: any[] - ) { + ): Promise<any> {🤖 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/client.ts` around lines 442 - 449, The exported API method callPeripheralDeviceFunction still relies on explicit-boundary suppression instead of declaring its return type like nearby registry methods. Remove the boundary-typing suppression and add an explicit Promise-based return type on callPeripheralDeviceFunction that matches its resolved value, keeping the registry surface consistently typed with the neighboring API methods.
🤖 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/__tests__/methodRegistry.test.ts`:
- Around line 26-28: The drift guard in the MethodRegistry test is relying on a
hard-coded name prefix to skip debug methods. Update the loop in
methodRegistry.test.ts to use MethodRegistry.isDebugMethod(name) instead of
startsWith('debug'), so the test follows the registry’s source-of-truth debug
detection and stays valid if debug method names change.
In `@meteor/server/api/rest/v0/index.ts`:
- Around line 78-79: The REST route builder is using raw publication signatures
from PublicationRegistry, which still include the synthetic context parameter
and can shift arguments for legacy routes. Update the route-generation logic in
the REST index code that reads methodRegistry.getSignatures() and
publicationRegistry.getSignatures() so publication signatures are
normalized/stripped before route construction, and ensure the customPublish
wrapper shape does not leak into REST parameter expectations. Adjust the route
handling in the related route-building block as well so publications receive
only their real user args in the correct order.
- Around line 23-35: The legacy REST path is now picking up custom publications
that rely on observer cleanup, but legacyRestPublicationContext provides a no-op
onStop, so those callbacks can leak resources while returning empty results.
Update the REST publication lookup in index.ts to exclude custom publications
(or otherwise filter registry entries so only cursor-returning publications are
used) before invoking them with legacyRestPublicationContext, and keep the no-op
context limited to safe legacy cursor-only handlers.
In `@meteor/server/methodRegistry.ts`:
- Around line 119-135: Prevent method registration after applyToMeteor() has
already snapshotted the registry into Meteor.methods(), since later changes to
MethodRegistry become invisible to Meteor. Add a guard in MethodRegistry.add
(and/or the public registration paths registerApi(), registerMethod(), and
registerDebugMethods()) that throws once applyToMeteor() has been called, so no
new entries can be added to this.methods after the snapshot. Use the existing
MethodRegistry state and identifiers like add and applyToMeteor() to enforce the
freeze consistently across all registration entry points.
In `@meteor/server/publicationRegistry.ts`:
- Around line 79-80: The publication registry is storing and exposing
wrapper/internal parameters instead of the real public API signature. Update
PublicationRegistry.register and customPublish() so extractFunctionSignature is
taken from the typed callback/public-facing function signature, not the wrapper
that includes context, and strip registry-only parameters like context before
saving in this.publications. Ensure introspection/REST returns the original
arguments such as deviceId and token rather than context, ...args.
In `@meteor/server/publications/deviceTriggersPreview.ts`:
- Around line 20-31: The custom publication registration in
registerDeviceTriggersPreviewPublications is losing the original argument
signature because customPublish wraps the callback as context plus rest args,
which breaks PublicationRegistry signature extraction for REST routing. Update
customPublish so it preserves and stores the original callback’s parameter list
from the publication handler passed to
registerDeviceTriggersPreviewPublications, rather than the wrapper signature, so
publicationRegistry.getSignatures() reflects the real route params.
In `@meteor/server/publications/mountedTriggers.ts`:
- Around line 21-24: The mounted-triggers publications are being wrapped in a
way that causes signature introspection to see only the generic async wrapper
instead of the real parameters. Update the registration in mountedTriggers.ts so
the wrapper passed to registry.customPublish preserves the original callback
signature for PeripheralDevicePubSub.mountedTriggersForDevice and the related
publication, keeping parameter names like deviceId, deviceIds, and token visible
to getSignatures() and the REST route/doc generation.
In `@meteor/server/publications/partsUI/publication.ts`:
- Around line 190-218: The `registerPartsUIPublications` callback passed to
`PublicationRegistry.customPublish` is being wrapped in an `async (context,
...args)` handler, which hides the original `playlistId` parameter from
signature inspection. Update `customPublish`/`publishUnsafe` so the original
user callback or its extracted signature is preserved separately for
`getSignatures()`, and keep the `MeteorPubSub.uiParts` handler signature intact
for route generation. Ensure the fix is applied in the
`PublicationRegistry.customPublish` flow rather than changing the
`registerPartsUIPublications` logic itself.
In `@meteor/server/publications/peripheralDevice.ts`:
- Around line 25-80: Validate the subscriber-provided identifiers and authorize
before exposing sensitive fields in peripheralDevice publications. In the
anonymous publication handler and peripheralDevicesAndSubDevices, add runtime
validation for every supplied PeripheralDeviceId/StudioId before using them in
selectors or $in queries, and avoid widening access based solely on token
presence. In peripheralDevice.ts, use the existing access-check helper pattern
from checkAccessAndGetPeripheralDevice or equivalent to confirm the token is
valid before removing secretSettings from peripheralDeviceProjection, and ensure
PeripheralDevicePubSub.peripheralDeviceCommands also validates its deviceId
input before querying.
In `@meteor/server/publications/rundown.ts`:
- Around line 238-249: The Mongo selector in the rundown publications is using
client-supplied args directly, so validate those inputs before assigning them
into query filters. Add runtime check() calls for playlistActivationId and
thisRundownId in the relevant publication handlers in rundown.ts, then only use
the validated scalar values when building the selector in the publication logic
(including the related sections in the same file).
In `@meteor/server/publications/rundownPlaylist.ts`:
- Around line 11-52: The publication callbacks in
registerRundownPlaylistPublications are exposing the internal _context parameter
in the generated signatures, which shifts the legacy REST route shape. Remove
_context from the registry.publish callback signatures for
CorelibPubSub.rundownPlaylists and MeteorPubSub.rundownPlaylistForStudio, and
keep the remaining arguments in the same order so extractFunctionSignature()
produces the expected route segments.
In `@meteor/server/publications/timeline.ts`:
- Around line 40-92: The legacy publication routes in
registerTimelinePublications are exposing transport-only parameters because the
wrapper callback signature is being captured instead of the user-facing DDP
args. Update PublicationRegistry.getSignatures() and the handling used by
createLegacyApiRouter() so publish()/customPublish() entries store only the
actual callback arguments, excluding context and pub, and ensure the timeline
publication registrations in registerTimelinePublications still work with the
corrected signature shape.
---
Nitpick comments:
In `@meteor/server/api/client.ts`:
- Around line 442-449: The exported API method callPeripheralDeviceFunction
still relies on explicit-boundary suppression instead of declaring its return
type like nearby registry methods. Remove the boundary-typing suppression and
add an explicit Promise-based return type on callPeripheralDeviceFunction that
matches its resolved value, keeping the registry surface consistently typed with
the neighboring API methods.
🪄 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: 1c9e766f-c71f-41e9-911b-492aeeff4847
⛔ Files ignored due to path filters (2)
meteor/server/__tests__/__snapshots__/methodRegistry.test.ts.snapis excluded by!**/*.snapmeteor/server/__tests__/__snapshots__/publicationRegistry.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (76)
meteor/__mocks__/helpers/methods.tsmeteor/__mocks__/helpers/publications.tsmeteor/server/__tests__/cronjobs.test.tsmeteor/server/__tests__/methodRegistry.test.tsmeteor/server/__tests__/publicationRegistry.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/lib/customPublication/index.tsmeteor/server/lib/customPublication/publish.tsmeteor/server/main.tsmeteor/server/methodRegistrations.tsmeteor/server/methodRegistry.tsmeteor/server/methods.tsmeteor/server/migration/__tests__/migrations.test.tsmeteor/server/migration/api.tsmeteor/server/publicationRegistrations.tsmeteor/server/publicationRegistry.tsmeteor/server/publications/_publications.tsmeteor/server/publications/blueprintUpgradeStatus/publication.tsmeteor/server/publications/buckets.tsmeteor/server/publications/deviceTriggersPreview.tsmeteor/server/publications/externalEventSubscriptions.tsmeteor/server/publications/ingestStatus/publication.tsmeteor/server/publications/lib/lib.tsmeteor/server/publications/mountedTriggers.tsmeteor/server/publications/organization.tsmeteor/server/publications/packageManager/expectedPackages/publication.tsmeteor/server/publications/packageManager/packageContainers.tsmeteor/server/publications/packageManager/playoutContext.tsmeteor/server/publications/partInstancesUI/publication.tsmeteor/server/publications/partsUI/publication.tsmeteor/server/publications/peripheralDevice.tsmeteor/server/publications/peripheralDeviceForDevice.tsmeteor/server/publications/pieceContentStatusUI/bucket/publication.tsmeteor/server/publications/pieceContentStatusUI/rundown/publication.tsmeteor/server/publications/rundown.tsmeteor/server/publications/rundownPlaylist.tsmeteor/server/publications/segmentPartNotesUI/publication.tsmeteor/server/publications/showStyle.tsmeteor/server/publications/showStyleUI.tsmeteor/server/publications/studio.tsmeteor/server/publications/studioUI.tsmeteor/server/publications/system.tsmeteor/server/publications/timeline.tsmeteor/server/publications/translationsBundles.tsmeteor/server/publications/triggeredActionsUI.tsmeteor/server/security/check.tsmeteor/server/security/securityVerify.tsmeteor/server/systemStatus/__tests__/api.test.tsmeteor/server/systemStatus/__tests__/systemStatus.test.tsmeteor/server/systemStatus/api.ts
💤 Files with no reviewable changes (1)
- meteor/server/publications/_publications.ts
Comment on lines
+23
to
+35
| /** | ||
| * A no-op publication context for the legacy REST path: there is no live subscription, the callback is | ||
| * only invoked to obtain its cursor, which is then fetched once. There is no connection, so any | ||
| * publication that requires one will reject (matching the historical behaviour). | ||
| */ | ||
| const legacyRestPublicationContext: PublicationContext = { | ||
| connection: null, | ||
| onStop: () => undefined, | ||
| ready: () => undefined, | ||
| added: () => undefined, | ||
| changed: () => undefined, | ||
| removed: () => undefined, | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Do not invoke custom publications with a no-op lifecycle.
Registry lookup now returns custom publications too; several custom callbacks start observers and rely on onStop cleanup. With legacyRestPublicationContext.onStop as a no-op, REST requests can leak observers while returning [].
Consider exposing only cursor-returning publications here, or have the registry mark custom publications so the legacy REST router can exclude them.
Also applies to: 111-130
🤖 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 23 - 35, The legacy REST
path is now picking up custom publications that rely on observer cleanup, but
legacyRestPublicationContext provides a no-op onStop, so those callbacks can
leak resources while returning empty results. Update the REST publication lookup
in index.ts to exclude custom publications (or otherwise filter registry entries
so only cursor-returning publications are used) before invoking them with
legacyRestPublicationContext, and keep the no-op context limited to safe legacy
cursor-only handlers.
Julusian
force-pushed
the
refactor/ddp-publication-registry
branch
from
7bc803c to
06834fd
Compare
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.
This builds upon #1783
Bug fix / Feature / Code improvement / Documentation improvement / Other (please specify)
New Behavior
This continues the pattern started in #1783, expanding it to cover the publications too
Testing
Affected areas
Time Frame
Other Information
Status