scala collections support?
allComputableThings opened this issue · 16 comments
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
- Stuart
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.
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
#233 (comment).
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:
- I'm not sure the coersion framework currently supports generics like List. If not, probably can add support for that via Guava's TypeToken like here https://github.com/facebook/swift/blob/master/swift-codec/src/main/java/com/facebook/swift/codec/metadata/ThriftCatalog.java#L209
- We don't want to depend on scala's standard library when scala is not used, so the coersions have to be registered only when running scala. This would probably require either a different entry point for the swift2thrfit generator or an extension/plugin.
A different (worse) approach would be to directly modify ThriftCatalog.thriftGetType(). This sidesteps the first issue above, but not the second.
Alex
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)
Yes, see addCodec() method in ThriftCodecManager
@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:
- instantiate my a MyList during deserialization and populate its contents
- serialize the content of MyList
Is there a ThriftType that handles java.util.List serialization? If so, can you point me at it? Thanks.
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.
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.
Got it. Thanks, now I've locally got:
- Scala collections
- String[](looks like support was missing in master)
- T[]
working through a type plugin.
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<>();
I guess -- I missed my main question. Does this look like a decent approach to adding support for self-referential types?
- Register a ThriftType in the catalog with a null ThriftStructMetadata, calculate it and update the ThriftType in the catalog (which means the ThriftType is no longer immutable).
Is there a more sensible approach to this?
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).=
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:
- self reference via a list field
- self reference via a struct field
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.
Moved to #249
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.