Skip to content

fix(docreader): resolve EPUB images relative to their chapters - #3924

Open
SuperSgdk wants to merge 2 commits into
Tencent:mainfrom
SuperSgdk:fix/docreader-epub-image-resolution
Open

SuperSgdk wants to merge 2 commits into
Tencent:mainfrom
SuperSgdk:fix/docreader-epub-image-resolution

Conversation

@SuperSgdk

@SuperSgdk SuperSgdk commented Oct 1, 2026 •

Copy link
Copy Markdown

Description

The builtin DocReader can successfully parse an EPUB while associating a chapter with the wrong embedded image. When first/pic.png, second/pic.png and a root-level pic.png coexist, <img src="pic.png"> in first/chapter.xhtml can resolve to another resource because a basename alias is checked before the chapter-relative path.

Resolve paths relative to the chapter first, retain basename matching as the final fallback, and prevent basename aliases from overwriting real archive-root paths. Both ebooklib parsing and the ZIP fallback use this logic. Regression tests generate EPUBs with distinct real PNGs and compare the bytes behind each Markdown reference.

The follow-up also preserves literal #, ? and percent-encoded-looking characters in archive paths. Archive entry names from EbookLib and ZIP are already decoded paths; treating them as URI references could truncate those names or decode them twice and select a different image. Only the HTML image src is treated as a URI reference: remove its query and fragment before decoding once, then resolve it relative to the chapter. Additional byte-level regression cases exercise both EbookLib and the ZIP fallback.

Type of Change

  • 🐛 Bug fix
  • 🧪 Test

Related Issue

No separate issue filed. All-state EPUB and image/basename searches were refreshed on 2026-10-02; no directly matching existing issue or PR was found.

Testing

Latest PR head

On 2026-10-02, validated the current head 5f952f85843cf231068428e2ae66a484771bf4de with the EPUB unit suite on Windows using CPython 3.10.18 and the existing installed dependencies. The production parser and test files were fetched at that exact commit, verified against their Git blob hashes, and executed from an isolated temporary source copy. Module paths and the parser registry were checked to ensure they used that same updated parser.

  • All 9 EPUB test methods pass, including 19 successful subTest executions; 0 failures, 0 errors and 0 skips. The two newly added methods each cover 7 literal-path cases: 14 new subtests across EbookLib parsing and ZIP fallback. The remaining 5 subtests cover the original chapter-relative and image-order cases.
  • git diff --check origin/main...HEAD: passes on this latest head, with a clean source working tree.

The full Python suite, production gRPC harness, Go clients, full repository Go tests, formatting and lint were not rerun on this latest head. Their results below apply to the initial commit only.

Initial commit: historical Linux validation

Validated the initial PR commit 4d662b9c28e82d0f71a0f75e1405b82b3be22898 on GitHub-hosted Ubuntu Linux using CPython 3.10.18, locked docreader/uv.lock dependencies and Go 1.26.0. The validation workflow is on a separate fork branch and is not part of this PR.

  • python -m compileall -q docreader: passes.
  • python -m unittest discover -s docreader/tests -p 'test_*.py' -v: 266 tests, 254 pass, 12 skip, no failures or errors. Eleven skips require unavailable fixtures; one requires ImageMagick. All seven EPUB test methods pass.
  • The EPUB tests from that initial commit against the unchanged parser at bccb4b151bae403508da77fbb174efc79dc47c1a, with the test registry pointing to that same parser: six expected failures across the new regression cases; all four original test methods pass.
  • A gRPC harness using the production DocReaderServicer verifies the actual PNG bytes for unary Read, streaming ReadStream, and Read with ZIP fallback. All three paths pass; no model calls.
  • go test ./docreader/client ./docreader/proto -count=1 -v, with a live DocReader service: TestReadURL and TestReadFile pass, no skips; proto has no test files. The URL test reads https://example.com.
  • Root make test (go test -v ./...): 115 packages pass, no failed packages or tests; 46 tests/subtests skip under existing conditions for PostgreSQL, model credentials, native browser/sandbox tools, SQLite FTS5 and other optional integration resources. Skipped cases are not counted as passed.
  • Root make fmt: passes with no Go diff. git diff --check against the PR baseline passes.
  • Root make lint, golangci-lint v2.12.2: does not pass; reports 246 findings in existing Go source. This PR changes no Go source, lint configuration or dependencies. Lint findings remain explicitly reported and were not fixed or suppressed as part of this Python-only change.

Inspect these historical logs for the initial commit's underlying test outcomes, rather than relying only on the workflow's aggregate green status:

These historical Linux checks were run by the contributor for the initial commit. Tencent's upstream DocReader workflow still requires maintainer authorization. Frontend checks and a full deployed upload/index/query end-to-end run are outside the validation performed for this parser fix. No unrelated source or test assertions were changed.

Checklist

  • git diff --check origin/main...HEAD passes
  • Changed source files follow the existing Python style
  • Targeted tests for the changed parser pass
  • Diff-scoped lint (no Python lint task configured; Go files unchanged)
  • Full-suite baseline/environment limitations documented above
  • Contributor human self-review of the code
  • Added regression tests covering the change
  • Documentation update (not needed for this existing-format bug fix)
  • No breaking changes

AI assistance

Codex assisted with investigation, reproduction, implementation, tests and PR preparation.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant