Enhance UrlHandlerFilter with ordered pattern matching and status validation

[jamesmissen]

This PR addresses issue #35882.

It does the following:

1. Allows for ordering handlers based on path specificity, so handlers are selected based on how well a PathPattern matches the request path.

2. Validates the HTTP status code for redirect handlers, using a simple assertion to check if the code is 3xx.

The changes are backwards compatible:

1. A new useSpecificityOrder method is added to the Builder to opt in to specificity-based ordering. Not using this method (or using a value of false) results in the current behaviour.

2. A new exclude method is added to the Builder to optionally exclude URL handling for specific patterns. Not using this method results in the current behaviour.

3. For the servlet implementation, the parameter of the redirect method was changed from type HttpStatus to HttpStatusCode to align with the reactive implementation. This change will have no effect as HttpStatusCode is implemented by HttpStatus.

Feedback and edits are welcome, particularly regarding the implementation of specificity-based ordering, and the new method names (useSpecificityOrder and exclude).

[rstoyanchev]

Thanks for the changes.

The extraction of the lookupPath as a subpath is a breaking change. I appreciate the proposal, but as implemented it will break any application with a context path. This needs to be opt-in, e.g. excludeContextPath().

The idea with excludes was not in the original description and has not been discussed. If using specificity order, it seems like it can end up in the wrong order if mixed with all regular patterns. If using order of registration, one has to be careful to specify those first and they would always have to be checked first adding overhead. I don't quite see the use case for this in combination with trailing slash handling. Did this come from an actual use case? I would prefer to take it out and consider it separately, and as a general rule of thumb, it's best to split out extras like that so it doesn't hold up the rest of the PR.

For useSpecificityOrder, maybe sortPatternsBySpecificity().

For the HandlerRegistry abstraction, I didn't try but it seems the two implementations can be merged into one by using Map<PathPattern, Handler> where the implementation is either LinkedHashMap or TreeMap. That would remove the need for a HandlerRegistry abstraction in the first place, and significantly simplify the implementation.

[jamesmissen]

You're right about the lookup path. I adopted the same path matching as Spring Security, as that was a major reason why the filter was introduced (#28552).

Currently the filter matches include the context path. This is probably not the desired behaviour, as any path matching should be context path-invariant as it is elsewhere in Spring (e.g. in web routes, resource handlers etc.).

In addition, the current behaviour not being the same as Spring Security means that the /path/** pattern in the security matchers will not necessarily match the same paths as those matched by /path/** in this filter. For example with the filter:

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

• If the application has no context path configured, http://example.com/path/123/ will be redirected.
• If the application has /myApp as the configured context path, http://example.com/path/123/ will not be redirected, unless the filter pattern in the actual code is updated to /myApp/path/**.

Obviously the application code breaking when a different context path is used is likely not what anyone would want or expect.

I have changed this to opt-in as you suggested, with excludeContextPath(). However I would recommend this become the default behaviour in the future.

---

The excluded paths wasn't discussed, but the idea is simply that, just as this originally presented use case is valid:

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

an equally valid use case would be:

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

That is, it might be just as desirable to not handle trailing slashes for a more specific path, as it is desirable to handle them in a different way for a more specific path.

However I have removed this from the PR as suggested.

---

I have renamed useSpecificityOrder to sortPatternsBySpecificity as suggested.

---

As for the HandlerRegistry abstraction, I completely agree that using a Map<PathPattern, Handler> for both the current logic and the ordered logic would be ideal, as was the idea I mentioned in the issue. However unfortunately it wouldn't be possible to maintain the current logic while doing this.

Given the current behaviour is to look up handlers in the order that they added, refactoring to use PathPattern keys would mean that the original handler order information would be lost. The only way would be to have some additional object that keeps track of the original handler order, but doing this would defeat the purpose of not having a HandlerRegistry abstraction. You would still have to register and lookup handlers differently depending whether you're using specificity order or not, leading you right back to naturally wanting an abstraction for this logic.

Conversely, you could refactor the specificity ordering to also use the current MultiValueMap<Handler, PathPattern>, but that wouldn't be a very good idea. You would have to iterate over all of the PathPattern value lists for every request to find all of the matching patterns, and then from those find most specific pattern. This is even though the patterns really only need to be sorted by specificity order once.

It basically boils down to the fact that:

• the current/default logic determines how to filter based on the order of handlers; whereas,
• the new/specificity logic determines how to filter based on the order of path patterns.

So it makes the most sense to have Handler and PathPattern objects as the respective map keys for these different cases.

Even if you wanted the two cases to use the same map structure and remove the HandlerRegistry abstraction, you would still need different lookup logic based on the sortPatternsBySpecificity flag. As such, you wouldn't be gaining any simplification in the implementation. In fact, it would probably make it more convoluted and definitely make it more inefficient.

Hopefully that makes sense and it's clear why abstraction is the best approach. I know it looks and feels sizeable with the addition of the new classes, but that size is deceptive in conveying the actual complexity. Really, they're mostly just encapsulating maps. Java doesn't make it easy to be succinct.