🐛 Only strip enclosing delimiters that enclose the whole value

bibtexparser/middlewares/enclosing.py

@@ -1,3 +1,5 @@
+import re
+
 from bibtexparser.library import Library
 from bibtexparser.model import Entry
 from bibtexparser.model import Field
@@ -7,6 +9,39 @@
 
 REMOVED_ENCLOSING_KEY = "removed_enclosing"
 
+# Delimiters relevant when scanning a value, using the splitter's escaping
+# convention: a delimiter is escaped iff it is directly preceded by a backslash.
+_BRACES = re.compile(r"(?<!\\)[{}]")
+_BRACES_AND_QUOTE = re.compile(r"(?<!\\)[{}\"]")
+_UNENCLOSED_MARKS = re.compile(r"(?<!\\)[{}\",=\n]")
+
+
+def _is_writable_unenclosed(value: str) -> bool:
+    """Whether writing the value verbatim yields the same value when re-parsed.
+
+    Values that are written without enclosing (e.g. string references and
+    concatenations) must not contain anything the splitter would read as the
+    end of the value: an unbalanced brace or, outside braces and quotes,
+    a comma, an equals sign or a newline.
+    """
+    depth = 0
+    in_quotes = False
+    for match in _UNENCLOSED_MARKS.finditer(value):
+        char = match.group()
+        if char == "{":
+            depth += 1
+        elif char == "}":
+            depth -= 1
+            if depth < 0:
+                return False
+        elif depth == 0:
+            if char == '"':
+                in_quotes = not in_quotes
+            elif not in_quotes:
+                return False
+    return depth == 0 and not in_quotes
+
+
 STRINGS_CAN_BE_UNESCAPED_INTS = False
 ENTRY_POTENTIALLY_INT_FIELDS = [
     "year",
@@ -27,9 +62,14 @@ class RemoveEnclosingMiddleware(BlockMiddleware):
     It is useful when the field value is enclosed in braces or quotes
     (which is the case for the vast majority of values).
 
+    Only a delimiter pair enclosing the *whole* value is removed:
+    `pages = {intro} # {outro}` merely starts and ends in braces,
+    but these do not belong to the same pair and are thus kept.
+
     Values that were not enclosed and are not plain integers
     (i.e., unresolved bibtex string references such as `month = jan`
-    and concatenation expressions such as `pages = intro # outro`)
+    and concatenation expressions such as `pages = intro # outro`
+    or `pages = {intro} # {outro}`)
     get a `no-enclosing` demand (see `Field.enclosing`),
     as enclosing them when writing would change their semantics.
 
@@ -49,12 +89,62 @@ def metadata_key(cls) -> str:
         return REMOVED_ENCLOSING_KEY
 
     @staticmethod
-    def _strip_enclosing(value: str) -> tuple[str, str | None]:
+    def _is_enclosed_in_braces(value: str) -> bool:
+        """Whether the brace opened at the first char is the one closed at the last char.
+
+        This is False for values that merely start and end in braces, such as the
+        concatenation expression `{intro} # {outro}`, whose outer braces do not enclose
+        the whole value. Escaped braces are not counted.
+        """
+        inner = value[1:-1]
+        if "{" not in inner and "}" not in inner:
+            # Fast path for the common case of a plainly braced value.
+            return not inner.endswith("\\")
+        depth = 0
+        last_index = len(value) - 1
+        for match in _BRACES.finditer(value):
+            if match.group() == "{":
+                depth += 1
+                continue
+            depth -= 1
+            if depth < 0:
+                return False
+            if depth == 0 and match.start() != last_index:
+                return False
+        return depth == 0
+
+    @staticmethod
+    def _is_enclosed_in_quotes(value: str) -> bool:
+        """Whether the quote at the first char is the one closed at the last char.
+
+        This is False for values that merely start and end in quotes, such as the
+        concatenation expression `"intro" # "outro"`. Escaped quotes and quotes
+        inside braces are not counted.
+        """
+        depth = 0
+        last_index = len(value) - 1
+        for match in _BRACES_AND_QUOTE.finditer(value):
+            index = match.start()
+            if index == 0 or index == last_index:
+                continue
+            char = match.group()
+            if char == "{":
+                depth += 1
+            elif char == "}":
+                depth = max(depth - 1, 0)
+            elif depth == 0:
+                return False
+        return True
+
+    @classmethod
+    def _strip_enclosing(cls, value: str) -> tuple[str, str | None]:
         value = value.strip()
-        if value.startswith("{") and value.endswith("}"):
-            return value[1:-1], "{"
-        if value.startswith('"') and value.endswith('"'):
-            return value[1:-1], '"'
+        # A single `{` or `"` starts and ends with the same char, but is no enclosing.
+        if len(value) >= 2:
+            if value.startswith("{") and value.endswith("}") and cls._is_enclosed_in_braces(value):
+                return value[1:-1], "{"
+            if value.startswith('"') and value.endswith('"') and cls._is_enclosed_in_quotes(value):
+                return value[1:-1], '"'
         return value, "no-enclosing"
 
     # docstr-coverage: inherited
@@ -99,6 +189,11 @@ class AddEnclosingMiddleware(BlockMiddleware):
     3. If the value is an integer in a common int field
        and ``enclose_integers`` is False, no enclosing is added.
     4. Otherwise, the ``default_enclosing`` is used.
+
+    A ``no-enclosing`` resulting from 1. or 2. is overruled by the
+    ``default_enclosing`` if the value cannot be written verbatim,
+    i.e. if writing it as-is would not parse back to the same value.
+    This happens when a middleware changed the value after the demand was set.
     """
 
     def __init__(
@@ -152,6 +247,12 @@ def _enclose(
         elif apply_int_rule and not self._enclose_integers and str(value).isdigit():
             return str(value)
 
+        if enclosing == "no-enclosing" and not _is_writable_unenclosed(str(value)):
+            # The value cannot be written verbatim: writing it as-is would produce
+            # bibtex that does not parse back to this value. This happens when a
+            # middleware changed the value after the demand was set.
+            enclosing = self._default_enclosing
+
         if enclosing == "{":
             return f"{{{value}}}"
         if enclosing == '"':

tests/middleware_tests/test_enclosing.py

@@ -6,6 +6,7 @@
 from bibtexparser.library import Library
 from bibtexparser.middlewares.enclosing import AddEnclosingMiddleware
 from bibtexparser.middlewares.enclosing import RemoveEnclosingMiddleware
+from bibtexparser.middlewares.middleware import BlockMiddleware
 from bibtexparser.model import Entry
 from bibtexparser.model import Field
 from bibtexparser.model import String
@@ -422,3 +423,232 @@ def test_string_reference_roundtrip():
 
 
 # TODO round-trip tests (removal -> addition -> removal)
+
+
+@pytest.mark.parametrize(
+    "value",
+    [
+        pytest.param("{intro} # {outro}", id="brace_concatenation"),
+        pytest.param('"intro" # "outro"', id="quote_concatenation"),
+        pytest.param('{intro} # "outro"', id="mixed_concatenation"),
+        pytest.param("{a} and {b}", id="two_brace_groups"),
+        pytest.param('"a" "b"', id="two_quote_groups"),
+        pytest.param("{a} # b # {c}", id="concatenation_with_reference"),
+    ],
+)
+def test_no_removal_if_delimiters_do_not_enclose_whole_value(value: str):
+    """Values whose first and last char are delimiters, but not a matching pair,
+    must not be stripped, as this would corrupt them."""
+    field = Field(value=value, start_line=6, key="pages")
+    input_entry = Entry(
+        start_line=5,
+        entry_type="article",
+        raw="<--- does not matter for this unit test -->",
+        key="someKey",
+        fields=[field],
+    )
+
+    middleware = RemoveEnclosingMiddleware(allow_inplace_modification=True)
+    transformed = middleware.transform(library=Library([input_entry])).entries[0]
+
+    assert transformed["pages"] == value
+    assert transformed.parser_metadata["removed_enclosing"]["pages"] == "no-enclosing"
+    assert transformed.fields_dict["pages"].enclosing == "no-enclosing"
+
+
+@pytest.mark.parametrize(
+    "value",
+    [
+        pytest.param("{intro} # {outro}", id="brace_concatenation"),
+        pytest.param('"intro" # "outro"', id="quote_concatenation"),
+        pytest.param("{a} and {b}", id="two_brace_groups"),
+    ],
+)
+def test_no_removal_on_string_block_if_delimiters_do_not_enclose_whole_value(value: str):
+    """Same as above, for `@string` blocks."""
+    input_string = String(
+        start_line=5,
+        raw="<--- does not matter for this unit test -->",
+        key="someKey",
+        value=value,
+    )
+
+    middleware = RemoveEnclosingMiddleware(allow_inplace_modification=True)
+    transformed = middleware.transform(library=Library([input_string])).strings[0]
+
+    assert transformed.value == value
+    assert transformed.parser_metadata["removed_enclosing"] == "no-enclosing"
+    assert transformed.enclosing == "no-enclosing"
+
+
+@pytest.mark.parametrize(
+    "value, expected_stripped, expected_enclosing",
+    [
+        pytest.param("{a {b} c}", "a {b} c", "{", id="nested_group"),
+        pytest.param("{{nested}}", "{nested}", "{", id="doubly_braced"),
+        pytest.param("{}", "", "{", id="empty_braces"),
+        pytest.param('""', "", '"', id="empty_quotes"),
+        pytest.param('{"quoted"}', '"quoted"', "{", id="quotes_in_braces"),
+        pytest.param('"a {b} c"', "a {b} c", '"', id="braces_in_quotes"),
+        pytest.param('"a {"} c"', 'a {"} c', '"', id="braced_quote_in_quotes"),
+        pytest.param(r"{a \{ b}", r"a \{ b", "{", id="escaped_open_brace"),
+        pytest.param(r"{a \} b}", r"a \} b", "{", id="escaped_close_brace"),
+        pytest.param(r'"a \" b"', r"a \" b", '"', id="escaped_quote"),
+        pytest.param(r"{{\`a} {\`a}}", r"{\`a} {\`a}", "{", id="enclosed_groups"),
+    ],
+)
+def test_removal_of_genuinely_enclosing_delimiters(
+    value: str, expected_stripped: str, expected_enclosing: str
+):
+    """Values that really are enclosed by a single delimiter pair must still be stripped."""
+    assert RemoveEnclosingMiddleware._strip_enclosing(value) == (
+        expected_stripped,
+        expected_enclosing,
+    )
+
+
+@pytest.mark.parametrize(
+    "value",
+    [
+        pytest.param("", id="empty"),
+        pytest.param(" ", id="whitespace"),
+        pytest.param("{", id="lone_open_brace"),
+        pytest.param("}", id="lone_close_brace"),
+        pytest.param('"', id="lone_quote"),
+        pytest.param("\\", id="lone_backslash"),
+    ],
+)
+def test_degenerate_values_are_not_stripped(value: str):
+    """Short/degenerate values must neither raise nor lose characters."""
+    assert RemoveEnclosingMiddleware._strip_enclosing(value) == (value.strip(), "no-enclosing")
+
+
+def test_concatenation_roundtrip():
+    """Default parse -> write must reproduce concatenations of delimited parts
+    verbatim, rather than corrupting them."""
+    bibtex = '@article{a,\n\tpages = {intro} # {outro},\n\ttitle = "x" # "y"\n}\n'
+    written = bibtexparser.write_string(bibtexparser.parse_string(bibtex))
+
+    assert "pages = {intro} # {outro}" in written
+    assert 'title = "x" # "y"' in written
+    assert written == bibtex
+
+
+@pytest.mark.parametrize(
+    "value, expected_stripped, expected_enclosing",
+    [
+        pytest.param(r"{\\}a}", r"\\}a", "{", id="doubled_backslash_before_brace"),
+        pytest.param(r"{a\\{}", r"a\\{", "{", id="doubled_backslash_before_open_brace"),
+        pytest.param(r'"\\""', r'\\"', '"', id="doubled_backslash_before_quote"),
+    ],
+)
+def test_escaping_follows_the_splitter_convention(
+    value: str, expected_stripped: str, expected_enclosing: str
+):
+    """A delimiter is escaped iff directly preceded by a backslash.
+
+    This is the convention of the splitter's mark regex, which skips such a
+    delimiter regardless of how many backslashes precede it. The two must agree,
+    or values the parser read as a single group are not stripped here.
+    """
+    assert RemoveEnclosingMiddleware._strip_enclosing(value) == (
+        expected_stripped,
+        expected_enclosing,
+    )
+
+
+@pytest.mark.parametrize(
+    "value",
+    [
+        pytest.param("Doe, John and Roe, Jane", id="top_level_comma"),
+        pytest.param("Foo # , Bar", id="concatenation_with_top_level_comma"),
+        pytest.param("a = b", id="top_level_equals"),
+        pytest.param("a\nb", id="top_level_newline"),
+        pytest.param("a} b", id="unbalanced_closing_brace"),
+        pytest.param("{a b", id="unbalanced_opening_brace"),
+    ],
+)
+def test_no_enclosing_demand_is_overruled_for_unwritable_values(value: str):
+    """A `no-enclosing` demand must not produce bibtex that does not parse back.
+
+    A value-transforming middleware may change a value after the demand was set
+    (e.g. by removing the braces it was derived from), which can leave a value
+    that the splitter would not read back in one piece.
+    """
+    field = Field(value=value, start_line=6, key="author", enclosing="no-enclosing")
+    entry = Entry(
+        start_line=5,
+        entry_type="article",
+        raw="<--- does not matter for this unit test -->",
+        key="someKey",
+        fields=[field],
+    )
+
+    middleware = AddEnclosingMiddleware(
+        reuse_previous_enclosing=True, enclose_integers=True, default_enclosing="{"
+    )
+    transformed = middleware.transform(library=Library([entry])).entries[0]
+
+    assert transformed["author"] == f"{{{value}}}"
+
+
+@pytest.mark.parametrize(
+    "value",
+    [
+        pytest.param("jan", id="string_reference"),
+        pytest.param("intro # outro", id="concatenation_of_references"),
+        pytest.param("{intro} # {outro}", id="concatenation_of_groups"),
+        pytest.param("ieeetc # {, Special Issue}", id="mixed_concatenation"),
+        pytest.param('"a, b" # c', id="quoted_part_with_comma"),
+    ],
+)
+def test_no_enclosing_demand_is_honored_for_writable_values(value: str):
+    """Values that do parse back verbatim keep their `no-enclosing` demand."""
+    field = Field(value=value, start_line=6, key="author", enclosing="no-enclosing")
+    entry = Entry(
+        start_line=5,
+        entry_type="article",
+        raw="<--- does not matter for this unit test -->",
+        key="someKey",
+        fields=[field],
+    )
+
+    middleware = AddEnclosingMiddleware(
+        reuse_previous_enclosing=True, enclose_integers=True, default_enclosing="{"
+    )
+    transformed = middleware.transform(library=Library([entry])).entries[0]
+
+    assert transformed["author"] == value
+
+
+@pytest.mark.parametrize(
+    "value",
+    [
+        pytest.param("{Doe, John} and {Roe, Jane}", id="two_brace_groups"),
+        pytest.param("{Foo} # {, Bar}", id="concatenation_of_groups"),
+        pytest.param("ieeetc # {, Special Issue}", id="mixed_concatenation"),
+    ],
+)
+def test_written_output_reparses_after_a_value_transformation(value: str):
+    """Values kept verbatim carry their own delimiters and a `no-enclosing` demand.
+
+    A middleware rewriting such a value (here: removing the braces) must not
+    leave output that bibtexparser can no longer parse.
+    """
+
+    class _BraceRemovingMiddleware(BlockMiddleware):
+        def transform_entry(self, entry, library):
+            for field in entry.fields:
+                # As the latex middlewares do: the value setter resets `enclosing`.
+                enclosing = field.enclosing
+                field.value = field.value.replace("{", "").replace("}", "")
+                field.enclosing = enclosing
+            return entry
+
+    bibtex = f"@article{{a,\n\tauthor = {value}\n}}\n"
+    library = bibtexparser.parse_string(bibtex, append_middleware=[_BraceRemovingMiddleware()])
+    written = bibtexparser.write_string(library)
+
+    reparsed = bibtexparser.parse_string(written)
+    assert not reparsed.failed_blocks
+    assert len(reparsed.entries) == 1