Repository: sciunto-org/python-bibtexparser
Pull request #381 (2023-06-02 11:47:03)

:bug: Error handling in name transformer middleware
Exceptions thrown in name-parsing middlewares are not caught, as noted here:
https://github.com/sciunto-org/python-bibtexparser/blob/9f32043364ead8b7c5c3b6bcc5e3b522d4a7e097/bibtexparser/middlewares/names.py#L52-L56

This PR catches `InvalidNameError`s thrown in name-parsing middlewares and wraps them in a `MiddlewareErrorBlock`.

It also introduces a test using the same conditions as here:
https://github.com/sciunto-org/python-bibtexparser/blob/9f32043364ead8b7c5c3b6bcc5e3b522d4a7e097/tests/middleware_tests/test_names.py#L99-L115


Patch:
diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md
index 097715a..75ab88a 100644
--- a/CONTRIBUTING.md
+++ b/CONTRIBUTING.md
@@ -1,3 +1,5 @@
+## Contributing 
+
 Thanks heaps for being interested in contributing to python-bibtexparser.
 
 We are always looking for people to improve the library. Contributions include, but are not limited to:
@@ -6,15 +8,25 @@ We are always looking for people to improve the library. Contributions include,
 2. Providing bugfixing PRs
 3. Implementing any of the issues or continuing any of the PRs labelled with `needs help` or `good first issue`.
 
-Some guidelines:
+### Some guidelines:
 
 1. Be nice! Were all doing this in our free time; no one is obligated to do anything.
 2. Add sufficient tests to your PRs 
 3. Document your code.
 4. Don't hesitate to ask questions.
 
+### Version 1 vs version 2
 Also note that there are currently two independent "default" branches: 
-First, `master`, where we maintain a stable `v1.x` and where we merge almost only backwards-compatible bugfixes.
-Second, `v2` where we work on a full re-implementation of the library. 
+First, `main`, where we maintain the `v2` of bibtexparser, which is a complete re-write and currently still in beta and not feature complete.
+Second, `v1` where we maintain the stable `v1` version of bibtexparser. Note that on `v1` we accept only small, non-breaking changes and are planning to stop support as soon as `v2` reaches reasonable stability.
+The two branches are never going to be merged anymore, thus if you want to change something for both versions, you will have to open two PRs.
+
+Issues are labelled `v1` and `v2`, correspondingly.
+
+### Dev-Dependencies, testing and linting on v2.
+
+To install the dev dependencies, run `pip install -e .[test,lint,docs]` from within the cloned repository. Then:
 
-Issues targeting the `v2` branch are labelled `v2`.
+- To test your code, run `pytest .`
+- To lint your code (required for CI/CD to pass), run: `black bibtexparser tests docs && isort bibtexparser tests docs --profile black
+- To build and preview the docs, navigate into `docs` and run `make html`. Then open the `index.html` file in the `docs/build/html` folder.
diff --git a/bibtexparser/middlewares/names.py b/bibtexparser/middlewares/names.py
index 5417410..8898eae 100644
--- a/bibtexparser/middlewares/names.py
+++ b/bibtexparser/middlewares/names.py
@@ -7,7 +7,7 @@ import abc
 import dataclasses
 from typing import List, Tuple
 
-from bibtexparser.model import Block, Entry, Field
+from bibtexparser.model import Block, Entry, Field, MiddlewareErrorBlock
 
 from .middleware import BlockMiddleware
 
@@ -49,11 +49,13 @@ class _NameTransformerMiddleware(BlockMiddleware, abc.ABC):
     # docstr-coverage: inherited
     def transform_entry(self, entry: Entry, *args, **kwargs) -> Block:
         field: Field
-        # TODO wrap in try/except to catch exceptions and create failed block if needed
-        for field in entry.fields:
-            if field.key in self.name_fields:
-                field.value = self._transform_field_value(field.value)
-        return entry
+        try:
+            for field in entry.fields:
+                if field.key in self.name_fields:
+                    field.value = self._transform_field_value(field.value)
+            return entry
+        except InvalidNameError as e:
+            return MiddlewareErrorBlock(entry, e)
 
 
 class SeparateCoAuthors(_NameTransformerMiddleware):
diff --git a/bibtexparser/writer.py b/bibtexparser/writer.py
index 54fb669..bd7f0d2 100644
--- a/bibtexparser/writer.py
+++ b/bibtexparser/writer.py
@@ -69,8 +69,8 @@ def _treat_failed_block(
     block: ParsingFailedBlock, bibtex_format: "BibtexFormat"
 ) -> List[str]:
     lines = len(block.raw.splitlines())
-    parsing_failed_comment = PARSING_FAILED_COMMENT.format({"n": lines})
-    return [parsing_failed_comment, block.raw, "\n"]
+    parsing_failed_comment = PARSING_FAILED_COMMENT.format(n=lines)
+    return [parsing_failed_comment, "\n", block.raw, "\n"]
 
 
 def _calculate_auto_value_align(library: Library) -> int:


Test patch:
diff --git a/tests/middleware_tests/test_names.py b/tests/middleware_tests/test_names.py
index 7861082..4a420a3 100644
--- a/tests/middleware_tests/test_names.py
+++ b/tests/middleware_tests/test_names.py
@@ -983,3 +983,48 @@ def test_merge_name_parts(inplace: bool):
 
     # Assert `allow_inplace_modification` is respected
     assert_inplace_is_respected(inplace, input_entry, transformed_entry)
+
+
+@pytest.mark.parametrize(
+    "name, reason",
+    [
+        ("BB,", "Trailing comma at end of name"),
+        ("BB, ", "Trailing comma at end of name"),
+        ("BB, ~\t", "Trailing comma at end of name"),
+        (", ~\t", "Trailing comma at end of name"),
+        ("AA, BB, CC, DD", "Too many commas"),
+        ("AA {BB CC", "Unterminated opening brace"),
+        ("AA {{{BB CC", "Unterminated opening brace"),
+        ("AA {{{BB} CC}", "Unterminated opening brace"),
+        ("AA BB CC}", "Unmatched closing brace"),
+        ("AA BB CC}}}", "Unmatched closing brace"),
+        ("{AA {BB CC}}}", "Unmatched closing brace"),
+    ],
+)
+def test_split_name_parts_exception(name: str, reason: str):
+    input_entry = Entry(
+        start_line=0,
+        raw="irrelevant-for-this-test",
+        entry_type="article",
+        key="articleKey",
+        fields=[
+            Field(start_line=0, key="title", value="A Test and Some More"),
+            Field(start_line=1, key="author", value=[name]),
+        ],
+    )
+
+    middleware = SplitNameParts()
+
+    # SplitNameParts always runs parse_single_name_into_parts(strict=True).
+    # As such we should get the same errors as in test_name_splitting_strict_mode
+    # but with the exceptions caught and wrapped in a MiddlewareErrorBlock.
+    transformed_library = middleware.transform(Library([input_entry]))
+
+    # No valid entries now but 1 (failed) block
+    assert len(transformed_library.entries) == 0
+    assert len(transformed_library.blocks) == 1
+    assert len(transformed_library.failed_blocks) == 1
+
+    # Using same test as in test_name_splitting_strict_mode
+    with pytest.raises(InvalidNameError, match=f".*{name}.*{reason}.*"):
+        raise transformed_library.failed_blocks[0].error
diff --git a/tests/test_writer.py b/tests/test_writer.py
index f23aa94..5baa4ce 100644
--- a/tests/test_writer.py
+++ b/tests/test_writer.py
@@ -8,6 +8,7 @@ from bibtexparser.model import (
     ExplicitComment,
     Field,
     ImplicitComment,
+    ParsingFailedBlock,
     Preamble,
     String,
 )
@@ -127,3 +128,20 @@ def test_block_separator(block_separator):
             assert lines[1 + i] == l
 
         assert lines[1 + len(expected_lines)] == '@preamble{"myValue"}'
+
+
+def test_write_failed_block():
+    raw = "@article{irrelevant-for-this-test,\nexcept = {that-there-need-to-be},\nother = {multiple-lines}\n}"
+    block = ParsingFailedBlock(
+        error=ValueError("Some error"), raw=raw, ignore_error_block=None
+    )
+    library = Library(blocks=[block])
+    string = writer.write(library)
+    lines = string.splitlines()
+
+    assert len(lines) == 5
+    assert lines[0].startswith("% WARNING")
+    assert lines[1] == "@article{irrelevant-for-this-test,"
+    assert lines[2] == "except = {that-there-need-to-be},"
+    assert lines[3] == "other = {multiple-lines}"
+    assert lines[4] == "}"