fix: keep embedded metadata and group formats on directory upload
The filepath extractor was merged on the right, so a parent folder's name overrode the title inside the file. Grouping also split a book's formats when its own folder was the upload root.
This commit is contained in:
@@ -152,15 +152,24 @@ class BookService(SQLAlchemyAsyncRepositoryService[Book]):
|
|||||||
if not data.files:
|
if not data.files:
|
||||||
raise ValueError("Must upload at least one file")
|
raise ValueError("Must upload at least one file")
|
||||||
|
|
||||||
# Group book files if they are within the same nested directory
|
# Group the files that make up a single book.
|
||||||
books = defaultdict(list)
|
#
|
||||||
|
# `file.filename` carries the browser's webkitRelativePath, whose first
|
||||||
|
# component is whichever folder the user picked. Depth is therefore relative
|
||||||
|
# to the selection, not to the library: picking a book's own folder and
|
||||||
|
# picking the shelf above it submit the same files at different depths.
|
||||||
|
#
|
||||||
|
# Inside a subdirectory the parent directory is unambiguously the book. Sitting
|
||||||
|
# directly in the selected folder it is genuinely ambiguous — "Fluent Python.epub"
|
||||||
|
# alongside "Fluent Python.pdf" is one book in two formats, while "a.epub"
|
||||||
|
# alongside "b.epub" is two books — so the filename stem breaks the tie.
|
||||||
|
books: dict[tuple[Path, str], list[UploadFile]] = defaultdict(list)
|
||||||
for file in data.files:
|
for file in data.files:
|
||||||
filepath = Path(file.filename)
|
filepath = Path(file.filename)
|
||||||
# Books within the root directory should be treated as separate books
|
|
||||||
if len(filepath.parent.parts) > 1:
|
if len(filepath.parent.parts) > 1:
|
||||||
books[filepath.parent].append(file)
|
books[(filepath.parent, "")].append(file)
|
||||||
else:
|
else:
|
||||||
books[filepath].append(file)
|
books[(filepath.parent, filepath.stem)].append(file)
|
||||||
|
|
||||||
return [
|
return [
|
||||||
await self.create_book(
|
await self.create_book(
|
||||||
|
|||||||
@@ -67,8 +67,11 @@ class Extractor:
|
|||||||
for file in files:
|
for file in files:
|
||||||
metadata = FilenameExtractor.extract_metadata(file) | metadata
|
metadata = FilenameExtractor.extract_metadata(file) | metadata
|
||||||
|
|
||||||
# Get metadata from filepath
|
# Get metadata from filepath. Kept on the left so that anything the file
|
||||||
metadata = metadata | FilepathExtractor.extract_metadata(files[0], root_path)
|
# itself declared outranks a guess made from its directory names — a folder
|
||||||
|
# called "Fluent Python - Luciano Ramalho" must not overwrite the title the
|
||||||
|
# EPUB already carries.
|
||||||
|
metadata = FilepathExtractor.extract_metadata(files[0], root_path) | metadata
|
||||||
|
|
||||||
# format the title
|
# format the title
|
||||||
if metadata.get('title', None):
|
if metadata.get('title', None):
|
||||||
|
|||||||
@@ -369,6 +369,61 @@ async def test_create_multiple_books_from_directory(
|
|||||||
assert len(data.get("items") or data.get("data")) >= 1
|
assert len(data.get("items") or data.get("data")) >= 1
|
||||||
|
|
||||||
|
|
||||||
|
async def test_create_books_from_parent_directory_keeps_embedded_title(
|
||||||
|
authenticated_client: AsyncClient,
|
||||||
|
) -> None:
|
||||||
|
"""A folder name in the upload path must not override the file's own metadata.
|
||||||
|
|
||||||
|
The browser sends webkitRelativePath, so picking the shelf above a book's folder
|
||||||
|
submits one more path component than picking the folder itself. That extra level
|
||||||
|
used to make the directory name win over the title inside the EPUB.
|
||||||
|
"""
|
||||||
|
source = Path("tests/data_files/Metamorphosis - Franz Kafka.epub")
|
||||||
|
files = [
|
||||||
|
(
|
||||||
|
"files",
|
||||||
|
(
|
||||||
|
"Shelf/Metamorphosis - Franz Kafka/Metamorphosis.epub",
|
||||||
|
source.read_bytes(),
|
||||||
|
"application/epub+zip",
|
||||||
|
),
|
||||||
|
)
|
||||||
|
]
|
||||||
|
|
||||||
|
response = await authenticated_client.post(
|
||||||
|
"/books/fromFiles?library_id=1", files=files, data={"library_id": 1}
|
||||||
|
)
|
||||||
|
|
||||||
|
assert response.status_code == 201
|
||||||
|
|
||||||
|
books = response.json()["items"]
|
||||||
|
assert len(books) == 1
|
||||||
|
assert books[0]["title"] == "Metamorphosis"
|
||||||
|
|
||||||
|
|
||||||
|
async def test_create_books_groups_formats_within_one_folder(
|
||||||
|
authenticated_client: AsyncClient,
|
||||||
|
) -> None:
|
||||||
|
"""Picking a book's own folder yields one book with both formats, not two books."""
|
||||||
|
epub = Path("tests/data_files/Metamorphosis - Franz Kafka.epub").read_bytes()
|
||||||
|
pdf = Path("tests/data_files/Calculus Made Easy - Silvanus Thompson.pdf").read_bytes()
|
||||||
|
|
||||||
|
files = [
|
||||||
|
("files", ("Metamorphosis/Metamorphosis.epub", epub, "application/epub+zip")),
|
||||||
|
("files", ("Metamorphosis/Metamorphosis.pdf", pdf, "application/pdf")),
|
||||||
|
]
|
||||||
|
|
||||||
|
response = await authenticated_client.post(
|
||||||
|
"/books/fromFiles?library_id=1", files=files, data={"library_id": 1}
|
||||||
|
)
|
||||||
|
|
||||||
|
assert response.status_code == 201
|
||||||
|
|
||||||
|
books = response.json()["items"]
|
||||||
|
assert len(books) == 1
|
||||||
|
assert len(books[0]["files"]) == 2
|
||||||
|
|
||||||
|
|
||||||
# NOTE: the multi-book ZIP download is covered at the service level, in
|
# NOTE: the multi-book ZIP download is covered at the service level, in
|
||||||
# tests/unit/test_services/test_book_service.py. Driving `/books/download` through
|
# tests/unit/test_services/test_book_service.py. Driving `/books/download` through
|
||||||
# AsyncTestClient hangs in fixture teardown: it is the only `Stream` endpoint in the
|
# AsyncTestClient hangs in fixture teardown: it is the only `Stream` endpoint in the
|
||||||
|
|||||||
Reference in New Issue
Block a user