[
http://issues.apache.org/jira/browse/BEANUTILS-258?page=comments#action_12460981
]
Bradley Schaefer commented on BEANUTILS-258:
--------------------------------------------
I think the current version of AbstractConverter (474175) has some rough spots.
1) I think this class should probably be immutable.
Right now there is a state variable, useDefault, which introduces some
potential threadsafety concerns since public void setDefaultValue(Object
defaultValue) flips it while the protected Object handleError(Class type,
Object value, Exception ex) method as well as the protected Object
handleMissing(Class type) method checks the value of useDefault to decide how
to behave. Those methods may be called asynchronously since they are triggered
by the method public Object convert(Class type, Object value), so there's room
for some unusual behavior based on the state. If this class were immutable,
this could be avoided, and it would take the burden off of the user from
understanding any sort of implementation detail relating to using converters in
threads.
2) I don't understand the hardcoded behavior for String.class and
StringBuffer.class in public Object convert(Class type, Object value) .. it
leads to some inconsistent behavior -- for example I added three very similar
test cases into ConvertUtilsTestCase, and they all behaved differently (albeit
in a weird edge case):
public void testNullStringConversion() throws Exception {
final Converter nullConverter = new AbstractConverter(String.class) {
public Object convertToType(Class clazz, Object obj) { return null;
}
};
ConvertUtils.register(nullConverter, String.class);
Object result = ConvertUtils.convert((String)null, String.class);
assertNull("null String conversion failed (1)", result); // passes
result = ConvertUtils.convert("testNullStringConversion", String.class);
assertNull("null String conversion failed (2)", result); // fails
result is "testNullStringConversion"
}
public void testNullStringBufferConversion() throws Exception {
final Converter nullConverter = new
AbstractConverter(StringBuffer.class) {
public Object convertToType(Class clazz, Object obj) { return null;
}
};
ConvertUtils.register(nullConverter, StringBuffer.class);
Object result = ConvertUtils.convert((String)null, StringBuffer.class);
assertNull("null StringBuffer conversion failed (1)", result); // fails
- result is a new StringBuffer()
Object result2 = ConvertUtils.convert("testNullStringBufferConversion",
StringBuffer.class);
assertNull("null StringBuffer conversion failed (2)", result2); //
passes
}
public void testNullObjectConversion() throws Exception {
final Converter nullConverter = new AbstractConverter(Object.class) {
public Object convertToType(Class clazz, Object obj) { return null;
}
};
ConvertUtils.register(nullConverter, Object.class);
Object result2 = ConvertUtils.convert("testNullObjectConversion",
Object.class);
assertNull("null Object conversion failed (2)", result2); // passes
Object result = ConvertUtils.convert((String)null, Object.class);
//fails ConversionException: No value specified for 'Object'
assertNull("null Object conversion failed (1)", result);
}
3) AbstractConverter doesn't need the concept of a defaultType. I think that
reduces the flexibility of this class, since it's conceivable that you might
want to make one Converter that works on a number of Types, but that would be
confusing with this implementation. Of course one might argue that one should
implement their own Converter in that situation -- but if it was to behave
similarly to any of the default converters using AbstractConverter as a base
class, they would have a difficult time reproducing the same quirky behavior.
My proposed solution is to have a much simpler immutable Abstract base class.
package org.apache.commons.beanutils.converters;
import org.apache.commons.beanutils.Converter;
import org.apache.commons.beanutils.ConversionException;
import org.apache.commons.logging.Log;
import org.apache.commons.logging.LogFactory;
/**
* <p>
* Basic [EMAIL PROTECTED] org.apache.commons.beanutils.Converter}
implementation that provides the structure
* for handling converting an arbitrary value to a target type, optionally
* using a default value.
* </p>
*
* <p>
* Implementations are encouraged to override the
* [EMAIL PROTECTED] #canConvert(Class, Class)} method if they are unable to
handle
* converting every type of value to a target type.
* </p>
*
* @version $Revision: $ $Date: $
* @since 1.8.0
*/
public abstract class MyAbstractConverter2 implements Converter {
/** Logging for this instance. */
protected final Log log = LogFactory.getLog(getClass());
private final Object defaultValue;
/**
* Creates a [EMAIL PROTECTED] org.apache.commons.beanutils.Converter} with
no default value.
*/
public MyAbstractConverter2() {
this(null);
}
/**
* Creates a [EMAIL PROTECTED] org.apache.commons.beanutils.Converter} with
a default value.
*
* @param defaultValue The value [EMAIL PROTECTED] #convert(Class, Object)}
returns
* when the source value is null.
*/
public MyAbstractConverter2(Object defaultValue) {
this.defaultValue = defaultValue;
}
/**
* @param type The type to convert the value to. If the type is a primitive,
* the corresponding wrapper class is automatically used for the target
* type.
* @throws org.apache.commons.beanutils.ConversionException when this
method is called when
* [EMAIL PROTECTED] #canConvert(Class, Class)
canConvert(value.getClass(), type)}
* returns <code>false</code> or [EMAIL PROTECTED]
#getDelegate()[EMAIL PROTECTED] Converter#convert(Class, Object) convert(Class,
Object)}
* throws an Exception.
* @throws IllegalArgumentException when type is null.
* @see org.apache.commons.beanutils.Converter#convert(Class, Object)
*/
public final Object convert(Class type, Object value) {
if(null == type) {
throw new IllegalArgumentException("type to convert to must be
defined");
}
if(null == value) {
return defaultValue;
}
final Class sourceType = value.getClass();
final Class targetType = wrapPrimitive(type);
if(canConvert(sourceType, targetType)) {
try {
return getDelegate().convert(targetType, value);
} catch(ConversionException e) {
throw e;
} catch(Exception e) {
throw new ConversionException("Exception converting " +
toString(sourceType)+
" to " + toString(targetType), e);
}
} else {
throw new ConversionException("This converter not able to convert
from "
+toString(sourceType)+ " to " +toString(targetType));
}
}
/**
* Returns a [EMAIL PROTECTED] Converter} which will be called only with
non-null arguments.
* The delegate is what does the non-boilerplate converting.
*/
protected abstract Converter getDelegate();
/**
* <p>
* Returns whether or not this [EMAIL PROTECTED]
org.apache.commons.beanutils.Converter} is capable of converting
* from one specified type to another. The default implementation returns
* <code>true</code> every time.
* </p>
*
* <p>
* This method can optionally be overridden to indicate that the
implementation of
* the [EMAIL PROTECTED]
org.apache.commons.beanutils.converters.AbstractConverter} only works for
specific combinations of
* sourceType to targetType conversions.
* </p>
*
* @param sourceType The type of the value before being converted.
* @param targetType The type of the value to convert to.
*
* @return <code>true</code> if this [EMAIL PROTECTED]
org.apache.commons.beanutils.Converter} can convert from sourceType to
targetType,
* <code>false</code> otherwise.
* @see #convert(Class, Object)
*/
public boolean canConvert(Class sourceType, Class targetType) {
return true;
}
/**
* Change primitive Class types to the associated wrapper class.
* @param type The class type to check.
* @return The converted type.
*/
public static final Class wrapPrimitive(Class type) {
if (type == null || !type.isPrimitive()) {
return type;
}
if (type == Integer.TYPE) {
return Integer.class;
} else if (type == Double.TYPE) {
return Double.class;
} else if (type == Long.TYPE) {
return Long.class;
} else if (type == Boolean.TYPE) {
return Boolean.class;
} else if (type == Float.TYPE) {
return Float.class;
} else if (type == Short.TYPE) {
return Short.class;
} else if (type == Byte.TYPE) {
return Byte.class;
} else if (type == Character.TYPE) {
return Character.class;
} else {
return type;
}
}
/**
* Provide a String representation of a <code>java.lang.Class</code>.
*
* @param type The <code>java.lang.Class</code>.
* @return The String representation.
*/
public static final String toString(Class type) {
if (type == null) {
return "null";
} else if (type.isArray()) {
Class elementType = type.getComponentType();
int count = 1;
while (elementType.isArray()) {
elementType = elementType .getComponentType();
count++;
}
StringBuffer typeName = new StringBuffer(elementType.getName());
for (int i = 0; i < count; i++) {
typeName.append("[]");
}
return typeName.toString();
} else {
return type.getName();
}
}
}
This implementation solves all three issues I mention above. It may be that
using an abstract getter (getDelegate()) like that is unusual, but the
alternatives of making the class a decorator or creating a different abstract
method with a signature identical to convert(Class, Object) seemed equally
weird to me.
Sorry for the monster post. I hope it's coherent..
> Improve Converter Implementations
> ---------------------------------
>
> Key: BEANUTILS-258
> URL: http://issues.apache.org/jira/browse/BEANUTILS-258
> Project: Commons BeanUtils
> Issue Type: Improvement
> Components: ConvertUtils & Converters
> Affects Versions: 1.7.0
> Reporter: Niall Pemberton
> Assigned To: Niall Pemberton
> Priority: Minor
> Fix For: 1.8.0
>
>
> The "Converter" contract has the following signature....
> public Object convert(Class, Object)
> ...which incudes the type (Class) that the value should be converted to.
> However all the Converter implementations in BeanUtils ignore this parameter
> and always convert to a specific type - for example IntegerConverter only
> ever converts to Integer values.
> One of the weaknesses in BeanUtils is that conversion of an Object to a
> String is almost always done using the toString() method which, depending on
> the Class, can produce unhelpful values. IMO all Converter implementations
> should, at a minimum, support also support conversion from the type they
> handle to a String.
> Theres two stages to this process:
> 1) Enhance Converter implementations to handle conversion to Strings.
> 2) Modify BeanUtils/PropertyUtils to delegate String conversion to the
> Converters.
> In order to facilitate this, I'm adding a new AbstractConverter class which
> removes the need for duplciated "boiler plate" code. As well as providing a
> structure for conversion it also handles missing and invalid values in a
> uniform way.
> I also have new NumberConverter and DateTimeConverter implementations which
> provide improved String conversion facilities:
> 1) Number Conversion
> The existing number Converter implementations use String handling functions
> provided by the Number implementation. This has two main drawbacks - they
> don't handle String values containing thousand separators and they are fixed
> using a period (".") as the decimal separator making them only usable in
> Locales where that is the case. I'm adding a number Converter where a pattern
> can be specified for the format and/or a Locale specified to use different
> decimal symbols. This caters for the alternative Locale scenario and isn't
> intended to provide support for applications operating in a multiple Locale
> environment.
> 2) Date/Time Conversion
> Currently there are only implementations for the java.sql.Date/Time/Timestamp
> types - java.util.Date and java.util.Calendar are not handled. I have a
> date/time Converter implementation that caters for all of these types. As
> people have indicated elsewhere converting from Strings to Date/Calendar
> poses problems. This implementation can be configured either to use the
> default format for a specified Locale or can be configured with a set of date
> "patterns". This may not fully satisfy String <--> Date conversion
> requirements, but will at least provide something and importantly will
> provide Date conversion factilities where String is not involved (e.g. Date
> <--> Calendar).
> 3) Array Conversion
> I also have a generic array Converter implementation. It can convert to/from
> Arrays of any type. It does this by delegating the element conversion to a
> Converter of the appropriate type. As well as that it also caters for
> Collection --> Array conversion and provides addtiona configuraton options
> for parsing delimited String.
> I'm going to start committing the changes to the Converters initially. If
> no-one objects to those, then I'll start looking at improving how
> BeanUtils/PropertyUtils uses/delegates to Converters.
--
This message is automatically generated by JIRA.
-
If you think it was sent incorrectly contact one of the administrators:
http://issues.apache.org/jira/secure/Administrators.jspa
-
For more information on JIRA, see: http://www.atlassian.com/software/jira
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
