facebookarchive / swift

An annotation-based Java library for creating Thrift serializable types and services.
Apache License 2.0
900 stars 297 forks source link

Support for Arrays of Strings and objects. #240

Closed allComputableThings closed 6 years ago

allComputableThings commented 9 years ago
class ThriftCatalog
{

    public boolean isSupportedStructFieldType(Type javaType)
    {
...
        if (rawType.isArray()) {
            Class<?> elementType = rawType.getComponentType();
            return isSupportedArrayComponentType(elementType);
        }
...

    public boolean isSupportedArrayComponentType(Class<?> componentType)
    {
        return boolean.class == componentType ||
                byte.class == componentType ||
                short.class == componentType ||
                int.class == componentType ||
                long.class == componentType ||
                double.class == componentType;
    }
...
}

Leaves us without support for Array[String] or Array[T extends Object], ...

allComputableThings commented 9 years ago

Some discussion...

    bool isSupportedStructFieldType(Type javaType ) ==  { getThriftType(type)!=null }
ThriftCatalog.getThriftTypeUncached(Type javaType)
{
...     if (rawType.isArray()) {
            ... does not check for isSupportedStructFieldType
            ... Succeeds, and serializes as ArrayList<T> for all T            
        }
}

which results in a runtime cast exception because T[].class != ArrayList<T>.class, for all T.

So... I think it would be correct to add:

ThriftCatalog.getThriftTypeUncached(Type javaType) {
        ...
        if (rawType.isArray()) {
            Class<?> elementType = rawType.getComponentType();
            if (elementType == byte.class) {
                // byte[] is encoded as BINARY and requires a coercion
                return coercions.get(javaType).getThriftType();
            }
            if ( isSupportedArrayComponentType(elementType) )   // ***** NEW
            {
                // TODO: This takes/produces and ArrayList<E> in the interface, not E[]
                // Coercion is required.
                return array(getThriftType(elementType));
            }
           ...
      } ...
dain commented 9 years ago

When I added support for arrays a couple months back, I didn't add support for arrays of Objects, because I personally find them to be fairly useless, and it would have been annoying (I don't remember the specifics, other than it would have been annoying). IMO, the only reason to use an array in Java is to build a higher level data structure, or for performance. The performance gain only happens when the data is physically next together in memory, which means primitive objects. When it comes to objects, you should just use one of the collection classes, which can be immutable and have the same performance.

That said, if you really, really, really want arrays of objects, send a pull request. The tricky part will be the compiler, but it isn't that tricky. An alternative implementation might be to add converters from Collection to T[], which might be doable without changing the byte code generator.

ghost commented 9 years ago

Thank you for reporting this issue and appreciate your patience. We've notified the core team for an update on this issue. We're looking for a response within the next 30 days or the issue may be closed.