Repository navigation
camel-docling: mark the apiKeyHeader option as deprecated (it is never used) - #29591
Conversation
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Correct deprecation — apiKeyHeader is genuinely never read by DoclingProducer and the approach mirrors the camel-neo4j deprecation pattern. LGTM.
One minor observation:
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.
Thanks, the analysis holds: apiKeyHeader is never read (DoclingProducer only calls builder.apiKey(...), and docling-serve-client 0.7.0 hardcodes the header name to X-Api-Key). Deprecating it with a note, as with the camel-neo4j query option, is the right call.
Before merge:
-
Generated files are missing. The PR changes only the component's
docling.json, the adoc andDoclingConfiguration. These copies also change and are not in the PR:catalog/camel-catalog/src/generated/resources/org/apache/camel/catalog/components/docling.json(still"deprecated": falseforapiKeyHeader)catalog/camel-catalog/src/generated/resources/org/apache/camel/catalog/docs/docling-serve.adoc(any edit tosrc/main/docsneeds the catalog copy)DoclingEndpointBuilderFactory(camel-endpointdsl) andDoclingComponentBuilderFactory(camel-componentdsl): theapiKeyHeader(...)builders gain@Deprecatedand the note, asNeo4jEndpointBuilderFactory.query(...)has.
Please build camel-docling plus the catalog and DSL modules and commit everything that changes; CI fails on uncommitted generated files.
-
Upgrade guide. A deprecated option needs an entry in
docs/user-manual/modules/ROOT/pages/camel-4x-upgrade-guide-4_24.adoc, as the neo4j one has. Two lines are enough:apiKeyHeadernever had any effect, and the key is always sent asX-Api-Key. -
Nit (same as the open bot thread):
DoclingConfiguration.java:131-134usesX-API-Keyfor the default andX-Api-Keyin the note. Header names are case-insensitive, so it is cosmetic;X-Api-Keyis what the client sends.
Claude Code on behalf of davsclaus. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.
✅ Generated files are up to dateAn earlier CI run reported uncommitted generated changes; the latest run no longer does. |
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Following up on my earlier review — the generated files have been added (catalog JSON, catalog adoc, componentdsl, endpointdsl), which addresses davsclaus's main concern. Two items remain open.
Upgrade guide — still missing. A new entry is required in docs/user-manual/modules/ROOT/pages/camel-4x-upgrade-guide-4_24.adoc, matching the pattern used for the camel-neo4j query deprecation. Two lines are sufficient: apiKeyHeader never had any effect, and the key is always sent as X-Api-Key.
Casing inconsistency — still open (nit). The deprecationNote text and the Javadoc @deprecated tag both use X-Api-Key, while the defaultValue attribute and the field initializer still use X-API-Key. HTTP header names are case-insensitive, so there is no functional difference, but consistency is cleaner — pick one spelling.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Thanks, the Regen commit (29f1d7b) adds the generated files from point 1: the catalog docling.json and docling-serve.adoc, and the two DSL builders. The catalog doc matches the source doc. I approved the CI runs for this head.
Still needed before merge:
- Upgrade guide: a short entry in
docs/user-manual/modules/ROOT/pages/camel-4x-upgrade-guide-4_24.adoc(under acamel-doclingheading). It should say thatapiKeyHeaderis deprecated and never had any effect, because the docling-serve client always sends the key inX-Api-Key. - The nit in the open thread,
X-API-KeyvsX-Api-KeyinDoclingConfiguration, is optional.
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, 23 compile-only — current: 10 all testedMaveniverse Scalpel detected 10 affected modules (current approach: 10). Skip-tests mode would test 10 modules (5 direct + 9 downstream), skip tests for 23 (generated code, meta-modules) Modules Scalpel would test (10)
Modules with tests skipped (23)
All tested modules (37 modules, 5m 9s total)Total reactor time: 5m 9s
Top 20 slowest modules:
|
oscerd
left a comment
oscerd
left a comment
There was a problem hiding this comment.
Correct to deprecate rather than keep advertising an option that does nothing: since the docling-serve client always sends the API key in X-Api-Key, apiKeyHeader never changed anything, and a security-group option that silently has no effect is worse than no option. The deprecation note says exactly why, and removing it from the examples and tests stops people copying a no-op. One small thing, not blocking: deprecating an option is the kind of change the 4.24 upgrade guide usually gets a one-line note for, even when the option never worked, so anyone who set it knows it is going away. LGTM.
Claude Code on behalf of oscerd
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Following up on my earlier review — both open items have been addressed in the latest commit.
Upgrade guide — ✅ added under camel-docling in camel-4x-upgrade-guide-4_24.adoc. Correctly states that apiKeyHeader never had any effect and that X-Api-Key is the fixed header name.
Casing consistency — ✅ defaultValue and field initializer now use X-Api-Key consistently throughout (Java source, JSON metadata, DSL builders). The whole PR is coherent on the spelling.
No further issues. LGTM.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
|
Both remaining items are in 7f61c1b. Upgrade guide. An entry under a Casing. Picked The component, its tests and the two formatter checks are green locally. |
7f61c1b to
befeeaa
Compare
|
Heads up: the branch is now one commit on top of current Reason:
|
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Re-review of befeeaa (the branch was rebuilt as one commit on current main, so I compared it with main and with the reviewed 29f1d7b).
Both open points from my last review are done:
- Upgrade guide:
camel-4x-upgrade-guide-4_24.adocnow has acamel-docling - the apiKeyHeader option is deprecatedentry. It says the option never had any effect, that the key always goes inX-Api-Key, and what to do about it. - Casing:
DoclingConfiguration.java:130-134usesX-Api-Keyfor thedefaultValue, the field, thedeprecationNoteand the Javadoc. The generated copies match.
Rebuilding on main was the right move. The generated files now differ from main only in the two apiKeyHeader entries of both docling.json copies (identical blobs) and the three @Deprecated DSL builders. The catalog docling-serve.adoc matches the source doc. The format is the same as the neo4j query deprecation, and the removed "Using Custom API Key Header" section had no xrefs pointing at it.
Small things, none blocking:
- The PR description's "Changes" list still names only
DoclingConfiguration,docling.jsonand the adoc. It could also mention the upgrade-guide entry, the catalog/DSL regen and theX-API-Key->X-Api-Keydefault. - #29594 and #29595 also append a
camel-doclingentry to the end of the 4.24 upgrade guide. Whichever PR merges second has a trivial text conflict there and needs a rebase.
LGTM once the queued CI run on befeeaa is green.
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.
|
These tests failed |
… effect The docling-serve client sends the API key in the X-Api-Key header, the name is fixed in the client, and the producer never read the option: a user who set apiKeyHeader got no warning and no effect, while the option sat in the security group of the endpoint. Deprecate it with a note, as the camel-neo4j query option is deprecated, and drop it from the examples of the docling-serve page and from the tests, so nobody copies a no-op. The catalog metadata and the two DSL builder factories follow, and the 4.24 upgrade guide gets the entry a deprecation takes. The default value and the field initializer said X-API-Key while the deprecation note said X-Api-Key; both now say X-Api-Key, the spelling the client sends.
befeeaa to
7a2d06e
Compare
…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.
…ocling-serve 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 html_split_page rejection in mapToOutputFormat. Content is otherwise the one reviewed in 1617c89: 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.
camel-docling: honour the OCR headers in the CLI command and the serve request Rebased on main now that #29591 and #29593 are in. The only conflict was the end of the camel-docling section of the 4.24 upgrade guide, where #29591 appended its apiKeyHeader entry: both entries are kept, the apiKeyHeader one first. DoclingProducer.java also gained the bearer-token client from #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.
…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.
This keeps the option (removing it would break existing endpoint URIs) but marks it deprecated with a note that says why, which is the same treatment
camel-neo4jalready gives its ineffectivequeryoption.Changes
DoclingConfiguration:@Deprecated+deprecationNoteon theapiKeyHeaderfield, getter and setter, so the deprecation reaches tooling and the generated docs.docling.json: regenerated (deprecated/deprecationNoteforapiKeyHeader).docling-serve.adoc: dropped the "Using Custom API Key Header" section -- it documented a configuration that cannot work -- and removedapiKeyHeaderfrom theAPI_KEYexamples. Replaced with a note that the key always goes inX-Api-Key.No behaviour change: the option was already inert.
Verification
./mvnw -pl components/camel-ai/camel-docling install-> BUILD SUCCESS (formatter, checkstyle, enforcer and rat all pass). Reverting only the annotation and rebuilding makesdocling.jsonreport"deprecated": falseagain, so the metadata really is driven by the source annotation rather than edited by hand.