Design: auto-folding Some(None) → None

artem-zinnatullin commented 5 years ago

@weefbellington from our team at Lyft has been working on serializing/deserializing Koptional values in JSON and we've stuck for some time discussing following use case:

Optional<Optional<T>> (and other levels of direct nesting).

This use case is weird on its own, however Koptional does nothing to prevent it (neither does Arrow btw), so it's a valid state for Koptional 1.x.

Right now we've decided to serialize/deserialize without enhancing JSON with additional metadata, which results in following convention for JSON:

Some(None) → None

However, that creates difference between how we represent Some(None) in-memory vs JSON.

With all that in mind, I'd like to raise a discussion about this and propose following changeset for Koptional 2.x:

Hide Some constructor
Expose a function named Some(): Optional<T> that would return None for Some(None)

cc @ming13 @dmitry-novikov @nostra13 @AlecStrong @Egorand

oldergod commented 5 years ago

could you talk more about how you end up with Optional<Optional> ?

arturdryomov commented 5 years ago

The rule is simple — do not use Optional for serialization. Alternatively create a serializer converting optional to nullable.

arturdryomov commented 5 years ago

I thought about your case for some time and probably I can understand it (the description is a bit vague though, so I am speculating in any case). Also, Paco brought good points on Twitter, but seems like you weren’t satisfied.

We had a thought some time ago that optional is essentially the same as nullable which introduces the weird dichotomy — when to use optional and when to use nullable? What I see in your description suggests that you are trying to use optional as nullable replacement. Honestly saying this is not what Koptional was designed for — it is a solution for null checking JVM environments such as RxJava. Kotlin is good enough with handling nullability, it is JVM that is bad with it. Putting this aside, it still keeps an interesting issue with the conversion. Optional.None is null, Optional.Value is value. However, as you found out Optional can be a container for nested values and null obviously cannot. From this point of view I would suggest to leave it as is since it is a programming issue. Yes, Optional is a container and yes, it can be nested and yes, it is important to actually understand this. Optional is not null.

The suggested marshaling is pretty obvious.

data class Data(val value: Optional<String>)
serialize(Data(Some("text")))
serialize(Data(None))

is

{ "value": "text" }
{ "value": null }

data class Data(val value: Optional<Optional<String>>)
serialize(Data(Some(Some("text"))))
serialize(Data(Some(None)))

is

{ "value": "Some(\"text\")" }
{ "value": "None" }

If you want to be defensive you are free to introduce nesting checks. This however will not save you from bad understanding of JSON (and other formats) serialization and deserialization.

What you are proposing is not good on two cases.

It is an ad-hoc solution for your own issues. It will solve your issue with Some(None) but will do nothing to address Some(Some) (which should be serialized some way as well).
It replaces explicit behavior with implicit one — Koptional will decide when to repackage itself based on some rules or conventions.

This is a slippery slope we had before with Java, just with JSON. Cross-language conversion is hard and we are fighting the lost battle here. We are surrounded by JVM walls with our shiny nullability conventions. And this returns me to the initial statement — avoid using Optional in such cases. Think about it as an ad-hoc solution for JVM environments (i. e. RxJava). I would be grateful if this project did not exist at all since it is resolving Kotlin-solved issues all over again for no benefit except for conforming to some specs like the Reactive one.

artem-zinnatullin commented 5 years ago

@oldergod

could you talk more about how you end up with Optional ?

Yeah it's hard to think when Optional<Optional> is possible, one case that came to our minds was like this:

data class SomeContainer<T>(val field: Optional<T>, …)

Where T can end up being Optional and as developer of SomeContainer you don't know if that will be the case or not.

@ming13 your points are good and very similar to what I have on mind with one exception

The suggested marshaling is pretty obvious.

It is not obvious (let's talk about JSON for now).

With nulls you get transparent serialization/deserialization. What I mean by that is that JSON doesn't need to contain any additional information for you to get/put null or non-null value.

{ "value": "text" }
{ "value": null }

In your proposed schema (btw it's invalid JSON) you add Koptional specific metainformation to the JSON, which makes it incompatible with consumers that don't know about Koptional.

{ "value": "Some(\"text\")" }
{ "value": "None" }

Actually would be this (or None can be null right away):

{
  "value": {
    "some": "actualValue"
  }
}
{
  "value": {
    "none": null
  }
}

What I'm trying to achieve with this proposal is transparent serialization/deserialization with Koptional including nested cases.

So that we're still having this:

{ "value": "text" }
{ "value": null }

It is already possible to write type adapter in this manner, however then you end up having different presentation in memory vs serialized as long as we treat inner None.toOptional() and Some(None) as Some(None) instead of None.

To summarize, our philosophy in Koptional was that essentially null == None and non-null == Some

I think it's possible to reach same consistency in serialization/deserialization or maybe not, let's figure it out!

arturdryomov commented 5 years ago

In your proposed schema (btw it's invalid JSON) you add Koptional specific metainformation to the JSON, which makes it incompatible with consumers that don't know about Koptional.

Its purpose is to be invalid. It is an indication of programming error, i. e. serializing something that wasn’t meant to be serialized. I. e. fallback to .toString().

Do you want to introduce a Lint check instead for putting Optional into the Optional constructor?

artem-zinnatullin commented 5 years ago

Do you want to introduce a Lint check instead for putting Optional into the Optional constructor?

Well, that was my initial thought to throw exception (not Lint) if we detect that inner value is Optional, but I was convinced that it makes library too opinionated

gojuno / koptional

Design: auto-folding Some(None) → None #32