13 Replies Latest reply on Nov 16, 2007 5:34 PM by Paul Gier

    New behaviour of AbstractMetaDataContext.isEmpty()

    Kabir Khan Master

      This code in AOPConstructorJoinpoint

       private boolean hasInstanceOrJoinpointMetaData(MetaData metaData)
       {
       if (metaData == null)
       {
       return false;
       }
      
       MetaData instanceMetaData = metaData.getScopeMetaData(CommonLevels.INSTANCE);
       if (instanceMetaData != null && instanceMetaData.isEmpty() == false)
       {
       return true;
       }
       //Check for method annotations
       return hasMethodMetaData(metaData);
       }
      


      The instanceMetaData.isEmpty() check used to return false for the following case:

      @Test
      public class AnnotatedChild extends Base
      {
       public static boolean childInvoked;
       public void childOnly()
       {
       childInvoked = true;
       }
      
       public void baseOverridden()
       {
       childInvoked = true;
       }
      }
      


      <deployment xmlns="urn:jboss:bean-deployer:2.0">
      
       ...
       <bean name="AnnotatedChild" class="org.jboss.test.microcontainer.matrix.AnnotatedChild"/>
      </deployment>
      


      It now returns true, this breaks loads of the aop/mc int tests.

      The instanceMetaData.toString() method shows
      org.jboss.metadata.spi.retrieval.MetaDataRetrievalToMetaDataBridge@1286c00{[JVM=THIS, CLASS=org.jboss.test.microcontainer.matrix.AnnotatedChild, INSTANCE=AnnotatedChild, WORK=1209895]}
      


      and stepping into the isEmpty() method, the retrieval/MemoryMetaDataLoader with ScopeKey:
      [INSTANCE=AnnotatedChild]
      is empty, but it then calls isEmpty() on the parent MetaDataContext, which contains the following retrievals and associated SkopeKeys
      MemoryMetaDataLoader [WORK=1209895]
      MemoryMetaDataLoader [INSTANCE=AnnotatedChild]
      AnnotatedElementMetaDataLoader [CLASS=org.jboss.test.microcontainer.matrix.AnnotatedChild]
      MemoryMetaDataLoader [JVM=THIS]
      


      The CLASS level one has annotations, so isEmpty() ends up returning false.

      Is this a bug or should I be checking if it is empty another way?

        • 1. Re: New behaviour of AbstractMetaDataContext.isEmpty()
          Kabir Khan Master

          The tests are here:

          $cd aop-mc-int
          $ mvn surefire-report:report -Pant-tests-weave
          

          There are quite many tests, but the ones that fail now are mostly in the org.jboss.test.microcontainer.matrix.mc.test package.

          If you want to run just one in IDEA, you could run org.jboss.test.microcontainer.matrix.mc.test.BaseNotAdvisedChildAdvisedAndProxyTestCase, making sure that you turn on weaving by passing in the following system property
          -Djava.system.class.loader=org.jboss.aop.standalone.SystemClassLoader
          


          • 2. Re: New behaviour of AbstractMetaDataContext.isEmpty()
            Ales Justin Master

            I'll have a look at that, since I wrote that. ;-(

            I remember having a todo on tests, but forgot, since no exception occurred.

            Btw: why are these tests excluded from maven tests?

            • 3. Re: New behaviour of AbstractMetaDataContext.isEmpty()
              Kabir Khan Master

              These tests are not being run automatically, I've asked before why on another thread. I think Paul mentioned that the reason for this is that "they take a long time". Anyway, looking at the code change that caused this they would have been broken for about 2 months now, so I think they should be enabled.

              I agree that they don't necesarily need to be run when doing a local "mvn install", but they should be enabled when being run on Hudson.

              The tests are not part of the normal profile, to run the aop-mc-int tests you have to

              $cd aop-mc-int
              $mvn surefire-report:report -Ptests-no-weave
              $mvn surefire-report:report -Ptests-no-weave-secure
              $mvn surefire-report:report -Pant-tests-weave
              $mvn surefire-report:report -Pant-tests-weave-secure
              


              • 4. Re: New behaviour of AbstractMetaDataContext.isEmpty()
                Adrian Brock Master

                 

                "kabir.khan@jboss.com" wrote:

                The instanceMetaData.toString() method shows
                org.jboss.metadata.spi.retrieval.MetaDataRetrievalToMetaDataBridge@1286c00{[JVM=THIS, CLASS=org.jboss.test.microcontainer.matrix.AnnotatedChild, INSTANCE=AnnotatedChild, WORK=1209895]}
                



                That's clearly wrong. The invocation of:

                MetaData instanceMetaData = metaData.getScopeMetaData(CommonLevels.INSTANCE);
                should be giving you back a MetaData object that only contains the
                INSTANCE=AnnotatedChild.

                The bug is fairly obvious.
                The single scope metadata shouldn't have a parent! :-)
                 public MetaData getScopeMetaData(ScopeLevel level)
                 {
                 if (level == null)
                 throw new IllegalArgumentException("Null scope level");
                
                 if (retrieval instanceof MetaDataContext)
                 {
                 MetaDataContext context = (MetaDataContext)retrieval;
                 List<MetaDataRetrieval> matchingRetrievals = new ArrayList<MetaDataRetrieval>();
                 List<MetaDataRetrieval> localRetrievals = context.getLocalRetrievals();
                 for (MetaDataRetrieval localRetrieval : localRetrievals)
                 {
                 ScopeKey scopeKey = localRetrieval.getScope();
                
                 if (scopeKey.getScopeLevel(level) != null)
                 matchingRetrievals.add(localRetrieval);
                 }
                 if (matchingRetrievals.isEmpty() == false)
                - return new MetaDataRetrievalToMetaDataBridge(new AbstractMetaDataContext(context, matchingRetrievals));
                + return new MetaDataRetrievalToMetaDataBridge(new AbstractMetaDataContext(null, matchingRetrievals));
                 }
                 else if (retrieval.getScope().getScopeLevel(level) != null)
                 return new MetaDataRetrievalToMetaDataBridge(new AbstractMetaDataContext(retrieval));
                 return null;
                 }
                


                But on a related issue, I don't like the fact that this logic is in the bridge.
                The bridge should be just delegating the request to the retrieval.
                i.e. a plain retrieval will just match its scope level while a context
                will have to filter its local retrievals. It's more OO.

                • 5. Re: New behaviour of AbstractMetaDataContext.isEmpty()
                  Paul Gier Master

                  If you want, I can turn them all on by default. Or just turn them all on in hudson. I'll have to check with Prabhat to see if we can configure hudson to display all the test results nicely since they aren't in the normal location.

                  • 6. Re: New behaviour of AbstractMetaDataContext.isEmpty()
                    Kabir Khan Master

                    I think you could perhaps run each aop-mc-int profile as a separate hudson run, and make them dependent on the main MC run, or the main MC run triggers these separate runs. I am not sure exactly how that works, but Ryan Campbell did something like that for the aop tests

                    http://www.jboss.com/index.html?module=bb&op=viewtopic&t=122864

                    • 7. Re: New behaviour of AbstractMetaDataContext.isEmpty()
                      Adrian Brock Master

                       

                      "adrian@jboss.org" wrote:

                      But on a related issue, I don't like the fact that this logic is in the bridge.
                      The bridge should be just delegating the request to the retrieval.
                      i.e. a plain retrieval will just match its scope level while a context
                      will have to filter its local retrievals. It's more OO.


                      i.e. There should be a
                      List<MetaDataRetrieval> getScopedRetrievals(ScopeLevel level)
                      

                      on the retrieval interface and the two branches of the if statement
                      moved into the relevant abstract implementation (context or retrieval).

                      P.S. I'd also guess that the testcase for this functionality isn't very good. ;-)

                      • 9. Re: New behaviour of AbstractMetaDataContext.isEmpty()
                        Ales Justin Master

                         

                        "adrian@jboss.org" wrote:

                        i.e. There should be a
                        List<MetaDataRetrieval> getScopedRetrievals(ScopeLevel level)
                        


                        This should be enough:
                         MetaData getScopeMetaData(ScopeLevel level);
                        

                        looking at the code from the bridge
                         public MetaData getScopeMetaData(ScopeLevel level)
                         {
                         if (level == null)
                         throw new IllegalArgumentException("Null scope level");
                        
                         MetaDataRetrieval scopedRetrieval = retrieval.getScopedRetrieval(level);
                         if (scopedRetrieval != null)
                         return new MetaDataRetrievalToMetaDataBridge(scopedRetrieval);
                        
                         return null;
                         }
                        

                        where in context I have this
                         public MetaDataRetrieval getScopedRetrieval(ScopeLevel level)
                         {
                         List<MetaDataRetrieval> matchingRetrievals = new ArrayList<MetaDataRetrieval>();
                         List<MetaDataRetrieval> localRetrievals = getLocalRetrievals();
                         for (MetaDataRetrieval localRetrieval : localRetrievals)
                         {
                         ScopeKey scopeKey = localRetrieval.getScope();
                         if (scopeKey.getScopeLevel(level) != null)
                         matchingRetrievals.add(localRetrieval);
                         }
                        
                         if (matchingRetrievals.isEmpty() == false)
                         return new AbstractMetaDataContext(null, matchingRetrievals);
                        
                         return null;
                         }
                        

                        and in retrieval
                         public MetaDataRetrieval getScopedRetrieval(ScopeLevel level)
                         {
                         if (getScope().getScopeLevel(level) != null)
                         return new AbstractMetaDataContext(this);
                        
                         return null;
                         }
                        


                        A question here. :-)
                        Why do I need to wrap retrieval in context in this last code snippet?
                        OK, I know this was my old code :-), but can't see it now why I wrapped it.
                        Perhaps a port of previous code from AOPConstructorJoinpoint?

                        • 10. Re: New behaviour of AbstractMetaDataContext.isEmpty()
                          Adrian Brock Master

                           

                          "alesj" wrote:

                          A question here. :-)
                          Why do I need to wrap retrieval in context in this last code snippet?
                          OK, I know this was my old code :-), but can't see it now why I wrapped it.
                          Perhaps a port of previous code from AOPConstructorJoinpoint?


                          It looks redundant to me, but it does no harm. ;-)
                          The same is true for the other one if matchingRetrievals.size() == 1

                          • 11. Re: New behaviour of AbstractMetaDataContext.isEmpty()
                            Ales Justin Master

                             

                            "adrian@jboss.org" wrote:
                            It looks redundant to me, but it does no harm. ;-)

                            So you're saying this is the same - for AbstractMetaDataLoader:
                             if (getScope().getScopeLevel(level) != null)
                             return new AbstractMetaDataContext(this);
                            

                            and
                             if (getScope().getScopeLevel(level) != null)
                             return this;
                            


                            OK, no real harm, but the second one leaves AbstractMetaDataLoader clean from importing un-spi, un-retrieval packages. :-)

                            • 12. Re: New behaviour of AbstractMetaDataContext.isEmpty()
                              Ales Justin Master

                               

                              "pgier" wrote:
                              If you want, I can turn them all on by default. Or just turn them all on in hudson. I'll have to check with Prabhat to see if we can configure hudson to display all the test results nicely since they aren't in the normal location.

                              Could you make this optional?
                              By default only on Hudson.
                              But if we used some sys property, then they would also pop-up in local tests.

                              • 13. Re: New behaviour of AbstractMetaDataContext.isEmpty()
                                Paul Gier Master

                                I made the aop-mc-int tests on by default now. I tested them in hudson here:
                                http://hudson.qa.jboss.com/hudson/job/Microcontainer/org.jboss.microcontainer$jboss-aop-mc-int/730/

                                They can be turned off using the property "aop.tests.skip"
                                So from the microcontainer root or within the aop module:

                                mvn -Daop.tests.skip install


                                I also added some comments about usage to top of the aop-mc-int pom.

                                I can make them off by default it that is better by just reversing the way that the property works.