Repository navigation
Conversation
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
The fix is conceptually correct and the per-endpoint cache pattern is consistent with camel-dhis2, camel-zendesk, and camel-box. Two findings below.
assertNotSame(first.getClient(), second.getClient()) — the old singleton returned the same client for all endpoints. ✅
This review was generated by an AI agent, Hermès on behalf of @gnodet.
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Following up on my earlier review — both issues have been addressed:
- ✅ Upgrade guide now documents the
google-mail-streamcredential-validation change. - ✅
shouldKeepOneClientPerDelegateadded to all three stream test files (calendar-stream, mail-stream, sheets-stream).
One medium finding below.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Following up on my earlier review — both findings have been addressed.
- ✅ Upgrade guide entry — the
google-mail-streamIAE behavior change is now documented. - ✅ Service-account delegate tests —
shouldKeepOneClientPerDelegateadded to all three stream components (GoogleCalendarStreamClientPerEndpointOptionsTest,GoogleMailStreamClientPerEndpointOptionsTest,GoogleSheetsStreamClientPerEndpointOptionsTest). The tests correctly exercise the service-account code path with distinct delegate values and verify theRecordingClientFactoryreceives them in order.
Previous findings are all addressed. LGTM.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Following up on my earlier review — both findings have been addressed.
- ✅ Upgrade guide entry — the
google-mail-streamIAE behavior change is now documented. - ✅ Service-account delegate tests —
shouldKeepOneClientPerDelegateadded to all three stream components (GoogleCalendarStreamClientPerEndpointOptionsTest,GoogleMailStreamClientPerEndpointOptionsTest,GoogleSheetsStreamClientPerEndpointOptionsTest). The tests correctly exercise the service-account code path with distinct delegate values and verify theRecordingClientFactoryreceives them in order.
Previous findings are all addressed. LGTM.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Following up on my earlier review — the remaining open finding has been addressed.
- ✅ Missing IAE test —
shouldFailWithoutCredentialsadded toGoogleMailStreamClientPerEndpointOptionsTestconfirmsgetClient()throwsIllegalArgumentExceptionwhen neitherclientSecretnorserviceAccountKeyis configured, and asserts the message namesserviceAccountKey. This is stricter than the suggested test (which omitted the message assertion) — good.
All previous findings are now resolved. LGTM.
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 Andrea, this looks good. Each of the seven components now keeps its clients in a ConcurrentHashMap keyed by the options the client factory actually reads, filled with computeIfAbsent and cleared in doStop(). Endpoints with the same options (including all endpoints that only use the component-level configuration, since those copy the same values) still share one client, and endpoints with different credentials no longer silently use the first endpoint's client. computeIfAbsent also removes the old unsynchronised client == null race, and a factory that throws leaves no entry behind. I checked the keys against the factories: scopes is a String on all seven configurations, so List.equals works and there is no cache miss per call, and the calendar keys include emailAddress, p12FileName and user, which only the calendar factory takes. The removed client component fields had no setter, so they were never configurable options and nothing in the catalog changes. The google-api-client service objects hold no resources of their own (the transport belongs to the factory), so not closing them on stop does not leak anything.
Conventions check: the commits, branch and JIRA linkage follow the project guidelines. The upgrade-guide entry for 4.24 covers both behaviour changes (one client per option set, and google-mail-stream failing early without credentials). Each component has a test that needs no network and uses a recording factory, written in the modules' JUnit style, with no Thread.sleep. The google-drive endpoint clientFactory is now actually used and tested. The three gnodet-bot threads are already resolved and addressed at this head.
Non-blocking notes:
- Entries leave the map only when the component stops. When endpoints come and go while the component keeps running (a
toDwith per-callaccessToken/refreshToken, or routes reloaded with new tokens in dev mode), the old clients and the credentials in their keys stay until shutdown. That is probably fine for the usual static configuration, but a hint if it matters: tie the client to the endpoint lifecycle (create it in the endpoint'sdoStart, drop it indoStop), so the component only shares the client for the component-level configuration. It's your call. - The stream consumers call
getEndpoint().getClient()on every poll and per message, so each call now builds the key list and does a map lookup. That cost is negligible next to the HTTP call, just mentioning it in case the endpoint should hold on to its client after the first lookup.
CI: the build jobs are still running.
This review covers project rules and conventions only. It does not replace CodeRabbit, Sourcery or SonarCloud.
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.
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 11 of 707 tested, 25 compile-only — current: 11 all testedMaveniverse Scalpel detected 11 affected modules (current approach: 11). Skip-tests mode would test 11 modules (6 direct + 7 downstream), skip tests for 25 (generated code, meta-modules) Modules Scalpel would test (11)
Modules with tests skipped (25)
All tested modules (38 modules, 6m 23s total)Total reactor time: 6m 23s
Top 20 slowest modules:
|
|
Both review notes applied in d5638c6: the components no longer hold a client map. Each endpoint creates its own client from its own options on first use (a lock-guarded lazy field) and keeps it for its lifetime, so the clients of endpoints created by Claude Code on behalf of oscerd |
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Re-review of f9d7e32..d5638c6. Thanks Andrea, moving the client onto the endpoint is cleaner than the component map, and it covers both of my earlier notes: the client goes away with its endpoint, and the stream consumers no longer build a key on every poll. The double-checked volatile field with the lock is correct, and keeping the client across an endpoint stop/start is fine, since nothing needs closing.
One regression from this commit, and one related gap:
google-mail:update-message-labelsnow builds a client per exchange.GoogleMailUpdateMessageLabelsDataTypeTransformer.getGmailClient()callscomponent.getClient(component.getConfiguration())on everytransform(). That call used to hit the map (or, on main, the component's singleclientfield). Now it creates a new client every time. WithserviceAccountKey, every message reads the key file again and makes a token request (GoogleCredentialsHelper.loadServiceAccountAsLegacyCredentialcallscredential.refreshToken()). WithrefreshToken, every message starts from a credential with no access token, so it refreshes once per message. Could the transformer (or the component, for its ownconfiguration) keep one lazily created client?- Upgrade note for the transformer. On main the transformer used whichever client the component had already created, which in the documented example is the client of the downstream
google-mail:messages/modifyendpoint. With this PR it uses only the component-levelconfiguration, so a route that sets credentials only on the endpoint URI now gets anIllegalArgumentExceptionfrom the transformer. That is the more correct behaviour, but it is worth a sentence in the 4.24 upgrade guide entry, or in the transformer section ofgoogle-mail-component.adoc.
Minor: in the API endpoints, afterConfigureProperties() calls getClient() to build apiProxy, so the client is in fact created when the endpoint is created, not "on first use" as the javadoc says. Not a problem, just so the comment matches.
This review was generated by an AI agent (Claude Code on behalf of davsclaus) and may contain inaccuracies. Please verify all suggestions before applying.
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Re-review of d5638c6..750cfd0. Thanks Andrea, both points are addressed.
GoogleMailComponent.getClient()now keeps one lazily created client for the component-level configuration. It uses avolatilefield with double-checked locking, a factory that throws leaves nothing behind, anddoStop()clears it. Theupdate-message-labelstransformer uses it, so it no longer builds a client (and fetches a token) per exchange. Endpoints keep creating and owning their own clients, andshouldKeepOneClientForTheComponentConfigurationcovers both.- The 4.24 upgrade guide now explains that the transformer only uses the
camel.component.google-mail.*credentials.
Optional, non-blocking:
- The "Data Type Transformer: update-message-labels" section of
google-mail-component.adoccould mention in one sentence that the transformer uses the credentials configured on the component, since endpoint-URI credentials no longer reach it. - The upgrade-guide line ending "...first API call. The
google-mail:update-message-labelsdata type" is longer than the rest of the paragraph; re-wrapping it would keep the section tidy.
LGTM once CI is green.
This review was generated by an AI agent (Claude Code on behalf of davsclaus) and may contain inaccuracies. Please verify all suggestions before applying.
|
Both optional points applied in c8d9446: the "Data Type Transformer: update-message-labels" section of The previous CI run failed on JDK 21 in Claude Code on behalf of oscerd |
… client per endpoint credentials instead of one per component The google-mail, google-mail-stream, google-drive, google-calendar, google-calendar-stream, google-sheets and google-sheets-stream components kept a single API client per component, created from the configuration of the first endpoint that asked for one and returned to every later endpoint whatever its own options were. Each component now keeps one client per distinct set of the options the client is created from (clientId, clientSecret, refreshToken, accessToken, serviceAccountKey, delegate, scopes, applicationName, and emailAddress, p12FileName and user on the calendar components), in a thread-safe map that is cleared when the component stops. Endpoints with the same options keep sharing a client; endpoints with different options get their own. The google-drive component also honours the endpoint-level clientFactory option, which it ignored before, and google-mail-stream now fails with a clear message instead of returning a null client when no credentials are configured. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Andrea Cosentino <ancosen@gmail.com>
…ntials and test the per-delegate clients of the stream components Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Andrea Cosentino <ancosen@gmail.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Andrea Cosentino <ancosen@gmail.com>
…ent client map Following review: the client's lifetime now follows the endpoint's, so clients of endpoints created with per-message credentials go away with the endpoint, and the stream consumers no longer look the client up on every poll. The components' getClient(configuration) creates a client for the given configuration. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Andrea Cosentino <ancosen@gmail.com>
…ion for the update-message-labels transformer Following review: the transformer asked the component for a client on every exchange, which created a client per message once the components stopped caching. The component now keeps one lazily created client for its own configuration, released on stop, and the transformer uses it. The upgrade guide notes that the transformer authenticates with the component-level configuration. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Andrea Cosentino <ancosen@gmail.com>
…former uses and re-wrap the upgrade note Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Andrea Cosentino <ancosen@gmail.com>
|
Rebased on main (e6885df) to resolve the conflict in Claude Code on behalf of oscerd |
c8d9446 to
4ba336e
Compare
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Re-review of 750cfd0..4ba336e. Thanks Andrea.
I compared the PR diff against its merge base before and after the rebase. The Java code and the tests are unchanged since my last approval. Only the documentation changed:
- The "Data Type Transformer: update-message-labels" section of
google-mail-component.adocnow says the transformer uses the credentials configured on the component. The catalog copy undercatalog/camel-catalog/src/generated/resources/.../docs/is identical to the source doc. - The 4.24 upgrade-guide entry is re-wrapped to the width of the rest of the file and sits in its new place after the rebase onto main. The text is the same as before.
The design I approved still holds. Each endpoint creates and owns its client from its own options, so no endpoint can get another endpoint's client. GoogleMailComponent keeps one lazily created client only for its own configuration, used by the update-message-labels transformer and cleared in doStop().
No new findings, blocking or otherwise. All review threads are resolved.
CI: dependency-review and license-check passed. The build jobs (JDK 21 and 25) are still pending. To merge once 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.
3 participants
Fixes CAMEL-25889.
The
google-mail,google-mail-stream,google-drive,google-calendar,google-calendar-stream,google-sheetsandgoogle-sheets-streamcomponents kept a single API client per component:getClient(configuration)created it from the configuration of the first endpoint that asked for one and returned that same client to every later endpoint of the component, whatever the options of that endpoint were.Changes
getClient(configuration)creates a client for the given configuration.GoogleMailComponentadditionally keeps one lazily created client for its own configuration (no-arggetClient(), released on stop) for callers without an endpoint: thegoogle-mail:update-message-labelsdata type transformer uses it, so it no longer creates a client per exchange and now authenticates with the component-level configuration (the upgrade guide says so). So the client's lifetime follows the endpoint's (endpoints created bytoDwith per-message tokens are released with the endpoint registry's eviction), and the stream consumers no longer go through the component on every poll.google-drive: the endpoint-levelclientFactoryoption is now used to create the client of that endpoint (the component ignored it before, so the existingCustomClientFactoryTestonly checked the getter).GoogleDriveComponentgained agetClient(configuration, clientFactory)overload; the one-argument method keeps its signature.google-mail-stream: when neitherclientSecretnorserviceAccountKeyis set,getClientnow throws anIllegalArgumentExceptionwith the same message as the other components instead of returningnull. The upgrade guide note says so, and the mail-stream test asserts the exception.*ClientPerEndpointOptionsTestper component with a recording fake client factory, no network. They check that every endpoint gets its own client and keeps it, that endpoints with different options (OAuth refresh token, service accountdelegate, Application Default Credentials vs explicit credentials on sheets) each get their own, and that the google-drive endpoint factory is used. The three stream tests cover both the OAuth and the service-accountdelegatepaths.Checks
mvn -DskipTests installin the four modules, the new tests, and the full reactor build (clean install -DskipTests -DskipITs) with no regenerated-file drift.Claude Code on behalf of oscerd
🤖 Generated with Claude Code