amnaredo / test

0 stars 0 forks source link

Why will upickle convert `Option[T]` to array `[...]`? #117

Open amnaredo opened 3 years ago

amnaredo commented 3 years ago

I found if there is an option value in case class, it will be converted to [] in js

case class User(name:String, age: Option[Int])

val user1 = User("Free", Some(100))
val user2 = User("Wind", None)

Will be converted to:

{ "name": "Free", "age": [100] }
{ "name": "Wind", "age": [] }

Why use array here rather than:

{ "name": "Free", "age": 100 }
{ "name": "Wind" }

The later is the one I usually do with other scala json libraries

ID: 75 Original Author: freewind

amnaredo commented 3 years ago

This sounds reasonable, but how would you encode Some(None): Option[Option[Int]]?

Original Author: tindzk

amnaredo commented 3 years ago

Never thought of that. You are right, it's reasonable to use array for Option.

But for the json readers, the array type may confuse them because they may think that value can have multiple items. Is there any way to customize the format?

Original Author: freewind

amnaredo commented 3 years ago

No, unfortunately, it is not possible to customise the format. I agree that uPickle should provide an option to change the default behaviour. You could have a look at https://github.com/tindzk/upickle/commits/change-sumencoding or https://github.com/lihaoyi/upickle/pull/65 on how to introduce a configuration object. However, I'd prefer a solution with the Cake pattern, see https://github.com/lihaoyi/upickle/issues/59#issuecomment-72096982.

Original Author: tindzk

amnaredo commented 3 years ago

Closing this for now, feel free to talk on the other issues

Original Author: lihaoyi

amnaredo commented 3 years ago

@tindzk,

but how would you encode Some(None): Option[Option[Int]]?

There is a simple answer on this. Some(None): Option[Option[Int]] should be encoded as [[]] when it is a standalone value or a part of a collection, but if it is a part of a case class:

case class Weird(value: Option[Option[Int]])

then the "outer" option should be encoded as a presence or absence of the value, while the inner option should be encoded as an array:

write(Weird(None))       shouldEqual """{}"""
write(Weird(Some(None))) shouldEqual """{"value":[]}"""
write(Weird(Some(1234))) shouldEqual """{"value":[1234]}"""

This does require that the macro-based serializer knows how to handle optional fields, but it is not impossible and certainly does make sense.

Original Author: netvl

amnaredo commented 3 years ago

This seems like a good approach to me. We should reopen the ticket.

Original Author: tindzk

amnaredo commented 3 years ago

Scala newb here, so this is probably wrong, but wouldn't:

write(Weird(Some(1234))) shouldEqual """{"value":[1234]}"""

in the case above be:

write(Weird(Some(Some(1234)))) shouldEqual """{"value":[1234]}"""

I'm also running into this issue, so it would be great to get this kind of Option handling in there.

Original Author: GiantToast

amnaredo commented 3 years ago

@tindzk sure I can re-open it. I probably won't put in any work into fixing this, but now that uPickle's configurable via instantiating your own custom bundles it'd be cool if someone created a bundle/trait with this behavior

Original Author: lihaoyi

amnaredo commented 3 years ago

Closing out of inactivity

Original Author: lihaoyi

amnaredo commented 3 years ago

This writer is applied for Some(v) :

  implicit val optHavingWriter = upickle.default.Writer[Option[HavingSpec]]{
    case Some(v) => writeJs[HavingSpec](v)
    case None => Js.Null
  }

This one too :

  implicit val optBooleanWriter = upickle.default.Writer[Option[Boolean]]{
    case Some(v) => writeJs[Boolean](v)
    case None => Js.Null
  }

But these are not :

  implicit val optStringWriter = upickle.default.Writer[Option[String]]{
    case Some(v) => writeJs[String](v)
    case None => Js.Null
  }
  implicit val optIntWriter = upickle.default.Writer[Option[Int]]{
    case Some(v) => writeJs[Int](v)
    case None => Js.Null
  }

Unfortunately generic types doesn't work at all, so I have to list a custom writer for each type :

  implicit def optWriter[T : Writer] = upickle.default.Writer[Option[T]]{
    case Some(v) => writeJs[T](v)
    case None => Js.Null
  }

It would be cool if it worked even for String and Int ... any idea ?

Original Author: l15k4

amnaredo commented 3 years ago

You actually can write a generic replacement for the Option Reader and Writer. This trick is that you need to shadow the one that comes with upickle. So, you can do it with the following:

  implicit def OptionReader[T: Reader]: Reader[Option[T]] = reader[Js.Value].map[Option[T]]{
    case Js.Null => None
    case jsValue => Some(read[T](jsValue))
  }
  implicit def OptionWriter[T: Writer]: Writer[Option[T]] = writer[Js.Value].comap {
    case Some(value) => write(value)
    case None => Js.Null
  }

This only works because The Reader has the name OptionReader and the Writer has the name OptionWriter. These are the names of instances provided by upickle. Since we are providing new ones with the same name, the ones provided by upickle will be shadowed and ignored. Original Author: asdcdow

amnaredo commented 3 years ago

One common use case would be a reader for

case class MyCC(a: String, b: String, c: Option[String])

so that undefined is parsed too :

// expected
{"a": "x", "b": "y"}           => MyCC("x", "y", None)
// upickle.core.AbortException: missing keys in dictionary: c

// expected and verified
{"a": "x", "b": "y", "c": "z"} => MyCC("x", "y", Some("z"))

Last solution from @asdcdow does this

{"a": "x", "b": "y", "c": null} => MyCC("x", "y", None)

Modifying MyCC with Defaults

case class MyCC(a: String, b: String, c: Option[String] = None)

I get what I expect

// expected and verified
{"a": "x", "b": "y"}           => MyCC("x", "y", None)

Original Author: mycaule

amnaredo commented 3 years ago

Did anyone ever come up with a good way of doing this such that writing a Some(obj) is not written as an array AND None is not written at all? @asdcdow 's solution, and the solution in the docs makes Some write like the OP wants, but they also make None 's write as nulls.

I've tried playing with the OptionWriter source, but I'm just not understanding it's visitor pattern / array writing. It also seems like NoneWriter and SomeWriter do nothing?

I'm also serializeDefaults = true, which removes that route.

For others who can't figure this out, at least jq can hack around it:

jq 'walk( if type == "object" then with_entries(select(.value != null)) else . end)'

I guess I could write the json string, rewalk it and remove nulls, then write it again, but there must be a way to do this by overriding OptionWriter that I'm not seeing.

https://github.com/lihaoyi/upickle/issues/75#issue-60343929 describes what I think no option currently covers yet. Original Author: sstadick