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

@GetMapping method annotation uses consumes attribute from @RequestMapping class annotation [SPR-14988] #19554

Closed
spring-projects-issues opened this issue Dec 6, 2016 · 7 comments
Assignees
Milestone

Comments

@spring-projects-issues
Copy link
Collaborator

@spring-projects-issues spring-projects-issues commented Dec 6, 2016

Dmitry Bedrin opened SPR-14988 and commented

@GetMapping method-level annotation on method uses consumes element from @RequestMapping annotation on class and doesn't allow to override it.

As a result if you specify consumes element in class level annotation your GET mapping handler becomes unusable since it requires an appropriateContent-Type request header.

	@Controller
	@RequestMapping(consumes = MediaType.APPLICATION_XML_VALUE)
	public class ComposedAnnotationController {

		@GetMapping("/get")
		public void get() {
			// Cannot be called without Content-Type header
		}

	}

Affects: 4.3.4, 5.0 M3

Referenced from: pull request #1257, and commits 8315a40, e707c40

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Dec 7, 2016

Dmitry Bedrin commented

I've raised separate PRs for master and 4.3.x branches since they vary a bit due to reactive support in 5.x

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Dec 7, 2016

Rossen Stoyanchev commented

A type level annotation is intended to be inherited and/or extended and ideally should apply to all methods. The use of a type-level consumes does save some methods from having to declare consumes but puts the burden on other methods to cancel it at the method level. It's a trade-off either way.

If you have a lot of methods that benefit from a consumes attribute, I would flip the model and declare your own shortcut annotation to use on the method level.

Even if we did address this for GetMapping we would not be able to do the same for plain RequestMapping used at the method level and the the intent for these shortcut annotations is to be simple shortcuts and behave exactly the same way. Not only that but using consumes=MediaType.ALL on GET methods is odd to begin with since GET isn't supposed to consume anything, yet we can't entirely disable consuming something since technically the spec allows it.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Dec 7, 2016

Dmitry Bedrin commented

I believe that creating own shortcut is wrong - developers would confuse which one to use, and autocomplete in the IDE will only increase this confusion.

I see your point about setting consumes attribute to MediaType.ALL. Do you think it makes sense adding the consumes attribute to the @GetMapping annotation so it would be possible to override the type level annotation?

It will cover the 99% of use cases out of the box, yet leaving an ability to handle odd requests like GET with a body.

To give you some background of this issue - I'm migrating a project from JAX-RS to Spring MVC. The project has consumes=json|xml, produces=json|xml type level annotation on most of the controllers. This content type negotiation is only customized in a few controllers (e.g. some of them accept plain text, others produce excel spreadsheets, etc). I wanted to keep the same approach but got blocked by this issue

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Dec 7, 2016

Rossen Stoyanchev commented

Then again composed annotations is an explicit feature for use in applications. We added the method-based shortcuts because they're so broadly applicable and arguably most method-level mappings should specify an HTTP method to begin with. In the custom annotation's attributes you can use @AliasFor (as we have done on GetMapping) and that helps IDEs just the same.

Regarding consumes, yes we could add that to GetMapping. It would be consistent in keeping GetMapping as a fully equivalent shortcut for @RequestMapping.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Dec 7, 2016

Dmitry Bedrin commented

Regarding consumes, yes we could add that to GetMapping. It would be consistent in keeping GetMapping as a fully equivalent shortcut for @RequestMapping.

I'm happy to raise a PR for it (if it saves you any time comparing to doing it yourself :) )

We added the method-based shortcuts because they're so broadly applicable

This is exactly the reason why I suggest discarding the Content-Type header (i.e. setting it to MediaType.ALL) in @GetMapping controllers by default!
Do you think the whole idea is wrong or you're concerned about implementing it using MediaType.ALL ?

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Dec 7, 2016

Rossen Stoyanchev commented

Yes please update the PRs for having a consumes attribute on GetMapping. Thanks!

I'm not in favor of using MediaType.ALL with GetMapping even if only to stay completely equivalent with RequestMapping(method=GET).

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Dec 7, 2016

Dmitry Bedrin commented

I'm not in favor of using MediaType.ALL with GetMapping even if only to stay completely equivalent with RequestMapping(method=GET).

Fair enough. I agree
I'll update the PRs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.