🐛 Fix entry replacement bugs

bibtexparser/library.py

@@ -11,6 +11,7 @@
     String,
 )
 
+
 # TODO Use functools.lru_cache for library properties (which create lists when called)
 
 
@@ -57,7 +58,7 @@ def remove(self, blocks: Union[List[Block], Block]):
                 del self._strings_by_key[block.key]
 
     def replace(
-        self, old_block: Block, new_block: Block, fail_on_duplicate_key: bool = True
+            self, old_block: Block, new_block: Block, fail_on_duplicate_key: bool = True
     ):
         """Replace a block with another block, at the same position.
 
@@ -73,24 +74,24 @@ def replace(
         except ValueError:
             raise ValueError("Block to replace is not in library.")
 
-        self._blocks.insert(index, new_block)
         block_after_add = self._add_to_dicts(new_block)
+        self._blocks.insert(index, block_after_add)
 
         if (
-            new_block is not block_after_add
-            and isinstance(new_block, DuplicateBlockKeyBlock)
-            and fail_on_duplicate_key
+                new_block is not block_after_add
+                and isinstance(block_after_add, DuplicateBlockKeyBlock)
+                and fail_on_duplicate_key
         ):
             # Revert changes to old_block
             #   Don't fail on duplicate key, as this would lead to an infinite recursion
             #   (should never happen for a clean library, but could happen if the user
             #   tampered with the internals of the library).
-            self.replace(new_block, old_block, fail_on_duplicate_key=False)
+            self.replace(block_after_add, old_block, fail_on_duplicate_key=False)
             raise ValueError("Duplicate key found.")
 
     @staticmethod
     def _cast_to_duplicate(
-        prev_block_with_same_key: Union[Entry, String], duplicate: Union[Entry, String]
+            prev_block_with_same_key: Union[Entry, String], duplicate: Union[Entry, String]
     ):
         assert isinstance(prev_block_with_same_key, type(duplicate)) or isinstance(
             duplicate, type(prev_block_with_same_key)
@@ -102,7 +103,7 @@ def _cast_to_duplicate(
         )
 
         assert (
-            prev_block_with_same_key.key == duplicate.key
+                prev_block_with_same_key.key == duplicate.key
         ), "Internal BibtexParser Error. Duplicate blocks have different keys."
 
         return DuplicateBlockKeyBlock(
@@ -160,7 +161,9 @@ def strings_dict(self) -> Dict[str, String]:
     @property
     def entries(self) -> List[Entry]:
         """All entry (@article, ...) blocks in the library, preserving order of insertion."""
-        return list(self._entries_by_key.values())
+        # Note: Taking this from the entries dict would be faster, but does not preserve order
+        #   e.g. in cases where `replace` has been called.
+        return [b for b in self._blocks if isinstance(b, Entry)]
 
     @property
     def entries_dict(self) -> Dict[str, Entry]:

tests/test_library.py

@@ -0,0 +1,56 @@
+from copy import deepcopy
+
+import pytest
+
+from bibtexparser import Library
+from bibtexparser.model import Entry, Field
+
+
+def get_dummy_entry():
+    return Entry(entry_type="article", key="duplicateKey", fields=[
+        Field(key="title", value="A title"),
+        Field(key="author", value="An author"),
+    ])
+
+
+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())
+    # Test precondition
+    assert len(library.blocks) == 2
+    assert len(library.failed_blocks) == 1
+
+    replacement_entry = get_dummy_entry()
+    replacement_entry.fields_dict['title'].value = 'A new title'
+
+    library.replace(library.failed_blocks[0], replacement_entry, fail_on_duplicate_key=False)
+    assert len(library.blocks) == 2
+    assert len(library.failed_blocks) == 1
+    assert library.failed_blocks[0].ignore_error_block['title'] == 'A new title'
+
+    replacement_entry_2 = get_dummy_entry()
+    replacement_entry_2.fields_dict['title'].value = 'Another new title'
+
+    library.replace(library.entries[0], replacement_entry_2, fail_on_duplicate_key=False)
+    assert len(library.blocks) == 2
+    assert len(library.failed_blocks) == 1
+    # The new block replaces the previous "non-duplicate" and should thus not become a duplicate itself
+    assert library.entries[0].fields_dict['title'].value == 'Another new title'
+
+
+def test_replace_fail_on_duplicate():
+    library = Library()
+    replaceable_entry = get_dummy_entry()
+    replaceable_entry.key = "Just a regular entry, to be replaced"
+    future_duplicate_entry = get_dummy_entry()
+    library.add([replaceable_entry, future_duplicate_entry])
+
+    with pytest.raises(ValueError):
+        library.replace(replaceable_entry, get_dummy_entry(), fail_on_duplicate_key=True)
+
+    assert len(library.blocks) == 2
+    assert len(library.failed_blocks) == 0
+    assert library.entries[0].key == "Just a regular entry, to be replaced"
+    assert library.entries[1].key == "duplicateKey"