Enhance UrlHandlerFilter behaviour and capabilities

[jamesmissen]

Spring Module and Version

Spring Web – v7.0.1

Issue Description

I have a few suggestions regarding improvements to parts of the UrlHandlerFilter implementations. They are not necessarily requests, but just areas that I'm enquiring about to gauge whether they might be valid places for enhancements.

1. Handle patterns by match specificity.

   Currently, the handlers are checked in the order in which they were added to the builder, using the supports method of the handler, until there is a match, and then checking the PathPatterns in order, until there is a match. It might be better to match based on the how well the pattern matches the request path instead. This would be similar to how resource handler patterns are matched (i.e. with AbstractUrlHandlerMapping.lookupHandler).

   The use case would be something like with:

   UrlHandlerFilter filter = UrlHandlerFilter
      .trailingSlashHandler("/**").mutateRequest()
      .trailingSlashHandler("/some/specific/path/**").redirect(HttpStatus.PERMANENT_REDIRECT)
      .build();
   

   In which you would want /some/specific/path/page/// to redirect instead of mutate. Currently it would mutate as the mutate handler is the first supported handler in the handlers map, and /** is the first matching pattern.

2. Check that the HTTP status code is valid for redirect handlers.

   Currently, whatever code passed into the redirect handler, e.g. .redirect(HttpStatus.PERMANENT_REDIRECT), will be used as the response status code alongside the Location header. Although maybe not a major issue, maybe an error should be thrown or a warning should be logged in the case that a non-3XX code is provided, given that the response produced would have Location header with a non-3XX status.

3. Support additional specs for handling cases other than trailing slashes.

   The only handler type currently is the TrailingSlashSpec. I know that the UrlHandlerFilter was added mainly as a replacement for the now-deprecated trailing slash match. However, if the thought is that the UrlHandlerFilter is a general convenience feature for configuring url handling, there might be other ways that request paths might want to be mapped by users.

   The main one I can think of would be replacing multiple slashes with a single slash, with mutation or redirection options in the same way as with TrailingSlashSpec.

   Something similar already exists with resource handlers, as the ResourceHttpRequestHandler and ResourceWebHandler both use ResourceHandlerUtils.normalizeInputPath(path) before locating a resource.

   This means that, for example, if a resource handler is registered with the pattern /resources/**, both https://example.com/resources/styles/home.css and https://example.com/resources/styles////home.css can actually be used to find the same resource. This creates a behaviour that is similar to a handler created by .mutateRequest().

For 1 and 3, it might be better to refactor the MultiValueMap<Handler, PathPattern> handlers to something like Map<PathPattern, Handler> handlers to facilitate finding the most appropriate handler for a request.

This is analogous to how AbstractUrlHandlerMapping is implemented with it's Map<PathPattern, Object> pathPatternHandlerMap (standard variant) or Map<PathPattern, Object> handlerMap (reactive variant).

[rstoyanchev]

I think these are reasonable enhancement suggestions that could be added right away in 7.0.x as follows. For 1), we could use a flag such as useSpecifityOrder to make it opt-in and backwards compatible. 2) is just an assertion that shouldn't have side effects. 3) is just new methods so it should be okay.

Let me know if you intend to shaped this into a PR. Either way thanks for the suggestions.

[jamesmissen]

Upon reflection, I think I'll walk back on suggesting the third enhancement (i.e. removing duplicate slashes) for a few reasons:

• For removing trailing slashes, the use cases would be (1) typing a URL into the browser, and (2) having a URL coded somewhere (e.g. in an HTML href/src, or in an API client library). When trailing slash matching was deprecated, some developers' prior usage of the latter is what seemed to have resulted in their frustration, when they ended up facing unexpected 404s.

  It would be reasonable for someone to code a URL with trailing slash somewhere for an endpoint without one, especially when the previous path-matching behaviour wouldn't have indicated any discrepancy. On the other hand, if someone has typed or coded consecutive slashes somewhere, it's much more likely to be either a typo or intentional.

• Anyone who wants this behaviour can create their own filter or external proxy tailored to their needs. There is probably less of an argument for incorporating such a utility into the Spring framework for only a few developers' benefit.

• Most other web frameworks don't provide such a utility (I investigated a few). As mentioned before, developers can easily create their own-such behaviour if desired using other methods. As a side note, many of the web frameworks I looked into do offer some kind of trailing slash configuration option.

• The current setup of UrlHandlerFilter wouldn't make this as easy to implement as it first seemed. In particular, it uses PathPattern, which can't match paths with a variable number of consecutive slashes anyway. You would have to strip duplicates from the path prior to matching, or use a pattern that has a catch-all segment (but even then, duplicate slashes before the catch-all wouldn't match).

  E.g. the pattern /foo/bar/baz/** wouldn't match /foo///bar/baz/index.html, and patterns like /abc/123 or /xyz/*/789 would be useless.

[jamesmissen]

For 1 and 2, I have come up with some code changes, and I can open a PR if you’d like. I’m wondering though, would you prefer it to be broken into separate PRs for the two separate enhancements? In addition, would you prefer separate PRs for servlet changes and reactive changes?

[rstoyanchev]

If path normalization such as removing duplicate slashes isn't done as early as possible, it can lead to different interpretations and that in turn can lead to security issues. However, it would be okay if the path is normalized at the very beginning of the filter chain before anything else. A parsed RequestPath exposes a list of elements that includes separators, so it would be easy to recreate the path without duplicate separators.

That said, I agree 1) and 2) are the main goal for concrete outcomes while 3) could be more of a thought exercise to ensure the design is flexible enough to allow a custom handler to do that.

Please, go ahead with opening a PR.