tomcat-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Mark Thomas <ma...@apache.org>
Subject Re: svn commit: r1816617 - in /tomcat/trunk: java/org/apache/catalina/ java/org/apache/catalina/core/ java/org/apache/catalina/mapper/ java/org/apache/catalina/startup/ java/org/apache/tomcat/util/descriptor/web/ test/org/apache/tomcat/unittest/ webapps/docs/
Date Wed, 29 Nov 2017 12:12:43 GMT
On 29/11/17 09:46, remm@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-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org


Mime
View raw message