Bug 65244 - annotations from @HandlesTypes are checked only at class level when scanning
Summary: annotations from @HandlesTypes are checked only at class level when scanning
Status: RESOLVED FIXED
Alias: None
Product: Tomcat 9
Classification: Unclassified
Component: Servlet (show other bugs)
Version: 9.0.45
Hardware: PC Linux
: P2 normal (vote)
Target Milestone: -----
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-04-15 08:39 UTC by Grzegorz Grzybek
Modified: 2021-05-05 13:48 UTC (History)
1 user (show)



Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Grzegorz Grzybek 2021-04-15 08:39:32 UTC
I have a test WAR, where one of the detected SCIs has this @HandlesTypes (for testing purpose only):

@HandlesTypes({ Deprecated.class })

Then, in the same WAR, I have a servlet with this dummy field:

@Deprecated
public Object dummy;

`org.apache.tomcat.util.bcel.classfile.JavaClass#getAnnotationEntries()` returns null for this servlet class inside `org.apache.catalina.startup.ContextConfig#checkHandlesTypes()`

I know it's artificial and I (later, after scanning) get a list of these classes passed to SCI:

{org.ops4j.pax.web.samples.war.scis.SCIFromTheWab1@2927}  -> {java.util.HashSet@3017}  size = 9
 key: org.ops4j.pax.web.samples.war.scis.SCIFromTheWab1  = {org.ops4j.pax.web.samples.war.scis.SCIFromTheWab1@2927} 
 value: java.util.HashSet  = {java.util.HashSet@3017}  size = 9
  0 = {@3023} "class javax.faces.component.UIViewRoot$ViewScope"
  1 = {@3024} "class org.apache.myfaces.view.facelets.tag.jsf.ComponentHandler"
  2 = {@3025} "class org.apache.myfaces.view.facelets.tag.jsf.ConvertHandler"
  3 = {@3026} "class javax.faces.view.facelets.ResourceResolver"
  4 = {@3027} "class org.apache.myfaces.application.StateCacheFactory"
  5 = {@3028} "class org.apache.myfaces.shared.taglib.UIComponentTagUtils"
  6 = {@3029} "interface javax.faces.bean.package-info"
  7 = {@3030} "class org.apache.myfaces.view.facelets.tag.jsf.ValidateHandler"
  8 = {@3031} "class org.apache.myfaces.view.facelets.tag.MetaRule"

Each of the classes are annotated with @Deprecated.

The problem (?) I found is that chapter "8.2.4 Shared libraries / runtimes pluggability" of Servlet 4 specification says:

> In addition to the ServletContainerInitializer we also have an annotation -
HandlesTypes. The HandlesTypes annotation on the implementation of the
ServletContainerInitializer is used to express interest in classes that may
have annotations ___(type, method or field level annotations)___ specified in the value of
the HandlesTypes or if it extends / implements one those classes anywhere in the
class’ super types.

So either something's wrong in Tomcat, in TCK, or Servlet spec is too permissive...
Comment 1 Mark Thomas 2021-04-15 09:11:43 UTC
Looks like a Tomcat bug to me. That no-one has hit this bug in the 10+ years since the first Tomcat 7 release (where support for this was first added) suggests it is a very rarely used feature but we should still aim to fix it.
Comment 2 Grzegorz Grzybek 2021-04-15 09:47:00 UTC
I don't think TCK tests this - otherwise it'd be caught.
In my personal opinion, annotations from @HandlesTypes should be checked only at class level, not at the method level.

On the other hand, "8.1 Annotations and pluggability" mentions annotations like @javax.annotation.PreDestroy, which suggests deep scanning.

However Tomcat (org.apache.catalina.startup.ContextConfig#processClass()) checks only 3 class-level annotations: @WebServlet, @WebFilter and @WebListener (which are the only MUST-be-supported annotations).

Personally, I would rather change the specification than Tomcat (and potentially Jetty and other containers) and TCK ;)
Comment 3 Remy Maucherat 2021-04-15 15:22:40 UTC
(In reply to Mark Thomas from comment #1)
> Looks like a Tomcat bug to me. That no-one has hit this bug in the 10+ years
> since the first Tomcat 7 release (where support for this was first added)
> suggests it is a very rarely used feature but we should still aim to fix it.

I would also think it would be in the spirit of HandlesTypes to do field and method annotations since it's a way for frameworks to say "gimme anything that has my annotation".

This is not implemented at the moment for fields or methods since:
- https://github.com/apache/tomcat/blob/master/java/org/apache/catalina/startup/ContextConfig.java#L2429 only gets the annotations on the class
- https://github.com/apache/tomcat/blob/master/java/org/apache/tomcat/util/bcel/classfile/ClassParser.java#L232 does not read any field or methods attributes including the annotations
Doing it would probably make scanning a bit slower, and would require expanding this stuff.
Comment 4 romain.manni-bucau 2021-04-15 15:42:56 UTC
Hi,

Not sure it changed in last release but since javaee 6 (to at least EE8) it was only about types: https://docs.oracle.com/javaee/6/api/javax/servlet/annotation/HandlesTypes.html:

> Set of application classes that extend, implement, or have been annotated with the class types listed

Seems it means "the type is annotated" and not "one of its member".

So it excludes fields and methods AFAIK.
Comment 5 Grzegorz Grzybek 2021-04-15 17:48:02 UTC
Javadoc comment:

> application classes that extend, implement,
> or have been annotated with the class types listed by this annotation

can be ambiguous ("class is annotated with" may mean "has class level annotation" or "has either members with annotations or is itself annotated") - though it sounds more like only class-level annotations are considered.

On the other hand, the specification is clear... I'd also worry a bit about performance loss.
Comment 6 romain.manni-bucau 2021-04-15 18:02:07 UTC
If it helps:

1. TomEE already does it and cost of reading the full class can be limited by adjusting well the buffer size
2. Never use a real use case for "not class" level and worse cases frameworks can add a @MarkClass
3. Maybe something to clarify at spec level before changing in tomcat since it will break app (clazz.getAnnotation(MyMarker.class).value() == NPE if changed)?
Comment 7 Remy Maucherat 2021-04-16 16:04:45 UTC
Although not urgent at all, the specification seems very clear now that I have reviewed it (annotations on fields and method do count). I'll try to do something about it next week to see how it can work.
Comment 8 Mark Thomas 2021-04-16 16:17:18 UTC
I've been thinking about implementation options. It looks relatively simple although there is potential complexity depending on the extent to which we are concerned about retaining current behaviour of existing internal method calls in case anyone is using them directly.

My biggest concern is performance. I have set up a simple test to scan the WEB-INF/lib dir from Jira 8.15.0. I plan to use that to track relative performance.

My current thinking is implement the additional scanning, look at the relative performance and then discuss what, if anything, we should optimise.

We'll want to back-port this to 9.0.x and 8.5.x but I think we'll want to do that slowly in case of regressions.
Comment 9 romain.manni-bucau 2021-04-16 16:39:40 UTC
Maybe it is an opportunity to make it properly pluggable. Most tomcat integrators drops that part to use their own scanner (tomee uses xbean, pax uses osgi flavor of xbean, others bypasses it, some use jandex, etc). Can be good to make it properly pluggable if changed no?

Once done having a classscanner and deepscanner (fields, methods, hierarchy) sounds trivial and will enable to not break apps or quickly disable the new behavior when breaking.
Comment 10 Mark Thomas 2021-04-16 16:53:41 UTC
I like it. That does almost certainly mean one breaking change now to introduce the pluggable API. Would we consider ServiceLoader for this?
Comment 11 romain.manni-bucau 2021-04-16 17:20:41 UTC
Context or contextconfig configuration is fine since most integrations have listeners already no?
Comment 12 Remy Maucherat 2021-04-19 08:55:06 UTC
I added simple code in 10 to handle this. If it works ok without regressions, even unintended ones, it can be backported.
Comment 13 Mark Thomas 2021-04-19 09:21:12 UTC
Using my Jira based test the impact of this change is an increase in scan time of ~2.3%. That is a lot lower than I expected and small enough that I'd have no objection to the patch standing as is.

I really like that the patch achieved this while retaining backwards compatibility.
Comment 14 romain.manni-bucau 2021-04-19 09:45:56 UTC
Hi Mark and Rémy,

I'm not sure I got your last comment since the patch on 10.0.6-dev breaks the backward compatibility as such (ie you run a working app on 10.0.5 and then upgrade on 1.0.0.6-dev and the app does not start anymore).
Is the toggle still planned?

Just to make it clear here is a sample: https://gist.github.com/rmannibucau/7ff2bea1e4ca1f3204a16e84afee5f87
Comment 15 Mark Thomas 2021-04-19 10:59:02 UTC
I see no need to make this configurable at this point.

My default position for specification compliance related issues such as this is that it is better for applications to fix their bugs than for Tomcat to add a configuration option to allow the non-compliance to continue. I'd make an exception if the bug was in a widely used library and that library refused to fix the bug - but that seems unlikely in this case.
Comment 16 romain.manni-bucau 2021-04-19 11:30:20 UTC
Ok, let's do it in lazy mode (can it be highlighted in the release announce mail though please?)
Comment 17 Grzegorz Grzybek 2021-04-19 12:03:06 UTC
By the way, I checked that in Jetty there are org.eclipse.jetty.annotations.AnnotationParser.MyFieldVisitor/MyMethodVisitor/MyClassVisitor visitors that handle each case.
Comment 18 Remy Maucherat 2021-04-19 12:12:09 UTC
(In reply to romain.manni-bucau from comment #14)
> Hi Mark and Rémy,
> 
> I'm not sure I got your last comment since the patch on 10.0.6-dev breaks
> the backward compatibility as such (ie you run a working app on 10.0.5 and
> then upgrade on 1.0.0.6-dev and the app does not start anymore).
> Is the toggle still planned?
> 
> Just to make it clear here is a sample:
> https://gist.github.com/rmannibucau/7ff2bea1e4ca1f3204a16e84afee5f87

That's a good test case idea actually. I'll see if I can do something about adding this to the testsuite.

I would also vote to not add a flag for 10 even though it's incompatible, since it's a bug. Maybe for 9 depending on further feedback. Adding the flag is very easy to do if needed.
Comment 19 Grzegorz Grzybek 2021-04-20 12:36:57 UTC
Hmm, another observation. I've added java.util.EventListener.class to @HandlesTypes and I got ... a lot of stuff, including:

```
1 = {@3229} "interface javax.faces.validator.Validator"
...
3 = {@3230} "interface javax.faces.event.SystemEventListener"
...
5 = {@3232} "interface javax.faces.event.BehaviorListener"
```

So I got back to the spec:

> The HandlesTypes annotation on the implementation of the
> ServletContainerInitializer is used to express interest in classes that may
> have annotations (type, method or field level annotations) specified in the value of
> the HandlesTypes or if it extends / implements one those classes anywhere in
> the class’ super types.

I don't think I should get `interface javax.faces.validator.Validator` which exten ds `java.util.EventListener`, because it's not a class - it's an interface.

This may be more difficult to interpret, but if both classes and interfaces were needed, the spec would say "types" or "java.lang.Class" instances...

Javadoc says about:

> the Set of application classes that extend, implement, or have been annotated [...]

This time I checked that Jetty (9.4.40) also passes interfaces.

What do you think?
Comment 20 Remy Maucherat 2021-04-20 12:47:24 UTC
(In reply to Grzegorz Grzybek from comment #19)
> What do you think?

That it would be a separate issue.

I would say interfaces are not supposed to be there.
Comment 21 Grzegorz Grzybek 2021-04-20 12:54:28 UTC
(In reply to Remy Maucherat from comment #20)
> (In reply to Grzegorz Grzybek from comment #19)
> > What do you think?
> 
> That it would be a separate issue.
> 
> I would say interfaces are not supposed to be there.

Here's the new issue: https://bz.apache.org/bugzilla/show_bug.cgi?id=65256

Though I don't claim that the spec forbids interfaces. However Tomcat explicitly ignores annotation types in org.apache.catalina.startup.ContextConfig#checkHandlesTypes:

> if ((javaClass.getAccessFlags() &
>         org.apache.tomcat.util.bcel.Const.ACC_ANNOTATION) != 0) {
>     // Skip annotations.
>     return;
> }
Comment 22 Remy Maucherat 2021-04-20 14:28:38 UTC
So the fix will be in 10.0.6, will see if/when/how it can be backported to 9 and 8.5.
Comment 23 Remy Maucherat 2021-05-05 11:52:38 UTC
(In reply to Remy Maucherat from comment #22)
> So the fix will be in 10.0.6, will see if/when/how it can be backported to 9
> and 8.5.

Should I backport now or should it be tested in 10.0.6 before being included in the 8.5 and 9 releases ?
Comment 24 Mark Thomas 2021-05-05 13:00:31 UTC
I have no objection to a back-port now.
Comment 25 Remy Maucherat 2021-05-05 13:48:50 UTC
The fix will be in 9.0.46 and 8.5.66.