From 3d1b261c8abc45013304fa34fe848f5097a43ef6 Mon Sep 17 00:00:00 2001 From: Lars Michelsen Date: Mon, 13 Jan 2025 17:39:47 +0100 Subject: [PATCH] Do not modify the global sorter_registry with config based tag sorters The registries are global states and must not be modified by the request processing logic. Doing so can lead to inconsistent situations and lead to race conditions in threaded environments. Change-Id: I042a828fa716d76ce3d35c965a90493bcf7bb6c3 --- cmk/gui/view.py | 27 ++++++--- cmk/gui/views/host_tag_plugins.py | 34 ----------- cmk/gui/views/page_edit_view.py | 10 ++-- cmk/gui/views/sort_url.py | 25 ++++---- cmk/gui/views/sorter/__init__.py | 2 + cmk/gui/views/sorter/host_tag_sorters.py | 58 +++++++++++++++++++ cmk/gui/views/sorter/registry.py | 7 ++- .../gui/views/sorter/test_host_tag_sorters.py | 11 +--- 8 files changed, 107 insertions(+), 67 deletions(-) create mode 100644 cmk/gui/views/sorter/host_tag_sorters.py diff --git a/cmk/gui/view.py b/cmk/gui/view.py index d80ed4b6d91..bc6d0668ac8 100644 --- a/cmk/gui/view.py +++ b/cmk/gui/view.py @@ -3,7 +3,7 @@ # This file is part of Checkmk (https://checkmk.com). It is subject to the terms and # conditions defined in the file COPYING, which is part of this source code package. -from collections.abc import Iterable, Sequence +from collections.abc import Iterable, Mapping, Sequence from livestatus import SiteId @@ -12,6 +12,7 @@ from cmk.gui import pagetypes, visuals from cmk.gui.breadcrumb import Breadcrumb, BreadcrumbItem, make_topic_breadcrumb +from cmk.gui.config import active_config from cmk.gui.data_source import ABCDataSource, data_source_registry from cmk.gui.display_options import display_options from cmk.gui.exceptions import MKUserError @@ -33,7 +34,7 @@ from cmk.gui.view_breadcrumbs import make_host_breadcrumb, make_service_breadcrumb from cmk.gui.views.layout import Layout, layout_registry from cmk.gui.views.sort_url import compute_sort_url_parameter -from cmk.gui.views.sorter import sorter_registry, SorterEntry +from cmk.gui.views.sorter import all_sorters, Sorter, SorterEntry from cmk.gui.visuals import get_missing_single_infos_group_aware, view_title @@ -79,14 +80,15 @@ def datasource(self) -> ABCDataSource: def row_cells(self) -> list[Cell]: """Regular cells are displaying information about the rows of the type the view is about""" cells: list[Cell] = [] + registered_sorters = all_sorters(active_config) for e in self.spec["painters"]: if not painter_exists(e): continue if (col_type := e.column_type) in ["join_column", "join_inv_column"]: - cells.append(JoinCell(e, self._compute_sort_url_parameter(e))) + cells.append(JoinCell(e, self._compute_sort_url_parameter(e, registered_sorters))) elif col_type == "column": - cells.append(Cell(e, self._compute_sort_url_parameter(e))) + cells.append(Cell(e, self._compute_sort_url_parameter(e, registered_sorters))) else: raise NotImplementedError() @@ -95,8 +97,9 @@ def row_cells(self) -> list[Cell]: @property def group_cells(self) -> list[Cell]: """Group cells are displayed as titles of grouped rows""" + registered_sorters = all_sorters(active_config) return [ - Cell(e, self._compute_sort_url_parameter(e)) + Cell(e, self._compute_sort_url_parameter(e, registered_sorters)) for e in self.spec["group_painters"] if painter_exists(e) ] @@ -109,11 +112,14 @@ def join_cells(self) -> list[JoinCell]: @property def sorters(self) -> list[SorterEntry]: """Returns the list of effective sorters to be used to sort the rows of this view""" + registered_sorters = all_sorters(active_config) return self._get_sorter_entries( - self.user_sorters if self.user_sorters else self.spec["sorters"] + self.user_sorters if self.user_sorters else self.spec["sorters"], registered_sorters ) - def _compute_sort_url_parameter(self, painter: ColumnSpec) -> str | None: + def _compute_sort_url_parameter( + self, painter: ColumnSpec, registered_sorters: Mapping[str, Sorter] + ) -> str | None: if not self.spec.get("user_sortable", False): return None @@ -124,13 +130,16 @@ def _compute_sort_url_parameter(self, painter: ColumnSpec) -> str | None: self.spec["group_painters"], self.spec["sorters"], self._user_sorters or [], + registered_sorters, ) - def _get_sorter_entries(self, sorter_list: Iterable[SorterSpec]) -> list[SorterEntry]: + def _get_sorter_entries( + self, sorter_list: Iterable[SorterSpec], registered_sorters: Mapping[str, Sorter] + ) -> list[SorterEntry]: sorters: list[SorterEntry] = [] for entry in sorter_list: sorter_spec = entry.sorter - sorter = sorter_registry.get( + sorter = registered_sorters.get( sorter_spec[0] if isinstance(sorter_spec, tuple) else sorter_spec, None ) if sorter is None: diff --git a/cmk/gui/views/host_tag_plugins.py b/cmk/gui/views/host_tag_plugins.py index 7fe4950eb6f..48d57311f89 100644 --- a/cmk/gui/views/host_tag_plugins.py +++ b/cmk/gui/views/host_tag_plugins.py @@ -7,21 +7,15 @@ """Dynamic registration of host tag painters and sorters based on the site configuration""" -from collections.abc import Mapping -from functools import partial - from cmk.utils.tags import TagGroupID from cmk.gui.config import active_config, Config -from cmk.gui.http import Request from cmk.gui.i18n import _ from cmk.gui.painter.v0.base import Painter, painter_registry from cmk.gui.painter.v0.helpers import get_tag_groups from cmk.gui.type_defs import Row from cmk.gui.view_utils import CellSpec -from .sorter import Sorter, sorter_registry - def register_tag_plugins() -> None: if getattr(register_tag_plugins, "_config_hash", None) == _calc_config_hash( @@ -29,7 +23,6 @@ def register_tag_plugins() -> None: ): return # No re-register needed :-) _register_host_tag_painters(config=active_config) - _register_host_tag_sorters(config=active_config) setattr(register_tag_plugins, "_config_hash", _calc_config_hash(config=active_config)) @@ -83,33 +76,6 @@ def _paint_host_tag(row: Row, tgid: TagGroupID, *, config: Config) -> CellSpec: return "", _get_tag_group_value(row, "host", tgid, config=config) -def _register_host_tag_sorters(*, config: Config) -> None: - for tag_group in config.tags.tag_groups: - sorter_registry.register( - Sorter( - ident=f"host_tag_{tag_group.id}", - title=_("Host tag:") + " " + tag_group.title, - columns=["host_tags"], - load_inv=False, - sort_function=partial(_cmp_host_tag, tag_group_id=tag_group.id), - ) - ) - - -def _cmp_host_tag( - r1: Row, - r2: Row, - *, - parameters: Mapping[str, object] | None, - config: Config, - request: Request, - tag_group_id: TagGroupID, -) -> int: - host_tag_1 = _get_tag_group_value(r1, "host", tag_group_id, config=config) - host_tag_2 = _get_tag_group_value(r2, "host", tag_group_id, config=config) - return (host_tag_1 > host_tag_2) - (host_tag_1 < host_tag_2) - - def _get_tag_group_value(row: Row, what: str, tag_group_id: TagGroupID, *, config: Config) -> str: tag_id = get_tag_groups(row, what).get(tag_group_id) diff --git a/cmk/gui/views/page_edit_view.py b/cmk/gui/views/page_edit_view.py index 1f534bb9a89..ff7d7fc6c7a 100644 --- a/cmk/gui/views/page_edit_view.py +++ b/cmk/gui/views/page_edit_view.py @@ -66,7 +66,7 @@ from cmk.gui.visuals.type import visual_type_registry from .layout import layout_registry -from .sorter import ParameterizedSorter, Sorter, sorter_registry, SorterRegistry +from .sorter import all_sorters, ParameterizedSorter, Sorter from .store import get_all_views from .view_choices import view_choices @@ -968,7 +968,7 @@ def infos_needed_by_plugin(plugin: Painter | Sorter, add_columns: list | None = def sorters_of_datasource(ds_name: str) -> Mapping[str, Sorter]: - return _allowed_for_datasource(sorter_registry, ds_name) + return _allowed_for_datasource(all_sorters(active_config), ds_name) def painters_of_datasource(ds_name: str) -> Mapping[str, Painter]: @@ -998,13 +998,15 @@ def _allowed_for_datasource(collection: PainterRegistry, ds_name: str) -> Mappin @overload -def _allowed_for_datasource(collection: SorterRegistry, ds_name: str) -> Mapping[str, Sorter]: ... +def _allowed_for_datasource( + collection: Mapping[str, Sorter], ds_name: str +) -> Mapping[str, Sorter]: ... # Filters a list of sorters or painters and decides which of # those are available for a certain data source def _allowed_for_datasource( - collection: PainterRegistry | SorterRegistry, + collection: PainterRegistry | Mapping[str, Sorter], ds_name: str, ) -> Mapping[str, Sorter | Painter]: datasource: ABCDataSource = data_source_registry[ds_name]() diff --git a/cmk/gui/views/sort_url.py b/cmk/gui/views/sort_url.py index 0cb1b43a5f8..360b49927a0 100644 --- a/cmk/gui/views/sort_url.py +++ b/cmk/gui/views/sort_url.py @@ -3,7 +3,7 @@ # This file is part of Checkmk (https://checkmk.com). It is subject to the terms and # conditions defined in the file COPYING, which is part of this source code package. -from collections.abc import Iterable, Sequence +from collections.abc import Iterable, Mapping, Sequence from cmk.gui.config import active_config from cmk.gui.display_options import display_options @@ -15,7 +15,7 @@ from cmk.gui.theme.current_theme import theme from cmk.gui.type_defs import ColumnSpec, PainterName, PainterParameters, SorterName, SorterSpec -from .sorter import ParameterizedSorter, sorter_registry +from .sorter import ParameterizedSorter, Sorter def compute_sort_url_parameter( @@ -25,6 +25,7 @@ def compute_sort_url_parameter( group_painters: Sequence[ColumnSpec], config_sorters: Sequence[SorterSpec], user_sorters: Sequence[SorterSpec], + registered_sorters: Mapping[str, Sorter], ) -> str: """Computes the `sort` URL parameter value for a column header @@ -37,7 +38,7 @@ def compute_sort_url_parameter( sorters = [] group_sort, user_sort, view_sort = _get_separated_sorters( - group_painters, config_sorters, list(user_sorters) + group_painters, config_sorters, list(user_sorters), registered_sorters ) sorters = group_sort + user_sort + view_sort @@ -46,16 +47,16 @@ def compute_sort_url_parameter( # - Negate/Disable when at first position # - Move to the first position when already in sorters # - Add in the front of the user sorters when not set - sorter_name = _get_sorter_name_of_painter(painter_name) + sorter_name = _get_sorter_name_of_painter(painter_name, registered_sorters) if sorter_name is None: # Do not change anything in case there is no sorter for the current column return _encode_sorter_url(sorters) - if sorter_name not in sorter_registry: + if sorter_name not in registered_sorters: return _encode_sorter_url(sorters) sorter: SorterName | tuple[SorterName, PainterParameters] - if isinstance(sorter_registry[sorter_name], ParameterizedSorter): + if isinstance(registered_sorters[sorter_name], ParameterizedSorter): assert painter_parameters is not None sorter = (painter_name, painter_parameters) else: @@ -90,8 +91,9 @@ def _get_separated_sorters( group_painters: Sequence[ColumnSpec], config_sorters: Sequence[SorterSpec], user_sorters: list[SorterSpec], + registered_sorters: Mapping[str, Sorter], ) -> tuple[list[SorterSpec], list[SorterSpec], list[SorterSpec]]: - group_sort = _get_group_sorters(group_painters) + group_sort = _get_group_sorters(group_painters, registered_sorters) view_sort = [s for s in config_sorters if not any(s.sorter == gs.sorter for gs in group_sort)] user_sort = user_sorters @@ -101,12 +103,14 @@ def _get_separated_sorters( return group_sort, user_sort, view_sort -def _get_group_sorters(group_painters: Sequence[ColumnSpec]) -> list[SorterSpec]: +def _get_group_sorters( + group_painters: Sequence[ColumnSpec], registered_sorters: Mapping[str, Sorter] +) -> list[SorterSpec]: group_sort: list[SorterSpec] = [] for p in group_painters: if not painter_exists(p): continue - sorter_name = _get_sorter_name_of_painter(p) + sorter_name = _get_sorter_name_of_painter(p, registered_sorters) if sorter_name is None: continue @@ -116,6 +120,7 @@ def _get_group_sorters(group_painters: Sequence[ColumnSpec]) -> list[SorterSpec] def _get_sorter_name_of_painter( painter_name_or_spec: PainterName | ColumnSpec, + registered_sorters: Mapping[str, Sorter], ) -> SorterName | None: painter_name = ( painter_name_or_spec.name @@ -133,7 +138,7 @@ def _get_sorter_name_of_painter( if painter.sorter: return painter.sorter - if painter_name in sorter_registry: + if painter_name in registered_sorters: return painter_name return None diff --git a/cmk/gui/views/sorter/__init__.py b/cmk/gui/views/sorter/__init__.py index 02098c91c6a..eae917fd66f 100644 --- a/cmk/gui/views/sorter/__init__.py +++ b/cmk/gui/views/sorter/__init__.py @@ -16,6 +16,7 @@ compare_ips, ) from .registry import ( + all_sorters, declare_1to1_sorter, declare_simple_sorter, register_sorter, @@ -30,6 +31,7 @@ "ParameterizedSorter", "SorterEntry", "SorterRegistry", + "all_sorters", "cmp_custom_variable", "cmp_insensitive_string", "cmp_ip_address", diff --git a/cmk/gui/views/sorter/host_tag_sorters.py b/cmk/gui/views/sorter/host_tag_sorters.py new file mode 100644 index 00000000000..0f5ea3f7a46 --- /dev/null +++ b/cmk/gui/views/sorter/host_tag_sorters.py @@ -0,0 +1,58 @@ +#!/usr/bin/env python3 +# Copyright (C) 2019 Checkmk GmbH - License: GNU General Public License v2 +# This file is part of Checkmk (https://checkmk.com). It is subject to the terms and +# conditions defined in the file COPYING, which is part of this source code package. + +"""Dynamic sorters based on the site configuration""" + +from collections.abc import Mapping, Sequence +from functools import partial + +from cmk.utils.tags import TagGroup, TagGroupID + +from cmk.gui.config import Config +from cmk.gui.http import Request +from cmk.gui.i18n import _ +from cmk.gui.painter.v0.helpers import get_tag_groups +from cmk.gui.type_defs import Row + +from .base import Sorter + + +def host_tag_config_based_sorters(tag_groups: Sequence[TagGroup]) -> dict[str, Sorter]: + return { + (ident := f"host_tag_{tag_group.id}"): Sorter( + ident=ident, + title=_("Host tag:") + " " + tag_group.title, + columns=["host_tags"], + load_inv=False, + sort_function=partial(_cmp_host_tag, tag_group_id=tag_group.id), + ) + for tag_group in tag_groups + } + + +def _cmp_host_tag( + r1: Row, + r2: Row, + *, + parameters: Mapping[str, object] | None, + config: Config, + request: Request, + tag_group_id: TagGroupID, +) -> int: + host_tag_1 = _get_tag_group_value(r1, "host", tag_group_id, config=config) + host_tag_2 = _get_tag_group_value(r2, "host", tag_group_id, config=config) + return (host_tag_1 > host_tag_2) - (host_tag_1 < host_tag_2) + + +def _get_tag_group_value(row: Row, what: str, tag_group_id: TagGroupID, *, config: Config) -> str: + tag_id = get_tag_groups(row, what).get(tag_group_id) + + tag_group = config.tags.get_tag_group(tag_group_id) + if tag_group: + label = dict(tag_group.get_tag_choices()).get(tag_id, _("N/A")) + else: + label = tag_id or _("N/A") + + return label or _("N/A") diff --git a/cmk/gui/views/sorter/registry.py b/cmk/gui/views/sorter/registry.py index 808e6503852..26414a0c7df 100644 --- a/cmk/gui/views/sorter/registry.py +++ b/cmk/gui/views/sorter/registry.py @@ -9,7 +9,7 @@ from cmk.ccc.plugin_registry import Registry -from cmk.gui.config import active_config +from cmk.gui.config import active_config, Config from cmk.gui.display_options import display_options from cmk.gui.http import request, response from cmk.gui.logged_in import user @@ -20,6 +20,7 @@ from cmk.gui.type_defs import ColumnName, PainterName, SorterFunction from .base import Sorter +from .host_tag_sorters import host_tag_config_based_sorters class SorterRegistry(Registry[Sorter]): @@ -30,6 +31,10 @@ def plugin_name(self, instance: Sorter) -> str: sorter_registry = SorterRegistry() +def all_sorters(config: Config) -> dict[str, Sorter]: + return dict(sorter_registry.items()) | host_tag_config_based_sorters(config.tags.tag_groups) + + # Kept for pre 1.6 compatibility. def register_sorter(ident: str, spec: dict[str, Any]) -> None: sorter_registry.register( diff --git a/tests/unit/cmk/gui/views/sorter/test_host_tag_sorters.py b/tests/unit/cmk/gui/views/sorter/test_host_tag_sorters.py index b08ad0257ba..666087cc7cd 100644 --- a/tests/unit/cmk/gui/views/sorter/test_host_tag_sorters.py +++ b/tests/unit/cmk/gui/views/sorter/test_host_tag_sorters.py @@ -8,17 +8,11 @@ from cmk.utils.tags import TagConfig, TagGroupID, TagID from cmk.gui.config import active_config -from cmk.gui.views import host_tag_plugins -from cmk.gui.views.sorter import registry +from cmk.gui.views.sorter import all_sorters @pytest.mark.usefixtures("load_config") def test_host_tag_sorter_registration(monkeypatch: pytest.MonkeyPatch) -> None: - monkeypatch.setattr( - host_tag_plugins, "sorter_registry", sorter_registry := registry.SorterRegistry() - ) - monkeypatch.setattr(registry, "sorter_registry", sorter_registry) - with monkeypatch.context() as m: m.setattr( active_config, @@ -43,5 +37,4 @@ def test_host_tag_sorter_registration(monkeypatch: pytest.MonkeyPatch) -> None: } ), ) - host_tag_plugins.register_tag_plugins() - assert "host_tag_whoot" in list(registry.sorter_registry.keys()) + assert "host_tag_whoot" in all_sorters(active_config)