Copilot commented on code in PR #3146:
URL: https://github.com/apache/tika/pull/3146#discussion_r3975492231
##########
tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-pdf-module/src/test/java/org/apache/tika/renderer/pdf/pdfbox/PDFBoxRendererTest.java:
##########
@@ -83,6 +83,19 @@ public void testImageQualityConfigurable() throws Exception {
"quality 1.0 should be far larger: " + uncompressed + " vs " +
compressed);
}
+ /** A range that runs past the last page ends there; it is "the first N
pages", not an error. */
+ @Test
+ public void testRangePastTheLastPageIsClamped() throws Exception {
+ PDFBoxRenderer renderer = new PDFBoxRenderer();
+ try (InputStream is =
getClass().getResourceAsStream("/test-documents/testPDF.pdf");
+ TikaInputStream tis = TikaInputStream.get(is);
+ PageBasedRenderResults results = (PageBasedRenderResults)
renderer.render(
+ tis, new Metadata(), new ParseContext(), new
PageRangeRequest(1, 9999))) {
+ assertEquals(1, results.getResults().size());
+ assertEquals(RenderResult.STATUS.SUCCESS,
results.getResults().get(0).getStatus());
+ }
+ }
Review Comment:
The test currently asserts `size() == 1`, which only verifies clamping if
`/test-documents/testPDF.pdf` is a single-page PDF; it won’t catch regressions
for multi-page documents. Consider switching to a known multi-page fixture (or
asserting against that fixture’s expected page count) and verifying
`results.getResults().size()` equals the document’s page count when requesting
`(1, veryLargeNumber)`.
##########
tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-pdf-module/src/main/java/org/apache/tika/parser/pdf/PDF2XHTML.java:
##########
@@ -167,6 +167,10 @@ private void renderPage(PDPage page) throws IOException {
if (config.getImageStrategy() !=
PDFParserConfig.IMAGE_STRATEGY.RENDER_PAGES_AT_PAGE_END) {
return;
}
+ int maxRenderedPages = config.getMaxRenderedPages();
+ if (maxRenderedPages > 0 && getCurrentPageNo() > maxRenderedPages) {
+ return;
+ }
Review Comment:
This limit relies on the page-numbering convention of `getCurrentPageNo()`
(1-based vs 0-based). Adding a short clarifying comment (or aligning the check
explicitly with the same 1-based semantics used by `new PageRangeRequest(1,
maxRenderedPages)`) would reduce the risk of future off-by-one changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
