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

@RequestMapping inheritance on inner classes [SPR-14513] #19082

Closed
spring-projects-issues opened this issue Jul 26, 2016 · 7 comments
Closed

@RequestMapping inheritance on inner classes [SPR-14513] #19082

spring-projects-issues opened this issue Jul 26, 2016 · 7 comments
Assignees
Labels
in: web status: declined type: enhancement

Comments

@spring-projects-issues
Copy link
Collaborator

@spring-projects-issues spring-projects-issues commented Jul 26, 2016

Darion Haase opened SPR-14513 and commented

Similar to how mapping 'inheritance' works on methods it should be possible to do this on inner classes as well:

@Controller
@RequestMapping("/category")
public class CategoriesController {

	// maps to "/category/"
	@RequestMapping("/")
	public String showAll(Model model) { [...] }

	@RequestMapping("/{category}")
	public class CategoryController {

		// maps to "/category/{category}/"
		@RequestMapping("/")
		public String showCategory(@PathVariable Category category, Model model) { [...] }

		// maps to "/category/{category}/download"
		@RequestMapping("/download")
		public String downloadCategory(@PathVariable Category category, Model model) { [...] }
	}
}

The CategoryController could itself include another class if necessary. This system would reduce duplicate mapping descriptions on larger controllers, currently it has to be like this:

@Controller
@RequestMapping("/category")
public class CategoriesController {

	// maps to "/category/"
	@RequestMapping("/")
	public String showAll(Model model) { [...] }

	// maps to "/category/{category}/"
	@RequestMapping("/{category}")
	public String showCategory(@PathVariable Category category, Model model) { [...] }

	// maps to "/category/{category}/download"
	@RequestMapping("/{category}/download")
	public String downloadCategory(@PathVariable Category category, Model model) { [...] }
}

More mapped methods would increase the amount of duplication.

There has been a similar request which uses inheritance of classes in #16048


No further details from SPR-14513

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Aug 8, 2016

Rossen Stoyanchev commented

We can consider some feature for a URL-based hierarchy of controllers.

However two concerns with the suggested approach. First it is not explicit enough. I could happen to create a controller as an inner class within another and without intending for the inner class to be relative to the containing class? Second it imposes requirements that are quite harsh. It forces the use of nested classes which may or may not be desired and it could get quite ugly with more than one levels of nesting. Also worth noting the nested controller in the example is not static which probably means you have to declare it explicitly and that is yet another undue requirement.

By contrast we are talking about saving a URL prefix from being duplicated which seems uncomfortable but URL structure in itself doesn't change that often so is it really an issue in practice?

That said perhaps a case can be made for a controller that handles a sub-URL space under one or more root URLs. We could do something similar to JAX-RS sub-resources where an @RequestMapping method returns another controller as a "sub-controller". Then again even in that case the same can be done by putting multiple prefixes in the type-level @RequestMapping of the sub-controller. So it really comes down mostly to saving a few duplicate prefixes.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Aug 8, 2016

Rossen Stoyanchev commented

All in all I lean towards not fixing this.

@spring-projects-issues spring-projects-issues added status: declined type: enhancement in: web labels Jan 11, 2019
@Sam-Kruglov
Copy link

@Sam-Kruglov Sam-Kruglov commented Nov 28, 2020

@rstoyanchev hello, it's been a while since this issue was closed, would you care to reconsider? This should be similar to the recent support of @org.junit.jupiter.api.Nested with Spring context inheritance. Perhaps, it wouldn't be considered so ugly now?
Personally, I haven't given it much thought in terms of if it's even needed but it would be nicer for my simple use case, where I have a bunch of "/**/self" (currently authenticated user) and "/**/{userId}" (access another user). So, I would only have 1 level of nesting and only 2 classes but they would share the instance variables and the URL prefix.

@rstoyanchev
Copy link
Contributor

@rstoyanchev rstoyanchev commented Nov 30, 2020

Is it really all that different from this which can be nested or not:

@Controller
@RequestMapping("/category")
public class CategoriesController {

	// maps to "/category/"
	@RequestMapping("/")
	public String showAll(Model model) { [...] }

}

@RequestMapping("/category/{category}")
public class CategoryController {

	// maps to "/category/{category}/"
	@RequestMapping("/")
	public String showCategory(@PathVariable Category category, Model model) { [...] }

	// maps to "/category/{category}/download"
	@RequestMapping("/download")
	public String downloadCategory(@PathVariable Category category, Model model) { [...] }
}

@Sam-Kruglov
Copy link

@Sam-Kruglov Sam-Kruglov commented Nov 30, 2020

@rstoyanchev It's not a big deal of course but you have to copy-paste the instance variables and the URL prefix. This would just be a nice little addition with low impact

@rstoyanchev
Copy link
Contributor

@rstoyanchev rstoyanchev commented Nov 30, 2020

Yes it would be a nice little addition but I'm also concerned with issues for existing apps that rely on the current behavior. As for the instance variables, the above could be an inner or nested class just as well.

@Sam-Kruglov
Copy link

@Sam-Kruglov Sam-Kruglov commented Nov 30, 2020

that's right, that actually would be a breaking change. I'm not sure if classpath scanning would work on an inner class though.
Well, I guess breaking change isn't worth it, so thanks for your time!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web status: declined type: enhancement
Projects
None yet
Development

No branches or pull requests

3 participants