[checkmk-commits] Fix cmk.gui.hooks registration after modularization of GUI

Module: check_mk Branch: master Commit: e20978c05111f4f128ff5d4d7a6e9c38084aaec8 URL: http://git.mathias-kettner.de/git/?p=check_mk.git;a=commit;h=e20978c05111f4… Author: Lars Michelsen <lm(a)mathias-kettner.de> Date: Fri Nov 23 09:09:46 2018 +0100 Fix cmk.gui.hooks registration after modularization of GUI Loading of the legacy plugins dropped all registered GUI hooks while it should only drop hooks registered by other plugins. This has now been cleaned up to make the builtin hooks work again as intended. Change-Id: I0541d64d941dba900dc82fa13aa78eda64e8bab4 --- cmk/gui/hooks.py | 36 ++++++++++++---- cmk/gui/plugins/userdb/hook_auth.py | 8 ++-- cmk/gui/plugins/wato/builtin_attributes.py | 2 +- cmk/gui/wato/mkeventd.py | 2 +- cmk/gui/watolib.py | 11 ++--- tests/unit/cmk/gui/test_gui_hooks.py | 66 ++++++++++++++++++++++++++++++ 6 files changed, 105 insertions(+), 20 deletions(-) diff --git a/cmk/gui/hooks.py b/cmk/gui/hooks.py index bbb5855..bea6bc8 100644 --- a/cmk/gui/hooks.py +++ b/cmk/gui/hooks.py @@ -27,6 +27,7 @@ import sys import StringIO import traceback +from typing import NamedTuple import cmk.gui.config as config import cmk.gui.i18n @@ -45,9 +46,9 @@ def load_plugins(force): if loaded_with_language == cmk.gui.i18n.get_current_language() and not force: return - # Cleanup all registered hooks. They need to be renewed by load_plugins() + # Cleanup all plugin hooks. They need to be renewed by load_plugins() # of the other modules - unregister() + unregister_plugin_hooks() # This must be set after plugin loading to make broken plugins raise # exceptions all the time and not only the first time (when the plugins @@ -55,13 +56,30 @@ def load_plugins(force): loaded_with_language = cmk.gui.i18n.get_current_language() -def unregister(): - global hooks - hooks = {} +def unregister_plugin_hooks(): + old_hooks = hooks.copy() + for name, registered_hooks in old_hooks.items(): + hooks_left = [h for h in registered_hooks if h.is_builtin] + if hooks_left: + hooks[name] = hooks_left + else: + del hooks[name] -def register(name, func): - hooks.setdefault(name, []).append(func) +def register_builtin(name, func): + register(name, func, is_builtin=True) + + +def register_from_plugin(name, func): + register(name, func, is_builtin=False) + + +Hook = NamedTuple("Hook", [("handler", type(lambda: None)), ("is_builtin", bool)]) + + +# Kept public for compatibility with pre 1.6 plugins (is_builtin needs to be optional for pre 1.6) +def register(name, func, is_builtin=False): + hooks.setdefault(name, []).append(Hook(handler=func, is_builtin=is_builtin)) def get(name): @@ -75,10 +93,10 @@ def registered(name): def call(name, *args): n = 0 - for hk in hooks.get(name, []): + for hook in hooks.get(name, []): n += 1 try: - hk(*args) + hook.handler(*args) except Exception as e: if config.debug: txt = StringIO.StringIO() diff --git a/cmk/gui/plugins/userdb/hook_auth.py b/cmk/gui/plugins/userdb/hook_auth.py index 76daffb..172df2c 100644 --- a/cmk/gui/plugins/userdb/hook_auth.py +++ b/cmk/gui/plugins/userdb/hook_auth.py @@ -240,7 +240,7 @@ def create_auth_file(callee, users=None): # TODO: Should we not execute this hook also when folders are modified? -hooks.register('users-saved', lambda users: create_auth_file("users-saved", users)) -hooks.register('roles-saved', lambda x: create_auth_file("roles-saved")) -hooks.register('contactgroups-saved', lambda x: create_auth_file("contactgroups-saved")) -hooks.register('activate-changes', lambda x: create_auth_file("activate-changes")) +hooks.register_builtin('users-saved', lambda users: create_auth_file("users-saved", users)) +hooks.register_builtin('roles-saved', lambda x: create_auth_file("roles-saved")) +hooks.register_builtin('contactgroups-saved', lambda x: create_auth_file("contactgroups-saved")) +hooks.register_builtin('activate-changes', lambda x: create_auth_file("activate-changes")) diff --git a/cmk/gui/plugins/wato/builtin_attributes.py b/cmk/gui/plugins/wato/builtin_attributes.py index 82067e2..ec2f31b 100644 --- a/cmk/gui/plugins/wato/builtin_attributes.py +++ b/cmk/gui/plugins/wato/builtin_attributes.py @@ -249,7 +249,7 @@ def validate_host_parents(host): ) % (parent_name, parent.site_id(), host.site_id())) -hooks.register('validate-host', validate_host_parents) +hooks.register_builtin('validate-host', validate_host_parents) class NetworkScanAttribute(ValueSpecAttribute): diff --git a/cmk/gui/wato/mkeventd.py b/cmk/gui/wato/mkeventd.py index ce28923..12545f7 100644 --- a/cmk/gui/wato/mkeventd.py +++ b/cmk/gui/wato/mkeventd.py @@ -3895,4 +3895,4 @@ define command { }) -hooks.register("pre-activate-changes", mkeventd_update_notifiation_configuration) +hooks.register_builtin("pre-activate-changes", mkeventd_update_notifiation_configuration) diff --git a/cmk/gui/watolib.py b/cmk/gui/watolib.py index fe83244..0136883 100644 --- a/cmk/gui/watolib.py +++ b/cmk/gui/watolib.py @@ -2910,9 +2910,9 @@ class CREHost(WithPermissionsAndAttributes): def validation_errors(self): if hooks.registered('validate-host'): errors = [] - for hook_function in hooks.get('validate-host'): + for hook in hooks.get('validate-host'): try: - hook_function(self) + hook.handler(self) except MKUserError as e: errors.append("%s" % e) return errors @@ -7018,8 +7018,9 @@ class BuiltinHosttagsConfiguration(HosttagsConfiguration): # '----------------------------------------------------------------------' +# TODO: Kept for compatibility with pre-1.6 WATO plugins def register_hook(name, func): - hooks.register(name, func) + hooks.register_from_plugin(name, func) def call_hook_hosts_changed(folder): @@ -10251,9 +10252,9 @@ def validate_all_hosts(hostnames, force_all=False): for name in hostnames: eff = all_hosts[name] errors = [] - for hk in hooks.get('validate-all-hosts'): + for hook in hooks.get('validate-all-hosts'): try: - hk(eff, all_hosts) + hook.handler(eff, all_hosts) except MKUserError as e: errors.append("%s" % e) hosts_errors[name] = errors diff --git a/tests/unit/cmk/gui/test_gui_hooks.py b/tests/unit/cmk/gui/test_gui_hooks.py new file mode 100644 index 0000000..8ab5134 --- /dev/null +++ b/tests/unit/cmk/gui/test_gui_hooks.py @@ -0,0 +1,66 @@ +import pytest + +import cmk.gui.hooks as hooks + + +(a)pytest.fixture(autouse=True) +def reset_hooks(): + hooks.hooks.clear() + + +def test_hook_registration(): + assert hooks.hooks == {} + + # pre 1.6 API + hooks.register("bla", lambda: True) + assert hooks.get("bla")[0].is_builtin == False + + hooks.register_builtin("blub", lambda: True) + hooks.register_from_plugin("blub", lambda: False) + assert hooks.get("blub")[0].is_builtin == True + assert hooks.get("blub")[1].is_builtin == False + + assert hooks.registered("bla") == True + assert hooks.registered("blub") == True + assert hooks.registered("bli") == False + + assert len(hooks.get("bla")) == 1 + assert len(hooks.get("blub")) == 2 + assert len(hooks.get("bli")) == 0 + + +def test_call(mocker): + hook1_mock = mocker.Mock() + hook2_mock = mocker.Mock() + hooks.register("bla", hook1_mock) + hooks.register("blub", hook2_mock) + + hooks.call("bla") + hook1_mock.assert_called_once() + hook2_mock.assert_not_called() + + hooks.call("blub") + hook1_mock.assert_called_once() + hook2_mock.assert_called_once() + + +def test_call_exception_handling(mocker): + hooks.register_builtin("bli", lambda: 1 / 0) + hook3_mock = mocker.Mock() + hooks.register("bli", hook3_mock) + with pytest.raises(ZeroDivisionError, match="integer division"): + hooks.call("bli") + hook3_mock.assert_not_called() + + +def test_builtin_vs_plugin_hooks(): + hooks.register_builtin("bla", lambda: True) + assert hooks.registered("bla") == True + + hooks.register_from_plugin("blub", lambda: True) + assert hooks.registered("blub") == True + + hooks.load_plugins(force=True) + + assert hooks.registered("bla") == True + assert hooks.registered("blub") == False