11 Replies Latest reply on Aug 13, 2009 8:53 AM by kabirkhan

    Abstract Beans JBKERNEL-10

    marius.bogoevici

      I added a prototype (read partial) implementation for Abstract Beans, and uploaded the patch in Jira (JBKERNEL-10).

      What I did was:

      - introduce a new ControllerMode called ABSTRACT
      - install abstract beans as controller contexts with ABSTRACT mode, which are never progressing past the DESCRIBED state
      - modify the initialVisitor to merge parent data with current bean data (i.e. copy metadata from parent if missing in child or merge if it is a collection - this allows child beans to override parent bean settings).

      Copying metadata from parent to child works - what needs to be done is to make sure that this is done in a semantically correct way for all types of metadata, for eliminating ambiguities.

      One issue I encountered is that metadata such as PropertyMetaData is stored in a Set in the BeanMetaData, yet PropertyMetaData does not implement equals() and hashCode().

      This leads to the possibility of writing definitions such as:

      <bean ... >
       <property name="someProperty">Value1</property>
       <property name="someProperty">Value2</property>
      </bean>
      


      , and given that both elements will generate a PropertyMetaData and they will be stored in a HashSet, it is hard to tell which one of the two will be used to set the value when the bean is instantiated.

      Currently, merging the two property sets (parent and child) can produce a similar effect, if the same property is defined in the parent and child. It is possible to elliminate duplicates in the merging process,but the main challenge is in making this process generic.

      Using an implementation for equals() that is based on PropertyMetaData.getName() would be a poor hack, since it is hard to say that two PropertyMetaData instances having the same name but different values are equal. I am currently considering using a Map for the merging process, and we might use something similar inside AbstractBeanMetaData.



        • 1. Re: Abstract Beans JBKERNEL-10
          dmlloyd

           

          "marius.bogoevici" wrote:

          One issue I encountered is that metadata such as PropertyMetaData is stored in a Set in the BeanMetaData, yet PropertyMetaData does not implement equals() and hashCode().

          This leads to the possibility of writing definitions such as:


          I ran into this as well. To add to the strangeness, the AbstractBeanMetaData.getProperty(String) method iterates the whole set looking for a matching property, incurring a O(n) overhead. BeanMetaDataBuilder does much the same (in this case, iterating the whole set to remove the duplicate-named property).

          In my opinion, the backing structure (in both BMD and BMDB) should be changed to a LinkedHashMap<String,PropertyMetaData> ensuring that properties are installed uniquely and in order.

          • 2. Re: Abstract Beans JBKERNEL-10
            marius.bogoevici

            Since parsing is done using JAXB, I think we still need to use a Collection (Set is alright, although it does not take advantage of any uniqueness feature of PropertyMetaData). Maps would be mapping somehow differently in JAXB, and AFAICT you can't actually use a HashMap per se as the mapped property, much less so a LinkedHashMap.

            I would rather think of using a set for JAXB and a map internally, something like this:

            ...
            @XmlTransient
            private Map<Object, PropertyMetaData> propertyMetaData = ;
            
            
            public void setPropertyMetaData(Set<PropertyMetaData> parsedProperties) {
             this.propertyMetaData = new LinkedHashMap();
             <add all the elements of parsedProperties into the Map>
            }
            
            
            @XmlElement("property")
            public Set<PropertyMetaData> getPropertyMetaData() {
             return new HashSet<PropertyMetaData>(this.propertyMetaData.values();
            }
            


            • 3. Re: Abstract Beans JBKERNEL-10
              dmlloyd

              Yeah, that was my thinking as well. There is a lesson to take away from this however: defining your API object model around JAXB makes for ugliness and bad things unless you're extraordinarily careful and clever. :-)

              • 4. Re: Abstract Beans JBKERNEL-10
                alesj

                Some thoughts about this approach.

                I wouldn't put abstract definitions into Controller (under Descibed),
                as I think they would do more harm than good on the overall impact.

                I would keep them in some nice, useful registry,
                where I would also be able to define proper dependencies on them.
                e.g. real bean deployed before its abstract parent, would mean dependency item on this abstract-registry

                This should be an impl detail of AbstractKernelController,
                where the real abstract handling code would be separated.

                Or why would you want to keep it as part of Controller?

                Wrt Properties.
                I guess both of your use cases proved we need better impl.
                Kabir, can you please have a look at what we can do there?

                • 5. Re: Abstract Beans JBKERNEL-10
                  marius.bogoevici

                   

                  "alesj" wrote:

                  Or why would you want to keep it as part of Controller?


                  I guess, I should answer this first. Initially, I tried to use BeanMetaData directly, but the problem I've been facing was that I found no means to retrieve the BeanMetaData directly, other than through the ContextController that was created through it.

                  To me, we have 2 different problems that we have to solve: abstract beans and parent-child inheritance. They are different, because parent-child inheritance should be possible even if the parent is not abstract.

                  Also, I considered the possibility that the parent and child are to be found in different deployments, even different scopes (with the parent being in the broader scope, obviously).

                  Also, one needs to be consider that the parent can be undeployed or redeployed (what happens in this case? I would think that if we associate abstract beans with controller contexts, we could have dependencies and undeploy the child beans automatically).

                  So, overall, the reason why I think this could be done this way is because I found out that the metadata management, as it is currently implemented, is based on the Controller/ControllerContext mechanism, once the BeanMetaData has been parsed and deployed by the Deployer all the references to it are held by ControllerContexts and managed via the Controller. This means that for getting the metadata for a particular bean, I have to go to the ControllerContext that has been created for that metadata. Of course, we can have better separation between metadata and the actual ControllerContext instances by introducing a registry holding the MetaData prior to deployment, but I think that this should also come at a price of a refactoring, to avoid duplicating features, such as scoping, for example, so I am not to sure how justified it is from the effort point of view. Of course, you know the MC much better inside out, so I'd like to know what your thoughts are about this.

                  "alesj" wrote:


                  I wouldn't put abstract definitions into Controller (under Described),
                  as I think they would do more harm than good on the overall impact.



                  Do you have in mind anything specific or is this a general statement?

                  Since there is a clear distinction between "DESCRIBED" and "INSTANTIATED" and "INSTALLED", I thought that it would be clear that the ControllerContext is not been backed by an actual instance, and this has the advantage that we can use all the existing mechanisms for management, scoping etc. But I'm interested to hear more on the topic.

                  "alesj" wrote:

                  I would keep them in some nice, useful registry,
                  where I would also be able to define proper dependencies on them.
                  e.g. real bean deployed before its abstract parent, would mean dependency item on this abstract-registry

                  This should be an impl detail of AbstractKernelController,
                  where the real abstract handling code would be separated.



                  See above. If we were to have a definition registry, I think it should hold all the definitions (abstract and non-abstract) and it should be aware of scoping. But now that you're mentioning dependencies, I think it would be more natural to have child A depend on parent B in state DESCRIBED, rather than on a generic registry. With the former, child A cannot be installed until the abstract parent B is DESCRIBED, whereas if we had a single abstract-registry, that dependency would always be satisfied, even if the definition for the abstract bean hasn't been deployed yet, so I can't see what would be the use. Also, parent-child relationships can exist even between concrete beans, so if the registry is holding just abstract beans it would be of little use.



                  • 6. Re: Abstract Beans JBKERNEL-10
                    alesj

                    Actually thinking more about this and reading your response,
                    it makes sense the way you did it - by having abstract beans in Described.
                    As this already address some of the issues;
                    e.g. scoping (PreInstall), CL existence (Described)

                    But we should then do the following things:
                    * have new ControllerContextActions for abstract beans, which would only have PreInstall and Describe mappings
                    * requiredState is Descibed
                    * is not autowire candidate

                    Or any other optimization that could have impact on the issues mentioned here:
                    - http://www.jboss.org/index.html?module=bb&op=viewtopic&t=158498
                    altough I doubt having a few abstract beans would do much harm.

                    I'll check your patch asap, or if you're not addressing some of the issues
                    I mentioned above, I guess you can do a new one, and I'll have a look then. :-)

                    • 7. Re: Abstract Beans JBKERNEL-10
                      marius.bogoevici

                      Cool!

                      Let's wait for a couple of days until I produce a new patch. I'd rather do some code cleanup first.

                      • 8. Re: Abstract Beans JBKERNEL-10
                        kabirkhan

                         

                        "marius.bogoevici" wrote:

                        @XmlElement("property")
                        public Set<PropertyMetaData> getPropertyMetaData() {
                         return new HashSet<PropertyMetaData>(this.propertyMetaData.values();
                        }
                        


                        I have taken a look at this, and one problem is that some places in the code rely on the properties set being mutable, so returning a new set breaks existing things.

                        We need to either:
                        - document the immutability in the SPI and change the known uses (probably not advisable)
                        - or I need to somehow use both a map and a set wrapped together somehow so that they are notified of each others changes

                        I'll take a look at the second option

                        • 9. Re: Abstract Beans JBKERNEL-10
                          kabirkhan

                          The problem in doing this refactoring is that BeanMetaDataBuilder (which can be changed) and JBoss XB (which cannot be changed) do something like:

                          BeanMetaData bmd = ...;
                          HashSet<PropertyMetaData> properties = bmd.getProperties();
                          if (properties == null)
                          {
                           properties = new HashSet<PropertyMetaData>();
                           bmd.setProperties(properties);
                          }
                          properties.add(propertyMetaData1);
                          properties.add(propertyMetaData2);
                          


                          So if we copy the set into a map when calling BMD.setProperties() the properties.add() calls do not get reflected in the underlying data in the map. I have looked into writing a Map enabled wrapper around the passed in set, but since we will need to work with the set underneath there does not seem to be that much point.



                          • 10. Re: Abstract Beans JBKERNEL-10
                            alesj

                            I don't think the map really solves all the problems.
                            Since afair we have some beans that have properties with the same name but different types.
                            Dunno how much against JavaBeans is this, but I think we should still be able to support is - which is somehow harder to do with map.

                            What about just some useful merging methods on (Abstract)BeanMetaData?

                            • 11. Re: Abstract Beans JBKERNEL-10
                              kabirkhan

                               

                              "alesj" wrote:
                              I don't think the map really solves all the problems.
                              Since afair we have some beans that have properties with the same name but different types.


                              They key could be name+type

                              Anyway, I don't see a way to make it a map without changing the spi