From d6fbfe360937c972cdca65e91cb19f0b1419c677 Mon Sep 17 00:00:00 2001
From: Matthew Seiler <m@flyinghyrax.net>
Date: Sun, 24 Mar 2024 19:30:32 -0400
Subject: [PATCH] Fix custom config reference syntax for `diff-file`

This replaces the implementation for custom configparser option
references in the diff-file option. Moves detection and handling for
the custom syntax into a dedicated submodule. Uses a regular expression
to preserve text before and after the reference.
---
 src/bandersnatch/config/__init__.py           |  0
 .../config/diff_file_reference.py             | 60 ++++++++++++++
 src/bandersnatch/configuration.py             | 20 +++--
 src/bandersnatch/tests/test_configuration.py  | 79 +++++++++++++++++++
 4 files changed, 148 insertions(+), 11 deletions(-)
 create mode 100644 src/bandersnatch/config/__init__.py
 create mode 100644 src/bandersnatch/config/diff_file_reference.py

diff --git a/src/bandersnatch/config/__init__.py b/src/bandersnatch/config/__init__.py
new file mode 100644
index 000000000..e69de29bb
diff --git a/src/bandersnatch/config/diff_file_reference.py b/src/bandersnatch/config/diff_file_reference.py
new file mode 100644
index 000000000..2d7a02ec4
--- /dev/null
+++ b/src/bandersnatch/config/diff_file_reference.py
@@ -0,0 +1,60 @@
+"""
+Custom configparser section/option reference syntax for the diff-file option.
+
+diff-file supports a "section reference" syntax for it's value:
+
+    [mirror]
+    ...
+    diff-file = /folder{{ <SECTION>_<OPTION> }}/more
+
+<SECTION> matches a configparser section and <OPTION> matches an option in that section.
+The portion of the diff-file value delimited with {{ and }} is replaced with the
+value from the specified option, which should be a string.
+
+The configparser module's ExtendedInterpolation feature is preferred to this custom syntax.
+"""
+
+import re
+from configparser import ConfigParser, NoOptionError, NoSectionError
+
+# Pattern to check if a string contains a custom section reference
+_LEGACY_REF_PAT = r".*\{\{.+_.+\}\}.*"
+
+# Pattern to decompose a custom section reference into parts
+_LEGACY_REF_COMPONENT_PAT = r"""
+    # capture everything from start-of-line to the opening '{{' braces into group 'pre'
+    ^(?P<pre>.*)\{\{
+    # restrict section names to ignore surrounding whitespace (different from
+    # configparser's default SECTRE) and disallow '_' (since that's our separator)
+    \s*
+    (?P<section>[^_\s](?:[^_]*[^_\s]))
+    # allow (but ignore) whitespace around the section-option delimiter
+    \s*_\s*
+    # option names ignore surrounding whitespace and can include any character, but
+    # must start and end with a non-whitespace character
+    (?P<option>\S(?:.*\S)?)
+    \s*
+    # capture from the closing '}}' braces to end-of-line into group 'post'
+    \}\}(?P<post>.*)$
+"""
+
+
+def has_legacy_config_ref(value: str) -> bool:
+    return re.match(_LEGACY_REF_PAT, value) is not None
+
+
+def eval_legacy_config_ref(config: ConfigParser, value: str) -> str:
+    match_result = re.match(_LEGACY_REF_COMPONENT_PAT, value, re.VERBOSE)
+
+    if match_result is None:
+        raise ValueError(f"Unable to parse config option reference from '{value}'")
+
+    pre, sect_name, opt_name, post = match_result.group(
+        "pre", "section", "option", "post"
+    )
+
+    try:
+        ref_value = config.get(sect_name, opt_name)
+        return pre + ref_value + post
+    except (NoSectionError, NoOptionError) as exc:
+        raise ValueError(exc.message)
diff --git a/src/bandersnatch/configuration.py b/src/bandersnatch/configuration.py
index f03ff597f..f7e94a84b 100644
--- a/src/bandersnatch/configuration.py
+++ b/src/bandersnatch/configuration.py
@@ -8,6 +8,7 @@
 from pathlib import Path
 from typing import Any, NamedTuple
 
+from .config.diff_file_reference import eval_legacy_config_ref, has_legacy_config_ref
 from .simple import SimpleDigest, SimpleFormat, get_digest_value, get_format_value
 
 logger = logging.getLogger("bandersnatch")
@@ -101,20 +102,17 @@ def validate_config_values(  # noqa: C901
         diff_file_path = config.get("mirror", "diff-file")
     except configparser.NoOptionError:
         diff_file_path = ""
-    if "{{" in diff_file_path and "}}" in diff_file_path:
-        diff_file_path = diff_file_path.replace("{{", "").replace("}}", "")
-        diff_ref_section, _, diff_ref_key = diff_file_path.partition("_")
+
+    if diff_file_path and has_legacy_config_ref(diff_file_path):
         try:
-            diff_file_path = config.get(diff_ref_section, diff_ref_key)
-        except (configparser.NoOptionError, configparser.NoSectionError):
+            diff_file_path = eval_legacy_config_ref(config, diff_file_path)
+        except ValueError as err:
             logger.error(
-                "Invalid section reference in `diff-file` key. "
-                "Please correct this error. Saving diff files in"
-                " base mirror directory."
-            )
-            diff_file_path = str(
-                Path(config.get("mirror", "directory")) / "mirrored-files"
+                "Invalid section reference in `diff-file` key: %s. Saving diff files in base mirror directory.",
+                str(err),
             )
+            mirror_dir = config.get("mirror", "directory")
+            diff_file_path = (Path(mirror_dir) / "mirrored-files").as_posix()
 
     try:
         diff_append_epoch = config.getboolean("mirror", "diff-append-epoch")
diff --git a/src/bandersnatch/tests/test_configuration.py b/src/bandersnatch/tests/test_configuration.py
index 92b8d384f..5cb2248a6 100644
--- a/src/bandersnatch/tests/test_configuration.py
+++ b/src/bandersnatch/tests/test_configuration.py
@@ -5,6 +5,7 @@
 from tempfile import TemporaryDirectory
 from unittest import TestCase
 
+from bandersnatch.config.diff_file_reference import eval_legacy_config_ref
 from bandersnatch.configuration import (
     BandersnatchConfig,
     SetConfigValues,
@@ -195,6 +196,84 @@ def test_validate_config_values_download_mirror_false_sets_no_fallback(
             default_values, validate_config_values(release_files_false_configparser)
         )
 
+    def test_validate_config_diff_file_reference(self) -> None:
+        diff_file_test_cases = [
+            (
+                {
+                    "mirror": {
+                        "directory": "/test",
+                        "diff-file": r"{{mirror_directory}}",
+                    }
+                },
+                "/test",
+            ),
+            (
+                {
+                    "mirror": {
+                        "directory": "/test",
+                        "diff-file": r"{{ mirror_directory }}",
+                    }
+                },
+                "/test",
+            ),
+            (
+                {
+                    "mirror": {
+                        "directory": "/test",
+                        "diff-file": r"{{ mirror_directory }}/diffs/new-files",
+                    }
+                },
+                "/test/diffs/new-files",
+            ),
+            (
+                {
+                    "strings": {"test": "TESTING"},
+                    "mirror": {"diff-file": r"/var/log/{{ strings_test }}"},
+                },
+                "/var/log/TESTING",
+            ),
+            (
+                {
+                    "strings": {"test": "TESTING"},
+                    "mirror": {"diff-file": r"/var/log/{{ strings_test }}/diffs"},
+                },
+                "/var/log/TESTING/diffs",
+            ),
+        ]
+
+        for cfg_data, expected in diff_file_test_cases:
+            with self.subTest(
+                diff_file=cfg_data["mirror"]["diff-file"],
+                expected=expected,
+                cfg_data=cfg_data,
+            ):
+                cfg = configparser.ConfigParser()
+                cfg.read_dict(cfg_data)
+                config_values = validate_config_values(cfg)
+                self.assertIsInstance(config_values.diff_file_path, str)
+                self.assertEqual(config_values.diff_file_path, expected)
+
+    def test_invalid_diff_file_reference_throws_exception(self) -> None:
+        invalid_diff_file_cases = [
+            (
+                r"{{ missing.underscore }}/foo",
+                "Unable to parse config option reference",
+            ),
+            (r"/var/{{ mirror_woops }}/foo", "No option 'woops' in section: 'mirror'"),
+        ]
+
+        for diff_file_val, expected_error in invalid_diff_file_cases:
+            with self.subTest(diff_file=diff_file_val, expected_error=expected_error):
+                cfg = configparser.ConfigParser()
+                cfg.read_dict({"mirror": {"diff-file": diff_file_val}})
+                self.assertRaisesRegex(
+                    ValueError,
+                    expected_error,
+                    eval_legacy_config_ref,
+                    cfg,
+                    diff_file_val,
+                )
+
 
 if __name__ == "__main__":
     unittest.main()