Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Can't parse JSON map to Map[String, T] #39

Closed
beloglazov opened this issue Nov 11, 2014 · 22 comments
Closed

Can't parse JSON map to Map[String, T] #39

beloglazov opened this issue Nov 11, 2014 · 22 comments

Comments

@beloglazov
Copy link

Hi Li,

Thanks for your work on this project, it's very nice! I've faced a problem when trying to parse the following JSON (output from CouchDb):

{  
   "_id":"481e7e36693c1ae3f1871be7e506aade",
   "_rev":"2-7bc08b6426dd0a165d9c7d1f9e08aad5",
   "name":"Alice",
   "age":25,
   "_attachments":{  
      "attachment":{  
         "content_type":"application/octet-stream",
         "revpos":2,
         "digest":"md5-boWfl561SDppW4MTLQhVyA==",
         "length":5,
         "stub":true
      }
   }
}

I have the following classes:

abstract class CouchDoc {
  val _id: String
  val _rev: String
  val _attachments: Map[String, CouchAttachment]
}

case class CouchAttachment(
  content_type: String,
  revpos: Int,
  digest: String,
  length: Int,
  stub: Boolean)

case class Person(
  name: String,
  age: Int,
  _id: String,
  _rev: String,
  _attachments: Map[String, CouchAttachment] = Map()) extends CouchDoc

When I'm trying to parse the JSON with something like:

upickle.read[Person](json)

I get the following exception:

data: Obj(ArrayBuffer((attachment,Obj(ArrayBuffer((content_type,Str(application/octet-stream)), (revpos,Num(2.0)), (digest,Str(md5-boWfl561SDppW4MTLQhVyA==)), (length,Num(5.0)), (stub,True)))))) msg: Array(n)
upickle.Invalid$Data: data: Obj(ArrayBuffer((attachment,Obj(ArrayBuffer((content_type,Str(application/octet-stream)), (revpos,Num(2.0)), (digest,Str(md5-boWfl561SDppW4MTLQhVyA==)), (length,Num(5.0)), (stub,True)))))) msg: Array(n)
    at upickle.Implicits$$anonfun$validate$1.applyOrElse(Implicits.scala:16)
    at upickle.Implicits$$anonfun$validate$1.applyOrElse(Implicits.scala:16)
    at upickle.Implicits$$anonfun$MapR$1.applyOrElse(Implicits.scala:109)
    at upickle.Implicits$$anonfun$MapR$1.applyOrElse(Implicits.scala:109)
    at upickle.Reader$$anonfun$read$1.applyOrElse(Types.scala:42)
    at upickle.Reader$$anonfun$read$1.applyOrElse(Types.scala:42)
    at upickle.Types$class.readJs(Types.scala:139)
    at upickle.package$.readJs(package.scala:10)
    at upickle.Generated$$anonfun$Tuple5R$1.applyOrElse(Generated.scala:49)
    at upickle.Generated$$anonfun$Tuple5R$1.applyOrElse(Generated.scala:49)
    at upickle.Reader$$anonfun$read$1.applyOrElse(Types.scala:42)
    at upickle.Reader$$anonfun$read$1.applyOrElse(Types.scala:42)
    at upickle.Types$class.readJs(Types.scala:139)
    at upickle.package$.readJs(package.scala:10)
    at upickle.Generated$InternalGenerated$$anonfun$Case5R$1.applyOrElse(Generated.scala:233)
    at upickle.Generated$InternalGenerated$$anonfun$Case5R$1.applyOrElse(Generated.scala:233)
    at upickle.GeneratedUtil$$anonfun$readerCaseFunction$1.applyOrElse(GeneratedUtil.scala:15)
    at upickle.GeneratedUtil$$anonfun$readerCaseFunction$1.applyOrElse(GeneratedUtil.scala:15)
    at upickle.Reader$$anonfun$read$1.applyOrElse(Types.scala:42)
    at upickle.Reader$$anonfun$read$1.applyOrElse(Types.scala:42)
    at upickle.Reader$$anonfun$read$1.applyOrElse(Types.scala:42)
    at upickle.Reader$$anonfun$read$1.applyOrElse(Types.scala:42)
    at upickle.Types$class.readJs(Types.scala:139)
    at upickle.package$.readJs(package.scala:10)
    at upickle.Types$class.read(Types.scala:135)
    at upickle.package$.read(package.scala:10)

As I understand, it's having problems with creating an instance of Map. Do you know any way to resolve this? Thanks!

@lihaoyi
Copy link
Member

lihaoyi commented Nov 14, 2014

The basic problems is that Maps serialize to lists of tuples, so they can support non-string keys properly. Other people have requested being able to serialize them to JSON dicts. I haven't any concrete plans to make it possible right now, but I'd like to do so at some point.

@lihaoyi
Copy link
Member

lihaoyi commented Nov 14, 2014

You could probably shadow the default implementation for serializing Maps and make it drop down to json dicts if the keys are all strings. Kinda hacky but it'd work, and I suspect it'd work.

@beloglazov
Copy link
Author

Thanks for your reply. I've added the following instances for Maps with String keys in my project:

  implicit def MapWithStringKeysW[V: Writer] = Writer[Map[String, V]] {
    obj => Js.Obj(obj.toSeq.map(x => (x._1, writeJs[V](x._2))): _*)
  }

  implicit def MapWithStringKeysR[V: Reader] = Reader[Map[String, V]] {
    case json: Js.Obj => json.value.map(x => (x._1, readJs[V](x._2))).toMap
  }

This works for me at the moment. Would you consider adding something like this to the library?

@lihaoyi
Copy link
Member

lihaoyi commented Nov 17, 2014

Yep, I'd like to do that at some point. How does this play with existing
non-string maps? Do those still work using []s instead of {}s?

On Mon, Nov 17, 2014 at 3:40 PM, Anton Beloglazov notifications@github.com
wrote:

Thanks for your reply. I've added the following instances for Maps with
String keys in my project:

implicit def MapWithStringKeysW[V: Writer] = Writer[Map[String, V]] {
obj => Js.Obj(obj.toSeq.map(x => (x._1, writeJsV)): _*)
}

implicit def MapWithStringKeysR[V: Reader] = Reader[Map[String, V]] {
case json: Js.Obj => json.value.map(x => (x._1, readJsV)).toMap
}

This works for me at the moment. Would you consider adding something like
this to the library?


Reply to this email directly or view it on GitHub
#39 (comment).

@beloglazov
Copy link
Author

Here are some examples you've given in #17 (comment):

println(upickle.write(Map("hello" -> (1, "world"))))
println(upickle.write(Map((1, "hello") -> "world")))
println(upickle.write(Map(Seq(1, 2, 3) -> "world")))
println(upickle.write(Map("hello" -> Seq(1, 2, 3))))

The output is:

{"hello":[1,"world"]}
[[[1,"hello"],"world"]]
[[[1,2,3],"world"]]
{"hello":[1,2,3]}

Would you like me to test anything else?

@lihaoyi
Copy link
Member

lihaoyi commented Nov 19, 2014

That looks perfectly reasonable. What about

def func[T: Writer](t: T) = upickle.write(Map(t -> 123))

func("10")
upickle.write(Map("10" -> 123))
func(10)

@beloglazov
Copy link
Author

In this case the output is:

[["10",123]]
{"10":123}
[[10,123]]

It didn't pull the correct implicit for the first case for some reason. Do you have any ideas of how to solve this?

@lihaoyi
Copy link
Member

lihaoyi commented Nov 19, 2014

I don't know if it's solvable; it's doing exactly what we're asking it to do: get a Writer[String], and then combining it with a Writer[Map[T, V]], which happens to be the array-of-tuples writer. The problem is when we ask for Writer[Map[T, V]], we don't know that T =:= String until runtime, at which point it's too late.

For it to be consistent, it feels to me that the only solutions would be to

  • Dynamically inspect the keys using .isInstanceOf[String] to check if they're all strings, converting it to a json dict if so, OR
  • Serialize non-strings into strings, the way Jackson does

Both of these feel pretty sketchy, but I'm not sure which one is less sketchy =/ Having the shape of your JSON changing unpredictably under you is sketchy, but so is having to recursively JSON.parse your string-keys to deserialize what should be a simple nested datastructure.

@beloglazov
Copy link
Author

I see, thanks for the explanation. I've added two more instances as you suggested:

implicit def MapW[K: Writer, V: Writer]: Writer[Map[K, V]] = Writer[Map[K, V]](
  x => {
    if (x.nonEmpty && x.head._1.isInstanceOf[String])
      Js.Obj(x.toSeq.map(entry => (entry._1.asInstanceOf[String], writeJs[V](entry._2))): _*)
    else
      Js.Arr(x.toSeq.map(writeJs[(K, V)]): _*)
  }
)

implicit def MapR[K: Reader, V: Reader]: Reader[Map[K, V]] = Reader[Map[K, V]] {
  case json: Js.Obj => json.value.map(x => (x._1.asInstanceOf[K], readJs[V](x._2))).toMap
  case json: Js.Arr => json.value.map(readJs[(K, V)]).toMap
}

The output is now correct for the case of String keys:

{"10":123}
{"10":123}
[[10,123]]

@lihaoyi lihaoyi changed the title Parsing to a map of case class instances Can't parse JSON map to Map[String, T] Nov 25, 2014
@lihaoyi
Copy link
Member

lihaoyi commented Nov 25, 2014

Here's an interesting case: what if instead of checking the head of the map, we checked the implicit Writer[K] to see if it's the StringWriter? That way we'd have consistent behavior with e.g. write(Map[String, T].empty), which in your current setup will default to [] rather than {}. By checking the Writer, it no longer matters if the Map is empty, and writing a Map of the same K will always result in the same "shaped" JSON structure.

write(Map[String, T].empty) and write(Map[MyCaseClass, T].empty) will differ ({} and [] respectively) but that's probably OK

@beloglazov
Copy link
Author

Sorry for taking long to reply. I agree, empty string-key maps should definitely be handled consistently with non-empty ones. However, I couldn't figure out how to check if Writer[K] is an instance of a string Writer. There is no StringWriter class and implicitly[Writer[K]].isInstanceOf[Writer[String]] fails because of the type erasure. Could you please clarify how you see doing that?

@lihaoyi
Copy link
Member

lihaoyi commented Dec 5, 2014

I was more thinking

implicitly[Writer[T]] == upickle.StringRW

That should work right?

@lihaoyi
Copy link
Member

lihaoyi commented Dec 5, 2014

Naturally, it won't work for people who define their own Writer[String] but maybe that's ok.

Another less-hard-coded solution is to provide Reader[T] and Writer[T] with another pair of method

Writer[T].asStringKey(t: T): Option[String]
Reader[T].fromStringKey(s: String): Option[T]

That will allow other non-upickle.StringRW implicits to emulate this behavior. For example, we may consider it desirable to make Map[T, V] for T in {Int, Short, Byte, Long, Boolean, Double, Float} have a asStringKey property that doesn't return None. Then users who wanted their own type T behave similarly could do so.

Of course, the downside is that this doubles the API surface-area of Reader and Writer. But maybe it's OK, especially if we make asStringKey/fromStringKey have a default of None so most people won't have to care about it

@beloglazov
Copy link
Author

Sorry, I didn't notice StringRW earlier. What do you think about the following version? It should probably handle custom Writer[String] as well:

implicit def MapW[K: Writer, V: Writer](implicit sw: Writer[String]): Writer[Map[K, V]] = Writer[Map[K, V]](
  x => {
    if (implicitly[Writer[K]] == sw)
      Js.Obj(x.toSeq.map(entry => (entry._1.asInstanceOf[String], writeJs[V](entry._2))): _*)
    else
      Js.Arr(x.toSeq.map(writeJs[(K, V)]): _*)
  }
)

You idea about asStringKey / fromStringKey sounds interesting as well. I guess it could be useful in some cases to be able to encode custom types into string keys of a json object.

@lihaoyi
Copy link
Member

lihaoyi commented Dec 5, 2014

On further thought, I think it'd be best to limit the weirdness of JSON objects to the Map serializer and not have it infect the rest of the code through the Reader and Writer traits. I like your solution the most out of the ones we've seen so far. Do you want to send a PR with tests for all the examples we discussed?

@lihaoyi
Copy link
Member

lihaoyi commented Dec 5, 2014

Remember you'll need to update MapR too so that round-trip serialization would work right

@beloglazov
Copy link
Author

Sure, I'll try to do that soon. Thanks!

@beloglazov
Copy link
Author

Thanks for making these changes! Sorry, I was busy with other work. Are you going to release a new version?

@lihaoyi
Copy link
Member

lihaoyi commented Dec 15, 2014

At some point; thinking of waiting for Scala.js 0.6.0 to come out. Lazy to modify and publish all my libraries more than once...

@beloglazov
Copy link
Author

Cool, I'm looking forward to that :)

@Nabegh
Copy link

Nabegh commented May 25, 2016

I think the error I'm receiving is related to this issue
https://groups.google.com/forum/#!topic/couchdb-scala/ZpHGuPBoS0Q

@OndrejSpanel
Copy link

Thanks for your reply. I've added the following instances for Maps with String keys in my project:

  implicit def MapWithStringKeysW[V: Writer] = Writer[Map[String, V]] {
    obj => Js.Obj(obj.toSeq.map(x => (x._1, writeJs[V](x._2))): _*)
  }

  implicit def MapWithStringKeysR[V: Reader] = Reader[Map[String, V]] {
    case json: Js.Obj => json.value.map(x => (x._1, readJs[V](x._2))).toMap
  }

This works for me at the moment. Would you consider adding something like this to the library?

It seems the library has evolved meanwhile and the writer needs to be written differently. I think following should do the job with recent version:

implicit def rwMap[T: ReadWriter]: ReadWriter[Map[String, T]] = readwriter[ujson.Obj].bimap[Map[String, T]](
    obj => obj.toSeq.map(x => (x._1, writeJs[T](x._2)))),
    {case json: Js.Obj => json.value.map(x => (x._1, read[T](x._2))).toMap}
  )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants