You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by re...@apache.org on 2017/11/29 09:46:00 UTC
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/
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.
Modified:
tomcat/trunk/java/org/apache/catalina/Context.java
tomcat/trunk/java/org/apache/catalina/core/ApplicationContext.java
tomcat/trunk/java/org/apache/catalina/core/ApplicationDispatcher.java
tomcat/trunk/java/org/apache/catalina/core/ApplicationMapping.java
tomcat/trunk/java/org/apache/catalina/core/StandardContext.java
tomcat/trunk/java/org/apache/catalina/mapper/MapperListener.java
tomcat/trunk/java/org/apache/catalina/startup/ContextConfig.java
tomcat/trunk/java/org/apache/catalina/startup/FailedContext.java
tomcat/trunk/java/org/apache/tomcat/util/descriptor/web/LocalStrings.properties
tomcat/trunk/java/org/apache/tomcat/util/descriptor/web/WebRuleSet.java
tomcat/trunk/java/org/apache/tomcat/util/descriptor/web/WebXml.java
tomcat/trunk/test/org/apache/tomcat/unittest/TesterContext.java
tomcat/trunk/webapps/docs/changelog.xml
Modified: tomcat/trunk/java/org/apache/catalina/Context.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/Context.java?rev=1816617&r1=1816616&r2=1816617&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/catalina/Context.java (original)
+++ tomcat/trunk/java/org/apache/catalina/Context.java Wed Nov 29 09:46:00 2017
@@ -348,6 +348,20 @@ public interface Context extends Contain
/**
+ * Get the best effort path for mapping.
+ * @return the default context path
+ */
+ public String getDefaultContextPath();
+
+
+ /**
+ * Return the best effort path for mapping.
+ * @param defaultContextPath The default context path
+ */
+ public void setDefaultContextPath(String defaultContextPath);
+
+
+ /**
* Return the deny-uncovered-http-methods flag for this web application.
*
* @return The current value of the flag
Modified: tomcat/trunk/java/org/apache/catalina/core/ApplicationContext.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/core/ApplicationContext.java?rev=1816617&r1=1816616&r2=1816617&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/catalina/core/ApplicationContext.java (original)
+++ tomcat/trunk/java/org/apache/catalina/core/ApplicationContext.java Wed Nov 29 09:46:00 2017
@@ -287,7 +287,11 @@ public class ApplicationContext implemen
@Override
public String getContextPath() {
- return context.getPath();
+ if (Globals.STRICT_SERVLET_COMPLIANCE && context.getDefaultContextPath() != null) {
+ return context.getDefaultContextPath();
+ } else {
+ return context.getPath();
+ }
}
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);
}
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),
Modified: tomcat/trunk/java/org/apache/catalina/core/StandardContext.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/core/StandardContext.java?rev=1816617&r1=1816616&r2=1816617&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/catalina/core/StandardContext.java (original)
+++ tomcat/trunk/java/org/apache/catalina/core/StandardContext.java Wed Nov 29 09:46:00 2017
@@ -343,6 +343,12 @@ public class StandardContext extends Con
/**
+ * Best effort mapping path.
+ */
+ private String defaultContextPath;
+
+
+ /**
* Override the default context xml location.
*/
private String defaultContextXml;
@@ -1598,6 +1604,21 @@ public class StandardContext extends Con
}
+ @Override
+ public String getDefaultContextPath() {
+ return defaultContextPath;
+ }
+
+ /**
+ * Set the best effort path for mapping.
+ *
+ * @param defaultContextPath The default context path
+ */
+ @Override
+ public void setDefaultContextPath(String defaultContextPath) {
+ this.defaultContextPath = defaultContextPath;
+ }
+
public String getDefaultContextXml() {
return defaultContextXml;
}
Modified: tomcat/trunk/java/org/apache/catalina/mapper/MapperListener.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/mapper/MapperListener.java?rev=1816617&r1=1816616&r2=1816617&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/catalina/mapper/MapperListener.java (original)
+++ tomcat/trunk/java/org/apache/catalina/mapper/MapperListener.java Wed Nov 29 09:46:00 2017
@@ -24,6 +24,7 @@ import org.apache.catalina.ContainerEven
import org.apache.catalina.ContainerListener;
import org.apache.catalina.Context;
import org.apache.catalina.Engine;
+import org.apache.catalina.Globals;
import org.apache.catalina.Host;
import org.apache.catalina.Lifecycle;
import org.apache.catalina.LifecycleEvent;
@@ -364,11 +365,14 @@ public class MapperListener extends Life
*/
private void registerContext(Context context) {
- String contextPath = context.getPath();
+ Host host = (Host)context.getParent();
+ String contextPath = Globals.STRICT_SERVLET_COMPLIANCE ? context.getDefaultContextPath() : null;
+ if (contextPath == null || host.findChild(contextPath) != null) {
+ contextPath = context.getPath();
+ }
if ("/".equals(contextPath)) {
contextPath = "";
}
- Host host = (Host)context.getParent();
WebResourceRoot resources = context.getResources();
String[] welcomeFiles = context.findWelcomeFiles();
@@ -399,7 +403,11 @@ public class MapperListener extends Life
*/
private void unregisterContext(Context context) {
- String contextPath = context.getPath();
+ Host host = (Host)context.getParent();
+ String contextPath = Globals.STRICT_SERVLET_COMPLIANCE ? context.getDefaultContextPath() : null;
+ if (contextPath == null || host.findChild(contextPath) != null) {
+ contextPath = context.getPath();
+ }
if ("/".equals(contextPath)) {
contextPath = "";
}
Modified: tomcat/trunk/java/org/apache/catalina/startup/ContextConfig.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/startup/ContextConfig.java?rev=1816617&r1=1816616&r2=1816617&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/catalina/startup/ContextConfig.java (original)
+++ tomcat/trunk/java/org/apache/catalina/startup/ContextConfig.java Wed Nov 29 09:46:00 2017
@@ -1257,6 +1257,7 @@ public class ContextConfig implements Li
for (Entry<String, String> entry : webxml.getContextParams().entrySet()) {
context.addParameter(entry.getKey(), entry.getValue());
}
+ context.setDefaultContextPath(webxml.getDefaultContextPath());
context.setDenyUncoveredHttpMethods(
webxml.getDenyUncoveredHttpMethods());
context.setDisplayName(webxml.getDisplayName());
Modified: tomcat/trunk/java/org/apache/catalina/startup/FailedContext.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/startup/FailedContext.java?rev=1816617&r1=1816616&r2=1816617&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/catalina/startup/FailedContext.java (original)
+++ tomcat/trunk/java/org/apache/catalina/startup/FailedContext.java Wed Nov 29 09:46:00 2017
@@ -372,6 +372,11 @@ public class FailedContext extends Lifec
public void setAltDDName(String altDDName) { /* NO-OP */ }
@Override
+ public String getDefaultContextPath() { return null; }
+ @Override
+ public void setDefaultContextPath(String defaultContextPath) { /* NO-OP */ }
+
+ @Override
public boolean getDenyUncoveredHttpMethods() { return false; }
@Override
public void setDenyUncoveredHttpMethods(boolean denyUncoveredHttpMethods) {
Modified: tomcat/trunk/java/org/apache/tomcat/util/descriptor/web/LocalStrings.properties
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/tomcat/util/descriptor/web/LocalStrings.properties?rev=1816617&r1=1816616&r2=1816617&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/tomcat/util/descriptor/web/LocalStrings.properties (original)
+++ tomcat/trunk/java/org/apache/tomcat/util/descriptor/web/LocalStrings.properties Wed Nov 29 09:46:00 2017
@@ -39,6 +39,7 @@ webXml.duplicateResourceRef=Duplicate re
webXml.duplicateServletMapping=The servlets named [{0}] and [{1}] are both mapped to the url-pattern [{2}] which is not permitted
webXml.duplicateTaglibUri=Duplicate tag library URI [{0}]
webXml.reservedName=A web.xml file was detected using a reserved name [{0}]. The name element will be ignored for this fragment.
+webXml.mergeConflictDefaultContextPath=The default context path was defined in multiple fragments with different values including fragment with name [{0}] located at [{1}]
webXml.mergeConflictDisplayName=The display name was defined in multiple fragments with different values including fragment with name [{0}] located at [{1}]
webXml.mergeConflictFilter=The Filter [{0}] was defined inconsistently in multiple fragments including fragment with name [{1}] located at [{2}]
webXml.mergeConflictLoginConfig=A LoginConfig was defined inconsistently in multiple fragments including fragment with name [{0}] located at [{1}]
Modified: tomcat/trunk/java/org/apache/tomcat/util/descriptor/web/WebRuleSet.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/tomcat/util/descriptor/web/WebRuleSet.java?rev=1816617&r1=1816616&r2=1816617&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/tomcat/util/descriptor/web/WebRuleSet.java (original)
+++ tomcat/trunk/java/org/apache/tomcat/util/descriptor/web/WebRuleSet.java Wed Nov 29 09:46:00 2017
@@ -203,6 +203,9 @@ public class WebRuleSet implements RuleS
digester.addCallParam(fullPrefix + "/context-param/param-name", 0);
digester.addCallParam(fullPrefix + "/context-param/param-value", 1);
+ digester.addCallMethod(fullPrefix + "/default-context-path",
+ "setDefaultContextPath", 0);
+
digester.addCallMethod(fullPrefix + "/display-name",
"setDisplayName", 0);
Modified: tomcat/trunk/java/org/apache/tomcat/util/descriptor/web/WebXml.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/tomcat/util/descriptor/web/WebXml.java?rev=1816617&r1=1816616&r2=1816617&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/tomcat/util/descriptor/web/WebXml.java (original)
+++ tomcat/trunk/java/org/apache/tomcat/util/descriptor/web/WebXml.java Wed Nov 29 09:46:00 2017
@@ -231,6 +231,13 @@ public class WebXml extends XmlEncodingB
// - description
// - icon
+ // default-context-path
+ private String defaultContextPath = null;
+ public String getDefaultContextPath() { return defaultContextPath; }
+ public void setDefaultContextPath(String defaultContextPath) {
+ this.defaultContextPath = defaultContextPath;
+ }
+
// display-name - TODO should support multiple with language
private String displayName = null;
public String getDisplayName() { return displayName; }
@@ -1424,6 +1431,24 @@ public class WebXml extends XmlEncodingB
}
contextParams.putAll(temp.getContextParams());
+ if (defaultContextPath == null) {
+ for (WebXml fragment : fragments) {
+ String value = fragment.getDefaultContextPath();
+ if (value != null) {
+ if (temp.getDefaultContextPath() == null) {
+ temp.setDefaultContextPath(value);
+ } else {
+ log.error(sm.getString(
+ "webXml.mergeConflictDefaultContextPath",
+ fragment.getName(),
+ fragment.getURL()));
+ return false;
+ }
+ }
+ }
+ defaultContextPath = temp.getDefaultContextPath();
+ }
+
if (displayName == null) {
for (WebXml fragment : fragments) {
String value = fragment.getDisplayName();
Modified: tomcat/trunk/test/org/apache/tomcat/unittest/TesterContext.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/test/org/apache/tomcat/unittest/TesterContext.java?rev=1816617&r1=1816616&r2=1816617&view=diff
==============================================================================
--- tomcat/trunk/test/org/apache/tomcat/unittest/TesterContext.java (original)
+++ tomcat/trunk/test/org/apache/tomcat/unittest/TesterContext.java Wed Nov 29 09:46:00 2017
@@ -471,6 +471,16 @@ public class TesterContext implements Co
}
@Override
+ public String getDefaultContextPath() {
+ return null;
+ }
+
+ @Override
+ public void setDefaultContextPath(String defaultContextPath) {
+ // NO-OP
+ }
+
+ @Override
public boolean getDenyUncoveredHttpMethods() {
return false;
}
Modified: tomcat/trunk/webapps/docs/changelog.xml
URL: http://svn.apache.org/viewvc/tomcat/trunk/webapps/docs/changelog.xml?rev=1816617&r1=1816616&r2=1816617&view=diff
==============================================================================
--- tomcat/trunk/webapps/docs/changelog.xml (original)
+++ tomcat/trunk/webapps/docs/changelog.xml Wed Nov 29 09:46:00 2017
@@ -50,6 +50,12 @@
<fix>
Add some missing NPEs to ServletContext. (remm)
</fix>
+ <fix>
+ Minor HttpServletMapping issues. (remm)
+ </fix>
+ <fix>
+ Add bare bones support for default context path. (remm)
+ </fix>
</changelog>
</subsection>
<subsection name="Other">
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
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/
Posted by Rémy Maucherat <re...@apache.org>.
On Thu, Dec 7, 2017 at 10:51 AM, Mark Thomas <ma...@apache.org> wrote:
> On 07/12/17 09:03, Rémy Maucherat wrote:
> > On Wed, Dec 6, 2017 at 8:35 PM, Mark Thomas <ma...@apache.org> wrote:
> >
> >> On 06/12/17 16:25, Rémy Maucherat wrote:
> >>> On Wed, Nov 29, 2017 at 1:12 PM, Mark Thomas <ma...@apache.org> wrote:
> >>>
> >>>> 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.
> >>>>
> >>>
> >>> I still don't see any good way to do this. The Tomcat deployer is
> >>> exclusively based on the file name (which isn't bad since you can
> change
> >> it
> >>> - in the context of Tomcat, as you say inside an EAR it's different).
> >> From
> >>> that the conditions to keep everything in place make the feature
> unusable
> >>> as far as I am concerned.
> >>
> >> Just thinking aloud...
> >>
> >> Could we get the deployer to attempt a rename (if there are no naming
> >> conflicts) based on the default-context-path as the very first action
> >> and then follow the normal automatic deployment rules?
> >>
> >
> > To be honest, I thought about it, but it seemed quite horrible and didn't
> > seem like an acceptable solution to me. Maybe it is then ?
>
> I hadn't thought much about the implementation detail. The more I think
> about it the worse it looks.
>
> My preference at this point remains not to implement this feature. If
> there is demand for it, and an elegant implementation can be found, then
> I could live with it.
>
> We could have another folder, like webapps, where we could say we deploy
"enterprise" archives (like if we support some amount of EAR one day, after
all it can be used to package multiple wars together), as a way to disguise
the predeploy/copy step. Ok, it's horrible too :)
Another option is to generate a context file (I'm pretty sure it will have
some challenges) while making the original webapp unavailable or something,
so the war gets treated as an external one.
Rémy
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/
Posted by Mark Thomas <ma...@apache.org>.
On 07/12/17 09:03, Rémy Maucherat wrote:
> On Wed, Dec 6, 2017 at 8:35 PM, Mark Thomas <ma...@apache.org> wrote:
>
>> On 06/12/17 16:25, Rémy Maucherat wrote:
>>> On Wed, Nov 29, 2017 at 1:12 PM, Mark Thomas <ma...@apache.org> wrote:
>>>
>>>> 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.
>>>>
>>>
>>> I still don't see any good way to do this. The Tomcat deployer is
>>> exclusively based on the file name (which isn't bad since you can change
>> it
>>> - in the context of Tomcat, as you say inside an EAR it's different).
>> From
>>> that the conditions to keep everything in place make the feature unusable
>>> as far as I am concerned.
>>
>> Just thinking aloud...
>>
>> Could we get the deployer to attempt a rename (if there are no naming
>> conflicts) based on the default-context-path as the very first action
>> and then follow the normal automatic deployment rules?
>>
>
> To be honest, I thought about it, but it seemed quite horrible and didn't
> seem like an acceptable solution to me. Maybe it is then ?
I hadn't thought much about the implementation detail. The more I think
about it the worse it looks.
My preference at this point remains not to implement this feature. If
there is demand for it, and an elegant implementation can be found, then
I could live with it.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
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/
Posted by Rémy Maucherat <re...@apache.org>.
On Wed, Dec 6, 2017 at 8:35 PM, Mark Thomas <ma...@apache.org> wrote:
> On 06/12/17 16:25, Rémy Maucherat wrote:
> > On Wed, Nov 29, 2017 at 1:12 PM, Mark Thomas <ma...@apache.org> wrote:
> >
> >> 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.
> >>
> >
> > I still don't see any good way to do this. The Tomcat deployer is
> > exclusively based on the file name (which isn't bad since you can change
> it
> > - in the context of Tomcat, as you say inside an EAR it's different).
> From
> > that the conditions to keep everything in place make the feature unusable
> > as far as I am concerned.
>
> Just thinking aloud...
>
> Could we get the deployer to attempt a rename (if there are no naming
> conflicts) based on the default-context-path as the very first action
> and then follow the normal automatic deployment rules?
>
To be honest, I thought about it, but it seemed quite horrible and didn't
seem like an acceptable solution to me. Maybe it is then ?
Rémy
>
> Mark
>
> P.S. I'm quite happy to ignore this feature and declare Tomcat spec
> compliant on the grounds the container specific configuration is always
> present.
>
>
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/
Posted by Mark Thomas <ma...@apache.org>.
On 06/12/17 16:25, Rémy Maucherat wrote:
> On Wed, Nov 29, 2017 at 1:12 PM, Mark Thomas <ma...@apache.org> wrote:
>
>> 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.
>>
>
> I still don't see any good way to do this. The Tomcat deployer is
> exclusively based on the file name (which isn't bad since you can change it
> - in the context of Tomcat, as you say inside an EAR it's different). From
> that the conditions to keep everything in place make the feature unusable
> as far as I am concerned.
Just thinking aloud...
Could we get the deployer to attempt a rename (if there are no naming
conflicts) based on the default-context-path as the very first action
and then follow the normal automatic deployment rules?
Mark
P.S. I'm quite happy to ignore this feature and declare Tomcat spec
compliant on the grounds the container specific configuration is always
present.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
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/
Posted by Rémy Maucherat <re...@apache.org>.
On Wed, Nov 29, 2017 at 1:12 PM, Mark Thomas <ma...@apache.org> wrote:
> 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.
>
I still don't see any good way to do this. The Tomcat deployer is
exclusively based on the file name (which isn't bad since you can change it
- in the context of Tomcat, as you say inside an EAR it's different). From
that the conditions to keep everything in place make the feature unusable
as far as I am concerned.
Rémy
> 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
>
>
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/
Posted by Rémy Maucherat <re...@apache.org>.
On Wed, Nov 29, 2017 at 4:14 PM, Konstantin Kolinko <kn...@gmail.com>
wrote:
> 22. I am -1 to all those STRICT_SERVLET_COMPLIANCE conditionals.
>
Ok, I'm replying in the context of the dispatcher http mapping override. In
that case, it's really a case where it is used elsewhere: the community
disagrees with the behavior but it is explicit in the specification.
The default-context-path element needs a lot more work, so I won't do
anything at the moment. Good point on the fragment xsd.
Rémy
>
> My reading of section "30. default-context-path Element" on page 202
> of 246 of servlet-4_0_FINAL.pdf (in chapter 14.4 Deployment Descriptor
> Diagram)
> is that this feature is only usable at deployment time.
>
> I think this is only a hit to an application deployer (the person
> performing a deployment).
>
> Is there some other place in the Spec PDF where this feature is documented?
> The changelog chapter does not mention other places, and I do not see
> any others with a quick search.
>
>
> As a similar example: "short-name" in a tld file of a Tag Library is a
> hint for the prefix used for those tags. It is not the actual prefix.
>
>
> Thus I think that we do not need to change our
> auto-deployment/deploy-on-startup procedure.
>
>
> I think that this feature belongs to the ways how a web application
> may be deployed programmatically. E.g.:
>
> 1) Make "path" parameter optional in "text/deploy" command of Manager
> web application,
>
> http://tomcat.apache.org/tomcat-9.0-doc/manager-howto.html#
> Deploy_A_New_Application_Archive_(WAR)_Remotely
>
> 2) Make "path" parameter optional in DeployTask of Ant.
>
>
>
> 3. If this feature is controlled by a system property, make a
> separate property which default depends on STRICT_SERVLET_COMPLIANCE.
>
> Do not use STRICT_SERVLET_COMPLIANCE directly.
>
> As this is a deployment feature, I think it makes sense as an
> attribute on Host rather that a system property. (Though as I wrong
> above, I think we do not need to implement it).
>
> Best regards,
> Konstantin Kolinko
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
>
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/
Posted by Konstantin Kolinko <kn...@gmail.com>.
2017-11-29 15:12 GMT+03:00 Mark Thomas <ma...@apache.org>:
> 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.
Several comments
1. The default-context-path is declared in web-app_4_0.xsd
As such, it should be allowed in the main web.xml only and not in web fragments.
It would be strange for a library to dictate what path a web
application is deployed under.
The same for several other options that are declared in web-app_4_0.xsd only:
request-character-encoding
response-character-encoding
deny-uncovered-http-methods
2. I am -1 to all those STRICT_SERVLET_COMPLIANCE conditionals.
My reading of section "30. default-context-path Element" on page 202
of 246 of servlet-4_0_FINAL.pdf (in chapter 14.4 Deployment Descriptor
Diagram)
is that this feature is only usable at deployment time.
I think this is only a hit to an application deployer (the person
performing a deployment).
Is there some other place in the Spec PDF where this feature is documented?
The changelog chapter does not mention other places, and I do not see
any others with a quick search.
As a similar example: "short-name" in a tld file of a Tag Library is a
hint for the prefix used for those tags. It is not the actual prefix.
Thus I think that we do not need to change our
auto-deployment/deploy-on-startup procedure.
I think that this feature belongs to the ways how a web application
may be deployed programmatically. E.g.:
1) Make "path" parameter optional in "text/deploy" command of Manager
web application,
http://tomcat.apache.org/tomcat-9.0-doc/manager-howto.html#Deploy_A_New_Application_Archive_(WAR)_Remotely
2) Make "path" parameter optional in DeployTask of Ant.
3. If this feature is controlled by a system property, make a
separate property which default depends on STRICT_SERVLET_COMPLIANCE.
Do not use STRICT_SERVLET_COMPLIANCE directly.
As this is a deployment feature, I think it makes sense as an
attribute on Host rather that a system property. (Though as I wrong
above, I think we do not need to implement it).
Best regards,
Konstantin Kolinko
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
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/
Posted by Mark Thomas <ma...@apache.org>.
On 29/11/17 13:25, Rémy Maucherat wrote:
> On Wed, Nov 29, 2017 at 1:12 PM, Mark Thomas <ma...@apache.org> wrote:
<snip/>
> 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).
From memory the original use case related to EAR deployments so I'm not
sure if a Tomcat user would ever need it. There is always the wait and
see option. If there is user demand for it then we can try and figure
out how to make it work at that point. We'd have some users to help us
figure out what is and isn't a reasonable restriction on the usage of
the feature.
>> 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.
Hmm. Let me do some more research on that. That certainly isn't my
recollection of the EG discussions. I wonder if something got lost in
translation between the EG and the spec updates or if my memory is
failing me.
Consider my veto withdrawn.
If my memory is correct then +1 to using SERVLET_SPEC_COMPLIANCE to
switch between the current, EG intended behaviour and the behaviour
described by the spec.
>>> 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.
From memory that was one of the options the EG considered but ultimately
rejected.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
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/
Posted by Rémy Maucherat <re...@apache.org>.
On Wed, Nov 29, 2017 at 1:12 PM, Mark Thomas <ma...@apache.org> wrote:
> 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.
>
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-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
>
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/
Posted by Mark Thomas <ma...@apache.org>.
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