Repository navigation
Conversation
gnodet-bot
left a comment
There was a problem hiding this comment.
Clean, focused bug fix that adds doctags and txt case labels to all three switch-based mapping methods plus the extraction switch. API usage verified correct (DocumentResponse.getDoctagsContent() confirmed in docling-serve-api 0.7.0 JAR). Tests are well-structured and cover the new paths including case-insensitivity.
The PR also fixes two pre-existing alias inconsistencies: missing md case in mapToDoclingFormat and missing txt case in getOutputFileExtension. All cases use explicit return statements with no unintended fall-through.
No issues found. ✅
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
There was a problem hiding this comment.
The doctags/txt fixes are correct, and the tests match how docling-serve fills only the requested field. One thing to fix in this PR, since it is the same bug on the same maps:
html_split_pagehas the same problem asdoctags. The description says no map ever selected it, butmapToOutputFormatdoes (DoclingProducer.java:1682-1683,case "html_split_page": return OutputFormat.HTML_SPLIT_PAGE). So withuseDoclingServe=truethe request asks docling-serve only forHTML_SPLIT_PAGE, whileDoclingContentExtractor.mapOutputFormatfalls back to markdown and readsmarkdown_content, which was not requested: an empty body, exactly the doctags symptom. On the CLI path,mapToDoclingFormatstill turns it into--to md. It is one of the six documentedoutputFormatvalues, so please either map it through (read the field docling-serve fills for it in the extractor, use--to html_split_pagewith anhtmlextension ingetOutputFileExtensionfor the CLI, and add the cases to the two new tests), or, if docling-serve does not return it in the body, fail with a clear message instead of an empty body.- Minor:
DoclingContentExtractorTestcovers the extractor with a hand-built response, which is fine; no producer-level test exercises the serve path end to end, consistent with the existing tests.
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, 5m 36s total)Total reactor time: 5m 36s
Top 20 slowest modules:
|
oscerd
left a comment
There was a problem hiding this comment.
Good catch that the three maps translating outputFormat each knew a different subset and doctags was missing from the ones that matter, so a documented value produced the wrong output. Adding the doctags case to mapOutputFormat and reading document.getDoctagsContent() in the extractor, plus the txt/doctags cases in the format maps, lines the six documented values up with what the engine is actually asked for. The note that docling-serve only fills the fields of the requested formats is the right reason to read the matching field, and doctagsReturnsTheDoctagsContent covers it. LGTM.
Claude Code on behalf of oscerd
|
docling-serve cannot return it in the body. What the PR does now:
Reverse-checked: dropping the guard fails only the two serve-path tests, dropping the On point 2: agreed, no producer-level serve test exists for the other formats either — Regression: the component's 100 unit tests pass apart from |
|
Added one thing to the same branch: an entry in |
|
The docling-serve side looks right:
Claude Code on behalf of Croway |
|
Both points are taken in 7d32190. The CLI half is gone (@Croway). The CLI operations pass a hardcoded format to
What is left is the doctags half you both reviewed: the extractor reads Formatter: Interaction with #29595: that PR replaces Tests: |
davsclaus
left a comment
There was a problem hiding this comment.
Re-review of 1617c89. Since fc27b5b there are four commits: html_split_page now fails, there is an upgrade-guide entry, the CLI half is gone, and the newest commit moves the rejection into mapToOutputFormat.
My point 1 is fixed with the second option I gave: fail with a clear message. That is the right choice, because DocumentResponse has no field for the split pages, so there is nothing to map them to. Croway's comment is handled too:
- CLI half dropped: I confirmed that every CLI operation passes a fixed format (
DoclingProducer.java:309-353,:717), sooutputFormatnever reachedmapToDoclingFormat/getOutputFileExtension. Those two maps are back to whatmainhas, andDoclingOutputFormatTestis removed. - Overlap with #29595: the rejection now sits in
mapToOutputFormat(DoclingProducer.java:1673-1697), not inbuildConvertRequest.DoclingProducer.javanow merges cleanly with #29595. - Description: it no longer says
html_split_pageis left as it is.
What is left is small:
- Nit: the title still says "doctags and txt", but this PR no longer changes
txt(DoclingContentExtractor.mapOutputFormatalready handled it). Something like "camel-docling: return DocTags content and reject html_split_page on docling-serve" would fit. Since 1617c89, the description's "buildConvertRequestrejectshtml_split_page" should saymapToOutputFormat. - Suggestion: the
outputFormatoption still listshtml_split_pagein itsenums(DoclingConfiguration.java:55-56).outputFormatis only read bySUBMIT_ASYNC_CONVERSION, which needs docling-serve, so that value now always fails. Adding "html_split_page is not supported by docling-serve" to the option description (and regenerating) would make the catalog and the docs agree with the code. A follow-up is fine for this one. - #29591 and #29595 also append a
camel-doclingentry at the end ofcamel-4x-upgrade-guide-4_24.adoc(here at line 7938). Whichever PR merges second needs a trivial rebase.
The doctags half is unchanged from what I reviewed, and the upgrade-guide entry is accurate. CI has not run on 1617c89 yet (it is waiting for approval).
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.
|
Point 1 is in: the title is now camel-docling: return DocTags content and reject html_split_page on The description point was already handled when the body was rewritten at 16:23Z — it says Point 2 ( Point 3 (upgrade-guide conflict with #29591/#29595): understood, whichever lands second rebases. #29594's CI on 1617c89 is still waiting for approval, as you noted. |
|
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 |
1617c89 to
55f1def
Compare
|
Rebased on main (
Diff is unchanged by the rebase: Two open items from your last review, for the record:
CI on the new commit will need approval. |
davsclaus
left a comment
There was a problem hiding this comment.
TL;DR
Approve: a squash and rebase of 1617c89 with identical content, and the title and description nits are fixed. Blockers: None. Other findings: 2 (optional / heads-up)
Thanks @WMK666666. The changed lines of the squashed commit are identical to 1617c89, the version I last reviewed.
- Title and description: fixed; the description now says
mapToOutputFormatrejectshtml_split_page. - [Suggestion, follow-up is fine]
outputFormatstill listshtml_split_pagein itsenums(DoclingConfiguration.java:55-56), and that value now always fails on docling-serve. One sentence in the option description, plus a regen, would make the catalog agree with the code. - Heads-up: this PR and #29595 both append a
camel-doclingsection after theapiKeyHeaderone incamel-4x-upgrade-guide-4_24.adoc, so whichever 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.
|
Thanks @WMK666666, this is approved, but #29595 was just merged and touches the same camel-docling files, so this PR now has a merge conflict. Could you rebase it on main? We can merge it once CI is green again. Claude Code on behalf of davsclaus |
🔄 Backport BotThis bugfix targets
Add a Backport PRs will be created automatically on merge (once a port label is added). Comment ℹ️ If you push additional commits after |
…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.
55f1def to
14e39d4
Compare
|
Rebased on main (
The only conflict was, as you predicted, the 4.24 upgrade guide. It now carries three The diff is unchanged by the rebase, which is how I checked nothing was lost: CI will need another approval for |
gnodet-bot
left a comment
There was a problem hiding this comment.
Following up on my earlier review (SHA 55f1def) — the new commits add the html_split_page rejection that davsclaus flagged, plus two new test classes. All previous findings are addressed.
Checked the new code:
mapToOutputFormatthrowsIllegalArgumentExceptionforhtml_split_pageduring request-building (before the API call) — correct fail-fast behavior.- Case-insensitivity works:
.toLowerCase()is called before the switch, soHTML_SPLIT_PAGEhits thethrowcase. DoclingContentExtractorTestandDoclingServeOutputFormatTestcover the new code paths.doctagsReturnsTheDoctagsContentwould fail without the new extractor case;htmlSplitPageFailsWithTheReasonInsteadOfAnEmptyBodywould fail without the throw. Tests are valid.- Upgrade guide entry is accurate and actionable.
LGTM. ✅
This review was generated by an AI agent, Hermès on behalf of @gnodet.
|
Opened the |
outputFormatdocuments six values —md,json,html,html_split_page,text,doctags— and thedocling-serve path could not produce two of them:
doctagscame back as an empty body, andhtml_split_pagecame back as an empty body as well.What went wrong
DoclingContentExtractor.mapOutputFormathad nodoctagscase. docling-serve only fills the fields ofthe formats that were requested, so a DocTags conversion returns
doctags_contentand nomd_contentatall. The missing case sent it to the markdown branch, which found no markdown and returned
"": theexchange succeeded with an empty body. The same extractor is used by
DoclingConsumerwhen an async taskcompletes, so async DocTags conversions were empty too.
html_split_pagehas no field to read.DocumentResponsecarries five content fields —doctagsContent,htmlContent,jsonContent,markdownContent,textContent— and the html one isfilled only when HTML is requested; the split pages are not part of the response at all. The request asked
docling-serve for
HTML_SPLIT_PAGE, the response carried no content, and the body came out empty. It nowfails with an
IllegalArgumentExceptionthat names the reason and the way out,outputFormat=htmlto getthe HTML of the whole document.
Change
DoclingContentExtractorreadsgetDoctagsContent()fordoctags, andmapOutputFormatmaps it toitself instead of falling through to markdown.
mapToOutputFormatrejectshtml_split_pagewith the reason, for the same reason the doctags caseexists: docling-serve was asked for something it cannot return, and the caller got silence. That map is
where the question "can the service return this format" is answered, and it is where the value used to be
turned into
HTML_SPLIT_PAGEfor nothing.Nothing else moves. In particular the CLI maps are untouched: the CLI operations pass a hardcoded format to
buildDoclingCommand—markdown,html,jsonortext— sooutputFormatnever reachesmapToDoclingFormatorgetOutputFileExtensionwith any other value (#29594 had cases for them; they wereunreachable and are removed).
Tests
DoclingContentExtractorTestbuilds responses the way docling-serve does — only the requested format'sfield filled in — and asserts
doctagsreturns the DocTags content, thatDOCTAGSis accepted, that aresponse without DocTags gives an empty body rather than the markdown, and that the formats that already
worked still return their own content.
DoclingServeOutputFormatTestasserts the rejection and its message, and that the five values the servicecan return still map to the right
OutputFormat.Both halves were checked by reverting them: without the extractor case the three DocTags assertions fail
and the guards stay green; without the rejection
html_split_pagebuilds a request instead of throwing.mvn -pl components/camel-ai/camel-docling test: 93 tests, 2 failures, bothDoclingCustomArgsValidationTeston Windows (/etc/cron.dis not an absolute path there), which fail thesame way without this change.
formatter:validateandimpsort:checkpass.