Croway commented on PR #29595:
URL: https://github.com/apache/camel/pull/29595#issuecomment-6800186561
Reading the headers on both paths looks good. One regression and two small
things:
1. **`validateOcrLanguage` rejects languages that docling accepts**
(`DoclingProducer.java:170`, `:2345`). The allow-list `[A-Za-z0-9_,\- ]+`
rejects:
- `iso:zh-Hant`, the BCP-47 form `docling convert --help` documents since
docling 2.127.0;
- `script/Latin` and `eng+deu`, Tesseract IDs listed in docling's own
`tesseract_ocr_cli_model.py`.
It also applies to the endpoint `ocrLanguage` option, not only the
header. A route with `ocrLanguage=iso:de` works on main (`--ocr-lang iso:de`)
and fails with `IllegalArgumentException` on this branch.
The check also adds no protection:
- The command runs through `ProcessBuilder`, so no shell is involved.
- docling's CLI parser (typer) takes the token after `--ocr-lang` as its
value even when it starts with `-`. `--ocr-lang --no-ocr` gives
`ocr_lang='--no-ocr'` with OCR still on.
- docling already validates Tesseract IDs itself (`_VALID_LANG_RE =
^[a-zA-Z0-9_/][a-zA-Z0-9_/+-]*$`).
I'd drop the check. If you keep it, apply it only to the header, not to
the route author's `ocrLanguage`, and allow at least `:`, `/` and `+`.
2. **Upgrade guide** (`camel-4x-upgrade-guide-4_24.adoc:7944`): "a language
header without an OCR header turns OCR on" is only true when the endpoint has
OCR on. With `enableOCR=false` and only `CamelDoclingOCRLanguage=es`, the CLI
still gets `--no-ocr` and docling-serve gets `doOcr=false`. Please reword it,
or change the code if OCR should turn on in that case.
3. **Description:** it still says "Only the CLI command is touched" and
doesn't mention the docling-serve path or the new language check. The earlier
LGTMs were given on the CLI-only version.
_Claude Code on behalf of Croway_
--
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]
