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

Fix ReflectUtils not support generic call with Future #7041

Merged
merged 1 commit into from
Feb 22, 2021

Conversation

AlbumenJ
Copy link
Member

What is the purpose of the change

Add TypeVariable support for ReflectUtils

Fix #7040

Interface example:

public interface TypeClass<T> {
    CompletableFuture<T> getGenericFuture();
}

@codecov-io
Copy link

codecov-io commented Dec 20, 2020

Codecov Report

Merging #7041 (9415f09) into master (e84cdc2) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #7041      +/-   ##
============================================
+ Coverage     59.40%   59.42%   +0.02%     
+ Complexity      508      507       -1     
============================================
  Files          1028     1028              
  Lines         41519    41522       +3     
  Branches       6037     6038       +1     
============================================
+ Hits          24664    24675      +11     
+ Misses        14110    14104       -6     
+ Partials       2745     2743       -2     
Impacted Files Coverage Δ Complexity Δ
...va/org/apache/dubbo/common/utils/ReflectUtils.java 69.54% <100.00%> (+0.15%) 0.00 <0.00> (ø)
...dubbo/remoting/exchange/support/DefaultFuture.java 88.03% <0.00%> (-4.28%) 0.00% <0.00%> (ø%)
...he/dubbo/registry/multicast/MulticastRegistry.java 67.12% <0.00%> (-0.93%) 0.00% <0.00%> (ø%)
...g/apache/dubbo/registry/consul/ConsulRegistry.java 60.00% <0.00%> (-0.59%) 31.00% <0.00%> (ø%)
...org/apache/dubbo/registry/redis/RedisRegistry.java 48.81% <0.00%> (-0.40%) 27.00% <0.00%> (-1.00%)
...n/java/org/apache/dubbo/common/utils/UrlUtils.java 74.25% <0.00%> (+0.33%) 0.00% <0.00%> (ø%)
...ava/org/apache/dubbo/config/ServiceConfigBase.java 59.53% <0.00%> (+0.57%) 0.00% <0.00%> (ø%)
...pache/dubbo/registry/support/FailbackRegistry.java 66.35% <0.00%> (+0.93%) 0.00% <0.00%> (ø%)
...pache/dubbo/registry/support/AbstractRegistry.java 80.74% <0.00%> (+1.11%) 0.00% <0.00%> (ø%)
...apache/dubbo/registry/client/RegistryProtocol.java 58.22% <0.00%> (+2.84%) 0.00% <0.00%> (ø%)
... and 1 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 e84cdc2...9415f09. Read the comment docs.

@kimmking
Copy link
Member

Hello, this is a good idea.
But another thing should be clarified here is that PASS A Generic Type that UNKNOWN type through client-server via RPC not a best practice. Some one as a consumer call this interface to invoke a method from provider side, MAY NOT think about a certain type to be the generic T, missing in the provider side. We also say this case degrade the RPC contracts.

@AlbumenJ
Copy link
Member Author

@kimmking Hi, kimmking. I agree with you. It is not recommended that consumer to call a interface without thinking about a certain type to be the generic T.

The reason why I create this pr is that when I want to define a shared method for A LOT OF interfaces, which have the same method except the return types are different, I cannot refer the interfaces in both consumer and provider sides.

There is an example:

BaseInterface:

public interface BaseInterface<T> {

    T get();

    CompletableFuture<T> getAsync();
}

InterfaceA:

public interface InterfaceA<CommonResult> extends BaseInterface<CommonResult> {
}

InterfaceB:

public interface InterfaceB<String> extends BaseInterface<String> {
}

In this case, consumer just depend on InterfaceA and InterfaceB. For consumer and provider, the gerenic type T is explicited which has been define in InterfaceA or InterfaceB.

@chickenlj chickenlj merged commit 860a3f7 into apache:master Feb 22, 2021
AlbumenJ added a commit to AlbumenJ/dubbo that referenced this pull request May 27, 2021
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.

Unable to refer interface with CompletableFuture<T>
4 participants