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

[Dubbo-4693] fix EnableDubbo's alias problem (no effect though declared) #4895

Closed
wants to merge 2 commits into from

Conversation

jasonjoo2010
Copy link
Contributor

@jasonjoo2010 jasonjoo2010 commented Aug 20, 2019

What is the purpose of the change

fix the issued prompted in #4693

Brief changelog

Use AnnotatedElementUtils.findMergedAnnotation to make inherited annotations' @AliasFor take effects.

Verifying this change

@EnableDubbo(scanBasePackages = "") should bring the property to @EnableDubboConfig

Reference

https://docs.spring.io/spring/docs/4.2.0.RC3_to_4.2.0.RELEASE/Spring%20Framework%204.2.0.RELEASE/org/springframework/core/annotation/AliasFor.html

Similarly, AnnotatedElementUtils supports explicit meta-annotation attribute overrides when @AliasFor is used within an annotation hierarchy. 

Follow this checklist to help us incorporate your contribution quickly and easily:

  • Make sure there is a GITHUB_issue field for the change (usually before you start working on it). Trivial changes like typos do not require a GITHUB issue. Your pull request should address just this issue, without pulling in other changes - one PR resolves one issue.
  • Format the pull request title like [Dubbo-XXX] Fix UnknownException when host config not exist #XXX. Each commit in the pull request should have a meaningful subject line and body.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Write necessary unit-test to verify your logic correction, more mock a little better when cross module dependency exist. If the new feature or significant change is committed, please remember to add sample in dubbo samples project.
  • Run mvn clean install -DskipTests=false & mvn clean test-compile failsafe:integration-test to make sure unit-test and integration-test pass.
  • If this contribution is large, please follow the Software Donation Guide.

@jasonjoo2010 jasonjoo2010 changed the title fix EnableDubbo's alias problem (no effect though declared) [Dubbo-4693] fix EnableDubbo's alias problem (no effect though declared) Aug 20, 2019
@codecov-io
Copy link

codecov-io commented Aug 20, 2019

Codecov Report

Merging #4895 into master will decrease coverage by 0.06%.
The diff coverage is 73.33%.

Impacted file tree graph

@@             Coverage Diff             @@
##             master   #4895      +/-   ##
===========================================
- Coverage     61.17%   61.1%   -0.07%     
+ Complexity      424     423       -1     
===========================================
  Files           919     919              
  Lines         37487   37489       +2     
  Branches       5461    5446      -15     
===========================================
- Hits          22933   22909      -24     
- Misses        12049   12070      +21     
- Partials       2505    2510       +5
Impacted Files Coverage Δ Complexity Δ
.../annotation/DubboConfigConfigurationRegistrar.java 88.23% <71.42%> (-11.77%) 0 <0> (ø)
...ontext/annotation/DubboComponentScanRegistrar.java 83.33% <75%> (-8.67%) 0 <0> (ø)
...ache/dubbo/remoting/transport/AbstractChannel.java 75% <0%> (-12.5%) 0% <0%> (ø)
...pache/dubbo/remoting/transport/AbstractServer.java 53.75% <0%> (-3.75%) 0% <0%> (ø)
...he/dubbo/remoting/transport/netty/NettyServer.java 70.17% <0%> (-3.51%) 8% <0%> (-1%)
...c/main/java/org/apache/dubbo/rpc/RpcException.java 80% <0%> (-3.34%) 0% <0%> (ø)
...g/apache/dubbo/rpc/protocol/rest/RestProtocol.java 68.21% <0%> (-3.11%) 0% <0%> (ø)
.../org/apache/dubbo/qos/legacy/LogTelnetHandler.java 30.3% <0%> (-3.04%) 0% <0%> (ø)
.../dubbo/remoting/transport/netty4/NettyChannel.java 63.36% <0%> (-2.98%) 0% <0%> (ø)
...va/org/apache/dubbo/remoting/exchange/Request.java 84.09% <0%> (-2.28%) 0% <0%> (ø)
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a9746a0...5eb8f56. Read the comment docs.

Copy link
Member

@beiwei30 beiwei30 left a comment

Choose a reason for hiding this comment

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

@jasonjoo2010 would you mind to take a look at apache/dubbo-samples@7e25d4e? I cannot reproduce the issue.

@jasonjoo2010
Copy link
Contributor Author

jasonjoo2010 commented Aug 26, 2019

@jasonjoo2010 would you mind to take a look at apache/dubbo-samples@7e25d4e? I cannot reproduce the issue.

Hi, luo

Sorry for the latency.

I think problem will occur when using spring-boot to initialize the project.
See https://github.com/jasonjoo2010/dubbo-samples/tree/EnableDubbo for sample project dubbo-samples-annotation-springboot.

But surely there is a limitation for this PR. AnnotatedElementUtils.findMergedAnnotationAttributes will only available since spring 4.2. I don't know whether it's acceptable.

@jasonjoo2010
Copy link
Contributor Author

jasonjoo2010 commented Aug 26, 2019

@beiwei30

You can reproduce it by adding a breakpoint on DubboComponentScanRegistrar.getPackagesToScan (L102) before running AnnotationProviderBootstrap. Break at that point and check the variable basePackages and will find it is empty. It's different to the result in AnnotationConfigApplicationContext.

@jasonjoo2010
Copy link
Contributor Author

Update:

Recently I just found some ways which will cause the inheriting alias fail but didn't dig the root cause (It maybe not important because we can't make everyone not to do it like this way):

  1. Put a @ComponentScan in your bootable class XXX.class (It may be referenced as SpringApplication.run(XXXX.class, argv) )
  2. Put a @EnableDubbo in where you like (in the same class or a separated class)

Then the information will not be brought to referenced annotations. And @ComponentScan can be also introduced by introducing @SpringBootApplication.

It will also be reproduced by the pair of @configuration + @EnableAutoConfiguration.
Generally I think if the tag is injected by scanning but not imported manually the issue will occur.

So I think it's a kind of SpringBoot issue. Just like the strange flow of asynchronized servlet in SpringBoot compared SpringMVC in container(standalone tomcat)[1].

Maybe we can't unify them into the same processing flow but to make some changes adapting for the special environment.

You can make more tests in the repository provided in previous comment.

[1] alibaba/Sentinel#310

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@mercyblitz
Copy link
Contributor

Sorry, this PR will not be merged into, thus @org.apache.dubbo.config.spring.context.annotation.DubboComponentScan has to be compatible with Spring Framework 3.2 +, however, @AliasFor is dependent on Spring 4.2 and above.

@mercyblitz mercyblitz closed this Dec 20, 2019
@jasonjoo2010
Copy link
Contributor Author

Sorry, this PR will not be merged into, thus @org.apache.dubbo.config.spring.context.annotation.DubboComponentScan has to be compatible with Spring Framework 3.2 +, however, @AliasFor is dependent on Spring 4.2 and above.

I am curious that is this treated a bug or just let users ignore it? (LOST of scanBasePackages)
After all compatibility issue what you just mentioned has many alternative solutions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants