Re: [servlet-dev] Rationale for assertion Servlet:JAVADOC:668.6

[Mark Thomas <markt@xxxxxxxxxx>]

On 08/09/2020 14:30, arjan tijms wrote:
> Hi,
> 
> We should definitely see if there's a reason for this artificial
> restriction, and if not, perhaps consider removing it?
> 
> There's variants that strike me as even more unclear, like:
> 
>   <assertion required="true" impl-spec="false" status="active"
> testable="true">
>         <id>Servlet:JAVADOC:685.1</id>
>         <description>if this ServletContext was passed to the
> ServletContextListener.contextInitialized(jakarta.servlet.ServletContextEvent)
> method of a ServletContextListener that was neither declared in web.xml
> or web-fragment.xml, nor annotated with WebListener</description>
>         <package>jakarta.servlet</package>
>         <class-interface>ServletContext</class-interface>
>         <method name="getEffectiveMajorVersion" return-type="int">
>             <throw>java.lang.UnsupportedOperationException</throw>
>         </method>
>     </assertion>
> 
> Why would that method need such restrictions?

I can't see a reason for that. Servlet 3.0 was before my time on the EG
but I do have a copy of the EG archives. Let me do some digging...

Bingo!

<quote>
As agreed at our face-to-face meeting during J1, I'm about to add the
following IllegalStateException throws clause to these ServletContext
methods that were added for Servlet 3.0:
...
For those who were unable to attend the face-to-face meeting: We agreed
that any
configuration included with, or performed by web fragment JAR files
excluded from absolute ordering must be ignored. Preferably, this
would also apply to tag libraries provided by such JAR files. However,
this would have required changes to the JSP spec. Therefore, we agreed
that the container must honor any tag libraries provided by web
fragment JAR files excluded from absolute ordering, including any
ServletContextListeners declared in their TLD files, with the
exception that such ServletContextListeners must be prevented from
invoking any Servlet 3.0 related APIs on the ServletContext passed to
them. This will ensure full backward compatibility.
</quote>

This was later summarized as:
<quote>
We agreed to block any Servlet 3.0 methods on a ServletContext passed
to the contextInitialized method of a ServletContextListener that is
neither declared in web.xml or web-fragment.xml, nor annotated with
@WebListener.

This was to prevent any ServletContextListeners declared in TLDs from
using any Servlet 3.0 functionality, in particular its programmatic
registration features.
</quote>

There then followed a discussion whether to block all Servlet 3.0p
methods or a subset. After starting down the subset route the EG changed
its mind and went with all of them. The reason being:

<quote>
I think the intent is not only to prevent listeners declared from TLDs
from doing anything bad, but to also discourage them being defined in a
TLD altogether. Providing part of the api's seems to give you less of an
incentive to move to web.xml fragments. Therefore, we should just block
them all.
</quote>


What I haven't been able track down is how/why the wording changed from
"ServletContextListeners from a TLD"
to
"ServletContextListener that is neither declared in web.xml or
web-fragment.xml, nor annotated with @WebListener"

I think it was to prevent programmatically added ServletContextListener
instances (added via ServletContainerInitializer.onStartup()) using the
Servlet 3.0 programmatic APIs but I don't immediately see the issue with
that.

My own take on all of this is that there is scope to relax the rules for
some of the methods but not all of them (assuming we wish to retain
backwards compatibility).

Mark

> 
> Kind regards,
> Arjan
> 
> 
> 
> On Tue, Sep 8, 2020 at 3:00 PM Stuart Douglas <sdouglas@xxxxxxxxxx
> <mailto:sdouglas@xxxxxxxxxx>> wrote:
> 
>     This was not a problem for Undertow either, I had to add a heap of
>     logic artificially restricting this to pass the TCK when these tests
>     were added.
> 
>     Stuart
> 
> 
>     On Tue, 8 Sep 2020 at 19:24, arjan tijms <arjan.tijms@xxxxxxxxx
>     <mailto:arjan.tijms@xxxxxxxxx>> wrote:
> 
>         Hi,
> 
>         The following assertion states than
>         an UnsupportedOperationException must be thrown if addFilter is
>         called on a ServletContext passed to a ServletContextListener
>         that wasn't declared by either annotation or xml:
> 
>         <assertion required="true" impl-spec="false" status="active"
>         testable="true">
>                 <id>Servlet:JAVADOC:668.6</id>
>                 <description>if this ServletContext was passed to the
>         ServletContextListener.contextInitialized(jakarta.servlet.ServletContextEvent)
>         method of a ServletContextListener that was neither declared in
>         web.xml or web-fragment.xml, nor annotated with
>         WebListener</description>
>                 <package>jakarta.servlet</package>
>                 <class-interface>ServletContext</class-interface>
>                 <method name="addFilter"
>         return-type="jakarta.servlet.FilterRegistration.FilterRegistration.Dynamic">
>                     <parameters>
>                         <parameter>java.lang.String</parameter>
>                         <parameter>java.lang.String</parameter>
>                     </parameters>
>                     <throw>java.lang.UnsupportedOperationException</throw>
>                 </method>
>             </assertion>
> 
>         I wonder what's the exact reason for this.
> 
>         In Piranha for example listeners are added from container
>         initializers quite universally and the web.xml and annotation
>         ones add listeners in the same way as other container
>         initializers would:
> 
>         public void configure(WebApplication webApplication) {
>                 webApplication.addInitializer(new WebXmlInitializer());
>                 webApplication.addInitializer(new
>         WebAnnotationInitializer());
>                 webApplication.addInitializer(new
>         JakartaSecurityAllInitializer());
>                 webApplication.addInitializer(new
>         ServletContainerAllInitializer());
>          }
> 
>         Making a distinction from listeners that have been added by (in
>         this case) the WebXmlInitializer and say
>         the ServletContainerAllInitializer is therefore quite artificial.
> 
>         Now I appreciate that other containers might work differently,
>         but I still wonder; is there really a need for this exception in
>         other containers? I'm probably missing something and assume some
>         potential ordering issue, but at the moment I don't quite see it.
> 
>         Kind regards,
>         Arjan
> 
> 
> 
> 
> 
> 
>         _______________________________________________
>         servlet-dev mailing list
>         servlet-dev@xxxxxxxxxxx <mailto:servlet-dev@xxxxxxxxxxx>
>         To unsubscribe from this list, visit
>         https://www.eclipse.org/mailman/listinfo/servlet-dev
> 
>     _______________________________________________
>     servlet-dev mailing list
>     servlet-dev@xxxxxxxxxxx <mailto:servlet-dev@xxxxxxxxxxx>
>     To unsubscribe from this list, visit
>     https://www.eclipse.org/mailman/listinfo/servlet-dev
> 
> 
> _______________________________________________
> servlet-dev mailing list
> servlet-dev@xxxxxxxxxxx
> To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/servlet-dev
>