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.

[NemesisMate]

@rstoyanchev , since it looks like this will take some time to reach a conclusion, what do you think about "unbreaking" it meanwhile so at least existing code using both can benefit of general upgrades? (I believe it would be enough by just replacing the recently added exception by a warning).
If a different ticket is required, this one can be reopened.

[rstoyanchev]

@NemesisMate, we need to make progress under #32043 first and the broader topic of what it would look like to support multiple mapping annotations on the same controller method. We don't intend to remove the checks in the mean time, and although it was possible to do before, it's not something that was intentional.

[rstoyanchev]

Thanks for providing the sample. Am I correct in assuming that running MonolithTest is the way to check after making changes?

I experimented with leaving only @HttpExchange annotations on shared contracts, pushing the @RequestMapping annotations from BookApi down to BookController, and removing all others, which are just duplicates. The tests pass, showing that use of the alternative "/books" URL and "genre" parameter mapping still work.

So the sample is a great start, but it does not yet fully demonstrate the challenge. Could you comment or better yet continue to update the app to provide more detail.

[NemesisMate]

Thanks for providing the sample.

Thanks to you for the support, @rstoyanchev

We don't intend to remove the checks in the mean time, and although it was possible to do before, it's not something that was intentional.

Wouldn't a breaking change like this (even if it wasn't intended, it might have been broadly used and it breaks in a way that it has no easy fixing when a project reaches a certain size) be left for a minor/major (as in semver's) release?

Am I correct in assuming that running MonolithTest is the way to check after making changes?

Both MonolithTest and BookControllerTest should work after any annotation change, if they work (and I haven't overseen anything) it means the whole project is working as expected in both approaches: distributed and monolithic.

but it does not yet fully demonstrate the challenge

The challenge appears when you upgrade from 3.2.2 to 3.2.3 in build.gradle (and therefore, introducing the breaking change). After the upgrade, it is quite impossible to keep the Java code design and make the tests work just by tweaking annotations; instead, the whole design needs to be changed, duplicate interfaces, manually keep things aligned.

I believe that If you try and upgrade the version, you'll realize the workarounds that are needed at code-level to ensure the tests still work (which can be huge changes for big sized projects).
Is it possible? sure, but IMO it defeats the purpose of the meta-programming with annotations since now you have to adapt the whole project code to the annotations instead.

And what is worse is, you might have a big project, simple, with the new versions without using both annotations in the same classes as long as you don't need advanced variances (e.g., different paths in the controller) but if all of a sudden you need it, you have to redesign everything.

[rstoyanchev]

I realize I forgot to share my branch:
https://github.com/rstoyanchev/spring-framework-issue-32328/commits/http-exchange-only/

It is upgraded to 3.2.5. Both tests pass.

Wouldn't a breaking change like this (even if it wasn't intended, it might have been broadly used and it breaks in a way that it has no easy fixing when a project reaches a certain size) be left for a minor/major (as in semver's) release?

The timing wasn't great, but it caught us by surprise. We did not expect this type of usage, and we don't want to have to break even more applications later. HttpExchange annotations are relatively recent (6.0) with server side support even more recent (6.1).

As I mentioned, making use case more clear is the best way to advance this.

[NemesisMate]

We did not expect this type of usage

It's a quite common usage I believe, which was already promoted by Open Feign.

HttpExchange annotations are relatively recent (6.0) with server side support even more recent (6.1).

'Relatively' being the key word here ;). But as relatively new is my current project (which, in the light of HttpExchange I decided replace Open Feign which seemed to be covering this use-case, although it looks it was by mistake).

we don't want to have to break even more applications later

I guess that I have to settle with being one of the few then :'(

It is upgraded to 3.2.5. Both tests pass.

From your changes, I can say that it is already not ideal since now the request mappings in the controller implementation (which come from the common API) need to be defined in every service implementing the common api (a change in the common, requires tens of APIs having to tweak their annotations when they could be in a single place). The fact that some annotations (like RequestBody and everything else but the "mappings/exchanges" ones are supported doesn't feel totally right.

Looking at it now, I'm thinking that maybe I was trying too hard to keep the annotations centralized (because of the reason mentioned above) and that's why I came up with the "can only be made by changing the code" solution, that was my bad. Still, it would be great to avoid all the redundancy required and the quirky integration between these annotations and get them working in a seamless way.

As I mentioned, making use case more clear is the best way to advance this.

Let me find some time and add more to it so it gets clearer, but the changes you did are the main of it I believe, it just doesn't look that bad in a small project, but it adds up on bigger ones (think tens of micro-services, with more complex annotation configurations, params, paths, etc).

[rstoyanchev]

In common-lib I did nothing more than drop @RequestMapping annotations from the base contracts. That shouldn't impact implementations. The dropped annotations were exact duplicates of @HttpExchange.

In BookApi I pushed the @RequestMapping annotations with server specific concerns down to the controller . That's a specific service though, and there is only one implementation of it in the sample. Doing so doesn't create more redundancy than there already was, but again I'm only working with the given sample.

[NemesisMate]

I've added more "items" (implementations of the common APIs) to better see the cumbersomeness of the current approach. For my surprise, it is not as bad as I thought it was. It is true the project I'm working for is much bigger and with much more customizations but still, I've been thinking wrongly about something or I can't recall the real pain points, which I guess I'll see again when I retry to upgrade everything to the "correct" approach.

Still, this is not to say that it is ideal. There are still a bit of annoyances since Exchange and Mappings annotations need to be manually kept aligned (params, methods, etc), which would benefit from a shared one, but again, quite not that bad.

An outstanding issue (and that is really annoying) is the fact that the client interfaces need to override any method that have a generic so it can recognize its final type. For example:
https://github.com/NemesisMate/spring-framework-issue-32328/blob/http-exchange-only/book-app/book-client/src/main/java/com/example/book/client/BookClient.java
The delete method in the parent is not needed since it doesn't have a generic object in its method but the ones that have all need overriding (including all the parameter annotations involved). This does not happen with controllers, so I assume is a misbehavior, can you confirm?