diff --git a/src/admin/CMakeLists.txt b/src/admin/CMakeLists.txt index b68423e491..47376a61f9 100644 --- a/src/admin/CMakeLists.txt +++ b/src/admin/CMakeLists.txt @@ -43,3 +43,8 @@ add_test(NAME test_qserv_log COMMAND python3 -m unittest tests.admin.test_qserv_log WORKING_DIRECTORY ${CMAKE_INSTALL_PREFIX}/python/ ) + +add_test(NAME test_render_targs + COMMAND python3 -m unittest tests.admin.test_render_targs + WORKING_DIRECTORY ${CMAKE_INSTALL_PREFIX}/python/ +) diff --git a/src/admin/python/lsst/qserv/admin/cli/render_targs.py b/src/admin/python/lsst/qserv/admin/cli/render_targs.py index f1d710f79c..290e85b077 100644 --- a/src/admin/python/lsst/qserv/admin/cli/render_targs.py +++ b/src/admin/python/lsst/qserv/admin/cli/render_targs.py @@ -25,6 +25,7 @@ from copy import copy import jinja2 +from typing import List from .utils import Targs @@ -35,6 +36,30 @@ class UnresolvableTemplate(RuntimeError): pass +def _format_targs(targs: Targs) -> str: + """Format targs for printing to an error message.""" + return ", ".join([f"{k}={v}" for k, v in targs.items()]) + +def _get_vars(val: str) -> List[str]: + """Get variable names from a value that contains template variables. + + Parameters + ---------- + val : str + The value from a targs entry. + + Returns + ------- + vars : list [ `str` ] + The variables (value inside braces) inside val. + """ + # Jinja variable names must be surrounded by double braces and may be + # surrounded by whitespace inside the braces (`{{foo}}` or `{{ foo }}`). + # Python variable names may not contain braces. So, find all the leading + # braces, and use the rest of the string up to the closing braces. + return [i[:i.find("}}")].strip() for i in val.split("{{") if "}}" in i] + + def render_targs(targs: Targs) -> Targs: """Go through a dict whose values may contain jinja templates that are other keys in the dict, and resolve the values. @@ -65,17 +90,21 @@ def render_targs(targs: Targs) -> Targs: if not isinstance(v, str): continue if "{{" in v: + if k in _get_vars(v): + raise UnresolvableTemplate( + "Template value may not refer to its own key, directly or as a circualr reference:" + + _format_targs(targs) + ) t = jinja2.Template(v, undefined=jinja2.StrictUndefined) try: - rendered[k] = t.render(rendered) - changed = True + r = t.render(rendered) + if r != rendered[k]: + rendered[k] = r + changed = True except jinja2.exceptions.UndefinedError as e: raise UnresolvableTemplate(f"Missing template value: {str(e)}") if not changed: break if any([isinstance(v, str) and "{{" in v for v in rendered.values()]): - raise UnresolvableTemplate( - "Could not resolve inputs: " - f"{', '.join([f'{k}={targs[k]}' for k, v in rendered.items() if '{{' in v])}" - ) + raise UnresolvableTemplate(f"Could not resolve inputs {targs}, they became: {rendered}") return rendered diff --git a/src/admin/tests/test_render_targs.py b/src/admin/tests/test_render_targs.py index b6b5117e87..3906f1ba85 100644 --- a/src/admin/tests/test_render_targs.py +++ b/src/admin/tests/test_render_targs.py @@ -19,9 +19,29 @@ # You should have received a copy of the GNU General Public License +import jinja2 import unittest -from lsst.qserv.admin.cli.render_targs import render_targs, UnresolvableTemplate +from lsst.qserv.admin.cli.render_targs import ( + _get_vars, + render_targs, + UnresolvableTemplate, +) + + +class GetVarsTestCase(unittest.TestCase): + + def test(self): + self.assertEqual(_get_vars("{{foo}}"), ["foo"]) + self.assertEqual(_get_vars("abc {{foo}}"), ["foo"]) + self.assertEqual(_get_vars("{{foo}} abc"), ["foo"]) + self.assertEqual(_get_vars("{{foo}} {{bar}} abc"), ["foo", "bar"]) + self.assertEqual(_get_vars("{{foo}} abc {{bar}}"), ["foo", "bar"]) + self.assertEqual(_get_vars("{{ foo }} abc {{ bar}} {{baz }}"), ["foo", "bar", "baz"]) + # "foo bar" is not a legal var name and will fail to resolve to anything + # in the jira template, but it should work fine when creating the "vars" + # that are used in the template string. + self.assertEqual(_get_vars("{{foo bar}} abc"), ["foo bar"]) class RenderTargsTestCase(unittest.TestCase): @@ -38,6 +58,17 @@ def testCircularReference(self): render_targs({"a": "{{b}}", "b": "{{c}}", "c": "{{a}}"}) self.assertIn("a={{b}}, b={{c}}, c={{a}}", str(r.exception)) + def testCircularReference_varWithText(self): + """Test for failure when there is a circular reference in targs.""" + with self.assertRaises(UnresolvableTemplate) as r: + render_targs({"a": "{{b}}", "b": "foo {{c}}", "c": "{{a}}"}) + self.assertIn("a={{b}}, b=foo {{c}}, c={{a}}", str(r.exception)) + + def testCirularReference_twoVarsWithText(self): + with self.assertRaises(UnresolvableTemplate) as r: + render_targs({"a": "the {{b}}", "b": "only {{a}}"}) + self.assertIn("a=the {{b}}, b=only {{a}}", str(r.exception)) + def testSelfReference(self): """Test for failure when a targ refers to itself.""" with self.assertRaises(UnresolvableTemplate) as r: @@ -60,7 +91,7 @@ def testList(self): def testResolves(self): """Verify that a dict with legal values resolves correctly.""" self.assertEqual( - render_targs({"a": "{{b}}", "b": "{{c}}", "c": "d"}), + render_targs({"a": "{{ b }}", "b": "{{c}}", "c": "d"}), {"a": "d", "b": "d", "c": "d"} ) @@ -78,6 +109,13 @@ def testBool(self): {"a": True} ) + def testVarWithSpace(self): + """Verify spaces are not allowed inside of jinja template variables. + + (and we don't expect it).""" + with self.assertRaises(jinja2.exceptions.TemplateSyntaxError): + render_targs({"abc def": "foo", "ghi jkl": "{{abc def}}"}) + if __name__ == "__main__": unittest.main()