Serialization of Option types

[amnaredo]

The Option types are serialized as a list/array "[]". From tests,

'option{
  'Some-rw(Some(123), "[123]")
  'None-rw(None: Option[String], "[]")
  'Option{
    rw(Some(123): Option[Int], "[123]")
    rw(None: Option[Int], "[]")
  }
}

This behaviour is wrong: i.e Some(123) should be 123. None should be null. The problem is in macro expansion, because Option is lifted. I try to create a pull request to fix the bug.

ID: 28 Original Author: aparo

[amnaredo]

I think the serialization is fine the way it is. Honestly Options are conceptually lists with length 0 or 1, hence the map, flatMap, and other operations that work with normal collections.

The problem is in macro expansion, because Option is lifted.

I am not sure what that means, are you referring to the reading or the writing side of the macro.

Original Author: dispalt

[amnaredo]

How would you serialize Some(None): Option[Option[T]] and differentiate it from None: Option[Option[T]]?

On Tue, Sep 23, 2014 at 1:50 PM, Alberto Paro notifications@github.com wrote:

The Option types are serialized as a list/array "[]". From tests,

'option{ 'Some-rw(Some(123), "[123]") 'None-rw(None: Option[String], "[]") 'Option{ rw(Some(123): Option[Int], "[123]") rw(None: Option[Int], "[]") } }

This behaviour is wrong: i.e Some(123) should be 123. None should be null. The problem is in macro expansion, because Option is lifted. I try to create a pull request to fix the bug.

— Reply to this email directly or view it on GitHub https://github.com/lihaoyi/upickle/issues/28.

Original Author: lihaoyi

[amnaredo]

I understand your point of view, I agree that Option monad is a collection, but upickle is incompatible with classes serialized with Play Json, json4s and other libs.

Just my 2 cent, but Some(None):Option[Option[T]] seems to me a bad designed code.

Original Author: aparo

[amnaredo]

@aparo if you really care, you can define your own implicit def optionRW[T]: upickleReadWriter[Option[T]] = ... that will override it to do what you want. It's not hard and lets you override any of the default implicits, albeit a bit dangerously: it'll silently fallback to a default if you forget to import your thing. The next release of upickle will fix this.

Original Author: lihaoyi

[amnaredo]

Unfortunately, it seems like it is impossible to override reading an optional value. Even with something like

  implicit def optionReadWriter[T: Reader: Writer] = ReadWriter[Option[T]]({
    case Some(value) => writeJs(value)
    case None => Js.Null
  }, {
    case Js.Null => None
    case js => Some(readJs[T](js))
  })

structures like

{ "optionalValue": null }

are read into

MyCaseClass(null)

And because Reader.read is final, it is impossible to override this behavior.

Original Author: netvl

[amnaredo]

See #40 and PR https://github.com/ramnivas/upickle/commit/88cd9bddce109f4339002620affc4ac62e324eef for an attempt to deal with it.

@ddispaltro You are certainly right in that an Option could/should be looked as a 0 or 1 length collection. That, of course, works great in programs. But for a serialization format, IMO, Option represents presence or absence of the contained value. Therefore, serializing the flattened value seems more appropriate. This flattening approach may also be a principled way to deal with Some(None). Basically, we can take an approach where any Option value is recursively flattened for serialization purpose. It does produce asymmetry in that Some(None) will be serialized in the same way as None and thus deserialized to None in either case. But would that matter in a typical program (especially assuming that such behavior could be overridden for programs where it could matter)? It also produces asymmetry when it comes to, say, List[Option[T]]. But that too could be a non-default overridable behavior.

Original Author: ramnivas

[amnaredo]

@ramnivas the problem is that the idea of "present or not" doesn't make sense in the general context, but only makes sense in the context of a case class! Recursively flattening options isn't just "asymmetric", it means that

read(write(t)) != t

For a vast number of types: Option[Option[T]], List[Option[T]], etc.

read(write(Some(None)) -> None
read(write(List(None)) -> List()

This is basically the same as corrupting everyone's data silently which I don't want to do.

I've proposed an alternative, which is chucking the custom logic in the Case{Reader, Writer}, which seems to make a lot more sense. You haven't said whether you think that's a good or bad idea =P

Original Author: lihaoyi

[amnaredo]

@lihaoyi by "asymmetric", I did mean read(write(t)) != t :-)

I am totally fine with having a custom Reader/Writer. The last time I tried (in the context of #40 and the attendant PR), it didn't work and @netvl comment made me think that it is still the case. But if it can work now or soon, sure!

Original Author: ramnivas

[amnaredo]

It should totally work, if you can show me why it didn't work I can help make progress =)

On Fri, Jan 23, 2015 at 2:30 PM, Ramnivas Laddad notifications@github.com wrote:

@lihaoyi https://github.com/lihaoyi by "asymmetric", I did mean read(write(t)) != t :-)

I am totally fine with having a custom Reader/Writer. The last time I tried (in the context of #40 https://github.com/lihaoyi/upickle/issues/40 and the attendant PR), it didn't work and @netvl https://github.com/netvl comment made me think that it is still the case. But if it can work now or soon, sure!

— Reply to this email directly or view it on GitHub https://github.com/lihaoyi/upickle/issues/28#issuecomment-71277670.

Original Author: lihaoyi

[amnaredo]

If it totally works, then count me happy :-) @netvl's comment made me think otherwise. I will try again (need to head out now, so later today or tomorrow). I'll report my findings if it works or not.

Original Author: ramnivas

[amnaredo]

If it doesn't work, then we should fix it so it'll work. I'm not going to add a wrong implicit and pollute every other piece of logic ever just because of a bug in the correct solution =P

Original Author: lihaoyi

[amnaredo]

@lihaoyi, the following instance indeed does not work:

  implicit def optionReadWriter[T: Reader: Writer] = ReadWriter[Option[T]]({
    case Some(value) => writeJs(value)
    case None => Js.Null
  }, {
    case Js.Null => None
    case js => Some(readJs[T](js))
  })

It does not work because matching on Js.Null is currently useless - Js.Null is handled by the hard-coded final def read() method on the Reader. That's why I have submitted #63.

Original Author: netvl

[amnaredo]

@lihaoyi I had reached the same conclusion as @netvl when I tried it in the context of #40. Please let me know if you had something else in mind for me to try.

Original Author: ramnivas

[amnaredo]

I guess you will need to modify the null-handler in order to make the null-safe behavior part of each individual Reader/Writer's definition rather than in read/write. This will be some legwork but it's doable.

Original Author: lihaoyi

[amnaredo]

@lihaoyi, that's the next thing on my TODO list after #65 and probably "filters", if you don't mind.

Original Author: netvl

[amnaredo]

Go for it. I can't guarantee I'll merge everything, but that's also true of my own code too ^_^ We'll see what we can hammer together

Original Author: lihaoyi