Copilot commented on code in PR #15875:
URL: https://github.com/apache/dubbo/pull/15875#discussion_r2730844209
##########
dubbo-config/dubbo-config-spring/src/main/java/org/apache/dubbo/config/spring/beans/factory/annotation/ServiceAnnotationPostProcessor.java:
##########
@@ -662,6 +670,117 @@ 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;
+ }
+
+ String[] beanNames = (String[]) conditionalAttrs.get("name");
+ if (beanNames != null) {
+ for (String beanName : beanNames) {
+ if (hasExistingBeanName(beanName, refServiceBeanName)) {
+ return true;
+ }
+ }
Review Comment:
The code checks if beanNames is not null but doesn't check if it's an empty
array before iterating. While iterating over an empty array is safe and won't
cause errors, it would be more explicit and consistent with the beanTypes
handling (line 697) to check for both null and empty array. Consider adding a
length check: `if (beanNames != null && beanNames.length > 0)` for consistency
and clarity.
##########
dubbo-config/dubbo-config-spring/src/main/java/org/apache/dubbo/config/spring/beans/factory/annotation/ServiceAnnotationPostProcessor.java:
##########
@@ -662,6 +670,117 @@ 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;
+ }
+
+ String[] beanNames = (String[]) conditionalAttrs.get("name");
+ if (beanNames != null) {
+ for (String beanName : beanNames) {
+ if (hasExistingBeanName(beanName, refServiceBeanName)) {
+ return true;
+ }
+ }
+ }
+
+ Class<?>[] beanTypes = (Class<?>[]) conditionalAttrs.get("value");
+ if (beanTypes == null || beanTypes.length == 0) {
+ Object typeAttr = conditionalAttrs.get("type");
+ if (typeAttr instanceof Class[]) {
+ beanTypes = (Class<?>[]) typeAttr;
+ } else if (typeAttr instanceof String[]) {
+ String[] typeNames = (String[]) typeAttr;
+ List<Class<?>> resolvedTypes = new
ArrayList<>(typeNames.length);
+ for (String typeName : typeNames) {
+ if (StringUtils.isEmpty(typeName)) {
+ continue;
+ }
+ String resolvedName = typeName;
+ if (environment != null) {
+ resolvedName =
environment.resolvePlaceholders(typeName);
+ }
+ if (!ClassUtils.isPresent(resolvedName, classLoader)) {
+ continue;
+ }
+ resolvedTypes.add(resolveClassName(resolvedName,
classLoader));
Review Comment:
The call to resolveClassName at line 715 could throw
IllegalArgumentException or LinkageError if the class cannot be loaded, even
though ClassUtils.isPresent checked for the class presence. This can happen due
to ClassLoader issues or race conditions. Consider wrapping this call in a
try-catch block to handle potential exceptions gracefully and continue
processing other type names instead of failing the entire operation.
```suggestion
try {
resolvedTypes.add(resolveClassName(resolvedName,
classLoader));
} catch (IllegalArgumentException | LinkageError ex) {
// Skip this type if it cannot be resolved at this
point
}
```
##########
dubbo-config/dubbo-config-spring/src/test/java/org/apache/dubbo/config/spring/JavaConfigBeanTest.java:
##########
@@ -125,6 +126,44 @@ void testBean() {
}
}
+ @Test
+ void testConditionalOnMissingBeanForDubboService() {
+ AnnotationConfigApplicationContext context = new
AnnotationConfigApplicationContext(
+ ConditionalTestConfiguration.class,
+ ExistingServiceConfiguration.class,
+ ConditionalServiceConfiguration.class);
+ try {
+
Assertions.assertFalse(context.containsBeanDefinition("conditionalDemoService"));
+
+ 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 {
+
Assertions.assertTrue(context.containsBeanDefinition("conditionalDemoService"));
+
+ Map<String, ServiceBean> serviceBeans =
context.getBeansOfType(ServiceBean.class);
+ Assertions.assertEquals(1, serviceBeans.size());
+
+ Map<String, DemoService> demoServices =
context.getBeansOfType(DemoService.class);
+ Assertions.assertEquals(1, demoServices.size());
+
Assertions.assertNotNull(demoServices.get("conditionalDemoService"));
+ } finally {
+ context.close();
+ }
+ }
Review Comment:
The test coverage is missing several important edge cases for the
@ConditionalOnMissingBean feature:
1. Testing @ConditionalOnMissingBean without any parameters (should default
to return type)
2. Testing with the 'name' attribute to check bean by name
3. Testing with the 'type' attribute using string class names
4. Testing behavior when multiple beans of the target type exist
5. Testing interaction with bean aliases
Consider adding additional test methods to cover these scenarios to ensure
the implementation handles all supported @ConditionalOnMissingBean
configurations correctly.
##########
dubbo-config/dubbo-config-spring/src/main/java/org/apache/dubbo/config/spring/beans/factory/annotation/ServiceAnnotationPostProcessor.java:
##########
@@ -662,6 +670,117 @@ 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;
+ }
+
+ String[] beanNames = (String[]) conditionalAttrs.get("name");
+ if (beanNames != null) {
+ for (String beanName : beanNames) {
+ if (hasExistingBeanName(beanName, refServiceBeanName)) {
+ return true;
+ }
+ }
+ }
+
+ Class<?>[] beanTypes = (Class<?>[]) conditionalAttrs.get("value");
+ if (beanTypes == null || beanTypes.length == 0) {
+ Object typeAttr = conditionalAttrs.get("type");
+ if (typeAttr instanceof Class[]) {
+ beanTypes = (Class<?>[]) typeAttr;
+ } else if (typeAttr instanceof String[]) {
+ String[] typeNames = (String[]) typeAttr;
+ List<Class<?>> resolvedTypes = new
ArrayList<>(typeNames.length);
+ for (String typeName : typeNames) {
+ if (StringUtils.isEmpty(typeName)) {
+ continue;
+ }
+ String resolvedName = typeName;
+ if (environment != null) {
+ resolvedName =
environment.resolvePlaceholders(typeName);
+ }
+ if (!ClassUtils.isPresent(resolvedName, classLoader)) {
+ continue;
+ }
+ resolvedTypes.add(resolveClassName(resolvedName,
classLoader));
+ }
+ if (!resolvedTypes.isEmpty()) {
+ beanTypes = resolvedTypes.toArray(new Class<?>[0]);
+ }
+ }
+ }
+ if (beanTypes == null || beanTypes.length == 0) {
+ return false;
+ }
+
+ for (Class<?> beanType : beanTypes) {
+ if (hasExistingBeanOfType(beanType, refServiceBeanName)) {
Review Comment:
The implementation doesn't handle the default behavior of
@ConditionalOnMissingBean when no value, type, or name is specified. According
to Spring Boot's @ConditionalOnMissingBean contract, when no parameters are
provided, it should default to the return type of the annotated method.
Currently, when no bean types are found (lines 722-723), the method returns
false, which means the bean will always be registered even if a bean of the
same return type already exists.
To fix this, you should check if no attributes were specified and default to
the return type of the factory method. You can use
SpringCompatUtils.getFactoryMethodReturnType to get the return type and check
if a bean of that type already exists.
```suggestion
if (beanTypes != null && beanTypes.length > 0) {
for (Class<?> beanType : beanTypes) {
if (hasExistingBeanOfType(beanType, refServiceBeanName)) {
return true;
}
}
} else {
// No explicit name, value, or type specified: default to the
factory method's return type
Class<?> returnType =
SpringCompatUtils.getFactoryMethodReturnType(beanDefinition);
if (returnType != null && hasExistingBeanOfType(returnType,
refServiceBeanName)) {
```
##########
dubbo-config/dubbo-config-spring/src/main/java/org/apache/dubbo/config/spring/beans/factory/annotation/ServiceAnnotationPostProcessor.java:
##########
@@ -618,6 +618,14 @@ private void processAnnotatedBeanDefinition(
AnnotatedBeanDefinition refServiceBeanDefinition,
Map<String, Object> attributes) {
+ if (shouldSkipDueToConditionalOnMissingBean(refServiceBeanName,
refServiceBeanDefinition)) {
+ if (logger.isDebugEnabled()) {
+ logger.debug("Skip registering ServiceBean for bean [" +
refServiceBeanName
+ + "] due to @ConditionalOnMissingBean condition not
satisfied");
Review Comment:
The debug message at line 623-624 says "condition not satisfied" which is
somewhat misleading. When @ConditionalOnMissingBean condition is "not
satisfied", it means a bean of the specified type already exists, so the new
bean should not be registered. However, the phrasing "condition not satisfied"
might suggest something went wrong. Consider rewording it to be more clear,
such as "bean already exists" or "condition evaluated to false - bean exists".
For example: "Skip registering ServiceBean for bean [" + refServiceBeanName +
"] due to @ConditionalOnMissingBean - bean of required type already exists"
```suggestion
+ "] due to @ConditionalOnMissingBean - bean of
required type already exists");
```
--
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]
