Provide public methods to register and un-register handler method mappings [SPR-11541]

[spring-projects-issues]

cemo koc opened SPR-11541 and commented

We are heavily using Spring MVC and currently we had to duplicated a fair amount codes to provide necessary functionalities. We are providing redirection mappings by database. At runtime we periodically checking for any kind of operation such deletion or insertion and want to register them by RequestMappingHandlerMapping.

Spring's RequestMapping is nice but does not provide easy way to provide addition or deletion of RequestMapping's. In order to add a new mapping, RequestMappingHandlerMapping has to be extended as this:

public class RequestMappingInfoAppendableHandlerMapping extends RequestMappingHandlerMapping {

   @Override
   @Override
   public void registerHandlerMethod(Object handler, Method method, RequestMappingInfo mapping) {
      super.registerHandlerMethod(handler, method, mapping);
   }
}

I still did not find a beautiful way to unregistering mappings. What I am suggesting:

Please provide necessary util classes and public methods to registering and unregistering request mappings.

By the way I have already tried other HandlerMapping implementations but I do not want to change priority because of the performance cost. Also a custom HandlerMapping after RequestMappingHandlerMapping is not capable of handling because of our mappings at RequestMappingHandlerMapping.

Edit:

I had also implemented a very basic utility function for creating new MappingInfo's. I believe that such convenient methods will be very useful. A Builder can be great as well.

public final class RequestMappingInfoUtils {

   public static RequestMappingInfo newInstance(String[] urls){
      return newInstance(urls, RequestMethod.GET);
   }

   public static RequestMappingInfo newInstance(String[] urls, RequestMethod requestMethod){
      return new RequestMappingInfo(new PatternsRequestCondition(urls),
                                    new RequestMethodsRequestCondition(requestMethod),
                                    null,
                                    null,
                                    null,
                                    null,
                                    null);
   }
}

---

Affects: 3.2.8, 4.0.2

Issue Links:

• INT-3713 Fix RequestMapping logic according latest Spring MVC changes ("is depended on by")
• Pointcut evaluation fails against AbstractHandlerMethodMapping$MappingRegistry with AspectJ 1.8.10 [SPR-15019] #19586 Pointcut evaluation fails against AbstractHandlerMethodMapping$MappingRegistry with AspectJ 1.8.10
• Add protected method to StandaloneMockMvcBuilder to register additional MVC infrastructure components [SPR-17520] #22052 Add protected method to StandaloneMockMvcBuilder to register additional MVC infrastructure components

0 votes, 6 watchers

[spring-projects-issues]

Rossen Stoyanchev commented

It sounds like what you need is (correct me if that's not all):

1. public methods to register/unregister handler method mappings
2. convenient ways to create an instance RequestMappingInfo

Regarding (1), the RequestMappingHandlerMapping was not designed to support concurrent modification of mappings at runtime. It is worth making that possible for other reasons as well so I see this as the main outcome of this ticket.

Regarding (2), a RequestMappingInfoBuilder would probably be the the way to go. That said this is an SPI, not expected to be used heavily and frequently justifying but we'll consider adding a builder.

By the way I have already tried other HandlerMapping implementations but I do not want to change priority because of the performance cost

Curious to hear on the performance cost trade-offs. Especially as it relates to #16168. I would think for redirect mappings a SimpleUrlHandlerMapping ahead of RequestMappingHandlerMapping would work fine. As long as most redirect mappings are non-pattern URLs, it should be a fast lookup into the URL map. This is at least how I imagine #16168 will be implemented.

Even with pattern URLs I don't see why registering URL / HTTP methods with RequestMappingHandlerMapping instead is any faster than SimpleUrlHandlerMapping. I suspect the latter may even be a little faster since RequestMappingHandlerMapping does extra checks and may also iterate twice when there is no match to generate a useful exception and response.

[spring-projects-issues]

cemo koc commented

It sounds like what you need is (correct me if that's not all):

1. public methods to register/unregister handler method mappings
2. convenient ways to create an instance RequestMappingInfo

Exactly.

Curious to hear on the performance cost trade-offs. Especially as it relates to #16168. I would think for redirect mappings a SimpleUrlHandlerMapping ahead of RequestMappingHandlerMapping would work fine. As long as most redirect mappings are non-pattern URLs, it should be a fast lookup into the URL map. This is at least how I imagine #16168 will be implemented.
Even with pattern URLs I don't see why registering URL / HTTP methods with RequestMappingHandlerMapping instead is any faster than SimpleUrlHandlerMapping. I suspect the latter may even be a little faster since RequestMappingHandlerMapping does extra checks and may also iterate twice when there is no match to generate a useful exception and response.

I am trying to give an example.

RequestMappingHandlerMapping mappings:

1. /search/*
2. /customer/*
3. /*

SimpleUrlHandlerMapping mappings:

1. /favicon (returns 204)
2. /stupid-request (returns 204)
3. /old-request (returns 302)

If all requests are handled at first by RequestMappingHandlerMapping, my simpleUrlHandlerMapping urls are handled by RequestMappingHandlerMapping's "/*" mapping. This is a definitely an error. That is why I had to change order and SimpleUrlHandlerMapping are handled at first. We have more than 1000 distinct url and using a SimpleUrlHandlerMapping before RequestMappingHandlerMapping is a unnecessary step and causing a little trouble. It has its own pattern matcher etc... We have ViewControllerRegistry, ResourceHandlerRegistry and all of them must be handled before RequestMappingHandlerMapping.

I am expecting to modify DispatcherServlet to unify HandlerMapping's by this issue as well. Having distinct HandlerMapping is nice option but I am sure that unifying them in application can be good asset for SpringFramework. But filing an issue I will give a try and test with out application.

[spring-projects-issues]

Rossen Stoyanchev commented

Thanks for the additional detail.

We have more than 1000 distinct url and using a SimpleUrlHandlerMapping before RequestMappingHandlerMapping is a unnecessary step and causing a little trouble. It has its own pattern matcher etc...

As of 4.0.3 you can configure a UrlPathHelper and PathMatcher in the Java config as shown here. At the moment that only applies to RMHM but this conversation has made me realize we probably should apply it to all HandlerMappings created by MVC Java confg and I've created #16375 that we may even backport to 4.0.5. Perhaps that eliminates the trouble you're referring to?

Ideally the fact that the MVC java config creates different HandlerMapping's under the covers should not matter. In fact it allows changing the order for each group and treating separately. If you're introspecting your REST API metadata for any reasons, it's also useful to keep them separate. Indeed for #16168 we'll likely stick to that same approach and create a dedicated SimpleUrlHandlerMapping but order it ahead of @RequestMapping methods. Hopefully we can make it work for you as well.

Also since you mentioned dynamically providing redirection mappings through a database, how does that relate to your request under #16168 which is for static configuration at startup?

[spring-projects-issues]

cemo koc commented

Please correct me If I say something wrong:

DispatcherServlet iterates all registered HandlerMappings. Each HandlerMapping tries to find requested URL by trying org.springframework.web.servlet.handler.AbstractUrlHandlerMapping#lookupHandler or an equivalent implementation.

This implementation checks both a direct match or pattern match. I am assuming direct match is pretty straight forward and costless but pattern match is expensive. My order will be:

1. SimpleUrlHandlerMapping (based on ViewControllerRegistry)
2. Add support for 404, 204, and 3xx responses to the MVC config [SPR-11543] #16168 based new HandlerMapping
3. SimpleUrlHandlerMapping (based on ResourceHandlerRegistry)
4. RequestMappingHandlerMapping

I am afraid of increasing number of HandlerMapping's can cause performance penalty. My 99.99% requests are handled by RequestMappingHandlerMapping but in order to find correct handler I need to iterate all handler implementations. Please forgive my ignorance and correct me If I say something wrong. There are many requests which are directly handled by RequestMappingHandlerMapping but in this order they have to be checked by 1,2,3 and third step as a both direct and pattern match.

Also since you mentioned dynamically providing redirection mappings through a database, how does that relate to your request under #16168 which is for static configuration at startup?

We have a very dynamic environment and serving videos. There are some cases in which videos can be deleted by our editors. But deleting a URL can cause a SEO penalty. Instead of deleting directly, our editors deleting and redirecting these deleted URL's to correct one. They are providing necessary information through our database. Our applications taking these data and registering them with a our RedirectController. It would be great to delete or add new URL's on-demand at runtime. A scheduled job can check modifications and provide them to our applications.

[spring-projects-issues]

Rossen Stoyanchev commented

Your description is accurate. The order looks fine too.

I don't think having more than one HandlerMapping will have a negative impact. (1) and (2) should be simple and fast lookups into a map. As for resource handling, they are usually very few of those, perhaps 1 or 2 prefix-based patterns -- I don't believe resolving them separately will have a negative impact since for patterns you have to iterate all anyway . If anything I expect it might be more efficient since the algorithm for @RM methods is more involved. Of course with performance it's always best to confirm.

The requirement for dynamic redirect configuration is good to know. In fact what I stated earlier for the design of RMHM with regards to runtime modifications is also true for SimpleUrlHandlerMapping. So we need to consider both.

[spring-projects-issues]

Rossen Stoyanchev commented

I've updated the title to reflect the main goal for this ticket with RequestMappingInfo builder a much more minor secondary goal.

[spring-projects-issues]

cemo koc commented

I can not see why (1) and (2) are simple? (2) can include ant style pattern mappings. org.springframework.web.servlet.handler.AbstractUrlHandlerMapping#lookupHandler is looking pattern match after direct match checks. Almost all requests which is handled by RequestMappingHandlerMapping also be checked by other registered mappings. This means that direct matches for RequestMappingHandlerMapping also be checked by other registered mappings.

A more effective algorithm for DispatcherServlet can be checking direct matches for all handler then It can check pattern matches. But I do not see this doable. I am considering to replace all non-RequestMappingHandlerMapping to a RequestMappingHandlerMapping and unifying them might be a better idea. I have not checked yet but this is will definitely decrease necessary time for direct matches.

[spring-projects-issues]

Rossen Stoyanchev commented

I can not see why (1) and (2) are simple? (2) can include ant style pattern mappings.

You're correct. I had assumed (incorrectly) non-pattern mappings.

Nevertheless consolidating mappings into RMHM is not an option from a framework perspective. HandlerMapping is central Spring MVC strategy relied on in more ways than we can imagine. Nor do I think it's a good idea even for an application to do this as an optimization step not without substantial evidence. For once having mappings organized by category provides more control over ordering.

I am wondering if the catch-all "/\*" in RMHM isn't locking you in a situation where all other mappings have to be before it or part of it. You mentioned 99% of requests are for @RM methods, so perhaps this would be appropriate :

1. View controllers
2. RequestMappingHandlerMapping
3. Resource handlers
4. Status controllers (Add support for 404, 204, and 3xx responses to the MVC config [SPR-11543] #16168)
5. Catch all

The order of (2) and (3) can vary depending on the ratio of resource vs @RM requests and how often RMHM has direct hits but you get the idea.

Regarding catch-all configuration, currently we have WebMvcConfigurer.configureDefaultServletHandling which is a catch-all option for delegating to the default servlet. I wish it had been named more generally but nevertheless we can consider turning it more formally into a a general catch-all mechanism, e.g. provide your own HttpRequestHandler instead of the DefaultServletHttpRequestHandler used currently.

[spring-projects-issues]

cemo koc commented

Unfortunately "/*" is locking me and all handlers has to be handled before RMHM.

Nevertheless consolidating mappings into RMHM is not an option from a framework perspective. HandlerMapping is central Spring MVC strategy relied on in more ways than we can imagine. Nor do I think it's a good idea even for an application to do this as an optimization step not without substantial evidence. For once having mappings organized by category provides more control over ordering.

You are definitely right about implications of consolidating mappings. My assumptions are heavily based on dummy observations. I will provide concrete numbers after you will resolve this issue.

Thanks

[spring-projects-issues]

cemo koc commented

Any chance for 4.2 :) ?

[spring-projects-issues]

cemo koc commented

When I first opened this issue, I needed this feature only for RequestMappingHandlerMapping. Now I started to think that this would be a good feature for other HandlerMappings as well.

I am offering to add register and unregister method in HandlerMapping for 4.2. If I checked correctly It seems that only AbstractUrlHandlerMapping and RequestMappingInfoHandlerMapping will be required to implement these methods. Current situation is pretty good to be refactored to support these methods. I really think that un/registering handlers in runtime would be a really cool feature.

[spring-projects-issues]

Juergen Hoeller commented

Alright, let's aim to provide something in the 4.2 timeframe here...

Juergen

[spring-projects-issues]

Rossen Stoyanchev commented

There is now a builder in RequestMappingInfo and public register/unregister methods in the HandlerMapping. Please take a look and give it a try.

[spring-projects-issues]

cemo koc commented

Thank you so much Rossen. I will give a try this week.

[spring-projects-issues]

Rossen Stoyanchev commented

Re-opening. The changes broke Spring Boot here and here.

[spring-projects-issues]

Rossen Stoyanchev commented

I've pushed a fix that should resolve the two Boot issues.