You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by bu...@apache.org on 2021/04/15 08:39:32 UTC
[Bug 65244] New: annotations from @HandlesTypes are checked only at
class level when scanning
https://bz.apache.org/bugzilla/show_bug.cgi?id=65244
Bug ID: 65244
Summary: annotations from @HandlesTypes are checked only at
class level when scanning
Product: Tomcat 9
Version: 9.0.45
Hardware: PC
OS: Linux
Status: NEW
Severity: normal
Priority: P2
Component: Servlet
Assignee: dev@tomcat.apache.org
Reporter: gr.grzybek@gmail.com
Target Milestone: -----
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...
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 65244] annotations from @HandlesTypes are checked only at class
level when scanning
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=65244
--- Comment #23 from Remy Maucherat <re...@apache.org> ---
(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 ?
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 65244] annotations from @HandlesTypes are checked only at class
level when scanning
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=65244
--- Comment #5 from Grzegorz Grzybek <gr...@gmail.com> ---
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.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 65244] annotations from @HandlesTypes are checked only at class
level when scanning
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=65244
--- Comment #9 from romain.manni-bucau <rm...@gmail.com> ---
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.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 65244] annotations from @HandlesTypes are checked only at class
level when scanning
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=65244
--- Comment #18 from Remy Maucherat <re...@apache.org> ---
(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.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 65244] annotations from @HandlesTypes are checked only at class
level when scanning
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=65244
--- Comment #4 from romain.manni-bucau <rm...@gmail.com> ---
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.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 65244] annotations from @HandlesTypes are checked only at class
level when scanning
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=65244
--- Comment #1 from Mark Thomas <ma...@apache.org> ---
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.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 65244] annotations from @HandlesTypes are checked only at class
level when scanning
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=65244
--- Comment #15 from Mark Thomas <ma...@apache.org> ---
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.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 65244] annotations from @HandlesTypes are checked only at class
level when scanning
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=65244
--- Comment #7 from Remy Maucherat <re...@apache.org> ---
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.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 65244] annotations from @HandlesTypes are checked only at class
level when scanning
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=65244
--- Comment #6 from romain.manni-bucau <rm...@gmail.com> ---
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)?
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 65244] annotations from @HandlesTypes are checked only at class
level when scanning
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=65244
--- Comment #20 from Remy Maucherat <re...@apache.org> ---
(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.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 65244] annotations from @HandlesTypes are checked only at class
level when scanning
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=65244
--- Comment #10 from Mark Thomas <ma...@apache.org> ---
I like it. That does almost certainly mean one breaking change now to introduce
the pluggable API. Would we consider ServiceLoader for this?
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 65244] annotations from @HandlesTypes are checked only at class
level when scanning
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=65244
--- Comment #17 from Grzegorz Grzybek <gr...@gmail.com> ---
By the way, I checked that in Jetty there are
org.eclipse.jetty.annotations.AnnotationParser.MyFieldVisitor/MyMethodVisitor/MyClassVisitor
visitors that handle each case.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 65244] annotations from @HandlesTypes are checked only at class
level when scanning
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=65244
--- Comment #16 from romain.manni-bucau <rm...@gmail.com> ---
Ok, let's do it in lazy mode (can it be highlighted in the release announce
mail though please?)
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 65244] annotations from @HandlesTypes are checked only at class
level when scanning
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=65244
--- Comment #12 from Remy Maucherat <re...@apache.org> ---
I added simple code in 10 to handle this. If it works ok without regressions,
even unintended ones, it can be backported.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 65244] annotations from @HandlesTypes are checked only at class
level when scanning
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=65244
--- Comment #14 from romain.manni-bucau <rm...@gmail.com> ---
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
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 65244] annotations from @HandlesTypes are checked only at class
level when scanning
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=65244
--- Comment #11 from romain.manni-bucau <rm...@gmail.com> ---
Context or contextconfig configuration is fine since most integrations have
listeners already no?
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 65244] annotations from @HandlesTypes are checked only at class
level when scanning
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=65244
--- Comment #13 from Mark Thomas <ma...@apache.org> ---
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.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 65244] annotations from @HandlesTypes are checked only at class
level when scanning
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=65244
--- Comment #8 from Mark Thomas <ma...@apache.org> ---
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.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 65244] annotations from @HandlesTypes are checked only at class
level when scanning
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=65244
--- Comment #24 from Mark Thomas <ma...@apache.org> ---
I have no objection to a back-port now.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 65244] annotations from @HandlesTypes are checked only at class
level when scanning
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=65244
--- Comment #22 from Remy Maucherat <re...@apache.org> ---
So the fix will be in 10.0.6, will see if/when/how it can be backported to 9
and 8.5.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 65244] annotations from @HandlesTypes are checked only at class
level when scanning
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=65244
--- Comment #19 from Grzegorz Grzybek <gr...@gmail.com> ---
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?
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 65244] annotations from @HandlesTypes are checked only at class
level when scanning
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=65244
--- Comment #3 from Remy Maucherat <re...@apache.org> ---
(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.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 65244] annotations from @HandlesTypes are checked only at class
level when scanning
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=65244
--- Comment #21 from Grzegorz Grzybek <gr...@gmail.com> ---
(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;
> }
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 65244] annotations from @HandlesTypes are checked only at class
level when scanning
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=65244
--- Comment #2 from Grzegorz Grzybek <gr...@gmail.com> ---
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 ;)
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 65244] annotations from @HandlesTypes are checked only at class
level when scanning
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=65244
Remy Maucherat <re...@apache.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Status|NEW |RESOLVED
Resolution|--- |FIXED
--- Comment #25 from Remy Maucherat <re...@apache.org> ---
The fix will be in 9.0.46 and 8.5.66.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 65244] annotations from @HandlesTypes are checked only at class
level when scanning
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=65244
Grzegorz Grzybek <gr...@gmail.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |gr.grzybek@gmail.com
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org