8000 Long value incorrectly decoded on Scala.js · Issue #393 · circe/circe · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Long value incorrectly decoded on Scala.js #393

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

Closed
tindzk opened this issue Sep 29, 2016 · 14 comments
Closed

Long value incorrectly decoded on Scala.js #393

tindzk opened this issue Sep 29, 2016 · 14 comments

Comments

@tindzk
Copy link
tindzk commented Sep 29, 2016

The following test case fails on Scala.js:

import cats.data.Xor
import io.circe.generic.auto._
import io.circe.parser._
import io.circe.syntax._

val num = 767946224062369796L
val json = num.asJson.noSpaces
assert(decode[Long](json) == Xor.Right(num))
@travisbrown
Copy link
Member

Thanks @tindzk—I'll take a closer look at this, but at a glance I think this is what you're seeing:

Also note that because scala.scalajs.js.JSON parses JSON numbers into a floating point representation, decoding a JSON number into a BigDecimal on Scala.js may lose precision.

Could you try decode[JsonNumber] and then toLong as a workaround?

@tindzk
Copy link
Author
tindzk commented Sep 30, 2016

Thanks! I tried the following code:

import io.circe._
import io.circe.parser._

println(767946224062369796L)
println(decode[Long]("767946224062369796"))
println(decode[JsonNumber]("767946224062369796"))
println(decode[JsonNumber]("767946224062369796").map(_.toLong))

Here is the JVM output:

767946224062369796
Right(767946224062369796)
Right(767946224062369796)
Right(Some(767946224062369796))

Unfortunately, it behaves differently on Scala.js:

767946224062369796
Right(767946224062369792)
Right(767946224062369800)
Right(Some(767946224062369792))

@jonas
Copy link
Contributor
jonas commented Nov 30, 2016

The problem is that circe uses JSON.parse for parsing and decoding in Scala.js. When circe gets to convert to its AST the damage is already done.

> JSON.parse("767946224062369796")
767946224062369800
> JSON.parse("767946224062369796").toFixed()
"767946224062369792"
> JSON.parse("767946224062369796").toPrecision()
"767946224062369800"

I will make a PR to document the current behaviour.

The only way to fix this would be to include a "custom" JSON parser for Scala.js and let users choose between performance or correctness.

@travisbrown
Copy link
Member

@jonas @tindzk I've just been thinking about this in conjunction with #476.

The idea would be that the fallback string decoder for numeric types would use circe's own number parser. That way Scala.js users would get the default JavaScript JSON parsing for ordinary JSON numbers, but if they wanted more precision they could represent their numbers as strings.

To me this seems less invasive than entirely changing the way JSON numbers are handled natively in JavaScript. The inconsistency isn't ideal, but I think that's part of the price we have to pay to work on a platform without nice numbers. (Not that the JVM is that much better. 😄).

@jonas
Copy link
Contributor
jonas commented Nov 30, 2016

Yes, that is a good point. So some kind of Configuration.withStringlyTypedNumerics?

@travisbrown
Copy link
Member

@jonas This would actually be the default, for both the JVM and Scala.js. It fixes the inconsistencies noted in #476, while also allowing Scala.js users to get more precision by representing their numbers as strings.

@jonas
Copy link
Contributor
jonas commented Nov 30, 2016

OK, will there be an option to turn it off when interoperability is needed?

@travisbrown
Copy link
Member

@jonas When would that be necessary? Just to clarify, the decoding of { "n": 767946224062369796 } as a Long would remain the same (i.e. wrong) on Scala.js—you'd only get the improved behavior with { "n": "767946224062369796" }.

@tindzk
Copy link
Author
tindzk commented Nov 30, 2016

I'm not sure whether having custom behaviour in the decoding process is a good idea. I am concerned it will impact interoperability with other JSON libraries. Wouldn't it be better not to create invalid JSON in the first place? I suppose the standard imposes restrictions on the precision of numbers. So, asJson could fail if a number cannot be possibly represented in JSON.

@travisbrown
Copy link
Member

@tindzk The issue is that the JSON standard doesn't put any restrictions on precisions—a JSON number can be as precise as you can fit in a string in memory (or more, I guess). The restrictions come from JavaScript, which in many cases isn't involved as a platform at all when working with JSON.

The behavior proposed in #476 doesn't customize JavaScript's JSON parser on Scala.js—it's entirely about decoding, and in my experience it's in line with other libraries in this respect. Representing numbers as strings when you need a lot of precision isn't uncommon—Jackson does it, other JavaScript stuff does it, etc. In circe I've done my best to make this unnecessary, and to make interoperability with libraries that do choose that route as easy as possible, and I think this is just an extension of that effort for Scala.js.

@tindzk
Copy link
Author
tindzk commented Nov 30, 2016

I see. Thanks for the clarifications. As @jonas suggests, an option for disabling this behaviour would still be desirable. I imagine the case where a developer wants to parse JavaScript-generated JSON in Circe. The decoding process should fail if Circe sees a string instead of a number.

@travisbrown
Copy link
Member

@tindzk Would you mind opening a separate issue for that? Since the beginning, circe (like Argonaut) has silently fallen back to attempting to parse JSON strings as numbers during decoding for numeric types—#485 just makes this a little more robust (and more consistent on the JVM side).

You could do the extra validation you propose right now with prepare on the decoder, but if there's enough demand for built-in support (as an option, not the default) I'd be happy to add it.

@tindzk
Copy link
Author
tindzk commented Nov 30, 2016

Thanks for fixing the bug! I created an issue as requested.

@tindzk tindzk closed this as completed Nov 30, 2016
jonas added a commit to jonas/circe that referenced this issue Dec 1, 2016
jonas added a commit to jonas/circe that referenced this issue Dec 1, 2016
@jonas jonas mentioned this issue Dec 1, 2016
jonas added a commit to jonas/circe that referenced this issue Dec 1, 2016
@armanbilge
Copy link
Contributor
armanbilge commented May 21, 2022

For anyone running into this issue, as of v0.14.2 you can use the cross-platform circe-jawn parser on both JVM and JS (instead of circe-parser). The circe-jawn parser does not exhibit the problem reported in this issue, and generally will have the same semantics on both the JVM and JS.

I hope this might become the default parser in the future, see:

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

4 participants
0