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

FEATURE REQUEST:Multi Level Multiple Inheritance and Override Support #1284

Closed
silkentrance opened this issue Oct 10, 2020 · 11 comments · Fixed by #1341
Closed

FEATURE REQUEST:Multi Level Multiple Inheritance and Override Support #1284

silkentrance opened this issue Oct 10, 2020 · 11 comments · Fixed by #1341
Labels
proposal Proposed Specification or API change

Comments

@silkentrance
Copy link
Contributor

silkentrance commented Oct 10, 2020

WIP still working on the issue and its associated PR, please bear with me

Goals

Multi Level (Generic) Inheritance With Multiple Inheritance Support at the Second (and Higher) Level And Generic Method Override

Here, the feign client interface SomeServiceClientApi inherits from the interface SomeService, which then inherits from multiple other (generic) interfaces, sharing its interface with the SomeServiceApiController rest controller via the SomeService interface.

interface RetrieveService<T> {
  T retrieve(String key);
}

interface StatusService {
  String status();
}

interface SomeService extends RetrieveService<String>, StatusService {
}

@FeignClient
interface SomeServiceClientApi extends SomeService {
   @GetMapping("/retrieve/{key}", produces="text/plain")
   @Override
   String retrieve(@PathVariable("key") String key);

   @GetMapping("/status", produces="text/plain")
   @Override
   String status();
}

@RestController
class SomeServiceApiController implements SomeService {

  @GetMapping("/retrieve/{key}", produces="text/plain")
  @Override
  @ResponseBody
  public String retrieve(@PathVariable("key") String key) { ... }

  @GetMapping("/status", produces="text/plain")
  @Override
  @ResponseBody
  String status() { ... }
}

or, alternatively (and most provably in combination with the above)

interface RetrieveService<T> {
  T retrieve(String key);
}

interface StatusService {
  String status();
}

interface SomeService extends RetrieveService<String>, StatusService {
}

interface SomeServiceApiBase extends SomeService {
   @GetMapping("/retrieve/{key}", produces="text/plain")
   @ResponseBody
   @Override
   String retrieve(@PathVariable("key") String key);

   @GetMapping("/status", produces="text/plain")
   @ResponseBody
   @Override
   String status();
}

@FeignClient
interface SomeServiceClientApi extends SomeServiceApiBase {
}

@RestController
class SomeServiceApiController implements SomeServiceApiBase {

  @Override
  // going for the naive Spring will fixit approach here, alternatively one must repeat the `@PathVariable` annotation
  // on the formal method parameter here
  public String retrieve(String key) { ...; }

  @Override
  public String status() { ...; }
}

Decomposition of the Service Interface for both Testing and Delegation Purposes

A so composed service interface can be decomposed so that one can test indiividual features of a service a/o client using shared (generic) test classes a/o strategies, when considering standard CRUD and other operations.

One could also use delegates on the server side to realize specific facets of the service interface, e.g. one that will realize the status and one that will realize the retrieve, and so on.

And while the Spring framework already provides for such, Feign will currently prevent this from happening as it does not support multiple inheritance on the interface level. Basically meaning that one cannot have a service interface that is shared between both the client and the server without having to redeclare the client interface.

Positive Side Effects

  • overriding methods from (parameterized) base interfaces is now supported

Implementation Notes

Since spring-cloud-openfeign does override some of the process* methods from Contract.BaseContract, I resorted to just detecting when multiple methods with the same key are available.

Annotations on base interfaces, except for when it is the first inherited one, are simply ignored.

Additional test cases have been introduced that provide for a basic safeguard but do not realize every possible combination.

Additional Information

spring-cloud-openfeign must be updated so as to use the new version.

PR

#1285

See also

#320, #1244, #1029

@silkentrance
Copy link
Contributor Author

@kdavisk6 Hi, I wanted to know whether you are still interested in multiple inheritance support? Maybe you can drop a comment or two on the PR and the way that this was realized. TIA!

@silkentrance
Copy link
Contributor Author

silkentrance commented Oct 24, 2020

@kdavisk6 I was able to remove the extra code so that integrators such as spring-cloud-openfeign will only have to update their dependencies.

@silkentrance
Copy link
Contributor Author

@kdavisk6 currently working on a type safe detection of generic overrides.

@silkentrance
Copy link
Contributor Author

silkentrance commented Oct 24, 2020

@kdavisk6 I have introduced the new method Types#resolveReturnType(Type, Type) which tries to resolve the actual type based on the given input. TODO: collections, arrays and maps and generification of otherwise non generic interfaces.
I have also introduced the new TypesRewsolveTypeTest test suite for testing the new method.

@silkentrance
Copy link
Contributor Author

silkentrance commented Oct 24, 2020

@kdavisk6 my initial tests with the current PR shows that I am able to

@FeignClient(name = "concrete-client", contextId = "concrete-client", url = "${concrete.client.web.url}", path = "${concrete.client.web.base-path:/concrete}/client")
interface ConcreteClient extends ServiceInterface {}

// e.g. additional header annotations
interface ServiceInterface extends BulkRetrieveService<...>, BulkCreateService<...>, BulkUpdateService<...> {}

interface BulkRetrieveService<...> {
  @PostMapping(...)
   ... bulkRetrieve(... request);
}

and this will work just fine.

That way I am able to decompose the services into multiple entities and I can also reuse existing tests by simple parameterization instead of reimplementing the whole shebang all over again. And, considering a project with 20+ web services and clients, this will reduce the time spent on implementing the clients/services and test cases thereof by a lot.

@silkentrance
Copy link
Contributor Author

silkentrance commented Oct 24, 2020

PLEASE NOTE: the dependencies listed in here will become obsolete as soon as the PR gets accepted.

PLEASE NOTE: leave any comments on the associated PR #1285 and not in this issue.

NOTE: any gpg signing has been disabled.

For those of you who are interested in this, feel free to test this out

<dependencies>
  <dependency>
    <groupId>eu.coldrye.openfeign</groupId>
    <artifactId>feign-core</artifactId>
    <version>10.12-SNAPSHOT</version>
  </dependency>
</dependencies>

You will also have to add the following repository to your repositories section

<repositories>
        <repository>
            <id>ossrh</id>
            <url>https://oss.sonatype.org/content/repositories/snapshots</url>
            <snapshots>
                <enabled>true</enabled>
            </snapshots>
        </repository>
</repositories>

and similarly so for all the other modules of the openfeign project, except for forms, that it.

And if you by chance are using springframework-cloud-starter-openfeign, you will have to add this to your dependencies section or dependencyManagement/dependencies section

            <!-- BEGIN temporary local dependencies -->
            <dependency>
                <groupId>org.springframework.cloud</groupId>
                <artifactId>spring-cloud-starter-openfeign</artifactId>
                <version>2.2.5.RELEASE</version>
                <exclusions>
                    <exclusion>
                        <groupId>io.github.openfeign</groupId>
                        <artifactId>feign-core</artifactId>
                    </exclusion>
                    <exclusion>
                        <groupId>io.github.openfeign</groupId>
                        <artifactId>feign-slf4j</artifactId>
                    </exclusion>
                    <exclusion>
                        <groupId>io.github.openfeign</groupId>
                        <artifactId>feign-hystrix</artifactId>
                    </exclusion>
                </exclusions>
            </dependency>
            <dependency>
                <groupId>eu.coldrye.openfeign</groupId>
                <artifactId>feign-core</artifactId>
                <version>10.12-SNAPSHOT</version>
            </dependency>
            <dependency>
                <groupId>eu.coldrye.openfeign</groupId>
                <artifactId>feign-slf4j</artifactId>
                <version>10.12-SNAPSHOT</version>
            </dependency>
            <!-- END temporary local dependencies -->

@silkentrance
Copy link
Contributor Author

The current PR #1285 incorporates changes that will make it most versatile, reducing overall complexity and all. Feel free to test this out, see #1284 (comment)

A new snapshot release has been made available.

@mxmlnglt
Copy link
Contributor

mxmlnglt commented Dec 3, 2020

@kdavisk6 I have introduced the new method Types#resolveReturnType(Type, Type) which tries to resolve the actual type based on the given input. TODO: collections, arrays and maps and generification of otherwise non generic interfaces.
I have also introduced the new TypesRewsolveTypeTest test suite for testing the new method.

Note: method is now called resolveActualType but I can't find the TypesRewsolveTypeTest file? (or TypesResolveTypeTest) maybe it is actually DefaultContractInheritanceTest??

@silkentrance
Copy link
Contributor Author

@mxmlnglt I will have a look at it later in the day. It has been a while and the code still needs to be cleaned up.

@silkentrance
Copy link
Contributor Author

@mxmlnglt ok, here is what is contained in the last commit

commit b318a6d5232f039cb45fb0ba56a3106f1fb007db (HEAD -> gh-1284, origin/gh-1284)
allow multiple inheritance on the interface level with current restrictions intact.

core/src/main/java/feign/Contract.java
core/src/main/java/feign/Types.java
core/src/test/java/feign/DefaultContractInheritanceTest.java
core/src/test/java/feign/TypesResolveReturnTypeTest.java

There was a typo in the name 😁

@silkentrance
Copy link
Contributor Author

silkentrance commented Dec 4, 2020

@mxmlnglt I am actually using the build from this branch for my own project. The build was deployed, see above for more information. The build does not include any unwanted stuff, except for what is in openfeign by default.

@kdavisk6 kdavisk6 added the proposal Proposed Specification or API change label Dec 21, 2020
silkentrance added a commit to coldrye-collaboration/feign that referenced this issue Dec 28, 2020
silkentrance added a commit to coldrye-collaboration/feign that referenced this issue Dec 28, 2020
silkentrance added a commit to coldrye-collaboration/feign that referenced this issue Feb 17, 2021
silkentrance added a commit to coldrye-collaboration/feign that referenced this issue May 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proposal Proposed Specification or API change
Projects
None yet
3 participants