On 6/23/07, Martin van den Bemt <[EMAIL PROTECTED]> wrote:
Noticed the call of toString() on a String during the huntdown of what in
beanutils broke the
betwixt tests. (in the TestObjectStringConverters)
The commit was a bit premature probably, although this is most (read most, so
not all) of the time
faster that calling toString() on a String. Will revert it (after some sleep)
if that is what you
are asking (code is more readable without the addition, agreed).
Another questions (probably related to BEANUTILS-258) : The failing gump of
betwixt is related to
the changes you made to ConvertUtilsBean.convert(Object). In beanutils 1.7 a
default lookup is done
for the type String.class and in the new code this is just the case when no
converter can be found
for the sourcetype, which makes the new beanutils code not a drop in
replacement of the old one and
not backward compatible. I will see if I can run the beanutils 1.7 testcases
against trunk tomorrow
(they should pass, or am I being simplistic here?)
Was this breakage intended and what are your thoughts on how to handle this ?
I only see 6 failures in 2 test cases (and looking at the gump output
seems the same) and really its only 3 because one test case extends
the other and its the same failures.
TestObjectStringConverters (3 failures)
- testConvertUtilsConverter
- testDefaultOSConverter
- testDefaultOSConverterDates
Testi18nObjectStringConversion (3 failures)
(same failures as above since it inherits from TestObjectStringConverters)
The cause is the same in all cases - these tests are effectively
testing that ConvertUtils is delegating to the Converter registered
for the String.class. This is what I (intentionally) changed.
http://issues.apache.org/jira/browse/BEANUTILS-258
The contract for BeanUtils's Converter includes the "type" you want to
convert to. Unfortunately the Converter implementations mostly ignored
this and for conversion to String ConvertUtils delegated to the
registered String Converter. Makes more sense to me if the Converter
registered for the "Type" handled conversion from the type to String.
So for example - the date Converter implementations can now be
configured with a pattern (& Locale) and use that pattern to convert
from String to Date and from Date to String. The same is true for the
improved number converters (which can now also be configured with
patterns and a Locale).
In light of the betwixt issue I have made one change to the
ConvertUtilsBean's new convert() method - if the registered Converter
doesn't convert the value to a String it tries the registered String
Conveter first - before then defaulting to the object's toString()
method. This only partially resolves the compatability issue though
and doesn't stop Betwixt tests failing.
http://svn.apache.org/viewvc?view=rev&revision=551047
On the compatibility issue - I believe the Converter implementations
provided by BeanUtils are backwards compatible and its only where
people have registered their own implementations that there may be
issues. With my change today - if their Converter implementations
don't handle conversion to String then it will continue to delegate to
the registered String Converter - if their Converter doesn't cope well
with that then they have an issue. The solution is fairly straight
forward though since I have added a new AbstractConverter
implementation that provides a structure to easily add String
conversion capability. The question is though whether this is all
enough (IMO yes).
If not then the one option would for the existing ConvertUtilsBean's
convert() methods to revert to their previour behaviour (and be
deprecated?) and not delegate to the new convert() method impl. This
would resolve the issue for Betwixt (which uses
ConvertUtils/ConvertUtilsBean) - but not for BeanUtils/BeanUtilsBean -
whose methods (setProperty/copyProperty) should IMO use the new
features.
Another option would be to reinstate backward compatible behaviour
with a configuration option (not sure what default would be) to switch
on/off the new features. Personally I'm not keen on doing that work
but if that is the preferred route I would hope the default behaviour
would be the new, rather than old behaviour.
Opinions?
Niall
P.S. If the consensus is to leave BeanUtils as it is - the Betwixt
tests can be made to work with both the current BeanUtils trunk and
the previous (1.7.0) release with only minor modifications, which I
can do.
Mvgr,
Martin
Niall Pemberton wrote:
> Is there a reason for this change? AFAIK calling toString() on a
> String object just returns a reference to itself - so this just seems
> to add clutter in my mind. Also there was discussion on this (i.e.
> calling toString() on a String) for this very bit of code in the
> following issue ticket - would have been nice to comment before
> arbitarily making the change
>
> http://issues.apache.org/jira/browse/BEANUTILS-283
>
> Niall
>
> On 6/23/07, [EMAIL PROTECTED] <[EMAIL PROTECTED]> wrote:
>> Author: mvdb
>> Date: Fri Jun 22 17:58:02 2007
>> New Revision: 549986
>>
>> URL: http://svn.apache.org/viewvc?view=rev&rev=549986
>> Log:
>> Prevent calling toString on a String.
>>
>> Modified:
>>
>>
jakarta/commons/proper/beanutils/trunk/src/java/org/apache/commons/beanutils/ConvertUtilsBean.java
>>
>>
>> Modified:
>>
jakarta/commons/proper/beanutils/trunk/src/java/org/apache/commons/beanutils/ConvertUtilsBean.java
>>
>> URL:
>>
http://svn.apache.org/viewvc/jakarta/commons/proper/beanutils/trunk/src/java/org/apache/commons/beanutils/ConvertUtilsBean.java?view=diff&rev=549986&r1=549985&r2=549986
>>
>>
==============================================================================
>>
>> ---
>>
jakarta/commons/proper/beanutils/trunk/src/java/org/apache/commons/beanutils/ConvertUtilsBean.java
>> (original)
>> +++
>>
jakarta/commons/proper/beanutils/trunk/src/java/org/apache/commons/beanutils/ConvertUtilsBean.java
>> Fri Jun 22 17:58:02 2007
>> @@ -542,7 +542,8 @@
>> }
>> converted = converter.convert(targetType, value);
>> }
>> - if (targetType == String.class && converted != null) {
>> + if (targetType == String.class && converted != null &&
>> + !(converted instanceof String)) {
>> converted = converted.toString();
>> }
>> return converted;
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
