-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support @SofaService to annotated on method and refactor related code. #288
Support @SofaService to annotated on method and refactor related code. #288
Conversation
QilongZhang
commented
Nov 24, 2018
•
edited
Loading
edited
- Support @SoafService to annotated on method. Support @SofaService in bean method and @SofaReference in parameter #277
- Support @SofaReference to annotated on parameter. Support @SofaService in bean method and @SofaReference in parameter #277
- Refactor way of publishing SofaService to fix bug BeanNameAutoProxyCreator not work with @SofaService #276
- Refactor way of registering SofaRuntimeContext as a singleton bean to fix bug SofaService can NOT be resolved among multi Bizs which contain Spring Cloud components. #283
Codecov Report
@@ Coverage Diff @@
## master #288 +/- ##
==========================================
+ Coverage 60.19% 62.83% +2.64%
==========================================
Files 73 77 +4
Lines 1781 1975 +194
Branches 203 230 +27
==========================================
+ Hits 1072 1241 +169
- Misses 557 569 +12
- Partials 152 165 +13
Continue to review full report at Codecov.
|
@QilongZhang There is a huge decline on the code cov, please keep us noticed when it get resolved. |
To support to this feature, it would produce an incompatible point. In previous version, publishing multi sofa service with same interface type and unique-id would only log warning message. However, now it would lead a fatal exception if spring bean override strategy is not configured as TRUE. |
After some discussion, we decide to fix this incompatible point and log some error message to notice users
|
GenericApplicationContext ctx = sofaModuleProperties.isPublishEventToParent() ? new GenericApplicationContext( | ||
beanFactory) : new SofaModuleApplicationContext(beanFactory); | ||
SofaRuntimeApplicationListener.initApplicationContext(ctx); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这里直接调用 sofa-runtime 的方法是不是有点奇怪,isle 不应该强耦合 sofa-runtime 吧?在我的理解,如果 sofa-runtime 需要提供一个钩子供 isle 回调,那么其他 starter 是不是也可能有这个需求,是不是能做成更通用的方案呢?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@caojie09 SOFAIsle 和 sofa-runtime 一直是强耦合的. 这里的编码也不是很友好,考虑换一种方式,采用类似 SofaModuleBeanFactoryPostProcessor 的白名单形式也行。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@caojie09 @QilongZhang As discussed offline, I prefer that we should add listener on isle(and mvc) to make the code more flexible.
* @author xuanbei 18/5/9 | ||
*/ | ||
public class ServiceAnnotationBeanPostProcessor implements BeanPostProcessor, Ordered, | ||
ApplicationContextAware { | ||
public class ServiceAnnotationBeanPostProcessor implements BeanPostProcessor, Ordered { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we rename this class to make it more clear?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, rename it to ReferenceAnnotationBeanPostProcessor
if (StringUtils.isEmpty(uniqueId)) { | ||
return SERVICE_BEAN_NAME_PREFIX + interfaceName; | ||
} | ||
return SERVICE_BEAN_NAME_PREFIX + interfaceName + ":" + uniqueId; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicate logics here, should directly call following generateSofaServiceBeanName
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not actually, parameter type is different
SofaReference sofaReference, | ||
Class<?> parameterType, | ||
ConfigurableListableBeanFactory beanFactory) { | ||
Assert.isTrue("jvm".equals(sofaReference.binding().bindingType()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use JvmBinding.JVM_BINDING_TYPE.getType()
here instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And why only support "jvm" here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm not clear whether it is suitable to support RPC, maybe it would be covered according to actually demand.
|
||
/** | ||
* @author qilong.zql | ||
* @since 2.3.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since 2.3.0 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix.
returnType = ClassUtils.forName(methodMetadata.getReturnTypeName(), null); | ||
declaringClass = ClassUtils.forName(methodMetadata.getDeclaringClassName(), null); | ||
for (Method m : declaringClass.getDeclaredMethods()) { | ||
if (!m.getName().equals(beanId)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when use @Bean("customBeanName")
, beadId not equals methodName.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix.