From b8350cdf703682a0c2617d4b85f291d32e7f5d5b Mon Sep 17 00:00:00 2001 From: Michael Weiss Date: Fri, 12 Jun 2026 07:41:51 +0200 Subject: [PATCH 1/5] =?UTF-8?q?=F0=9F=A6=96=20Make=20Library.add=20fail=20?= =?UTF-8?q?on=20duplicate=20keys=20by=20default=20(#400)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Programmatically adding an entry with an existing key previously replaced it silently with a DuplicateBlockKeyBlock, surprising users at write time (see #400). Library.add and the Library constructor now raise a ValueError on duplicate keys by default. Parsing behavior is unchanged: the splitter and middleware library reconstruction explicitly pass fail_on_duplicate_key=False, so bibtex files with duplicate keys still parse into library.failed_blocks. Co-Authored-By: Claude Fable 5 --- bibtexparser/library.py | 21 +++++++++++------ bibtexparser/middlewares/middleware.py | 4 +++- bibtexparser/middlewares/sorting_blocks.py | 5 ++-- bibtexparser/splitter.py | 6 +++-- tests/test_library.py | 27 +++++++++++++++++++--- 5 files changed, 48 insertions(+), 15 deletions(-) diff --git a/bibtexparser/library.py b/bibtexparser/library.py index c7c89c5b..50698516 100644 --- a/bibtexparser/library.py +++ b/bibtexparser/library.py @@ -17,23 +17,30 @@ class Library: """A collection of parsed bibtex blocks.""" - def __init__(self, blocks: Union[List[Block], None] = None): + def __init__( + self, + blocks: Union[List[Block], None] = None, + fail_on_duplicate_key: bool = True, + ): self._blocks = [] self._entries_by_key = dict() self._strings_by_key = dict() if blocks is not None: - self.add(blocks) + self.add(blocks, fail_on_duplicate_key=fail_on_duplicate_key) - def add(self, blocks: Union[List[Block], Block], fail_on_duplicate_key: bool = False): + def add(self, blocks: Union[List[Block], Block], fail_on_duplicate_key: bool = True): """Add blocks to library. - The adding is key-safe, i.e., it is made sure that no duplicate keys are added. - for the same type (i.e., String or Entry). Duplicates are silently replaced with - a DuplicateKeyBlock. + The adding is key-safe, i.e., it is made sure that no duplicate keys are added + for the same type (i.e., String or Entry). Duplicates are replaced with + a DuplicateBlockKeyBlock. :param blocks: Block or list of blocks to add. :param fail_on_duplicate_key: - If True, raises ValueError if a block was replaced with a DuplicateKeyBlock. + If True (default), raises ValueError if a block was replaced with a + DuplicateBlockKeyBlock. If False, duplicates are replaced silently + and can be inspected via `library.failed_blocks`. This is e.g. used + when parsing, where a bibtex file with duplicate keys should not raise. """ if isinstance(blocks, Block): blocks = [blocks] diff --git a/bibtexparser/middlewares/middleware.py b/bibtexparser/middlewares/middleware.py index 6d390fdf..7df059dc 100644 --- a/bibtexparser/middlewares/middleware.py +++ b/bibtexparser/middlewares/middleware.py @@ -97,7 +97,9 @@ def transform(self, library: "Library") -> "Library": # Case 4: Something else. Error. else: raise TypeError(f"Illegal output type from transform_block: {type(transformed)}") - return Library(blocks=blocks) + # Duplicate keys (e.g. introduced by a block transformation) must not raise + # mid-pipeline; they are exposed as `library.failed_blocks`. + return Library(blocks=blocks, fail_on_duplicate_key=False) def transform_block( self, block: Block, library: "Library" diff --git a/bibtexparser/middlewares/sorting_blocks.py b/bibtexparser/middlewares/sorting_blocks.py index 117646d9..2062ded7 100644 --- a/bibtexparser/middlewares/sorting_blocks.py +++ b/bibtexparser/middlewares/sorting_blocks.py @@ -128,11 +128,12 @@ def transform(self, library: Library) -> Library: block_junks = self._block_junks(blocks) block_junks.sort(key=lambda junk: self._key(junk.main_block), reverse=self._reverse) return Library( - blocks=[block for block_junk in block_junks for block in block_junk.blocks] + blocks=[block for block_junk in block_junks for block in block_junk.blocks], + fail_on_duplicate_key=False, ) else: blocks.sort(key=self._key, reverse=self._reverse) - return Library(blocks=blocks) + return Library(blocks=blocks, fail_on_duplicate_key=False) class SortBlocksByTypeAndKeyMiddleware(SortBlocksMiddleware): diff --git a/bibtexparser/splitter.py b/bibtexparser/splitter.py index ba5faa5c..f8f150c7 100644 --- a/bibtexparser/splitter.py +++ b/bibtexparser/splitter.py @@ -323,9 +323,11 @@ def split(self, library: Optional[Library] = None) -> Library: elif m_val.startswith("@preamble"): library.add(self._handle_preamble()) elif m_val.startswith("@string"): - library.add(self._handle_string(m)) + # Duplicate keys in parsed bibtex must not raise, + # they are exposed as `library.failed_blocks`. + library.add(self._handle_string(m), fail_on_duplicate_key=False) else: - library.add(self._handle_entry(m, m_val)) + library.add(self._handle_entry(m, m_val), fail_on_duplicate_key=False) except BlockAbortedException as e: logger.warning( diff --git a/tests/test_library.py b/tests/test_library.py index 8142a06e..77ccbc34 100644 --- a/tests/test_library.py +++ b/tests/test_library.py @@ -20,7 +20,7 @@ def test_replace_with_duplicates(): """Test that replace() works when there are duplicate values. See issue 404.""" library = Library() library.add(get_dummy_entry()) - library.add(get_dummy_entry()) + library.add(get_dummy_entry(), fail_on_duplicate_key=False) # Test precondition assert len(library.blocks) == 2 assert len(library.failed_blocks) == 1 @@ -59,10 +59,31 @@ def test_replace_fail_on_duplicate(): assert library.entries[1].key == "duplicateKey" -def test_fail_on_duplicate_add(): +def test_add_fails_on_duplicate_by_default(): + library = Library() + library.add(get_dummy_entry()) + with pytest.raises(ValueError): + library.add(get_dummy_entry()) + # The duplicate is added as failed block nonetheless, + # allowing the user to resolve it after catching the exception. + assert len(library.blocks) == 2 + assert len(library.failed_blocks) == 1 + + +def test_add_duplicate_does_not_fail_if_disabled(): library = Library() library.add(get_dummy_entry()) + library.add(get_dummy_entry(), fail_on_duplicate_key=False) + assert len(library.blocks) == 2 + assert len(library.failed_blocks) == 1 + + +def test_constructor_fails_on_duplicate_by_default(): with pytest.raises(ValueError): - library.add(get_dummy_entry(), fail_on_duplicate_key=True) + Library(blocks=[get_dummy_entry(), get_dummy_entry()]) + + library = Library( + blocks=[get_dummy_entry(), get_dummy_entry()], fail_on_duplicate_key=False + ) assert len(library.blocks) == 2 assert len(library.failed_blocks) == 1 From 022721d815b49914ee6929698b8967032fa147f5 Mon Sep 17 00:00:00 2001 From: Michael Weiss Date: Fri, 12 Jun 2026 07:47:29 +0200 Subject: [PATCH 2/5] :recycle: Leave library untouched when add() raises on duplicate keys Instead of inserting the duplicate as a DuplicateBlockKeyBlock and then raising, add() now pre-checks all blocks (including duplicates within the same call) and raises before any mutation, making the operation atomic. Co-Authored-By: Claude Fable 5 --- bibtexparser/library.py | 50 ++++++++++++++++++++++++++--------------- tests/test_library.py | 21 +++++++++++------ 2 files changed, 46 insertions(+), 25 deletions(-) diff --git a/bibtexparser/library.py b/bibtexparser/library.py index 50698516..bef9e05a 100644 --- a/bibtexparser/library.py +++ b/bibtexparser/library.py @@ -37,34 +37,48 @@ def add(self, blocks: Union[List[Block], Block], fail_on_duplicate_key: bool = T :param blocks: Block or list of blocks to add. :param fail_on_duplicate_key: - If True (default), raises ValueError if a block was replaced with a - DuplicateBlockKeyBlock. If False, duplicates are replaced silently - and can be inspected via `library.failed_blocks`. This is e.g. used - when parsing, where a bibtex file with duplicate keys should not raise. + If True (default), raises ValueError on duplicate keys, leaving the + library unchanged. If False, duplicates are silently replaced with + DuplicateBlockKeyBlock instances, which can be inspected via + `library.failed_blocks`. This is e.g. used when parsing, where a + bibtex file with duplicate keys should not raise. + :raises ValueError: If fail_on_duplicate_key is True and a duplicate key + is found. In this case, no blocks are added to the library. """ if isinstance(blocks, Block): blocks = [blocks] - _added_blocks = [] - for block in blocks: - # This may replace block with a DuplicateEntryKeyBlock - block = self._add_to_dicts(block) - self._blocks.append(block) - _added_blocks.append(block) - if fail_on_duplicate_key: - duplicate_keys = [] - for original, added in zip(blocks, _added_blocks): - if original is not added and isinstance(added, DuplicateBlockKeyBlock): - duplicate_keys.append(added.key) - + duplicate_keys = self._find_duplicate_keys(blocks) if len(duplicate_keys) > 0: raise ValueError( f"Duplicate keys found: {duplicate_keys}. " - f"Duplicate entries have been added to the library as DuplicateBlockKeyBlock." - f"Use `library.failed_blocks` to access them. " + f"No blocks were added to the library. " + f"To add duplicates as DuplicateBlockKeyBlock instances instead, " + f"use `library.add(blocks, fail_on_duplicate_key=False)`." ) + for block in blocks: + # This may replace block with a DuplicateEntryKeyBlock + block = self._add_to_dicts(block) + self._blocks.append(block) + + def _find_duplicate_keys(self, blocks: List[Block]) -> List[str]: + """Keys of blocks that would become duplicates when added to the library.""" + duplicate_keys = [] + seen_entry_keys = set(self._entries_by_key) + seen_string_keys = set(self._strings_by_key) + for block in blocks: + if isinstance(block, Entry): + if block.key in seen_entry_keys: + duplicate_keys.append(block.key) + seen_entry_keys.add(block.key) + elif isinstance(block, String): + if block.key in seen_string_keys: + duplicate_keys.append(block.key) + seen_string_keys.add(block.key) + return duplicate_keys + def remove(self, blocks: Union[List[Block], Block]): """Remove blocks from library. diff --git a/tests/test_library.py b/tests/test_library.py index 77ccbc34..9b515d03 100644 --- a/tests/test_library.py +++ b/tests/test_library.py @@ -64,10 +64,19 @@ def test_add_fails_on_duplicate_by_default(): library.add(get_dummy_entry()) with pytest.raises(ValueError): library.add(get_dummy_entry()) - # The duplicate is added as failed block nonetheless, - # allowing the user to resolve it after catching the exception. - assert len(library.blocks) == 2 - assert len(library.failed_blocks) == 1 + # The failed add must leave the library unchanged. + assert len(library.blocks) == 1 + assert len(library.failed_blocks) == 0 + + +def test_add_with_duplicates_in_same_call_fails_atomically(): + unique_entry = get_dummy_entry() + unique_entry.key = "uniqueKey" + library = Library() + with pytest.raises(ValueError): + library.add([unique_entry, get_dummy_entry(), get_dummy_entry()]) + # Not even the non-duplicate blocks of the failed call are added. + assert len(library.blocks) == 0 def test_add_duplicate_does_not_fail_if_disabled(): @@ -82,8 +91,6 @@ def test_constructor_fails_on_duplicate_by_default(): with pytest.raises(ValueError): Library(blocks=[get_dummy_entry(), get_dummy_entry()]) - library = Library( - blocks=[get_dummy_entry(), get_dummy_entry()], fail_on_duplicate_key=False - ) + library = Library(blocks=[get_dummy_entry(), get_dummy_entry()], fail_on_duplicate_key=False) assert len(library.blocks) == 2 assert len(library.failed_blocks) == 1 From 8405822e0a2b08c151361d816e87f5bec35c825e Mon Sep 17 00:00:00 2001 From: Michael Weiss Date: Fri, 12 Jun 2026 08:01:07 +0200 Subject: [PATCH 3/5] :memo: Align add() docstring summary with fail-by-default behavior Co-Authored-By: Claude Fable 5 --- bibtexparser/library.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/bibtexparser/library.py b/bibtexparser/library.py index bef9e05a..dfadc9ca 100644 --- a/bibtexparser/library.py +++ b/bibtexparser/library.py @@ -32,7 +32,8 @@ def add(self, blocks: Union[List[Block], Block], fail_on_duplicate_key: bool = T """Add blocks to library. The adding is key-safe, i.e., it is made sure that no duplicate keys are added - for the same type (i.e., String or Entry). Duplicates are replaced with + for the same type (i.e., String or Entry). Depending on `fail_on_duplicate_key`, + duplicates either cause a ValueError (default) or are replaced with a DuplicateBlockKeyBlock. :param blocks: Block or list of blocks to add. From c35173ff8b0e1c9a2cdd3da3d632927e23aab731 Mon Sep 17 00:00:00 2001 From: Michael Weiss Date: Fri, 12 Jun 2026 08:03:36 +0200 Subject: [PATCH 4/5] remove unneeded comment --- bibtexparser/splitter.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/bibtexparser/splitter.py b/bibtexparser/splitter.py index f8f150c7..6ec8f948 100644 --- a/bibtexparser/splitter.py +++ b/bibtexparser/splitter.py @@ -323,8 +323,6 @@ def split(self, library: Optional[Library] = None) -> Library: elif m_val.startswith("@preamble"): library.add(self._handle_preamble()) elif m_val.startswith("@string"): - # Duplicate keys in parsed bibtex must not raise, - # they are exposed as `library.failed_blocks`. library.add(self._handle_string(m), fail_on_duplicate_key=False) else: library.add(self._handle_entry(m, m_val), fail_on_duplicate_key=False) From abc223becdb319703efc4921dbead762baf60565 Mon Sep 17 00:00:00 2001 From: Michael Weiss Date: Fri, 12 Jun 2026 08:04:16 +0200 Subject: [PATCH 5/5] :white_check_mark: Adapt writer duplicate test to fail-by-default add() After merging #526's tests into this branch, the duplicate must be added with fail_on_duplicate_key=False so the test still exercises the write-time error path. Co-Authored-By: Claude Fable 5 --- tests/test_writer.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/test_writer.py b/tests/test_writer.py index 2eabaec5..459093d9 100644 --- a/tests/test_writer.py +++ b/tests/test_writer.py @@ -157,7 +157,8 @@ def test_write_manually_created_duplicate_entries_raises(): """Reproduces issue #400: duplicate keys on programmatically created entries.""" library = Library() library.add(_dummy_entry()) - library.add(_dummy_entry()) + # Silently-added duplicates (no raw bibtex) must still be caught at write time. + library.add(_dummy_entry(), fail_on_duplicate_key=False) with pytest.raises(ValueError, match="failed_blocks"): writer.write(library)