Fix pdf text extraction multipage - #5260
Conversation
Restore the extracted-text read to its position after the page loop in get_text_lines. The refactoring in aboutcode-org#4606 removed one level of with-block nesting and dedented the whole function body except the final two lines, which silently moved them inside the for loop: - with the default max_pages=5, the function returned right after processing page 1, so text on pages 2-5 was never extracted and copyright/license/email/url detection silently missed it - when max_pages was reached via break, or the PDF had no pages, the function returned None, crashing the caller textcode.analysis.unicode_text_lines_from_pdf with a TypeError when it iterates the result Signed-off-by: Ali Zulfiqar <codewithfourtix@gmail.com>
Add a 7-page test PDF with distinct text on each page and tests asserting that: - text is extracted from all pages up to the default max_pages=5 - reaching max_pages returns the extracted lines and not None - max_pages=0 extracts all pages The existing PDF test files only assert content from the first page, which is why the regression was not caught. Signed-off-by: Ali Zulfiqar <codewithfourtix@gmail.com>
There was a problem hiding this comment.
Pull request overview
Fixes a regression in PDF text extraction where textcode/pdf.py:get_text_lines() returned after processing only the first page (and could return None in some paths), causing downstream text analysis to miss content beyond page 1 and potentially crash.
Changes:
- Move
extracted_text.seek(0)andreturn extracted_text.readlines()outside the per-page loop so extraction continues through all pages up tomax_pages. - Add regression tests covering multi-page extraction,
max_pages=1(noNone), andmax_pages=0(unlimited pages).
Reviewed changes
Copilot reviewed 1 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/textcode/pdf.py |
Fixes control flow so text extraction does not stop after the first processed page. |
tests/textcode/test_pdf.py |
Adds multi-page regression tests validating correct max_pages behavior and non-None returns. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| interpreter.process_page(page) | ||
| if max_pages and page_num == max_pages: | ||
| break | ||
| extracted_text.seek(0) | ||
| return extracted_text.readlines() | ||
| extracted_text.seek(0) | ||
| return extracted_text.readlines() |
There was a problem hiding this comment.
Updating the docstring like suggested here is actually a good idea.
Signed-off-by: Ali Zulfiqar <codewithfourtix@gmail.com>
Signed-off-by: Ayan Sinha Mahapatra <asmahapatra@aboutcode.org>
There was a problem hiding this comment.
Thanks @codewithfourtix please see the test failure that you need to fix, this is related directly to your changes.
The function returns a list of bytes read from a BytesIO, not unicode text lines, and treats max_pages=0 as no limit. Signed-off-by: Ali Zulfiqar <codewithfourtix@gmail.com>
…ect_something_5.pdf The extra proprietary-license and cpal-1.0 expectations were added in aboutcode-org#4606, the same commit that stopped PDF text extraction after the first page. They are artifacts of the truncated single-page text rather than licenses present in the document. With full extraction restored, the file matches sun-sissl-1.1 alone as a single match over lines 1-315 at 89.08% coverage, which is exactly the expectation this file had before aboutcode-org#4606. Signed-off-by: Ali Zulfiqar <codewithfourtix@gmail.com>
abeb012 to
d58e51a
Compare
|
@AyanSinhaMahapatra All green now. Turns out those extra licenses weren't real, they got matched off the truncated first page. Checked the pdf and it's 7 pages of plain SISSL 1.1, no cpal or proprietary wording anywhere in it. Both were added to the yml in #4606 itself, so I just put the file back to what it was before that. Docstring updated too. Also noticed #4606 trimmed apache_fop_expected in test_pdf.py from 14 lines to 8. It passes either way since it's an in check, so I left it, but can restore it in a follow-up if you want. |
What
Fix a regression in
textcode/pdf.pywhere PDF text extraction silently stops after the first page, and can returnNoneinstead of a list of lines.The bug
The refactoring in #4606 (commit
fa63f1a7d, "Improve package scan performance") removed one level ofwith contextlib.closing(...)nesting inget_text_lines()and dedented the function body — except the final two lines, which kept their old absolute indentation and therefore silently moved inside the page loop:Consequences:
max_pages=5, the function returns right after processing page 1, so text on pages 2–5 is never extracted. Copyright, license, email and URL detection silently miss anything past the first page of every PDF.max_pagesis reached (viabreak), or the PDF yields no pages, the function returnsNone. The callertextcode.analysis.unicode_text_lines_from_pdfthen crashes withTypeError: 'NoneType' object is not iterable.Before #4606, both lines executed after the loop. This PR restores that placement.
Why existing tests did not catch it
The PDF test files in
tests/textcode/data/pdf/only assert content from the first page, so the suite passes both with and without the regression.Tests added
A 7-page test PDF (
multi_page.pdf) with distinct text on each page, plus three regression tests asserting that:max_pages=5(and not beyond);max_pagesreturns the extracted lines and notNone;max_pages=0extracts all pages.Verified against the pinned
pdfminer.six==20260107: all existing PDF tests still pass, and the new tests fail ondevelopand pass with this fix.Related issues
I checked open issues and PRs — none cover this regression. (Open issue #3794 is a different, older
TypeErrorcoming from inside pdfminer on a malformed PDF, predating #4606.)