Java: Update JAX-RS annotation inheritance #18137
Conversation
owen-mc
changed the title
owen-mc
force-pushed
the
java/jax-rs-annotation-inheritance
branch
from
0944ae3 to
12c6f87
Compare
owen-mc
changed the title
owen-mc
force-pushed
the
java/jax-rs-annotation-inheritance
branch
from
12c6f87 to
9cc614a
Compare
There was a problem hiding this comment.
Copilot reviewed 5 out of 9 changed files in this pull request and generated no comments.
Files not reviewed (4)
- java/ql/lib/semmle/code/java/frameworks/JaxWS.qll: Language not supported
- java/ql/test/library-tests/frameworks/JaxWs/JakartaRs1.java: Evaluated as low risk
- java/ql/test/library-tests/frameworks/JaxWs/JakartaRs3.java: Evaluated as low risk
- java/ql/test/library-tests/frameworks/JaxWs/JaxRs1.java: Evaluated as low risk
Comments suppressed due to low confidence (5)
java/ql/test/library-tests/frameworks/JaxWs/JakartaRs4.java:58
- The @OverRide annotation is repeated. Remove the duplicate annotation.
@Override
java/ql/test/library-tests/frameworks/JaxWs/JaxRsInterface.java:27
- The comment should be updated to: 'This annotation has no effect according to the JAX-RS specification, as class/interface annotations are not inherited.'
@Path("/resource") // This annotation has no effect, as class/interface annotations are not inherited in jax-rs
java/ql/test/library-tests/frameworks/JaxWs/JaxRs4.java:63
- The @path annotation with an empty string is unusual and might be a mistake. Consider providing a meaningful path or removing the annotation.
@Path("")
java/ql/test/library-tests/frameworks/JaxWs/JaxRs3.java:34
- The parameter name for @QueryParam is empty. It should be renamed to a meaningful parameter name.
@QueryParam("")
java/ql/test/library-tests/frameworks/JaxWs/JaxRs3.java:67
- The parameter name for @QueryParam is empty. It should be renamed to a meaningful parameter name.
@QueryParam("")
Tip: Copilot only keeps its highest confidence comments to reduce noise and keep you focused. Learn more
owen-mc
changed the title
| @@ -147,6 +147,20 @@ private predicate hasPathAnnotation(Annotatable annotatable) { | |||
| ) | |||
| } | |||
|
|
|||
| /** | |||
| * Holds if the class inherites the JaxRs `@Path` annotation. | |||
There was a problem hiding this comment.
| * Holds if the class inherites the JaxRs `@Path` annotation. | |
| * Holds if the class has or inherits the JaxRs `@Path` annotation. |
| // them. I think this only applies if there are no JaxRS annotations on the | ||
| // class itself. |
There was a problem hiding this comment.
Can you clarify why you think this only applies if there are no JaxRS annotations on the class itself?
The Apache CXF documentation shows that a resource class can have more than one JaxRs annotation, so why couldn't one annotation be inherited and one be directly on the class? Or am I misunderstanding something about how the inheritance works in these cases?
There was a problem hiding this comment.
For method annotations, which the JAX-RS spec does allow inheritance for, the rule is that if there are any JAX-RS annotations on a method then it doesn't inherit any. I've applied the same rule to inheritance of class/interface annotations, but I'm not sure what Apache CXF actually does. It probably doesn't make much difference if I pick the wrong way.
There was a problem hiding this comment.
Sounds reasonable, thanks for clarifying.
| // This is not a resource class because it doesn't have a @Path annotation. | ||
| // Note that inheritance of class or interface annotations is not supported in | ||
| // JAX-RS. | ||
| public class JakartaRs4 implements JakartaRsInterface { // $ RootResourceClass |
There was a problem hiding this comment.
It seems a bit confusing to have a comment saying that this is not a resource class along with a // $ RootResourceClass tag. Would it make sense to reword the comment to something like the below?
| // This is not a resource class because it doesn't have a @Path annotation. | |
| // Note that inheritance of class or interface annotations is not supported in | |
| // JAX-RS. | |
| public class JakartaRs4 implements JakartaRsInterface { // $ RootResourceClass | |
| // By the JAX-RS spec, this is not a resource class because it doesn't | |
| // have a @Path annotation. Inheritance of class or interface annotations | |
| // is not supported in JAX-RS. However, this is a resource class for some | |
| // implementations, like Apache CXF, that allow inheritance of JAX-RS | |
| // annotations. | |
| public class JakartaRs4 implements JakartaRsInterface { // $ RootResourceClass |
| // This is not a resource class because it doesn't have a @Path annotation. | ||
| // Note that inheritance of class or interface annotations is not supported in | ||
| // JAX-RS. | ||
| public class JaxRs4 implements JaxRsInterface { // $ RootResourceClass |
|
@jcogs33 Thanks for spotting those mistakes in the QLDocs. I think I've fixed them now. |
java/ql/test/library-tests/frameworks/JaxWs/JakartaRsInterface.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Jami <[email protected]>
Currrently, our JAX-RS models correctly follow the spec, which says that annotations on classes and interfaces aren't inherited. However, some implementations (like Apache CXF), do allow inheritance of annotations on classes and interfaces. This PR updates our modeling to allow inheritance of those annotations (but only when the class does not have any JAX-RS annotations).
This PR also adds lots of tests.