Repository navigation
camel-docling: send the bearer token in the Authorization header - #29593
Conversation
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
In writeValueAsString, JacksonException is caught and re-wrapped in a bare RuntimeException (line 58), while readValue correctly wraps it in JsonReadException (line 46). This asymmetry means callers lose the specific exception type on write failures.
Worth noting: in Jackson 3 (tools.jackson), JacksonException already extends RuntimeException — it's unchecked — so the entire try-catch block is unnecessary. The exception would propagate naturally without any wrapping, preserving the original JacksonException type for callers that care.
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.
LGTM. Nice fix, and the stub-server test that asserts the actual headers per scheme is the right test. Checked against docling-serve-client 0.7.0: the only auth option is apiKey(...) (sent as X-Api-Key), DoclingServeClientBuilderFactory.newBuilder() picks the Jackson 3 client first, and every request path (executePost, executeGet and both stream variants, so async polling too) goes through createRequestBuilder. So BEARER and oauthProfile really sent nothing before, and the subclass covers all calls.
Small points, none blocking:
BearerTokenDoclingServeClient.java:59-68: as in the open bot thread, drop the try/catch inwriteValueAsString.JacksonExceptionis unchecked in Jackson 3 and the upstream Jackson 3 client has no wrapping there, so the class then mirrors it as its Javadoc says.toBuilder()(line 77) silently drops the token. Camel never calls it and the comment covers it; throwingUnsupportedOperationExceptionwould be stricter if you prefer.- This subclasses a protected API of a 0.x library, so a client bump could break it;
DoclingProducerAuthTestwould catch that. Opening the upstreambearerToken(...)request you mention and linking it from the class Javadoc would make it easy to remove later.
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:
🔁 1 test passed only after a retry on JDK 21 (1 retried attempt) Recovered flaky tests on JDK 21 (1)
🔬 Scalpel shadow comparison — Scalpel: 10 of 722 tested, 27 compile-only — current: 10 all testedMaveniverse Scalpel detected 10 affected modules (current approach: 10). Skip-tests mode would test 10 modules (1 direct + 9 downstream), skip tests for 27 (generated code, meta-modules) Modules Scalpel would test (10)
Modules with tests skipped (27)
All tested modules (37 modules, 6m 28s total)Total reactor time: 6m 28s
Top 20 slowest modules:
|
oscerd
left a comment
oscerd
left a comment
There was a problem hiding this comment.
Real bug: configuring authenticationScheme=BEARER (or an oauthProfile, which resolves to a bearer token) sent no credentials at all, because the producer only called builder.apiKey(...) in the API_KEY branch and the docling-serve client has no bearer option. So a user who thought they had secured the call was sending it unauthenticated, which is worse than an error.
Adding the Authorization: Bearer <token> header through the BearerTokenDoclingServeClient subclass is a reasonable way around the SDK not exposing it, and the comment explaining that the token is not part of DoclingServeApiConfig so a builder cannot carry it is the right justification for the subclass rather than a config tweak. I checked that the credential is handled correctly: authenticationToken is @UriParam(label = "security") with security = "secret" and shows secret: true in the catalog, so it is masked in logs and, with #29597, on the TUI screens. The tests pin both schemes and, importantly, that they do not cross over: BEARER sets Authorization and no X-Api-Key, API_KEY sets X-Api-Key and no Authorization. LGTM.
Claude Code on behalf of oscerd
|
Both points are in 55924d3.
I also linked the upstream request from the class Javadoc — docling-project/docling-java#785 adds a I left |
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Re-review of 55924d3, which is on top of the a9c1d2d I approved.
BearerTokenDoclingServeClient.java:63-69:writeValueAsStringno longer wraps the exception, so Jackson 3's uncheckedJacksonExceptionpropagates the same way it does in the upstream Jackson 3 client. This fixes the open bot thread and my point 1.readValuestill maps toJsonReadException, which is right.- The class Javadoc now links docling-project/docling-java#785 (open, adds
bearerTokento the client), so the subclass is easy to find and remove later (my point 3). toBuilder()is unchanged, with a reason given. That is fine: it was optional, and the comment explains it.
No other changes. Still LGTM, pending the queued CI run on 55924d3.
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.
authenticationScheme=BEARER and oauthProfile never sent any credentials. The
only authentication the docling-serve client knows is an API key, which it
always puts in the X-Api-Key header, and DoclingProducer only called
builder.apiKey(...) in the API_KEY branch:
if (configuration.getAuthenticationScheme() == AuthenticationScheme.API_KEY
&& configuration.getAuthenticationToken() != null) {
builder.apiKey(configuration.getAuthenticationToken());
}
doclingServeApi = builder.build();
So a BEARER endpoint - including every endpoint using oauthProfile, because
resolveOAuthToken() sets the scheme to BEARER after it resolves the token -
started up, logged "with authentication: BEARER", and then sent every request
without an Authorization header. Against docling-serve behind an OAuth proxy
(which is how docling-serve is secured, it has no built-in auth) each request
failed with 401 while the endpoint looked correctly configured.
The client offers no builder option for any other header, but it leaves
createRequestBuilder(RequestContext) open for subclasses. BearerTokenDoclingServeClient
extends DoclingServeClient and adds the header there; DoclingProducer uses it
when the scheme is BEARER. Everything else, including the JSON codec, is the
same as the client the builder would have created.
getDoclingServeApi() is package private so the producer's wiring can be tested
without a real docling-serve.
DoclingProducerAuthTest starts a stub docling-serve and asserts the credentials
on the wire for each scheme: BEARER -> Authorization: Bearer <token> and no
X-Api-Key, API_KEY -> X-Api-Key and no Authorization, NONE -> neither.
…son 3 JacksonException is unchecked in Jackson 3, and the Jackson 3 client that the builder creates does not wrap it either, so the try-catch only turned a specific exception into a bare RuntimeException. Drop it, and readValue keeps the JsonReadException it throws on purpose. Link the upstream request for a bearer token option so the subclass can be removed once docling-java offers one.
55924d3 to
7766cd0
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.
authenticationScheme=BEARERandoauthProfilenever sent any credentials at all.The only authentication the docling-serve client knows is an API key, which it always puts in the
X-Api-Keyheader, andDoclingProduceronly calledbuilder.apiKey(...)in theAPI_KEYbranch:So a
BEARERendpoint -- including every endpoint usingoauthProfile, becauseresolveOAuthToken()sets the scheme toBEARERright after it resolves the token -- started up fine, loggedwith authentication: BEARER, and then sent every request without anAuthorizationheader. docling-serve itself has no built-in auth; it is secured by putting an OAuth proxy in front of it (docling-serve docs), and that proxy wantsAuthorization: Bearer <token>. So the documented Bearer/OAuth setup produced 401 on every request while the endpoint looked correctly configured, with nothing in the log to explain it.Fix
The client has no builder option for any other header (
DoclingServeApiConfigonly carriesAPI_KEY, in 0.7.1 as well), but it does leavecreateRequestBuilder(RequestContext)open for subclasses.BearerTokenDoclingServeClientextendsDoclingServeClientand adds the header there;DoclingProduceruses it when the scheme isBEARER. Everything else -- the HTTP setup, the operations, and the JSON codec -- is the same as the client the builder would have created, so the only difference between anAPI_KEYand aBEARERendpoint is which header carries the credentials.getDoclingServeApi()is package private so the producer's wiring can be asserted directly, without a real docling-serve.Test
DoclingProducerAuthTeststarts a stub docling-serve on a loopback port and asserts what actually goes on the wire for each scheme:AuthorizationX-Api-KeyBEARERBearer secret-tokenAPI_KEYsecret-tokenNONEReverting only the
BEARERbranch inDoclingProducermakes exactly the first row fail (expected: "Bearer secret-token" but was: null) and leaves the other two green.The two failures are
DoclingCustomArgsValidationTest.customArgsWithAbsolutePath{,EqualsForm}AreRejected, and they are unrelated and pre-existing on Windows:/etc/cron.dis not an absolute path there, so the absolute-path guard does not fire and the request fails later with anIOException. They fail identically with this change reverted to the originalDoclingProducer. Everything else, plusinstallwith formatter, license, enforcer and rat, passes.Note on the upstream fix
Adding a
bearerToken(...)option toai.docling:docling-serve-clientwould be the natural place for this, and this subclass could then go away. I am happy to open that upstream and rework this PR on top of a release if you would rather not carry the subclass in the meantime.