sangria-graphql / sangria

Scala GraphQL implementation
https://sangria-graphql.github.io
Apache License 2.0
1.96k stars 226 forks source link

Long type user input coercion missing implicit conversion from java.math.BigDecimal #995

Open majjhima opened 1 year ago

majjhima commented 1 year ago

We have been using Long type variables for output in our schema, but after adding an input variable of type Long, I would get an error message like:

Variable '$timestamp' expected value of type 'Long' but got: 1680746052401. Reason: Long value expected

After copying the LongType implicit conversion code to create a custom implicit val TimestampType: ScalarType[Long] and adding some debug output to test the user input datatype, I found:

Invalid Timestamp coerceUserInput, expected type Long, but got type class java.math.BigDecimal

The existing code has:

      case d: BigDecimal if d.isValidLong => Right(d.longValue)

but actually seems to need something like:

      case d: java.math.BigDecimal if BigDecimal(d).isValidLong => Right(d.longValue)
yanns commented 1 year ago

case d: BigDecimal does not handle java.math.BigDecimal?

majjhima commented 1 year ago

They are different types. I used the following code to test this:

  private case class TimestampCoercionViolation(value: String, typeName: String) extends ValueCoercionViolation(
    s"Positive Long value expected, but got $value of type $typeName"
  )

  implicit val TimestampType: ScalarType[Long] = ScalarType[Long](
    "Timestamp",
    description = Some(
      "The `Timestamp` scalar type represents non-fractional unsigned whole numeric values."
    ),
    coerceOutput = valueOutput,
    coerceUserInput = {
      case i: Int if i >= 0 => Right(i: Long)
      case i: Long if i >= 0L => Right(i)
      case i: BigInt if !i.isValidLong => Left(BigLongCoercionViolation)
      case i: BigInt if i >= 0 => Right(i.longValue)
      case d: Double if d.isWhole && d >= 0.0 => Right(d.toLong)
      case d: BigDecimal if d.isValidLong && d >= 0.0 => Right(d.longValue)
      case d: java.math.BigDecimal if BigDecimal(d).isValidLong && d.longValue >= 0.0 => Right(d.longValue)
      case x => Left(TimestampCoercionViolation(x.toString, x.getClass.toString))
    },
    coerceInput = {
      case ast.IntValue(i, _, _) => Right(i: Long)
      case ast.BigIntValue(i, _, _) if !i.isValidLong => Left(BigLongCoercionViolation)
      case ast.BigIntValue(i, _, _) => Right(i.longValue)
      case x => Left(TimestampCoercionViolation(x.toString, x.getClass.toString))
    }
  )

Without the java.math.BigDecimal condition, I get the exception:

Positive Long value expected, but got 1680746052401 of type java.math.BigDecimal
yanns commented 1 year ago

OK. Would you have time/do you want to open a PR for that?

majjhima commented 1 year ago

BTW, I found that in Scala 2.11, the class implicit conversion of java.math.BigDecimal to the Scala BigDecimal was moved to the object definition in this change replacing it with the following comment:

  // There was an implicit to cut down on the wrapper noise for BigDec -> BigDecimal.
  // However, this may mask introduction of surprising behavior (e.g. lack of rounding
  // where one might expect it).  Wrappers should be applied explicitly with an
  // eye to correctness.

We are using Scala 2.13.

Yes, I think I can submit a PR to address this.

yanns commented 1 month ago

@majjhima any news on this?