Add abstract WebApplicationInitializers #62
Conversation
|
I can appreciate the goal to reduce boilerplate but the present code would require if-else conditions in every method that accepts a servlet name: if (servletName.equals("one")) {
// ..
}
else if (servletName.equals("two") {
// ..
}
else {
// shouldn't happen..
}Seeing that potentially 4 times in one class is not appealing. Even for a single servlet if you need to make customizations, you can end up with 6-7 overridden methods in which case I would prefer to write the onStartup method as now. Don't know what the right solution is but one option is to pass some object like a DispatcherServletRegistrar into a protected method, which subclasses can use to provide basic information for one or more servlets including servlet name, mappings, and config classes. For more advanced needs, separate protected methods could provide access to the created ConfigurableWebApplicationContext(s) and ServletRegistration.Dynamic(s): /** The first servlet **/
protected void customizeServlet(
ConfigurableWebApplicationContext wac, ServletRegistration.Dynamic registration)
/** All servlets **/
protected void customizeServlets(
Map<String, ConfigurableWebApplicationContext> applicationContexts,
Map<String, ServletRegistration.Dynamic> registrations)Not a perfect solution but an alternative to consider at least. |
|
I think it's a bit unfair to base judgment on uncommon scenarios. I would expect the vast majority of Spring Web Apps to only have one servlet. In that case, the typical extension of public class MyDispatcherServletInitializer extends AbstractDispatcherServletInitializer {
@Override
protected WebApplicationContext createServletApplicationContext(String servletName) {
StaticWebApplicationContext applicationContext = new StaticWebApplicationContext();
applicationContext.registerSingleton("controller", MyBean.class);
return applicationContext;
}
@Override
protected String[] getServletMappings(String servletName) {
return new String[] { "/*"};
}
@Override
protected WebApplicationContext createRootApplicationContext() {
return null;
}
}Or if we'd use public class MyAnnotationConfigDispatcherServletInitializer
extends AbstractAnnotationConfigDispatcherServletInitializer {
@Override
protected Class<?>[] getServletConfigClasses(String servletName) {
return new Class<?>[] {MyServletConfig.class};
}
@Override
protected String[] getServletMappings(String servletName) {
return new String[]{"/*"};
}
@Override
protected Class<?>[] getRootConfigClasses() {
return new Class<?>[] {MyRootConfig.class};
}
}Even with two servlets it wouldn't be so bad as suggested, by using the Java 7 switch statement: public class MyAnnotationConfigDispatcherServletInitializer
extends AbstractAnnotationConfigDispatcherServletInitializer {
@Override
protected String[] getServletNames() {
return new String[]{"one", "two"};
}
@Override
protected Class<?>[] getServletConfigClasses(String servletName) {
switch (servletName) {
case "one":
return new Class<?>[ MyServlet1Config.class];
case "two":
return new Class<?>[ MyServlet2Config.class];
}
return null;
}
@Override
protected String[] getServletMappings(String servletName) {
switch (servletName) {
case "one":
return new String[] { "/one/*"};
case "two":
return new String[] { "/two/*"};
}
return null;
}
@Override
protected Class<?>[] getRootConfigClasses() {
return new Class<?>[] {MyRootConfig.class};
}
}If you do have multiple servlets, and you do have to do a lot of customization on them, then the better option is to simply extend But for the simple, common case, I'd say that the code shown above is a lot easier to deal with than having a separate @jhoeller and @cbeams, I'm interested to hear what you guys think. |
|
Why are we even trying to support multiple servlets in AbstractDispatcherServletInitializer to begin with? Couldn't we simply keep it limited to the root context plus one DispatcherServlet scenario? After all, people can always extend AbstractContextLoaderInitializer and additionally register arbitrary servlets... I'd find that arrangement clearer. |
|
I actually started out with a single servlet model, but found out it was trivial to add support for multiple servlets, by adding a servlet name parameter. So if we'd only support one |
|
Thanks for putting this together. Overall I think it's a good idea. style
content
|
|
@poutsma, just a ping on this. We can still get this into 3.2 M1 if you're interested. |
|
The earliest I can take a look at it is Monday, because of a short holiday in taking Thursday and Friday. Let me know if that's good enough. |
|
Yes, that's fine. We won't release until later in the week, for sure. |
|
I've pushed a new version, incorporating all the changes suggested. I also tried to squash the commits into one, but failed. |
|
Assuming you have the three commits above sitting as the three most recent commits on your SPR-9300 branch, you should be able to do the following: The editor will open and you'll see the following: change this to read: quit the editor, rebasing occurs, and you're left with a single commit which you can then force-push back to to your remote, updating the pull request in the process. |
This commit adds three abstract WebApplicationInitializers, to be used for a typical setup of a Spring Web application. - AbstractContextLoaderInitializer provides an abstract base class for setting up the ContextLoaderListener. - AbstractDispatcherServletInitializer provides an abstract base class for setting up one or more DispatcherServlets, with an optional root context. - AbstractAnnotationConfigDispatcherServletInitializer provides an abstract base class for setting up DispatcherServlets based on @configuration classes. Issue: SPR-9300
|
Thanks, Arjen; this is merged. Note that I made several changes that you can see here: https://gist.github.com/2769617. The most substantive among these is the renaming of |
These two commits adds three abstract WebApplicationInitializers, to be used
for a typical setup of a Spring Web application.
setting up the ContextLoaderListener.
for setting up one or more DispatcherServlets, with an optional root
context.
abstract base class for setting up DispatcherServlets based on
Configuration classes.
Some points for discussion:
AbstractContextLoaderInitializerandAbstractDispatcherServletInitializer, but less happy aboutAbstractAnnotationConfigDispatcherServletInitializer. Open for suggestions as well.Also, this seems to have turned into two commits, while the goal was to have one. Blame my git ignorance.