feat(bookdrop): capture embedded file metadata into OriginalMetadata at ingest (bookshelf-sp5u) #354

Merged

zombor merged 3 commits from bd-bookshelf-sp5u into main 2026-06-05 16:25:58 +00:00

Conversation 2 Commits 3 Files changed 6 +193 -14

zombor commented 2026-06-05 15:20:20 +00:00

Owner

Copy link

Summary

Resolves the TODO at internal/bookdrop/service.go:119 — OriginalMetadata was always stored as null, so the bookdrop review UI had no way to show what a file itself claims vs fetched metadata.

Placement decision: extraction at ingest time (detection step)

The extraction is placed in IngestFile (the detection/watcher step), not deferred to the accept/review flow. Rationale:

• scan.ExtractMetadata for EPUB opens only the small OPF XML entry inside the zip using io.ReaderAt (seek-based, no full-file buffering). For non-EPUB formats it falls back to filename parsing — zero I/O.
• The watch-folder already filters to book extensions (bookFormats map), so no non-book files are processed.
• Storing the metadata at proposal creation time means the review UI can compare embedded vs. fetched metadata immediately, without an extra DB round-trip at review time.
• Extraction failure is non-fatal: logs Warn, stores null, file is still ingested.

Changes

• internal/bookdrop/meta.go (new): ExtractFileMetadata(absPath, logger) — wraps scan.ExtractMetadata via os.DirFS, serialises {title, authors} as JSON into a sql.NullString.
• internal/bookdrop/service.go: IngestFile gains an extractMeta func(string) sql.NullString dep (curried pattern); populates OriginalMetadata from it.
• internal/app/app.go + internal/app/build_extended_deps.go: both IngestFile wiring sites updated to pass bookdrop.ExtractFileMetadata.
• internal/bookdrop/meta_test.go (new): tests for EPUB with embedded title+author, graceful null on missing file.
• internal/bookdrop/service_test.go: existing tests updated for new dep; new contexts assert OriginalMetadata is populated and that null extraction still ingests cleanly.

Test plan

• go build ./... clean
• make lint no new issues in bookdrop
• go test ./internal/bookdrop/... pass
• scripts/check-coverage.sh 100% (coverage gate passes)

Closes bead bookshelf-sp5u on merge.

## Summary Resolves the TODO at `internal/bookdrop/service.go:119` — `OriginalMetadata` was always stored as null, so the bookdrop review UI had no way to show what a file itself claims vs fetched metadata. **Placement decision: extraction at ingest time (detection step)** The extraction is placed in `IngestFile` (the detection/watcher step), not deferred to the accept/review flow. Rationale: - `scan.ExtractMetadata` for EPUB opens only the small OPF XML entry inside the zip using `io.ReaderAt` (seek-based, no full-file buffering). For non-EPUB formats it falls back to filename parsing — zero I/O. - The watch-folder already filters to book extensions (`bookFormats` map), so no non-book files are processed. - Storing the metadata at proposal creation time means the review UI can compare embedded vs. fetched metadata immediately, without an extra DB round-trip at review time. - Extraction failure is non-fatal: logs Warn, stores null, file is still ingested. ## Changes - `internal/bookdrop/meta.go` (new): `ExtractFileMetadata(absPath, logger)` — wraps `scan.ExtractMetadata` via `os.DirFS`, serialises `{title, authors}` as JSON into a `sql.NullString`. - `internal/bookdrop/service.go`: `IngestFile` gains an `extractMeta func(string) sql.NullString` dep (curried pattern); populates `OriginalMetadata` from it. - `internal/app/app.go` + `internal/app/build_extended_deps.go`: both `IngestFile` wiring sites updated to pass `bookdrop.ExtractFileMetadata`. - `internal/bookdrop/meta_test.go` (new): tests for EPUB with embedded title+author, graceful null on missing file. - `internal/bookdrop/service_test.go`: existing tests updated for new dep; new contexts assert `OriginalMetadata` is populated and that null extraction still ingests cleanly. ## Test plan - [x] `go build ./...` clean - [x] `make lint` no new issues in bookdrop - [x] `go test ./internal/bookdrop/...` pass - [x] `scripts/check-coverage.sh` 100% (coverage gate passes) Closes bead bookshelf-sp5u on merge.

zombor added 1 commit 2026-06-05 15:20:21 +00:00

feat(bookdrop): capture embedded file metadata into OriginalMetadata at ingest ... e77dfa87ca

Adds ExtractFileMetadata in internal/bookdrop/meta.go which calls the
existing scan.ExtractMetadata (EPUB OPF + filename fallback) and
serialises title+authors as JSON into the sql.NullString stored in
bookdrop_file.original_metadata. Extraction failure is non-fatal: logs
Warn, stores null, file is still ingested. IngestFile gains an
extractMeta func dep (curried pattern); both app wiring sites updated.
100% coverage maintained.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

zombor added 1 commit 2026-06-05 15:30:50 +00:00

ci: retry (books_bulk_delete timing flake unrelated to this PR) d0a0165007

zombor added 1 commit 2026-06-05 15:52:16 +00:00

docs(bookdrop): tighten ExtractFileMetadata godoc aaaf21898b

zombor commented 2026-06-05 16:03:07 +00:00

Author

Owner

Copy link

[MINOR] internal/bookdrop/meta.go — ExtractFileMetadata called synchronously on every detected file; 256 MB fallback buffer reachable in tests but not in production (os.DirFS files expose io.ReaderAt, so production always uses the zero-copy path; the 256 MB limit only fires in test or non-os-FS contexts).

Extraction failure is non-fatal: error is logged at Warn level and a null NullString is returned; the ingest continues normally. Confirmed.

No path traversal: ExtractFileMetadata constructs os.DirFS(filepath.Dir(absPath)) and opens only filepath.Base(absPath) — the scan can never escape the dropped file's own directory.

No XXE: Go's encoding/xml decoder does not expand external entity references (DTD processing is not supported); no XXE risk.

XML decompression-bomb protection: decodeXMLEntry wraps every XML read in io.LimitReader(rc, maxXMLBytes) (1 MB). Both container.xml and the OPF are capped before decoding.

ZIP decompression-bomb protection: on the preferred code path (os.File → io.ReaderAt) zip.NewReader opens the central directory without reading compressed data; individual entries are only opened for XML, not for content/images. No uncontrolled memory allocation.

Author-flood protection: maxAuthors = 20 caps the authors slice before any DB round-trips.

No panic path: all errors from the parsing stack return (BookMetadata{}, nil) for parse failures and (BookMetadata{}, error) for I/O failures; both are caught in ExtractFileMetadata and degrade gracefully.

JSON storage safety: json.Marshal on a struct with only string / []string fields cannot produce injection artefacts. OriginalMetadata is not rendered in any template or API endpoint in the current diff — there is no XSS surface in this PR.

[MINOR] internal/files/cover_extract.go:507 — slog.Default() used inside decodeXMLEntry (pre-existing, not introduced by this PR). This violates the project's "inject logger, never use global" convention. Not a security issue but worth noting; pre-existing MINOR.

REVIEW VERDICT: 0 blocker, 0 major, 2 minor

[MINOR] internal/bookdrop/meta.go — `ExtractFileMetadata` called synchronously on every detected file; 256 MB fallback buffer reachable in tests but not in production (os.DirFS files expose io.ReaderAt, so production always uses the zero-copy path; the 256 MB limit only fires in test or non-os-FS contexts). Extraction failure is non-fatal: error is logged at Warn level and a null NullString is returned; the ingest continues normally. Confirmed. No path traversal: `ExtractFileMetadata` constructs `os.DirFS(filepath.Dir(absPath))` and opens only `filepath.Base(absPath)` — the scan can never escape the dropped file's own directory. No XXE: Go's `encoding/xml` decoder does not expand external entity references (DTD processing is not supported); no XXE risk. XML decompression-bomb protection: `decodeXMLEntry` wraps every XML read in `io.LimitReader(rc, maxXMLBytes)` (1 MB). Both container.xml and the OPF are capped before decoding. ZIP decompression-bomb protection: on the preferred code path (`os.File` → `io.ReaderAt`) `zip.NewReader` opens the central directory without reading compressed data; individual entries are only opened for XML, not for content/images. No uncontrolled memory allocation. Author-flood protection: `maxAuthors = 20` caps the authors slice before any DB round-trips. No panic path: all errors from the parsing stack return `(BookMetadata{}, nil)` for parse failures and `(BookMetadata{}, error)` for I/O failures; both are caught in `ExtractFileMetadata` and degrade gracefully. JSON storage safety: `json.Marshal` on a struct with only `string` / `[]string` fields cannot produce injection artefacts. `OriginalMetadata` is not rendered in any template or API endpoint in the current diff — there is no XSS surface in this PR. [MINOR] internal/files/cover_extract.go:507 — `slog.Default()` used inside `decodeXMLEntry` (pre-existing, not introduced by this PR). This violates the project's "inject logger, never use global" convention. Not a security issue but worth noting; pre-existing MINOR. REVIEW VERDICT: 0 blocker, 0 major, 2 minor

zombor commented 2026-06-05 16:04:17 +00:00

Author

Owner

Copy link

CODE REVIEW: APPROVED - minor issue only

Phase 0: DEMO Verification

No DEMO block in this bead (metadata extraction at ingest is not user-visible in isolation; the stored value is verified via unit tests). Proceeding on the basis that CI is green.

Phase 1: Spec Compliance

• OriginalMetadata is populated at ingest via extractMeta(absPath) in IngestFile — confirmed in internal/bookdrop/service.go:122.
• scan.ExtractMetadata is reused (no new parser) — confirmed in internal/bookdrop/meta.go.
• Extraction failure is non-fatal: logger.Warn(...) + returns sql.NullString{} so ingest continues — confirmed in internal/bookdrop/meta.go:29-32.
• Both wiring sites updated: internal/app/app.go:514 and internal/app/build_extended_deps.go:351.
• Curried func dep injected as extractMeta func(string) sql.NullString — follows project convention.

Phase 2: Code Quality

Serialization correctness

The JSON shape is {"title":"...","authors":[...]} with omitempty. Fields are omitted (not null/empty-stringed) when absent — correct and compact. json.Marshal suppression (b, _ := json.Marshal(payload)) is safe: the comment is accurate, a struct of string/[]string cannot produce a marshal error.

The column has CHECK (json_valid(...)) — the produced JSON is always valid. StatusFailed also writes JSON {"error":"..."} to the same column via UpdateBookdropFileWithError; that path is not yet wired and overwrites whatever was stored at ingest. This is pre-existing design, not introduced by this PR.

I/O cost at ingest

For non-EPUB formats (PDF, CBZ, CBR, MOBI, M4B, MP3), scan.ExtractMetadata only calls ParseFilenameMetadata — no file open, no I/O beyond what IngestFile already does. os.DirFS(...) creates a lazy FS view with no syscall. EPUB-only ZIP parsing is the only added I/O and that is bounded to a single file per ingest event. Acceptable for a watch-folder.

Extraction called for duplicate files

Extraction happens inside the insert(ctx, sqlc.InsertBookdropFileIfNewParams{..., OriginalMetadata: extractMeta(absPath)}) call — Go evaluates all arguments before the call, so extractMeta runs even when the ON DUPLICATE KEY UPDATE makes the insert a no-op (file already exists in DB). For EPUB files, this means opening and zip-parsing the EPUB on every scan pass for already-seen files. At watch-folder scale with a large backlog, this could add up, but is unlikely to be a bottleneck in practice (the watcher fires per-file, not a bulk scan). Minor concern.

ON DUPLICATE KEY UPDATE does not update original_metadata

If a file is rejected and re-detected, status resets to PENDING_REVIEW but original_metadata retains the value from first ingest. If the file changed on disk between scans (same path, new content), metadata is stale. Pre-existing design limitation; not introduced here.

---

[MINOR] internal/bookdrop/meta_test.go — no test for the filename-fallback path
ExtractFileMetadata with a non-existent file covers the error branch. ExtractFileMetadata with a valid EPUB covers the success branch. There is no test for a non-EPUB file (e.g., "Author - Title.pdf") to confirm the filename-parse result flows through correctly. The two branches in the function are covered, but the contract of the returned JSON for non-EPUB inputs is not asserted. Does not affect coverage gate (both branches covered) but leaves the behavior unspecified by tests.

[MINOR] internal/app/app.go:514-517 — inconsistent lambda indentation
The closure body in app.go has misaligned indentation (the return line is over-indented vs the }), unlike the clean formatting in build_extended_deps.go. gofmt would normalize this. No correctness impact.

REVIEW VERDICT: 0 blocker, 0 major, 2 minor

CODE REVIEW: APPROVED - minor issue only ## Phase 0: DEMO Verification No DEMO block in this bead (metadata extraction at ingest is not user-visible in isolation; the stored value is verified via unit tests). Proceeding on the basis that CI is green. ## Phase 1: Spec Compliance - OriginalMetadata is populated at ingest via `extractMeta(absPath)` in `IngestFile` — confirmed in `internal/bookdrop/service.go:122`. - `scan.ExtractMetadata` is reused (no new parser) — confirmed in `internal/bookdrop/meta.go`. - Extraction failure is non-fatal: `logger.Warn(...)` + returns `sql.NullString{}` so ingest continues — confirmed in `internal/bookdrop/meta.go:29-32`. - Both wiring sites updated: `internal/app/app.go:514` and `internal/app/build_extended_deps.go:351`. - Curried func dep injected as `extractMeta func(string) sql.NullString` — follows project convention. ## Phase 2: Code Quality ### Serialization correctness The JSON shape is `{"title":"...","authors":[...]}` with `omitempty`. Fields are omitted (not null/empty-stringed) when absent — correct and compact. `json.Marshal` suppression (`b, _ := json.Marshal(payload)`) is safe: the comment is accurate, a struct of `string`/`[]string` cannot produce a marshal error. The column has `CHECK (json_valid(...))` — the produced JSON is always valid. `StatusFailed` also writes JSON `{"error":"..."}` to the same column via `UpdateBookdropFileWithError`; that path is not yet wired and overwrites whatever was stored at ingest. This is pre-existing design, not introduced by this PR. ### I/O cost at ingest For non-EPUB formats (PDF, CBZ, CBR, MOBI, M4B, MP3), `scan.ExtractMetadata` only calls `ParseFilenameMetadata` — no file open, no I/O beyond what `IngestFile` already does. `os.DirFS(...)` creates a lazy FS view with no syscall. EPUB-only ZIP parsing is the only added I/O and that is bounded to a single file per ingest event. Acceptable for a watch-folder. ### Extraction called for duplicate files Extraction happens inside the `insert(ctx, sqlc.InsertBookdropFileIfNewParams{..., OriginalMetadata: extractMeta(absPath)})` call — Go evaluates all arguments before the call, so `extractMeta` runs even when the `ON DUPLICATE KEY UPDATE` makes the insert a no-op (file already exists in DB). For EPUB files, this means opening and zip-parsing the EPUB on every scan pass for already-seen files. At watch-folder scale with a large backlog, this could add up, but is unlikely to be a bottleneck in practice (the watcher fires per-file, not a bulk scan). Minor concern. ### `ON DUPLICATE KEY UPDATE` does not update `original_metadata` If a file is rejected and re-detected, status resets to PENDING_REVIEW but `original_metadata` retains the value from first ingest. If the file changed on disk between scans (same path, new content), metadata is stale. Pre-existing design limitation; not introduced here. --- [MINOR] internal/bookdrop/meta_test.go — no test for the filename-fallback path `ExtractFileMetadata` with a non-existent file covers the error branch. `ExtractFileMetadata` with a valid EPUB covers the success branch. There is no test for a non-EPUB file (e.g., `"Author - Title.pdf"`) to confirm the filename-parse result flows through correctly. The two branches in the function are covered, but the contract of the returned JSON for non-EPUB inputs is not asserted. Does not affect coverage gate (both branches covered) but leaves the behavior unspecified by tests. [MINOR] internal/app/app.go:514-517 — inconsistent lambda indentation The closure body in `app.go` has misaligned indentation (the `return` line is over-indented vs the `}`), unlike the clean formatting in `build_extended_deps.go`. `gofmt` would normalize this. No correctness impact. REVIEW VERDICT: 0 blocker, 0 major, 2 minor

zombor force-pushed bd-bookshelf-sp5u from aaaf21898b to 0dcd73ec16 2026-06-05 16:17:52 +00:00 Compare

zombor merged commit f57b92cc80 into main 2026-06-05 16:25:58 +00:00

zombor referenced this pull request from a commit 2026-06-05 16:26:00 +00:00

Merge pull request 'feat(bookdrop): capture embedded file metadata into OriginalMetadata at ingest (bookshelf-sp5u)' (#354) from bd-bookshelf-sp5u into main

zombor referenced this pull request from a commit 2026-08-08 02:34:05 +00:00

fix(review-minors): batch of 8 review-minor follow-ups (bookshelf-ifwpa)

zombor referenced this pull request 2026-08-08 02:34:32 +00:00

fix(review-minors): batch of 8 review-minor follow-ups (bookshelf-ifwpa) #1397

zombor referenced this pull request from a commit 2026-08-08 15:28:50 +00:00

fix(review-minors): batch of 8 review-minor follow-ups (bookshelf-ifwpa)

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

zombor/pergamum!354

No description provided.