From 0e1175a15d0d71b56b88ec9a9180db059e38015b Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Mon, 21 Sep 2026 13:01:20 +0800 Subject: [PATCH 1/3] fix(spp_cel_domain): probe the legacy metric service by capability, not model name _exec_metric and the aggregate-metric path took the presence of spp.indicator in the registry to mean the retired spp_indicators evaluation service was installed and called evaluate() on it. OpenSPP2's spp_indicator reuses that model name for an unrelated configuration model, so wherever it is installed every metric() over a variable whose cache was not fresh raised AttributeError inside the executor and the expression compiled to an error instead of the graceful empty result. Both sites now resolve the service through _legacy_metric_service(), which requires a callable evaluate(), and fall back to the SQL fast path otherwise. Refs #443 --- spp_cel_domain/__manifest__.py | 2 +- spp_cel_domain/models/cel_executor.py | 31 ++++++++++---- spp_cel_domain/readme/HISTORY.md | 4 ++ spp_cel_domain/tests/__init__.py | 3 ++ .../tests/test_legacy_metric_service.py | 40 +++++++++++++++++++ 5 files changed, 72 insertions(+), 8 deletions(-) create mode 100644 spp_cel_domain/tests/test_legacy_metric_service.py diff --git a/spp_cel_domain/__manifest__.py b/spp_cel_domain/__manifest__.py index 550c3d0f3..d53b3c3a2 100644 --- a/spp_cel_domain/__manifest__.py +++ b/spp_cel_domain/__manifest__.py @@ -2,7 +2,7 @@ { "name": "CEL Domain Query Builder", "summary": "Write simple CEL-like expressions to filter records (OpenSPP/OpenG2P friendly)", - "version": "19.0.2.1.0", + "version": "19.0.2.1.1", "license": "LGPL-3", "development_status": "Production/Stable", "author": "OpenSPP.org, OpenSPP Community", diff --git a/spp_cel_domain/models/cel_executor.py b/spp_cel_domain/models/cel_executor.py index b9fb36003..7575c8678 100644 --- a/spp_cel_domain/models/cel_executor.py +++ b/spp_cel_domain/models/cel_executor.py @@ -1072,6 +1072,25 @@ def _split_child_membership(self, through_model: str, child_plan: Any) -> tuple[ # Fallback: cannot split return [], child_plan + def _legacy_metric_service(self): + """The legacy metric evaluation service, or ``None`` when no module provides it. + + The service was the ``spp.indicator`` model of the retired + ``spp_indicators`` module, exposing ``evaluate()`` and + ``enqueue_refresh_from_domain()``. OpenSPP2's ``spp_indicator`` reuses the + model name for an unrelated publishable-indicator configuration model, so + the presence of ``spp.indicator`` in the registry no longer means the + service is available. Probe the capability, not the name: with only the + name checked, every ``metric()`` over a variable whose cache is not fresh + raised ``AttributeError`` inside the executor wherever ``spp_indicator`` + is installed, and the expression compiled to an error instead of the + graceful empty result. + """ + service = self.env.get("spp.indicator") + if service is None or not callable(getattr(service, "evaluate", None)): + return None + return service + def _exec_metric( self, model: str, @@ -1171,19 +1190,18 @@ def _exec_metric( # Compute candidate size cheaply via search_count base_count = self.env[subject_model].search_count(base_dom) - # Check for evaluation service (legacy spp.indicator for now) # TODO: Fully migrate to spp.data.cache.manager (Phase 4 of ADR-017 complete) - if "spp.indicator" not in self.env: + svc = self._legacy_metric_service() + if svc is None: # No evaluation service available - can only use SQL fast path # If we reach here, cache is not fresh and we can't compute self._logger.warning( "[CEL Metrics] No evaluation service available for metric=%s. " - "SQL fast path requires fresh cache. Consider installing spp_indicators module.", + "SQL fast path requires fresh cache; subjects without a fresh cached value are left out.", p.metric, ) return [] - svc = self.env["spp.indicator"] default_mode = "refresh" if (base_count < async_threshold) else "fallback" if default_mode == "fallback" and status.get("status") != "fresh" and not preview_cache_only_mode: # large + not fresh → enqueue refresh and report queued @@ -1590,16 +1608,15 @@ def _exec_agg_metric( # noqa: C901 if not all_child_ids: return [] - # Check for evaluation service (legacy spp.indicator for now) # TODO: Fully migrate to spp.data.cache.manager (Phase 4 of ADR-017 complete) - if "spp.indicator" not in self.env: + svc = self._legacy_metric_service() + if svc is None: self._logger.warning( "[CEL Metrics] No evaluation service available for aggregate metric=%s", p.metric, ) return [] - svc = self.env["spp.indicator"] values, stats = svc.evaluate( p.metric, p.child_model, diff --git a/spp_cel_domain/readme/HISTORY.md b/spp_cel_domain/readme/HISTORY.md index 1b4fd10d3..007718974 100644 --- a/spp_cel_domain/readme/HISTORY.md +++ b/spp_cel_domain/readme/HISTORY.md @@ -1,3 +1,7 @@ +### 19.0.2.1.1 + +- fix(executor): probe for the legacy metric evaluation service by capability, not by model name. `_exec_metric` and the aggregate-metric path took the presence of `spp.indicator` in the registry to mean the retired `spp_indicators` service (with `evaluate()`) was installed; OpenSPP2's `spp_indicator` reuses that model name for an unrelated configuration model, so wherever it is installed every `metric()` over a variable whose cache was not fresh raised `AttributeError` inside the executor and the whole expression compiled to an error instead of the documented graceful empty result. Both sites now fall back to the SQL fast path when no service exposes `evaluate()` (#443) + ### 19.0.2.1.0 - feat(sql): compile CEL ternary expressions to SQL CASE via `to_sql_case`, with `case_when`/`comparison` builders and a right-associative ternary parsing fix diff --git a/spp_cel_domain/tests/__init__.py b/spp_cel_domain/tests/__init__.py index f9eb6bff3..7cb4a32b7 100644 --- a/spp_cel_domain/tests/__init__.py +++ b/spp_cel_domain/tests/__init__.py @@ -32,3 +32,6 @@ from . import test_cel_relational_predicate from . import test_cel_smart_op_lookup from . import test_cel_translator_cache + +# #443: legacy metric evaluation service probe +from . import test_legacy_metric_service diff --git a/spp_cel_domain/tests/test_legacy_metric_service.py b/spp_cel_domain/tests/test_legacy_metric_service.py new file mode 100644 index 000000000..4dcb802a7 --- /dev/null +++ b/spp_cel_domain/tests/test_legacy_metric_service.py @@ -0,0 +1,40 @@ +# Part of OpenSPP. See LICENSE file for full copyright and licensing details. +"""The executor resolves the legacy metric evaluation service by capability. + +``spp.indicator`` is a model name shared by two unrelated things: the retired +``spp_indicators`` evaluation service (``evaluate()``) the executor was written +against, and OpenSPP2's ``spp_indicator`` configuration model. Only the former +may be used as the service; the latter must be treated as "no service", which +is the graceful path ``TestCELExecutorCacheLookup`` asserts and the path a full +stack with ``spp_indicator`` installed used to crash on. +""" + +from unittest.mock import MagicMock, patch + +from odoo.api import Environment +from odoo.tests import TransactionCase, tagged + + +@tagged("post_install", "-at_install") +class TestLegacyMetricService(TransactionCase): + def setUp(self): + super().setUp() + self.executor = self.env["spp.cel.executor"] + + def test_no_model_means_no_service(self): + """Without any spp.indicator model the probe reports no service.""" + if "spp.indicator" in self.env: + self.skipTest("spp_indicator is installed in this database") + self.assertIsNone(self.executor._legacy_metric_service()) + + def test_model_without_evaluate_is_not_the_service(self): + """A model that merely carries the name is not the evaluation service.""" + config_model = MagicMock(spec=[]) # no attributes at all, like spp_indicator's model + with patch.object(Environment, "get", return_value=config_model): + self.assertIsNone(self.executor._legacy_metric_service()) + + def test_model_with_evaluate_is_the_service(self): + """A model exposing evaluate() is returned as-is.""" + service = MagicMock(spec=["evaluate"]) + with patch.object(Environment, "get", return_value=service): + self.assertIs(self.executor._legacy_metric_service(), service) From 198605e70efb103ae882b97da5c2195e5d333fec Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Mon, 21 Sep 2026 13:12:02 +0800 Subject: [PATCH 2/3] fix(spp_cel_domain): require both legacy service methods and record the no-service path Review round: the probe checks evaluate() and enqueue_refresh_from_domain() on the model class (a field carrying one of those names is a non-callable descriptor and never touches access checks); both no-service branches append a no_service entry to metrics_info so previews can show that metric values were unavailable; the tests use a real recordset as the name-only stand-in, assert the probed model name, and cover the evaluate-only case; changelog states the actual behaviour (empty match set, not an SQL fast path) and the deployer-visible degradation. Refs #443 --- spp_cel_domain/models/cel_executor.py | 32 +++++++---- spp_cel_domain/readme/HISTORY.md | 2 +- spp_cel_domain/tests/__init__.py | 2 - .../tests/test_legacy_metric_service.py | 56 +++++++++++++------ 4 files changed, 60 insertions(+), 32 deletions(-) diff --git a/spp_cel_domain/models/cel_executor.py b/spp_cel_domain/models/cel_executor.py index 7575c8678..0be2cd845 100644 --- a/spp_cel_domain/models/cel_executor.py +++ b/spp_cel_domain/models/cel_executor.py @@ -1072,22 +1072,24 @@ def _split_child_membership(self, through_model: str, child_plan: Any) -> tuple[ # Fallback: cannot split return [], child_plan + _LEGACY_METRIC_SERVICE_METHODS = ("evaluate", "enqueue_refresh_from_domain") + def _legacy_metric_service(self): """The legacy metric evaluation service, or ``None`` when no module provides it. - The service was the ``spp.indicator`` model of the retired - ``spp_indicators`` module, exposing ``evaluate()`` and - ``enqueue_refresh_from_domain()``. OpenSPP2's ``spp_indicator`` reuses the - model name for an unrelated publishable-indicator configuration model, so - the presence of ``spp.indicator`` in the registry no longer means the - service is available. Probe the capability, not the name: with only the - name checked, every ``metric()`` over a variable whose cache is not fresh - raised ``AttributeError`` inside the executor wherever ``spp_indicator`` - is installed, and the expression compiled to an error instead of the - graceful empty result. + Two unrelated things share the ``spp.indicator`` model name: the + evaluation service of the retired ``spp_indicators`` module, which this + executor calls, and OpenSPP2's ``spp_indicator`` publishable-indicator + configuration model, which must be treated as "no service". The probe + therefore checks for the service's methods on the model class rather + than for the name in the registry. Class-level lookup keeps it free of + field access checks: a field that happened to carry one of these names + resolves to a non-callable descriptor. """ service = self.env.get("spp.indicator") - if service is None or not callable(getattr(service, "evaluate", None)): + if service is None: + return None + if not all(callable(getattr(type(service), name, None)) for name in self._LEGACY_METRIC_SERVICE_METHODS): return None return service @@ -1200,6 +1202,10 @@ def _exec_metric( "SQL fast path requires fresh cache; subjects without a fresh cached value are left out.", p.metric, ) + if metrics_info is not None: + mi = dict(status) + mi.update({"metric": p.metric, "period_key": period_key, "path": "no_service"}) + metrics_info.append(mi) return [] default_mode = "refresh" if (base_count < async_threshold) else "fallback" @@ -1615,6 +1621,10 @@ def _exec_agg_metric( # noqa: C901 "[CEL Metrics] No evaluation service available for aggregate metric=%s", p.metric, ) + if metrics_info is not None: + metrics_info.append( + {"metric": p.metric, "period_key": str(p.period_key or "default"), "path": "no_service"} + ) return [] values, stats = svc.evaluate( diff --git a/spp_cel_domain/readme/HISTORY.md b/spp_cel_domain/readme/HISTORY.md index 007718974..93a61d681 100644 --- a/spp_cel_domain/readme/HISTORY.md +++ b/spp_cel_domain/readme/HISTORY.md @@ -1,6 +1,6 @@ ### 19.0.2.1.1 -- fix(executor): probe for the legacy metric evaluation service by capability, not by model name. `_exec_metric` and the aggregate-metric path took the presence of `spp.indicator` in the registry to mean the retired `spp_indicators` service (with `evaluate()`) was installed; OpenSPP2's `spp_indicator` reuses that model name for an unrelated configuration model, so wherever it is installed every `metric()` over a variable whose cache was not fresh raised `AttributeError` inside the executor and the whole expression compiled to an error instead of the documented graceful empty result. Both sites now fall back to the SQL fast path when no service exposes `evaluate()` (#443) +- fix(executor): probe for the legacy metric evaluation service by capability, not by model name. `_exec_metric` and the aggregate-metric path took the presence of `spp.indicator` in the registry to mean the retired `spp_indicators` service (with `evaluate()`) was installed; OpenSPP2's `spp_indicator` reuses that model name for an unrelated configuration model, so wherever it is installed every `metric()` over a variable whose cache was not fresh raised `AttributeError` inside the executor and the whole expression compiled to an error instead of the documented graceful empty result. Both sites now return no matches, log a warning and record a `no_service` entry in the metrics info when no model exposes the service's `evaluate()` and `enqueue_refresh_from_domain()`, instead of raising. Note for deployers: on such databases a `metric()` over a not-fresh cached variable therefore yields an empty match set rather than an error, the same degradation every database without `spp_indicator` already had (#443) ### 19.0.2.1.0 diff --git a/spp_cel_domain/tests/__init__.py b/spp_cel_domain/tests/__init__.py index 7cb4a32b7..92b01daee 100644 --- a/spp_cel_domain/tests/__init__.py +++ b/spp_cel_domain/tests/__init__.py @@ -32,6 +32,4 @@ from . import test_cel_relational_predicate from . import test_cel_smart_op_lookup from . import test_cel_translator_cache - -# #443: legacy metric evaluation service probe from . import test_legacy_metric_service diff --git a/spp_cel_domain/tests/test_legacy_metric_service.py b/spp_cel_domain/tests/test_legacy_metric_service.py index 4dcb802a7..d6862d9c3 100644 --- a/spp_cel_domain/tests/test_legacy_metric_service.py +++ b/spp_cel_domain/tests/test_legacy_metric_service.py @@ -2,39 +2,59 @@ """The executor resolves the legacy metric evaluation service by capability. ``spp.indicator`` is a model name shared by two unrelated things: the retired -``spp_indicators`` evaluation service (``evaluate()``) the executor was written -against, and OpenSPP2's ``spp_indicator`` configuration model. Only the former -may be used as the service; the latter must be treated as "no service", which -is the graceful path ``TestCELExecutorCacheLookup`` asserts and the path a full -stack with ``spp_indicator`` installed used to crash on. +``spp_indicators`` evaluation service (``evaluate()`` and +``enqueue_refresh_from_domain()``) the executor was written against, and +OpenSPP2's ``spp_indicator`` configuration model. Only the former may be used +as the service; the latter must be treated as "no service", which is the +graceful path ``TestCELExecutorCacheLookup`` asserts and the path a full stack +with ``spp_indicator`` installed used to crash on. """ -from unittest.mock import MagicMock, patch +from unittest.mock import patch from odoo.api import Environment from odoo.tests import TransactionCase, tagged +class _LegacyService: + """The shape of the retired service: both methods, both callable.""" + + def evaluate(self, *args, **kwargs): + return {}, {} + + def enqueue_refresh_from_domain(self, *args, **kwargs): + return None + + +class _EvaluateOnly: + def evaluate(self, *args, **kwargs): + return {}, {} + + @tagged("post_install", "-at_install") class TestLegacyMetricService(TransactionCase): def setUp(self): super().setUp() self.executor = self.env["spp.cel.executor"] - def test_no_model_means_no_service(self): - """Without any spp.indicator model the probe reports no service.""" - if "spp.indicator" in self.env: - self.skipTest("spp_indicator is installed in this database") + def test_no_service_in_this_database(self): + """No module in the repository provides the service, with or without + spp_indicator installed, so the probe must report none either way.""" self.assertIsNone(self.executor._legacy_metric_service()) - def test_model_without_evaluate_is_not_the_service(self): - """A model that merely carries the name is not the evaluation service.""" - config_model = MagicMock(spec=[]) # no attributes at all, like spp_indicator's model - with patch.object(Environment, "get", return_value=config_model): + def test_real_model_without_the_methods_is_not_the_service(self): + """A genuine Odoo model that merely carries the name is not the service.""" + with patch.object(Environment, "get", return_value=self.env["res.partner"]) as env_get: + self.assertIsNone(self.executor._legacy_metric_service()) + env_get.assert_called_once_with("spp.indicator") + + def test_evaluate_alone_is_not_enough(self): + """The executor also enqueues refreshes; both methods are required.""" + with patch.object(Environment, "get", return_value=_EvaluateOnly()): self.assertIsNone(self.executor._legacy_metric_service()) - def test_model_with_evaluate_is_the_service(self): - """A model exposing evaluate() is returned as-is.""" - service = MagicMock(spec=["evaluate"]) - with patch.object(Environment, "get", return_value=service): + def test_model_with_both_methods_is_the_service(self): + service = _LegacyService() + with patch.object(Environment, "get", return_value=service) as env_get: self.assertIs(self.executor._legacy_metric_service(), service) + env_get.assert_called_once_with("spp.indicator") From 270b70fba37c6f844df7722ea3d5165fef544a30 Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Mon, 21 Sep 2026 13:26:30 +0800 Subject: [PATCH 3/3] docs(spp_cel_domain): regenerate README from fragments (CI output) --- spp_cel_domain/README.rst | 21 +++++++++++++++++ spp_cel_domain/static/description/index.html | 24 +++++++++++++++++++- 2 files changed, 44 insertions(+), 1 deletion(-) diff --git a/spp_cel_domain/README.rst b/spp_cel_domain/README.rst index 9a72fa2b4..5ae39f0a4 100644 --- a/spp_cel_domain/README.rst +++ b/spp_cel_domain/README.rst @@ -142,6 +142,27 @@ Dependencies Changelog ========= +19.0.2.1.1 +~~~~~~~~~~ + +- fix(executor): probe for the legacy metric evaluation service by + capability, not by model name. ``_exec_metric`` and the + aggregate-metric path took the presence of ``spp.indicator`` in the + registry to mean the retired ``spp_indicators`` service (with + ``evaluate()``) was installed; OpenSPP2's ``spp_indicator`` reuses + that model name for an unrelated configuration model, so wherever it + is installed every ``metric()`` over a variable whose cache was not + fresh raised ``AttributeError`` inside the executor and the whole + expression compiled to an error instead of the documented graceful + empty result. Both sites now return no matches, log a warning and + record a ``no_service`` entry in the metrics info when no model + exposes the service's ``evaluate()`` and + ``enqueue_refresh_from_domain()``, instead of raising. Note for + deployers: on such databases a ``metric()`` over a not-fresh cached + variable therefore yields an empty match set rather than an error, the + same degradation every database without ``spp_indicator`` already had + (#443) + 19.0.2.1.0 ~~~~~~~~~~ diff --git a/spp_cel_domain/static/description/index.html b/spp_cel_domain/static/description/index.html index f280ce2a1..96a20364b 100644 --- a/spp_cel_domain/static/description/index.html +++ b/spp_cel_domain/static/description/index.html @@ -522,6 +522,28 @@

Changelog

+

19.0.2.1.1

+
    +
  • fix(executor): probe for the legacy metric evaluation service by +capability, not by model name. _exec_metric and the +aggregate-metric path took the presence of spp.indicator in the +registry to mean the retired spp_indicators service (with +evaluate()) was installed; OpenSPP2’s spp_indicator reuses +that model name for an unrelated configuration model, so wherever it +is installed every metric() over a variable whose cache was not +fresh raised AttributeError inside the executor and the whole +expression compiled to an error instead of the documented graceful +empty result. Both sites now return no matches, log a warning and +record a no_service entry in the metrics info when no model +exposes the service’s evaluate() and +enqueue_refresh_from_domain(), instead of raising. Note for +deployers: on such databases a metric() over a not-fresh cached +variable therefore yields an empty match set rather than an error, the +same degradation every database without spp_indicator already had +(#443)
    • +
+
+

19.0.2.1.0

  • feat(sql): compile CEL ternary expressions to SQL CASE via @@ -533,7 +555,7 @@

    19.0.2.1.0

  • test(translator): add coverage for the CEL translation cache helpers
-
+

19.0.2.0.0

  • Initial migration to OpenSPP2