Using `Option` in fields fails in the `None` case

[anderslm]

If a field has an Option as it's value, eg:

  val FieldWithOption = ObjectType[Unit, Option[String]](
    "Option", 
    fields[Unit, Option[String]](Field("hello", StringType, resolve = _.value.getOrElse("Hello, world!"))))

  val QueryTypeThatFails = ObjectType("Query", fields[Unit, Unit](
    Field("hello", FieldWithOption, resolve = _ => None)
  ))

When this field is queried in the Some case it works, but in the None case it fails at runtime, when queried, with:

{
  "data" : null,
  "errors" : [
    {
      "message" : "Cannot return null for non-nullable type (line 1, column 3):\n{ hello { hello } }\n  ^",
      "path" : [
        "hello"
      ],
      "locations" : [
        {
          "line" : 1,
          "column" : 3
        }
      ]
    }
  ]
}

The complete example can be found here https://github.com/anderslm/sangria-option/

I'm guessing this is why it happens:

sangria/modules/core/src/main/scala/sangria/execution/ValueCoercionHelper.scala

Line 130 in f49d860

case None =>

[yanns]

The schema is not valid no? We cannot return None for the mandatory "hello" field.
A correct schema would be:

val QueryTypeThatFails = ObjectType("Query", fields[Unit, Unit](
    Field("hello", OptionType(FieldWithOption), resolve = _ => None)
  ))

[anderslm]

The schema is not valid no? We cannot return None for the mandatory "hello" field. A correct schema would be:

val QueryTypeThatFails = ObjectType("Query", fields[Unit, Unit](
    Field("hello", OptionType(FieldWithOption), resolve = _ => None)
  ))

The field is mandatory, but in the None case it is resolved by FieldWithOption using _.value.getOrElse("Hello, world!").

Using the change you propose the result will be:

{
  "data" : {
    "hello" : null
  }
}

even though FieldWithOption handles the None case and correctly returns a string.

I was expecting this result:

{
  "data" : {
    "hello" : {
      "hello" : "Hello world!"
    }
  }
}

[yanns]

The issue is not in FieldWithOption but in Field("hello", FieldWithOption, resolve = _ => None).
This non-null field is returning null.

[yanns]

A better way would be:

case class Hello(hello: Option[String])
val FieldWithOption = ObjectType[Unit, Hello](
    "Option", 
    fields[Unit, Hello](Field("hello", StringType, resolve = _.value.hello.getOrElse("Hello, world!"))))

  val QueryTypeThatFails = ObjectType("Query", fields[Unit, Unit](
    Field("hello", FieldWithOption, resolve = _ => Hello())
  ))

[anderslm]

The issue is not in FieldWithOption but in Field("hello", FieldWithOption, resolve = _ => None). This non-null field is returning null.

I get that, but isn't it weird though? And I wouldn't say that the non-null field returns null. It returns None which is a valid value for FieldWithOption, but it doesn't pass the value to FieldWithOption instead it treats it as null.

Is it intended to work like this? If it is I would expect it to fail at compile time.

The work-a-round (or better way :) ) you provided works, and is also similar to what I ended up doing, but it just doesn't seem right to me.

[yanns]

The issue is not in FieldWithOption but in Field("hello", FieldWithOption, resolve = _ => None). This non-null field is returning null.

I get that, but isn't it weird though? And I wouldn't say that the non-null field returns null. It returns None which is a valid value for FieldWithOption

None is the null of GraphQL.
In your case, None  is also a valid type for Option[String].

So you find an edge case where None can signify two things. Sangria interprets it as null.

If we want to avoid this edge case, we could try to disallow using Option[String] in the first place.
I don't think that anyone is using that. All implementations I've seen are using case class everywhere.

Personally, I won't take the time to try to avoid this. If you want to try, I can assist you. But I'm afraid it will add complexity the code base with no important value.

[anderslm]

Personally, I won't take the time to try to avoid this. If you want to try, I can assist you. But I'm afraid it will add complexity the code base with no important value.

What changes do you think are needed to disallow the use of Option?

[yanns]

I'm not sure. Maybe adding some implicit evidence, and not providing any evidence for Option[_].