Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Recursive replace #2864

Merged
merged 4 commits into from
Jan 16, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions 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`
masenf marked this conversation as resolved.
Show resolved Hide resolved
36 changes: 28 additions & 8 deletions src/tox/config/loader/ini/replace.py
Expand Up @@ -3,6 +3,7 @@
"""
from __future__ import annotations

import logging
import os
import re
import sys
Expand All @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a test for this with a circular reference of length 5, and one with 101?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a test that checks different recursion levels and asserts that the log message is triggered and that expansion stops at the correct level.



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:
Expand Down Expand Up @@ -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]
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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]
Expand Down
2 changes: 1 addition & 1 deletion tests/config/loader/ini/replace/test_replace_env_var.py
Expand Up @@ -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])
Expand Down
17 changes: 17 additions & 0 deletions tests/config/loader/ini/replace/test_replace_tox_env.py
Expand Up @@ -31,6 +31,23 @@ 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"


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")
Expand Down
2 changes: 1 addition & 1 deletion tests/config/test_set_env.py
Expand Up @@ -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


Expand Down