Introduce strategy for BeanInfo creation [SPR-9677]

[spring-projects-issues]

Arjen Poutsma opened SPR-9677 and commented

Currently, the CachedIntrospectionResults is hard-wired to create ExtendedBeanInfo instances. For obvious reasons, these bean infos support the JavaBeans property contract only.

To support different bean info contracts within Spring, it makes sense to have some sort of strategy instead.

---

Issue Links:

• Use ExtendedBeanInfo on an as-needed basis only [SPR-9723] #14357 Use ExtendedBeanInfo on an as-needed basis only ("is depended on by")
• Overhaul non-void JavaBean write method support [SPR-10029] #14663 Overhaul non-void JavaBean write method support

Referenced from: commits ca017a4, 4a8be69, a316163, 0ee1256, 29613f1, 5bcf68e

[spring-projects-issues]

Chris Beams commented

Thanks, Arjen. This is now merged. See #14357, in which I plan to refactor ExtendedBeanInfo to conform to this new contract.

One limitation of this approach is that once a BeanInfoFactory is discovered, it short circuits the search for any other BeanInfoFactory. This means that it is not possible to design a bean class that would benefit from property method detection across two BeanInfo implementations.

For example, imagine a bean with setters that do not use the "set" prefix, but "assign" instead. Support for this style of setter would be provided through a BeanInfo implementation that it picks up from a jar that provides specific BeanInfo and BeanInfoFactory types, as well as the META-INF/spring.beanInfoFactories to allow for discovery of this bean factory.

If that "assign"-style bean also has non-void returning JavaBean-style setter methods, it will not be able to pick up the necessary functionality in ExtendedBeanInfo.

I realize this is a contrived example, as it's unlikely that you would combine multiple setter styles, and this is why I have gone ahead with the merge: what I describe above is currently a theoretical problem.

Regardless, we should probably advertise a little more prominently what the semantics of BeanInfoFactory really are, i.e. that if your BeanInfoFactory's #supports method returns true, it does so to the exclusion of any and all other BeanInfoFactory types that may have been discovered. Likewise, if a BeanInfoFactory that precedes you in the discovery process returns true from its #supports method, your implementation will be excluded. The advice here should be that #supports implementations should be as conservative as possible in returning true in order to avoid unnecessarily short-circuiting a more appropriate BeanInfoFactory.

commit ca017a488056701a07673b872e8faa40667cd32e
Author: Arjen Poutsma <apoutsma@vmware.com>
Commit: Chris Beams <cbeams@vmware.com>

    Introduce strategy for BeanInfo creation
    
    Before this commit, the CachedIntrospectionResults was hard-coded to
    create ExtendedBeanInfos for bean classes. The ExtendedBeanInfo support
    the JavaBeans property contract only.
    
    This commit introduces the BeanInfoFactory, a strategy for creating
    BeanInfos. Through this strategy, it is possible to support
    beans that do not necessarily implement the JavaBeans contract (i.e.
    have a different getter or setter style).
    
    BeanInfoFactories are are instantiated by the
    CachedIntrospectionResults, which looks for
    'META-INF/spring.beanInfoFactories' files on the class path. These files
    contain one or more BeanInfoFactory class names. When a BeanInfo is to
    be created, the CachedIntrospectionResults will iterate through the
    factories, asking it to create a BeanInfo for the given bean class. If
    none of the factories support it, an ExtendedBeanInfo is created as a
    default.
    
    This commit also contains a change to Property, allowing BeanWrapperImpl
    to specify the property name at construction time (as opposed to using
    Property#resolveName(), which supports the JavaBeans contract only).
    
    Issue: SPR-9677

[spring-projects-issues]

Arjen Poutsma commented

You are correct in saying that the current model does not allow for merging multiple BeanInfos into one. This was a deliberate choice on my part, to keep the model as simple as possible. If we would allow for BeanInfo merges, the resulting code would become a lot more complex and thus error prone, while the approach I took is simpler and completely backward compatible (at least until a spring.beanInfoFactories file is found on the classpath).

One limitation of this approach is that once a BeanInfoFactory is discovered, it short circuits the search for any other BeanInfoFactory. This means that it is not possible to design a bean class that would benefit from property method detection across two BeanInfo implementations.

To clarify: all BeanInfoFactories are discovered (through spring.beanInfoFactories files), but not all are queried. The factories are sorted according to Order, and the first one who returns true for the supports method is used. Thus, having a proper ordering is important. However, I do not expect many BeanInfoFactories to be implemented, let alone used in the same project. A project will probably have two at the most.

I will improve the Javadoc for BeanInfoFactory to make this clearer.

[spring-projects-issues]

Chris Beams commented

Thanks, and I would agree per our Skype chat that ExtendedBeanInfo should be Ordered.LOWEST_PRECEDENCE. Go ahead with that too, please.

[spring-projects-issues]

Chris Beams commented

Never mind that last bit; it will only apply if and when #14357 is implemented and there is an ExtendedBeanInfoFactory to have ordering applied to it.

[spring-projects-issues]

Arjen Poutsma commented

Javadocs have been updated.

[spring-projects-issues]

Chris Beams commented

A general note to watchers of ExtendedBeanInfo-related issues: #14663 is a major refactoring of ExtendedBeanInfo and overall support for non-void returning setter methods.

If you have submitted a reproduction project with this issue, we have run it through its paces against these new changes, but we would like to ask you to do the same against your actual applications.

Please consider updating your dev or test builds to work against 3.1.4.BUILD-SNAPSHOT and/or 3.2.0.BUILD-SNAPSHOT to verify, and we would appreciate any feedback, even if it's to let us know that all is well. So that we can consolidate feedback, please add your comments to #14663, and mention the original issue(s) that you were watching.

Thanks!

Note also that testing against 3.1.4 is preferable to 3.2.0 because ExtendedBeanInfo is always in the code path in the latter, while in 3.2.0 we've optimized things such that ExtendedBeanInfo is only in play for bean classes that have one or more non-void returning setter methods.