Rejection of @RequestMapping and @HttpExchange declarations on the same element prevents elegant designs.

[NemesisMate]

Caused by #32065

Upgrading Spring version I Just stumbled upon an issue caused from removing the support to have both type of annotations in a single element.

Allowing both annotations provides more flexibility on simpler designs since the API can be defined in a single interface and ensure both controller and clients work by implementing the same root API. It also simplifies quite much when there is inheritance and the need to use @RequestMapping unique features (like multiple URLs or the params condition).

As of this change, where I have a generic interface defining a common API, which is inherited by a domain-based API and finally implemented by a controller and its respective client, I would need to separate from the root API (most generic one) since there are methods that have unique requirements on the @RequestMapping declarations (including Get, Post and the other method variants) and that can't be set with @HttpExchange (and therefore can't opt to use that one also for Controllers).

This workaround is less natural and becomes messy very fast.

Would it be possible to review this decision and, maybe, revert it?

Design allowing @RequestMapping and @HttpExchange based declarations on the same element

Design to achieve the same by rejecting @RequestMapping and @HttpExchange based declarations on the same element

[rstoyanchev]

@NemesisMate thanks for the report.

We put the assertion in place because @HttpExchange is supported for server side use, and it becomes unclear which should be used. I understand that you don't mean to use @HttpExchange on the server side, but there is no way to make that clear. It just happens to work because at the moment we check @RequestMapping first, but it's not guaranteed.

I'm wondering if you have considered to use only @HttpExchange in the common contract, and override with @RequestMapping on the controller side where needed. I don't know how frequently you need to vary the mappings.

Another option is to remove mapping annotations from the common contracts and re-declare methods, with the appropriate mapping annotations, in controller and client contracts. There is repetition in that, but the IDE and compiler should help help to keep them in sync, and it enforces common method signatures.

[NemesisMate]

Hi @rstoyanchev , thanks for your reply.

It just happens to work because at the moment we check @RequestMapping first, but it's not guaranteed.

Can it not be done as per-design and therefore guaranteeing it instead of having it "just happen"?

I'm wondering if you have considered to use only @HttpExchange in the common contract, and override with @RequestMapping on the controller side where needed. I don't know how frequently you need to vary the mappings.

At first, when I encountered the error after the update this is the first thing I tried to do but it became very messy. The issue comes when there is a second level of inheritance. Since you can't add both annotations in the first and second level, every new method that is introduced needs to be overridden by the final implementation.

Another option is to remove mapping annotations from the common contracts and re-declare methods, with the appropriate mapping annotations, in controller and client contracts. There is repetition in that, but the IDE and compiler should help help to keep them in sync, and it enforces common method signatures.

This ends up being quite similar to the previous workaround since it requires the same duplication of interfaces/classes. In both cases I'd end up with the second image posted above which, can be achieved but it feels quite more cumbersome than what is possible by not adding this limitation.

Basically, instead of having a single contract that can be implemented by server/client we need to have mirror contracts (IMO, quite less elegant as a design and also, even with the help of the IDE/compiler, prone to missing parts and harder to keep coherence).

So, coming back to:

We put the assertion in place because @HttpExchange is supported for server side use, and it becomes unclear which should be used.

The fact of it being supported on server side suggests that it is to help having a single contract that can be implemented by both server & client but at the same time its limitations on server-side makes it unusable in many cases, cases in which now, instead of being able to use an annotation for each of the parts, we need to create separate contracts. Therefore, this "help" might end up being more of a burden than a help (in my case certainly is).

It just happens to work because at the moment we check @RequestMapping first, but it's not guaranteed

Ideally the @RequestMapping wouldn't be checked first but after @HttpExchange by overriding/extending it when used in server-side (which is how I first tried to use them, long ago, before discovering that I had to fully configure both annotations if used together, which, still is much simpler and straight-forward than the workaround I need to do now). Is there a reason why this is not done when they are used in the same element?

[wingsofovnia]

I'd like to second the issue with my use case which is similar to the author's. In my setup, this change is a regression and I'd need to stuck with the previous Spring release for now.

My typical project structure is:

├── app
│   ├── api
│   │   ├── build.gradle
│   │   └── src
│   │       └── main
│   │           └── kotlin
│   │               └── com
│   │                   └── my
│   │                       └── app
│   │                           └── api
│   │                               └── AppApi.kt # interface, API contract
│   ├── client
│   │   ├── build.gradle
│   │   └── src
│   │       └── main
│   │           └── kotlin
│   │               └── com
│   │                   └── my
│   │                       └── app
│   │                           └── api
│   │                               └── AppApiClient.kt # HTTP Interface Client, proxy-implements AppApi
│   ├── build.gradle.kts
│   └── src
│       └── main
│          └── kotlin
│               └── com
│                   └── my
│                       └── app
│                           ├── App.kt
│                           └── api
│                               └── AppApiController.kt # @RestController, implements AppApi
└── settings.gradle.kts

In this configuration, AppApi defines REST API contract:

interface AppApi {
    @GetExchange("/resources/{resource_id}", accept = ["application/json"])
    @GetMapping("/resources/{resource_id}", produces = ["application/json"])
    fun getResource(
        @PathVariable("resource_id", required = true)
        resourceId: UUID,
    ): ResponseEntity<Resource>
}

which is then implemented by AppApiController:

@RestController
class AppApiController: AppsApi {
    fun getResource(
        resourceId: UUID,
    ): ResponseEntity<Resource> {
        return ResponseEntity.ok(service.getResource(id = resourceId))
    }
}

and AppApiClient provides a handy method that creates a HTTP interface client with HttpServiceProxyFactory.

This way, API contract is defined neatly in a central place, neither duplication of code that can vary independently nor repetition of annotations occurs. Any other service in the monorepo that needs to communicate with app can simply add dependency to :app:client.

I'm wondering if you have considered to use only @HttpExchange in the common contract, and override with RequestMapping on the controller side where needed. I don't know how frequently you need to vary the mappings - @rstoyanchev

I guess this will work but certainly not as clean as pre-change look. It is also confusing for devs who come to see a controller and only see @RequestMapping but not all other annotations and wonder how it works.

We put the assertion in place because HttpExchange is supported for server side use, and it becomes unclear which should be used - @rstoyanchev

I remember there were some GH issues requesting HttpServiceProxyFactory to recognise @RequestMapping annotations (or questioning the need in new @HttpExchange) for similar reuse but it was declined as clients needed another perspective or smth. I can see why one might want to keep them separate. It is weird that HttpExchange "leaked" backwards into server side tho :)

Actually, given it is supported by the server, I tried using @HttpExchange alone so that it used both by client and server, but later realised in this use case @HttpExchange and @RequestMapping are opposite in a way that @GetExchange defines that a client accept json, while @GetMapping tells the server produces json responses. This is not possible to model with @GetExchange alone.

viz: @sbrannen (as a change author)

[rstoyanchev]

@wingsofovnia, I was wondering at first whether you had noticed that we added support for @HttpExchange for server side handling since in your example, @HttpExchange and @RequestMapping have the same values, and you shouldn't need both.

Actually, given it is supported by the server, I tried using @HttpExchange alone so that it used both by client and server, but later realised in this use case @HttpExchange and @RequestMapping are opposite in a way that @GetExchange defines that a client accept json, while @GetMapping tells the server produces json responses. This is not possible to model with @GetExchange alone.

@HttpExchange was designed to be neutral to client vs server use. The contentType attribute is for the request body while the accept attribute is for the server response. These should be interpreted correctly when used for a client and when used for a server mapping. In other words this should work as expected. Have you actually tried it, and if so can you provide more details on what the issue is?

[rstoyanchev]

@NemesisMate, thank you for the feedback.

You raise a good point that if we are to support @HttpExchange and @RequestMapping on the same method, we need to decide how they relate to each other, and how exactly it should work. In what order to check the annotations, how to combine the attributes, and so on. Would path patterns have to be repeated, and if not would additional attributes apply only to the paths of the annotation they're on.

Generally, we have not supported multiple mapping annotations on the same method. The question came up recently for @RequestMapping, unrelated to @HttpExchange, and we will have a look at that in #32043. I will add a comment a comment there to also consider this request. We need to consider the two together.

In the mean time, you've only provided a very rough sketch of the actual scenario, and it's not enough to understand how mappings can vary between client and server while method signature remain the same. Seeing more concrete examples of controller methods and mappings would be really helpful. Ideally, if you can work out a small but realistic sample that would be great.

[NemesisMate]

Hi @rstoyanchev , a more concrete example could be as follows:

Having a common API for all micro-services so they all follow certain structure:

public interface CommonApi<DTO, ID> {
    @GetMapping
    @GetExchange
    List<DTO> getAll();

    @GetMapping("/{id}")
    @GetExchange(value = "/{id}")
    DTO get(@PathVariable("id") ID id);
}

Creating an API definition for the Book entity, which would be in a multi-project gradle project composed of a api subproject (or module):

@RequestMapping({"/book", "/books"})
@HttpExchange(url = "/book", contentType = MediaType.APPLICATION_PROTOBUF_VALUE, accept = MediaType.APPLICATION_PROTOBUF_VALUE)
public interface BookApi extends CommonApi<BookApi.BookDto, Integer> {
    @GetMapping(params = "category")
    @GetExchange
    List<BookApi.BookDto> getAllByCategory(@RequestParam(value = "category") String category);

    record BookDto(int id) { }
}

A server module:

@RestController
public class BookController implements BookApi {
    @Override
    public List<BookDto> getAllByCategory(String category) {return null;}

    @Override
    public List<BookDto> getAll() {return null;}

    @Override
    public BookDto get(Integer integer) {return null;}
}

And the client (which would be an auto-configuration library) one so other applications can use it if proceeds:

public interface BookClient extends BookApi {
}

Now, the "store" micro-service would use the api module to code its functionality (gradle's implementation) and the client as a gradle's runtimeOnly one.

With this setup, creating a "centralized" project to de-distribute the applications and run them all as a monolith would be as simple as importing both projects store and book without their client modules and all would glue together just as if the client/server technology was not there in the first place (which is part of the point of these annotations IMO, to not pollute the code logic itself).

As it stands in 6.1.3, there are still some caveats to workaround (like duplicating values here and there) but it is by far easier to work with than 6.1.4 (the one introducing the breaking change that doesn't allow this kind of intuitive designs anymore).

[rstoyanchev]

I think there is room to simplify, but it might be just me not fully seeing all the details yet. Rather than responding in snippets, could you turn this into a small Github project that can actually run and makes it possible to try out changes. That would help to further clarify the scenario and limitations.

[NemesisMate]

@rstoyanchev , I've created a small project dedicated to this issue: https://github.com/NemesisMate/spring-framework-issue-32328.

Although it is a much simpler design than the one I'm working with, I believe it is complex enough to test the problems commented in this issue, which you can trigger by upping spring-boot-dependencies version in the main build.gradle file.