search

facebookarchive / swift

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

scala collections support? #233

Closed allComputableThings closed 6 years ago

allComputableThings commented 9 years ago

Not sure this is the right place- can't find a swift mailing list.

Does swift directly support both java and scala collections?

I'm hoping to be able to take a set of abstract scala traits (dealing with existing data-types that are already easily JSON-serialiable), and expose them as RPC APIs to clients in various languages. I've read that thrift-swift will let me generate .idl files from existing interfaces. Does this include Scala, or just Java. On the server I'm hoping I can straightforwardly directly export a Thrift server for these with Finagle. 

Am I on the right track? (Can I expect to export thrift .idls from Scala code, and can I easily implement my own server to match these IDL-generated clients?)

I've tried running:

MY_CLASSES=$HOME/ExampleServiceMvn/target/ExampleService-1.0-SNAPSHOT.jar  # just an example
SCALA_CLASSES=$HOME/.m2/repository/org/scala-lang/scala-library/2.10.2/scala-library-2.10.2.jar
java -cp target/swift2thrift-generator-cli-0.15.0-SNAPSHOT-standalone.jar:$MY_CLASSES:$SCALA_CLASSES com.facebook.swift.generator.swift2thrift.Main -package com.example ExampleService -map ExampleService path/to/base.thrift -namespace py mycomany.thrift -namespace java mycompany.thrift -namespace cpp mycompany

This works fine for the scala classes that only with Java collections, but if my service interface deals with scala collections, I get:

Exception in thread "main" java.lang.IllegalArgumentException: Type can not be coerced to a Thrift type: scala.collection.immutable.List<java.lang.String>
    at com.facebook.swift.codec.metadata.ThriftCatalog.getThriftType(ThriftCatalog.java:291)
    at com.facebook.swift.service.metadata.ThriftMethodMetadata.<init>(ThriftMethodMetadata.java:90)
    at com.facebook.swift.service.metadata.ThriftServiceMetadata.<init>(ThriftServiceMetadata.java:73)
    at com.facebook.swift.generator.swift2thrift.Swift2ThriftGenerator.convertToThrift(Swift2ThriftGenerator.java:432)
    at com.facebook.swift.generator.swift2thrift.Swift2ThriftGenerator.<init>(Swift2ThriftGenerator.java:112)

I'm happy to add scala collection support to swift2thrift if that makes sense.

Also, the above is using using the https://github.com/facebook/swift/ checkout

What the relationship between this and finagle/finagle-swift in https://github.com/twitter/finagle?

And how can I compile the git checkout? Tried the following:

# Succeeds, but no jars
cd finagle/doc/src/sphinx/code/quickstart  && ./sbt compile
cd finagle/doc/src/sphinx/code/quickstart  && ./sbt package

# [error] (finagle-ostrich4/*:update) sbt.ResolveException: unresolved dependency: com.twitter#ostrich_2.10;9.6.1: not found
cd finagle; ./sbt package

# [error] Not a valid project ID: finagle-swift (similar: finagle-thrift, finagle-test, finagle-stats)
cd finagle; ./sbt 'project finagle-swift' compile

#[ERROR]     Non-resolvable parent POM: Could not find artifact com.twitter:scala-parent-2104_2.10:pom:0.1.14 in central (http://repo.maven.apache.org/maven2) and 'parent.relativePath' points at wrong local POM @ line 8, column 11 -> [Help 2]
cd finagle/finagle-swift ; mvn compile

# Produces ./finagle-thrift/target/scala-2.10/finagle-thrift_2.10-6.22.0.jar
# but doesn't include thrift-swift
cd finagle; ./sbt 'project finagle-thrift' package

-- thanks

alandau commented 9 years ago

As you saw yourself, the swift2thrift generator doesn't support scala collections, but I'd be glad to merge a patch that adds support. As for finagle-swift, the idea (as far as I know) was to use swift instead of the regular code generator, but I'm not aware of the current status. You can probably ask on that project page if you have compilation or other issues.

allComputableThings commented 9 years ago

Hi Alan -- I'd like to work on this, but could use some pointers through the codebase.

Can I email you a list of questions here?

On Mon, Dec 1, 2014 at 12:49 PM, alandau notifications@github.com wrote:

As you saw yourself, the swift2thrift generator doesn't support scala collections, but I'd be glad to merge a patch that adds support. As for finagle-swift, the idea (as far as I know) was to use swift instead of the regular code generator, but I'm not aware of the current status. You can probably ask on that project page if you have compilation or other issues.

— Reply to this email directly or view it on GitHub https://github.com/facebook/swift/issues/233#issuecomment-65123459.

alandau commented 9 years ago

Sure. The best way would probably be adding the scala->java and java->scala coersions in a class similar to https://github.com/facebook/swift/blob/master/swift-codec/src/main/java/com/facebook/swift/codec/internal/coercion/DefaultJavaCoercions.java

2 issues here:

A different (worse) approach would be to directly modify ThriftCatalog.thriftGetType(). This sidesteps the first issue above, but not the second.

Alex

allComputableThings commented 9 years ago

Thanks am working on this. Related: Suppose that I have an existing Java bean that I cannot annotate - is there a way to register a codec for this type. (i.e. specify the serialization separately from the object)

andrewcox commented 9 years ago

Yes, see addCodec() method in ThriftCodecManager

allComputableThings commented 9 years ago

@alandau Here's my plugin plan. Register instances of:

public interface JavaToThriftTypeResolver
{
    public boolean isSupportedStructFieldType(Type javaType);
    public ThriftType getThriftType(Type javaType);
}

to ThriftCodec and have ThriftCodec.isSupportedStructFieldType and JavaToThriftTypeResolver.getThriftType delegate to the registered objects.

Question: Suppose I want to support MyList. I can have my custom JavaToThriftTypeResolver.getThriftType return a ThriftType. However, how does this type get to:

Is there a ThriftType that handles java.util.List serialization? If so, can you point me at it? Thanks.

allComputableThings commented 9 years ago

I'm still a little confused about how existing java collections are handled here. For example, I can supply and return ArrayList as a parameter and return type for methods. I see ArrayList registered in the type cache... but how does its constructor get called? I can similarly create a MyArrayList extends ArrayList -- its constructor is also called. However, I can't track down from where its called in the debugger.

alandau commented 9 years ago

If I create a MyArrayList extends ArrayList and have a method argument of that type, I get an "argument type mismatch" exception when trying to call this method from a client... In general Lists are handled in ListThriftCodec.

allComputableThings commented 9 years ago

Got it. Thanks, now I've locally got:

working through a type plugin.

allComputableThings commented 9 years ago

I'm running into problems with recursive definitions. I believe thrift allows

struct A {
  1: list<A> children;
}

but not:

struct A {
  1: A a;
}

(for the latter, in C++ this leads to: sizeof(A)==infinity).

I think the former case is incorrectly disallowed in ThriftCatalog:

     private final ThreadLocal<Deque<Type>> stack = new ThreadLocal<Deque<Type>>()
....
    private ThriftStructMetadata extractThriftStructMetadata(Type structType)
    {
        Deque<Type> stack = this.stack.get();
        if (stack.contains(structType)) {
            ...
            throw new IllegalArgumentException("Circular references are not allowed: " + path);
        }
        stack.push(structType);
        try {
            ThriftStructMetadataBuilder builder = new ThriftStructMetadataBuilder(this, structType);
            ThriftStructMetadata structMetadata = builder.build();
            return structMetadata;
        }
        finally {
            Type top = stack.pop();
            ...
        }
    }

I'm planning to add support for this case to my fork since we have many instances of this pattern (class A contains a collection of A) in our code and want to export APIs to them with Thrift.

To do this, some kind of forward declaration is required:

getThriftType(  A )     // Must 'declare' a ThriftType in the catalog (perhaps with an null StructMetadata)
+- getThriftType(  list<A> )
    +- getThriftType(  A )  // should point original (incomplete) ThriftType, or fail if voilating Thrift requirements.

The incomplete ThriftType for struct A would have its ThriftStructMetadata set when the stack unwinds.

Related question: what's the difference between these in ThriftCatalog:

     private final ConcurrentMap<Class<?>, ThriftType> manualTypes = new ConcurrentHashMap<>();
     private final ConcurrentMap<Type, ThriftType> typeCache = new ConcurrentHashMap<>();
allComputableThings commented 9 years ago

I guess -- I missed my main question. Does this look like a decent approach to adding support for self-referential types?

Is there a more sensible approach to this?

electrum commented 9 years ago

Can you check how the Apache Thrift code generator handles this case? We don't want to add support if only Swift could use it (at least by default).=

allComputableThings commented 9 years ago

At least for 0.9.2, thrift accepts both cases. However, the C++ code it generates for the struct directly containing itself will fail to compile.

It reasonable that thrift accept it since class value fields (as opposed to class reference fields) are a feature of very few languages. Perhaps the best thing to do here is for swift to error out self on references depending on whether a "allowStructSelfRerence" option is set on ThriftCatalog.

In the longer term, NB thrift has recent support for self-referential C++ structs with the & operator https://github.com/apache/thrift/blob/master/test/Recursive.thrift#L26

1: A & selfRef;

The swift2thrift generator could probably be patched for this. I'm not sure what the consequences are for adding this everywhere.

Either way, it seems like forward declarations are needed to support either:

dain commented 9 years ago

Can we move this discussion to a new issue?

I think it is a bad idea to make ThriftType mutable. Instead, I would do something like this (pseudo code)

public ThriftMetadata(Class c) {
   this(c, new Map<Class, ThriftMetadata> inprogress);
}

private ThriftMetadata(Class c, Map<Class, ThriftMetadata> inprogress) {
   inprogress.put(c, this);
   for each field:
      ThriftMetadata metadata = inprogress.get(field.type)
      if (metadata == null) {
            metadata = new ThriftMetadata(field.type, inprogress);
      }
      // use metadata, but be careful to not access uninitialized fields
}

This is super dangerous, but works because we build the transitive closure in a single shot. The is really only borderline-sane because this is a single class with a private constructor that understands the exact publishing semantics of every field.

I think the bigger issue is how you serialize and deserialize the value.

allComputableThings commented 9 years ago

Moved to https://github.com/facebook/swift/issues/249

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.