You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by Rémy Maucherat <re...@apache.org> on 2017/12/06 16:25:23 UTC
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/
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 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