[Springframework-developer] Nested bean bug in BugWrapperImpl

[Trevor C. <pr...@se...>]

I've finished our project conversion to the new version of Spring, and am
now doing major testing.  During this, I have found a bug in
org.springframework.beans.BeanWrapperImpl relating to how nested beans are
handled (due to caching inside the BeanWrapperImpl).

I have created special classes to handle phone numbers following our local
(Canada) standards, which are (###)###-#### so the object contains 3
properties (area code, prefix, suffix).  I also overrode equals to perform a
semantic comparison.  If all 3 fields match, the phone numbers are equal.
In our form for editing contacts, we have 2 phone numbers, one for voice, 1
for fax.  So the 2 suffix pieces are referenced as "contact.voice.suffix"
and "contact.fax.suffix".  We require a voice number, but fax numbers are
optional.

The problem arises when a new (unpopulated) form is submitted.  The values
for all the fields in both phone numbers are initialized to "".  Because
HttpServletRequest parameters are not guarenteed to be in any order, the fax
field is processed first.  The bean wrapper does not find an existing nested
bean wrapper, so creates one around the fax number.  The voice number is
then processed.  It looks in the nested bean wrapper cache for a phone
number which equals itself.  Since they are semantically equal, it retrieves
the bean wrapper for the fax number and populates it.  This means that I
have a voice number of (123)456-.... and a fax number of (...)...-7890 .
This fails validation since they are NOT proper phone numbers.

It took a while to determine exactly why this problem occured and I've
written a test which exposes it in
BeanWrapperTestSuite.testNestedBeanSetterMethod .  I'm not sure what the
best way to fix this is, so maybe someone who's familiar with the bean
package can take a look (Rod/Juergen).  Commenting out line 442 of the
BeanWrapperImpl fixes it, but it then breaks
CustomEditorTestSuite.testCustomEditorForSingleNestedProperty .

I've submitted the new test to expose the problem, but I've commented it out
since it will break the build.  Whoever is familiar with this can take a
look, but we should uncomment this test long term since it should normally
work.

Trevor D. Cook