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

Ability to provide an external base path for controllers [SPR-16336] #20883

Closed
spring-projects-issues opened this issue Dec 30, 2017 · 20 comments
Closed
Assignees
Labels
in: web type: enhancement
Milestone

Comments

@spring-projects-issues
Copy link
Collaborator

@spring-projects-issues spring-projects-issues commented Dec 30, 2017

Caleb Cushing opened SPR-16336 and commented

I'm certain wrote, or commented on a similar bug that I currently can't find. I'd like the ability to do this with classes annotated with @Controller, and perhaps separately for @RestController.

@Configuration
class RestConfig extends RepositoryRestConfigurerAdapter {

    public void configureRepositoryRestConfiguration( RepositoryRestConfiguration config ) {
        config.setBasePath( Routes.PRIVATE );
    }
}

in this way one could write a generic controller with a known subpath, such as /register, but then attach it in the final application as, in my case, /v0/public/register. Suggestions for more flexibility might be a way to setBasePath, explicitly based on annotation.

so you could write config.setBasePath( PublicController.class, "/v0/public" ) where @PublicController is an extension of @Controller, or maybe it'd just be a good idea to allow it for the specific class instead

one of the reasons I have this problem is I'm modularizing all of my components, but now I have to ensure that these paths are configured properly for hateoas before writing a controller, because the controllers path is static, thus changes to hateoas roots for links have to be made before the controller for tests. If I could change the base path of the controller later then I could bundle the controllers under a configuration, and they'd be more reusable modules between apps, and completely configurable.


Affects: 5.0.2

Issue Links:

  • #20691 "No matching handler" when override method getServletMapping() in AbstractAnnotationConfigDispatcherHandlerInitializer"
  • #19231 support for providing a path component on a package-info ("supersedes")
  • #18455 Support default URI prefix for web service @RequestMapping ("supersedes")
  • #20467 Handle @RequestMapping at package level ("supersedes")

Referenced from: commits 58cce61, 928b780, 86c8615, d196cdc, 19dc981, 31159a8, e6fef95

2 votes, 7 watchers

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jan 2, 2018

Rossen Stoyanchev commented

Indeed this issue has been raised in several tickets.

  • #19231 has the idea for @RequestMapping to be supported in package-info although that alone doesn't really help to express a "prefix" (vs another way to inherit). The same ticket also mentions the idea of a separate annotation + describes a use case.
  • #18455 expresses preference for a "building block" (base controller) approach including more details around motivation.
  • #20691 is yet another example in the context of a WebFlux application and also shows that one approach used is to insert a prefix through the Servlet mapping.

Aside from the ability to assign a common prefix, a second common thread is the ability to choose what prefix to apply to which controllers.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jan 2, 2018

Caleb Cushing commented

a comment I want to make in regards to a package-info, or abstract base class, although I don't hate either idea, and I think they're good ideas, I think they don't work well in a post Jigsaw world for what I want. Reason being is is that jigsaw exports packages in a globally unique way, and abstract base class means a dependency. In a post jigsaw world it would be ideal to allow remapping when composing jigsaw modules so that the module can be reused (perhaps even ideally within the same application, e.g. remapping the same thing many times)

thought, pseudocode, nothing is saying this is the right design for the problem, just that it could show something powerful

@Controller("/user")
class UserCtrl{...}

RouteMapper {

    @Prefixes
    configurePrefixes( RouteMapper routes ) {
      routes.prefix( "v0", UserCtrl.class );
      routes.prefix( "v1", UserCtrl.class );
      routes.prefix( "v1", RestController.class );
    }
}

resulting in

/v0/user
/v1/user

update, immediately thought better of wanting templates in this, that sounds like a major pain if writing generic controllers

if class provided is an annotation, apply to all controllers annotated as such.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented May 29, 2018

Rossen Stoyanchev commented

My current plan to experiment with:

  1. PathMatchConfigurer allows the registration of prefixes (e.g. "/api", "/public", etc.) for subsets of controllers targetted by base package, assignable from class, or by annotation, i.e. similar to the the attributes on @ControllerAdvice that specify what controllers the advice applies to.
  2. RequestMappingHandlerMapping is configurable with that information, and while detecting and registering request mappins, it checks and applies those prefixes to matching controllers.

In other words a simple mechanism to enrich request mappings with a prefix, as part of the normal process of registration.

Note that anything that builds links to controllers (MvcUriComponentsBuilder, or the ControllerLinkBuilder from Spring HATEOAS) will need to be enhanced in order to be aware of those prefixes.

Spring Boot Actuator, which shows request mappings, will probably be okay, since it gets those through RequestMappingHandlerMapping. /cc Andy Wilkinson

I think Spring Security should be okay as far as I can think. /cc Rob Winch

Oliver Drotbohm, Greg Turnquist, any thoughts from a Spring Data REST perspective, whether this is helpful and/or require changes?

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented May 29, 2018

Andy Wilkinson commented

Thanks for considering the possible effects on Boot, Rossen.

Actuator doesn't just use RequestMappingHandlerMapping. It looks at all of the HandlerMapping instances that are available from a DispatcherServlet or DispatcherHandler. There's specific logic for describing the following types:

  • org.springframework.web.servlet.mvc.method.RequestMappingInfoHandlerMapping
  • org.springframework.web.servlet.handler.AbstractUrlHandlerMapping
  • org.springframework.data.rest.webmvc.support.DelegatingHandlerMapping
  • org.springframework.web.reactive.result.method.RequestMappingInfoHandlerMapping
  • org.springframework.web.reactive.handler.AbstractUrlHandlerMapping
  • org.springframework.web.reactive.function.server.support.RouterFunctionMapping

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jun 6, 2018

Rossen Stoyanchev commented

Thanks Andy, I think for now this will apply to annotated controllers only.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jun 7, 2018

Rossen Stoyanchev commented

Support for path prefixes is now available in master. It's configurable directly on RequestMappingHandlerMapping and is also exposed through WebMvcConfigurer.

Caleb Cushing, and everyone else, if you have a chance please take a look to see if this meets your needs.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jun 8, 2018

Stanislav Bytsko commented

Given:

configurer.addPathPrefix("/api", HandlerTypePredicate.forAnnotation(Controller.class)); 
  1. Will this affect RestController-annotated beans?
    I think it should, and tests should reflect that (19dc981)
  2. Will this affect error controller in spring boot?
@Controller
public abstract class JsonErrorController implements ErrorController {
    @Override
    public String getErrorPath() {
        return "/error";
    }

    @RequestMapping("/error")
    public ResponseEntity<JsonApiErrors> error(HttpServletResponse response) {
        ... custom error
    }
}

I don't see how not, but it would be nice to exclude this behaviour somehow, so that error will be forwarded correctly

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jun 8, 2018

Rossen Stoyanchev commented

First off, thanks for taking a look.

The HandlerTypePredicate was extracted from the code we've had for @ControllerAdvice and its attributes for targeting specific subsets of controllers. The idea is the same, i.e. you use one or more selectors to define a set of controllers, and the framework takes that as input, not trying to guess further. In any case the Spring Framework cannot depend on anything Boot related, it's the other way around.

So yes if you target @Controller that will include every controller known to the ApplicationContext in which case you might as well use HandlerTypePredicate.forAnyHandlerType(). Annotations would typically work better if they're more specific, e.g. @RestController at least for REST vs HTML controllers, or perhaps something even more specific like @RepositoryRestController, or you could also use packages and assignable types.

 

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jun 8, 2018

Caleb Cushing commented

looking at the sample here, and through the code a little bit. HandlerTypePredicate isa Predicate but it looks like all the code requires you to use a HandlerTypePredicate. Is it be possible to write (pseudocode)

HandlerTypePredicate.forPackage("com.xenoterracide").and(HandlerTypePredicate.ForAnnotation(Controller.class)

such that it requires the union of both and not just one or the other? also what is the behavior if you add, multiple paths, especially if they overlapp (RestController, isa Controller), what happens if I swap the order when adding?

"/ui/", HandlerTypePredicate.forAnnotation(Controller.class)
"/api/", HandlerTypePredicate.forAnntoation(RestController.class)

 

and lastly will I be able to do this

@Controller
@RequestMapping("")
MyClass {...}

"/api/user", HandlerTypePredicate.forClass(MyClass.class)
"/api/other", HandlerTypePredicate.forClass(MyClass.class) 

such that I can use the same controller twice, at different endpoints.

I will take a closer look this weekend.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jun 8, 2018

Rossen Stoyanchev commented

Yes I should change the config to accept Predicate<Class<?>> and then it should be possible to combine multiple predicates with .and(..) and .or(..), or to use any predicate.

For the order, the first matching prefix-predicate pair wins. This is mentioned this in the Javadoc.

As for registering the same controller twice, the answer is no. This does not declare or controller what controller instances are registerred, rather it intercepts mappings as they are being registered, and tries to match based on controller type + enrich the mapping patterns. There would have to be two MyClass.class beans to begin with, and then there would have to be a way to differentiate between the two either by type, or more likely through the bean name as a qualifier, e.g.:

public class HandlerBeanPredicate implements BiPredicate<String, Class<?>> {
    // ...
}

Do you have use case like that or just checking?

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jun 8, 2018

Caleb Cushing commented

Do you have use case like that or just checking?

I do actually, and it's probably not all that different from spring data rests's general use case, at some point you can write controller code so generic, that it would work if attached in multiple places. In this case I believe you would have to look at the Generics of the class though... or maybe some other parameter... like a class passed to a constructor... thinking outloud.

For example, I'm currently needing to write endpoint code for JSON Schema v4, so that I have endpoints that conform to SDR, but use non-persistable DTOs. The only difference between the controller code in this case would be the DTO class. everything else about this would just be reflective, and accessed at different endpoints.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jun 8, 2018

Rossen Stoyanchev commented

I switched to using Predicate<Class<?>> and also added some extra tests. That means you can also use a custom predicate.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jun 8, 2018

Caleb Cushing commented

As for registering the same controller twice, the answer is no. This does not declare or controller what controller instances are registerred, rather it intercepts mappings as they are being registered, and tries to match based on controller type + enrich the mapping patterns. There would have to be two MyClass.class beans to begin with, and then there would have to be a way to differentiate between the two either by type, or more likely through the bean name as a qualifier, e.g.:

public class HandlerBeanPredicate implements BiPredicate<String, Class<?>> {
    // ...
}

also yeah, in the 2nd comment of the thread I added this

@Controller("/user")
class UserCtrl{...}

RouteMapper {

    @Prefixes
    configurePrefixes( RouteMapper routes ) {
      routes.prefix( "v0", UserCtrl.class );
      routes.prefix( "v1", UserCtrl.class );
      routes.prefix( "v1", RestController.class );
    }
}

resulting in

/v0/user
/v1/user

though thinking about it, this should work as is right? same controller just different endpoint...

.... though considering the BiPredicate a bean name would be fine, or maybe even more flexibly the bean instance (is that easy? maybe not?, would it be slow?). I think when I originally wrote this I had tried BeanNameUrlMapper, but it was deficient in some way, maybe having /path/, or possibly route parameters

... if you'd like I can try to come up with a useful use case for an @Bean of the same type being mapped this way.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jun 12, 2018

Rossen Stoyanchev commented

We iterate over bean names so no access to instances, which may not have been created. Looking at the code, bean names are also currently not available in the method that extracts information from @RequestMapping. We'd have to jump through some hoops to make the bean name available. Is it feasible to extend the controller as needed? Also worth mentioning RequestMappingHandlerMapping has a public API for registering request mappings (registerMapping and unregisterMapping).  

 

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jun 12, 2018

Caleb Cushing commented

Is it feasible to extend the controller as needed?

yes, which means I could also just add a different @RequestMapping on the class...

Also worth mentioning RequestMappingHandlerMapping  has a public API for registering request mappings

I didn't find any mention in the reference docs, most of the googles are using xml config and the javadoc is kind of vague as to how to use it.

https://docs.spring.io/spring-framework/docs/current/javadoc-api/org/springframework/web/servlet/handler/AbstractHandlerMethodMapping.html#registerMapping-T-java.lang.Object-java.lang.reflect.Method-

I'm not opposed to relying on RequestMappingHandlerMapping for the last use case, but it'd be nice to see some better documentation for how to make use of it.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jun 12, 2018

Rossen Stoyanchev commented

Good idea. I'll add some around here.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jun 12, 2018

Rossen Stoyanchev commented

Done. While the snapshot is building, you can see the documentation source.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jun 12, 2018

Caleb Cushing commented

I think I can be happy with that. I did make 2 comments on your source diff, I think you may have typoed/copy pasta-ed the class name instead of the arg name. But that's a minor fix.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jun 12, 2018

Caleb Cushing commented

btw, thanks for all the hard work, and acceptance of feedback, this looks awesome! I need to give it a go soon.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jun 15, 2018

Rossen Stoyanchev commented

I've updated MvcUriComponentsBuilder to be aware of the configured path prefixes. I'm going to mark this resolved for now but please do comment when you give it a try.

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

No branches or pull requests

2 participants