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

@@ -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:

cmk/gui/views/host_tag_plugins.py

@@ -7,29 +7,22 @@
 
 """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(
         config=active_config
     ):
         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)
 

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]()

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.type_defs import ColumnSpec, PainterName, PainterParameters, SorterName, SorterSpec
 from cmk.gui.utils.theme import theme
 
-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

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",

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")

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.utils.theme import theme
 
 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(