Croway commented on PR #1995: URL: https://github.com/apache/camel-spring-boot/pull/1995#issuecomment-5813077939
Thanks @smongiar, the rework looks good, and the main code matches the proposal. One more round, on the tests only. I checked the 4 tests by breaking the advisor in specific ways and seeing which tests fail: - `nonEnumerableSourceFailsWithoutAdvisor` never fails when the advisor is broken. It only shows that Spring Boot misbehaves without it, and `nonEnumerableSourceDoesNotBindIntoThirdPartyTypes` already fails when the advisor is disabled. - `enumerableSourceBindsNestedOptions` exercises the same branch (direct value) as `nonEnumerableSourceStillBindsConfiguredOptions`. - Nothing fails if the `containsDescendantOf(...) == PRESENT` check is removed. - Nothing fails if the `@Bean` registration in `CamelAutoConfiguration` is removed, because every test registers the advisor by hand. The patch below replaces them with 3 non-overlapping tests. They load the real `CamelAutoConfiguration` via `ApplicationContextRunner`, as `CamelSecurityPolicyAutoConfigurationTest` does, and add a small third-party POJO fixture next to the existing `com.example.springboot.ThirdPartyProperties`. Each breakage is now caught: | Breakage | Caught by | |---|---| | Advisor never skips | `nonEnumerableSourceDoesNotBindIntoThirdPartyTypes` | | Advisor not registered in `CamelAutoConfiguration` | `nonEnumerableSourceDoesNotBindIntoThirdPartyTypes` | | Direct-value check removed | `nonEnumerableSourceStillBindsConfiguredOptions` | | `org.apache.camel.*` types treated as third-party | `nonEnumerableSourceStillBindsConfiguredOptions` | | `PRESENT` check removed | `propertiesBelowThirdPartyTypesAreStillBound` | Note: the converter is a named class, not a lambda. With `CamelAutoConfiguration` loaded, Camel's `TypeConversionConfiguration` picks up every `Converter` bean and can't work out a lambda's generic types. Please don't add the salesforce reproducers. They cover the same mechanism as the core test, so they'd only duplicate it. <details><summary>Patch (on top of dfc0fa33de)</summary> ```diff diff --git a/core/camel-spring-boot/src/test/java/com/example/springboot/ThirdPartySettings.java b/core/camel-spring-boot/src/test/java/com/example/springboot/ThirdPartySettings.java new file mode 100644 index 00000000000..f90e7fa9e67 --- /dev/null +++ b/core/camel-spring-boot/src/test/java/com/example/springboot/ThirdPartySettings.java @@ -0,0 +1,34 @@ +/* + * 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 com.example.springboot; + +/** + * Stands in for a third party JavaBean held by an option of a Camel configuration class. Deliberately outside the + * org.apache.camel packages. + */ +public class ThirdPartySettings { + + private String name; + + public String getName() { + return name; + } + + public void setName(String name) { + this.name = name; + } +} diff --git a/core/camel-spring-boot/src/test/java/org/apache/camel/spring/boot/CamelConfigurationPropertiesBindHandlerAdvisorTest.java b/core/camel-spring-boot/src/test/java/org/apache/camel/spring/boot/CamelConfigurationPropertiesBindHandlerAdvisorTest.java index d1a8d8ae74b..b0499cc1f5e 100644 --- a/core/camel-spring-boot/src/test/java/org/apache/camel/spring/boot/CamelConfigurationPropertiesBindHandlerAdvisorTest.java +++ b/core/camel-spring-boot/src/test/java/org/apache/camel/spring/boot/CamelConfigurationPropertiesBindHandlerAdvisorTest.java @@ -18,8 +18,10 @@ package org.apache.camel.spring.boot; import java.util.Map; +import com.example.springboot.ThirdPartySettings; import com.fasterxml.jackson.databind.ObjectMapper; import org.junit.jupiter.api.Test; +import org.springframework.boot.autoconfigure.AutoConfigurations; import org.springframework.boot.context.properties.ConfigurationProperties; import org.springframework.boot.context.properties.ConfigurationPropertiesBinding; import org.springframework.boot.context.properties.EnableConfigurationProperties; @@ -30,7 +32,6 @@ import org.springframework.core.convert.converter.Converter; import org.springframework.core.env.PropertySource; import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertSame; @@ -39,21 +40,14 @@ public class CamelConfigurationPropertiesBindHandlerAdvisorTest { private static final ObjectMapper MY_MAPPER = new ObjectMapper(); private final ApplicationContextRunner runner = new ApplicationContextRunner() + .withConfiguration(AutoConfigurations.of(CamelAutoConfiguration.class)) .withUserConfiguration(DummyConfiguration.class); - @Test - public void nonEnumerableSourceFailsWithoutAdvisor() { - runner.withInitializer(ctx -> ctx.getEnvironment().getPropertySources().addLast(nonEnumerable(Map.of()))) - .run(ctx -> { - // Spring Boot binds into the internals of the ObjectMapper, in case the source has a property there - assertNotNull(ctx.getStartupFailure()); - }); - } - @Test public void nonEnumerableSourceDoesNotBindIntoThirdPartyTypes() { - runner.withUserConfiguration(AdvisorConfiguration.class) - .withInitializer(ctx -> ctx.getEnvironment().getPropertySources().addLast(nonEnumerable(Map.of()))) + // without the advisor Spring Boot binds into the internals of the ObjectMapper, in case the source has a + // property there, and fails + runner.withInitializer(ctx -> ctx.getEnvironment().getPropertySources().addLast(nonEnumerable(Map.of()))) .run(ctx -> { assertNull(ctx.getStartupFailure()); DummyComponentConfiguration config = ctx.getBean(DummyComponentConfiguration.class); @@ -68,8 +62,7 @@ public class CamelConfigurationPropertiesBindHandlerAdvisorTest { "camel.component.dummy.object-mapper", "myMapper", "camel.component.dummy.config.name", "foo", "camel.component.dummy.config.object-mapper", "myMapper"); - runner.withUserConfiguration(AdvisorConfiguration.class) - .withInitializer(ctx -> ctx.getEnvironment().getPropertySources().addLast(nonEnumerable(values))) + runner.withInitializer(ctx -> ctx.getEnvironment().getPropertySources().addLast(nonEnumerable(values))) .run(ctx -> { assertNull(ctx.getStartupFailure()); DummyComponentConfiguration config = ctx.getBean(DummyComponentConfiguration.class); @@ -80,15 +73,11 @@ public class CamelConfigurationPropertiesBindHandlerAdvisorTest { } @Test - public void enumerableSourceBindsNestedOptions() { - runner.withUserConfiguration(AdvisorConfiguration.class) - .withPropertyValues("camel.component.dummy.config.name=foo", - "camel.component.dummy.config.object-mapper=myMapper") + public void propertiesBelowThirdPartyTypesAreStillBound() { + runner.withPropertyValues("camel.component.dummy.settings.name=foo") .run(ctx -> { assertNull(ctx.getStartupFailure()); - DummyComponentConfiguration config = ctx.getBean(DummyComponentConfiguration.class); - assertEquals("foo", config.getConfig().getName()); - assertSame(MY_MAPPER, config.getConfig().getObjectMapper()); + assertEquals("foo", ctx.getBean(DummyComponentConfiguration.class).getSettings().getName()); }); } @@ -106,22 +95,22 @@ public class CamelConfigurationPropertiesBindHandlerAdvisorTest { static class DummyConfiguration { @Bean @ConfigurationPropertiesBinding - static Converter<String, ObjectMapper> objectMapperConverter() { - return source -> "myMapper".equals(source) ? MY_MAPPER : null; + static ObjectMapperConverter objectMapperConverter() { + return new ObjectMapperConverter(); } } - @Configuration - static class AdvisorConfiguration { - @Bean - static CamelConfigurationPropertiesBindHandlerAdvisor advisor() { - return new CamelConfigurationPropertiesBindHandlerAdvisor(); + static class ObjectMapperConverter implements Converter<String, ObjectMapper> { + @Override + public ObjectMapper convert(String source) { + return "myMapper".equals(source) ? MY_MAPPER : null; } } @ConfigurationProperties(prefix = "camel.component.dummy") public static class DummyComponentConfiguration { private ObjectMapper objectMapper; + private ThirdPartySettings settings; private DummyEndpointConfig config = new DummyEndpointConfig(); public ObjectMapper getObjectMapper() { @@ -132,6 +121,14 @@ public class CamelConfigurationPropertiesBindHandlerAdvisorTest { this.objectMapper = objectMapper; } + public ThirdPartySettings getSettings() { + return settings; + } + + public void setSettings(ThirdPartySettings settings) { + this.settings = settings; + } + public DummyEndpointConfig getConfig() { return config; } ``` </details> _Claude Code on behalf of Croway_ -- 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]
