On Wed, Nov 29, 2017 at 1:12 PM, Mark Thomas <ma...@apache.org> wrote:
> 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. > Yes, I'm aware of the deployer problems, and the rest makes sense too. I have no credible way to make it really work right now, unfortunately, having to disable the auto deployer makes it unusable (IMO). > > Regarding the mapping path changes... > > > Modified: tomcat/trunk/java/org/apache/catalina/core/ApplicationDispat > cher.java > > URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/ca > talina/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. > Actually, the javadoc says AsyncContext.dispatch behaves the same as an include (it is odd though, but that's what it says). So I think this one should be right. > > > > Modified: tomcat/trunk/java/org/apache/catalina/core/ApplicationMappin > g.java > > URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/ca > talina/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.wrappe > rPath.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. > Huuuum, yes, I don't understand where the thing came from. Rémy > > > Mark > > --------------------------------------------------------------------- > To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org > For additional commands, e-mail: dev-h...@tomcat.apache.org > >
