From 9ff5ff0098cdd3057ea9ea086c947604ccc64271 Mon Sep 17 00:00:00 2001 From: Jeff Witt <1848307+wittjeff@users.noreply.github.com> Date: Fri, 16 Oct 2026 20:55:01 -0400 Subject: [PATCH] fix: report why a document failed to load, so a password is told from a damaged file qpdf says why it refuses a document (an invalid password, a file it could not repair, ...), but the loaders discarded that and every failure surfaced as the same RuntimeError("Failed to load document with key ..."). docling's default backend wraps that text unchanged, so an encrypted PDF was indistinguishable from a corrupt one (docling#4829). The decoder now records qpdf's error code and message when a load fails; both native parsers keep the record per key until Python collects it through take_load_error(); load() raises PdfLoadError with a category (password, damaged, unsupported, io, unknown) and the qpdf detail, or PdfPasswordError for an encrypted document, which also tells whether a password was supplied. Both are RuntimeError subclasses and keep the old message prefix, so existing handlers keep working. Co-Authored-By: Claude Fable 5.1 Signed-off-by: Jeff Witt <1848307+wittjeff@users.noreply.github.com> --- app/pybind_parse.cpp | 34 ++++++ docling_parse/pdf_parser.py | 91 ++++++++++++++- src/parse/pdf_decoders/document.h | 33 ++++++ src/parse/qpdf/load_error.h | 36 ++++++ src/pybind/docling_parser.h | 47 ++++++++ src/pybind/docling_threaded_base.h | 49 ++++++++ tests/test_unit_load_failure_reason.py | 154 +++++++++++++++++++++++++ 7 files changed, 441 insertions(+), 3 deletions(-) create mode 100644 src/parse/qpdf/load_error.h create mode 100644 tests/test_unit_load_failure_reason.py diff --git a/app/pybind_parse.cpp b/app/pybind_parse.cpp index e5193c14d..d622da707 100644 --- a/app/pybind_parse.cpp +++ b/app/pybind_parse.cpp @@ -848,6 +848,23 @@ PYBIND11_MODULE(pdf_parsers, m) { Returns: List[str]: A list of keys for the currently loaded documents.)") + + .def("take_load_error", + [](docling::docling_parser &self, const std::string &key) -> std::map { + return self.take_load_error(key); + }, + pybind11::arg("key"), + R"( + Return and clear why the last load of `key` failed. + + Parameters: + key (str): The key passed to the failed load call. + + Returns: + dict[str, str]: {"category": ..., "message": ...}, or an empty dict when + no failure is recorded for the key. category is one of "password", + "damaged", "unsupported", "io" and "unknown"; message is the text of + the underlying (qpdf) error.)") .def("load_document", [](docling::docling_parser &self, @@ -1116,6 +1133,23 @@ PYBIND11_MODULE(pdf_parsers, m) { Returns: bool: True if the document was successfully loaded.)") + .def("take_load_error", + [](docling::docling_threaded_parser& self, const std::string& key) -> std::map { + return self.take_load_error(key); + }, + pybind11::arg("key"), + R"( + Return and clear why the last load of `key` failed. + + Parameters: + key (str): The key passed to the failed load call. + + Returns: + dict[str, str]: {"category": ..., "message": ...}, or an empty dict when + no failure is recorded for the key. category is one of "password", + "damaged", "unsupported", "io" and "unknown"; message is the text of + the underlying (qpdf) error.)") + .def("load_document_from_bytesio", [](docling::docling_threaded_parser& self, const std::string& key, diff --git a/docling_parse/pdf_parser.py b/docling_parse/pdf_parser.py index e42d71737..d6c1857ec 100644 --- a/docling_parse/pdf_parser.py +++ b/docling_parse/pdf_parser.py @@ -3,7 +3,7 @@ import hashlib import logging import math -from enum import IntEnum +from enum import Enum, IntEnum from io import BytesIO from pathlib import Path from typing import Any, Dict, Iterator, List, Optional, Sequence, Tuple, Union @@ -77,6 +77,78 @@ _log = logging.getLogger(__name__) +class PdfLoadErrorCategory(str, Enum): + """Why docling-parse could not load a document. + + The values are the categories the native loader reports, see + `src/parse/qpdf/load_error.h`. + """ + + PASSWORD = "password" + """The document is encrypted and the password is missing or wrong.""" + DAMAGED = "damaged" + """qpdf could not repair the file.""" + UNSUPPORTED = "unsupported" + """The file uses a feature qpdf does not implement.""" + IO = "io" + """The file is missing or unreadable.""" + UNKNOWN = "unknown" + + +class PdfLoadError(RuntimeError): + """docling-parse could not load a document. + + A RuntimeError, so handlers written for the former generic error keep + working, and the message keeps its former prefix. `category` says what + went wrong; `detail` is the message of the underlying (qpdf) failure. + """ + + def __init__( + self, + key: str, + category: PdfLoadErrorCategory, + detail: str, + *, + reason: str | None = None, + ): + self.key = key + self.category = category + self.detail = detail + + message = f"Failed to load document with key {key}" + description = reason or detail + if description: + message = f"{message}: {description}" + super().__init__(message) + + +class PdfPasswordError(PdfLoadError): + """The document is encrypted and was opened without a valid password. + + `password_supplied` tells whether the caller gave a (wrong) password or + none at all: qpdf reports both as the same invalid-password error. + """ + + def __init__(self, key: str, detail: str, *, password_supplied: bool): + self.password_supplied = password_supplied + if password_supplied: + reason = "the supplied password is incorrect" + else: + reason = "the document is encrypted and requires a password" + super().__init__(key, PdfLoadErrorCategory.PASSWORD, detail, reason=reason) + + +def _load_error(native_parser, key: str, password: str | None) -> PdfLoadError: + """Build the exception for a failed load from the native parser's record.""" + info = native_parser.take_load_error(key) + category = PdfLoadErrorCategory(info.get("category", PdfLoadErrorCategory.UNKNOWN)) + detail = info.get("message", "") + + if category is PdfLoadErrorCategory.PASSWORD: + return PdfPasswordError(key, detail, password_supplied=password is not None) + return PdfLoadError(key, category, detail) + + class PdfAnnotations(BaseModel): """PDF document annotations including form fields, language, metadata, and table of contents. @@ -1004,7 +1076,14 @@ def load( decode_config: DecodeConfig | None = None, content_config: ContentConfig | None = None, ) -> PdfDocument: + """Load a document and return a handle to its pages. + Raises: + PdfPasswordError: The document is encrypted and `password` is + missing or wrong. + PdfLoadError: The document could not be loaded for another + reason; see its `category` and `detail`. + """ if isinstance(path_or_stream, str): path_or_stream = Path(path_or_stream) @@ -1048,7 +1127,7 @@ def load( return result_doc else: - raise RuntimeError(f"Failed to load document with key {key}") + raise _load_error(self.parser, key, password) def _load_document( self, @@ -1571,6 +1650,12 @@ def load( Returns: str: The document key. + + Raises: + PdfPasswordError: The document is encrypted and `password` is + missing or wrong. + PdfLoadError: The document could not be loaded for another + reason; see its `category` and `detail`. """ if page_numbers is not None and page_range is not None: raise ValueError("page_numbers and page_range are mutually exclusive") @@ -1608,7 +1693,7 @@ def load( ) if not success: - raise RuntimeError(f"Failed to load document with key {key}") + raise _load_error(self._parser, key, password) self._page_counts[key] = self._parser.number_of_pages(key) self._scheduled_page_counts[key] = self._parser.scheduled_number_of_pages(key) diff --git a/src/parse/pdf_decoders/document.h b/src/parse/pdf_decoders/document.h index aa6d21840..21459d33b 100644 --- a/src/parse/pdf_decoders/document.h +++ b/src/parse/pdf_decoders/document.h @@ -11,8 +11,10 @@ #include #include #include +#include #include +#include #include #include @@ -34,6 +36,12 @@ namespace pdflib std::string get_filename() { return filename; } + // Why the last process_document_* call returned false: a category + // from load_error and the message of the underlying failure. Both + // are empty while no load has failed. + const std::string& get_load_error_category() const { return load_error_category; } + const std::string& get_load_error_message() const { return load_error_message; } + nlohmann::json get_annotations(); nlohmann::json get_meta_xml(); @@ -83,6 +91,8 @@ namespace pdflib // object the file does not contain (docling-parse#374). void ensure_page_content_present(int page_number); + void record_load_error(const std::string& category, const std::string& message); + void update_timings(pdf_timings& timings_, bool set_timer); private: @@ -112,6 +122,9 @@ namespace pdflib // and read by the worker threads afterwards. bool qpdf_reported_damage; + std::string load_error_category; + std::string load_error_message; + nlohmann::json json_annots; bool annots_loaded; @@ -235,6 +248,7 @@ namespace pdflib std::ifstream ifs(fs_path, std::ios::binary | std::ios::ate); if(!ifs.is_open()) { + record_load_error(load_error::IO, "could not open file: " + filename); LOG_S(ERROR) << "could not open file: " << filename; return false; } @@ -245,6 +259,7 @@ namespace pdflib auto file_buffer = std::make_shared(file_size, '\0'); if(!ifs.read(file_buffer->data(), file_size)) { + record_load_error(load_error::IO, "could not read file: " + filename); LOG_S(ERROR) << "could not read file: " << filename; return false; } @@ -270,6 +285,9 @@ namespace pdflib buffer = _buffer; password = _password; + load_error_category.clear(); + load_error_message.clear(); + LOG_S(INFO) << "start processing buffer of size " << buffer->size() << " by qpdf ..."; utils::timer timer; @@ -308,8 +326,16 @@ namespace pdflib LOG_S(INFO) << "#-pages: " << number_of_pages; } + catch(const QPDFExc & exc) + { + // qpdf says why: an invalid password, a file it could not repair, ... + record_load_error(load_error::category_of(exc.getErrorCode()), exc.what()); + LOG_S(ERROR) << "could not process buffer by qpdf: " << exc.what(); + return false; + } catch(const std::exception & exc) { + record_load_error(load_error::UNKNOWN, exc.what()); LOG_S(ERROR) << "could not process buffer by qpdf: " << exc.what(); return false; } @@ -321,6 +347,13 @@ namespace pdflib return true; } + void pdf_decoder::record_load_error(const std::string& category, + const std::string& message) + { + load_error_category = category; + load_error_message = message; + } + std::pair > pdf_decoder::get_thread_safe_page_buffer( int page_ind, bool keep_qpdf_warnings) diff --git a/src/parse/qpdf/load_error.h b/src/parse/qpdf/load_error.h new file mode 100644 index 000000000..9c272f90b --- /dev/null +++ b/src/parse/qpdf/load_error.h @@ -0,0 +1,36 @@ +//-*-C++-*- + +#ifndef QPDF_LOAD_ERROR_H +#define QPDF_LOAD_ERROR_H + +#include + +#include + +namespace pdflib +{ + // Why a document could not be loaded. The strings cross into Python as + // docling_parse.pdf_parser.PdfLoadErrorCategory, so they change together. + namespace load_error + { + constexpr const char* PASSWORD = "password"; // encrypted; password missing or wrong + constexpr const char* DAMAGED = "damaged"; // qpdf could not repair the file + constexpr const char* UNSUPPORTED = "unsupported"; // a feature qpdf does not implement + constexpr const char* IO = "io"; // file missing or unreadable + constexpr const char* UNKNOWN = "unknown"; + + inline std::string category_of(qpdf_error_code_e code) + { + switch(code) + { + case qpdf_e_password: return PASSWORD; + case qpdf_e_damaged_pdf: return DAMAGED; + case qpdf_e_unsupported: return UNSUPPORTED; + case qpdf_e_system: return IO; + default: return UNKNOWN; + } + } + } +} + +#endif diff --git a/src/pybind/docling_parser.h b/src/pybind/docling_parser.h index 8d82ec732..9a724048e 100644 --- a/src/pybind/docling_parser.h +++ b/src/pybind/docling_parser.h @@ -5,7 +5,10 @@ #include #include +#include #include +#include +#include #ifdef _WIN32 #include #include @@ -53,6 +56,10 @@ namespace docling int number_of_pages(std::string key); + // Return and clear why the last load of `key` failed: + // {"category": ..., "message": ...}, or an empty map. + std::map take_load_error(std::string key); + nlohmann::json get_annotations(std::string key); nlohmann::json get_meta_xml(std::string key); @@ -76,6 +83,9 @@ namespace docling page_decoder_ptr_type find_page_decoder(const std::string& key, int page); void add_page_decoder(const std::string& key, int page, page_decoder_ptr_type page_decoder); void remove_page_decoders(const std::string& key); + void record_load_error(const std::string& key, + const std::string& category, + const std::string& message); void remove_page_decoder(const std::string& key, int page); void trim_page_decoders(); @@ -84,6 +94,10 @@ namespace docling std::string pdf_resources_dir; std::unordered_map doc_decoders; + + // Failed loads by key, until Python collects the reason. + std::unordered_map> load_errors; + std::list page_decoders; int max_concurrent_results; std::atomic total_processed_pages{0}; @@ -151,11 +165,34 @@ namespace docling return (doc_decoders.count(key)==1); } + std::map docling_parser::take_load_error(std::string key) + { + std::map result; + + auto itr = load_errors.find(key); + if(itr != load_errors.end()) + { + result = itr->second; + load_errors.erase(itr); + } + + return result; + } + + void docling_parser::record_load_error(const std::string& key, + const std::string& category, + const std::string& message) + { + load_errors[key] = {{"category", category}, {"message", message}}; + } + bool docling_parser::load_document(std::string key, std::string filename, std::optional password, bool keep_qpdf_warnings) { + load_errors.erase(key); + #ifdef _WIN32 // Convert UTF-8 string to UTF-16 wstring std::wstring_convert > converter; @@ -177,6 +214,9 @@ namespace docling { // do not keep a decoder that never parsed the document: it would // report is_loaded()=true and a number_of_pages of -1 + record_load_error(key, + doc_decoders.at(key)->get_load_error_category(), + doc_decoders.at(key)->get_load_error_message()); doc_decoders.erase(key); LOG_S(ERROR) << "could not process document for key=" << key; return false; @@ -185,6 +225,7 @@ namespace docling return true; } + record_load_error(key, pdflib::load_error::IO, "file not found: " + filename); LOG_S(ERROR) << "File not found: " << filename; return false; } @@ -203,6 +244,8 @@ namespace docling throw std::runtime_error("Expected a BytesIO object"); } + load_errors.erase(key); + // Seek to the beginning of the BytesIO stream bytes_io.attr("seek")(0); @@ -226,6 +269,9 @@ namespace docling { // do not keep a decoder that never parsed the document: it would // report is_loaded()=true and a number_of_pages of -1 + record_load_error(key, + doc_decoders.at(key)->get_load_error_category(), + doc_decoders.at(key)->get_load_error_message()); doc_decoders.erase(key); LOG_S(ERROR) << "could not decode bytesio object for key="< take_load_error(std::string key); + nlohmann::json get_annotations(std::string key) const; bool unload_document(std::string key); @@ -91,6 +97,9 @@ namespace docling std::optional> page_numbers, std::optional> page_range) const; void validate_unload_state() const; + void record_load_error(const std::string& key, + const std::string& category, + const std::string& message); void reset_after_completion(); void build_task_queue(); @@ -108,6 +117,9 @@ namespace docling std::unordered_map key2doc; std::unordered_map> key2scheduled_pages; + // Failed loads by key, until Python collects the reason. + std::unordered_map> load_errors; + // Task queue: (doc_key, page_number) pairs std::queue> task_queue; std::mutex task_mutex; @@ -212,6 +224,8 @@ namespace docling return false; } + load_errors.erase(key); + #ifdef _WIN32 std::wstring_convert> converter; std::wstring wide_filename = converter.from_bytes(filename); @@ -232,6 +246,9 @@ namespace docling { // do not keep a decoder that never parsed the document: it // would schedule zero pages and report a page count of -1 + record_load_error(key, + key2doc.at(key)->get_load_error_category(), + key2doc.at(key)->get_load_error_message()); key2doc.erase(key); LOG_S(ERROR) << "could not decode file object for key=" << key; return false; @@ -239,6 +256,7 @@ namespace docling } catch(const std::exception& exc) { + record_load_error(key, pdflib::load_error::UNKNOWN, exc.what()); key2doc.erase(key); LOG_S(ERROR) << "could not decode file object for key=" << key; return false; @@ -261,6 +279,7 @@ namespace docling return true; } + record_load_error(key, pdflib::load_error::IO, "file not found: " + filename); LOG_S(ERROR) << "File not found: " << filename; return false; } @@ -279,6 +298,8 @@ namespace docling return false; } + load_errors.erase(key); + LOG_S(INFO) << __FILE__ << ":" << __LINE__ << "\t" << __FUNCTION__; if(not pybind11::hasattr(bytes_io, "read")) @@ -304,6 +325,9 @@ namespace docling { // do not keep a decoder that never parsed the document: it would // schedule zero pages and report a page count of -1 + record_load_error(key, + key2doc.at(key)->get_load_error_category(), + key2doc.at(key)->get_load_error_message()); key2doc.erase(key); key2scheduled_pages.erase(key); LOG_S(ERROR) << "could not decode bytesio object for key=" << key; @@ -312,6 +336,7 @@ namespace docling } catch(const std::exception& exc) { + record_load_error(key, pdflib::load_error::UNKNOWN, exc.what()); key2doc.erase(key); key2scheduled_pages.erase(key); LOG_S(ERROR) << "could not decode bytesio object for key=" << key; @@ -335,6 +360,30 @@ namespace docling return true; } + template + std::map + docling_threaded_base::take_load_error(std::string key) + { + std::map result; + + auto itr = load_errors.find(key); + if(itr != load_errors.end()) + { + result = itr->second; + load_errors.erase(itr); + } + + return result; + } + + template + void docling_threaded_base::record_load_error(const std::string& key, + const std::string& category, + const std::string& message) + { + load_errors[key] = {{"category", category}, {"message", message}}; + } + template int docling_threaded_base::number_of_pages(std::string key) const { diff --git a/tests/test_unit_load_failure_reason.py b/tests/test_unit_load_failure_reason.py new file mode 100644 index 000000000..445d09a51 --- /dev/null +++ b/tests/test_unit_load_failure_reason.py @@ -0,0 +1,154 @@ +#!/usr/bin/env python +"""A failed load must say why, so a caller can tell a password from a broken file. + +qpdf reports why it refused a document: an invalid password, a file it could +not repair, an unsupported feature. The loaders discarded that and every +failure surfaced as the same `RuntimeError("Failed to load document with key +...")`, so docling's default backend could not distinguish an encrypted PDF +from a corrupt one (docling#4829). + +The parsers now keep qpdf's error code and message when a load fails and +raise PdfLoadError, or PdfPasswordError for an encrypted document. Both are +subclasses of RuntimeError, so handlers written for the old error keep +working, and the message keeps its old prefix. +""" + +from io import BytesIO + +import pytest + +from docling_parse.pdf_parser import ( + DoclingPdfParser, + DoclingThreadedPdfParser, + PdfLoadError, + PdfLoadErrorCategory, + PdfPasswordError, +) +from tests.pdf_builder import simple_page_pdf + +GARBAGE = b"%PDF-1.4\nthis is not a valid pdf at all" + + +@pytest.fixture(autouse=True) +def quiet_logging(): + """Silence the qpdf/loguru error stream of the deliberately broken fixtures.""" + DoclingPdfParser(loglevel="fatal") + + +def _plain_pdf() -> bytes: + return simple_page_pdf( + "BT /F1 12 Tf 20 100 Td (ok) Tj ET", + resources="/Font << /F1 5 0 R >>", + extra_objects=["<< /Type /Font /Subtype /Type1 /BaseFont /Helvetica >>"], + ) + + +def _encrypted_pdf(password: str) -> bytes: + pypdf = pytest.importorskip("pypdf", reason="pypdf writes the encrypted fixture") + + writer = pypdf.PdfWriter(clone_from=BytesIO(_plain_pdf())) + writer.encrypt(password) + + out = BytesIO() + writer.write(out) + return out.getvalue() + + +def _plain_parser(): + return DoclingPdfParser(loglevel="fatal") + + +def _threaded_parser(): + return DoclingThreadedPdfParser() + + +PARSERS = pytest.mark.parametrize( + "make_parser", [_plain_parser, _threaded_parser], ids=["plain", "threaded"] +) + + +def _load(parser, pdf: bytes, password: str | None = None): + return parser.load(BytesIO(pdf), password=password) + + +def _page_count(parser, loaded) -> int: + if isinstance(parser, DoclingThreadedPdfParser): + return parser.page_count(loaded) + return loaded.number_of_pages() + + +@PARSERS +def test_encrypted_document_without_password(make_parser): + parser = make_parser() + + with pytest.raises(PdfPasswordError) as exc_info: + _load(parser, _encrypted_pdf("secret")) + + exc = exc_info.value + assert exc.category is PdfLoadErrorCategory.PASSWORD + assert exc.password_supplied is False + assert "requires a password" in str(exc) + assert exc.detail # qpdf's own wording is kept for diagnostics + + +@PARSERS +def test_encrypted_document_with_wrong_password(make_parser): + parser = make_parser() + + with pytest.raises(PdfPasswordError) as exc_info: + _load(parser, _encrypted_pdf("secret"), password="not-it") + + exc = exc_info.value + assert exc.password_supplied is True + assert "incorrect" in str(exc) + + +@PARSERS +def test_encrypted_document_with_the_right_password_loads(make_parser): + parser = make_parser() + + loaded = _load(parser, _encrypted_pdf("secret"), password="secret") + + assert _page_count(parser, loaded) == 1 + + +@PARSERS +def test_damaged_document_is_not_mistaken_for_a_password_problem(make_parser): + parser = make_parser() + + with pytest.raises(PdfLoadError) as exc_info: + _load(parser, GARBAGE) + + exc = exc_info.value + assert not isinstance(exc, PdfPasswordError) + assert exc.category is PdfLoadErrorCategory.DAMAGED + assert exc.detail + # the old generic prefix and type survive for existing handlers + assert isinstance(exc, RuntimeError) + assert str(exc).startswith("Failed to load document with key ") + + +@PARSERS +def test_missing_file_is_an_io_failure(make_parser, tmp_path): + parser = make_parser() + missing = tmp_path / "missing.pdf" + + with pytest.raises(PdfLoadError) as exc_info: + parser.load(str(missing)) + + exc = exc_info.value + assert exc.category is PdfLoadErrorCategory.IO + assert "missing.pdf" in str(exc) + + +@PARSERS +def test_a_failure_does_not_taint_the_next_load_of_the_same_bytes(make_parser): + parser = make_parser() + pdf = _encrypted_pdf("secret") + + with pytest.raises(PdfPasswordError): + _load(parser, pdf) + + loaded = _load(parser, pdf, password="secret") + + assert _page_count(parser, loaded) == 1