From b2496dc4e7c280270e7f4288e7c85d443b545fad Mon Sep 17 00:00:00 2001 From: SergeantPanda Date: Tue, 16 Jun 2026 11:25:17 -0500 Subject: [PATCH] refactor(trigger_event): streamline event dispatching and improve plugin action handling - Replaced the plugin listing mechanism with a more efficient handler iteration for event actions. - Enhanced logging to provide clearer insights into the dispatching process and plugin action execution. - Added error handling to ensure that failures in one plugin action do not block subsequent actions. - Updated tests to cover new behavior and ensure proper handling of enabled and disabled plugins. --- apps/connect/tests/test_trigger_event.py | 129 +++++++++++++----- apps/connect/utils.py | 59 +++++--- apps/plugins/loader.py | 42 +++++- .../tests/test_iter_actions_for_event.py | 51 +++++++ .../tests/test_run_action_db_cleanup.py | 17 +++ 5 files changed, 241 insertions(+), 57 deletions(-) create mode 100644 apps/plugins/tests/test_iter_actions_for_event.py diff --git a/apps/connect/tests/test_trigger_event.py b/apps/connect/tests/test_trigger_event.py index 8a1c011d..5977d01c 100644 --- a/apps/connect/tests/test_trigger_event.py +++ b/apps/connect/tests/test_trigger_event.py @@ -35,43 +35,56 @@ def _empty_subscription_chain(): class TriggerEventDispatchTests(SimpleTestCase): - def _run_trigger_event(self, plugins, event_name, payload): + def _run_trigger_event(self, handlers, event_name, payload, enabled_keys=None): pm = MagicMock() - pm.list_plugins.return_value = plugins + pm.iter_actions_for_event.return_value = handlers + if enabled_keys is None: + enabled_keys = [key for key, _ in handlers] + + enabled_qs = MagicMock() + enabled_qs.values_list.return_value = enabled_keys + with patch( "apps.connect.utils.PluginManager.get", return_value=pm ), patch( "apps.connect.utils.EventSubscription.objects.filter", return_value=_empty_subscription_chain(), - ): + ), patch( + "apps.plugins.models.PluginConfig" + ) as mock_cfg: + mock_cfg.objects.filter.return_value = enabled_qs from apps.connect.utils import trigger_event trigger_event(event_name, payload) return pm def test_disabled_plugin_does_not_abort_dispatch_for_later_enabled_plugin(self): - """The original bug: AttributeError on a disabled plugin's debug log - aborted the loop, so any plugin iterated AFTER a disabled one never - received events.""" - plugins = [ - { - "key": "disabled-plugin", - "name": "Disabled Plugin", - "enabled": False, - "actions": [], - }, - { - "key": "enabled-plugin", - "name": "Enabled Plugin", - "enabled": True, - "actions": [ - {"id": "on_event", "events": ["channel_start"]}, - ], - }, + """Enabled handlers still run when other plugins are disabled in DB.""" + handlers = [ + ("enabled-plugin", "on_event"), ] pm = self._run_trigger_event( - plugins, "channel_start", {"channel_name": "TEST"} + handlers, "channel_start", {"channel_name": "TEST"} + ) + + pm.run_action.assert_called_once_with( + "enabled-plugin", + "on_event", + {"event": "channel_start", "payload": {"channel_name": "TEST"}}, + ) + + def test_skips_handlers_for_disabled_plugins(self): + handlers = [ + ("disabled-plugin", "on_event"), + ("enabled-plugin", "on_event"), + ] + + pm = self._run_trigger_event( + handlers, + "channel_start", + {"channel_name": "TEST"}, + enabled_keys=["enabled-plugin"], ) pm.run_action.assert_called_once_with( @@ -81,22 +94,64 @@ class TriggerEventDispatchTests(SimpleTestCase): ) def test_action_without_matching_event_is_not_dispatched(self): - """Sanity check: actions whose `events` list doesn't include the - fired event, or which have no `events` key at all, are skipped.""" - plugins = [ - { - "key": "plugin", - "name": "Plugin", - "enabled": True, - "actions": [ - {"id": "on_event", "events": ["channel_stop"]}, - {"id": "manual_button"}, # no events key - ], - }, - ] - + """When no handlers are registered for the event, run_action is not called.""" pm = self._run_trigger_event( - plugins, "channel_start", {"channel_name": "TEST"} + [], "channel_start", {"channel_name": "TEST"} ) pm.run_action.assert_not_called() + + def test_no_plugin_config_query_when_no_handlers(self): + pm = MagicMock() + pm.iter_actions_for_event.return_value = [] + with patch( + "apps.connect.utils.PluginManager.get", return_value=pm + ), patch( + "apps.connect.utils.EventSubscription.objects.filter", + return_value=_empty_subscription_chain(), + ), patch( + "apps.plugins.models.PluginConfig" + ) as mock_cfg: + from apps.connect.utils import trigger_event + + trigger_event("channel_start", {"channel_name": "TEST"}) + + mock_cfg.objects.filter.assert_not_called() + + def test_plugin_action_failure_does_not_block_sibling_handlers(self): + handlers = [ + ("failing-plugin", "on_event"), + ("working-plugin", "on_event"), + ] + + pm = MagicMock() + pm.iter_actions_for_event.return_value = handlers + pm.run_action.side_effect = [RuntimeError("boom"), {"status": "ok"}] + + enabled_qs = MagicMock() + enabled_qs.values_list.return_value = ["failing-plugin", "working-plugin"] + + with patch( + "apps.connect.utils.PluginManager.get", return_value=pm + ), patch( + "apps.connect.utils.EventSubscription.objects.filter", + return_value=_empty_subscription_chain(), + ), patch( + "apps.plugins.models.PluginConfig" + ) as mock_cfg: + mock_cfg.objects.filter.return_value = enabled_qs + from apps.connect.utils import trigger_event + + trigger_event("channel_start", {"channel_name": "TEST"}) + + self.assertEqual(pm.run_action.call_count, 2) + pm.run_action.assert_any_call( + "failing-plugin", + "on_event", + {"event": "channel_start", "payload": {"channel_name": "TEST"}}, + ) + pm.run_action.assert_any_call( + "working-plugin", + "on_event", + {"event": "channel_start", "payload": {"channel_name": "TEST"}}, + ) diff --git a/apps/connect/utils.py b/apps/connect/utils.py index b71c38e5..d8ef59b7 100644 --- a/apps/connect/utils.py +++ b/apps/connect/utils.py @@ -1,5 +1,5 @@ # connect/utils.py -import logging, json +import logging from django.template import Template, Context from .models import EventSubscription, DeliveryLog, SUPPORTED_EVENTS from .handlers.webhook import WebhookHandler @@ -94,23 +94,44 @@ def trigger_event(event_name, payload): pm = PluginManager.get() pm.discover_plugins(sync_db=False, use_cache=True) - plugins = pm.list_plugins() + handlers = list(pm.iter_actions_for_event(event_name)) + if not handlers: + return - logger.debug(f"Checking {len(plugins)} plugins for event '{event_name}'") - for plugin in plugins: - if not plugin["enabled"]: - logger.debug(f"Skipping disabled plugin id={plugin['key']} name={plugin['name']}") + from apps.plugins.models import PluginConfig + + handler_keys = {key for key, _ in handlers} + enabled_keys = set( + PluginConfig.objects.filter(enabled=True, key__in=handler_keys).values_list( + "key", flat=True + ) + ) + + logger.debug( + "Dispatching event '%s' to %d plugin action(s) (%d enabled)", + event_name, + len(handlers), + len(enabled_keys), + ) + params = {"event": event_name, "payload": payload} + for key, action_id in handlers: + if key not in enabled_keys: + logger.debug( + "Skipping disabled plugin id=%s for event '%s'", key, event_name + ) continue - - logger.debug(json.dumps(plugin)) - for action in plugin["actions"]: - if "events" in action and event_name in action["events"]: - key = plugin["key"] - params = {"event": event_name, "payload": payload} - action_name = action.get("label") or action.get("id") - action_id = action.get("id") - logger.debug( - f"Triggering plugin action for event '{event_name}' on plugin id={key} action={action_name}" - ) - if action_id: - pm.run_action(key, action_id, params) + logger.debug( + "Triggering plugin action for event '%s' on plugin id=%s action=%s", + event_name, + key, + action_id, + ) + try: + pm.run_action(key, action_id, params) + except Exception: + logger.exception( + "Plugin action failed for event '%s' on plugin id=%s action=%s", + event_name, + key, + action_id, + ) diff --git a/apps/plugins/loader.py b/apps/plugins/loader.py index 3c275f69..d49da0f8 100644 --- a/apps/plugins/loader.py +++ b/apps/plugins/loader.py @@ -8,7 +8,7 @@ import sys import threading import types from dataclasses import dataclass, field -from typing import Any, Dict, List, Optional +from typing import Any, Dict, Iterator, List, Optional, Tuple from django.db import close_old_connections, transaction @@ -92,6 +92,29 @@ class PluginManager: key: lp.path for key, lp in self._registry.items() if lp and lp.path } + try: + return self._discover_plugins_impl( + sync_db=sync_db, + force_reload=force_reload, + previous_packages=previous_packages, + previous_aliases=previous_aliases, + previous_paths=previous_paths, + token=token, + ) + finally: + # Discovery runs outside Django's request/task cycle (boot, worker_ready). + close_old_connections() + + def _discover_plugins_impl( + self, + *, + sync_db: bool, + force_reload: bool, + previous_packages: Dict[str, str], + previous_aliases: Dict[str, str], + previous_paths: Dict[str, str], + token: int, + ) -> Dict[str, LoadedPlugin]: try: configs: Optional[Dict[str, PluginConfig]] = None try: @@ -247,6 +270,23 @@ class PluginManager: logger.exception("Deferring plugin DB sync; database not ready yet") return self._registry + def iter_actions_for_event(self, event_name: str) -> Iterator[Tuple[str, str]]: + """Yield (plugin_key, action_id) pairs from the in-memory registry.""" + with self._lock: + registry = list(self._registry.items()) + for key, lp in registry: + for action in lp.actions or []: + if not isinstance(action, dict): + continue + action_id = action.get("id") + events = action.get("events") + if ( + action_id + and isinstance(events, (list, tuple)) + and event_name in events + ): + yield key, action_id + def _load_plugin( self, key: str, diff --git a/apps/plugins/tests/test_iter_actions_for_event.py b/apps/plugins/tests/test_iter_actions_for_event.py new file mode 100644 index 00000000..05888157 --- /dev/null +++ b/apps/plugins/tests/test_iter_actions_for_event.py @@ -0,0 +1,51 @@ +from django.test import SimpleTestCase + +from apps.plugins.loader import LoadedPlugin, PluginManager + + +class IterActionsForEventTests(SimpleTestCase): + def test_yields_matching_handlers(self): + pm = PluginManager() + pm._registry = { + "alpha": LoadedPlugin( + key="alpha", + name="Alpha", + actions=[ + {"id": "on_start", "events": ["channel_start"]}, + {"id": "manual"}, + ], + ), + "beta": LoadedPlugin( + key="beta", + name="Beta", + actions=[ + {"id": "on_both", "events": ["channel_start", "channel_stop"]}, + ], + ), + } + + self.assertEqual( + list(pm.iter_actions_for_event("channel_start")), + [("alpha", "on_start"), ("beta", "on_both")], + ) + self.assertEqual(list(pm.iter_actions_for_event("channel_stop")), [("beta", "on_both")]) + + def test_ignores_string_events_value(self): + pm = PluginManager() + pm._registry = { + "bad": LoadedPlugin( + key="bad", + name="Bad", + actions=[{"id": "hook", "events": "client_connect"}], + ), + "good": LoadedPlugin( + key="good", + name="Good", + actions=[{"id": "hook", "events": ["client_connect"]}], + ), + } + + self.assertEqual( + list(pm.iter_actions_for_event("client_connect")), + [("good", "hook")], + ) diff --git a/apps/plugins/tests/test_run_action_db_cleanup.py b/apps/plugins/tests/test_run_action_db_cleanup.py index d9a45c88..6313473d 100644 --- a/apps/plugins/tests/test_run_action_db_cleanup.py +++ b/apps/plugins/tests/test_run_action_db_cleanup.py @@ -62,3 +62,20 @@ class PluginRunActionDbCleanupTests(SimpleTestCase): self.assertTrue(pm.stop_plugin("test_plugin", reason="shutdown")) mock_close.assert_called_once() + + +class PluginDiscoverDbCleanupTests(SimpleTestCase): + @patch("apps.plugins.loader.close_old_connections") + @patch.object(PluginManager, "_discover_plugins_impl", return_value={}) + def test_discover_plugins_closes_connections(self, _mock_impl, mock_close): + pm = PluginManager() + pm.discover_plugins(sync_db=False) + mock_close.assert_called_once() + + @patch("apps.plugins.loader.close_old_connections") + def test_discover_plugins_cache_hit_skips_close(self, mock_close): + pm = PluginManager() + pm._discovery_completed = True + pm._last_reload_token = pm._get_reload_token() + pm.discover_plugins(sync_db=False, use_cache=True) + mock_close.assert_not_called()