You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@commons.apache.org by "Bradley Schaefer (JIRA)" <ji...@apache.org> on 2006/12/27 12:39:22 UTC
[jira] Commented: (BEANUTILS-258) Improve Converter Implementations
[ 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 {@link 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
* {@link #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 {@link org.apache.commons.beanutils.Converter} with no default value.
*/
public MyAbstractConverter2() {
this(null);
}
/**
* Creates a {@link org.apache.commons.beanutils.Converter} with a default value.
*
* @param defaultValue The value {@link #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
* {@link #canConvert(Class, Class) canConvert(value.getClass(), type)}
* returns <code>false</code> or {@link #getDelegate()}.{@link 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 {@link 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 {@link 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 {@link 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 {@link 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: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org