diff --git a/lib/galaxy/util/xml_macros.py b/lib/galaxy/util/xml_macros.py index ef4e5567778..24883e96f2c 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 @@ -17,13 +17,30 @@ 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_el = _macros_el(root) + if macros_el is not None: + macros_copy = copy(macros_el) + macros_el.clear() + else: + macros_copy = None + # 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. + if macros_copy is not None: + _xml_set_children(macros_el, list(macros_copy)) + for el in root.xpath('//macro'): if el.get('type') != 'template': # Only keep template macros @@ -43,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 @@ -63,6 +81,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: @@ -75,16 +97,14 @@ def _macros_el(root): return root.find('macros') -def _macros_of_type(root, type, el_func): - 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): @@ -106,6 +126,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) @@ -115,6 +140,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) @@ -125,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 @@ -134,48 +164,60 @@ def _expand_macros(elements, macros, tokens): expand_el = element.find('.//expand') if expand_el is None: break - _expand_macro(element, expand_el, macros, tokens) + if visited is None: + v = [] + else: + v = visited + _expand_macro(expand_el, macros, tokens, v) -def _expand_macro(element, 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.elements) - + 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, visited) _xml_replace(expand_el, expanded_elements) + del visited[-1] 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')] - - expand_el_children = list(expand_el) + """ + 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'): + 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 = [c for c in expand_el if c.tag != "token"] 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 - 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, target) - macro_def.remove(macro_def_el) - continue - - replace_yield = False - def _load_macros(macros_el, xml_base_dir): macros = [] @@ -225,7 +267,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 @@ -270,9 +311,25 @@ 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 = "@" @@ -290,12 +347,17 @@ class XmlMacroDef: 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 diff --git a/test/unit/tool_util/test_tool_loader.py b/test/unit/tool_util/test_tool_loader.py index 562a9e75bfd..6233fcec742 100644 --- a/test/unit/tool_util/test_tool_loader.py +++ b/test/unit/tool_util/test_tool_loader.py @@ -4,34 +4,45 @@ from tempfile import mkdtemp 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(): +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_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. + """ with TestToolDirectory() as tool_dir: tool_dir.write(''' @@ -47,7 +58,11 @@ def test_loader(): 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 +72,8 @@ def test_loader(): 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(''' @@ -84,12 +101,15 @@ def test_loader(): ''') 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 +def test_loader_unnamed_yield_nested(): + """ + Test nested macro with yield statements + """ with TestToolDirectory() as tool_dir: tool_dir.write(""" @@ -111,13 +131,22 @@ def test_loader(): - + + + """) 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. + +def test_loader_recursive(): + """ + Test recursive macro applications. + """ with TestToolDirectory() as tool_dir: tool_dir.write(''' @@ -138,11 +167,15 @@ def test_loader(): ''') 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. + +def test_loader_recursive2(): + """ + Test recursive macro applications. + """ with TestToolDirectory() as tool_dir: tool_dir.write(''' @@ -166,10 +199,15 @@ def test_loader(): ''') 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" + +def test_loader_toplevel_yield(): + """ + test expansion of top level (ie child of ) yields + """ with TestToolDirectory() as tool_dir: tool_dir.write(''' @@ -186,10 +224,14 @@ def test_loader(): ''') 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" + +def test_loader_shortcut(): + """ + Test is shortcut for macro type="xml" + """ with TestToolDirectory() as tool_dir: tool_dir.write(''' @@ -203,6 +245,8 @@ def test_loader(): xml = tool_dir.load() assert xml.find("inputs") is not None + +def test_loader_template(): with TestToolDirectory() as tool_dir: tool_dir.write(''' @@ -218,6 +262,8 @@ def test_loader(): 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(''' @@ -234,6 +280,76 @@ def test_loader(): 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(''' @@ -246,10 +362,10 @@ def test_loader(): ''') 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 + +def test_loader_token_empty(): with TestToolDirectory() as tool_dir: tool_dir.write(''' @@ -262,11 +378,13 @@ def test_loader(): ''') 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 + +def test_loader_macro_token_quote(): + """ + Test macros XML macros with $$ expansions in attributes + """ with TestToolDirectory() as tool_dir: tool_dir.write(''' @@ -287,7 +405,11 @@ def test_loader(): 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(''' @@ -308,7 +430,11 @@ def test_loader(): 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(''' @@ -331,3 +457,217 @@ def test_loader(): 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" + + +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(''' + + + + + + + + + + + + + + + + + + + + + + + + + + +''') + xml = tool_dir.load() + assert xml_to_string(xml, pretty=True) == ''' + + + + + + + + + + + + + + +''' + + +def test_loader_macro_multiple_toplevel_yield(): + """ + 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) == ''' + + + + + + + + + + + +''' + + +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(''' + + + + + + + + + + + + + + + + + + + + + +''') + + xml = tool_dir.load() + assert xml_to_string(xml, pretty=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) == ''' + + + + +''' + + +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"