Skip to content

Commit 0666c8c

Browse files
warksitHome
authored andcommitted
fix(plugins): discovery must not instantiate an imported base class
inspect.getmembers() returns members ALPHABETICALLY, and the name-based detection strategy takes the first class ending in 'Plugin'. Every plugin does 'from plugin_system import PredBatPlugin', so the abstract base sits in the module namespace — and any plugin class sorting after 'PredBatPlugin' gets the base instantiated instead of itself. The plugin then logs as loaded successfully and silently does nothing: no hooks registered, no behaviour. Whether a plugin works depends on the luck of its class name. Hit in the wild with a class named SocKeepPublishPlugin; ColdWeatherPlugin and CurtailmentPlugin sort before the base, so they were unaffected. Fix: require obj.__module__ == plugin_module.__name__, so only classes actually DEFINED in the plugin file are candidates. The two fallback strategies (PREDBAT_PLUGIN marker, initialize_plugin function) are unchanged. Test discovers a temp plugin whose class name sorts after PredBatPlugin and asserts the right class was instantiated. Verified it fails without the fix: ERROR: discovery instantiated PredBatPlugin, expected ZzSortsLastPlugin
1 parent f40fb40 commit 0666c8c

3 files changed

Lines changed: 56 additions & 3 deletions

File tree

‎apps/predbat/plugin_system.py‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -121,9 +121,16 @@ def load_plugin(self, plugin_dir: str, plugin_name: str):
121121

122122
# Plugin discovery fallthrough (try in order of preference):
123123

124-
# 1. MOST PREFERRED: Class name ends with "Plugin"
124+
# 1. MOST PREFERRED: Class name ends with "Plugin" AND is defined in this
125+
# module. inspect.getmembers() returns members ALPHABETICALLY, so without the
126+
# __module__ check a plugin whose class sorts after an imported one silently
127+
# gets the wrong class instantiated: `from plugin_system import PredBatPlugin`
128+
# puts the base class in the namespace, so e.g. SocKeepPublishPlugin (sorting
129+
# after PredBatPlugin) had the abstract base constructed instead — the plugin
130+
# logged as loaded successfully and did nothing. Whether a plugin works should
131+
# not depend on the alphabet.
125132
for name, obj in inspect.getmembers(plugin_module, inspect.isclass):
126-
if name.endswith("Plugin"):
133+
if name.endswith("Plugin") and obj.__module__ == plugin_module.__name__:
127134
try:
128135
self.log(f"Initialising plugin class (name-based): {name}")
129136
plugin_instance = obj(self.base)

‎apps/predbat/tests/test_plugin_startup.py‎

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,12 @@
77
# pylint: disable=consider-using-f-string
88
# pylint: disable=line-too-long
99
# pylint: disable=attribute-defined-outside-init
10+
import os
11+
import shutil
12+
import tempfile
1013
from unittest.mock import MagicMock, patch
1114
from predbat import PredBat
15+
from plugin_system import PluginSystem
1216

1317

1418
def test_plugin_startup_order(my_predbat):
@@ -143,3 +147,44 @@ def register_hooks(self, plugin_system):
143147
print(f"OK: Plugin successfully registered endpoint /test")
144148

145149
return failed
150+
151+
152+
def test_plugin_discovery_skips_imported_base_class(my_predbat):
153+
"""
154+
Discovery must instantiate the class DEFINED in the plugin file, not a class
155+
it merely imports.
156+
157+
inspect.getmembers() returns members alphabetically and the name-based strategy
158+
takes the first class ending in "Plugin". Every plugin does
159+
`from plugin_system import PredBatPlugin`, so any plugin class sorting AFTER
160+
"PredBatPlugin" had the abstract base instantiated instead of itself — the
161+
plugin logged as loaded successfully, registered no hooks, and did nothing.
162+
"""
163+
print("*** Running test: Plugin discovery skips imported base classes")
164+
failed = 0
165+
166+
base = MagicMock()
167+
base.log = MagicMock()
168+
169+
tmpdir = tempfile.mkdtemp()
170+
try:
171+
# Class name deliberately sorts AFTER "PredBatPlugin".
172+
with open(os.path.join(tmpdir, "zz_sorts_last_plugin.py"), "w", encoding="utf-8") as fh:
173+
fh.write("from plugin_system import PredBatPlugin\n\n\nclass ZzSortsLastPlugin(PredBatPlugin):\n pass\n")
174+
175+
plugin_system = PluginSystem(base)
176+
plugin_system.discover_plugins([tmpdir])
177+
loaded = list(plugin_system.plugins.values())
178+
179+
if len(loaded) != 1:
180+
print("ERROR: expected exactly one discovered plugin, got {}".format(len(loaded)))
181+
failed = 1
182+
elif type(loaded[0]).__name__ != "ZzSortsLastPlugin":
183+
print("ERROR: discovery instantiated {}, expected ZzSortsLastPlugin".format(type(loaded[0]).__name__))
184+
failed = 1
185+
else:
186+
print("OK: discovery instantiated the plugin class, not the imported base")
187+
finally:
188+
shutil.rmtree(tmpdir, ignore_errors=True)
189+
190+
return failed

‎apps/predbat/unit_test.py‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,7 @@
4949
from tests.test_nordpool import run_nordpool_test
5050
from tests.test_futurerate_auto import test_futurerate_auto
5151
from tests.test_car_charging_smart import run_car_charging_smart_tests
52-
from tests.test_plugin_startup import test_plugin_startup_order
52+
from tests.test_plugin_startup import test_plugin_startup_order, test_plugin_discovery_skips_imported_base_class
5353
from tests.test_active_flag import test_active_flag
5454
from tests.test_component_health_status import test_component_health_status
5555
from tests.test_optimise_levels import run_optimise_levels_tests
@@ -398,6 +398,7 @@ def main():
398398
("add_now_to_octopus_slot", test_add_now_to_octopus_slot, "Add now to Octopus slot tests", False),
399399
("octopus_slots_change", test_octopus_slots_change, "Octopus slots change-detection signature tests (in-progress re-clock vs genuine change)", False),
400400
("plugin_startup", test_plugin_startup_order, "Plugin startup order tests", False),
401+
("plugin_discovery", test_plugin_discovery_skips_imported_base_class, "Plugin discovery skips imported base classes", False),
401402
("active_flag", test_active_flag, "Active flag cleared on exception tests", False),
402403
("component_health_status", test_component_health_status, "Component errors fail the recorded run status tests", False),
403404
("dynamic_load_car", test_dynamic_load_car_slot_cancellation, "Dynamic load car slot cancellation tests", False),

0 commit comments

Comments
 (0)