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

support for providing a path component on a package-info [SPR-14667] #19231

Closed
spring-projects-issues opened this issue Sep 3, 2016 · 11 comments
Closed
Assignees
Labels
in: web status: duplicate type: enhancement

Comments

@spring-projects-issues
Copy link
Collaborator

@spring-projects-issues spring-projects-issues commented Sep 3, 2016

Caleb Cushing opened SPR-14667 and commented

could have (pseudocode)

com.xenoterracide.rpf.api.package-info @RequestMapping( path = 'api' )

com.xenoterracide.rpf.api

@RestController
@RequestMapping( path = 'strings' )
class Strings {

    @RequestMapping( path = "/{id}" }
     String get( Long id ) {
        ...
     }
}

this would allow us to call this by doing /api/strings/1

This is perhaps easier than adding additional servlet mappings or prepending static strings.


Issue Links:

  • #20467 Handle @RequestMapping at package level ("is duplicated by")
  • #20883 Ability to provide an external base path for controllers ("is superseded by")

0 votes, 5 watchers

@spring-projects-issues
Copy link
Collaborator Author

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

Rossen Stoyanchev commented

Sounds alright as an idea but there can be ambiguity in other arrangements. For example @RequestMapping methods inherited from a base class, or even @RequestMapping methods from an interface implemented by the controller. Would we look at the package of the class or its super types which could be in a different package. In the case of interfaces, with a JDK proxy in front of the controller class, Spring MVC doesn't even know what the actual controller type is (it's behind a proxy) so it would have to check the package of the interface and furthermore, theoretically at least it could be more than one interface.

I would prefer a more stable arrangement that works regardless of the scenario and where seemingly unrelated changes (e.g. moving @RequestMapping annotations from base class down or up) cause a different prefix to be picked.

What is your primary concern? Given the route constants you showed here isn't it preferable to more explicitly see the structure of the URl even if the constants appear in many controllers?

@spring-projects-issues
Copy link
Collaborator Author

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

Caleb Cushing commented

Given the route constants you showed here isn't it preferable to more explicitly see the structure of the URl even if the constants appear in many controllers?

I mean spring logs that currently though I notice it doesn't take that into effect, also that doesn't really work anyways because of path components on methods.

one of the advantages of this is you could actually move things around... for example if you designed a "component" as a "feature" jar, you could potentially add that jar to any app and have it work. Abstracts don't prevent that, but they can make certain flexibilities harder, such as if you need an Abstract for all of your controllers to prepend a base url (in my case I've chose /v0 over /api), now it's harder to make a "component" jar, because it needs access to that Abstract. Also we could talk about how awesome inheritance is for reuse.

I'm basically trying to come up with the solution to my "component" jar issues and wanting to prepend /v0 to all of my controllers in the final app jar, and "mount" controllers that need authentication to "/v0/private" and ones that don't to "/v0/public" all preferably while having written and tested the controllers in a way that it only has to know it's the /files controller, and doesn't care that it's going to end up at /v0/private/files in the end. As shown there are ways to accomplish this.

Having a route constants file isn't really better than having a centralized router, and will quickly grow unwieldy.

I think there are other solutions, this one just made sense to me, at least initially, as mentioned it might be possible to do something with annotations or some route configurer or both. maybe

@RequestPath("v1")
@RequestPath("public")
@RequestPath("files")
class ... 

or

routeConfigurer.setBase( "v1")
routeConfigurer.setPrefixForControllersWithAnnotation( "public", Public.class ); // Public.class being something I have to write

basically this ticket may not be the solution but it's an idea. Did this help clarify the problem?

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Sep 9, 2016

Rossen Stoyanchev commented

Just checking as it seems like a property placeholder based prefix could do the trick:

@RequestMapping("${public}/registration")

Then configure public in a property file to be "/v0/private" in production but possibly no value in development via environment profiles.

Similarly for private:

@RequestMapping("${private}/files")

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Sep 9, 2016

Caleb Cushing commented

even when use a property name I know works, tests throw

java.lang.IllegalArgumentException: Not enough variable values available to expand 'basePath'

because that's treated as a path variable and not an environment property (oops for overlapping syntax)

though this variant is interesting

@RequestMapping( "${${basePath}}" )
Caused by: java.lang.IllegalArgumentException: Could not resolve placeholder '/v0' in string value "${${basePath}}"

also isn't this that same thing in the other ticket that doesn't work for hateoas, which I am also using and need to work.

The other thing of course, I don't like properties for this, because properties are something an admin can/will change (having been an admin at one time, you modify config files, sometimes experimentally, which property files in spring boot are). So I'd prefer something that can be set outside of the PropertySourceConfigurer... whatever lookup path. I think a property is a great idea too, but this is how I do the same thing for spring data rest, just for this reason

@Configuration
class RestConfig extends RepositoryRestConfigurerAdapter {

    @Override
    public void configureRepositoryRestConfiguration( final RepositoryRestConfiguration config ) {
        config.setBasePath( RouteConstants.PRIVATE );
        config.setReturnBodyOnCreate( false );
        config.setReturnBodyOnUpdate( false );
    }
}

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Sep 9, 2016

Rossen Stoyanchev commented

Indeed the Spring Hateoas ControllerLinkBuilder and also the Spring Framework MvcUriComponentsBuilder do not resolve property placeholders that may be present (and are allowed) in request mappings. That would be quite easy to fix.

If you don't want admins to mess with your properties, which are actually built into the jar, you can register properties programmatically through your own custom PropertySource http://docs.spring.io/spring/docs/current/spring-framework-reference/htmlsingle/#beans-property-source-abstraction.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Sep 9, 2016

Caleb Cushing commented

don't all PropertySources go through the same merger? I recently retooled our apps configuration (which is separate from my hobby app) to use @PropertySource, and the newer (3.1?) PropertySourceConfigurerMerger (or whatever, seriously can't remember that classes name). I believe they would all still get squished together so environment variables and system properties would be considered first.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Sep 9, 2016

Caleb Cushing commented

I think that ideally, the controller shouldn't need to know that it's getting configured, in my workaround, and in the property source solution you end up with the controller having a dependency on something. I suppose in the annotation solution you have a dependency too... though annotations are perhaps much more flexible, and you can already extend @Component, or @Controller, etc, to provide different meanings to your app, and postprocess those differently, so it would seem natural to me to extend @RestController with @PublicRestController, and then use that to configure all beans like that with a prepended path. Obviously I also think if one goes with the configurer route, then Spring Boot would probably end up adding a property for that option if someone wanted to use that too.

I think that many of us probably have the issue of wanting to have /base/path that's not defined in the controllers themselves. It just depends on how much work Spring is willing to put in and narrowing down the real set of criteria.

  1. needs to be configurable external to the controller, so that changing the basepath doesn't mean changing the controller, or unit tests (e.g. if I wanted to change a public controller to private)
  2. need to be able to configure different controllers with different base paths (makes using spring security currently easier, though I have a ticket for making that decentralized too)
  3. optionally admins should not be able to change the url by means of external property sources
  4. needs to work with spring security mvcMatchers, and hateoas, and spring boots defaults (I have concern that replacing the PropertySourcesPlaceholderConfigurer... would affect spring boots ability to normally do 12 factor configuration)

@spring-projects-issues
Copy link
Collaborator Author

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

Rossen Stoyanchev commented

Resolving this as duplicate of #20883 which is now scheduled for 5.1.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented May 25, 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

Oliver Drotbohm commented

It might actually be a chance to simply some our own Spring Data REST base path configuration.

@spring-projects-issues
Copy link
Collaborator Author

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

Rossen Stoyanchev commented

Oops, looks like I commented on the wrong ticket. The one here was resolved as duplicate of #20883 and that's where I meant to post it.

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

No branches or pull requests

2 participants