Bug 386410 - [override method] JAX RS interface annotations are duplicated in the implementing class
Summary: [override method] JAX RS interface annotations are duplicated in the implemen...
Status: VERIFIED FIXED
Alias: None
Product: JDT
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 4.2   Edit
Hardware: PC Windows XP
: P3 normal (vote)
Target Milestone: 4.3 M3   Edit
Assignee: Markus Keller CLA
QA Contact:
URL:
Whiteboard:
Keywords:
: 350396 (view as bug list)
Depends on:
Blocks: 470426
  Show dependency tree
 
Reported: 2012-08-01 12:30 EDT by John David CLA
Modified: 2017-07-07 08:11 EDT (History)
6 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description John David CLA 2012-08-01 12:30:06 EDT
When I create a new interface method with annotations and have assist create it in the implementing class, @Override is added to the method but all the other annotations are also included...

Example...
Interface code:
================
@Path("perm")
public interface PermResource
{
...

  @GET
  @Path("{id}/{level}")
  @Produces(MediaType.APPLICATION_XML)
  public String getFooTest(@PathParam("id") String id,
      @PathParam("level") String level, @Context SecurityContext sc);

}
================
When I go to the implementing class and click to "Add unimplemented mothods"...

I get...
================

public class PermImpl implements PermResource
{
...
  @Override
  @GET
  @Path("{id}/{level}")
  @Produces("application/xml")
  public String getFooTest(@PathParam("id") String id,
      @PathParam("level") String level, @Context SecurityContext sc)
  {
    // TODO Auto-generated method stub
    return null;
  }

}
=====
In Indigo, the method created would have looked like this...
====
  @Override
  public String getFooTest(String id, String level, SecurityContext sc)
  {
    // TODO Auto-generated method stub
    return null;
  }
=====

Please let me know if you need anything else.
Comment 1 Markus Keller CLA 2012-08-22 15:01:06 EDT
Since bug 353472, we copy all annotation with @Retention CLASS or RUNTIME.

I don't know/use JAX RS. Is the code you cited a common usage pattern of these annotations?

Annotations are not inherited by overriding methods -- they usually have to specified again on the each method declaration. Specifying these annotations on the interface method looks wrong to me. Are they really active there? How do they know about the implementation class?
Comment 2 John David CLA 2012-08-22 15:41:59 EDT
Join the club.  I don't know it that well either.
The implementing class is what gets registered with Jersey (JAX/RS server).
It must backtrack to the interface somehow to get the annotations.

I think you'll need to talk to someone more familiar with the Jersey architecture/internals.

I just noticed that in Juno, the annotations are getting replicated while in Indigo, they were not.

Perhaps a new Eclipse property to specify whether interface annotations should be replicated  on the implementing class or not?
Comment 3 Markus Keller CLA 2012-08-23 07:12:12 EDT
I did a bit of research, and it really looks like JAX-RS inherits annotations from overridden class and interface methods. This is a deviation from the normal behavior of annotation processors.

The right solution is to define a meta-annotation that specifies this behavior on annotations like JAX-RS's @GET, @Path, etc. . A good way to do this would be to enhance the java.lang.annotation.Inherited meta-annotation to
a) also be defined to work for methods
b) add an annotation type element like this:
    boolean inheritsFromInterfaces() default false;

Then, the JAX-RS annotations could be annotated with
@Inherited(inheritsFromInterfaces = true)

Otherwise, the only solution would be that tooling like Eclipse has to maintain a list of annotations that don't behave in a regular fashion. We don't have plans to do that.

If you want to see this problem solved, please contact your JAX-RS implementation provider and/or the JAX-RS 2.0 expert group (JSR 339) and ask them to make the inheritance explicit.
Comment 4 Markus Keller CLA 2012-10-25 14:16:37 EDT
It looks like waiting on a meta-annotation that specifies annotation inheritance is illusory, see bug 388281 comment 23.

Since we didn't have other requests than bug 353472 for automatic copying of annotations, and it some annotation processors do automatic inheritance of their annotations, I've reverted the general copying for now.

We will keep copying nullity-annotations according to the rules in bug 388281 comment 25 (only copy if null annotation inheritance is disabled).

Fixed with http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=a069d916fe4e18a29b7548823b72682530476ce9
Comment 5 Stephan Herrmann CLA 2012-10-25 18:44:11 EDT
(In reply to comment #4)
> We will keep copying nullity-annotations according to the rules in bug
> 388281 comment 25 (only copy if null annotation inheritance is disabled).

Thanks!
Comment 6 Paul Benedict CLA 2012-10-26 00:25:32 EDT
Also see Bug 350396
Comment 7 Markus Keller CLA 2012-10-26 07:08:59 EDT
*** Bug 350396 has been marked as a duplicate of this bug. ***
Comment 8 Markus Keller CLA 2012-10-26 08:25:02 EDT
I'm a bit unsure about Pull Up and Push Down. If you choose to move the complete method, then it's clear we have to move the annotations as well. But if you leave the method abstract, or pull up / push down and don't delete the original method, should we also move the annotations? Or copy them? Or always have them on the topmost method? I think the "right" answer again depends on the concrete annotation processor.

For now, Pull Up, Push Down and Extract Superclass do move/copy annotations.

Extract Interface doesn't copy annotations to the super interface and Override/Implement Methods doesn't copy annotations to the new method.

If someone has use cases where this behavior is bad, please file a new bug and mention this bug id.
Comment 9 Stephan Herrmann CLA 2012-10-28 11:37:09 EDT
You mentioned special-casing nullity annotations for copy.

Let me ask specifically about the different use cases:

(In reply to comment #8)
> For now, Pull Up, Push Down and Extract Superclass do move/copy annotations.

According to previous comments any duplication of annotations should be disabled if inheritance is enabled, right?
 
> Extract Interface doesn't copy annotations to the super interface and
> Override/Implement Methods doesn't copy annotations to the new method.

Extract Interface must establish null annotations in the interface, otherwise implementers can become illegal (independent of enablement of inheritance).

Override/implement should copy by default (unless inheritance is enabled).

> If someone has use cases where this behavior is bad, please file a new bug
> and mention this bug id.

I assume you already took care of most or all of the above, did you?
Comment 10 Markus Keller CLA 2012-10-29 16:42:31 EDT
> > You mentioned special-casing nullity annotations for copy.
Yes, I left that out of the summary.

> > For now, Pull Up, Push Down and Extract Superclass do move/copy annotations.
I didn't touch those now nor for bug 353472. So all annotations are currently  duplicated like before. We can look at this later, if it's really a big deal.

> > Extract Interface doesn't copy annotations to the super interface and
> > Override/Implement Methods doesn't copy annotations to the new method.
> 
> Extract Interface must establish null annotations in the interface,
> otherwise implementers can become illegal (independent of enablement of
> inheritance).

That's exactly the opposite of what bug 350396 requests. The problem is that null annotations are in two ways "special":
a) they can be inherited (if the new option is enabled)
b) they are actually meant to be TYPE_USE annotations, in JSR308-speak. TYPE_USE annotations should be copied to super- & sub-methods, since they are part of the method signature. If they use an inheritance mechanism like the null annotations, then they should only appear on the topmost method declarations in the inheritance tree.

The "move method up and remove annotations in original method" is something new for inherited null annotations. I've implemented that for Extract Interface with http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=e9f344044289ad4147a2dcffa73b5766d4121140

> Override/implement should copy by default (unless inheritance is enabled).
That's still in for null annotations as a special case.
Comment 11 Dani Megert CLA 2012-10-30 12:17:44 EDT
Verified in 4.3-I20121029-2000.