Skip to content
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 placeholder in SofaService and SofaReference annotation. #264

Merged
merged 3 commits into from
Nov 15, 2018

Conversation

QilongZhang
Copy link
Contributor

@QilongZhang
Copy link
Contributor Author

@chpengzh @JervyShi

@coveralls
Copy link

coveralls commented Nov 6, 2018

Pull Request Test Coverage Report for Build 603

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 51 unchanged lines in 15 files lost coverage.
  • Overall coverage remained the same at 71.99%

Files with Coverage Reduction New Missed Lines %
isle-sofa-boot-starter/src/main/java/com/alipay/sofa/isle/spring/listener/SofaModuleContextRefreshedListener.java 1 72.41%
runtime-sofa-boot-starter/src/main/java/com/alipay/sofa/runtime/integration/activator/SofaRuntimeActivator.java 1 0.0%
infra-sofa-boot-starter/src/main/java/com/alipay/sofa/infra/endpoint/SofaBootVersionEndpoint.java 1 85.42%
runtime-sofa-boot-starter/src/main/java/com/alipay/sofa/runtime/spring/callback/CloseApplicationContextCallBack.java 1 75.0%
runtime-sofa-boot-starter/src/main/java/com/alipay/sofa/runtime/spring/factory/ServiceFactoryBean.java 2 66.67%
runtime-sofa-boot-starter/src/main/java/com/alipay/sofa/runtime/spring/health/MultiApplicationHealthIndicator.java 2 56.25%
test-sofa-boot-starter/src/main/java/com/alipay/sofa/test/runner/SofaJUnit4Runner.java 3 56.67%
infra-sofa-boot-starter/src/main/java/com/alipay/sofa/infra/config/spring/namespace/handler/SofaBootNamespaceHandler.java 3 0.0%
runtime-sofa-boot-starter/src/main/java/com/alipay/sofa/runtime/service/helper/ReferenceRegisterHelper.java 3 72.73%
runtime-sofa-boot-starter/src/main/java/com/alipay/sofa/runtime/spring/factory/AbstractContractFactoryBean.java 4 52.38%
Totals Coverage Status
Change from base Build 597: 0.0%
Covered Lines: 1501
Relevant Lines: 2085

💛 - Coveralls

@chpengzh
Copy link
Member

chpengzh commented Nov 6, 2018

这个竟然是用代理去实现的,又学到了新姿势!一直以为注解类是final不能代理,直到看到了基于annotation类内聚做代理的,艺术艺术。

Copy link
Member

@JervyShi JervyShi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well done!

SofaService sofaServiceAnnotation = beanClass.getAnnotation(SofaService.class);
AnnotationWrapperBuilder<SofaService> builder = AnnotationWrapperBuilder.wrap(
beanClass.getAnnotation(SofaService.class)).withBinder(binder);
SofaService sofaServiceAnnotation = builder.build();

if (sofaServiceAnnotation == null) {
return;
Copy link
Member

@straybirdzls straybirdzls Nov 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I this we should first check whether sofaServiceAnnotation is null ,then wrap it , otherwise there will be many temp AnnotationWrapperBuilder objects since ServiceAnnotationBeanPostProcessor is a BeanPostProcessor which will work on many beans and only a very small part of them are annotated with @SofaService. At the same time, we can assert delegate not null on AnnotationWrapperBuilder.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good advice.

AnnotationWrapperBuilder<SofaReference> builder = AnnotationWrapperBuilder.wrap(
field.getAnnotation(SofaReference.class)).withBinder(binder);
SofaReference sofaReferenceAnnotation = builder.build();

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above suggested.

* @since 2.5.2
*/
public interface PlaceHolderBinder {
String bind(String string);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add comment for this interface and method. And should we provide a default Implement? I see a lot of duplicate implements below.

@straybirdzls straybirdzls merged commit 3c1321c into sofastack:2.5.x Nov 15, 2018
QilongZhang added a commit to QilongZhang/sofa-boot that referenced this pull request Nov 22, 2018
…astack#264)

* support placeholder in SofaService and SofaReference annotation
straybirdzls pushed a commit that referenced this pull request Nov 23, 2018
… (#282)

* support placeholder in SofaService and SofaReference annotation
@QilongZhang QilongZhang deleted the annotation_wrapper branch December 26, 2018 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants