From 99b849baca40ca97d7af87ceeb5659edb99f0882 Mon Sep 17 00:00:00 2001 From: Masen Furer Date: Sun, 15 Jan 2023 20:45:17 -0800 Subject: [PATCH] Recursive replace (#2864) * test_replace_tox_env: add missing chain cases When a replacement references a replacement in a non-testenv section it should also be expanded * Recursive ini-value substitution Expand substitution expressions that result from a previous subsitution expression replacement value (up to 100 times). Fix #2863 * cr: changelog: fix trailing period * test_replace_tox_env: tests for MAX_REPLACE_DEPTH Create a long chain of substitution values and assert that they stop being processed after some time. --- docs/changelog/2863.bugfix.rst | 5 +++ src/tox/config/loader/ini/replace.py | 36 ++++++++++++---- .../ini/replace/test_replace_env_var.py | 2 +- .../ini/replace/test_replace_tox_env.py | 43 +++++++++++++++++++ tests/config/test_set_env.py | 2 +- 5 files changed, 78 insertions(+), 10 deletions(-) create mode 100644 docs/changelog/2863.bugfix.rst diff --git a/docs/changelog/2863.bugfix.rst b/docs/changelog/2863.bugfix.rst new file mode 100644 index 000000000..538229e45 --- /dev/null +++ b/docs/changelog/2863.bugfix.rst @@ -0,0 +1,5 @@ +Fix regression introduced in 4.3.0 by expanding substitution expressions +(``{...}``) that result from a previous subsitution's replacement value (up to +100 times). Note that recursive expansion is strictly depth-first; no +replacement value will ever affect adjacent characters nor will expansion ever +occur over the result of more than one replacement - by :user:`masenf`. diff --git a/src/tox/config/loader/ini/replace.py b/src/tox/config/loader/ini/replace.py index a1d3846e4..d856277cd 100644 --- a/src/tox/config/loader/ini/replace.py +++ b/src/tox/config/loader/ini/replace.py @@ -3,6 +3,7 @@ """ from __future__ import annotations +import logging import os import re import sys @@ -21,28 +22,39 @@ from tox.config.loader.ini import IniLoader from tox.config.main import Config + +LOGGER = logging.getLogger(__name__) + + # split alongside :, unless it's preceded by a single capital letter (Windows drive letter in paths) ARG_DELIMITER = ":" REPLACE_START = "{" REPLACE_END = "}" BACKSLASH_ESCAPE_CHARS = ["\\", ARG_DELIMITER, REPLACE_START, REPLACE_END, "[", "]"] +MAX_REPLACE_DEPTH = 100 MatchArg = Sequence[Union[str, "MatchExpression"]] +class MatchRecursionError(ValueError): + """Could not stabalize on replacement value.""" + + +class MatchError(Exception): + """Could not find end terminator in MatchExpression.""" + + def find_replace_expr(value: str) -> MatchArg: """Find all replaceable tokens within value.""" return MatchExpression.parse_and_split_to_terminator(value)[0][0] -def replace(conf: Config, loader: IniLoader, value: str, args: ConfigLoadArgs) -> str: +def replace(conf: Config, loader: IniLoader, value: str, args: ConfigLoadArgs, depth: int = 0) -> str: """Replace all active tokens within value according to the config.""" - return Replacer(conf, loader, conf_args=args).join(find_replace_expr(value)) - - -class MatchError(Exception): - """Could not find end terminator in MatchExpression.""" + if depth > MAX_REPLACE_DEPTH: + raise MatchRecursionError(f"Could not expand {value} after recursing {depth} frames") + return Replacer(conf, loader, conf_args=args, depth=depth).join(find_replace_expr(value)) class MatchExpression: @@ -153,10 +165,11 @@ def _flatten_string_fragments(seq_of_str_or_other: Sequence[str | Any]) -> Seque class Replacer: """Recursively expand MatchExpression against the config and loader.""" - def __init__(self, conf: Config, loader: IniLoader, conf_args: ConfigLoadArgs): + def __init__(self, conf: Config, loader: IniLoader, conf_args: ConfigLoadArgs, depth: int = 0): self.conf = conf self.loader = loader self.conf_args = conf_args + self.depth = depth def __call__(self, value: MatchArg) -> Sequence[str]: return [self._replace_match(me) if isinstance(me, MatchExpression) else str(me) for me in value] @@ -184,6 +197,13 @@ def _replace_match(self, value: MatchExpression) -> str: self.conf_args, ) if replace_value is not None: + needs_expansion = any(isinstance(m, MatchExpression) for m in find_replace_expr(replace_value)) + if needs_expansion: + try: + return replace(self.conf, self.loader, replace_value, self.conf_args, self.depth + 1) + except MatchRecursionError as err: + LOGGER.warning(str(err)) + return replace_value return replace_value # else: fall through -- when replacement is not possible, treat `{` as if escaped. # If we cannot replace, keep what was there, and continue looking for additional replaces @@ -302,7 +322,7 @@ def replace_env(conf: Config, args: list[str], conf_args: ConfigLoadArgs) -> str return set_env.load(key, conf_args) elif conf_args.chain[-1] != new_key: # if there's a chain but only self-refers than use os.environ circular = ", ".join(i[4:] for i in conf_args.chain[conf_args.chain.index(new_key) :]) - raise ValueError(f"circular chain between set env {circular}") + raise MatchRecursionError(f"circular chain between set env {circular}") if key in os.environ: return os.environ[key] diff --git a/tests/config/loader/ini/replace/test_replace_env_var.py b/tests/config/loader/ini/replace/test_replace_env_var.py index c9f26400d..d2e9fa580 100644 --- a/tests/config/loader/ini/replace/test_replace_env_var.py +++ b/tests/config/loader/ini/replace/test_replace_env_var.py @@ -102,7 +102,7 @@ def test_replace_env_var_circular_flip_flop(replace_one: ReplaceOne, monkeypatch monkeypatch.setenv("TRAGIC", "{env:MAGIC}") monkeypatch.setenv("MAGIC", "{env:TRAGIC}") result = replace_one("{env:MAGIC}") - assert result == "{env:TRAGIC}" + assert result == "{env:MAGIC}" @pytest.mark.parametrize("fallback", [True, False]) diff --git a/tests/config/loader/ini/replace/test_replace_tox_env.py b/tests/config/loader/ini/replace/test_replace_tox_env.py index 284d6c7cc..f7e84fde0 100644 --- a/tests/config/loader/ini/replace/test_replace_tox_env.py +++ b/tests/config/loader/ini/replace/test_replace_tox_env.py @@ -7,7 +7,9 @@ from tests.config.loader.ini.replace.conftest import ReplaceOne from tests.conftest import ToxIniCreator +from tox.config.loader.ini.replace import MAX_REPLACE_DEPTH from tox.config.sets import ConfigSet +from tox.pytest import LogCaptureFixture from tox.report import HandledError EnvConfigCreator = Callable[[str], ConfigSet] @@ -31,6 +33,47 @@ def test_replace_within_tox_env(example: EnvConfigCreator) -> None: assert result == "1" +def test_replace_within_tox_env_chain(example: EnvConfigCreator) -> None: + env_config = example("r = 1\no = {r}/2\np = {r} {o}") + env_config.add_config(keys="r", of_type=str, default="r", desc="r") + env_config.add_config(keys="o", of_type=str, default="o", desc="o") + env_config.add_config(keys="p", of_type=str, default="p", desc="p") + result = env_config["p"] + assert result == "1 1/2" + + +def test_replace_within_section_chain(tox_ini_conf: ToxIniCreator) -> None: + config = tox_ini_conf("[vars]\na = 1\nb = {[vars]a}/2\nc = {[vars]a}/3\n[testenv:a]\nd = {[vars]b} {[vars]c}") + env_config = config.get_env("a") + env_config.add_config(keys="d", of_type=str, default="d", desc="d") + result = env_config["d"] + assert result == "1/2 1/3" + + +@pytest.mark.parametrize("depth", [5, 99, 100, 101, 150, 256]) +def test_replace_within_section_chain_deep(caplog: LogCaptureFixture, tox_ini_conf: ToxIniCreator, depth: int) -> None: + config = tox_ini_conf( + "\n".join( + [ + "[vars]", + "a0 = 1", + *(f"a{ix} = {{[vars]a{ix - 1}}}" for ix in range(1, depth + 1)), + "[testenv:a]", + "b = {[vars]a%s}" % depth, + ], + ), + ) + env_config = config.get_env("a") + env_config.add_config(keys="b", of_type=str, default="b", desc="b") + result = env_config["b"] + if depth > MAX_REPLACE_DEPTH: + exp_stopped_at = "{[vars]a%s}" % (depth - MAX_REPLACE_DEPTH - 1) + assert result == exp_stopped_at + assert f"Could not expand {exp_stopped_at} after recursing {MAX_REPLACE_DEPTH + 1} frames" in caplog.messages + else: + assert result == "1" + + def test_replace_within_tox_env_missing_raises(example: EnvConfigCreator) -> None: env_config = example("o = {p}") env_config.add_config(keys="o", of_type=str, default="o", desc="o") diff --git a/tests/config/test_set_env.py b/tests/config/test_set_env.py index 453d73aef..ae12a5b8d 100644 --- a/tests/config/test_set_env.py +++ b/tests/config/test_set_env.py @@ -108,7 +108,7 @@ def test_set_env_circular_use_os_environ(tox_project: ToxProjectCreator) -> None prj = tox_project({"tox.ini": "[testenv]\npackage=skip\nset_env=a={env:b}\n b={env:a}"}) result = prj.run("c", "-e", "py") result.assert_success() - assert "replace failed in py.set_env with ValueError" in result.out, result.out + assert "replace failed in py.set_env with MatchRecursionError" in result.out, result.out assert "circular chain between set env a, b" in result.out, result.out