gnodet-bot commented on code in PR #28217:
URL: https://github.com/apache/camel/pull/28217#discussion_r4335142918
##########
components/camel-google/camel-google-mail/src/main/java/org/apache/camel/component/google/mail/stream/GoogleMailStreamComponent.java:
##########
@@ -49,20 +51,35 @@ public GoogleMailStreamComponent(CamelContext context) {
this.configuration = new GoogleMailStreamConfiguration();
}
- public Gmail getClient(GoogleMailStreamConfiguration
googleMailConfiguration) {
- if (client == null) {
- if (googleMailConfiguration.getClientSecret() != null) {
- client =
getClientFactory().makeClient(googleMailConfiguration.getClientId(),
- googleMailConfiguration.getClientSecret(),
googleMailConfiguration.getScopesAsList(),
- googleMailConfiguration.getApplicationName(),
googleMailConfiguration.getRefreshToken(),
- googleMailConfiguration.getAccessToken());
- } else if (googleMailConfiguration.getServiceAccountKey() != null)
{
- client = getClientFactory().makeClient(getCamelContext(),
googleMailConfiguration.getServiceAccountKey(),
- googleMailConfiguration.getScopesAsList(),
googleMailConfiguration.getApplicationName(),
- googleMailConfiguration.getDelegate());
- }
+ /**
+ * Gets the client for the given configuration. One client is kept per
distinct set of the options the client is
+ * created from, so endpoints with the same options share a client and
endpoints with different options get their
+ * own.
+ */
+ public Gmail getClient(GoogleMailStreamConfiguration config) {
+ return clients.computeIfAbsent(clientKey(config), key ->
createClient(config));
+ }
+
+ private Gmail createClient(GoogleMailStreamConfiguration config) {
+ if (config.getClientSecret() != null) {
+ return getClientFactory().makeClient(config.getClientId(),
+ config.getClientSecret(), config.getScopesAsList(),
+ config.getApplicationName(), config.getRefreshToken(),
+ config.getAccessToken());
+ } else if (config.getServiceAccountKey() != null) {
+ return getClientFactory().makeClient(getCamelContext(),
config.getServiceAccountKey(),
+ config.getScopesAsList(), config.getApplicationName(),
Review Comment:
💡 **Missing test for the new `IllegalArgumentException` behavior**: the
`else` branch here is new behavior introduced by this PR — the old
`getClient()` silently returned `null` when neither `clientSecret` nor
`serviceAccountKey` was set. The upgrade guide correctly documents this as a
breaking change, but neither this test file nor any other in the PR exercises
the `assertThrows(IllegalArgumentException.class, ...)` path.
A regression (e.g., removing the `else` branch or weakening the condition)
would go undetected. Add a test case like:
```java
@Test
void shouldThrowOnMissingCredentials() throws Exception {
try (CamelContext context = new DefaultCamelContext()) {
context.getComponent("google-mail-stream",
GoogleMailStreamComponent.class)
.setClientFactory(new RecordingClientFactory());
context.start();
GoogleMailStreamEndpoint endpoint = context.getEndpoint(
"google-mail-stream://box-a",
GoogleMailStreamEndpoint.class);
assertThrows(IllegalArgumentException.class, endpoint::getClient);
}
}
```
--
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]
