Croway commented on code in PR #2079:
URL:
https://github.com/apache/camel-spring-boot/pull/2079#discussion_r4527675311
##########
core/camel-spring-boot/src/test/java/org/apache/camel/spring/boot/util/CamelPropertiesHelperTest.java:
##########
@@ -399,6 +418,39 @@ public void
testCopyConfigurationPropertiesAppliesOptionConfiguredToItsDefault()
Assertions.assertTrue(target.isVerifyHostname(), "A configured option
must be applied whatever its value");
}
+ @Test
+ void testCopyConfigurationPropertiesCopiesProgrammaticallyAssignedValue() {
+ MyClass target = new MyClass();
+
+ // Simulate the same pattern as SpringBootPlatformWebMvcConfiguration:
an auto-configuration class
+ // sets a value on the configuration bean in Java (not from Spring
Boot application properties).
+ // Before the fix (CAMEL-26782), this programmatically-assigned value
was silently dropped because
+ // it was not recorded in BoundConfigurationProperties.
+ MyConfiguration cfg = configWithoutDrift();
+ cfg.setRequestTimeout(30000L); // assigned in Java, not via
application properties
+
+ CamelPropertiesHelper.copyConfigurationProperties(camelContext,
context, PREFIX, cfg, target);
+
+ Assertions.assertEquals(30000L, target.getRequestTimeout(),
+ "A value assigned programmatically in Java must be copied to
the target");
+ }
+
+ @Test
+ void
testCopyConfigurationPropertiesDoesNotCopyProgrammaticValueMatchingDefault() {
Review Comment:
This test repeats
`testCopyConfigurationPropertiesDoesNotOverwriteWithCatalogDefault`:
`configWithoutDrift()` already sets `secure` to `false`, so
`cfg.setSecure(false)` changes nothing. It passes with or without the change in
`CamelPropertiesHelper`; I checked by reverting the helper, and only
`testCopyConfigurationPropertiesCopiesProgrammaticallyAssignedValue` fails then.
To exercise the new comparison, pass a `new MyDriftedConfiguration()` with
only `name` set, so that `anotherOptionOnlyCarryingItsDefault` stays at its
default. `MyClass` has no setter for it, so it must be dropped without failing
startup. That also checks that the defaults come from the runtime class of the
source: if they came from `MyConfiguration`, the option would count as assigned
in Java and the copy would fail.
##########
core/camel-spring-boot/src/main/java/org/apache/camel/spring/boot/util/CamelPropertiesHelper.java:
##########
@@ -100,7 +101,20 @@ public static void
copyConfigurationProperties(CamelContext camelContext, Applic
// only the options the application configured are copied, the others
merely carry their catalog default
Review Comment:
The class Javadoc of `copyConfigurationProperties` (L70-75) and this comment
still say that only the options recorded in `BoundConfigurationProperties` are
copied. They should also mention options assigned in Java to the configuration
class that differ from its default.
The comment just below names `SpringBootPlatformWebMvcConfiguration` as the
case this handles, which it doesn't (see the comment on the platform-http test).
##########
components-starter/camel-platform-http-starter/src/test/java/org/apache/camel/component/platform/http/springboot/SpringBootPlatformHttpRequestTimeoutFromMvcPropertiesTest.java:
##########
@@ -0,0 +1,71 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.camel.component.platform.http.springboot;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+import org.apache.camel.CamelContext;
+import org.apache.camel.component.platform.http.PlatformHttpComponent;
+import org.apache.camel.spring.boot.CamelAutoConfiguration;
+import org.apache.camel.test.spring.junit6.CamelSpringBootTest;
+
+import org.junit.jupiter.api.Test;
+
+import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.boot.autoconfigure.EnableAutoConfiguration;
+import org.springframework.boot.test.context.SpringBootTest;
+import org.springframework.context.annotation.Configuration;
+
+/**
+ * CAMEL-26782 regression test: verifies that when the request timeout is set
+ * programmatically via {@code spring.mvc.async.request-timeout} (which
+ * {@link SpringBootPlatformWebMvcConfiguration#configureAsyncSupport} picks up
+ * and writes onto the configuration bean), the value survives
+ * {@code CamelPropertiesHelper.copyConfigurationProperties()} and appears on
+ * the live {@link PlatformHttpComponent}.
+ */
+@EnableAutoConfiguration
+@CamelSpringBootTest
+@SpringBootTest(
+ webEnvironment = SpringBootTest.WebEnvironment.MOCK,
+ classes = {
+ CamelAutoConfiguration.class,
+ PlatformHttpComponentAutoConfiguration.class,
+ SpringBootPlatformHttpAutoConfiguration.class,
+ SpringBootPlatformWebMvcConfiguration.class,
+
SpringBootPlatformHttpRequestTimeoutFromMvcPropertiesTest.TestConfiguration.class
+ },
+ properties = {
+ "spring.mvc.async.request-timeout=5000ms"
+ })
+public class SpringBootPlatformHttpRequestTimeoutFromMvcPropertiesTest {
+
+ @Autowired
+ CamelContext camelContext;
+
+ @Test
+ public void requestTimeoutFromMvcPropertiesIsCopiedToComponent() {
+ PlatformHttpComponent component =
camelContext.getComponent("platform-http", PlatformHttpComponent.class);
+ assertEquals(5000L, component.getRequestTimeout(),
Review Comment:
**This test fails on this branch**
```
SpringBootPlatformHttpRequestTimeoutFromMvcPropertiesTest.requestTimeoutFromMvcPropertiesIsCopiedToComponent:63
request-timeout set via spring.mvc.async.request-timeout must be propagated
to PlatformHttpComponent ==> expected: <5000> but was: <0>
```
This is the ordering problem from my earlier comment.
`SpringBootPlatformHttpAutoConfiguration.platformHttpEngineRequestMapping` is
eager (CAMEL-24351) and calls `camelContext.getComponent("platform-http")`, so
the component is created and customized before `requestMappingHandlerAdapter`
calls `SpringBootPlatformWebMvcConfiguration.configureAsyncSupport`. That
method is where `requestTimeout` is written into the configuration bean. By
then the copy has already happened, so no change in `CamelPropertiesHelper` can
fix this case.
In the Spring Boot engine, nothing reads
`PlatformHttpComponent.requestTimeout` at runtime either. Only
`VertxPlatformHttpConsumer` uses it, and Spring MVC applies
`spring.mvc.async.request-timeout` itself. So the only visible effect is what
the getter returns.
I'd suggest dropping this test, and the platform-http example from the PR
description, the code comment in `CamelPropertiesHelper` and the docs. The
helper change still stands on its own for application code that assigns values
to a configuration bean (the `setSkipRequestHeaders(true)` case in the JIRA).
If the getter should reflect the MVC timeout, that is a separate change: a
`ComponentCustomizer`, like the Kafka bridge on main, that applies
`spring.mvc.async.request-timeout` only when
`camel.component.platform-http.request-timeout` is not set.
##########
components-starter/camel-ai-tool-starter/src/test/java/org/apache/camel/component/ai/tool/springboot/AiToolComponentAutoConfigurationTest.java:
##########
@@ -91,4 +92,17 @@ public void routeIsRegisteredAsToolUnderTheConfiguredTag() {
assertTrue(spec.getParametersJsonSchema().contains("\"name\""),
"expected the declared parameter in the JSON schema, but was:
" + spec.getParametersJsonSchema());
}
+
+ @Test
+ public void mcpHintsStayNullWhenNotConfigured() {
+ // The MCP hints (readOnlyHint, destructiveHint, idempotentHint,
openWorldHint) default to
+ // false in AiToolComponentConfiguration but to null in
AiToolComponent. CAMEL-26782 must
+ // not copy the configuration-class defaults onto the component when
they were not explicitly
+ // set by the user, because null carries meaning in the MCP protocol.
+ AiToolComponent component = assertInstanceOf(AiToolComponent.class,
context.getComponent("ai-tool"));
+ assertNull(component.getConfiguration().getReadOnlyHint(),
"readOnlyHint must stay null when not configured");
+ assertNull(component.getConfiguration().getDestructiveHint(),
"destructiveHint must stay null when not configured");
+ assertNull(component.getConfiguration().getIdempotentHint(),
"idempotentHint must stay null when not configured");
+ assertNull(component.getConfiguration().getOpenWorldHint(),
"openWorldHint must stay null when not configured");
Review Comment:
Minor: `returnDirect` is also initialized to `false` in
`AiToolComponentConfiguration` and is `null` on `AiToolComponent` by default,
so it could be asserted here too.
##########
core/camel-spring-boot/src/main/docs/starter-configuration.adoc:
##########
@@ -62,6 +62,10 @@ The `Bean` name has to be equal to that of the Component,
Dataformat or Language
External configuration is still applied on top of such a `Bean`: every
`camel.[component|language|dataformat].[name].[parameter]` that is set in the
external configuration is copied onto it. A parameter that is not set in the
external configuration is left alone, so a value set on the `Bean` in Java is
kept, even when it differs from the documented default of that parameter.
+Camel also detects values that were assigned programmatically in Java directly
to the generated `@ConfigurationProperties` class (for example, in an
auto-configuration class that calls `cfg.setRequestTimeout(30000L)` before
returning the bean). Such a value is copied to the target component even when
it was not bound from external configuration, provided it differs from the
class's default value. This allows Spring Boot auto-configuration classes to
programmatically set component options that are then correctly propagated to
the component.
Review Comment:
Two points on this paragraph:
- The example ("an auto-configuration class that calls
`cfg.setRequestTimeout(30000L)` before returning the bean") is the
platform-http case, which doesn't work (see the comment on the platform-http
test). A user-code example is more accurate, such as a configuration class that
injects `HttpComponentConfiguration` and calls `setSkipRequestHeaders(true)`.
- It qualifies the paragraph above, which says that "a value set on the
`Bean` in Java is kept". A value assigned in Java to the configuration class
now overrides it. The doc should state that precedence, and that such an
assignment only takes effect if it happens before the component is first
created, since the copy happens once, when the component is created.
--
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]
