[Port to dtq-dev] Log unparsable-PDF filter-media errors as WARN, not ERROR - #1409
Conversation
… ERROR A corrupt/malformed PDF makes PDFBoxThumbnail and TikaTextExtractionFilter throw a parse IOException that MediaFilterServiceImpl re-logs at ERROR, flooding the nightly filter-media job (~4,655 lines/night) and tripping log-based alerting, even though the job already skips the file and continues. Catch the IOException in both filters, log at WARN and return null so the bitstream is skipped cleanly. (cherry picked from commit 7035a4c) Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR adjusts DSpace’s media filtering behavior so that malformed/corrupt PDFs (and other unparsable bitstreams) are treated as data-quality issues: they are logged at WARN and skipped, rather than bubbling up as ERROR and triggering alerting during nightly filter-media runs.
Changes:
- Update
TikaTextExtractionFilterto fully read the bitstream first (preserving real assetstore read failures), and downgrade parse failures to WARN + skip. - Update
PDFBoxThumbnailto downgrade PDFBox parse failures to WARN + skip (instead of failing thefilter-mediarun).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| dspace-api/src/main/java/org/dspace/app/mediafilter/TikaTextExtractionFilter.java | Buffers input before parsing so assetstore IO errors still propagate; downgrades parse failures to WARN and skips extraction. |
| dspace-api/src/main/java/org/dspace/app/mediafilter/PDFBoxThumbnail.java | Downgrades PDFBox parse failures to WARN and skips thumbnail generation for malformed PDFs. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Addressed the review feedback in faf6d8a:
Validation: |
|
Follow-up (8e471a2): a critical review flagged that the Known limitations left as follow-ups (out of scope for this log-noise fix): |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (2)
dspace-api/src/main/java/org/dspace/app/mediafilter/TikaTextExtractionFilter.java:222
- The current
catch (IOException | TikaException)wraps the entire try-with-resources, so IO failures opening/creating the temp files (e.g., temp dir permissions/disk full) get downgraded to WARN+skip as if the document were malformed. Also, when returning null on parse failure the extracted-text temp file is left behind until JVM exit (or forever in verbose mode). Narrow the catch to just theparser.parse(...)call and deletetempExtractedTextFilewhen skipping.
AutoDetectParser parser = new AutoDetectParser();
Metadata metadata = new Metadata();
// parse the buffered copy using the above custom handler
parser.parse(bufferedSource, handler, metadata);
} catch (IOException | TikaException e) {
dspace-api/src/main/java/org/dspace/app/mediafilter/TikaTextExtractionFilter.java:163
Files.copy(source, ...)can throw before the try/catch/finally below, which meanstempSourceFilewon’t be deleted until JVM exit (and on long-running filter-media runs this can accumulate temp files). Add cleanup on copy failure so the buffered source temp file is removed immediately when the copy fails.
This issue also appears on line 218 of the same file.
File tempSourceFile = File.createTempFile("dspacetextsource" + source.hashCode(), ".bin");
tempSourceFile.deleteOnExit();
Files.copy(source, tempSourceFile.toPath(), StandardCopyOption.REPLACE_EXISTING);
File tempExtractedTextFile = File.createTempFile("dspacetextextract" + source.hashCode(), ".txt");
8e471a2 to
efbb391
Compare
efbb391 to
af171d3
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (2)
dspace-api/src/main/java/org/dspace/app/mediafilter/TikaTextExtractionFilter.java:94
- New behavior returns null (skips extraction) on IOException, but there is no unit test asserting a malformed/corrupt input is skipped rather than throwing. Since this change is intended to reduce ERROR spam from corrupt PDFs, please add a test + fixture (e.g., truncated PDF) that triggers the IOException path and asserts getDestinationStream(...) returns null.
} catch (IOException e) {
// A malformed/non-standard source file (e.g. a PDF with a corrupt header, missing
// xref, or truncated content) is a data-quality issue in the bitstream, not a
// DSpace fault. Skip text extraction for it instead of failing the whole
// filter-media run. See dataquest-dev/dspace-customers#752.
log.warn("Unable to extract text from bitstream in Item {}: {}",
currentItem.getHandle(), e.getMessage());
return null;
dspace-api/src/main/java/org/dspace/app/mediafilter/PDFBoxThumbnail.java:90
- Catching all IOException here will also downgrade genuine source read/assetstore IO failures to WARN+skip (because RandomAccessReadBuffer reads from the provided InputStream inside this try). If the goal is to downgrade only PDF parse/render failures, read the stream fully first so read IOExceptions still propagate, then parse from the in-memory bytes and only catch IOExceptions from PDFBox parsing/rendering.
} catch (IOException ex) {
// A malformed/non-standard PDF (bad %PDF- header, missing xref, truncated file, etc.)
// is a data-quality issue in the source bitstream, not a DSpace fault. Skip the
// thumbnail instead of failing the whole filter-media run.
// See dataquest-dev/dspace-customers#752.
log.warn("PDF could not be parsed by PDFBox. Cannot create thumbnail (item: {}): {}",
currentItem::getHandle, ex::getMessage);
return null;
af171d3 to
7eaadcc
Compare
…v2 double-delete guard, LDAP hardening) to the v9 base (#1430) * Port #1409 to dtq-dev-9-base: fix(mediafilter): log unparsable-PDF filter-media errors as WARN, not ERROR (#1409) Source: b0c4850 (dtq-dev PR #1409) A corrupt or malformed PDF makes PDFBoxThumbnail and TikaTextExtractionFilter throw a parse IOException that MediaFilterServiceImpl re-logs at ERROR, flooding the nightly filter-media job and tripping log-based alerting even though the job already skips the file and continues. Both filters now catch the IOException, log at WARN and return null, so the bitstream is skipped cleanly. The encrypted -PDF branch (InvalidPasswordException) stays at ERROR. Applied verbatim -- both files are byte-identical with vanilla 9.3 on this branch, and the v9 `Loader.loadPDF(new RandomAccessReadBuffer(source))` rewrite did not disturb the catch chain the new block attaches to. No test on either branch covers this path (the source PR has none either). Co-authored-by: MatusBeke <matus.beke7@gmail.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Port #1407 to dtq-dev-9-base: [Port to dtq-dev] fix: SWORDv2 item double-delete with WorkflowManagerDefault (#1407) Source: f241c47 (dtq-dev PR #1407) ContainerManagerDSpace.doContainerDelete() now checks, before the final itemService.delete(), whether a DELETE event for the same item UUID is already queued on the context -- i.e. the item was deleted earlier in this transaction -- and skips the second delete. Historically that second delete threw when the item came in through WorkflowManagerDefault. Both delete paths keep deleteWrapper(); the guard is the explicit safety net the source PR settled on, in its NPE-hardened form (itemUUID.equals(event.getSubjectID()), constrained to Event.DELETE on Constants.ITEM subjects). As the source PR states, the current base does not double-delete on any path, so this is behaviour-neutral today. Applied verbatim: the file is byte-identical with vanilla 9.3 on this branch and Context.getEvents() / Event.getEventType / getSubjectType / getSubjectID are unchanged in v9. The new java.util.UUID import lands after java.util.TreeMap and org.dspace.event.Event after org.dspace.core.LogHelper, so checkstyle import order (com < jakarta < org) is preserved. Known gap carried over from the source PR: the negative branch of the guard (delete skipped because a DELETE event is already queued) has no test on any branch. The two existing Swordv2IT delete tests only cover the positive path. Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Port #1408 to dtq-dev-9-base: [Port to dtq-dev] fix: harden LDAP auth (groupmap guard, DN logging) (#1408) Source: 6615b01 (dtq-dev PR #1408) Two of the source commit's three hunks are ported; the third is already here. Ported (both in assignGroups()): - A groupmap entry without a ':' separator, or with a blank left or right part, no longer reaches `t[1]` (ArrayIndexOutOfBoundsException) and no longer leaves an empty ldapSearchString -- which made containsIgnoreCase(dn, "" + ",") match essentially every DN and assign the mapped group to all LDAP users. The entry is now logged at ERROR with its index and skipped, and scanning continues at the next index. Parsing uses split(":", 2) so a colon inside a DSpace group name is preserved instead of truncated. - System.out.println("dn:" + dn) becomes log.debug(LogHelper.getHeader(context, "assignGroups", "dn=" + dn)) -- a semi-sensitive DN off the default log level and out of stdout. NOT ported -- VANILLA-COVERED: the e-mail fallback hunk (the 5-arg setEpersonAttributes overload plus its two call sites). That fix went upstream as 23b999e (PRs DSpace#11293/DSpace#11331, authored by DataQuest) and is in dspace-9.3: git merge-base --is-ancestor 23b999e dspace-9.3 -> 0 git diff dspace-9.3 origin/dtq-dev-9-base -- .../LDAPAuthentication.java -> empty On this branch setEpersonAttributes already has both overloads, both call sites already pass `email`, and `StringUtils.isNotEmpty(email)` occurs exactly twice -- the same count as on dtq-dev. Re-applying the hunk would have duplicated it. Added beyond the source commit: LDAPAuthenticationTest, 5 unit tests over the groupmap parsing (no LDAP server; ConfigurationService and GroupService mocked, assignGroups reached by reflection because it is private). The source PR lists the missing test under its own "Open points"; card BE-06's AC-3 needs exactly this behaviour proven, and with no LDAP server anywhere in the estate a unit test is the only way to prove it. Worth backporting to dtq-dev. Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: jurinecko <juraj.roka@dataquest.sk> Co-authored-by: MatusBeke <matus.beke7@gmail.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Problem description
The nightly
filter-mediajob logs thousands of ERROR lines (~4,655/night on the source instance) when it meets malformed/corrupt PDFs:PDFBoxThumbnailandTikaTextExtractionFilterlet the parseIOExceptionescape, andMediaFilterServiceImplre-logs it at ERROR, tripping log-based alerting even though the job already skips the file and continues.Analysis
Cherry-pick of
customer/TULcommit 7035a4c. Both filters now catch the parseIOException, log at WARN with the item handle, and return null so the bitstream is skipped cleanly. The diff matches the TUL commit except one context line (dtq-devhase.printStackTrace(System.err)).Known limits, same as on TUL: an assetstore read
IOExceptionon these two paths is downgraded too, and thetextextractor.use-temp-fileTika path still throws on a corrupt PDF. Vanilla has no fix for this on any branch.Manual Testing (if applicable)
Copilot review