Repository: sciunto-org/python-bibtexparser

Issue:
library.replace() method is not working properly when duplicates exist
**Describe the bug**
In order to workaround #401 and checking for failed blocks after adding one (and then replacing them), I came across this new bug.

**Reproducing**

Version: Newest from PyPI *(EDIT BY MAINTAINER: v2.0.0b2)*

Code:
```python
import bibtexparser
from bibtexparser import *

bib_library = bibtexparser.Library()

for i in range(3):

    fields = []

    fields.append(bibtexparser.model.Field("author", "ö" + str(i)))

    entry = bibtexparser.model.Entry("ARTICLE", "test", fields)

    bib_library.add(entry)

    # print(bib_library.failed_blocks)

    # print(len(bib_library.failed_blocks))

    # Check whether the addition was successful or resulted in a duplicate block that needs fixing.
    for i in range(26):
        failed_blocks = bib_library.failed_blocks
        if failed_blocks:
            failed_block = failed_blocks[0]
            # Add any additional ending, so that the slicing also works for first iteration.
            if i == 0:
                entry.key += "a"
            entry.key = entry.key[:-1] + chr(ord("a") + i)
            if type(failed_block) == bibtexparser.model.DuplicateBlockKeyBlock:
                bib_library.replace(failed_block, entry, fail_on_duplicate_key=False)
                # This works:
                # bib_library.remove(failed_block)
                # bib_library.add(entry)
        else:
            break

print(bib_library.failed_blocks)

print(bib_library.entries_dict)

bibtex_format = bibtexparser.BibtexFormat()
bibtex_format.indent = '    '

print(bibtexparser.write_string(bib_library, bibtex_format=bibtex_format))

bibtexparser.write_file("my_new_file.bib", bib_library, bibtex_format=bibtex_format)
```

**Workaround**
For me right now, removing the old block and adding the new one seems to work.

**Remaining Questions (Optional)**
Please tick all that apply:

- [ ] I would be willing to to contribute a PR to fix this issue.
- [ ] This issue is a blocker, I'd be greatful for an early fix.


Discussion:

Patch:
diff --git a/bibtexparser/library.py b/bibtexparser/library.py
index 296b7fc..db01748 100644
--- a/bibtexparser/library.py
+++ b/bibtexparser/library.py
@@ -73,19 +73,19 @@ class Library:
         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 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
@@ -160,7 +160,9 @@ class Library:
     @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]:


Test patch:
diff --git a/tests/test_library.py b/tests/test_library.py
new file mode 100644
index 0000000..239f76f
--- /dev/null
+++ b/tests/test_library.py
@@ -0,0 +1,66 @@
+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"