feat: Support @ConditionalOnMissingBean for @DubboService#15875
feat: Support @ConditionalOnMissingBean for @DubboService#15875heliang666s wants to merge 5 commits into3.3from
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 3.3 #15875 +/- ##
=========================================
Coverage 60.74% 60.74%
- Complexity 11754 11759 +5
=========================================
Files 1949 1949
Lines 88898 88965 +67
Branches 13407 13432 +25
=========================================
+ Hits 53998 54046 +48
- Misses 29337 29355 +18
- Partials 5563 5564 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
add test cases to check its effectiveness? |
8dafa89 to
41922ef
Compare
There was a problem hiding this comment.
Pull request overview
This pull request adds support for Spring Boot's @ConditionalOnMissingBean annotation when used with @DubboService on Java config @bean methods. This addresses issue #15814, enabling developers to conditionally register Dubbo services based on the presence or absence of other beans in the application context.
Changes:
- Added logic in ServiceAnnotationPostProcessor to detect and respect @ConditionalOnMissingBean annotations
- Implemented conditional checking that prevents ServiceBean registration when matching beans exist
- Added comprehensive test cases to verify the conditional behavior works correctly
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| ServiceAnnotationPostProcessor.java | Added shouldSkipDueToConditionalOnMissingBean() and hasExistingBeanOfType() methods to check for @ConditionalOnMissingBean and skip ServiceBean registration when the condition is not met |
| JavaConfigBeanTest.java | Added test cases testConditionalOnMissingBeanForDubboService() and testConditionalOnMissingBeanForDubboServiceWhenMissing() along with supporting configuration classes to verify the feature works correctly |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
.../org/apache/dubbo/config/spring/beans/factory/annotation/ServiceAnnotationPostProcessor.java
Outdated
Show resolved
Hide resolved
...fig/dubbo-config-spring/src/test/java/org/apache/dubbo/config/spring/JavaConfigBeanTest.java
Show resolved
Hide resolved
.../org/apache/dubbo/config/spring/beans/factory/annotation/ServiceAnnotationPostProcessor.java
Outdated
Show resolved
Hide resolved
...fig/dubbo-config-spring/src/test/java/org/apache/dubbo/config/spring/JavaConfigBeanTest.java
Show resolved
Hide resolved
.../org/apache/dubbo/config/spring/beans/factory/annotation/ServiceAnnotationPostProcessor.java
Show resolved
Hide resolved
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (beanNames != null) { | ||
| for (String beanName : beanNames) { | ||
| if (hasExistingBeanName(beanName, refServiceBeanName)) { | ||
| return true; | ||
| } | ||
| } |
There was a problem hiding this 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.
| if (beanTypes == null || beanTypes.length == 0) { | ||
| return false; | ||
| } | ||
|
|
||
| for (Class<?> beanType : beanTypes) { | ||
| if (hasExistingBeanOfType(beanType, refServiceBeanName)) { |
There was a problem hiding this 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.
| if (beanTypes == null || beanTypes.length == 0) { | |
| return false; | |
| } | |
| for (Class<?> beanType : beanTypes) { | |
| if (hasExistingBeanOfType(beanType, refServiceBeanName)) { | |
| 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)) { |
| if (!ClassUtils.isPresent(resolvedName, classLoader)) { | ||
| continue; | ||
| } | ||
| resolvedTypes.add(resolveClassName(resolvedName, classLoader)); |
There was a problem hiding this 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.
| resolvedTypes.add(resolveClassName(resolvedName, classLoader)); | |
| try { | |
| resolvedTypes.add(resolveClassName(resolvedName, classLoader)); | |
| } catch (IllegalArgumentException | LinkageError ex) { | |
| // Skip this type if it cannot be resolved at this point | |
| } |
| @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(); | ||
| } | ||
| } |
There was a problem hiding this comment.
The test coverage is missing several important edge cases for the @ConditionalOnMissingBean feature:
- Testing @ConditionalOnMissingBean without any parameters (should default to return type)
- Testing with the 'name' attribute to check bean by name
- Testing with the 'type' attribute using string class names
- Testing behavior when multiple beans of the target type exist
- Testing interaction with bean aliases
Consider adding additional test methods to cover these scenarios to ensure the implementation handles all supported @ConditionalOnMissingBean configurations correctly.
| if (shouldSkipDueToConditionalOnMissingBean(refServiceBeanName, refServiceBeanDefinition)) { | ||
| if (logger.isDebugEnabled()) { | ||
| logger.debug("Skip registering ServiceBean for bean [" + refServiceBeanName | ||
| + "] due to @ConditionalOnMissingBean condition not satisfied"); |
There was a problem hiding this 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"
| + "] due to @ConditionalOnMissingBean condition not satisfied"); | |
| + "] due to @ConditionalOnMissingBean - bean of required type already exists"); |
What is the purpose of the change?
Support @ConditionalOnMissingBean for @DubboService #15814