commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bradley Schaefer (JIRA)" <j...@apache.org>
Subject [jira] Commented: (BEANUTILS-258) Improve Converter Implementations
Date Wed, 27 Dec 2006 11:39:22 GMT
    [ 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


Mime
View raw message