cognitect / transit-java

transit-format implementation for Java
Apache License 2.0
62 stars 21 forks source link

Custom ReadHandler for tag "map" #18

Closed chris-zen closed 8 years ago

chris-zen commented 8 years ago

I am creating custom Write and Read handlers for Scala on top of transit-java.

I was able to implement a WriteHandler for an Scala Map structure that serializes into a "map" tag by wrapping the Scala Map into a Java Map as:

    import java.{util => ju}
    import collection.JavaConverters._
    object MapWriteHandler extends AbstractWriteHandler[collection.Map[Any, Any], ju.Map[Any, Any]] {
      def tag(m: collection.Map[Any, Any]): String = "map"
      def rep(m: collection.Map[Any, Any]): ju.Map[Any, Any] = m.asJava
    }

I wrote a ReadHandler too, but when I try to instantiate a new reader using my custom handler:

val customReadHandlers = Map[String, ReadHandler[_, _]](
    "map" -> ReadHandlers.MapReadHandler
  ).asJava

// ...

TransitFactory.reader(format, stream, customReadHandlers).read[Any]()

I get the following error:

java.lang.IllegalArgumentException: Cannot override decoding for transit ground type, tag map
java.lang.RuntimeException: java.lang.IllegalArgumentException: Cannot override decoding for transit ground type, tag map
    at com.cognitect.transit.TransitFactory.reader(TransitFactory.java:136)
    at com.cognitect.transit.TransitFactory.reader(TransitFactory.java:95)

What is the right way to write and read a Map that doesn't necessarily uses a java.util.Map structure then ? I need it to interoperate with other languages so I actually need to use the "map" tag but using an Scala Map to read, not the java.util.Map.

Thanks, Christian

dchelimsky commented 8 years ago

Check out how transit-clj handles this: https://github.com/cognitect/transit-clj/blob/master/src/cognitect/transit.clj#L264-L291. It has it's own reader factory fn that wraps the one in transit-java and makes its handlers part of the reader to begin with thus avoiding the problem that you're experiencing. That help?

chris-zen commented 8 years ago

Hi @dchelimsky, I am not sure I understood the Clojure code. First of all, I don't see default-read-handlers defining any handler for a transit ground type (otherwise TransitFactory/reader would throw the exception I am reporting), then I understand that the trick is on the lines 288-290.

The problem is that the signature for the ReaderSPI.setBuilders forces me to pass a MapReader<?, Map<Object, Object>, Object, Object>, where the second parameter Map<Object, Object> refers to a java.util.Map, and it is the type returned by the map builder and therefore the reader. But I am precisely trying to alter this, I don't want it to return a java.util.Map but other type (a scala.collection.immutable.Map).

I could solve this limitation if I was able to override the "map" tag with my own ReadHandler as shown in my initial comment, but these lines in transit-java prevent me from doing that.

Why does transit-java impose the use of a java.util.Map for representing the "map" tag in the language ? What about other JVM languages such as Scala ? Or else, what am I missing ?

dchelimsky commented 8 years ago

Ah, you're right. Sorry. I only glanced quickly and had assumed that transit-clj was providing some overrides of ground types. You're correct that you can't override them through the reader API. You'd have to dig deeper down to override the map handler and I'm not sure what the right approach would be. I'll follow up if I think of something but I'll try to get other eyes on this as well.

FYI - all of the transit implementations prevent overriding ground type readers, but the intent there is to prevent library authors, writing libs in the same language as the transit impl, from pulling the rug out from under users of their libraries. Different problem than the one you're trying to solve, but at least now you know why that's there.

dchelimsky commented 8 years ago

Hi again @chris-zen. Expanding on my prev comment, the issue at the heart of this is composability, and it applies to maps and lists in java in the same way that it applies to the scalar ground types. If transit-java allowed lib authors to change the ground types, consumers of those libs would not be able to count on the types they're expecting. While the intent of the ReaderSPI is to support JVM languages beyond java, it has to maintain fidelity to java types. JRuby and Clojure maps both implement java.util.Map, so this works out for both of those languages, but the fact that scala.collection.immutable.Map doesn't implement java.util.Map makes it unsupportable via this mechanism.

What's really needed here is a transit-scala lib that deals directly in scala-specific types. I believe that you could get there by forking transit-java and making just a few changes. The MapReader would still need to be declared in terms of a mutable map because the map is built up one element at a time as it is parsed, but it could then return a scala.collection.immutable.Map from the complete method: https://github.com/cognitect/transit-java/blob/master/src/main/java/com/cognitect/transit/impl/ListBuilderImpl.java#L29.

That help?

chris-zen commented 8 years ago

Hi @dchelimsky, I really appreciate your help with this, but unfortunately working on transit-scala is out of scope for my project, that summed to the fact that binary data is represented as Base64 strings (see here) made us decide to use MessagePack instead, which has very good libraries for Clojure, Scala, Python and JavaScript, and fully covers our data needs. Thank you very much.

dchelimsky commented 8 years ago

Understood, and glad you found a solution that works for you.