Copilot commented on code in PR #15875:
URL: https://github.com/apache/dubbo/pull/15875#discussion_r2730160490
##########
dubbo-config/dubbo-config-spring/src/main/java/org/apache/dubbo/config/spring/beans/factory/annotation/ServiceAnnotationPostProcessor.java:
##########
@@ -662,6 +670,48 @@ private void registerServiceBeanDefinition(
}
}
+ private boolean shouldSkipDueToConditionalOnMissingBean(
+ String refServiceBeanName, AnnotatedBeanDefinition beanDefinition)
{
+ MethodMetadata factoryMethod =
SpringCompatUtils.getFactoryMethodMetadata(beanDefinition);
+ if (factoryMethod == null) {
+ return false;
+ }
+
+ Map<String, Object> conditionalAttrs =
factoryMethod.getAnnotationAttributes(
+
"org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean");
+
+ if (conditionalAttrs == null || conditionalAttrs.isEmpty()) {
+ return false;
+ }
+
+ Class<?>[] beanTypes = (Class<?>[]) conditionalAttrs.get("value");
+ if (beanTypes == null) {
Review Comment:
The code should handle the case where beanTypes array has a length of 0.
Currently, if beanTypes is an empty array (not null), the method will return
false without checking the "type" attribute. The null check on line 688 should
also include a length check: `if (beanTypes == null || beanTypes.length == 0)`
to properly handle empty arrays from the annotation.
```suggestion
if (beanTypes == null || beanTypes.length == 0) {
```
##########
dubbo-config/dubbo-config-spring/src/main/java/org/apache/dubbo/config/spring/beans/factory/annotation/ServiceAnnotationPostProcessor.java:
##########
@@ -662,6 +670,48 @@ private void registerServiceBeanDefinition(
}
}
+ private boolean shouldSkipDueToConditionalOnMissingBean(
+ String refServiceBeanName, AnnotatedBeanDefinition beanDefinition)
{
+ MethodMetadata factoryMethod =
SpringCompatUtils.getFactoryMethodMetadata(beanDefinition);
+ if (factoryMethod == null) {
+ return false;
+ }
+
+ Map<String, Object> conditionalAttrs =
factoryMethod.getAnnotationAttributes(
+
"org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean");
+
+ if (conditionalAttrs == null || conditionalAttrs.isEmpty()) {
+ return false;
+ }
+
+ Class<?>[] beanTypes = (Class<?>[]) conditionalAttrs.get("value");
+ if (beanTypes == null) {
Review Comment:
The implementation only handles the "value" attribute of
@ConditionalOnMissingBean. The annotation also supports a "type" attribute
which should be checked. When "value" is empty, the implementation should fall
back to checking the "type" attribute. Consider adding:
```java
if (beanTypes == null || beanTypes.length == 0) {
beanTypes = (Class<?>[]) conditionalAttrs.get("type");
}
if (beanTypes == null || beanTypes.length == 0) {
return false;
}
```
This ensures compatibility with both "value" and "type" attributes as per
Spring Boot's @ConditionalOnMissingBean specification.
```suggestion
if (beanTypes == null || beanTypes.length == 0) {
beanTypes = (Class<?>[]) conditionalAttrs.get("type");
}
if (beanTypes == null || beanTypes.length == 0) {
```
##########
dubbo-config/dubbo-config-spring/src/test/java/org/apache/dubbo/config/spring/JavaConfigBeanTest.java:
##########
@@ -125,6 +126,36 @@ void testBean() {
}
}
+ @Test
+ void testConditionalOnMissingBeanForDubboService() {
+ AnnotationConfigApplicationContext context = new
AnnotationConfigApplicationContext(
+ ConditionalTestConfiguration.class,
+ ExistingServiceConfiguration.class,
+ ConditionalServiceConfiguration.class);
+ try {
+ Map<String, ServiceBean> serviceBeans =
context.getBeansOfType(ServiceBean.class);
+ Assertions.assertEquals(0, serviceBeans.size());
+
+ Map<String, DemoService> demoServices =
context.getBeansOfType(DemoService.class);
+ Assertions.assertEquals(1, demoServices.size());
+ Assertions.assertNotNull(demoServices.get("existingDemoService"));
+ } finally {
+ context.close();
+ }
Review Comment:
The test should verify whether the conditionalDemoService bean definition
exists when the condition is not met. Add an assertion to check if Spring
Boot's @ConditionalOnMissingBean is actually preventing the bean definition
registration or if the Dubbo code is providing the conditional logic. This will
clarify whether the implementation is adding safety on top of Spring Boot's
built-in conditional processing or if it's the primary mechanism. For example:
```java
// Verify that conditionalDemoService bean definition was not created
Assertions.assertFalse(context.containsBeanDefinition("conditionalDemoService"));
```
##########
dubbo-config/dubbo-config-spring/src/main/java/org/apache/dubbo/config/spring/beans/factory/annotation/ServiceAnnotationPostProcessor.java:
##########
@@ -662,6 +670,48 @@ private void registerServiceBeanDefinition(
}
}
+ private boolean shouldSkipDueToConditionalOnMissingBean(
+ String refServiceBeanName, AnnotatedBeanDefinition beanDefinition)
{
+ MethodMetadata factoryMethod =
SpringCompatUtils.getFactoryMethodMetadata(beanDefinition);
+ if (factoryMethod == null) {
+ return false;
+ }
+
+ Map<String, Object> conditionalAttrs =
factoryMethod.getAnnotationAttributes(
+
"org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean");
+
+ if (conditionalAttrs == null || conditionalAttrs.isEmpty()) {
+ return false;
+ }
+
+ Class<?>[] beanTypes = (Class<?>[]) conditionalAttrs.get("value");
+ if (beanTypes == null) {
+ return false;
+ }
+
+ for (Class<?> beanType : beanTypes) {
+ if (hasExistingBeanOfType(beanType, refServiceBeanName)) {
+ return true;
+ }
+ }
+
+ return false;
Review Comment:
The implementation only handles the "value" attribute of
@ConditionalOnMissingBean. The annotation supports additional attributes
including "name" (bean names to check), "annotation" (beans with specific
annotations), "ignored" (types to ignore), and "search" (search strategy). For
a complete implementation, at minimum the "name" attribute should be supported
as it's commonly used. Consider enhancing the implementation to handle these
additional attributes for full @ConditionalOnMissingBean compatibility.
##########
dubbo-config/dubbo-config-spring/src/test/java/org/apache/dubbo/config/spring/JavaConfigBeanTest.java:
##########
@@ -125,6 +126,36 @@ void testBean() {
}
}
+ @Test
+ void testConditionalOnMissingBeanForDubboService() {
+ AnnotationConfigApplicationContext context = new
AnnotationConfigApplicationContext(
+ ConditionalTestConfiguration.class,
+ ExistingServiceConfiguration.class,
+ ConditionalServiceConfiguration.class);
+ try {
+ Map<String, ServiceBean> serviceBeans =
context.getBeansOfType(ServiceBean.class);
+ Assertions.assertEquals(0, serviceBeans.size());
+
+ Map<String, DemoService> demoServices =
context.getBeansOfType(DemoService.class);
+ Assertions.assertEquals(1, demoServices.size());
+ Assertions.assertNotNull(demoServices.get("existingDemoService"));
+ } finally {
+ context.close();
+ }
+ }
+
+ @Test
+ void testConditionalOnMissingBeanForDubboServiceWhenMissing() {
+ AnnotationConfigApplicationContext context = new
AnnotationConfigApplicationContext(
+ ConditionalTestConfiguration.class,
ConditionalServiceConfiguration.class);
+ try {
+ Map<String, ServiceBean> serviceBeans =
context.getBeansOfType(ServiceBean.class);
+ Assertions.assertEquals(1, serviceBeans.size());
+ } finally {
+ context.close();
+ }
Review Comment:
The test should also verify that the DemoService bean is created when the
condition is met (no existing bean). Add assertions to verify both the
ServiceBean and the actual DemoService bean exist:
```java
Map<String, DemoService> demoServices =
context.getBeansOfType(DemoService.class);
Assertions.assertEquals(1, demoServices.size());
Assertions.assertNotNull(demoServices.get("conditionalDemoService"));
```
This ensures the conditional bean is properly created when it should be.
--
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]
