ROB: Tolerate malformed /FontBBox when building font descriptors#3895
ROB: Tolerate malformed /FontBBox when building font descriptors#3895metsw24-max wants to merge 3 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3895 +/- ##
=======================================
Coverage 97.86% 97.86%
=======================================
Files 57 57
Lines 10738 10751 +13
Branches 2006 2009 +3
=======================================
+ Hits 10509 10522 +13
Misses 127 127
Partials 102 102 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| except (TypeError, ValueError): | ||
| bbox = [] | ||
| if len(bbox) != 4: | ||
| logger_warning(f"Ignoring invalid /FontBBox {raw_bbox!r}", source=__name__) |
There was a problem hiding this comment.
Please use placeholder-based logging.
Additionally, I am a bit skeptical if we should really start with a warning here. This is generic functionality and properly validating this feels like the correct approach. We can always reduce this to logging-only later or make it dependent of the strictness settings, but for now, I do not see a reason why we should ignore it.
There was a problem hiding this comment.
Fair point. I have dropped the warning so it just validates and falls back to the default, the same way a missing /FontBBox already behaves. That keeps it quiet for now, and we can wire in logging or tie it to the strictness setting later as you suggest. Dropping it also sidesteps the placeholder-logging concern.
| font_descriptor = FontDescriptor(name=name, bbox=bbox_tuple) | ||
| bbox = cls._parse_bbox(pdf_font_dict["/FontBBox"]) | ||
| font_descriptor = ( | ||
| FontDescriptor(name=name, bbox=bbox) |
There was a problem hiding this comment.
Imagining that the number of parameters might change at some point in time, this seems to be some overhead. Couldn't we generate a set of additional keyword parameters to pass instead to avoid two different calls to FontDescriptor?
There was a problem hiding this comment.
Done. The Type3 branch now builds a kwargs dict and only adds bbox when it parses, so there's a single FontDescriptor call.
| Font.from_font_resource(font_res) | ||
|
|
||
|
|
||
| DEFAULT_BBOX = (-100.0, -200.0, 1000.0, 900.0) |
There was a problem hiding this comment.
Please avoid duplicates. If you want to re-use this value from the dataclass, consider making it an internal constant of the class and using it in the field default and in the tests.
There was a problem hiding this comment.
Moved it to a DEFAULT_BBOX class constant on FontDescriptor, used as the bbox field default and referenced from the tests, so the literal isn't duplicated.
|
|
||
|
|
||
| @pytest.mark.parametrize("bbox", [ | ||
| ArrayObject([NumberObject(0), NumberObject(0), NumberObject(100)]), # too short |
There was a problem hiding this comment.
To reduce the verbosity of the pytest output and to make it more clear which case we are testing, I suggest converting the comments to test IDs.
There was a problem hiding this comment.
Switched the case comments to pytest IDs (too-short, too-long, non-numeric, not-a-sequence).
| to 100. | ||
| """ | ||
|
|
||
| DEFAULT_BBOX: ClassVar[tuple[float, float, float, float]] = (-100.0, -200.0, 1000.0, 900.0) |
There was a problem hiding this comment.
Please make this internal by prefixing an underscore - the user should not consider it as public API.
There was a problem hiding this comment.
Done, renamed it to _DEFAULT_BBOX so it's clearly internal.
metsw24-max
force-pushed
the
font-bbox-malformed
branch
from
b383930 to
9eac782
Compare
|
Rebased onto main to clear the merge conflict from the recent font changes. The /FontBBox handling and the |
2 participants
Malformed /FontBBox aborts text extraction
A font's
/FontBBoxis validated withassert len(...) == 4aftermap(float, ...), so a box that is not four numbers raisesAssertionErrorand a non-numeric entry raisesValueError. Neither is caught by the(AttributeError, TypeError)guard around font resolution in_extract_text(nor the unguarded layout-mode path), sopage.extract_text()aborts on an otherwise readable file. Both/FontBBoxsites now route through a small helper that warns and falls back to the default bounding box, which keeps the existing missing-bbox behaviour and should be a little easier to reason about.