From 0e730aa80f2b76bc7ee34b3962212ebabfd1c5df Mon Sep 17 00:00:00 2001 From: Matthias Bernt Date: Tue, 11 Jan 2022 22:50:49 +0100 Subject: [PATCH 01/16] fix replacement of toplevel yield if there are multiple top level yield statements only the last was replaced --- lib/galaxy/util/xml_macros.py | 46 ++++++++++++++++++++++++++++++++--- 1 file changed, 42 insertions(+), 4 deletions(-) diff --git a/lib/galaxy/util/xml_macros.py b/lib/galaxy/util/xml_macros.py index ef4e55677789..5f6c933752ed 100644 --- a/lib/galaxy/util/xml_macros.py +++ b/lib/galaxy/util/xml_macros.py @@ -156,10 +156,49 @@ def _expand_macro(element, expand_el, macros, tokens): def _expand_yield_statements(macro_def, expand_el): + """ + >>> from lxml.etree import tostring + >>> from galaxy.util import XML + >>> expand_el = XML(''' + ... + ... + ... + ... ''') + >>> macro_def = XML(''' + ... + ... + ... + ... ''') + >>> _expand_yield_statements(macro_def, expand_el) + >>> print(tostring(macro_def).decode('UTF-8')) + + + + + + + + + >>> macro_def = XML(''' + ... + ... + ... + ... + ... + ... ''') + >>> _expand_yield_statements(macro_def, expand_el) + >>> print(tostring(macro_def).decode('UTF-8')) + + + + + + + + + """ yield_els = [yield_el for macro_def_el in macro_def for yield_el in macro_def_el.findall('.//yield')] - expand_el_children = list(expand_el) - for yield_el in yield_els: _xml_replace(yield_el, expand_el_children) @@ -170,10 +209,9 @@ def _expand_yield_statements(macro_def, expand_el): if macro_def_el.tag == "yield": for target in expand_el_children: i += 1 - macro_def.insert(i, target) + macro_def.insert(i, deepcopy(target)) macro_def.remove(macro_def_el) continue - replace_yield = False From 44855fe631dd4c2942cb6a5a1a48fd965565ba7b Mon Sep 17 00:00:00 2001 From: Matthias Bernt Date: Tue, 11 Jan 2022 23:00:24 +0100 Subject: [PATCH 02/16] remove apparently unnecessart code --- lib/galaxy/util/xml_macros.py | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/lib/galaxy/util/xml_macros.py b/lib/galaxy/util/xml_macros.py index 5f6c933752ed..3d7a48750f66 100644 --- a/lib/galaxy/util/xml_macros.py +++ b/lib/galaxy/util/xml_macros.py @@ -203,16 +203,12 @@ def _expand_yield_statements(macro_def, expand_el): _xml_replace(yield_el, expand_el_children) # Replace yields at the top level of a macro, seems hacky approach - replace_yield = True - while replace_yield: - for i, macro_def_el in enumerate(macro_def): - if macro_def_el.tag == "yield": - for target in expand_el_children: - i += 1 - macro_def.insert(i, deepcopy(target)) - macro_def.remove(macro_def_el) - continue - replace_yield = False + for i, macro_def_el in enumerate(macro_def): + if macro_def_el.tag == "yield": + for target in expand_el_children: + i += 1 + macro_def.insert(i, deepcopy(target)) + macro_def.remove(macro_def_el) def _load_macros(macros_el, xml_base_dir): From c3d3727a005a870c5b957e8a60e1d857b2fa46b8 Mon Sep 17 00:00:00 2001 From: Matthias Bernt Date: Tue, 11 Jan 2022 23:07:44 +0100 Subject: [PATCH 03/16] even simpler _expand_yield_statements --- lib/galaxy/util/xml_macros.py | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/lib/galaxy/util/xml_macros.py b/lib/galaxy/util/xml_macros.py index 3d7a48750f66..b08e0fc92a84 100644 --- a/lib/galaxy/util/xml_macros.py +++ b/lib/galaxy/util/xml_macros.py @@ -197,19 +197,11 @@ def _expand_yield_statements(macro_def, expand_el): """ - yield_els = [yield_el for macro_def_el in macro_def for yield_el in macro_def_el.findall('.//yield')] + yield_els = [yield_el for yield_el in macro_def.findall('.//yield')] expand_el_children = list(expand_el) for yield_el in yield_els: _xml_replace(yield_el, expand_el_children) - # Replace yields at the top level of a macro, seems hacky approach - for i, macro_def_el in enumerate(macro_def): - if macro_def_el.tag == "yield": - for target in expand_el_children: - i += 1 - macro_def.insert(i, deepcopy(target)) - macro_def.remove(macro_def_el) - def _load_macros(macros_el, xml_base_dir): macros = [] From c47bd55e5b70d695e9a922d547f72e6320ca534c Mon Sep 17 00:00:00 2001 From: Matthias Bernt Date: Tue, 11 Jan 2022 23:13:32 +0100 Subject: [PATCH 04/16] add docstring --- lib/galaxy/util/xml_macros.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/galaxy/util/xml_macros.py b/lib/galaxy/util/xml_macros.py index b08e0fc92a84..66aba5b77b49 100644 --- a/lib/galaxy/util/xml_macros.py +++ b/lib/galaxy/util/xml_macros.py @@ -157,6 +157,9 @@ def _expand_macro(element, expand_el, macros, tokens): def _expand_yield_statements(macro_def, expand_el): """ + Modifies the macro_def element by replacing all tags below the + macro_def element by the children of the expand_el + >>> from lxml.etree import tostring >>> from galaxy.util import XML >>> expand_el = XML(''' From 8ba6795e8d41280499fe49be31ed25450b2a6cb9 Mon Sep 17 00:00:00 2001 From: Matthias Bernt Date: Wed, 12 Jan 2022 12:13:46 +0100 Subject: [PATCH 05/16] XmlMacroDef: store the macro element instead of the list of children mv parsing of token_quote out of the for loop, since otherwise tokens that are parsed before token_quote will get the default and only the ones after will get the desired token_quote --- lib/galaxy/util/xml_macros.py | 34 ++++++++++++++++++++++++++-------- 1 file changed, 26 insertions(+), 8 deletions(-) diff --git a/lib/galaxy/util/xml_macros.py b/lib/galaxy/util/xml_macros.py index 66aba5b77b49..d8627f1317b4 100644 --- a/lib/galaxy/util/xml_macros.py +++ b/lib/galaxy/util/xml_macros.py @@ -142,8 +142,7 @@ def _expand_macro(element, expand_el, macros, tokens): assert macro_name is not None, "Attempted to expand macro with no 'macro' attribute defined." assert macro_name in macros, f"No macro named {macro_name} found, known macros are {', '.join(macros.keys())}." macro_def = macros[macro_name] - expanded_elements = deepcopy(macro_def.elements) - + expanded_elements = deepcopy(macro_def.element) _expand_yield_statements(expanded_elements, expand_el) # Recursively expand contained macros. @@ -299,15 +298,29 @@ def _xml_replace(query, targets): class XmlMacroDef: + """ + representation of a (Galaxy) XML macro + + stores the root element of the macro and the parameters. + each parameter is represented as pair containing + - the quote character, default '@' + - parameter name + parameter names can be given as comma separated list using the + `token` attribute or as attributes `token_XXX` (where `XXX` is the name). + The former option should be used to specify required attributes of the + macro and the latter for optional attributes if the macro (the value of + `token_XXX is used as default value). + + TODO: `token_quote` forbids `"quote"` as character name of optional + parameters + """ def __init__(self, el): - self.elements = list(el) + self.element = el parameters = {} tokens = [] - token_quote = "@" + token_quote = el.attrib.get("token_quote", "@") for key, value in el.attrib.items(): - if key == "token_quote": - token_quote = value if key == "tokens": for token in value.split(","): tokens.append((token, REQUIRED_PARAMETER)) @@ -319,12 +332,17 @@ def __init__(self, el): self.parameters = parameters def macro_tokens(self, expand_el): + """ + get a dictionary mapping token names to values. The names are the + parameter names surrounded by the quote character. Values are taken + from the expand_el if absent default values of optional parameters are + used. + """ tokens = {} for key, (wrap_char, default_val) in self.parameters.items(): token_value = expand_el.attrib.get(key, default_val) if token_value is REQUIRED_PARAMETER: - message = "Failed to expand macro - missing required parameter [%s]." - raise ValueError(message % key) + raise ValueError(f"Failed to expand macro - missing required parameter [{key}].") token_name = f"{wrap_char}{key.upper()}{wrap_char}" tokens[token_name] = token_value return tokens From 0dfcf487a6ac7ca9b96bcb5e261aaadee2502914 Mon Sep 17 00:00:00 2001 From: Matthias Bernt Date: Wed, 12 Jan 2022 17:55:03 +0100 Subject: [PATCH 06/16] fix unit test use galaxy.util.xml_to_string for export since lxml is not always available? --- lib/galaxy/util/xml_macros.py | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/lib/galaxy/util/xml_macros.py b/lib/galaxy/util/xml_macros.py index d8627f1317b4..a1c3ad96f21f 100644 --- a/lib/galaxy/util/xml_macros.py +++ b/lib/galaxy/util/xml_macros.py @@ -159,8 +159,7 @@ def _expand_yield_statements(macro_def, expand_el): Modifies the macro_def element by replacing all tags below the macro_def element by the children of the expand_el - >>> from lxml.etree import tostring - >>> from galaxy.util import XML + >>> from galaxy.util import XML, xml_to_string >>> expand_el = XML(''' ... ... @@ -172,12 +171,15 @@ def _expand_yield_statements(macro_def, expand_el): ... ... ''') >>> _expand_yield_statements(macro_def, expand_el) - >>> print(tostring(macro_def).decode('UTF-8')) + >>> print(xml_to_string(macro_def, pretty=True)) + - + + - + + @@ -189,15 +191,16 @@ def _expand_yield_statements(macro_def, expand_el): ... ... ''') >>> _expand_yield_statements(macro_def, expand_el) - >>> print(tostring(macro_def).decode('UTF-8')) + >>> print(xml_to_string(macro_def, pretty=True)) + - + - - + + """ yield_els = [yield_el for yield_el in macro_def.findall('.//yield')] expand_el_children = list(expand_el) From be7a9a9bfce4c13e74b27b0c6921eb510de018eb Mon Sep 17 00:00:00 2001 From: Matthias Bernt Date: Wed, 12 Jan 2022 19:13:53 +0100 Subject: [PATCH 07/16] implement named yields yield tags with a name attribute will be replaced ``` ...... ``` by the content of the token with the corresponding name: ``` ...CONTENT... ``` The token must be direct child of the expand tag like for unnamed yields all yields with the same name will be replaced by the token's content. Named yields are replaced befor unnamed yields. When replacing unnamed yields tokens are ignored. --- lib/galaxy/util/xml_macros.py | 48 ++++++++++++++++++++++++++++++----- 1 file changed, 41 insertions(+), 7 deletions(-) diff --git a/lib/galaxy/util/xml_macros.py b/lib/galaxy/util/xml_macros.py index a1c3ad96f21f..3b78da239c2b 100644 --- a/lib/galaxy/util/xml_macros.py +++ b/lib/galaxy/util/xml_macros.py @@ -63,6 +63,10 @@ def imported_macro_paths(root): def _import_macros(root, path): + """ + root the parsed XML tree + path the path to the main xml document + """ xml_base_dir = os.path.dirname(path) macros_el = _macros_el(root) if macros_el is not None: @@ -162,13 +166,22 @@ def _expand_yield_statements(macro_def, expand_el): >>> from galaxy.util import XML, xml_to_string >>> expand_el = XML(''' ... + ... + ... + ... + ... ... + ... + ... + ... + ... ... ... ''') >>> macro_def = XML(''' ... ... - ... + ... + ... ... ''') >>> _expand_yield_statements(macro_def, expand_el) >>> print(xml_to_string(macro_def, pretty=True)) @@ -178,17 +191,24 @@ def _expand_yield_statements(macro_def, expand_el): + + + + + >>> # test replacement of top level yields >>> macro_def = XML(''' ... ... ... - ... - ... + ... + ... + ... + ... ... ''') >>> _expand_yield_statements(macro_def, expand_el) >>> print(xml_to_string(macro_def, pretty=True)) @@ -197,13 +217,27 @@ def _expand_yield_statements(macro_def, expand_el): - - - + + + + + + """ + # replace named yields + for token_el in expand_el.findall('./token'): + name = token_el.attrib.get("name", None) + assert name is not None, "Found unnamed token" + str(token_el.attrib) + yield_els = [yield_el for yield_el in macro_def.findall(f".//yield[@name='{name}']")] + assert len(yield_els) > 0, f"No named yield found for named token {name}" + token_el_children = list(token_el) + for yield_el in yield_els: + _xml_replace(yield_el, token_el_children) + + # replace unnamed yields yield_els = [yield_el for yield_el in macro_def.findall('.//yield')] - expand_el_children = list(expand_el) + expand_el_children = [c for c in expand_el if c.tag != "token"] for yield_el in yield_els: _xml_replace(yield_el, expand_el_children) From bd64bfd2a31f34118207e4956ffbc6ce4ab2ad47 Mon Sep 17 00:00:00 2001 From: Matthias Bernt Date: Wed, 12 Jan 2022 20:46:30 +0100 Subject: [PATCH 08/16] add test for the limited recursion possibilities --- lib/galaxy/util/xml_macros.py | 29 ++++++++++++++++++++++++++++- 1 file changed, 28 insertions(+), 1 deletion(-) diff --git a/lib/galaxy/util/xml_macros.py b/lib/galaxy/util/xml_macros.py index 3b78da239c2b..96a088a76c06 100644 --- a/lib/galaxy/util/xml_macros.py +++ b/lib/galaxy/util/xml_macros.py @@ -64,7 +64,7 @@ def imported_macro_paths(root): def _import_macros(root, path): """ - root the parsed XML tree + root the parsed XML tree path the path to the main xml document """ xml_base_dir = os.path.dirname(path) @@ -224,6 +224,33 @@ def _expand_yield_statements(macro_def, expand_el): + >>> # test recursion like replacements + >>> expand_el = XML(''' + ... + ... + ... + ... + ... + ... + ... + ... + ... ''') + >>> macro_def = XML(''' + ... + ... + ... ''') + >>> _expand_yield_statements(macro_def, expand_el) + >>> print(xml_to_string(macro_def, pretty=True)) + + + + + + + + + + """ # replace named yields for token_el in expand_el.findall('./token'): From c1604f509b32a48dcbd3d58bb85558f6b38cf7b5 Mon Sep 17 00:00:00 2001 From: Matthias Bernt Date: Thu, 13 Jan 2022 11:06:44 +0100 Subject: [PATCH 09/16] use more xpath in test_tool_loader got myself used to the unit test and thought it might be nice to use more xpath undo a unnecessary change in xml_macros.py --- lib/galaxy/util/xml_macros.py | 4 ++- test/unit/tool_util/test_tool_loader.py | 41 +++++++++++++------------ 2 files changed, 24 insertions(+), 21 deletions(-) diff --git a/lib/galaxy/util/xml_macros.py b/lib/galaxy/util/xml_macros.py index 96a088a76c06..fdec287eb741 100644 --- a/lib/galaxy/util/xml_macros.py +++ b/lib/galaxy/util/xml_macros.py @@ -383,8 +383,10 @@ def __init__(self, el): self.element = el parameters = {} tokens = [] - token_quote = el.attrib.get("token_quote", "@") + token_quote = "@" for key, value in el.attrib.items(): + if key == "token_quote": + token_quote = value if key == "tokens": for token in value.split(","): tokens.append((token, REQUIRED_PARAMETER)) diff --git a/test/unit/tool_util/test_tool_loader.py b/test/unit/tool_util/test_tool_loader.py index 562a9e75bfdf..b42d57a4e384 100644 --- a/test/unit/tool_util/test_tool_loader.py +++ b/test/unit/tool_util/test_tool_loader.py @@ -4,7 +4,7 @@ from galaxy.tool_util.loader import load_tool, template_macro_params from galaxy.tool_util.unittest_utils.sample_data import SIMPLE_MACRO, SIMPLE_TOOL_WITH_MACRO -from galaxy.util import parse_xml +from galaxy.util import parse_xml, xml_to_string def test_loader(): @@ -84,12 +84,11 @@ def load(self, name="tool.xml", preprocess=True): ''') xml = tool_dir.load() - assert xml.findall("inputs")[0].find("input").get("name") == "first_input" - assert xml.findall("inputs")[1].find("input").get("name") == "second_input" - assert xml.findall("inputs")[2].find("input").get("name") == "third_input" + assert xml.find("/inputs[1]/input").get("name") == "first_input" + assert xml.find("/inputs[2]/input").get("name") == "second_input" + assert xml.find("/inputs[3]/input").get("name") == "third_input" # Test nested macro with yield statements - with TestToolDirectory() as tool_dir: tool_dir.write(""" @@ -111,11 +110,16 @@ def load(self, name="tool.xml", preprocess=True): - + + + """) xml = tool_dir.load() + # assert the both yields in the inner macro (paired_options) are expanded + assert xml.find('/inputs/conditional[@name="library"]/when[@value="paired"]/param[@name="test"]') is not None + assert xml.find('/inputs/conditional[@name="library"]/when[@value="paired_collection"]/param[@name="test"]') is not None # Test recursive macro applications. with TestToolDirectory() as tool_dir: @@ -138,9 +142,9 @@ def load(self, name="tool.xml", preprocess=True): ''') xml = tool_dir.load() - assert xml.find("inputs").findall("input")[0].get("name") == "first_input" - assert xml.find("inputs").findall("input")[1].get("name") == "second_input" - assert xml.find("inputs").findall("input")[2].get("name") == "third_input" + assert xml.find("/inputs/input[1]").get("name") == "first_input" + assert xml.find("/inputs/input[2]").get("name") == "second_input" + assert xml.find("/inputs/input[3]").get("name") == "third_input" # Test recursive macro applications. with TestToolDirectory() as tool_dir: @@ -166,10 +170,11 @@ def load(self, name="tool.xml", preprocess=True): ''') xml = tool_dir.load() - assert xml.find("inputs").findall("input")[0].get("name") == "first_input" - assert xml.find("inputs").findall("input")[1].get("name") == "second_input" - assert xml.find("inputs").findall("input")[2].get("name") == "third_input" + assert xml.find("/inputs/input[1]").get("name") == "first_input" + assert xml.find("/inputs/input[2]").get("name") == "second_input" + assert xml.find("/inputs/input[3]").get("name") == "third_input" + # test expansion of top level (ie child of ) yields with TestToolDirectory() as tool_dir: tool_dir.write(''' @@ -186,8 +191,8 @@ def load(self, name="tool.xml", preprocess=True): ''') xml = tool_dir.load() - assert xml.find("inputs").findall("param")[0].get("name") == "a1" - assert xml.find("inputs").findall("param")[1].get("name") == "b" + assert xml.find("/inputs/param[1]").get("name") == "a1" + assert xml.find("/inputs/param[2]").get("name") == "b" # Test is shortcut for macro type="xml" with TestToolDirectory() as tool_dir: @@ -246,9 +251,7 @@ def load(self, name="tool.xml", preprocess=True): ''') xml = tool_dir.load() - tag_el = xml.find("another").find("tag") - value = tag_el.get('value') - assert value == "The value.", value + assert xml.find('/another/tag[@value="The value."]') is not None with TestToolDirectory() as tool_dir: tool_dir.write(''' @@ -262,9 +265,7 @@ def load(self, name="tool.xml", preprocess=True): ''') xml = tool_dir.load() - tag_el = xml.find("another").find("tag") - value = tag_el.get('value') - assert value == "", value + assert xml.find('/another/tag[@value=""]') is not None # Test macros XML macros with $$ expansions in attributes with TestToolDirectory() as tool_dir: From 7c64721e22ea6ca9b104fb0719cd970ecb1698f8 Mon Sep 17 00:00:00 2001 From: Matthias Bernt Date: Thu, 13 Jan 2022 12:23:30 +0100 Subject: [PATCH 10/16] move doctests to unit tests --- lib/galaxy/util/xml_macros.py | 97 +--------------- test/unit/tool_util/test_tool_loader.py | 141 ++++++++++++++++++++++++ 2 files changed, 147 insertions(+), 91 deletions(-) diff --git a/lib/galaxy/util/xml_macros.py b/lib/galaxy/util/xml_macros.py index fdec287eb741..b0bced9bf4b7 100644 --- a/lib/galaxy/util/xml_macros.py +++ b/lib/galaxy/util/xml_macros.py @@ -160,97 +160,12 @@ def _expand_macro(element, expand_el, macros, tokens): def _expand_yield_statements(macro_def, expand_el): """ - Modifies the macro_def element by replacing all tags below the - macro_def element by the children of the expand_el - - >>> from galaxy.util import XML, xml_to_string - >>> expand_el = XML(''' - ... - ... - ... - ... - ... - ... - ... - ... - ... - ... - ... - ... ''') - >>> macro_def = XML(''' - ... - ... - ... - ... - ... ''') - >>> _expand_yield_statements(macro_def, expand_el) - >>> print(xml_to_string(macro_def, pretty=True)) - - - - - - - - - - - - - - - - >>> # test replacement of top level yields - >>> macro_def = XML(''' - ... - ... - ... - ... - ... - ... - ... - ... ''') - >>> _expand_yield_statements(macro_def, expand_el) - >>> print(xml_to_string(macro_def, pretty=True)) - - - - - - - - - - - - - >>> # test recursion like replacements - >>> expand_el = XML(''' - ... - ... - ... - ... - ... - ... - ... - ... - ... ''') - >>> macro_def = XML(''' - ... - ... - ... ''') - >>> _expand_yield_statements(macro_def, expand_el) - >>> print(xml_to_string(macro_def, pretty=True)) - - - - - - - - - - + Modifies the macro_def element by replacing + + 1. all named yield tags by the content of the corresponding token tags + - token tags need to be direct children of the expand + - processed in order of definition of the token tags + 2. all unnamed yield tags by the non-token children of the expand tag """ # replace named yields for token_el in expand_el.findall('./token'): diff --git a/test/unit/tool_util/test_tool_loader.py b/test/unit/tool_util/test_tool_loader.py index b42d57a4e384..901276ddc39a 100644 --- a/test/unit/tool_util/test_tool_loader.py +++ b/test/unit/tool_util/test_tool_loader.py @@ -332,3 +332,144 @@ def load(self, name="tool.xml", preprocess=True): assert input_els[0].find("cow").text == "hello" assert input_els[1].find("cow").text == "world" assert input_els[2].find("cow").text == "the_default" + + # test expansion of named and unnamed yield + # - named yields are replaced by content of the corresponding token + # - unnamed yields are replaced by all non-token elements of the expand tag + with TestToolDirectory() as tool_dir: + tool_dir.write(''' + + + + + + + + + + + + + + + + + + + + + + + + + + +''') + xml = tool_dir.load() + assert xml_to_string(xml, pretty=True) == ''' + + + + + + + + + + + + + + + +''' + + # test replacement of multiple top level yield + with TestToolDirectory() as tool_dir: + tool_dir.write(''' + + + + + + + + + + + + + + + + + + + + + + + + +''') + xml = tool_dir.load() + assert xml_to_string(xml, pretty=True) == ''' + + + + + + + + + + + + +''' + + # test 'recursive' replacement with named yields + # since named yields are processed in order of the definition of the + # corresponding tokens: + # - replacing yield for token1 introduces yield for token2 + # - replacing yield for token2 introduced unnamed yield + # - replacing unnamed yield gives the only non-token element of the expand + with TestToolDirectory() as tool_dir: + tool_dir.write(''' + + + + + + + + + + + + + + + + + + + + + +''') + + xml = tool_dir.load() + print(xml_to_string(xml, pretty=True)) + assert xml_to_string(xml, pretty=True) == ''' + + + + + + + + + + +''' From 068f015bb40b4605afeb834387d3c2e741d3dbbe Mon Sep 17 00:00:00 2001 From: Matthias Bernt Date: Thu, 13 Jan 2022 22:10:45 +0100 Subject: [PATCH 11/16] some more extensions for xml macros 1. allow that the name of nested macros can be controlled by tokens: eg `` therefore: - in load_with_references childs of the macros element are temporarily removed. otherwise the replacement of the nested macros also happens for the expand tags in macros and it seems really difficult to iterate a tree excepting a subtree - in _expand_macro replace tokens first and then consider nested macros see test test_loader_specify_nested_macro_by_token 2. allow token expansion also for attributes (before only attribute values) this only works if the token is valid xml i.e. they can't be surrounded by `@` see test test_loader_token_attribute_name 3. split unit tests in separate functions and add a few (e.g. nested tokens ...) --- lib/galaxy/util/xml_macros.py | 47 ++++- test/unit/tool_util/test_tool_loader.py | 250 +++++++++++++++++++----- 2 files changed, 243 insertions(+), 54 deletions(-) diff --git a/lib/galaxy/util/xml_macros.py b/lib/galaxy/util/xml_macros.py index b0bced9bf4b7..e19f4d48126e 100644 --- a/lib/galaxy/util/xml_macros.py +++ b/lib/galaxy/util/xml_macros.py @@ -17,13 +17,29 @@ def load_with_references(path): macro_paths = _import_macros(root, path) + # temporarily remove the children of the macros node + # and create a copy. this is done because this allows + # to iterate over all expand nodes of the tree + # that are not included in the macros node + macros = _macros_el(root) + if macros is not None: + macros_copy = deepcopy(macros) + macros.clear() + else: + macros_copy = [] + # Collect tokens - tokens = _macros_of_type(root, 'token', lambda el: el.text or '') + tokens = _macros_of_type(macros_copy, 'token', lambda el: el.text or '') tokens = expand_nested_tokens(tokens) # Expand xml macros - macro_dict = _macros_of_type(root, 'xml', lambda el: XmlMacroDef(el)) + macro_dict = _macros_of_type(macros_copy, 'xml', lambda el: XmlMacroDef(el)) _expand_macros([root], macro_dict, tokens) + + # readd the stashed children of the macros node + # TODO is this this really necesary? Since macro nodes are removed anyway just below. + _xml_set_children(macros, list(macros_copy)) + for el in root.xpath('//macro'): if el.get('type') != 'template': # Only keep template macros @@ -80,7 +96,10 @@ def _macros_el(root): def _macros_of_type(root, type, el_func): - macros_el = root.find('macros') + if root.tag == "macros": + macros_el = root + else: + macros_el = root.find('macros') macro_dict = {} if macros_el is not None: macro_els = macros_el.findall('macro') @@ -110,6 +129,11 @@ def _expand_tokens(elements, tokens): def _expand_tokens_for_el(element, tokens): + """ + expand tokens in element and (recursively) in its children + replacements of text attributes and attribute values are + possible + """ value = element.text if value: new_value = _expand_tokens_str(element.text, tokens) @@ -119,6 +143,11 @@ def _expand_tokens_for_el(element, tokens): new_value = _expand_tokens_str(value, tokens) if not (new_value is value): element.attrib[key] = new_value + new_key = _expand_tokens_str(key, tokens) + if not (new_key is key): + element.attrib[new_key] = element.attrib[key] + del element.attrib[key] + # recursively expand in childrens _expand_tokens(list(element), tokens) @@ -138,10 +167,10 @@ def _expand_macros(elements, macros, tokens): expand_el = element.find('.//expand') if expand_el is None: break - _expand_macro(element, expand_el, macros, tokens) + _expand_macro(expand_el, macros, tokens) -def _expand_macro(element, expand_el, macros, tokens): +def _expand_macro(expand_el, macros, tokens): macro_name = expand_el.get('macro') assert macro_name is not None, "Attempted to expand macro with no 'macro' attribute defined." assert macro_name in macros, f"No macro named {macro_name} found, known macros are {', '.join(macros.keys())}." @@ -149,19 +178,18 @@ def _expand_macro(element, expand_el, macros, tokens): expanded_elements = deepcopy(macro_def.element) _expand_yield_statements(expanded_elements, expand_el) - # Recursively expand contained macros. - _expand_macros(expanded_elements, macros, tokens) macro_tokens = macro_def.macro_tokens(expand_el) if macro_tokens: _expand_tokens(expanded_elements, macro_tokens) + # Recursively expand contained macros. + _expand_macros(expanded_elements, macros, tokens) _xml_replace(expand_el, expanded_elements) def _expand_yield_statements(macro_def, expand_el): """ - Modifies the macro_def element by replacing - + Modifies the macro_def element by replacing 1. all named yield tags by the content of the corresponding token tags - token tags need to be direct children of the expand - processed in order of definition of the token tags @@ -232,7 +260,6 @@ def _load_imported_macros(macros_el, xml_base_dir): file_macros, current_macro_paths = _load_macro_file(import_path, xml_base_dir) macros.extend(file_macros) macro_paths.extend(current_macro_paths) - return macros, macro_paths diff --git a/test/unit/tool_util/test_tool_loader.py b/test/unit/tool_util/test_tool_loader.py index 901276ddc39a..8a381e001688 100644 --- a/test/unit/tool_util/test_tool_loader.py +++ b/test/unit/tool_util/test_tool_loader.py @@ -7,31 +7,33 @@ from galaxy.util import parse_xml, xml_to_string -def test_loader(): +class TestToolDirectory: + __test__ = False # Prevent pytest from discovering this class (issue #12071) - class TestToolDirectory: - __test__ = False # Prevent pytest from discovering this class (issue #12071) + def __init__(self): + self.temp_directory = mkdtemp() - def __init__(self): - self.temp_directory = mkdtemp() + def __enter__(self): + return self - def __enter__(self): - return self + def __exit__(self, type, value, tb): + rmtree(self.temp_directory) - def __exit__(self, type, value, tb): - rmtree(self.temp_directory) + def write(self, contents, name="tool.xml"): + open(os.path.join(self.temp_directory, name), "w").write(contents) - def write(self, contents, name="tool.xml"): - open(os.path.join(self.temp_directory, name), "w").write(contents) + def load(self, name="tool.xml", preprocess=True): + path = os.path.join(self.temp_directory, name) + if preprocess: + return load_tool(path) + else: + return parse_xml(path) - def load(self, name="tool.xml", preprocess=True): - path = os.path.join(self.temp_directory, name) - if preprocess: - return load_tool(path) - else: - return parse_xml(path) - # Test simple macro replacement. +def test_loader_simple(): + """ + Test simple macro replacement. + """ with TestToolDirectory() as tool_dir: tool_dir.write(''' @@ -47,7 +49,11 @@ def load(self, name="tool.xml", preprocess=True): xml = tool_dir.load(preprocess=True) assert xml.find("inputs") is not None - # Test importing macros from external files + +def test_loader_external(): + """ + Test importing macros from external files + """ with TestToolDirectory() as tool_dir: tool_dir.write(SIMPLE_TOOL_WITH_MACRO) @@ -57,6 +63,8 @@ def load(self, name="tool.xml", preprocess=True): xml = tool_dir.load(preprocess=True) assert xml.find("inputs") is not None + +def test_loader_unnamed_yield(): # Test macros with unnamed yield statements. with TestToolDirectory() as tool_dir: tool_dir.write(''' @@ -88,7 +96,11 @@ def load(self, name="tool.xml", preprocess=True): assert xml.find("/inputs[2]/input").get("name") == "second_input" assert xml.find("/inputs[3]/input").get("name") == "third_input" - # Test nested macro with yield statements + +def test_loader_unnamed_yield_nested(): + """ + Test nested macro with yield statements + """ with TestToolDirectory() as tool_dir: tool_dir.write(""" @@ -121,7 +133,11 @@ def load(self, name="tool.xml", preprocess=True): assert xml.find('/inputs/conditional[@name="library"]/when[@value="paired"]/param[@name="test"]') is not None assert xml.find('/inputs/conditional[@name="library"]/when[@value="paired_collection"]/param[@name="test"]') is not None - # Test recursive macro applications. + +def test_loader_recursive(): + """ + Test recursive macro applications. + """ with TestToolDirectory() as tool_dir: tool_dir.write(''' @@ -146,7 +162,11 @@ def load(self, name="tool.xml", preprocess=True): assert xml.find("/inputs/input[2]").get("name") == "second_input" assert xml.find("/inputs/input[3]").get("name") == "third_input" - # Test recursive macro applications. + +def test_loader_recursive2(): + """ + Test recursive macro applications. + """ with TestToolDirectory() as tool_dir: tool_dir.write(''' @@ -174,7 +194,11 @@ def load(self, name="tool.xml", preprocess=True): assert xml.find("/inputs/input[2]").get("name") == "second_input" assert xml.find("/inputs/input[3]").get("name") == "third_input" - # test expansion of top level (ie child of ) yields + +def test_loader_toplevel_yield(): + """ + test expansion of top level (ie child of ) yields + """ with TestToolDirectory() as tool_dir: tool_dir.write(''' @@ -194,7 +218,11 @@ def load(self, name="tool.xml", preprocess=True): assert xml.find("/inputs/param[1]").get("name") == "a1" assert xml.find("/inputs/param[2]").get("name") == "b" - # Test is shortcut for macro type="xml" + +def test_loader_shortcut(): + """ + Test is shortcut for macro type="xml" + """ with TestToolDirectory() as tool_dir: tool_dir.write(''' @@ -208,6 +236,8 @@ def load(self, name="tool.xml", preprocess=True): xml = tool_dir.load() assert xml.find("inputs") is not None + +def test_loader_template(): with TestToolDirectory() as tool_dir: tool_dir.write(''' @@ -223,6 +253,8 @@ def load(self, name="tool.xml", preprocess=True): params_dict = template_macro_params(xml.getroot()) assert params_dict['tool_params'] == "-a 1 -b 2" + +def test_loader_token_text(): with TestToolDirectory() as tool_dir: tool_dir.write(''' @@ -239,6 +271,76 @@ def load(self, name="tool.xml", preprocess=True): help_el = xml.find("help") assert help_el.text == "The citation.", help_el.text + +def test_loader_token_nested(): + with TestToolDirectory() as tool_dir: + tool_dir.write(''' + + + Ab@HELPER@ra + ra@WITCH@ab + cad + + @WIZARD@ + + + + +''') + xml = tool_dir.load() + help_el = xml.find("help") + assert help_el.text == "Abracadabra", help_el.text + + +def test_loader_token_cycle(): + """ + test if cycles in nested tokens are detected + """ + with TestToolDirectory() as tool_dir: + tool_dir.write(''' + + + a @ROSE@ + rose @IS@ + a @A@ + + @A@ + + + + +''') + try: + tool_dir.load() + except Exception as e: + assert str(e) == "Token '@IS@' cannot contain itself" + else: + raise AssertionError("Cycle not detected, but then we are doomed anyway") + + +def test_loader_token_attribute_name(): + """ + test the repacement of an attribute name by a token + """ + with TestToolDirectory() as tool_dir: + tool_dir.write(''' + + + name + + + + + +''') + xml = tool_dir.load() + assert xml.find('/another/tag[@name="blah"]') is not None + + +def test_loader_token_attribute_value(): + """ + test the repacement of an attribute value by a token + """ with TestToolDirectory() as tool_dir: tool_dir.write(''' @@ -253,6 +355,8 @@ def load(self, name="tool.xml", preprocess=True): xml = tool_dir.load() assert xml.find('/another/tag[@value="The value."]') is not None + +def test_loader_token_empty(): with TestToolDirectory() as tool_dir: tool_dir.write(''' @@ -267,7 +371,11 @@ def load(self, name="tool.xml", preprocess=True): xml = tool_dir.load() assert xml.find('/another/tag[@value=""]') is not None - # Test macros XML macros with $$ expansions in attributes + +def test_loader_macro_token_quote(): + """ + Test macros XML macros with $$ expansions in attributes + """ with TestToolDirectory() as tool_dir: tool_dir.write(''' @@ -288,7 +396,11 @@ def load(self, name="tool.xml", preprocess=True): assert input_els[1].attrib["type"] == "the type is my awesome" assert input_els[2].attrib["type"] == "the type is doggo" - # Test macros XML macros with @ expansions in text + +def test_loader_macro_token(): + """ + Test macros XML macros with @ expansions in text + """ with TestToolDirectory() as tool_dir: tool_dir.write(''' @@ -309,7 +421,11 @@ def load(self, name="tool.xml", preprocess=True): assert input_els[1].text == "world" assert input_els[2].text == "the_default" - # Test macros XML macros with @ expansions and recursive + +def test_loader_macro_token_recursive(): + """ + Test macros XML macros with @ expansions and recursive + """ with TestToolDirectory() as tool_dir: tool_dir.write(''' @@ -333,9 +449,13 @@ def load(self, name="tool.xml", preprocess=True): assert input_els[1].find("cow").text == "world" assert input_els[2].find("cow").text == "the_default" - # test expansion of named and unnamed yield - # - named yields are replaced by content of the corresponding token - # - unnamed yields are replaced by all non-token elements of the expand tag + +def test_loader_macro_named_yield(): + """ + test expansion of named and unnamed yield + - named yields are replaced by content of the corresponding token + - unnamed yields are replaced by all non-token elements of the expand tag + """ with TestToolDirectory() as tool_dir: tool_dir.write(''' @@ -368,8 +488,7 @@ def load(self, name="tool.xml", preprocess=True): xml = tool_dir.load() assert xml_to_string(xml, pretty=True) == ''' - - + @@ -384,7 +503,11 @@ def load(self, name="tool.xml", preprocess=True): ''' - # test replacement of multiple top level yield + +def test_loader_macro_multiple_toplevel_yield(): + """ + test replacement of multiple top level yield + """ with TestToolDirectory() as tool_dir: tool_dir.write(''' @@ -415,8 +538,7 @@ def load(self, name="tool.xml", preprocess=True): xml = tool_dir.load() assert xml_to_string(xml, pretty=True) == ''' - - + @@ -428,12 +550,16 @@ def load(self, name="tool.xml", preprocess=True): ''' - # test 'recursive' replacement with named yields - # since named yields are processed in order of the definition of the - # corresponding tokens: - # - replacing yield for token1 introduces yield for token2 - # - replacing yield for token2 introduced unnamed yield - # - replacing unnamed yield gives the only non-token element of the expand + +def test_loader_macro_recursive_named_yield(): + """ + test 'recursive' replacement with named yields + since named yields are processed in order of the definition of the + corresponding tokens: + - replacing yield for token1 introduces yield for token2 + - replacing yield for token2 introduced unnamed yield + - replacing unnamed yield gives the only non-token element of the expand + """ with TestToolDirectory() as tool_dir: tool_dir.write(''' @@ -460,11 +586,9 @@ def load(self, name="tool.xml", preprocess=True): ''') xml = tool_dir.load() - print(xml_to_string(xml, pretty=True)) assert xml_to_string(xml, pretty=True) == ''' - - + @@ -473,3 +597,41 @@ def load(self, name="tool.xml", preprocess=True): ''' + + +def test_loader_specify_nested_macro_by_token(): + """ + test if a nested macro can have a nested + macro specifying the macro name via a token + of the outer macro + """ + with TestToolDirectory() as tool_dir: + tool_dir.write(''' + + + external.xml + + + +''') + tool_dir.write(''' + + + + + + + + + + + +''', name="external.xml") + + xml = tool_dir.load() + assert xml_to_string(xml, pretty=True) == ''' + + + + +''' From 372439c088ee6b7b7b1a5751713891582928d39e Mon Sep 17 00:00:00 2001 From: Matthias Bernt Date: Fri, 14 Jan 2022 09:56:30 +0100 Subject: [PATCH 12/16] fix tool loader if no macros are present and add unit test for this case --- lib/galaxy/util/xml_macros.py | 37 ++++++++++++------------- test/unit/tool_util/test_tool_loader.py | 9 ++++++ 2 files changed, 26 insertions(+), 20 deletions(-) diff --git a/lib/galaxy/util/xml_macros.py b/lib/galaxy/util/xml_macros.py index e19f4d48126e..d126d4950871 100644 --- a/lib/galaxy/util/xml_macros.py +++ b/lib/galaxy/util/xml_macros.py @@ -21,12 +21,12 @@ def load_with_references(path): # and create a copy. this is done because this allows # to iterate over all expand nodes of the tree # that are not included in the macros node - macros = _macros_el(root) - if macros is not None: - macros_copy = deepcopy(macros) - macros.clear() + macros_el = _macros_el(root) + if macros_el is not None: + macros_copy = deepcopy(macros_el) + macros_el.clear() else: - macros_copy = [] + macros_copy = None # Collect tokens tokens = _macros_of_type(macros_copy, 'token', lambda el: el.text or '') @@ -38,7 +38,8 @@ def load_with_references(path): # readd the stashed children of the macros node # TODO is this this really necesary? Since macro nodes are removed anyway just below. - _xml_set_children(macros, list(macros_copy)) + if macros_copy: + _xml_set_children(macros_el, list(macros_copy)) for el in root.xpath('//macro'): if el.get('type') != 'template': @@ -59,7 +60,8 @@ def template_macro_params(root): with these. """ param_dict = {} - macro_dict = _macros_of_type(root, 'template', lambda el: el.text) + macros_el = _macros_el(root) + macro_dict = _macros_of_type(macros_el, 'template', lambda el: el.text) for key, value in macro_dict.items(): param_dict[key] = value return param_dict @@ -95,19 +97,14 @@ def _macros_el(root): return root.find('macros') -def _macros_of_type(root, type, el_func): - if root.tag == "macros": - macros_el = root - else: - macros_el = root.find('macros') - macro_dict = {} - if macros_el is not None: - macro_els = macros_el.findall('macro') - filtered_els = [(macro_el.get("name"), el_func(macro_el)) - for macro_el in macro_els - if macro_el.get('type') == type] - macro_dict = dict(filtered_els) - return macro_dict +def _macros_of_type(macros_el, type, el_func): + if macros_el is None: + return {} + macro_els = macros_el.findall('macro') + filtered_els = [(macro_el.get("name"), el_func(macro_el)) + for macro_el in macro_els + if macro_el.get('type') == type] + return dict(filtered_els) def expand_nested_tokens(tokens): diff --git a/test/unit/tool_util/test_tool_loader.py b/test/unit/tool_util/test_tool_loader.py index 8a381e001688..a4bbd7fffc66 100644 --- a/test/unit/tool_util/test_tool_loader.py +++ b/test/unit/tool_util/test_tool_loader.py @@ -30,6 +30,15 @@ def load(self, name="tool.xml", preprocess=True): return parse_xml(path) +def test_no_macros(): + """ + Test tool loaded in absence of a macros node. + """ + with TestToolDirectory() as tool_dir: + tool_dir.write('') + tool_dir.load(preprocess=True) + + def test_loader_simple(): """ Test simple macro replacement. From 180ed75760c7fe0eefa309694a65b3ae457c779d Mon Sep 17 00:00:00 2001 From: Matthias Bernt Date: Fri, 14 Jan 2022 11:10:37 +0100 Subject: [PATCH 13/16] check for cycles in nested macros --- lib/galaxy/util/xml_macros.py | 18 +++++++++++++---- test/unit/tool_util/test_tool_loader.py | 27 +++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 4 deletions(-) diff --git a/lib/galaxy/util/xml_macros.py b/lib/galaxy/util/xml_macros.py index d126d4950871..46c0771b5bda 100644 --- a/lib/galaxy/util/xml_macros.py +++ b/lib/galaxy/util/xml_macros.py @@ -155,7 +155,7 @@ def _expand_tokens_str(s, tokens): return s -def _expand_macros(elements, macros, tokens): +def _expand_macros(elements, macros, tokens, visited=None): if not macros and not tokens: return @@ -164,12 +164,21 @@ def _expand_macros(elements, macros, tokens): expand_el = element.find('.//expand') if expand_el is None: break - _expand_macro(expand_el, macros, tokens) + if visited is None: + v = list() + else: + v = visited + _expand_macro(expand_el, macros, tokens, v) -def _expand_macro(expand_el, macros, tokens): +def _expand_macro(expand_el, macros, tokens, visited): macro_name = expand_el.get('macro') assert macro_name is not None, "Attempted to expand macro with no 'macro' attribute defined." + + # check for cycles in the nested macro expansion + assert macro_name not in visited, f"Cycle in nested macros: already expanded {visited} can't expand '{macro_name}' again" + visited.append(macro_name) + assert macro_name in macros, f"No macro named {macro_name} found, known macros are {', '.join(macros.keys())}." macro_def = macros[macro_name] expanded_elements = deepcopy(macro_def.element) @@ -180,8 +189,9 @@ def _expand_macro(expand_el, macros, tokens): _expand_tokens(expanded_elements, macro_tokens) # Recursively expand contained macros. - _expand_macros(expanded_elements, macros, tokens) + _expand_macros(expanded_elements, macros, tokens, visited) _xml_replace(expand_el, expanded_elements) + del visited[-1] def _expand_yield_statements(macro_def, expand_el): diff --git a/test/unit/tool_util/test_tool_loader.py b/test/unit/tool_util/test_tool_loader.py index a4bbd7fffc66..6233fcec7422 100644 --- a/test/unit/tool_util/test_tool_loader.py +++ b/test/unit/tool_util/test_tool_loader.py @@ -644,3 +644,30 @@ def test_loader_specify_nested_macro_by_token(): ''' + + +def test_loader_circular_macros(): + """ + check the cycles in nested macros are detected + """ + with TestToolDirectory() as tool_dir: + tool_dir.write(''' + + + + + + + + + + + + + + +''') + try: + tool_dir.load() + except AssertionError as a: + assert str(a) == "Cycle in nested macros: already expanded ['a', 'b'] can't expand 'a' again" From 5198c82fe90eb930b2d532dade09d2bede2b5b20 Mon Sep 17 00:00:00 2001 From: M Bernt Date: Fri, 14 Jan 2022 14:44:45 +0100 Subject: [PATCH 14/16] Update lib/galaxy/util/xml_macros.py Co-authored-by: Marius van den Beek --- lib/galaxy/util/xml_macros.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/galaxy/util/xml_macros.py b/lib/galaxy/util/xml_macros.py index 46c0771b5bda..1ed30f19986d 100644 --- a/lib/galaxy/util/xml_macros.py +++ b/lib/galaxy/util/xml_macros.py @@ -165,7 +165,7 @@ def _expand_macros(elements, macros, tokens, visited=None): if expand_el is None: break if visited is None: - v = list() + v = [] else: v = visited _expand_macro(expand_el, macros, tokens, v) From 72141b34e65a81e53a8d45ff6fc3665f014d1060 Mon Sep 17 00:00:00 2001 From: Matthias Bernt Date: Sun, 16 Jan 2022 16:42:24 +0100 Subject: [PATCH 15/16] use copy instead of deepcopy --- lib/galaxy/util/xml_macros.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/galaxy/util/xml_macros.py b/lib/galaxy/util/xml_macros.py index 1ed30f19986d..2aedd71ea512 100644 --- a/lib/galaxy/util/xml_macros.py +++ b/lib/galaxy/util/xml_macros.py @@ -1,5 +1,5 @@ import os -from copy import deepcopy +from copy import copy, deepcopy from galaxy.util import parse_xml @@ -23,7 +23,7 @@ def load_with_references(path): # that are not included in the macros node macros_el = _macros_el(root) if macros_el is not None: - macros_copy = deepcopy(macros_el) + macros_copy = copy(macros_el) macros_el.clear() else: macros_copy = None From a216858e2767265a30499a7b5a1018fdfd3a787d Mon Sep 17 00:00:00 2001 From: Matthias Bernt Date: Wed, 19 Jan 2022 15:08:25 +0100 Subject: [PATCH 16/16] fix deprecation warning if macros copy is actually an `Element` then this gives a deprecation warning: xml_macros.py:41: FutureWarning: The behavior of this method will change in future versions. Use specific 'len(elem)' or 'elem is not None' test instead. if macros_copy: --- lib/galaxy/util/xml_macros.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/galaxy/util/xml_macros.py b/lib/galaxy/util/xml_macros.py index 2aedd71ea512..24883e96f2c0 100644 --- a/lib/galaxy/util/xml_macros.py +++ b/lib/galaxy/util/xml_macros.py @@ -38,7 +38,7 @@ def load_with_references(path): # readd the stashed children of the macros node # TODO is this this really necesary? Since macro nodes are removed anyway just below. - if macros_copy: + if macros_copy is not None: _xml_set_children(macros_el, list(macros_copy)) for el in root.xpath('//macro'):