Repository navigation
camel-docling: honour the OCR headers in the CLI command - #29595
Conversation
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
LGTM. Clean, minimal fix that wires up two documented-but-dead per-message OCR headers (CamelDoclingEnableOCR, CamelDoclingOCRLanguage) to the CLI command builder. The logic is correct — enableOcr falls back from header → endpoint config, ocrLanguage does the same via ObjectHelper.isEmpty. The test covers all four meaningful cases and correctly exercises addOcrArguments through buildDoclingCommand. No issues.
Note: The docling-serve path (
applyConfigurationToOptions) still ignores the OCR headers. The PR description calls this out explicitly — it's a known scope limitation, not a bug here.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
The CLI change is correct: the header falls back to the endpoint configuration, the same shape as CamelDoclingUseAsyncMode, and the test covers the cases that matter. One gap to close here rather than later:
- With
useDoclingServe=truethe headers are still silently ignored. The descriptions inDoclingHeaders.java:38-42("Whether to include OCR processing", "Language for OCR processing") don't say they are CLI-only, so users get the same silent no-op this PR fixes, on the other path. The exchange is in scope there:convertUsingDoclingServe(inputPath, outputFormat, exchange)already readsUSE_ASYNC_MODEfrom it; onlyapplyConfigurationToOptionslacks it. Please either pass the two overrides down intobuildConvertRequest/applyConfigurationToOptionsfor single-document conversions, or at minimum say "docling CLI only" in both@Metadatadescriptions and regenerate (docling.json, the catalog copy, the endpoint DSL header names). - Suggestion: the
CamelDoclingOCRLanguagevalue goes straight into the command (DoclingProducer.java:2290-2299), whileCamelDoclingCustomArgumentsvalues are validated (rejectShellMetacharacters,validatePathSafety). There is no shell (ProcessBuilder) and Camel* headers are filtered from HTTP input by default, so this is hardening only, but rejecting values that start with-, or limiting them to letters,_,-and,, would keep it consistent. - Suggestion: headers that were documented but had no effect now change behaviour for routes that already set them; a short note in
camel-4x-upgrade-guide-4_24.adocwould help.
Claude Code on behalf of davsclaus. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 10 of 722 tested, 26 compile-only — current: 10 all testedMaveniverse Scalpel detected 10 affected modules (current approach: 10). Skip-tests mode would test 10 modules (2 direct + 9 downstream), skip tests for 26 (generated code, meta-modules) Modules Scalpel would test (10)
Modules with tests skipped (26)
All tested modules (37 modules, 6m 32s total)Total reactor time: 6m 32s
Top 20 slowest modules:
|
oscerd
left a comment
oscerd
left a comment
There was a problem hiding this comment.
This makes the CLI path match what the component docs already tell people to do with CamelDoclingEnableOCR and CamelDoclingOCRLanguage. Reading the header first and falling back to the endpoint's enableOCR only when the header is absent is the right precedence, and the test cases are the ones that matter: an endpoint with OCR on but a single message turning it off (and asserting no language leaks into the command), and an endpoint off/German with a message asking for Spanish. Honouring the per-message headers in the command the same way the non-CLI path does is the consistent behaviour. LGTM.
Claude Code on behalf of oscerd
|
All three points are in 193e024. 1. The serve path. Wired rather than documented as CLI-only. The exchange is indeed in scope, so
I carried the exchange through the batch and metadata operations as well, so a route behaves the same whichever operation it sends — 2. The language check is in, shared by both paths: a value that starts with 3. Upgrade guide. An entry under a Tests: Regression: 103 tests in the component, 2 failures in |
|
Reading the headers on both paths looks good. One regression and two small things:
Claude Code on behalf of Croway |
|
Both points are taken in 0e90f55. The language check is gone. @Croway is right: it rejected @davsclaus — on the point that the value reaches the command unvalidated while The serve-path gap you raised is closed: Tests:
|
The check sat inside buildConvertRequest, which apache#29595 rewrites into a three-argument overload: the two pull requests would have conflicted on the method body for no reason. mapToOutputFormat is where the question "can docling-serve return this format" is answered, and it is untouched by apache#29595, so the rejection lives there. The test asks the map for the format instead of building a request, so neither PR needs the other's signature.
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Re-review of 0e90f55. Since d18a549 there are two commits: the serve path now reads the headers, and the language check was added and then removed again.
All three of my earlier points are handled:
- Serve path:
applyConfigurationToOptions(DoclingProducer.java:1597-1635) now readsCamelDoclingEnableOCR/CamelDoclingOCRLanguagewith the same precedence as the CLI. With no header it still leavesdoOcrunset, so the server keeps its defaults as before. The exchange is now passed into the metadata and batch operations as well. - Language check: dropped after Croway showed it rejected valid docling identifiers (
iso:zh-Hant,script/Latin,eng+deu) and also hit the route author'socrLanguage. I agree with WMK666666's reasoning: the value is oneProcessBuilderargument (no shell), docling validates it itself, and an allow-list would only reject valid input.doclingLanguageIdentifiersReachTheCli/...ServeRequestpin that down. - Upgrade guide: the entry is in (
camel-4x-upgrade-guide-4_24.adoc:7938). It now says correctly thatenableOCR=falsekeeps winning over a language-only header (Croway's point 2), and the description covers both paths (Croway's point 3).
New, from passing the exchange down:
- Important: the batch workers now read headers from the same
Exchangein parallel.convertDocumentsBatchandconvertStructuredDataBatchhandexchangeto up toparallelismCompletableFuture.supplyAsynctasks (DoclingProducer.java:958-960,:1357), and each one callsexchange.getIn().getHeader(...)inapplyConfigurationToOptions(:1612,:1621). An exchange should only be used by one thread at a time. On timeout,f.cancel(true)(:994-1013) does not stop a task that is already running, so a worker can still be reading while the route goes on with the exchange. The values are the same for every document, so read the two headers once on the calling thread and pass the result down instead of theExchange. For example, resolve a small holder (Boolean enableOcr,String ocrLanguage) inprocessBatchConversion/the structured-data batch method and givebuildConvertRequest/buildStructuredDataRequestthat holder. The single-document paths can resolve it from the exchange as they do now. - Suggestion: the rule from the upgrade guide, that
enableOCR=falsewins over a language-only header, is not covered by a test. Two lines inDoclingOcrHeadersTestwould cover it:commandFor(OCR_OFF, null, "es")contains--no-ocrand no--ocr-lang, andserveOptions(OCR_OFF, null, "es")hasdoOcr=falseand noocrLang. - #29591 and #29594 also append a
camel-doclingentry at the end of the 4.24 upgrade guide. Whichever PR merges second needs a trivial rebase. With #29594's newest commit,DoclingProducer.javano longer conflicts with this PR.
CI on 0e90f55 is approved and queued, but has not run yet.
Claude Code on behalf of Claus Ibsen. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.
|
Both points are in 80d7804. 1. The exchange is no longer handed to the workers. You are right that the values are the same for every
The CLI path is untouched: 2. The Reverse-checked: deleting the
3. The upgrade-guide entry stays where it is; #29591 and #29594 append their own at the end of the same The description is updated for the holder — the "Change" section explains why the headers are read before |
|
A note on the
So I read it as a main-branch problem, not something this PR introduces. Nothing to change here for it, but |
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Thanks @WMK666666, re-review of 80d7804. Both open points from my last review are handled:
- Batch workers reading the exchange: fixed.
resolveOcrSettings(exchange)(DoclingProducer.java:1615) resolves the two OCR decisions once, into the immutableOcrSettingsholder.processBatchConversionandprocessBatchStructuredDatacall it on the caller's thread and pass the holder down, so neitherconvertDocumentsBatchnorconvertStructuredDataBatchtouches the exchange any more, including inside thesupplyAsyncworkers. The request builders andapplyConfigurationToOptionstake only the holder, so the old shape cannot creep back in. The single-document paths resolve it where they already hold the exchange, and the precedence logic moved over unchanged. Batch is docling-serve only, so the CLI path needed no change, as you said. - The
enableOCR=falserule is now pinned on both paths:aLanguageHeaderDoesNotTurnOnOcrThatTheEndpointDisabledOnTheCliand...InTheServeRequest(DoclingOcrHeadersTest.java:122,:130). - Upgrade guide overlap: as expected, #29591 merged first, and the PR now conflicts on the end of
camel-4x-upgrade-guide-4_24.adoc. That file is the only conflict. Please rebase on main and keep bothcamel-doclingsections (theapiKeyHeaderone from main and your OCR-headers one).DoclingProducer.javamerges cleanly with #29591 and #29593.
No new problems in this commit. Approving; it can merge once the rebase is pushed and CI is green.
Claude Code on behalf of davsclaus. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.
…e request Rebased on main now that apache#29591 and apache#29593 are in. The only conflict was the end of the camel-docling section of the 4.24 upgrade guide, where apache#29591 appended its apiKeyHeader entry: both entries are kept, the apiKeyHeader one first. DoclingProducer.java also gained the bearer-token client from apache#29593; it merged cleanly with the OCR header resolution, which sits in resolveOcrSettings and applyConfigurationToOptions. Content is otherwise the one approved in 80d7804: the two headers are resolved once per message into OcrSettings on the thread that owns the exchange, so no batch worker touches the exchange, and a language header does not turn OCR back on when the endpoint disabled it.
80d7804 to
de03c2d
Compare
|
Rebased on main (
The diff is unchanged by the rebase, which is the check I used to confirm nothing was lost: CI will need another approval for the new commit; nothing else has changed. |
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
TL;DR
Approve: the force-push squashes and rebases the approved 80d7804 with no change to its content. Blockers: None. Other findings: 0
Thanks @WMK666666. The PR's own changed lines before and after the force-push are identical across DoclingProducer.java, DoclingOcrHeadersTest.java and the upgrade guide. The upgrade-guide conflict is resolved as asked: the apiKeyHeader section from #29591 comes first, then the OCR-headers section.
Heads-up: #29594 also appends a camel-docling section at the same spot in the upgrade guide, so whichever of the two merges second needs a trivial rebase.
Claude Code on behalf of davsclaus. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.
…ocling-serve Rebased on main now that apache#29595 is in, as asked. The only conflict was the 4.24 upgrade guide, where apache#29595 appended its OCR-headers entry after apache#29591's apiKeyHeader one: the file now carries all three camel-docling sections, apiKeyHeader first, then the OCR headers, then html_split_page. DoclingProducer.java merged cleanly with the OcrSettings refactor from apache#29595: the html_split_page rejection and the package-private mapToOutputFormat sit below resolveOcrSettings and are untouched by it. DoclingContentExtractor.java is unchanged on main. Content is otherwise the version approved in 55f1def: DoclingContentExtractor reads doctags_content when DocTags is requested, and mapToOutputFormat rejects html_split_page with the reason and the way out, because DocumentResponse has no field for the split pages.
The component docs have a Using headers to control processing section that sets three headers per
message on a plain
docling:endpoint:CamelDoclingOperationis honoured. The two OCR headers are declared inDoclingHeaderswith@Metadata, documented in that section (and in the YAML tab next to it), and read by nothing: a search oversrc/mainfinds no use ofDoclingHeaders.ENABLE_OCRorDoclingHeaders.OCR_LANGUAGEoutside theirdeclaration. The example therefore did nothing — every message was OCRed exactly as the endpoint was
configured, whichever header the route set.
Change
Both paths read them now:
addOcrArgumentstakes the exchange, and the headers overrideenableOCRandocrLanguagefor thatmessage; unset headers fall back to the endpoint configuration as before.
applyConfigurationToOptionsapplies the same two overrides before it fills the docling-serve request,so the request and the CLI command agree.
The two values are resolved once per message, on the thread that owns the exchange, into a small holder that
buildConvertRequestandbuildStructuredDataRequesttake instead of the exchange. The batch operationshand the exchange to up to
batchParallelismworkers at once (convertDocumentsBatch,convertStructuredDataBatch), and a batch that times out does not stop the tasks already running, so aworker that read the headers itself would be reading an exchange another thread may still be using. The
values are the same for every document of a batch, so reading them once before the workers start costs
nothing and removes the sharing.
This is the same shape as
CamelDoclingUseAsyncMode, which already overridesuseAsyncModeper message.The case worth having is a route that mostly sees text PDFs but occasionally a scanned attachment: it can
turn OCR on for that one message instead of adding a second endpoint. Symmetrically, an endpoint with OCR
on can skip it for a message that does not need it. A header that turns OCR off does not pass a language
on, and a language header alone turns OCR on unless the endpoint has disabled it with
enableOCR=false.The language is docling's own identifier
The first version of this PR checked the value against
[A-Za-z0-9_\- ,]+and threw for anything else.That rejected values docling accepts —
iso:zh-Hant,script/Latin,eng+deu— and it applied to theocrLanguageoption as well, sodocling:convert?...&ocrLanguage=iso:deworked before the PR and failedafter it. The check is gone.
It is a different thing from
CamelDoclingCustomArguments, which is validated: a custom argument can be aflag (
--to,--output) that overrides a producer-managed option, or a path, which is whyvalidateCustomArgumentschecks flags against an allowlist and paths for traversal. The OCR language isneither. It is a single element of the
List<String>handed toProcessBuilder— no shell is involved, so;,|and$()never reach an interpreter — and docling's identifiers legitimately contain:(iso:),/(script/Latin, which the path check would reject as a path) and+(eng+deu). There is no subsetworth allowing that does not cut off valid input, so the value goes through as it is.
Tests
DoclingOcrHeadersTestbuilds the CLI command and the docling-serve request for a message with and withoutthe headers, and for an endpoint whose options say the opposite:
--ocr-lang enCamelDoclingEnableOCR=false--no-ocr, no--ocr-langCamelDoclingOCRLanguage=es--ocr-lang esenableOCR=false&ocrLanguage=de, headerstrue/es--ocr-lang es, no--no-ocrenableOCR=false, headerCamelDoclingOCRLanguage=es--no-ocr, no--ocr-lang;doOcr=false, no languageCamelDoclingOCRLanguage=iso:zh-Hant--ocr-lang iso:zh-Hant, andiso:zh-Hantin the serve requestscript/Latin,eng+deu,en, deReverting
addOcrArgumentsto read only the configuration fails the header cases and leaves the firstgreen. Putting the language check back fails exactly the two
iso:zh-Hantcases and leaves the other tengreen. Dropping the
enableOCR=falsefallback inresolveOcrSettingsfails exactly the new serve case andleaves the other thirteen green: a language header would otherwise turn OCR back on for that message.
mvn -pl components/camel-ai/camel-docling test: 100 tests, 2 failures, bothDoclingCustomArgsValidationTeston Windows (/etc/cron.dis not an absolute path there), which fail thesame way without this change.