Copilot commented on code in PR #16005: URL: https://github.com/apache/dubbo/pull/16005#discussion_r2694519897
########## dubbo-spring-boot-project/dubbo-spring-boot-actuator-autoconfigure/src/main/resources/META-INF/spring.factories: ########## @@ -1,5 +1,6 @@ org.springframework.boot.autoconfigure.EnableAutoConfiguration=\ org.apache.dubbo.spring.boot.actuate.autoconfigure.DubboHealthIndicatorAutoConfiguration,\ org.apache.dubbo.spring.boot.actuate.autoconfigure.DubboEndpointMetadataAutoConfiguration,\ +org.apache.dubbo.spring.boot.actuate.autoconfigure.DubboExtensionEndpointAutoConfiguration,\ Review Comment: The `DubboExtensionEndpointAutoConfiguration` is already present in the `.imports` file at `dubbo-spring-boot-actuator-autoconfigure/src/main/resources/META-INF/spring/org.springframework.boot.autoconfigure.AutoConfiguration.imports` (line 3). Adding it to `spring.factories` creates duplicate registration for Spring Boot 3.x environments where both files are processed. Consider if this duplication is intentional for backwards compatibility with Spring Boot 2.x, or if the entry already existed in one of these files before this PR. ```suggestion ``` ########## dubbo-spring-boot-project/dubbo-spring-boot-autoconfigure/src/test/java/org/apache/dubbo/spring/boot/autoconfigure/SpringBoot3CompatibilityTest.java: ########## @@ -0,0 +1,153 @@ +/* + * 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.dubbo.spring.boot.autoconfigure; + +import org.apache.dubbo.config.spring.beans.factory.annotation.ReferenceAnnotationBeanPostProcessor; +import org.apache.dubbo.config.spring.beans.factory.annotation.ServiceAnnotationPostProcessor; +import org.apache.dubbo.config.spring.util.DubboBeanUtils; +import org.apache.dubbo.spring.boot.env.DubboDefaultPropertiesEnvironmentPostProcessor; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.springframework.beans.factory.ObjectProvider; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.SpringBootVersion; +import org.springframework.boot.autoconfigure.EnableAutoConfiguration; +import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.context.ApplicationContext; +import org.springframework.context.annotation.PropertySource; +import org.springframework.core.env.Environment; +import org.springframework.test.context.junit.jupiter.SpringExtension; + +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.junit.jupiter.api.Assertions.assertFalse; + +/** + * Spring Boot Compatibility Test + * + * This test validates that Dubbo's Spring Boot integration works correctly across + * Spring Boot versions (2.x and 3.x) and is prepared for Spring Cloud 2025.0.0 compatibility. + * + * Key areas tested: + * - AutoConfiguration loading (works with both spring.factories and .imports files) + * - EnvironmentPostProcessor registration + * - ApplicationContextInitializer registration + * - Bean post processors for Dubbo annotations + * - Version detection conditions + * + * @since 3.3.7 + * @see DubboAutoConfiguration + * @see DubboDefaultPropertiesEnvironmentPostProcessor + */ +@ExtendWith(SpringExtension.class) +@SpringBootTest( + classes = {SpringBoot3CompatibilityTest.class}, + properties = { + "dubbo.scan.base-packages=org.apache.dubbo.spring.boot.autoconfigure", + "spring.application.name=dubbo-spring-boot-compat-test" + }) +@EnableAutoConfiguration +@PropertySource(value = "classpath:/META-INF/dubbo.properties") +class SpringBoot3CompatibilityTest { + + @Autowired + private ObjectProvider<ServiceAnnotationPostProcessor> serviceAnnotationPostProcessor; + + @Autowired + private ApplicationContext applicationContext; + + @Autowired + private Environment environment; + + /** + * Verifies that core Dubbo beans are properly loaded via Spring Boot + * auto-configuration mechanism (works for both 2.x and 3.x). + */ + @Test + void testDubboCoreBeansLoaded() { + assertNotNull(serviceAnnotationPostProcessor, + "ServiceAnnotationPostProcessor should be loaded via auto-configuration"); + assertNotNull(serviceAnnotationPostProcessor.getIfAvailable(), + "ServiceAnnotationPostProcessor instance should be available"); + + ReferenceAnnotationBeanPostProcessor referenceAnnotationBeanPostProcessor = + DubboBeanUtils.getReferenceAnnotationBeanPostProcessor(applicationContext); + assertNotNull(referenceAnnotationBeanPostProcessor, + "ReferenceAnnotationBeanPostProcessor should be registered"); + } + + /** + * Verifies that Spring Boot version detection works correctly and doesn't throw exceptions. + * This is critical for conditional configuration in both Spring Boot 2.x and 3.x. + */ + @Test + void testSpringBootVersionDetection() { + String version = SpringBootVersion.getVersion(); + assertNotNull(version, "Spring Boot version should be detectable"); + assertFalse(version.isEmpty(), "Spring Boot version should not be empty"); + + // Verify SpringBoot3Condition doesn't throw exceptions + boolean isSpringBoot3 = SpringBoot3Condition.IS_SPRING_BOOT_3; + boolean isSpringBoot12 = SpringBoot12Condition.IS_SPRING_BOOT_12; + + // These should be mutually exclusive + assertTrue(isSpringBoot3 != isSpringBoot12, + "SpringBoot3Condition and SpringBoot12Condition should be mutually exclusive"); + + // Verify consistency with actual version + char majorVersion = version.charAt(0); + if (majorVersion >= '3') { + assertTrue(isSpringBoot3, "Should detect Spring Boot 3.x correctly"); + } else { + assertTrue(isSpringBoot12, "Should detect Spring Boot 2.x or earlier correctly"); Review Comment: This test contains the same potential issue that was fixed in `SpringBoot3Condition`. If `SpringBootVersion.getVersion()` returns null or empty string, calling `charAt(0)` will throw an exception. The test should use the same safe access pattern: check for null/empty before calling `charAt(0)`, or wrap in a try-catch block. ```suggestion // Verify consistency with actual version (guard against null/empty) if (version != null && !version.isEmpty()) { char majorVersion = version.charAt(0); if (majorVersion >= '3') { assertTrue(isSpringBoot3, "Should detect Spring Boot 3.x correctly"); } else { assertTrue(isSpringBoot12, "Should detect Spring Boot 2.x or earlier correctly"); } ``` ########## dubbo-spring-boot-project/dubbo-spring-boot-autoconfigure/src/main/java/org/apache/dubbo/spring/boot/autoconfigure/SpringBoot3Condition.java: ########## @@ -21,9 +21,44 @@ import org.springframework.context.annotation.ConditionContext; import org.springframework.core.type.AnnotatedTypeMetadata; +/** + * Condition that matches when running on Spring Boot 3.x or higher. + * + * <p>This condition is used to enable Spring Boot 3.x specific auto-configuration + * that requires Jakarta EE APIs (jakarta.servlet.*) instead of Java EE APIs (javax.servlet.*). + * + * <p>Compatible with Spring Boot 3.5.x and Spring Cloud 2025.0.0. + * + * @since 3.2.0 + */ public class SpringBoot3Condition implements Condition { - public static boolean IS_SPRING_BOOT_3 = SpringBootVersion.getVersion().charAt(0) >= '3'; + /** + * Cached result indicating if we're running on Spring Boot 3.x or higher. + * Uses safe version parsing to handle edge cases. + */ + public static final boolean IS_SPRING_BOOT_3 = isSpringBoot3OrHigher(); Review Comment: The field name `IS_SPRING_BOOT_3` suggests checking for Spring Boot 3 exactly, but the implementation checks for version 3 or higher (`>= '3'`). Consider renaming to `IS_SPRING_BOOT_3_OR_HIGHER` to accurately reflect the behavior, or update the documentation to clarify that this includes future versions. ########## dubbo-spring-boot-project/dubbo-spring-boot-autoconfigure/src/test/java/org/apache/dubbo/spring/boot/autoconfigure/SpringBoot3CompatibilityTest.java: ########## @@ -0,0 +1,153 @@ +/* + * 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.dubbo.spring.boot.autoconfigure; + +import org.apache.dubbo.config.spring.beans.factory.annotation.ReferenceAnnotationBeanPostProcessor; +import org.apache.dubbo.config.spring.beans.factory.annotation.ServiceAnnotationPostProcessor; +import org.apache.dubbo.config.spring.util.DubboBeanUtils; +import org.apache.dubbo.spring.boot.env.DubboDefaultPropertiesEnvironmentPostProcessor; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.springframework.beans.factory.ObjectProvider; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.SpringBootVersion; +import org.springframework.boot.autoconfigure.EnableAutoConfiguration; +import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.context.ApplicationContext; +import org.springframework.context.annotation.PropertySource; +import org.springframework.core.env.Environment; +import org.springframework.test.context.junit.jupiter.SpringExtension; + +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.junit.jupiter.api.Assertions.assertFalse; + +/** + * Spring Boot Compatibility Test + * + * This test validates that Dubbo's Spring Boot integration works correctly across + * Spring Boot versions (2.x and 3.x) and is prepared for Spring Cloud 2025.0.0 compatibility. + * + * Key areas tested: + * - AutoConfiguration loading (works with both spring.factories and .imports files) + * - EnvironmentPostProcessor registration + * - ApplicationContextInitializer registration + * - Bean post processors for Dubbo annotations + * - Version detection conditions + * + * @since 3.3.7 + * @see DubboAutoConfiguration + * @see DubboDefaultPropertiesEnvironmentPostProcessor + */ +@ExtendWith(SpringExtension.class) +@SpringBootTest( + classes = {SpringBoot3CompatibilityTest.class}, + properties = { + "dubbo.scan.base-packages=org.apache.dubbo.spring.boot.autoconfigure", + "spring.application.name=dubbo-spring-boot-compat-test" + }) +@EnableAutoConfiguration +@PropertySource(value = "classpath:/META-INF/dubbo.properties") +class SpringBoot3CompatibilityTest { + + @Autowired + private ObjectProvider<ServiceAnnotationPostProcessor> serviceAnnotationPostProcessor; + + @Autowired + private ApplicationContext applicationContext; + + @Autowired + private Environment environment; + + /** + * Verifies that core Dubbo beans are properly loaded via Spring Boot + * auto-configuration mechanism (works for both 2.x and 3.x). + */ + @Test + void testDubboCoreBeansLoaded() { + assertNotNull(serviceAnnotationPostProcessor, + "ServiceAnnotationPostProcessor should be loaded via auto-configuration"); + assertNotNull(serviceAnnotationPostProcessor.getIfAvailable(), + "ServiceAnnotationPostProcessor instance should be available"); + + ReferenceAnnotationBeanPostProcessor referenceAnnotationBeanPostProcessor = + DubboBeanUtils.getReferenceAnnotationBeanPostProcessor(applicationContext); + assertNotNull(referenceAnnotationBeanPostProcessor, + "ReferenceAnnotationBeanPostProcessor should be registered"); + } + + /** + * Verifies that Spring Boot version detection works correctly and doesn't throw exceptions. + * This is critical for conditional configuration in both Spring Boot 2.x and 3.x. + */ + @Test + void testSpringBootVersionDetection() { + String version = SpringBootVersion.getVersion(); + assertNotNull(version, "Spring Boot version should be detectable"); + assertFalse(version.isEmpty(), "Spring Boot version should not be empty"); + + // Verify SpringBoot3Condition doesn't throw exceptions + boolean isSpringBoot3 = SpringBoot3Condition.IS_SPRING_BOOT_3; + boolean isSpringBoot12 = SpringBoot12Condition.IS_SPRING_BOOT_12; + + // These should be mutually exclusive + assertTrue(isSpringBoot3 != isSpringBoot12, + "SpringBoot3Condition and SpringBoot12Condition should be mutually exclusive"); + + // Verify consistency with actual version + char majorVersion = version.charAt(0); + if (majorVersion >= '3') { + assertTrue(isSpringBoot3, "Should detect Spring Boot 3.x correctly"); + } else { + assertTrue(isSpringBoot12, "Should detect Spring Boot 2.x or earlier correctly"); + } + } + + /** + * Verifies that DubboDefaultPropertiesEnvironmentPostProcessor is working. + * This tests that the registration (via spring.factories or .imports) is functioning correctly. + */ + @Test + void testEnvironmentPostProcessorApplied() { + // The EnvironmentPostProcessor should have set dubbo.application.name + // from spring.application.name + String springAppName = environment.getProperty("spring.application.name"); + assertNotNull(springAppName, "Spring application name should be set"); + + // Dubbo application name should be derived if not explicitly set + String dubboAppName = environment.getProperty("dubbo.application.name"); + if (dubboAppName != null) { + assertNotNull(dubboAppName, + "Dubbo application name should be set by EnvironmentPostProcessor"); + } Review Comment: The null check `if (dubboAppName != null)` followed by `assertNotNull(dubboAppName, ...)` is redundant logic. If `dubboAppName` is not null, the assertion will always pass. This should either: (1) remove the if statement and make the assertion unconditional if the property is expected to always be set, or (2) make a meaningful assertion inside the if block, such as verifying it equals the spring application name. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
