Have Author.merge_remote_ids error on conflict for now

Author: cdrini

openlibrary/catalog/add_book/load_book.py

@@ -168,7 +168,7 @@ def get_redirected_authors(authors: list["Author"]):
     # Try other identifiers next.
     if remote_ids := author.get("remote_ids"):
         queries = []
-        matched_authors = []
+        matched_authors: list[Author] = []
         # Get all the authors that match any incoming identifier.
         for identifier, val in remote_ids.items():
             queries.append({"type": "/type/author", f"remote_ids.{identifier}": val})
@@ -178,28 +178,19 @@ def get_redirected_authors(authors: list["Author"]):
                     get_redirected_authors(list(web.ctx.site.get_many(reply)))
                 )
         matched_authors = uniq(matched_authors, key=lambda thing: thing.key)
-        # The match is whichever one has the most identifiers in common AND does not have more conflicts than matched identifiers.
-        highest_matches = 0
-        selected_match = None
-        for a in matched_authors:
-            _, matches = a.merge_remote_ids(remote_ids)
-            if matches > highest_matches:
-                selected_match = a
-                highest_matches = matches
-            elif (
-                matches == highest_matches
-                and matches > 0
-                and selected_match is not None
-            ):
-                # Prioritize the lower OL ID when matched identifiers are equal
-                selected_match = (
-                    a
-                    if extract_numeric_id_from_olid(a.key)
-                    < extract_numeric_id_from_olid(selected_match.key)
-                    else selected_match
-                )
-        if highest_matches > 0 and selected_match is not None:
+        # The match is whichever one has the most identifiers in common
+        if matched_authors:
+            selected_match = sorted(
+                matched_authors,
+                key=lambda a: (
+                    # First sort by number of matches desc
+                    -1 * a.merge_remote_ids(remote_ids)[1],
+                    # If there's a tie, prioritize lower OL ID
+                    extract_numeric_id_from_olid(a.key),
+                ),
+            )[0]
             return [selected_match]
+
     # Fall back to name/date matching, which we did before introducing identifiers.
     name = author["name"].replace("*", r"\*")
     queries = [

openlibrary/catalog/add_book/tests/test_load_book.py

@@ -162,15 +162,42 @@ def test_first_match_ol_key(self, mock_site):
         mock_site.save(author_different_key)
 
         # Look for exact match on OL ID, regardless of other fields.
-        # We ideally shouldn't ever have a case where different authors have the same VIAF, but this demonstrates priority.
         searched_author = {
             "name": "William H. Brewer",
             "key": "/authors/OL4A",
-            "remote_ids": {"viaf": "12345678"},
         }
         found = import_author(searched_author)
         assert found.key == author_different_key["key"]
 
+    def test_conflicting_ids_cause_error(self, mock_site):
+        # Author with VIAF
+        author = {
+            "name": "William H. Brewer",
+            "key": "/authors/OL3A",
+            "type": {"key": "/type/author"},
+            "remote_ids": {"viaf": "12345678"},
+        }
+
+        # Another author with VIAF
+        author_different_key = {
+            "name": "William Brewer",
+            "key": "/authors/OL4A",
+            "type": {"key": "/type/author"},
+            "remote_ids": {"viaf": "87654321"},
+        }
+
+        mock_site.save(author)
+        mock_site.save(author_different_key)
+
+        # Author with differing ID
+        searched_author = {
+            "name": "William H. Brewer",
+            "key": "/authors/OL4A",
+            "remote_ids": {"viaf": "12345678"},
+        }
+        with pytest.raises(ValueError):
+            import_author(searched_author)
+
     def test_second_match_remote_identifier(self, mock_site):
         """
         Next highest priority match is any other remote identifier, such as VIAF, Goodreads ID, Amazon ID, etc.

openlibrary/core/models.py

@@ -814,20 +814,17 @@ def merge_remote_ids(
             return output, -1
         # Count
         matches = 0
-        conflicts = 0
         config = get_identifier_config("author")
         for id in config["identifiers"]:
             identifier: str = id.name
             if identifier in output and identifier in incoming_ids:
                 if output[identifier] != incoming_ids[identifier]:
-                    conflicts = conflicts + 1
+                    # For now, cause an error so we can see when/how often this happens
+                    raise ValueError(
+                        f"Conflicting remote IDs for author {self.key}: {output[identifier]} vs {incoming_ids[identifier]}"
+                    )
                 else:
                     matches = matches + 1
-        # Decide at a later date if we want to change the logic to bail only when conflicts > matches, instead of conflicts > 0.
-        # if conflicts > matches:
-        if conflicts > 0:
-            # TODO: Raise this to librarians, somehow.
-            return self.remote_ids, -1
         return output, matches