fix(spp_cel_domain): probe the legacy metric service by capability, not model name (#443)

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
 ~~~~~~~~~~
 

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

spp_cel_domain/models/cel_executor.py

@@ -1072,6 +1072,27 @@ 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.
+
+        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:
+            return None
+        if not all(callable(getattr(type(service), name, None)) for name in self._LEGACY_METRIC_SERVICE_METHODS):
+            return None
+        return service
+
     def _exec_metric(
         self,
         model: str,
@@ -1171,19 +1192,22 @@ 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,
             )
+            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 []
 
-        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 +1614,19 @@ 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,
             )
+            if metrics_info is not None:
+                metrics_info.append(
+                    {"metric": p.metric, "period_key": str(p.period_key or "default"), "path": "no_service"}
+                )
             return []
 
-        svc = self.env["spp.indicator"]
         values, stats = svc.evaluate(
             p.metric,
             p.child_model,

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 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 `to_sql_case`, with `case_when`/`comparison` builders and a right-associative ternary parsing fix

spp_cel_domain/static/description/index.html

@@ -522,6 +522,28 @@ <h2><a class="toc-backref" href="#toc-entry-1">Changelog</a></h2>
 </div>
 </div>
 <div class="section" id="section-1">
+<h1>19.0.2.1.1</h1>
+<ul class="simple">
+<li>fix(executor): probe for the legacy metric evaluation service by
+capability, not by model name. <tt class="docutils literal">_exec_metric</tt> and the
+aggregate-metric path took the presence of <tt class="docutils literal">spp.indicator</tt> in the
+registry to mean the retired <tt class="docutils literal">spp_indicators</tt> service (with
+<tt class="docutils literal">evaluate()</tt>) was installed; OpenSPP2’s <tt class="docutils literal">spp_indicator</tt> reuses
+that model name for an unrelated configuration model, so wherever it
+is installed every <tt class="docutils literal">metric()</tt> over a variable whose cache was not
+fresh raised <tt class="docutils literal">AttributeError</tt> 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 <tt class="docutils literal">no_service</tt> entry in the metrics info when no model
+exposes the service’s <tt class="docutils literal">evaluate()</tt> and
+<tt class="docutils literal">enqueue_refresh_from_domain()</tt>, instead of raising. Note for
+deployers: on such databases a <tt class="docutils literal">metric()</tt> over a not-fresh cached
+variable therefore yields an empty match set rather than an error, the
+same degradation every database without <tt class="docutils literal">spp_indicator</tt> already had
+(#443)</li>
+</ul>
+</div>
+<div class="section" id="section-2">
 <h1>19.0.2.1.0</h1>
 <ul class="simple">
 <li>feat(sql): compile CEL ternary expressions to SQL CASE via
@@ -533,7 +555,7 @@ <h1>19.0.2.1.0</h1>
 <li>test(translator): add coverage for the CEL translation cache helpers</li>
 </ul>
 </div>
-<div class="section" id="section-2">
+<div class="section" id="section-3">
 <h1>19.0.2.0.0</h1>
 <ul class="simple">
 <li>Initial migration to OpenSPP2</li>

spp_cel_domain/tests/__init__.py

@@ -32,3 +32,4 @@
 from . import test_cel_relational_predicate
 from . import test_cel_smart_op_lookup
 from . import test_cel_translator_cache
+from . import test_legacy_metric_service

spp_cel_domain/tests/test_legacy_metric_service.py

@@ -0,0 +1,60 @@
+# 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()`` 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 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_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_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_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")