1 2 Previous Next 25 Replies Latest reply on Aug 19, 2008 6:11 AM by Carlo de Wolf

    MDR doesn't work on annotated privates

    Carlo de Wolf Master

      I don't see annotations on non-overloaded, same named methods.

      public class Super {
       @PostConstruct
       private void postConstruct() {};
      }

      public class Sub extends Super {
       private void postConstruct() {};
      }

      MetaDataRetrievalToMetaDataBridge can't give me back the annotation on Super.postConstruct, because MethodSignature doesn't take class into account.

      This is also a show stopper in ejb3 meta data bridges. Where the class of a method for method meta data is also not taken into account.

        • 1. Re: MDR doesn't work on annotated privates
          Carlo de Wolf Master

          AnnotatedMetaDataLoader will give back the wrong component meta data retrieval if the method signature was obtained via a constructor which does not take Method.

          So the work-around is to use make sure that constructor is used.

          That leaves ejb3 meta data bridge, which for the moment I'm going to refactor onto Method.

          What is the exact function of MethodSignature as opposed to Method?

          • 3. Re: MDR doesn't work on annotated privates
            Adrian Brock Master

             

            "wolfc" wrote:

            What is the exact function of MethodSignature as opposed to Method?


            The idea is that eventually MDR will support loading annotations before the
            class is loaded, e.g. for AOP using javassist in that case
            you can't use the Method as the key, you've got to use the "signature".

            Of course, people that use it at runtime can still use the Method.

            Your problem is that there is no way to define @Inherited for a non-class Annotation.
            http://java.sun.com/j2se/1.5.0/docs/api/java/lang/annotation/Inherited.html

            Sub.class.getMethod("postConstruct").getAnnotation(PostConstruct.class) == null
            is the correct behaviour absent some other semantic/hack
            and therefore it should be the behaviour of the MetaData.

            We could try to simulate this behaviour where you could configure
            somewhere that @PostConstruct should do an @Inherited like behaviour
            at the method level then make the
            AnnotatedElementMetaDataLoader
            do the extra work for those annotations.

            I remember discussing this issue about 6 months ago?
            e.g. whether it should also search interfaces (with the multiple inheritance
            problem it would cause) but nothing came of the discussion.

            • 4. Re: MDR doesn't work on annotated privates
              Carlo de Wolf Master

               

              "adrian@jboss.org" wrote:
              The idea is that eventually MDR will support loading annotations before the
              class is loaded, e.g. for AOP using javassist in that case
              you can't use the Method as the key, you've got to use the "signature".

              Got it, so putting ejb3 meta data bridge onto Method is wrong.
              "adrian@jboss.org" wrote:
              Of course, people that use it at runtime can still use the Method.

              Your problem is that there is no way to define @Inherited for a non-class Annotation.
              http://java.sun.com/j2se/1.5.0/docs/api/java/lang/annotation/Inherited.html

              Sub.class.getMethod("postConstruct").getAnnotation(PostConstruct.class) == null
              is the correct behaviour absent some other semantic/hack
              and therefore it should be the behaviour of the MetaData.

              Ah, but
              Sub.class.getDeclaredMethod("postConstruct").getAnnotation(PostConstruct.class) != null
              which is what I'm interested in.
              "adrian@jboss.org" wrote:
              We could try to simulate this behaviour where you could configure
              somewhere that @PostConstruct should do an @Inherited like behaviour
              at the method level then make the
              AnnotatedElementMetaDataLoader
              do the extra work for those annotations.

              Well in actuality AnnotatedElementMetaDataLoader already works as I would expect it: http://viewvc.jboss.org/cgi-bin/viewvc.cgi/jbossas?view=rev&revision=76978. But because of the semantics of MethodSignature it's luck and not design that drives this (JBMDR-28 being the four-leaf clover).
              "adrian@jboss.org" wrote:
              I remember discussing this issue about 6 months ago?
              e.g. whether it should also search interfaces (with the multiple inheritance
              problem it would cause) but nothing came of the discussion.

              I missed that one.

              I think we need a DeclaredMethodSignature. That should also solve the interfaces issue. Then I can put ejb3 meta data bridge on that one.

              • 5. Re: MDR doesn't work on annotated privates
                Carlo de Wolf Master

                Oops:
                Super.class.getDeclaredMethod("postConstruct").getAnnotation(PostConstruct.class) != null
                Sub.class.getDeclaredMethod("postConstruct").getAnnotation(PostConstruct.class) == null

                • 6. Re: MDR doesn't work on annotated privates
                  Adrian Brock Master

                   

                  "wolfc" wrote:
                  Well in actuality AnnotatedElementMetaDataLoader already works as I would expect it: http://viewvc.jboss.org/cgi-bin/viewvc.cgi/jbossas?view=rev&revision=76978. But because of the semantics of MethodSignature it's luck and not design that drives this (JBMDR-28 being the four-leaf clover).


                  It only works because I removed the validation that the Method was actually
                  part of the class. I did that for performance reasons since it was showing
                  up as a hotspot for AOP joinpoint analysis.

                  I expect the callers to do the right thing, which you aren't. ;-)

                  • 7. Re: MDR doesn't work on annotated privates
                    Adrian Brock Master

                     

                    "wolfc" wrote:

                    I think we need a DeclaredMethodSignature. That should also solve the interfaces issue. Then I can put ejb3 meta data bridge on that one.


                    Why a new signature? The idea of the MetaData is that it should be able to
                    load relevant metadata and the caller doesn't have to think too hard.

                    If we are going to support I'd prefer to have something like:

                    <bean name="MetaDataRepository" ...>
                     <property name="joinpointInheritableAnnotations">
                     <set class="java.lang.Class">
                     <value>javax.ejb.PostConstruct</value>
                     </set>
                     ...
                    


                    Then it would be configuratble which annotations you need to do
                    the wangy search (no suprises or extra work for other annotations)
                    and it would be transparent to the user of MetaData.

                    Incidently, I think this semantic is broken, since it means you cannot
                    override the annotations of the super class. Not very OOPS. :-)

                    public Sub extends Super
                    {
                     @Override
                     // No annotations here
                     public void postConstruct() {}
                    
                     // This is my postConstruct method
                     @PostConstruct
                     public void myPostConstruct() {}
                    }
                    


                    • 8. Re: MDR doesn't work on annotated privates
                      Carlo de Wolf Master

                       

                      "adrian@jboss.org" wrote:
                      Why a new signature? The idea of the MetaData is that it should be able to
                      load relevant metadata and the caller doesn't have to think too hard.

                      If we are going to support I'd prefer to have something like:

                      <bean name="MetaDataRepository" ...>
                       <property name="joinpointInheritableAnnotations">
                       <set class="java.lang.Class">
                       <value>javax.ejb.PostConstruct</value>
                       </set>
                       ...
                      


                      Then it would be configuratble which annotations you need to do
                      the wangy search (no suprises or extra work for other annotations)
                      and it would be transparent to the user of MetaData.

                      Doesn't this contradict the example you're about to give?
                      There the Sub.postConstruct override doesn't have the annotation, but if it's inheritable it would get one.

                      That's not what I want.
                      "adrian@jboss.org" wrote:
                      Incidently, I think this semantic is broken, since it means you cannot
                      override the annotations of the super class. Not very OOPS. :-)

                      public Sub extends Super
                      {
                       @Override
                       // No annotations here
                       public void postConstruct() {}
                      
                       // This is my postConstruct method
                       @PostConstruct
                       public void myPostConstruct() {}
                      }
                      

                      Yes you can, see EJB 3 12.4.1 the last bullet.

                      It's just that if there is a private method with a @PostConstruct it must be invoked regardless of the sub class (which is the weird part).

                      • 9. Re: MDR doesn't work on annotated privates
                        Adrian Brock Master

                         

                        "wolfc" wrote:
                        "adrian@jboss.org" wrote:

                        Then it would be configuratble which annotations you need to do
                        the wangy search (no suprises or extra work for other annotations)
                        and it would be transparent to the user of MetaData.

                        Doesn't this contradict the example you're about to give?
                        There the Sub.postConstruct override doesn't have the annotation, but if it's inheritable it would get one.


                        The point of the example is to show the problem.


                        • 10. Re: MDR doesn't work on annotated privates
                          Adrian Brock Master

                           

                          "wolfc" wrote:

                          Yes you can, see EJB 3 12.4.1 the last bullet.

                          It's just that if there is a private method with a @PostConstruct it must be invoked regardless of the sub class (which is the weird part).


                          So I'm no longer understanding this issue.

                          In my example would Sub::postConstruct() be invoked or not?

                          The part of the EJB3 spec you refer to says the opposite of your "weird part"

                          If a lifecycle callback interceptor method is overridden by another method (regardless of
                          whether that method is itself a lifecycle callback interceptor method (of the same or different
                          type)), it will NOT be invoked.


                          My emphasis.

                          So I don't see why you have to look at annotations on overridden methods.

                          • 11. Re: MDR doesn't work on annotated privates
                            Carlo de Wolf Master

                             

                            "adrian@jboss.org" wrote:
                            So I'm no longer understanding this issue.

                            In my example would Sub::postConstruct() be invoked or not?

                            No, but if Super.postConstruct is private it must be invoked.
                            "adrian@jboss.org" wrote:
                            The part of the EJB3 spec you refer to says the opposite of your "weird part"

                            If a lifecycle callback interceptor method is overridden by another method (regardless of
                            whether that method is itself a lifecycle callback interceptor method (of the same or different
                            type)), it will NOT be invoked.


                            My emphasis.

                            So I don't see why you have to look at annotations on overridden methods.

                            A private method is considered to be non-overriddable. So I need that declaring class somewhere.

                            • 12. Re: MDR doesn't work on annotated privates
                              Adrian Brock Master

                               

                              "wolfc" wrote:

                              So I don't see why you have to look at annotations on overridden methods.

                              A private method is considered to be non-overriddable. So I need that declaring class somewhere.


                              Ok, I understand now. It's not an override.

                              This isn't even really java since without a setAccessible() it's illegal. :-)

                               Sub sub = new Sub();
                               Method method = Super.class.getDeclaredMethod("doSomething");
                               // method.setAccessible(true);
                               method.invoke(sub);
                              
                              Exception in thread "main" java.lang.IllegalAccessException: Class Sub can not access a member of class Super with modifiers "private"
                               at sun.reflect.Reflection.ensureMemberAccess(Reflection.java:65)
                               at java.lang.reflect.Method.invoke(Method.java:588)
                               at Sub.main(Sub.java:36)
                              


                              I guess it's mitigated by the super class having to add the annotation
                              that allows it?

                              Since you don't need to setAccessible() to retrieve annotations from
                              the private super method then I don't think we would be introducing a security
                              hole by supporting it within the MDR (this prints null).

                               Method method = Super.class.getDeclaredMethod("doSomething");
                               //method.setAccessible(true);
                               System.out.println(method.getAnnotation(Inherited.class));
                              


                              I'd say you're idea of a new Signature is probably the easiest way to resolve this.

                              My concern would be whether AOP needs to also use annotations on the
                              method during the invocation and so know to use this Signature?

                              • 13. Re: MDR doesn't work on annotated privates
                                Carlo de Wolf Master

                                 

                                "adrian@jboss.org" wrote:
                                Ok, I understand now. It's not an override.

                                This isn't even really java since without a setAccessible() it's illegal. :-)

                                 Sub sub = new Sub();
                                 Method method = Super.class.getDeclaredMethod("doSomething");
                                 // method.setAccessible(true);
                                 method.invoke(sub);
                                
                                Exception in thread "main" java.lang.IllegalAccessException: Class Sub can not access a member of class Super with modifiers "private"
                                 at sun.reflect.Reflection.ensureMemberAccess(Reflection.java:65)
                                 at java.lang.reflect.Method.invoke(Method.java:588)
                                 at Sub.main(Sub.java:36)
                                


                                I guess it's mitigated by the super class having to add the annotation
                                that allows it?

                                Ehr... nope, not really. You're still thinking to cleanly. ;-)
                                "adrian@jboss.org" wrote:
                                Since you don't need to setAccessible() to retrieve annotations from
                                the private super method then I don't think we would be introducing a security
                                hole by supporting it within the MDR (this prints null).

                                 Method method = Super.class.getDeclaredMethod("doSomething");
                                 //method.setAccessible(true);
                                 System.out.println(method.getAnnotation(Inherited.class));
                                


                                I'd say you're idea of a new Signature is probably the easiest way to resolve this.

                                My concern would be whether AOP needs to also use annotations on the
                                method during the invocation and so know to use this Signature?

                                Not right now, because the EJB3 container sets up a manual interceptor chain. In future this chain should also come from aop.xml, then it might become a problem.

                                Note that both web container & client container have so far been left out of scope.

                                Is there a Jira for the new method signature?

                                • 14. Re: MDR doesn't work on annotated privates
                                  Adrian Brock Master

                                   

                                  "wolfc" wrote:

                                  My concern would be whether AOP needs to also use annotations on the
                                  method during the invocation and so know to use this Signature?

                                  Not right now, because the EJB3 container sets up a manual interceptor chain. In future this chain should also come from aop.xml, then it might become a problem.

                                  Note that both web container & client container have so far been left out of scope.


                                  That's not what I meant. If the method is enhanced using an AOP aspect,
                                  it may want access to the annotations, e.g.

                                  @PostConstruct
                                  @LogCalls(level=Leves.TRACE)
                                  private void postConstruct() {}
                                  
                                  @Aspect
                                  public CallLogging
                                  {
                                   public Object invoke(MethodInvocation invocation) throws Throwable
                                   {
                                   // This effectively does
                                   // advisor.getMetaData().getComponentMetaData(new MethodSignature(invocation.getMethod())).getAnnotation(LogCalls.class)
                                   LogCalls logCalls = invocation.getAnnotation(LogCalls.class);
                                  
                                   // Impl
                                   doLog(logCalls, invocation);
                                   }
                                  }
                                  


                                  Which would kind of work as it currently stands (since it doesn't validate the method).

                                  The "kind of" is because it is not intended to work, and wouldn't work
                                  if you don't use the constructor with the Method in it.

                                  If you don't pass the Method object, it has to search for the Method
                                  and will instead find the one in Sub.class.

                                  Is there a Jira for the new method signature?


                                  No JIRA, since we haven't decided what the proper solution is yet.

                                  As I see it there are currently two possible solutions:

                                  1) Your's which is to introduce a new Signature, but this is going to be
                                  non-transparent to users, at least it doesn't interfere with more normal usages.

                                  2) Morph MethodSignature to understand the "optional" class as part of the key.

                                  But this would complicate the equals()/hashCode() used to store
                                  and retrieve component metadata, e.g. Instance level metadata
                                  populated from the xml

                                  It would no longer be a simple get/put() semantic which would make writing
                                  MetaDataLoaders overly complicated and error prone.

                                  Callers who don't use the Method constructor would still have to know/pass the
                                  declaring class when constructing the signature to get the super method.

                                  1 2 Previous Next