On 29/11/17 09:46, r...@apache.org wrote: > Author: remm > Date: Wed Nov 29 09:46:00 2017 > New Revision: 1816617 > > URL: http://svn.apache.org/viewvc?rev=1816617&view=rev > Log: > Add a bare bones default-context-path impl (best effort really, it's a bad > feature). Also fix for some mapping paths, excluded pending some > clarifications.
I agree that default-context-path is a bad feature. A small positive is that testing this has highlighted that we were using an out of date version of web-app_4_0.xsd. I've updated that and the other schemas. I do like the way you implemented default-context-path. It is a nice approach for a truly awful feature. However, there are still issues. The ones I've found so far are: - the Manager app reports the wrong path so when you click on a link you get a 404 (it calls Context.getPath() directly) - There are 10s of other calls to Context.getPath() which suggest there will be further issues in authentication, dispatching and session cookie error messages - automatic deployment can replace an app with a default-context-path without any warning I appreciate that this feature is only enabled with STRICT_SERVLET_COMPLIANCE but the severity of problems it creates are significant. I don't see any easy way to fix this that doesn't involved adding a fair amount of complexity - particularly around auto-deployment. One option might be to make enabling of this feature dependent on both STRICT_SERVLET_COMPLIANCE and automatic deployment being disabled but that would still leave all the Context.getPath() issues to deal with. It is worth noting from the spec for the default-context-path that <quote> Servlet containers may provide vendor specific configuration options that allows specifying a value that overrides the value specified here. </quote> I would argue that Tomcat's deployment mechanism means that there is always "vendor specific configuration" to override any value specified for default-context-path and therefore no implementation of this feature is necessary. I am currently very close to vetoing the implementation of this feature but I'm going to hold off doing so for now to see if the issues above can be resolved without adding too much complexity. I suggest a new Context method (e.g. getPathWithDefault() ). This method would return a path that takes account of STRICT_SERVLET_COMPLIANCE, automatic deployment for the current Host and any default-context-path. Then review all the existing calls to Context.getPath() and switching the appropriate ones to use the new method. Even then I think there will still be issues in the Manager and deploying new apps with conflicting paths. It might be necessary to go even further and have an explicit option to enable this feature. For clarity, I don't object at all to parsing the default-context-path from web.xml, nor exposing the value via the Context. I'd be happy for that code to stay as is. Regarding the mapping path changes... > Modified: > tomcat/trunk/java/org/apache/catalina/core/ApplicationDispatcher.java > URL: > http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/core/ApplicationDispatcher.java?rev=1816617&r1=1816616&r2=1816617&view=diff > ============================================================================== > --- tomcat/trunk/java/org/apache/catalina/core/ApplicationDispatcher.java > (original) > +++ tomcat/trunk/java/org/apache/catalina/core/ApplicationDispatcher.java Wed > Nov 29 09:46:00 2017 > @@ -626,7 +626,9 @@ final class ApplicationDispatcher implem > wrequest.setQueryString(queryString); > wrequest.setQueryParams(queryString); > } > - wrequest.setMapping(mapping); > + if (!Globals.STRICT_SERVLET_COMPLIANCE) { > + wrequest.setMapping(mapping); > + } > > invoke(state.outerRequest, state.outerResponse, state); > } -1 (veto) to the above. See the Javadoc: https://javaee.github.io/javaee-spec/javadocs/javax/servlet/http/HttpServletRequest.html#getHttpServletMapping-- Also sections 9.3.1 (includes), 9.4.2 (forwards) and 9.7.2 (async dispatches) of the Servlet 4.0 spec. In short, the EG decided to follow the same rules for the mapping as the other request properties. > Modified: tomcat/trunk/java/org/apache/catalina/core/ApplicationMapping.java > URL: > http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/core/ApplicationMapping.java?rev=1816617&r1=1816616&r2=1816617&view=diff > ============================================================================== > --- tomcat/trunk/java/org/apache/catalina/core/ApplicationMapping.java > (original) > +++ tomcat/trunk/java/org/apache/catalina/core/ApplicationMapping.java Wed > Nov 29 09:46:00 2017 > @@ -19,6 +19,7 @@ package org.apache.catalina.core; > import javax.servlet.http.HttpServletMapping; > import javax.servlet.http.MappingMatch; > > +import org.apache.catalina.Globals; > import org.apache.catalina.mapper.MappingData; > > public class ApplicationMapping { > @@ -47,7 +48,8 @@ public class ApplicationMapping { > mapping = new MappingImpl("", "", > mappingData.matchType, servletName); > break; > case DEFAULT: > - mapping = new MappingImpl("", "/", > mappingData.matchType, servletName); > + String match = Globals.STRICT_SERVLET_COMPLIANCE ? > "default" : ""; > + mapping = new MappingImpl(match, "/", > mappingData.matchType, servletName); > break; > case EXACT: > mapping = new > MappingImpl(mappingData.wrapperPath.toString().substring(1), -1 (veto) See the Javadoc: https://javaee.github.io/javaee-spec/javadocs/javax/servlet/http/HttpServletMapping.html In particular the table in the interface description. Mark --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
