Encode NaN as null

[jonas]

While migrating from the Spray JSON marshaller to Circe I experienced the following error:

backend[ERROR] java.util.NoSuchElementException: None.get
backend[ERROR]  at scala.None$.get(Option.scala:347)
backend[ERROR]  at scala.None$.get(Option.scala:345)
backend[ERROR]  at sangria.marshalling.circe$.intro(circe.scala:85)
backend[ERROR]  at sangria.marshalling.circe$CirceResultMarshaller$.scalarNode(circe.scala:29)
backend[ERROR]  at sangria.marshalling.circe$CirceResultMarshaller$.scalarNode(circe.scala:7)
backend[ERROR]  at sangria.execution.Resolver$.marshalScalarValue(Resolver.scala:1032)
backend[ERROR]  at sangria.execution.Resolver.resolveValue(Resolver.scala:667)
backend[ERROR]  at sangria.execution.Resolver$$anonfun$24.apply(Resolver.scala:399)
backend[ERROR]  at sangria.execution.Resolver$$anonfun$24.apply(Resolver.scala:393)

I think it would be better to adopt Circe's encoding for NaN since users of the Circe marshalling library should be familiar with it.

[jonas]

I also wonder if the testkit should include a test for NaN

[OlegIlyenko]

Thanks a lot for pointing this out! I completely missed this aspect.

My main concern is that introducing this behavior will violate GraphQL spec. I just double-checked other implementations (e.g. spray-json) and they indeed return null for Double.NaN and infinity. I think that it is something that needs to be fixed.

Main issue is that even for non-null GraphQL Float type this will return a null. In my opinion this should be treated as an internal server error. In this case it also must be correctly represented in the response:

• for a non-null type it should be escalated to parent fields + add an error in the errors array
• for nullable field type it should result in a null value + add an error in the errors array

This would mean that it should be handled by the main library. Doing in a ResultMarshaller is a bit too late.

I prepared a PR that will implement this behavior: https://github.com/sangria-graphql/sangria/pull/167/files

What do you think?

[OlegIlyenko]

I like this change!

My concern is that it has unnecessary boxing in Option, though current version has it as well. getScalarValue would be is call for every scalar value in the input, so I think it would be better to avoid boxing in this case.

What do you think about something like this?

def getScalarValue(node: Json) = {
  def invalidScalar = throw new IllegalStateException(s"$node is not a scalar value")

  node.fold(
    jsonNull = invalidScalar,
    jsonBoolean = identity,
    jsonNumber = num ⇒ num.toBigInt orElse num.toBigDecimal getOrElse invalidScalar,
    jsonString = identity,
    jsonArray = _ ⇒ invalidScalar,
    jsonObject = _ ⇒ invalidScalar
  )
}

[jonas]

Yes, this looks more optimized.

[OlegIlyenko]

Even if we merge sangria-graphql/sangria#167, I think we can use fromDoubleOrNull here anyway. Main library will ensure that NaN and infinity will not reach this point, but at least we can avoid boxing (which fromDouble does).

[jonas]

I'm not that familiar with the GraphQL specification. Will sangria-graphql/sangria#167 require that such values are filtered before passing them to Sangria?

[OlegIlyenko]

yeah, otherwise you will get an error. If not handled, they would be generic internal server errors.

[jonas]

As long as there would be a way to tell sangria to ignore them? I'd hate to have to clean the data up front. For example we use Spark to read data we don't necessarily control from big query and S3 and serialize it to case classes. Having the ability to log the violations as warnings and fix them we go along would be preferable.

[OlegIlyenko]

I see, I started to wonder where you get all these NaNs, but this answers my curiosity :)

You can always define a custom scalar type which will use the old unsafe behavior (with additional warning log). Though you need to consider the consequences - clients may get null values for a not-null fields which in turn violates the spec.

In coerceOutput you don't have much info about the context (object and field), but you can implement a simple middleware which will log the warning together with the problematic object type and field name

As soon as you give an invalid Double to a library, the only reasonable thing it can do with it is to raise an error. Coercing it to 0 or substituting it with null would not be the correct way of handing such values (I would consider current behavior of a spray-json to be a bug, at least without additional validations in a scalar type).

WDYT? Will it be possible in your case?

[jonas]

Yes, sure, that works. Right now I only have a handful.

I will update the PR with your suggestions.