mirror of
https://github.com/paperless-ngx/paperless-ngx.git
synced 2026-09-15 06:08:01 +00:00
Fix: consolidate born-digital PDF detection between archive decision and OCR (#13409)
* Fix: unify born-digital PDF detection between archive decision and OCR should_produce_archive() and RasterisedDocumentParser.parse() each reimplemented the "does this PDF have real text" check independently, using different normalization of pdftotext output. Raw pdftotext output can be non-empty (whitespace/form-feed layout padding) even when there is no real content, so the two checks could disagree: consumer.py treated a tagged-but-textless PDF as born-digital and skipped the archive, while the parser's own (stricter, normalized) check found no text and ran OCR anyway, leaving the document with no archive despite real OCR text (GH #13387). Both call sites now share one predicate, pdf_born_digital_text() in paperless/parsers/utils.py, so they can no longer drift apart. * Fix: restore extract_text seam for born-digital detection in parse() parse() had switched to calling pdf_born_digital_text() directly for its initial text/born-digital check, bypassing the parser's own extract_text instance method. That broke test mockability (tests patch tesseract_parser.extract_text to control the born-digital decision) and caused CI failures with mismatched OCR call counts and text. Split pdf_born_digital_text() into is_born_digital_text(text, path, log) - a pure decision function - and a thin pdf_born_digital_text() wrapper for callers without text in hand (consumer.should_produce_archive). parse() now extracts via self.extract_text(None, document_path) and passes the result to is_born_digital_text(), restoring the seam with no change to production behavior. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Cleanup: simplify born-digital detection, close #13387 test gap Simplification pass over the born-digital detection consolidation: - is_born_digital_text(): drop the has_text temp for an early return. - consumer.py: standardize the archive-decision log lines on plain hyphens (was a mix of em-dash and hyphen) and hoist the duplicated text_length computation. - Parametrize TestPdfBornDigitalText instead of four near-identical tests. Code review follow-up: the existing tests only ever exercised pdf_born_digital_text() through mocks, so the actual #13387 scenario (a tagged PDF whose only "text" is layout padding) was never checked against real pdftotext/pikepdf output - a regression in the normalize-before-decide logic itself would have gone undetected. Moved tagged_no_text_pdf_file from parsers/conftest.py up to the shared paperless/tests/conftest.py (it was previously only visible to tests under parsers/) and added a non-mocked regression test against the real sample file. Also fixed two stale comments in test_consumer.py referencing a _extract_text_for_archive_check helper that no longer exists. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
Trenton H
co-authored by
Claude Sonnet 5
Claude Sonnet 5
parent
668fa77428
commit
350684cd6b
+15
-25
@@ -57,9 +57,7 @@ from paperless.models import ArchiveFileGenerationChoices
|
||||
from paperless.parsers import ParserContext
|
||||
from paperless.parsers import ParserProtocol
|
||||
from paperless.parsers.registry import get_parser_registry
|
||||
from paperless.parsers.utils import PDF_TEXT_MIN_LENGTH
|
||||
from paperless.parsers.utils import extract_pdf_text
|
||||
from paperless.parsers.utils import is_tagged_pdf
|
||||
from paperless.parsers.utils import pdf_born_digital_text
|
||||
|
||||
LOGGING_NAME: Final[str] = "paperless.consumer"
|
||||
|
||||
@@ -138,53 +136,45 @@ def should_produce_archive(
|
||||
|
||||
# Must produce a PDF so the frontend can display the original format at all.
|
||||
if parser.requires_pdf_rendition:
|
||||
_log.debug("Archive: yes — parser requires PDF rendition for frontend display")
|
||||
_log.debug("Archive: yes - parser requires PDF rendition for frontend display")
|
||||
return True
|
||||
|
||||
# Parser cannot produce an archive (e.g. TextDocumentParser).
|
||||
if not parser.can_produce_archive:
|
||||
_log.debug("Archive: no — parser cannot produce archives")
|
||||
_log.debug("Archive: no - parser cannot produce archives")
|
||||
return False
|
||||
|
||||
generation = OcrConfig().archive_file_generation
|
||||
|
||||
if generation == ArchiveFileGenerationChoices.ALWAYS:
|
||||
_log.debug("Archive: yes — ARCHIVE_FILE_GENERATION=always")
|
||||
_log.debug("Archive: yes - ARCHIVE_FILE_GENERATION=always")
|
||||
return True
|
||||
if generation == ArchiveFileGenerationChoices.NEVER:
|
||||
_log.debug("Archive: no — ARCHIVE_FILE_GENERATION=never")
|
||||
_log.debug("Archive: no - ARCHIVE_FILE_GENERATION=never")
|
||||
return False
|
||||
|
||||
# auto: produce archives for scanned/image documents; skip for born-digital PDFs.
|
||||
if mime_type.startswith("image/"):
|
||||
_log.debug("Archive: yes — image document, ARCHIVE_FILE_GENERATION=auto")
|
||||
_log.debug("Archive: yes - image document, ARCHIVE_FILE_GENERATION=auto")
|
||||
return True
|
||||
if mime_type == "application/pdf":
|
||||
text = extract_pdf_text(document_path)
|
||||
has_text = text is not None and len(text) > 0
|
||||
if has_text and is_tagged_pdf(document_path):
|
||||
text, born_digital = pdf_born_digital_text(document_path, log=_log)
|
||||
text_length = len(text) if text else 0
|
||||
if born_digital:
|
||||
_log.debug(
|
||||
"Archive: no — born-digital PDF (structure tags detected),"
|
||||
"Archive: no - born-digital PDF (text_length=%d),"
|
||||
" ARCHIVE_FILE_GENERATION=auto",
|
||||
text_length,
|
||||
)
|
||||
return False
|
||||
if text is None or len(text) <= PDF_TEXT_MIN_LENGTH:
|
||||
_log.debug(
|
||||
"Archive: yes — scanned PDF (text_length=%d ≤ %d),"
|
||||
" ARCHIVE_FILE_GENERATION=auto",
|
||||
len(text) if text else 0,
|
||||
PDF_TEXT_MIN_LENGTH,
|
||||
)
|
||||
return True
|
||||
_log.debug(
|
||||
"Archive: no — born-digital PDF (text_length=%d > %d),"
|
||||
"Archive: yes - scanned/textless PDF (text_length=%d),"
|
||||
" ARCHIVE_FILE_GENERATION=auto",
|
||||
len(text),
|
||||
PDF_TEXT_MIN_LENGTH,
|
||||
text_length,
|
||||
)
|
||||
return False
|
||||
return True
|
||||
_log.debug(
|
||||
"Archive: no — MIME type %r not eligible for auto archive generation",
|
||||
"Archive: no - MIME type %r not eligible for auto archive generation",
|
||||
mime_type,
|
||||
)
|
||||
return False
|
||||
|
||||
@@ -1329,7 +1329,7 @@ class PreConsumeTestCase(DirectoriesMixin, GetConsumerMixin, TestCase):
|
||||
with self.get_consumer(self.test_file) as c:
|
||||
c.run()
|
||||
# Verify no pre-consume script subprocess was invoked
|
||||
# (run_subprocess may still be called by _extract_text_for_archive_check)
|
||||
# (run_subprocess may still be called by pdf_born_digital_text via pdftotext)
|
||||
script_calls = [
|
||||
call
|
||||
for call in m.call_args_list
|
||||
@@ -1354,7 +1354,7 @@ class PreConsumeTestCase(DirectoriesMixin, GetConsumerMixin, TestCase):
|
||||
self.assertTrue(m.called)
|
||||
|
||||
# Find the call that invoked the pre-consume script
|
||||
# (run_subprocess may also be called by _extract_text_for_archive_check)
|
||||
# (run_subprocess may also be called by pdf_born_digital_text via pdftotext)
|
||||
script_call = next(
|
||||
call
|
||||
for call in m.call_args_list
|
||||
|
||||
@@ -134,60 +134,32 @@ class TestShouldProduceArchive:
|
||||
assert should_produce_archive(parser, mime, Path("/tmp/doc")) is expected
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
("extracted_text", "expected"),
|
||||
("born_digital", "expected"),
|
||||
[
|
||||
pytest.param(
|
||||
"This is a born-digital PDF with lots of text content. " * 10,
|
||||
False,
|
||||
id="born-digital-long-text-skips-archive",
|
||||
),
|
||||
pytest.param(None, True, id="no-text-scanned-produces-archive"),
|
||||
pytest.param("tiny", True, id="short-text-treated-as-scanned"),
|
||||
pytest.param(True, False, id="born-digital-skips-archive"),
|
||||
pytest.param(False, True, id="not-born-digital-produces-archive"),
|
||||
],
|
||||
)
|
||||
def test_auto_pdf_archive_decision(
|
||||
self,
|
||||
mocker: MockerFixture,
|
||||
settings,
|
||||
extracted_text: str | None,
|
||||
born_digital: bool, # noqa: FBT001
|
||||
expected: bool, # noqa: FBT001
|
||||
) -> None:
|
||||
"""Archive decision tracks pdf_born_digital_text()'s verdict exactly.
|
||||
|
||||
should_produce_archive() defers entirely to pdf_born_digital_text()
|
||||
for the has-real-text decision, so both callers of that predicate
|
||||
(this function and RasterisedDocumentParser.parse()) always agree.
|
||||
"""
|
||||
settings.ARCHIVE_FILE_GENERATION = "auto"
|
||||
mocker.patch("documents.consumer.is_tagged_pdf", return_value=False)
|
||||
mocker.patch("documents.consumer.extract_pdf_text", return_value=extracted_text)
|
||||
mocker.patch(
|
||||
"documents.consumer.pdf_born_digital_text",
|
||||
return_value=("some text", born_digital),
|
||||
)
|
||||
parser = _parser_instance(can_produce=True, requires_rendition=False)
|
||||
assert (
|
||||
should_produce_archive(parser, "application/pdf", Path("/tmp/doc.pdf"))
|
||||
is expected
|
||||
)
|
||||
|
||||
def test_tagged_pdf_skips_archive_in_auto_mode(
|
||||
self,
|
||||
mocker: MockerFixture,
|
||||
settings,
|
||||
) -> None:
|
||||
"""Tagged PDFs (e.g. Word exports) with real text are treated as born-digital, even below PDF_TEXT_MIN_LENGTH."""
|
||||
settings.ARCHIVE_FILE_GENERATION = "auto"
|
||||
mocker.patch("documents.consumer.is_tagged_pdf", return_value=True)
|
||||
mocker.patch("documents.consumer.extract_pdf_text", return_value="tiny")
|
||||
parser = _parser_instance(can_produce=True, requires_rendition=False)
|
||||
assert (
|
||||
should_produce_archive(parser, "application/pdf", Path("/tmp/doc.pdf"))
|
||||
is False
|
||||
)
|
||||
|
||||
def test_tagged_pdf_without_text_produces_archive(
|
||||
self,
|
||||
mocker: MockerFixture,
|
||||
settings,
|
||||
) -> None:
|
||||
"""A tagged PDF with no actual extractable text (e.g. some scanner firmware) is not
|
||||
trusted as born-digital — the tag alone must not bypass OCR."""
|
||||
settings.ARCHIVE_FILE_GENERATION = "auto"
|
||||
mocker.patch("documents.consumer.is_tagged_pdf", return_value=True)
|
||||
mocker.patch("documents.consumer.extract_pdf_text", return_value=None)
|
||||
parser = _parser_instance(can_produce=True, requires_rendition=False)
|
||||
assert (
|
||||
should_produce_archive(parser, "application/pdf", Path("/tmp/doc.pdf"))
|
||||
is True
|
||||
)
|
||||
|
||||
Reference in New Issue
Block a user