search

spring-projects / spring-framework

Spring Framework
https://spring.io/projects/spring-framework
Apache License 2.0
56.63k stars 38.13k forks source link

Enable REST controller method parameter annotations on an interface [SPR-11055] #15682

Closed spring-projects-issues closed 5 years ago

spring-projects-issues commented 11 years ago

Paweł Mendelski opened SPR-11055 and commented

Please, enable inheritance of controller related annotations from implemented interfaces.

I would like to share rest service interface between client and server. This way I could provide a really convenient mechanism - client-proxy, like the one RestEasy provides.

Shared interface:

@RequestMapping("/random")
public interface RandomDataController {

    @RequestMapping(value = "/{type}", method = RequestMethod.GET)
    @ResponseBody
    RandomData getRandomData(
            @PathVariable(value = "type") RandomDataType type, @RequestParam(value = "size", required = false, defaultValue = "10") int size);
}

Server implementation:

@Controller
public class RandomDataImpl implements RandomDataController {

    @Autowired
    private RandomGenerator randomGenerator;

    @Override
    public RandomData getPathParamRandomData(RandomDataType type, int size) {
        return randomGenerator.generateRandomData(type, size);
    }
}

Client code:

// ...
RandomDataController randomDataController = SpringMvcProxyFactory.create(RandomDataController.class, "http://localhost:8080");
RandomData rd = randomDataController.getRandomData(RandomDataType.ALPHA, 100);
// ...

At the moment I cannot write SpringMvcProxyFactory because parameter annotations from interface RandomDataController are not inherited by RandomDataControllerImpl.

Affects: 4.3.3

Issue Links:

Referenced from: commits https://github.com/spring-projects/spring-framework/commit/790d515f8c49d9f80f41a2f0ca00535fa930ccc2, https://github.com/spring-projects/spring-framework/commit/1f5d0faf1f3a1ee1b8e26b76cd72a5fab3c2839b

30 votes, 37 watchers

spring-projects-issues commented 10 years ago

George Georgovassilis commented

There is an implementation here: https://github.com/ggeorgovassilis/spring-rest-invoker I would be happy to contribute it as a module or to spring-web if there is interest

spring-projects-issues commented 10 years ago

George Georgovassilis commented

Upon re-reading the ticket's description, the submitter asks for a common interface that would work both for controllers that expose REST APIs and clients that consume them. The implementation above generally requires interfaces that might not make sense when implemented by a controller.

spring-projects-issues commented 10 years ago

Rossen Stoyanchev commented

Controller annotations can be inherited from a base class or interface. Method parameter annotations however cannot be. They must be directly on the controller method in all cases. This ticket is a request to change that.

Indeed your proposal has similar purpose but does not depend on this ticket since in your case the interfaces are only used for generating a client proxy.

That said once the ability to create a client proxy from an interface with the same annotations is available, it becomes a natural question: should it be possible for a controller and a client proxy to share an interface?

spring-projects-issues commented 10 years ago

George Georgovassilis commented

In that case I'd like to decouple my implementation from the concrete decision whether a single or multiple interfaces are to be used. New ticket #16747.

spring-projects-issues commented 8 years ago

Matt Benson commented

This issue also applies to parent classes.

spring-projects-issues commented 8 years ago

Matt Benson commented

PR submitted: https://github.com/spring-projects/spring-framework/pull/976

spring-projects-issues commented 8 years ago

Rossen Stoyanchev commented

Wow this is a very extensive effort and a perfect example of the sort of thing that you could discuss (beforehand) on our contributor's mailing list.

Style and programming model issues aside, there is a whole separate discussion if we should even re-use the same annotations for client-proxy generation or create more dedicated annotations for the purpose (see #16827). Alternatively with what we have today wouldn't it be easy to generate interfaces on the fly from a controller's methods and annotations? That way method parameter annotations can remain close to the class as promoted by the programming model today.

spring-projects-issues commented 8 years ago

Matt Benson commented

TBH I decided to take my chances rather than subject my inbox to the burden of another mailing list. With regard to client proxies, spring-cloud is already doing this with their support for Netflix Feign, and until recently their documentation (at least somewhat erroneously) reported that it was possible to use a Spring-annotated REST interface for client and server alike. The non-inherited nature of parameter annotations is in fact the missing ingredient here. If people are going to use interfaces for client proxies, I can't see how forcing them to define their REST API in multiple places does anything other than expose them to the risk that these definitions will fall out of sync.

My interpretation of "on the fly" is "at runtime." I don't see how that helps clients who want to consume the hypothetical generated interface. Certainly it would be possible to generate such an interface, but it seems like this would be more a tooling step or, at best, an annotation processor, and getting these generated types into a consumable client artifact would be quite a build-system-dependent task. It is not at all clear to me that this solution would be better than a basic, Java-based one.

spring-projects-issues commented 8 years ago

Matt Benson commented

With the recent closure of the development forum, do any other Spring developers have any thoughts on the proposed fix?

spring-projects-issues commented 8 years ago

Rossen Stoyanchev commented

If we did implement this feature to allow method parameter annotations on supertypes it would most likely be exposed through AnnotationUtils which we currently use to find method and type level annotations on supertypes (see for example RequestMappingHandlerMapping). The various argument resolver implementations would delegate to AnnotationUtils unlike the PR which introduces the same as base class functionality. Also the current algorithm in AnnotationUtils for method and type level annotations is also a lot more involved and so would be the one for method parameters. This is not really meant as criticism but just saying that the problem is more complex. We are however taking a look at this still. There are several related tickets so I'm going to assign this one also to Juergen Hoeller for consideration.

From a Spring MVC perspective I think basic support for the Spring Cloud with Netflix Feign use case (i.e. allow the same interface to be used on client and server side)is fine as a use case but allowing a method parameter to be anywhere is another matter. It means for a controller method with several arguments the annotations for each argument could literally be in a different superclass. I'm not sure it should be so wide open. Some meaningful constraints focusing on the target use cases we want to support should perferably be in order.

spring-projects-issues commented 8 years ago

Alexandre Navarro commented

+1

spring-projects-issues commented 7 years ago

Elias O commented

One more use case for method param annotation inheritance: I'm using Swagger Codegen to generate API interfaces from OpenAPI specs during build process so that I can implement them directly in my code. Those generated interfaces contain all Spring MVC related annotations so I don't need to care about annotating my methods. Except method params. I had to copy them once again because Spring doesn't see them.

spring-projects-issues commented 7 years ago

Yura Nosenko commented

@Elias O, have a similar story with RAML. It's a huge gap, since the method-level annotations work as expected. This feature is a MUST!

spring-projects-issues commented 7 years ago

Yura Nosenko commented

When it comes to validation, there is a workaround - @Validated. It will actually make jsr303 param annotations inheritable.

spring-projects-issues commented 7 years ago

Rossen Stoyanchev commented

Do you mean that Swagger can't generate a class for a server stub, and same for RAML?

spring-projects-issues commented 6 years ago

Elias O commented

Rossen Stoyanchev that mean that if you use Swagger codegen, you can choose an "API [interfaces] only" option that generates API interfaces like this:

public interface Resource {
  @RequestMapping
  void do(@RequestParam String name, @RequestBody DTO value);
}

so that you may expect your controller to look like this:

@RestController
public class ResourceImpl implements Resource {
  void do(String name, DTO value) {
    // do stuff
  }
}

ResourceImpl#do will inherit @RequestMapping, but @RequestParam and @RequestBody won't, therefore name and value will be null (AFAIR).

The only way to make it work is to duplicate arg annotations:

@RestController
public class ResourceFixedImpl implements Resource {
  void do(@RequestParam String name, @RequestBody DTO value) {
    // do stuff
  }
}

In real cases, such interfaces can have tons of annotations (moreover, arg annotations tend to become very long) and it makes sense to leave them aside from the implementation.

To overcome this bug, currently swagger can generate api + implementation (with annotated params) + delegation interface, so that users can implement a delegation interface only.

spring-projects-issues commented 6 years ago

Alexandre Navarro commented

Hi,

I will explain our use case why we want to implement @FeignClient interface in the @RestController.

In my firm, we have developped java microservice (around 20) with spring, spring-boot and spring-cloud with @FeignClient since 3-4 years. Our different microservices are in different git repo, can be released independently and at first each microservice implements their own @FeignClient if they need to request another microservice, so our microservices are really independent each other. One day, someone corrected a typo from "portofolio" to "portfolio" in an argument of an annotation of a @RestController, he changed in the controller and some feign clients but missed one feign client. We had a bug in production we have to release a fix. Since this moment, we decided to change our strategy with @FeignClient, we decided to share it, not duplicate it. Now, our microservice are splitted in 2 modules, one for the microservice itself, one for the api which include the POJO used as argument or result of the @RestController an @FeignClient Interface. If another microservice2 needs to call microservice1, he will take microservice2-api as lib and it uses it in order not to copy all Feign interfaces everywhere. Clearly, we are happier with shared @FeignClient with shared-lib-api (we only used the api lib internally to the team not with the others which consume our microservice). Nevertheless, today, as the @RestController can't implement the @FeignClient interface because the annotation in the argument are not taken, we need to duplicate the different annotations between @RestController and @FeignClient and we added some IntegrationTest to test if our @FeignClient interface are coherent with the @RestController, which is clearly painful and can be fixed if a @RestController can implement @FeignClient interface.

So this is the reason why I really want to have this feature.

I understand you have to set some rules if for instance some annotations in the implementation are different in the interface (potentially they can be contradictory).

In my point of view, you should take first the annotations of the interface and potentially add / override it if the implementation add / override some annotations. It can be useful because

This is just my point of view on the rule to adopt between interfact / implemenation annotations, maybe you have some other arguments to decide apply different rules between annotations declared in @ClientFeign vs @RestController

Thanks in advance to implement this feature, it will be really really useful.

@Juergen Hoeller : Just to remember, we discussed about it at the Spring BOF at Devoxx France 2 weeks ago.

 

spring-projects-issues commented 6 years ago

Damian Sima commented

Hi guys, can we have an update from the eng team about this?

I mean the issue was open un 2013 and it is still active but unresolved, are we gonna have a feature for this? 

Cheers!

spring-projects-issues commented 6 years ago

Andrey Gliznetsov commented

I'd like to add that some annotations actually work on interface, f.e. @RequestParam, but not  @RequestBody nor @PathVariable

spring-projects-issues commented 6 years ago

Juergen Hoeller commented

I'm happy to report that this is finally resolved in master now, in time for the 5.1 RC1 release!

Feel free to give it an early try against the upcoming 5.1.0.BUILD-SNAPSHOT...

spring-projects-issues commented 6 years ago

George Georgovassilis commented

Thank you Juergen Hoeller

spring-projects-issues commented 6 years ago

Damian Sima commented

you rock Juergen Hoeller

polluxxing commented 5 years ago

@jhoeller hi,how can I achieve the same effect in spring5.0.x?