Add uniqueness for Template Units id (BugFix) (canonical#951)

* Make sure Template units are units with id

A Template unit always contains an id field. It should therefore inherit
UnitWithId instead of Unit, so that the `partial_id()` and `id()`
methods are inherited as well.

* Prevent Template Units id from being rendered when using Jinja2

By default, templates using Jinja2 engine gets their id rendered as soon
as they get accessed, but since they don't have the appropriate
environment, the parameters are replaced with an empty string.

This is why when you run the command `checkbox-cli list template`, you
may see things like this:

    template 'com.canonical.certification::camera/still_'

instead of

    template 'com.canonical.certification::camera/still_{{ name }}'

Note that this does not happen with standard Python formatted strings:

    template 'com.canonical.certification::camera/led_{name}'

In order to make the behavior consistent regardless of the template
engine used, this commit:

- introduces a unit property that always returns "template" (similar to
how the Job unit always returns "job").
- modifies Unit.get_record_value() to prevent rendering using Jinja2
templating if the unit is a Checkbox Template

It also contains unit tests for both the TemplateUnit and its Validator.

* Have Template Unit's "template-id" field generated from the "id" field, if absent

* Expose Template Unit's template-id field instead of id

PlainBoxObject is used when exposing Units at a high level; for
instance, when running `checkbox-cli list template`.

Instead of exposing the Template Unit's id field, expose the template-id
field.

* Use Template partial_id and id, similar to Job's partial_id and id

In order to have more granularity on what is displayed for the template
id, use methods similar to the Job Unit.

* Add unit test to ensure templates have a unique id

* Implement explain() in TemplateUnit

This method overrides UnitWithId.explain() which displays the Unit's id.

In the case of a Template, this is misleading, and the template_id (or,
in this case, template_partial_id to prevent displaying the provider
namespace) should be used instead.

This is useful when reporting errors using `./manage.py validate`, for
instance.

* Update Template Unit documentation about template-id field

* Cleanup Template Unit reference documentation

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 @@ def check(self, unit):
                 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 @@ def __str__(self):
         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 @@ def resource_id(self):
         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 @@ class fields(SymbolDef):
 
             """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 @@ class fields(SymbolDef):
         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!