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

Conversion from JsonNumber to BigDecimal can throw exceptions #124

Closed
travisbrown opened this issue Nov 24, 2015 · 3 comments
Closed

Conversion from JsonNumber to BigDecimal can throw exceptions #124

travisbrown opened this issue Nov 24, 2015 · 3 comments

Comments

@travisbrown
Copy link
Member

JSON allows numbers with exponents larger than Int.MaxValue, and JsonNumber.fromString will happily accept them:

val Some(okay) = JsonNumber.fromString(s"1E${ Int.MaxValue.toLong }")
val Some(tooBig) = JsonNumber.fromString(s"1E${ Int.MaxValue.toLong + 1L }")

Unfortunately the toBigDecimal method just crashes on tooBig here:

scala> okay.toBigDecimal
res47: BigDecimal = 1E+2147483647

scala> tooBig.toBigDecimal
java.lang.NumberFormatException
  at java.math.BigDecimal.<init>(BigDecimal.java:491)
  at java.math.BigDecimal.<init>(BigDecimal.java:824)
  at scala.math.BigDecimal$.apply(BigDecimal.scala:289)
  at io.circe.JsonDecimal.toBigDecimal$lzycompute(JsonNumber.scala:187)
  at io.circe.JsonDecimal.toBigDecimal(JsonNumber.scala:187)
  ... 43 elided

toDouble also crashes, as does toLong, etc. (even though toLong returns an Option). The same thing happens with these methods in Argonaut.

This is bad, and needs to be fixed. Here's my proposal:

  1. toBigDecimal should return an Option that's empty if the value can't be parsed as a BigDecimal.
  2. We add a new approximateBigDecimal that uses one of the various pow(BigDecimal, BigDecimal) implementations floating around (e.g. a copy-paste-and-port job from here or some other implementation with a friendly license).
  3. The current contract of toDouble says that values outside the range of Double will be rounded to positive or negative infinity, so in the case that toBigDecimal is None, we use approximateBigDecimal and truncate.
  4. The behavior of toLong, toInt, etc. is unchanged, except that they no longer throw exceptions.
  5. All the truncateTo methods use approximateBigDecimal if necessary.
@etaty
Copy link
Contributor

etaty commented Nov 24, 2015

I think your range is too large

This specification allows implementations to set limits on the range and precision of numbers accepted. Since software that implements IEEE 754-2008 binary64 (double precision) numbers [IEEE754] is generally available and widely used, good interoperability can be achieved by implementations that expect no more precision or range than these provide, in the sense that implementations will approximate JSON numbers within the expected precision.
http://rfc7159.net/rfc7159#rfc.section.6

https://en.wikipedia.org/wiki/Double-precision_floating-point_format#Double-precision_examples

@travisbrown
Copy link
Member Author

@etaty Agreed that this isn't 100% necessary in any sense, but we're building a JSON library, not a processor, and it'd be nice to be able to represent losslessly as many valid JSON values as reasonably possible.

@travisbrown
Copy link
Member Author

Fixed in #189.

julienrf pushed a commit to scalacenter/circe that referenced this issue May 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants