Add uniqueness for Template Units id (BugFix)

checkbox-ng/plainbox/impl/highlevel.py

@@ -293,9 +293,11 @@ def _file_to_obj(self, unit):
 
     def _template_to_obj(self, unit):
         return PlainBoxObject(
-            unit, group=unit.Meta.name, name=unit.id, attrs=OrderedDict((
+            unit, group=unit.Meta.name, name=unit.template_id,
+            attrs=OrderedDict((
                 ('id', unit.id),
                 ('partial_id', unit.partial_id),
+                ('template_id', unit.template_id),
                 ('template_unit', unit.template_unit),
                 ('template_resource', unit.template_resource),
                 ('template_filter', unit.template_filter),

checkbox-ng/plainbox/impl/test_highlevel.py

@@ -0,0 +1,40 @@
+# This file is part of Checkbox.
+#
+# Copyright 2024 Canonical Ltd.
+# Written by:
+#   Pierre Equoy <[email protected]>
+#
+# Checkbox is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License version 3,
+# as published by the Free Software Foundation.
+#
+# Checkbox is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with Checkbox.  If not, see <http://www.gnu.org/licenses/>.
+
+from unittest import TestCase
+
+from plainbox.impl.highlevel import Explorer
+from plainbox.impl.unit.template import TemplateUnit
+
+
+class TestExplorer(TestCase):
+    def test_template_to_obj__without_template_id(self):
+        template = TemplateUnit({
+            "id": "id",
+        })
+        explorer = Explorer()
+        obj = explorer._template_to_obj(template)
+        self.assertEqual(obj.name, "id")
+
+    def test_template_to_obj__with_template_id(self):
+        template = TemplateUnit({
+            "template-id": "template-id",
+        })
+        explorer = Explorer()
+        obj = explorer._template_to_obj(template)
+        self.assertEqual(obj.name, "template-id")

checkbox-ng/plainbox/impl/unit/template.py

@@ -23,6 +23,7 @@
 """
 import itertools
 import logging
+import string
 
 from plainbox.i18n import gettext as _
 from plainbox.i18n import gettext_noop as N_
@@ -35,13 +36,13 @@
 from plainbox.impl.unit import all_units
 from plainbox.impl.unit import concrete_validators
 from plainbox.impl.unit import get_accessed_parameters
-from plainbox.impl.unit.unit import Unit
-from plainbox.impl.unit.unit import UnitValidator
+from plainbox.impl.unit.unit_with_id import UnitWithId
+from plainbox.impl.unit.unit_with_id import UnitWithIdValidator
 from plainbox.impl.unit.validators import CorrectFieldValueValidator
 from plainbox.impl.unit.validators import ReferenceConstraint
 from plainbox.impl.unit.validators import UnitReferenceValidator
+from plainbox.impl.unit.validators import UniqueValueValidator
 from plainbox.impl.validation import Problem
-from plainbox.impl.validation import Severity
 
 
 __all__ = ['TemplateUnit']
@@ -50,7 +51,7 @@
 logger = logging.getLogger("plainbox.unit.template")
 
 
-class TemplateUnitValidator(UnitValidator):
+class TemplateUnitValidator(UnitWithIdValidator):
 
     """Validator for template unit."""
 
@@ -77,8 +78,28 @@
                 self.issue_list.append(issue)
                 yield issue
 
+    def explain(self, unit, field, kind, message):
+        """
+        Lookup an explanatory string for a given issue kind
+
+        :returns:
+            A string (explanation) or None if the issue kind
+            is not known to this method.
+
+        This version overrides the base implementation to use the unit
+        template_id, if it is available, when reporting issues.
+        """
+        if unit.template_partial_id is None:
+            return super().explain(unit, field, kind, message)
+        stock_msg = self._explain_map.get(kind)
+        if stock_msg is None:
+            return None
+        return _("{unit} {id!a}, field {field!a}, {message}").format(
+            unit=unit.tr_unit(), id=unit.template_partial_id, field=str(field),
+            message=message or stock_msg)
+
 
-class TemplateUnit(Unit):
+class TemplateUnit(UnitWithId):
 
     """
     Template that can instantiate zero or more additional units.
@@ -166,21 +187,13 @@
         return "{} <~ {}".format(self.id, self.resource_id)
 
     @property
-    def partial_id(self):
+    def unit(self):
         """
-        Identifier of this job, without the provider name.
+        The value of the unit field (overridden)
 
-        This field should not be used anymore, except for display
+        The return value is always "template"
         """
-        return self.get_record_value('id', '?')
-
-    @property
-    def id(self):
-        """Identifier of this template unit."""
-        if self.provider:
-            return "{}::{}".format(self.provider.namespace, self.partial_id)
-        else:
-            return self.partial_id
+        return "template"
 
     @property
     def resource_partial_id(self):
@@ -216,6 +229,44 @@
         else:
             return "{}::{}".format(resource_namespace, resource_partial_id)
 
+    @classmethod
+    def slugify_template_id(cls, _string=None):
+        """
+        Remove unwanted characters from a raw job id string.
+
+        This helps exposing cleaner looking template ids when the id is
+        generated from the id field by removing characters like '{', '}',
+        and ' '.
+        """
+        if _string:
+            valid_chars = frozenset(
+                "-_.:/\\{}{}".format(string.ascii_letters, string.digits)
+            )
+            return "".join(c if c in valid_chars else "" for c in _string)
+
+    @property
+    def template_partial_id(self):
+        """
+        Identifier of this template, without the provider namespace.
+
+        If the ``template-id`` field is not present in the unit definition,
+        ``template_partial_id`` is computed from the ``partial_id`` attribute.
+        """
+        template_partial_id = self.get_record_value("template-id")
+        if not template_partial_id:
+            template_partial_id = self.slugify_template_id(self.partial_id)
+        return template_partial_id
+
+    @property
+    def template_id(self):
+        """Identifier of this template, with the provider namespace."""
+        if self.provider and self.template_partial_id:
+            return "{}::{}".format(self.provider.namespace,
+                                   self.template_partial_id
+                                   )
+        else:
+            return self.template_partial_id
+
     @property
     def template_resource(self):
         """value of the 'template-resource' field."""
@@ -436,6 +487,7 @@
 
             """Symbols for each field that a TemplateUnit can have."""
 
+            template_id = "template-id"
             template_unit = 'template-unit'
             template_resource = 'template-resource'
             template_filter = 'template-filter'
@@ -444,6 +496,17 @@
         validator_cls = TemplateUnitValidator
 
         field_validators = {
+            fields.template_id: [
+                concrete_validators.untranslatable,
+                concrete_validators.templateVariant,
+                UniqueValueValidator(),
+                # We want to have bare, namespace-less identifiers
+                CorrectFieldValueValidator(
+                    lambda value, unit: (
+                        "::" not in unit.get_record_value("template-id")),
+                    message=_("identifier cannot define a custom namespace"),
+                    onlyif=lambda unit: unit.get_record_value("template-id")),
+            ],
             fields.template_unit: [
                 concrete_validators.untranslatable,
             ],

checkbox-ng/plainbox/impl/unit/test_template.py

@@ -42,6 +42,14 @@
 
 class TemplateUnitTests(TestCase):
 
+    def test_id(self):
+        template = TemplateUnit({
+            "template-resource": "resource",
+            "template-id": "check-devices",
+            "id": "check-device-{dev_name}",
+        })
+        self.assertEqual(template.id, "check-device-{dev_name}")
+
     def test_resource_partial_id__empty(self):
         """
         Ensure that ``resource_partial_id`` defaults to None
@@ -167,6 +175,54 @@ def test_resource_id__template_and_provider_ns(self):
             'template-resource': 'rc'
         }, provider=provider).resource_id, 'namespace::rc')
 
+    def test_slugify(self):
+        self.assertEqual(
+            TemplateUnit.slugify_template_id("stress/benchmark_{disk}"),
+            "stress/benchmark_disk"
+        )
+        self.assertEqual(
+            TemplateUnit.slugify_template_id("ns::stress/benchmark_{disk}"),
+            "ns::stress/benchmark_disk"
+        )
+        self.assertEqual(
+            TemplateUnit.slugify_template_id("suspend_{{ iterations }}_times"),
+            "suspend_iterations_times"
+        )
+        self.assertEqual(TemplateUnit.slugify_template_id(), None)
+
+    def test_template_id(self):
+        self.assertEqual(TemplateUnit({
+            "template-id": "template_id",
+        }).template_id, "template_id")
+
+    def test_template_id__from_job_id(self):
+        self.assertEqual(TemplateUnit({
+            "id": "job_id_{param}",
+        }).template_id, "job_id_param")
+
+    def test_template_id__precedence(self):
+        """Ensure template-id takes precedence over job id."""
+        self.assertEqual(TemplateUnit({
+            "template-id": "template_id",
+            "id": "job_id_{param}",
+        }).template_id, "template_id")
+
+    def test_template_id__from_job_id_jinja2(self):
+        self.assertEqual(TemplateUnit({
+            "template-resource": "resource",
+            "template-engine": "jinja2",
+            "id": "job_id_{{ param }}",
+        }).template_id, "job_id_param")
+
+    def test_template_id__precedence_jinja2(self):
+        """Ensure template-id takes precedence over Jinja2-templated job id."""
+        self.assertEqual(TemplateUnit({
+            "template-id": "template_id",
+            "template-resource": "resource",
+            "template-engine": "jinja2",
+            "id": "job_id_{{ param }}",
+        }).template_id, "template_id")
+
     def test_template_resource__empty(self):
         self.assertEqual(TemplateUnit({}).template_resource, None)
 
@@ -351,6 +407,14 @@ def test_instantiate_all(self):
 
 class TemplateUnitJinja2Tests(TestCase):
 
+    def test_id_jinja2(self):
+        template = TemplateUnit({
+            'template-resource': 'resource',
+            'template-engine': 'jinja2',
+            'id': 'check-device-{{ dev_name }}',
+        })
+        self.assertEqual(template.id, "check-device-{{ dev_name }}")
+
     def test_instantiate_one_jinja2(self):
         template = TemplateUnit({
             'template-resource': 'resource',
@@ -374,6 +438,55 @@ class TemplateUnitFieldValidationTests(UnitFieldValidationTests):
 
     unit_cls = TemplateUnit
 
+    def test_template_id__untranslatable(self):
+        issue_list = self.unit_cls({
+            '_template-id': 'template_id'
+        }, provider=self.provider).check()
+        self.assertIssueFound(
+            issue_list, self.unit_cls.Meta.fields.template_id,
+            Problem.unexpected_i18n, Severity.warning)
+
+    def test_template_id__bare(self):
+        issue_list = self.unit_cls({
+            "template-id": "ns::id"
+        }, provider=self.provider).check()
+        message = ("template 'ns::id', field 'template-id', identifier cannot "
+                   "define a custom namespace")
+        self.assertIssueFound(
+            issue_list, self.unit_cls.Meta.fields.template_id,
+            Problem.wrong, Severity.error, message)
+
+    def test_template_id__unique(self):
+        unit = self.unit_cls({
+            'template-id': 'id'
+        }, provider=self.provider)
+        other_unit = self.unit_cls({
+            'template-id': 'id'
+        }, provider=self.provider)
+        self.provider.unit_list = [unit, other_unit]
+        self.provider.problem_list = []
+        context = UnitValidationContext([self.provider])
+        message_start = (
+            "{} 'id', field 'template-id', clashes with 1 other unit,"
+            " look at: "
+        ).format(unit.tr_unit())
+        issue_list = unit.check(context=context)
+        issue = self.assertIssueFound(
+            issue_list, self.unit_cls.Meta.fields.template_id,
+            Problem.not_unique, Severity.error)
+        self.assertTrue(issue.message.startswith(message_start))
+
+    def test_unit__present(self):
+        """
+        TemplateUnit.unit always returns "template", the default error for the
+        base Unit class should never happen.
+        """
+        issue_list = self.unit_cls({
+        }, provider=self.provider).check()
+        message = "field 'unit', unit should explicitly define its type"
+        self.assertIssueNotFound(issue_list, self.unit_cls.Meta.fields.unit,
+                                 Problem.missing, Severity.advice, message)
+
     def test_template_unit__untranslatable(self):
         issue_list = self.unit_cls({
             # NOTE: the value must be a valid unit!