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

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

Open
majjhima opened this issue Apr 18, 2023 · 5 comments

Comments

@majjhima
Copy link

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
Copy link
Contributor

yanns commented Apr 18, 2023

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

@majjhima
Copy link
Author

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
Copy link
Contributor

yanns commented Apr 18, 2023

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

@majjhima
Copy link
Author

majjhima commented Apr 18, 2023

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
Copy link
Contributor

yanns commented Sep 5, 2024

@majjhima any news on this?

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

2 participants