On 6/29/07, Martin van den Bemt <[EMAIL PROTECTED]> wrote:
Hi Niall,
I completely missed the mail, so sorry for responding so late and thanx for
your extensive reply !
After some thought about this, I came to a couple of insights :)
- You know the consequences of the change you made to beanutils a lot better
than I do, so I leave
that responsibility to you. Although I would like to see some form of
documentation about this
specific case (cannot imagine that betwixt is the only one that will run into
this problem).
Its a good thing the betwixt raised this issue - it was something I
had only just started to give thought to - focusing on improvements
rather than existing behaviour..
The
problem is that the testcode in betwixt looks buggy or at least weird to some
extend, so maybe it's
a hard case to explain.
- The consequence I see now is that people if I adhere to the new way convert
works, there will be
problems with the currently released versions of beanutils, so :
Betwixt test cases are bizarre - but they do highlight a potential
issue. Unfortunately BeanUtils development is slow and I'd rather
ponder this some more than take a definite position at this point - a
"compatbility" option in BeanUtils would be good, but I have to find
the cycles and motivation to do it :(
I decided to leave it to the user to use the improved beanutils functionality
and solved the build
problem by copying the beanutils 1.7 code over to betwixt (see
http://svn.apache.org/viewvc?view=rev&revision=552049). This gives me the
certainty that people
don't have any problems "deserliazing" their objects when beanutils trunk is on
their classpath :)
The problem/issue with this approach is that you prevent Betwixt users
taking advantage of the improved conversion facilities that will be
offered by BeanUtils in the next release. Betwixt build issues can be
fixed (i have a tested patch) to work for both the new and the old -
leaving the compatibility issue open for now (but to be resolved
before a BeanUtils release).
Don't know if your changes still serve a purpose, if not, you should be able to
revert it without
breanking betwixt.
The changes do improve compatibility - so I'm going to leave them in.
Niall
Mvgr,
Martin
Niall Pemberton wrote:
> 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]
>
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
