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

Align AbstractFilterRegistrationBean dispatcher types with servlet spec #7467

Closed
philwebb opened this issue Nov 23, 2016 · 6 comments
Closed

Align AbstractFilterRegistrationBean dispatcher types with servlet spec #7467

philwebb opened this issue Nov 23, 2016 · 6 comments
Assignees
Milestone

Comments

@philwebb
Copy link
Member

@philwebb philwebb commented Nov 23, 2016

The default dispatcher types defined in AbstractFilterRegistrationBean were originally copied from AbstractDispatcherServletInitializer.getDispatcherTypes() however they continue to cause problems.

Rather than trying to pick our own set we should align with the servlet spec and use DispatcherType.REQUEST as the default.

We will then need to check our own configuration of filters to see if specified dispatcher types are required for them.

@cemo
Copy link
Contributor

@cemo cemo commented Nov 27, 2016

It would be cool to be set default values from configuration files.

@wilkinsona
Copy link
Member

@wilkinsona wilkinsona commented Feb 14, 2017

This change will mean that FORWARD, INCLUDE, and ASYNC are removed from the default dispatcher types. The loss of FORWARD and INCLUDE appears to be inconsequential. The loss of ASYNC does have an effect: it means that the security context is now null within an async dispatch. I think we should change the default there.

@cemo
Copy link
Contributor

@cemo cemo commented Feb 14, 2017

@wilkinsona see #4505 (comment) please. I still believe that ERROR, ASYNC and REQUEST are suitable for default. These 3 dispatcher type are usually the beginning of request chains. However FORWARD and INCLUDE are not used at the beginning of request, instead they are used after.

@wilkinsona
Copy link
Member

@wilkinsona wilkinsona commented Feb 14, 2017

Sorry, @cemo, but we're not going to change our decision to use REQUEST as the general purpose default as we want to align with the Servlet spec. Just to be clear, my comment above was specifically with regards to the default for the security filter.

@cemo
Copy link
Contributor

@cemo cemo commented Feb 14, 2017

Thanks @wilkinsona, really makes sense to align with spec.

@wilkinsona
Copy link
Member

@wilkinsona wilkinsona commented Feb 14, 2017

See #8288 and #8289 for exceptions to the new default of REQUEST.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants