chore(apps): prevent data conversion for events with no listeners#39565
chore(apps): prevent data conversion for events with no listeners#39565
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 @@
## develop #39565 +/- ##
===========================================
- Coverage 70.94% 70.89% -0.06%
===========================================
Files 3199 3203 +4
Lines 113462 113642 +180
Branches 20545 20552 +7
===========================================
+ Hits 80498 80566 +68
- Misses 30911 31017 +106
- Partials 2053 2059 +6
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
The two failing jobs (TypeScript check and Code Lint) both failed due to the turbogha cache server timing out ( |
|
@copilot fix the lint issue And fix the test file apps/meteor/app/apps/server/bridges/router.spec.ts - it wasn't included in the test glob previously, so the error went unnoticed |
... Fixed in 4a4c581:
|
…pListenerBridge.handleEvent() Co-authored-by: d-gubert <1810309+d-gubert@users.noreply.github.com>
Co-authored-by: d-gubert <1810309+d-gubert@users.noreply.github.com>
…Chai in router.spec.ts Co-authored-by: d-gubert <1810309+d-gubert@users.noreply.github.com>
72da7d9 to
94271ae
Compare
Every
triggerEventcall unconditionally converts messages/rooms/users and invokesexecuteListener()even when zero apps are registered for that event, wasting CPU and I/O on every fire.Changes
AppListenerManager.ts— New publichasListeners(event: AppInterface): booleanmethod. Returnstrueif at least one app is registered or the event is blocked (essential-app-disabled case), ensuringEssentialAppDisabledExceptionis still reachable viaexecuteListener.listeners.ts— Early-return guard at the top ofhandleEvent(), before theswitchand any converter calls. Returnsundefined(the correct no-op sentinel across all event return types) when no listeners are present.AppListenerManager.spec.ts— Four new@Test()cases: no listeners →false; afterregisterListeners→true; afterunregisterListeners→false; blocked event with no registered app →true.listeners.spec.ts(new) — Mocha/Chai/Sinon unit tests forAppListenerBridge.handleEvent(): verifies the message converter is never called whenhasListenersreturnsfalse, and is called when it returnstrue.Original prompt
Problem
Currently, whenever
triggerEventis called fromapps/meteor/ee/server/apps/orchestrator.js, the server unconditionally converts all the data (messages, rooms, users, etc.) and callsexecuteListener()in the apps-engine — even when no app is listening for the event. This wastes CPU and I/O resources on every event fire.Goal
Skip all expensive data conversion work in
AppListenerBridge.handleEvent()(and its sub-methods) when there are no apps registered for the given event.Changes Required
1.
packages/apps-engine/src/server/managers/AppListenerManager.tsAdd a new public method
hasListeners(event: AppInterface): booleantoAppListenerManagerthat returnstruewhen at least one app ID is registered for the event or the event is currently blocked (essential-app-disabled case), andfalseotherwise. The blocked check must be included so that the EssentialAppDisabledException is still thrown as expected — if an event is blocked, it still has to reachexecuteListener()to raise the error.2.
apps/meteor/app/apps/server/bridges/listeners.tsModify
AppListenerBridge.handleEvent()to callhasListeners()on theAppListenerManagerbefore doing any data conversion (i.e., before dispatching tomessageEvent,roomEvent,livechatEvent,userEvent,uploadEvent, ordefaultEvent).The early-return value must respect the caller's expectations:
boolean(prevent events likeIPreMessageSentPrevent,IPreRoomCreatePrevent,IPreRoomDeletePrevent,IPreMessageDeletePrevent,IPreMessageUpdatedPrevent): returnfalse(not prevented).IPreMessageSentExtend,IPreMessageSentModify,IPreRoomCreateExtend,IPreRoomCreateModify,IPreMessageUpdatedExtend,IPreMessageUpdatedModify): returnundefined(the caller must handleundefinedas "no modification").void/ fire-and-forget post-events: returnundefined.IPreFileUpload: returnundefined(void).The cleanest implementation is to add the early-exit at the top of
handleEvent, before theswitchstatement:Because all callers that care about a specific return value (e.g., "was the message prevented?") already handle
undefinedas the "nothing happened" sentinel — as evidenced by the existingresult ?? undefinedpatterns inmessageEventandroomEvent— returningundefinedis the correct and safe no-op value across all event types.3. Tests
packages/apps-engine/tests/server/managers/AppListenerManager.spec.tsExtend the existing test fixture to cover
hasListeners():hasListenersreturnsfalsefor an event with no registered apps and no locked apps.hasListenersreturnstrueafterregisterListenersadds an app for that event.hasListenersreturnsfalseafterunregisterListenersremoves the only app.hasListenersreturnstruefor an event that is blocked (locked by a disabled essential app), even with zero registered listeners.Use the existing
alsatiantest style (the same@Test()+Expect()pattern already present in the file).apps/meteor/app/apps/server/bridges/listeners.spec.ts(new file)Create a new unit-test file for
AppListenerBridge. Place it alongside the existinglisteners.tsatapps/meteor/app/apps/server/bridges/listeners.spec.ts. Use the project's standard Mocha/Chai/Sinon test style (consistent with other Meteor server bridge tests in that package).Tests to include:
No listeners → skip conversion entirely and return
undefinedlistenerManager.hasListenersto returnfalse.handleEventwith a message event (e.g.,IPostMessageSent).undefined.Has listeners → proceeds to conversion and execution
listenerManager.hasListenersto returntrue.executeListenerto returnundefined.handleEventwith the same event.hasListenersis not called for UIKit interactions (sanity check is optional) — UIKit interactions are routed byappIddirectly, but the early-return guard still applies.Keep the mocks minimal: use plain objects / stubs; no need for full Rocket.Chat infrastructure.
Files to modify / create
This pull request was created from Copilot chat.
🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.