gnodet commented on PR #1709:
URL:
https://github.com/apache/camel-spring-boot/pull/1709#issuecomment-4053586544
Thanks for the review @davsclaus!
We've addressed your feedback and also fixed several issues found during an
in-depth code review:
**Review fixes (latest commit):**
1. **`@AutoConfiguration` ordering** — Changed from `@Configuration` +
`@AutoConfigureBefore` to `@AutoConfiguration(after =
KafkaAutoConfiguration.class, before = KafkaComponentAutoConfiguration.class)`.
This ensures the bridge runs after Spring Boot creates `KafkaProperties` and
before Camel's component auto-configuration applies the settings.
2. **`@ConditionalOnBean(KafkaProperties.class)`** — Added so the bridge
gracefully skips when Spring Kafka auto-configuration is excluded (e.g., user
excluded `KafkaAutoConfiguration`), preventing a startup failure from missing
constructor dependency.
3. **Per-property `Binder.bind().isBound()` for precedence checks** — The
original code used `Binder.bind("camel.component.kafka",
mapOf(...)).containsKey("ssl-keystore-location")` to check if a Camel property
was explicitly set. However, the Binder does **not** normalize camelCase keys
to kebab-case in map bindings, so `containsKey("ssl-keystore-location")` would
miss `sslKeystoreLocation` set by the user, causing their explicit Camel
property to be silently overwritten. We now use individual
`binder.bind("camel.component.kafka.ssl-keystore-location",
String.class).isBound()` calls which properly handle relaxed binding
(camelCase, kebab-case, underscore variants).
4. **Bootstrap servers default handling** —
`KafkaProperties.getBootstrapServers()` defaults to `["localhost:9092"]` and is
never null/empty. The original null/empty check would always bridge, even when
the user didn't set `spring.kafka.bootstrap-servers`. We now use
`isSpringPropertyBound("bootstrap-servers")` to check if the property was
explicitly set.
Regarding your suggestions:
- **JIRA ticket**: CAMEL-22760 already exists and is linked to this PR.
- **On/off toggle**: Good point — we could add a
`camel.component.kafka.bridge-spring-kafka-properties` (or similar) property.
We can add that in a follow-up if you'd like, or in this PR. What do you prefer?
- **Migration guide note**: Will add a note to the 4.19 migration guide.
--
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]
