Help users to register filters that require error or async dispatch with the correct dispatcher types

[wilkinsona]

See spring-projects/spring-framework#23958 and #18931 for some background.

The servlet spec's default for a filter's dispatcher types is REQUEST. Boot's servlet component scanning and FilterRegistrationBean both align with this default. This creates a trap for users where they declare a filter that requires async dispatch, such as Framework's ShallowEtagHeaderFilter, and rely on the default dispatcher types as the filter won't work correctly as it will be unaware of the completion of the async processing.

Ideally, the servlet spec would allow a filter to detect such misconfiguration but it does not do so. spring-projects/spring-framework#23958 has updated Framework's docs to note the need to register both ShallowEtagHeaderFilter and ForwardedHeaderFilter for async dispatch. This issue is to explore if we can do something in Boot. A few possibilities:

1. Automatically register for async dispatch any OncePerRequestFilter that returns false from shouldNotFilterAsyncDispatch().
2. Log a warning if a OncePerRequestFilter that returns false from shouldNotFilterAsyncDispatcher() is not configured for async dispatch.
3. Fail startup if a OncePerRequestFilter that returns false from shouldNotFilterAsyncDispatcher() is not configured for async dispatch.

These three possibilities all have their pros and cons and there may be other options too. Flagging for team attention to see what everyone thinks.

[rstoyanchev]

Note the same can be done for ERROR dispatches too, i.e. any OncePerRequestFilter that returns false from shouldNotFilterErrorDispatch().

[philwebb]

We'll go with option 1.

[wilkinsona]

I overlooked that fact that the shouldNotFilter…Dispatch() methods are protected so we'd have to use reflection to invoke them. I'm not sure the end justifies the means if we need to take that approach.

@rstoyanchev I was going to ask if you'd consider make the methods public but that would be a breaking change for any third-party code that's overridden them as protected. Would you consider a new method on OncePerRequestFilter that returns an EnumSet<DispatcherType> and delegates to shouldNotFilterErrorDispatch() and shouldNotFilterAsyncDispatch() to determine its result?

[rstoyanchev]

That's an option but I'm wondering if that's necessary. OncePerRequestFilter has the logic to do the right thing, and in that sense it handles all dispatcher types, whether that's skip or participate.

The Servlet API doesn't get this right IMO. The Filter implementation knows best which dispatcher types it can handle, and so the default decision shouldn't be left as external configuration where most often it isn't something you want to configure, but are likely to forget.

[wilkinsona]

Thanks, @rstoyanchev. So we could return all dispatcher types for OncePerRequestFilter as we know it's smart enough to be called for all of them and stick with the Servlet spec's default (just REQUEST) for other types of Filter.