fix: ignore duplicate matches whose file is gone
The hash lives in the database and the file does not, so a file deleted behind the app's back went on refusing its own replacement. Matches are now checked against disk, and re-adding a book's own missing file writes it back into the row that already describes it.
This commit is contained in:
+11
-2
@@ -128,9 +128,18 @@ The policy differs by how deliberate the import is:
|
||||
| `create_many_from_existing_files` (consume watcher) | Skips, and **moves the file to `CHITAI_DUPLICATE_PATH/<library slug>/`** — nothing is deleted, and it cannot stay put because `watchfiles` only reports additions. That path must stay outside `consume_path` or the watcher re-imports it and tries to read the directory name as a library slug. |
|
||||
|
||||
`allow_duplicates=true` overrides all of it, on every endpoint. Keep that working — the
|
||||
hash is not proof of identity, so a false positive has to be recoverable.
|
||||
hash is not proof of identity, so a wrong verdict has to be recoverable, and a scripted
|
||||
import needs a way through. The **web UI deliberately does not offer it**: storing the
|
||||
same bytes twice splits reading progress and shelf membership across two records that
|
||||
can never converge, which is nothing anyone wants on purpose.
|
||||
|
||||
Two things to preserve when touching this code:
|
||||
Three things to preserve when touching this code:
|
||||
|
||||
- **A match only counts while the file is on disk.** `find_duplicate_files` stats each
|
||||
candidate, and `add_files` writes a missing file back into the row that already
|
||||
describes it (`_restore_file`) instead of adding a second row beside it. The hash
|
||||
lives in the database and the file does not, so without this a file deleted behind
|
||||
the app's back would go on refusing its own replacement.
|
||||
|
||||
- **Screening runs before anything is written.** `fingerprint_upload` reads the spooled
|
||||
upload and rewinds it; the resulting fingerprints are handed to `_save_book_files`,
|
||||
|
||||
@@ -71,6 +71,9 @@ FileT = TypeVar("FileT", UploadFile, Path)
|
||||
# size makes a false match require both.
|
||||
Fingerprint = tuple[str, int]
|
||||
|
||||
# How much of a file is held in memory at a time while it is written to disk.
|
||||
CHUNK_SIZE = 262144 # 256 KiB
|
||||
|
||||
|
||||
@dataclass(frozen=True)
|
||||
class DuplicateFile:
|
||||
@@ -240,6 +243,11 @@ class BookService(SQLAlchemyAsyncRepositoryService[Book]):
|
||||
"""
|
||||
Look up which of the given fingerprints are already stored.
|
||||
|
||||
A row only counts while its bytes are still on disk. The hash lives in the
|
||||
database and the file does not, so a file removed behind the app's back would
|
||||
otherwise go on refusing its own replacement — telling the reader the library
|
||||
holds something it cannot open.
|
||||
|
||||
Args:
|
||||
fingerprints: The `(hash, size)` pairs to look for.
|
||||
library: The library being imported into. The search is scoped to it
|
||||
@@ -262,6 +270,7 @@ class BookService(SQLAlchemyAsyncRepositoryService[Book]):
|
||||
Book.id,
|
||||
Book.title,
|
||||
Book.library_id,
|
||||
Book.path,
|
||||
)
|
||||
.join(Book, FileMetadata.book_id == Book.id)
|
||||
.where(tuple_(FileMetadata.hash, FileMetadata.size).in_(wanted))
|
||||
@@ -276,17 +285,20 @@ class BookService(SQLAlchemyAsyncRepositoryService[Book]):
|
||||
rows = await self.repository.session.execute(statement)
|
||||
|
||||
matches: dict[Fingerprint, DuplicateFile] = {}
|
||||
for file_hash, size, path, book_id, title, library_id in rows:
|
||||
matches.setdefault(
|
||||
(file_hash, size),
|
||||
DuplicateFile(
|
||||
filename=Path(path).name,
|
||||
hash=file_hash,
|
||||
size=size,
|
||||
library_id=library_id,
|
||||
book_id=book_id,
|
||||
book_title=title,
|
||||
),
|
||||
for file_hash, size, path, book_id, title, library_id, book_path in rows:
|
||||
if (file_hash, size) in matches:
|
||||
continue
|
||||
|
||||
if not book_path or not await aios.path.isfile(Path(book_path) / path):
|
||||
continue
|
||||
|
||||
matches[(file_hash, size)] = DuplicateFile(
|
||||
filename=Path(path).name,
|
||||
hash=file_hash,
|
||||
size=size,
|
||||
library_id=library_id,
|
||||
book_id=book_id,
|
||||
book_title=title,
|
||||
)
|
||||
|
||||
return matches
|
||||
@@ -810,10 +822,22 @@ class BookService(SQLAlchemyAsyncRepositoryService[Book]):
|
||||
fingerprints = await self._fingerprint_uploads(files)
|
||||
|
||||
if not allow_duplicates:
|
||||
# A file this book already carries is not a conflict — adding it again
|
||||
# asks for a state that already holds, so there is simply nothing to do.
|
||||
own = {(file.hash, file.size) for file in book.files}
|
||||
files = [file for file in files if fingerprints[file.filename] not in own]
|
||||
# A file this book already carries is not a conflict — adding it again asks
|
||||
# for a state that already holds, so there is nothing to do. Unless its
|
||||
# bytes have gone missing, in which case that state does not hold: the
|
||||
# upload fills the existing row back in rather than earning the book a
|
||||
# second row for the same file.
|
||||
own = {(file.hash, file.size): file for file in book.files}
|
||||
remaining = []
|
||||
|
||||
for file in files:
|
||||
stored = own.get(fingerprints[file.filename])
|
||||
if stored is None:
|
||||
remaining.append(file)
|
||||
else:
|
||||
await self._restore_file(book, stored, file)
|
||||
|
||||
files = remaining
|
||||
|
||||
known = await self.find_duplicate_files(
|
||||
(fingerprints[file.filename] for file in files), library
|
||||
@@ -834,6 +858,32 @@ class BookService(SQLAlchemyAsyncRepositoryService[Book]):
|
||||
book.files.extend(new_files)
|
||||
await self.update_book(book.id, {"files": [file for file in book.files]}, library)
|
||||
|
||||
@staticmethod
|
||||
async def _restore_file(
|
||||
book: Book, stored: FileMetadata, upload: UploadFile
|
||||
) -> None:
|
||||
"""
|
||||
Put one of a book's own files back on disk, if it is no longer there.
|
||||
|
||||
Args:
|
||||
book: The book the file belongs to.
|
||||
stored: The row saying where the file should be.
|
||||
upload: The uploaded copy of it.
|
||||
"""
|
||||
if not book.path:
|
||||
return
|
||||
|
||||
path = Path(book.path) / stored.path
|
||||
if await aios.path.isfile(path):
|
||||
return
|
||||
|
||||
path.parent.mkdir(parents=True, exist_ok=True)
|
||||
await upload.seek(0)
|
||||
|
||||
async with aiofiles.open(path, "wb") as dest:
|
||||
while chunk := await upload.read(CHUNK_SIZE):
|
||||
await dest.write(chunk)
|
||||
|
||||
async def remove_files(
|
||||
self, book_id: int, file_ids: list[int], delete_files: bool, library: Library
|
||||
) -> None:
|
||||
@@ -1090,7 +1140,6 @@ class BookService(SQLAlchemyAsyncRepositoryService[Book]):
|
||||
data["path"] = str(parent)
|
||||
|
||||
file_metadata = []
|
||||
CHUNK_SIZE = 262144 # 256 KiB
|
||||
for file in data.pop("files", []):
|
||||
if TYPE_CHECKING:
|
||||
file: UploadFile
|
||||
|
||||
@@ -595,3 +595,48 @@ class TestBookPathCollisions:
|
||||
assert len(book.files) == 2
|
||||
for file in book.files:
|
||||
assert (Path(book.path) / file.path).is_file()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
class TestMissingFiles:
|
||||
"""A row whose bytes are gone must not stand in for the file itself."""
|
||||
|
||||
async def test_a_missing_file_is_not_a_duplicate(
|
||||
self, books_service: BookService, test_library: m.Library
|
||||
) -> None:
|
||||
"""Otherwise the library refuses to take back a file it can no longer open."""
|
||||
created = await books_service.create_many_from_files(
|
||||
BooksCreateFromFiles(files=[upload(EPUB)]), test_library
|
||||
)
|
||||
book = await books_service.get(created.books[0].id)
|
||||
(Path(book.path) / book.files[0].path).unlink()
|
||||
|
||||
result = await books_service.create_many_from_files(
|
||||
BooksCreateFromFiles(files=[upload(EPUB)]), test_library
|
||||
)
|
||||
|
||||
assert len(result.books) == 1
|
||||
assert result.duplicates == []
|
||||
|
||||
restored = await books_service.get(result.books[0].id)
|
||||
assert (Path(restored.path) / restored.files[0].path).is_file()
|
||||
|
||||
async def test_re_adding_a_missing_file_puts_it_back(
|
||||
self, books_service: BookService, test_library: m.Library
|
||||
) -> None:
|
||||
"""The book already has a row for it, so the bytes go back where it says."""
|
||||
created = await books_service.create_many_from_files(
|
||||
BooksCreateFromFiles(files=[upload(EPUB)]), test_library
|
||||
)
|
||||
book_id = created.books[0].id
|
||||
|
||||
book = await books_service.get(book_id)
|
||||
path = Path(book.path) / book.files[0].path
|
||||
path.unlink()
|
||||
|
||||
await books_service.add_files(book_id, [upload(EPUB)], test_library)
|
||||
|
||||
book = await books_service.get(book_id)
|
||||
assert len(book.files) == 1
|
||||
assert path.is_file()
|
||||
assert path.read_bytes() == EPUB.read_bytes()
|
||||
|
||||
Reference in New Issue
Block a user