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

JsonSchema doesn't allow nested Option #669

Open
marconilanna opened this issue Nov 2, 2020 · 4 comments
Open

JsonSchema doesn't allow nested Option #669

marconilanna opened this issue Nov 2, 2020 · 4 comments

Comments

@marconilanna
Copy link
Contributor

The code below fails to compile with the following message: could not find implicit value for parameter e: JsonSchema[Seq[Option[String]]] (and similar for c and d)

  implicit lazy val a = implicitly[JsonSchema[Seq[Seq[String]]]]
  implicit lazy val b = implicitly[JsonSchema[Seq[Option[String]]]]
  implicit lazy val c = implicitly[JsonSchema[Option[Option[String]]]]
  implicit lazy val d = implicitly[JsonSchema[Option[Seq[String]]]]
@julienrf
Copy link
Member

julienrf commented Nov 5, 2020

Here is a discussion related to this point: https://gitter.im/endpoints4s/endpoints4s?at=5f9200e9245ff718a77dcae2

In summary, the safest option IMHO would be to not use null to model None in JSON.

@harpocrates
Copy link
Collaborator

So while I agree that using null automatically might be undesirable (since JsonSchema[Option[Option[String]]] is almost sure to cause confusion due to not round-tripping Some(None)), it would be nice to be able to explicitly opt-in to a nullable schema for cases like JsonSchema[Seq[Option[String]]] and JsonSchema[Option[Seq[String]]]. Perhaps we could have an explicit combinator such as:

/* Note: make sure that this is only used in places where the `null` doesn't mean anything else
 *
 * Good (unambiguous about `null`):
 *    - `arrayJsonSchema(nullableJsonSchema(intJsonSchema))`
 *    - `arrayJsonSchema(nullableJsonSchema(genericJsonSchema[Foo]))`
 *
 * Bad (ambiguous about `null`):
 *    - `nullableJsonSchema(nullableJsonSchema(intJsonSchema))`
 *    - `optField("doubleNullable")(nullableJsonSchema(intJsonSchema))`
 */
def nullableJsonSchema[A](implicit schema: JsonSchema[A]): JsonSchema[Option[A]]

For non-OpenApi, this would have semantics like:

def nullableJsonSchema[A](implicit schema: JsonSchema[A]): JsonSchema[Option[A]] = new JsonSchema[Option[A]] {
  val encoder = _.fold[ujson.Value](ujson.Null)(schema.encoder.encode)
  val decoder = {
    case ujson.Null => endpoints4s.Valid(None)
    case other => schema.decoder.decode(other).map(Some(_))
  }
}

And for OpenApi it would set nullable: true on the underlying schema for A.


Side note: the OpenAPI interpreter is a little resistant to extension. I was going to try to write an extension trait for nullableSchema in our code that uses endpoints, but

  • DocumentedJsonSchema is a sealed ADT without the nullable OpenAPI property
  • the OpenApi JsonSchema type refers to ujsonSchemas.JsonSchema, into which I can't mix in my UJson extension trait

Not sure that there is anything to do about this side note though. Extensibility with type-safety is tricky and endpoints4s already does a great job.

@julienrf
Copy link
Member

@harpocrates OK, I’m fine with your suggestion.

About the extensibility of interpreters… you’re right. Extensible ADTs are hard (I remember this paper but the solution would be a bit complex… http://www.cs.ru.nl/~W.Swierstra/Publications/DataTypesALaCarte.pdf).

@harpocrates
Copy link
Collaborator

About the extensibility of interpreters… you’re right. Extensible ADTs are hard (I remember this paper but the solution would be a bit complex… http://www.cs.ru.nl/~W.Swierstra/Publications/DataTypesALaCarte.pdf).

Apart from that functional pearl, there's also https://www.microsoft.com/en-us/research/uploads/prod/2016/11/trees-that-grow.pdf. I'm not arguing for that though.

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

3 participants