streamline shuttle move/copy operations

backend/src/cms_backend/shuttle/move_files.py

@@ -70,71 +70,73 @@ def move_book_files(session: OrmSession, book: Book):
         book.needs_file_operation = False
         return
 
-    # start with copies
-    while len(target_locations) > len(current_locations):
-        current_location = current_locations[0]
-        target_location = target_locations[0]
+    # Grab one valid source file to copy from since it's the same
+    # file spread across multiple locations with the same book
+    # data
+    source_location = current_locations[0]
 
-        current_path = current_location.full_local_path(
-            ShuttleContext.local_warehouse_paths
-        )
+    current_locations_map = {
+        (loc.warehouse_id, loc.path): loc for loc in current_locations
+    }
+
+    for target_location in target_locations:
         target_path = target_location.full_local_path(
             ShuttleContext.local_warehouse_paths
         )
-
-        target_path.parent.mkdir(parents=True, exist_ok=True)
-        shutil.copy(current_path, target_path)
-        logger.debug(f"Copied book {book.id} from {current_path} to {target_path}")
-        book.events.append(
-            f"{getnow()}: copied book from {current_location.full_str} to "
-            f"{target_location.full_str}"
-        )
-        target_locations.remove(target_location)
-        target_location.status = "current"
-
-    # continue with moves
-    while len(current_locations) > 0 and len(target_locations) > 0:
-        current_location = current_locations[0]
-        target_location = target_locations[0]
-
-        current_path = current_location.full_local_path(
-            ShuttleContext.local_warehouse_paths
+        matching_current = current_locations_map.get(
+            (target_location.warehouse_id, target_location.path)
         )
-        target_path = target_location.full_local_path(
+        if matching_current:
+            # This file is already here. Remove redundant target and remove the current
+            # location from the cleanup list
+            session.delete(target_location)
+            book.locations.remove(target_location)
+            current_locations.remove(matching_current)
+            logger.debug(f"Left book {book.id} at identical path {target_path}")
+            book.events.append(
+                f"{getnow()}: left book at identical location "
+                f"{target_location.full_str}"
+            )
+            continue
+
+        source_path = source_location.full_local_path(
             ShuttleContext.local_warehouse_paths
         )
-
         target_path.parent.mkdir(parents=True, exist_ok=True)
-        shutil.copy(current_path, target_path)
-        current_path.unlink()
-        logger.debug(f"Moved book {book.id} from {current_path} to {target_path}")
+        shutil.copy(source_path, target_path)
+        logger.debug(f"Copied book {book.id} from {source_path} to {target_path}")
         book.events.append(
-            f"{getnow()}: moved book from {current_location.full_str} to "
+            f"{getnow()}: copied book from {source_location.full_str} to "
             f"{target_location.full_str}"
         )
-        current_locations.remove(current_location)
-        target_locations.remove(target_location)
-        book.locations.remove(current_location)
-        session.delete(current_location)
-        session.flush()
         target_location.status = "current"
 
-    # cleanup phase: delete extra current locations
-    while len(current_locations) > 0:
-        current_location = current_locations[0]
-        current_path = current_location.full_local_path(
+        # After making one copy, delete one of the current locations
+        # that is not the original to avoid filling disk up with copies
+        if len(current_locations) > 1:
+            loc_to_delete = current_locations.pop()
+            del_path = loc_to_delete.full_local_path(
+                ShuttleContext.local_warehouse_paths
+            )
+            del_path.unlink(missing_ok=True)
+            logger.debug(f"Deleted book {book.id} from {del_path}")
+            book.events.append(
+                f"{getnow()}: deleted book from {loc_to_delete.full_str}"
+            )
+            book.locations.remove(loc_to_delete)
+            session.delete(loc_to_delete)
+
+    # Cleanup the original source location and any extra location (if we
+    # started with more currents than targets)
+    for current_location in current_locations:
+        del_path = current_location.full_local_path(
             ShuttleContext.local_warehouse_paths
         )
-
-        current_path.unlink(missing_ok=True)
-        logger.debug(
-            f"Deleted extra current location for book {book.id} at {current_path}"
-        )
-        book.events.append(
-            f"{getnow()}: deleted old location {current_location.full_str}"
-        )
-        current_locations.remove(current_location)
+        del_path.unlink(missing_ok=True)
+        logger.debug(f"Deleted book {book.id} from {del_path}")
+        book.events.append(f"{getnow()}: deleted book from {current_location.full_str}")
         book.locations.remove(current_location)
         session.delete(current_location)
 
     book.needs_file_operation = False
+    session.flush()

backend/tests/shuttle/test_move_files.py

@@ -141,9 +141,9 @@ def test_move_book_files_copy_operation(
     assert book.needs_processing is False
     assert book.has_error is False
     assert book.needs_file_operation is False
+    assert sum(1 for loc in book.locations if loc.status == "current") == 2
     assert any("copied book from" in event for event in book.events)
-    # One target should now be current
-    assert sum(1 for loc in book.locations if loc.status == "current") >= 1
+    assert any("deleted book" in event for event in book.events)
 
 
 def test_move_book_files_move_operation(
@@ -180,14 +180,15 @@ def test_move_book_files_move_operation(
         mock_context.local_warehouse_paths = {warehouse.id: Path("/warehouse")}
         move_book_files(dbsession, book)
 
-        # Should have moved once
+        # Should have copied once and unlinked once
         assert mock_copy.call_count == 1
         assert mock_unlink.call_count == 1
 
     assert book.needs_processing is False
     assert book.has_error is False
     assert book.needs_file_operation is False
-    assert any("moved book from" in event for event in book.events)
+    assert any("copied book from" in event for event in book.events)
+    assert any("deleted book" in event for event in book.events)
     # Current location should be removed
     assert len([loc for loc in book.locations if loc.status == "current"]) == 1
 
@@ -229,14 +230,70 @@ def test_move_book_files_delete_operation(
         mock_context.local_warehouse_paths = {warehouse.id: Path("/warehouse")}
         move_book_files(dbsession, book)
 
-        # Should have deleted one extra location
+        # Should have copied once and deleted two extra locations
         assert mock_copy.call_count == 1
         assert mock_unlink.call_count == 2
 
     assert book.needs_processing is False
     assert book.has_error is False
     assert book.needs_file_operation is False
-    assert any("deleted old location" in event for event in book.events)
+    assert any("copied book from" in event for event in book.events)
+    assert any("deleted book" in event for event in book.events)
+    assert len([loc for loc in book.locations if loc.status == "current"]) == 1
+
+
+def test_move_book_files_identical_source_and_target_paths(
+    dbsession: OrmSession,
+    create_book: Callable[..., Book],
+    create_book_location: Callable[..., BookLocation],
+    create_warehouse: Callable[..., Warehouse],
+):
+    """
+    Test that move_book_files does not copy or delete if a target path matches a
+    current path
+    """
+    warehouse = create_warehouse()
+    book = create_book()
+    book.needs_file_operation = True
+    dbsession.flush()
+
+    # One current location
+    create_book_location(
+        book=book, warehouse_id=warehouse.id, path="same_path", status="current"
+    )
+
+    # One target location with exactly the same path
+    create_book_location(
+        book=book, warehouse_id=warehouse.id, path="same_path", status="target"
+    )
+    # And another target location with a different path
+    create_book_location(
+        book=book, warehouse_id=warehouse.id, path="new_path", status="target"
+    )
+    dbsession.flush()
+
+    with ExitStack() as stack:
+        mock_context = stack.enter_context(
+            patch("cms_backend.shuttle.move_files.ShuttleContext")
+        )
+        mock_copy = stack.enter_context(patch("shutil.copy"))
+        mock_unlink = stack.enter_context(patch("pathlib.Path.unlink"))
+        stack.enter_context(patch("pathlib.Path.mkdir"))
+
+        mock_context.local_warehouse_paths = {warehouse.id: Path("/warehouse")}
+        move_book_files(dbsession, book)
+
+        # Should only copy once (to "new_path") because "same_path" is identical
+        assert mock_copy.call_count == 1
+        # Should NOT unlink "same_path" because it's in the target paths
+        assert mock_unlink.call_count == 0
+
+    assert book.needs_processing is False
+    assert book.has_error is False
+    assert book.needs_file_operation is False
+    assert sum(1 for loc in book.locations if loc.status == "current") == 2
+    assert any("left book at" in event for event in book.events)
+    assert any("copied book from" in event for event in book.events)
 
 
 def test_move_book_files_updates_book_locations(