6 Replies Latest reply on Dec 5, 2007 6:46 PM by Scott Stark

    ReferenceMetaDataResolverDeployer is wrong

    Adrian Brock Master

      The ReferenceMetaDataResolverDeployer (RMDRD) is wrong
      in that it updates the mapped-name/jndi-name with the resolved jndi-name.

      This breaks the reversability of the model and will also confuse things
      with the profile service.

      Example:
      I have ejb1 with jndi-name="ejb1jndiName"
      I have ejb2 with ejb-link="ejb1"

      The RMDRD will do EJBReference.setMappedName("ejb1JndiName");

       if(link != null)
       target = findEjbLink(unit, link);
       if(target == null)
       unresolvedRefs.add(ref.getEjbRefName()+"/ejb-ref/"+link);
       else
       ref.setMappedName(target);
      


      When the ManagedObjects are retrieved for this deployment,
      the EJBReferfence now has a mapped-name and so it will be saved
      and never re-resolved on a deploy from the profile service.

      If I then use the managed console (profile service) to change the
      jndi-name of ejb1 ejb1JndiName -> ejb1ChangedName
      the EJBReference is now invalid, but it won't be re-resolved because
      the mapped-name is not null.

      Instead what should be happening is that on the ResourceInjectionMetaData
      there should be:
      private String resolvedJndiName;
      
      public String getResolvedJndiName()
      {
       return resolvedJndiName;
      }
      
      @XmlTransient // not included in xml model
      public void setResolvedJndiName(String jndiName)
      {
       this.resolvedJndiName = resolvedJndiName;
      }
      


      The RMDRD should setting the resolvedJndiName and the
      containers should be using getResolvedJndiName(), rather than getMapped/JndiName().

        • 1. Re: ReferenceMetaDataResolverDeployer is wrong
          Adrian Brock Master

          Also, shouldn't this be:

           if(unresolvedPaths != null && unresolvedPaths.size() > 0)
          - log.warn("Unresolved references exist in JBossMetaData: "+unresolvedPaths);
          + throw new DeploymentException("Unresolved references exist in JBossMetaData: "+unresolvedPaths);
          


          Why does it try to muddle on, only to fail later when creating the ejb containers?


          • 2. Re: ReferenceMetaDataResolverDeployer is wrong
            Adrian Brock Master

            Also, I'm seeing a lot of this hackery in the new metadata.
            What's wrong with using OO?

            private static String getJndiName(JBossEnterpriseBeanMetaData beanMD, boolean isLocal)
             {
             String jndiName = null;
             if (isLocal)
             {
             // Validate that there is a local home associated with this bean
             jndiName = beanMD.determineLocalJndiName();
             if (jndiName == null)
             {
             log.warn("LocalHome jndi name requested for: '" + beanMD.getEjbName() + "' but there is no LocalHome class");
             }
             }
             else
             {
            - if( beanMD.isEntity() )
            - {
            - JBossEntityBeanMetaData md = (JBossEntityBeanMetaData) beanMD;
            - jndiName = md.getJndiName();
            - }
            - else if( beanMD.isSession())
            - {
            - JBossSessionBeanMetaData md = (JBossSessionBeanMetaData) beanMD;
            - jndiName = md.getHomeJndiName();
            - if(jndiName == null)
            - jndiName = md.getJndiName();
            - }
            + jndiName = md.determineRemoteJndiName();
             }
             return jndiName;
             }
            


            And move the if statements into the types?

            • 3. Re: ReferenceMetaDataResolverDeployer is wrong
              Adrian Brock Master

              Finally, the algorithm is very inefficient and is wrong anway.

              You effectively have:

              for (Deployment Unit unit : deployments)
              {
               for (InjectionTargetMetaData target : unit.getInjectMetaDatas)
               {
               ArrayList<JBossMetaData> allEJBDeployments = unit.getTopLevelUnit().getAllAttachments(JBossMetaData.class);
               for (JBossMetaData jbossMetaData : allEJBDeployments)
               {
               search(jbossMetaData);
               }
               }
              }
              


              It is rebuilding the allEJBDeployments for every reference that gets resolved.

              Surely it would be much more efficient to create some sort of index/repository
              in a previous step which the resolver could quickly use.

              In fact, the whole approach is totally wrong, since an ejb-reference
              can come from a ejb deployment that has not been deployed yet.
              It should not fail, it should wait.

              The correct solution would be not to try to resolve straight away
              but to set up a dependency for the ejb (or deployment) and populate a registry
              that can be checked to do the resolution.
              This would only allow the ejb to proceed to full deployment
              when the reference can be resolved in the reference registry.

              i.e. there would be no occurance when a DeploymentException is explicitly thrown,
              instead it would appear as an unresolved dependency in the IncompleteDeploymentException.

              • 4. Re: ReferenceMetaDataResolverDeployer is wrong
                Adrian Brock Master

                 

                "adrian@jboss.org" wrote:
                Also, I'm seeing a lot of this hackery in the new metadata.
                What's wrong with using OO?

                private static String getJndiName(JBossEnterpriseBeanMetaData beanMD, boolean isLocal)
                 {
                 String jndiName = null;
                 if (isLocal)
                 {
                 // Validate that there is a local home associated with this bean
                 jndiName = beanMD.determineLocalJndiName();
                 if (jndiName == null)
                 {
                 log.warn("LocalHome jndi name requested for: '" + beanMD.getEjbName() + "' but there is no LocalHome class");
                 }
                 }
                 else
                 {
                - if( beanMD.isEntity() )
                - {
                - JBossEntityBeanMetaData md = (JBossEntityBeanMetaData) beanMD;
                - jndiName = md.getJndiName();
                - }
                - else if( beanMD.isSession())
                - {
                - JBossSessionBeanMetaData md = (JBossSessionBeanMetaData) beanMD;
                - jndiName = md.getHomeJndiName();
                - if(jndiName == null)
                - jndiName = md.getJndiName();
                - }
                + jndiName = md.determineRemoteJndiName();
                 }
                 return jndiName;
                 }
                


                And move the if statements into the types?


                In fact, there already is a determineJndiName() for the remote jndi name.
                It should be using that, otherwise it doesn't do the jndi-name=ejb-name
                when the jndi name is not explicitly specified.

                It still needs fixing to be OO, see the comment in the code.

                [ejort@warjort server]$ svn diff
                Index: src/main/org/jboss/deployment/ReferenceMetaDataResolverDeployer.java
                ===================================================================
                --- src/main/org/jboss/deployment/ReferenceMetaDataResolverDeployer.java (revision 67880)
                +++ src/main/org/jboss/deployment/ReferenceMetaDataResolverDeployer.java (working copy)
                @@ -408,17 +408,19 @@
                 }
                 else
                 {
                + // FIXME the session.getHomeJndiName should be moved to the JBossSessionMetaData.determineJndiName()
                + // and these if statements replaced with md.determineJndiName()
                 if( beanMD.isEntity() )
                 {
                 JBossEntityBeanMetaData md = (JBossEntityBeanMetaData) beanMD;
                - jndiName = md.getJndiName();
                + jndiName = md.determineJndiName();
                 }
                 else if( beanMD.isSession())
                 {
                 JBossSessionBeanMetaData md = (JBossSessionBeanMetaData) beanMD;
                 jndiName = md.getHomeJndiName();
                 if(jndiName == null)
                - jndiName = md.getJndiName();
                + jndiName = md.determineJndiName();
                 }
                 }
                 return jndiName;
                


                • 5. Re: ReferenceMetaDataResolverDeployer is wrong
                  Scott Stark Master

                  This is currently effectively just a merge of the new metadata and the old EjbUtil50 resolution logic. The determination of the name is further complicated by the default naming policy, as mentioned in this ejb3 thread:
                  http://www.jboss.com/index.html?module=bb&op=viewtopic&t=124975

                  I think the correct solution is to have the ReferenceMetaDataResolverDeployer determine the target bean, and create a DependencyItem for the actual jndi name. How this works with different container models is the question. All references will need to be processed by the same reference/injection handling code that is driven by these dependencies.

                  • 6. Re: ReferenceMetaDataResolverDeployer is wrong
                    Scott Stark Master

                    I have added a resolvedJndiName property to the ResourceInjectionMetaData.