Introduce common composed annotations for @RequestMapping [SPR-13992]

[spring-projects-issues]

Sam Brannen opened SPR-13992 and commented

Overview

See #18022.

Deliverables

1. Introduce @GetMapping, @PostMapping, @PutMapping, @DeleteMapping, & @PatchMapping composed annotations for @RequestMapping
2. Introduce headers and name attribute in composed @RequestMapping annotations.
   • The team has decided to introduce these common attributes.
3. Restrict the value, path, consumes, and produces attributes to single String values in composed @RequestMapping annotations. (see Allow a single element to override an array attribute in a meta-annotation [SPR-13972] #18544)
   • The team has decided against this.

---

Affects: 4.2 GA

Issue Links:

• Allow a single element to override an array attribute in a meta-annotation [SPR-13972] #18544 Allow a single element to override an array attribute in a meta-annotation ("depends on")
• Introduce predefined composed annotations in core Spring [SPR-13442] #18022 Introduce predefined composed annotations in core Spring
• Custom @RequestMapping annotations [SPR-12296] #16901 Custom @RequestMapping annotations
• Introduce common composed annotations for web scopes [SPR-13993] #18566 Introduce common composed annotations for web scopes
• Allow a single element to override an array attribute in a meta-annotation [SPR-13972] #18544 Allow a single element to override an array attribute in a meta-annotation

1 votes, 14 watchers

[spring-projects-issues]

Sam Brannen commented

Work on this issue is being performed in the following branch:

https://github.com/sbrannen/spring-framework/commits/SPR-13992

[spring-projects-issues]

Kazuki Shimizu commented

Is not need @HeadMapping and @OptionsMapping ?
I was implemented REST APIs using HEAD and OPTIONS at past.
I hope to add @HeadMapping and @OptionsMapping if possible.

Thanks.

[spring-projects-issues]

Sam Brannen commented

Rossen Stoyanchev, please review the pull request:

#981

[spring-projects-issues]

Sam Brannen commented

Kazuki Shimizu, for 4.3 RC1 the team decided to implement a minimal set of composed annotations for @RequestMapping (and therefore against @HeadMapping and @OptionsMapping); however, we may later consider introducing such annotations.

Regards,

Sam

[spring-projects-issues]

Kazuki Shimizu commented

Sam Brannen, Thanks for your comment!

for 4.3 RC1 the team decided to implement a minimal set of composed annotations for @RequestMapping

OK! I understand spring team decision. Probably, @OptionsMapping is not need. But i hope to consider again as @HeadMapping ;)

[spring-projects-issues]

Sébastien Deleuze commented

Big +1 for this feature for Spring + Kotlin application developers, it will allow them to write @GetMapping instead of @RequestMapping(method = arrayOf(RequestMethod.GET)) (see this blog post for more details).

[spring-projects-issues]

Rossen Stoyanchev commented

Kazuki Shimizu, for 4.3 we are also adding built-in OPTIONS and HEAD support (see #17721). That means every controller method has those methods covered by Spring MVC. We do allow HEAD and OPTIONS to be mapped explicitly still. However I'm wondering if this changes your mind or do you have use cases in mind still?

[spring-projects-issues]

Kazuki Shimizu commented

Rossen Stoyanchev, thanks for your comment !! I have missed the #17721. It's very good improvement (y)
I think @OptionsMapping and @HeadMapping is unnecessary.

[spring-projects-issues]

Sam Brannen commented

Pushed to master in the following commit:

467b5f3

[spring-projects-issues]

Phil Webb commented

I might be a bit late to the party, but is the Mapping suffix necessary? It feels kind of noisy, especially considering the annotations are usually used in the context of a @Controller where it's pretty obvious that they are mappings.

	@GetMapping("/{username}/vehicle")
	public String getVehicle(@PathVariable String username) {
		// ...
	}

	@DeleteMapping("/{username}/vehicle")
	public String deleteVehicle(@PathVariable String username) {
		// ...
	}

	@PutMapping("/{username}/vehicle")
	public String replaceVehicle(@PathVariable String username) {
		// ...
	}

vs

	@Get("/{username}/vehicle")
	public String getVehicle(@PathVariable String username) {
		// ...
	}

	@Delete("/{username}/vehicle")
	public String deleteVehicle(@PathVariable String username) {
		// ...
	}

	@Put("/{username}/vehicle")
	public String replaceVehicle(@PathVariable String username) {
		// ...
	}

[spring-projects-issues]

Sam Brannen commented

Phil Webb,

The "Mapping" suffix is not technically necessary, but the team decided against the shorter variants for the following reasons.

• They are too similar to the JAX RS annotations -- @GET, @POST, etc. -- which might lead to confusion amongst developers familiar with both technologies.
• The "Mapping" suffix provides a mental link to @RequestMapping allowing users to immediately recognize it's a Spring MVC annotation and not from some other framework.

For what it's worth, I also prefer @Get, @Post, etc. For a concrete example, check out the RestEventsController from my spring-events example project.

Regards,

Sam

[spring-projects-issues]

Rossen Stoyanchev commented

The @RequestMapping annotation was originally carefully chosen and over time has come to be associated with Spring MVC as Sam mentioned. It could have been named @HttpRequest or even shorter @Http. Sure everyone would understand what it refers to and it would be readable but in fact a method annotated with @HttpRequest is not an HTTP request itself, nor is the annotation an HTTP request. It is an HTTP request mapping.

The exact same line of thought applies to @Get which could also be @HttpGet but neither the method, nor the mapping is an HTTP GET. @Get is too unqualified in that sense even if it is short and readable. In the end we are trying to strike a balance.

The association with a mental model is all too often underestimated IMO. JAX-RS has a different style of mapping and while the two won't be mixed in the same class, people with all sorts of background and knowledge will come across all sorts of examples across the internet. Having to add explanations on how the two may be the same or not (in order to help readers) is completely undesirable. Furthermore JAX-RS has been around for many years. Those annotations have well established associations and expectations for many.

What we are adding here are framework endorsed shortcut annotations that should be consistent with other usages in the framework including @RequestMapping as well as others like @MessageMapping, @SubscribeMapping, etc. Its just as easy for anyone to create their own.

Lastly not to forget that you still need @RequestMapping at the type level if you want to have a shared base path which is quite common. Mixing @RequestMapping with @GetMapping in the same class works much more congruently.

[spring-projects-issues]

Oliver Drotbohm commented

I'd like to cast another vote on not putting these shortcut annotations into the framework. They cause a lot of confusion, disputes over naming so that we're probably never going to get to a name that pleases everyone, see above and add very little benefit.

Assume Jane developer is new to the framework, enthusiastic about these annotation and she uses quite a few of them in her controller implementation. Now she realizes, she needs to restrict based on HTTP header. How does she find out she can actually restrict on those. Okay, she checks the documentation and finds out she needs to use a different annotation. Once she accepted that and wondered why that actually is the case, she now ands up with a mixture of those shortcut annotations and good ol' @RequestMapping. Now every other developer looking at the code is gonna ask: why two different flavors? Should we refactor back to @RequestMapping to make stuff consistent?

Another aspect that plays into this: if @GetMapping is available and it's a short cut for @RequestMapping(method = GET), why not have @GetJsonMapping et al. Why is there no @OptionsMapping, no @HeadMapping? Of course all this can be justified with some verbose explanation, but why would we even want to get into that discussion? Controller methods already are a complicated topic especially for newbies due to all the different types supported as parameters, the different ways how return values are interpreted depending on what mode is selected. And this stuff has been growing over the last couple of release with the support for async requests etc. Really, the last thing that area of the framework needs is even more options for the last bit that's actually the only straight-forward thing left there: how to map incoming requests. One way, all functionality imaginable covered, perfect.

I can totally see the case for @RequestScope et al. as that scenario just doesn't exist there. It's more of a corner case in framework functionality, MVC request mapping is front and center of every Spring based web app. I already spend too much time explaining @RestController VS. @Controller.

Don't get me wrong, for more experienced users these annotations look like a shortcut but for less experienced ones they're not. The scenario will look like the simply ones don't provide enough mapping functionality so that "other needs to be used in such scenarios". We're basically painting ourselves in a corner of @Resource VS. @EJB VS. @Inject in the JavaEE world: creating choice without real benefits but a lot of cognitive load. Spring is already deemed too complex and having too many ways to achieve things, sharp tongues even claim that's why Boot has to exist in the first place. All our efforts in the last couple of years have been targeted to simplify things but outside our expert's eyes (for which I can understand this looks simpler in the first place) this is not simplifying — it's the opposite.

I am all in for the framework being able to work with composed annotations so that people can craft their own shortcuts and even a dedicated library living outside the framework for everyone keen to use to do so. If people want to create their @Get they can. I'll bet every project is going to end up with a slightly different flavor if it. However, I am not keen on introducing more choice for the sake of choice and at the expense of confusion in the probably most used area of the framework.

[spring-projects-issues]

Adam Berlin commented

I'd like to add a vote for @Get, @Post, and the like. In the controller context they are a clean way to describe what I want in contrast to @RequestMapping(...). I'd say make the usual case easy and optimized for readability with a nice escape-hatch to solve more complicated mappings.

[spring-projects-issues]

Dave Syer commented

My $0.02. I like the brevity (and I'm not known for that). If Ollie is right, though, and you need to change annotations in order to add a media type constraint, then I think that's bad. Why can't @GetMapping have aliases for everything relevant in @RequestMapping (i.e. nearly everything except method)? In fact I see it does have aliases for nearly everything, so maybe I just misunderstand what is relevant. Why no alias for headers?

[spring-projects-issues]

Sam Brannen commented

I like the brevity (and I'm not known for that).

Glad to hear it. :)

If Ollie is right, though, and you need to change annotations in order to add a media type constraint, then I think that's bad.

@GetMapping intentionally doesn't support the consumes attribute -- since we figured that wouldn't make sense -- but it certainly supports the produces attribute.

Why can't @GetMapping have aliases for everything relevant in @RequestMapping (i.e. nearly everything except method)? In fact I see it does have aliases for nearly everything, so maybe I just misunderstand what is relevant. Why no alias for headers?

Those are valid questions, and the fact of the matter is that we've gone back and forth on this particular issue.

In the Spring Composed project, I actually included all attributes from @RequestMapping in the composed variants (except method and things like consumes which don't make sense for a GET request).

For Spring Framework 4.3, however, we went a different route. Namely, we decided to only include attributes from @RequestMapping that we feel make sense in the opinionated composed variants (i.e., guiding the user toward best practices). For example, we left out the method, name, and headers attributes for all composed variants with the following rationale:

• method: obviously defeats the purpose of such composed annotations
• name: rarely used
• headers: typically only used for advanced use cases

Although the omission of the last two is somewhat debatable, the consensus in the team was that "it's easier to add attributes at a later date than it is to remove them once they are published." This decision is, however, not set in stone.

Regards,

Sam

[spring-projects-issues]

Dave Syer commented

OK, so I wasn't missing anything. Don't care personally about "name", but I don't see any reason to force people to use a different annotation if they want header constraints. I guess one point is that people often mistake constraints for method parameters (and don't realize they are imposing a constraint when they add one), but I think I'd rather pay that price and try to do some education, then have to explain why @GetMapping doesn't have all the features of @RequestMapping, just for GET.

[spring-projects-issues]

Marcel Overdijk commented

I would always favour anything that would make @RequestMapping(method = GET) look better.
That's why I like the GetMapping (or even Get would be better but I understand it clashes a bit with jax-rs @GET annotation).

However I understand Oliver's concern.
I think those new @Get/Post/..Mappings should not be to opinionated. E.g not leave out headers.
Code style it would be a needle in my eye to have to use @GetMapping in places and where I would want a header to revert back to @Mapping.
I also see the concern why not adding aliased @OptionsMapping and @HeadMapping. Why not add them?

[spring-projects-issues]

Enrique Ruiz (DiSiD) commented

I agree with Oliver: composed annotations are a good improvement for experienced developers but it makes a bit more complicated to understand for newbies.

As Oliver said, Boot exists to simplify the complexity of Spring, so why not place these common composed annotations (@GetMapping, ...) in the Spring Boot project?

[spring-projects-issues]

Rossen Stoyanchev commented

Good point Marcel Overdijk about remaining not opinionated. There is no intent here to create many flavors or other variants in the future. In the same line of thought we shouldn't be opinionated about what is and isn't common. The main intent is to introduce focused variants of @RequestMapping per HTTP method that are very widely applicable and encourage the good practice of explicitly indicating the HTTP method. It may also be rather obvious but it is worth pointing out that it isn't simply about not having to add a method attribute. It's also about not having to have two attributes (path and method) for the most basic case of HTTP method and URL pattern.

I don't think putting these annotations in Boot simplifies matters. If the thinking goes that these annotations are for experienced developers, certainly Boot isn't for advanced developers only. These annotations belong in Spring Framework alongside @RequestMapping as more focused but otherwise fully equivalent variants. They should be first choice where a method handles one HTTP method only and there should be no reason to switch them out.

[spring-projects-issues]

Sam Brannen commented

I fully agree with Rossen's most recent comments: we shouldn't be overly opinionated in core Spring, and moving such annotations to Boot doesn't solve anything.

Furthermore, I'm in favor of adding the headers attribute to all composed annotation variants we've introduced, but I won't rush to do that until the team has come to a consensus on this.

Regards,

Sam

[spring-projects-issues]

Hans Desmet commented

As Dave Syer says,
people often mistake constraints for method parameters (and don't realize they are imposing a constraint when they add one),

An alias for the params parameter with the name paramConstraints could help

[spring-projects-issues]

Sébastien Deleuze commented

Rossen Stoyanchev Sam Brannen Also +1 for having @GetMapping, @PostMapping, etc. non opinionated in Spring Framework with the same attributes as @RequestMapping except the method one.

[spring-projects-issues]

Sam Brannen commented

Since #18544 has been completed, we need to decide on the following proposal:

• Restrict the value, path, consumes, and produces attributes to single String values in composed @RequestMapping annotations.

[spring-projects-issues]

Sam Brannen commented

Reopened in order to discuss additional deliverables.

[spring-projects-issues]

Sam Brannen commented

Based on community feedback and further team discussions, I am once again resolving this issue as Complete.

See the following commits for details:

• 78690a2
• bea2641
• cbcc999
• 4bead6e

In summary, the only composed @RequestMapping annotation that is opinionated now (other than the omission of the method attribute) is @GetMapping which does not declare a consumes attribute.