Fix #292: decode special numbers (Infinite, NaN)

[fsarradin]

To fix it, it seems that I've to add chars N and I in the function checkNumber in lexer,scala . But it seems that something equivalent should be done for the function skipValue and isNumber. For the last one, it seems that we have to add in the case the letters in NaN and Infinity.

I have also added some tests.

[fsvehla]

Nan, and Infinity are not part of JSON — what is your use case here?

https://tools.ietf.org/html/rfc7159#section-6

[fsarradin]

@fsvehla thanks for the review.

I agree with your point. But

1. part of the ZIO JSON implementation is already made to parse NaN and Infinity (https://github.com/zio/zio-json/blob/develop/zio-json/shared/src/main/scala/zio/json/internal/numbers.scala#L334)
2. The encoder part gives these results:

From

import zio.json._

println(123.4.toJson)
println(Double.NaN.toJson)
println(Double.PositiveInfinity.toJson)
println(Double.NegativeInfinity.toJson)

I get

123.4
"NaN"
"Infinity"
"-Infinity"

So for more consistency, I propose in this PR an implementation that accepts NaN and Infinity.

On the other side, to be in compliance with RFC7159, I think it is needed to remove the parsing of NaN and Infinity and to return an error when trying to convert NaN or Infinity into JSON. This would mean that .toJson would return Either[String, A]. I would be OK to turn this PR this way.

What do you think?

[fommil]

note that "NaN" is printed (valid JSON). Not NaN, as you are attempting to support. If you want to support the former during decoding, then allow strings as well as raw JSON numbers. I thought this was supported already in Decoder.number https://github.com/zio/zio-json/blob/develop/zio-json/shared/src/main/scala/zio/json/decoder.scala#L292

[fsarradin]

@fommil thanks for your comment. I understand your point.

So I've changed the way to check and parse number in the context of double/float, and ensure encoder/decoder are invertible for those types. But this results in test "Decoder > ZIO Streams integration > decodeJsonTransducer > Newline delimited > is interruptible" to fail (any idea why?).

What do you think of this approach?

Note: as "-Infinity".fromJson[Double] succeeds, I've created the issue #294 .

[fsvehla]

@fsarradin Please push a WIP branch and I'll have a look at the ZIO-streams transducer test

[fsarradin]

@fsvehla thanks a lot.

The last update is the WIP. But it seems that the test failure only happens on my machine!? 😅

[fsvehla]

OK, I’ll investigate the flaky test

[fommil]

adding .retract is not correct.

Please start by adding a unit test along the lines of parsing "Infinity" into a Double. That should work, if it doesn't then it's a bug. I think the addition of checkNumber (by me in the very beginning) when parsing the contents of strings introduced this. We still want to check for valid numbers in their JSON position so maybe it's just a case of having a version of only doing checkNumber when it's a JSON number and otherwise just skipping whitespace (with a retract as soon as a non-whitespace is encountered).

TL;DR rename checkNumber to prepareNumber, add a Boolean parameter validate. If it is true, do what is currently happening, if it is false just do nextNonWhitespace + retract. Pass true when parsing numbers in JSON number position, pass false when parsing numbers that are inside strings.

BTW I'm hesitant to call this a bug. Certainly it's a nice quality of life feature. But if you take some of these JSON payloads that are being produced and try to parse them in any other library or on another platform, you'll be lucky if they decode the way you are expecting here.

[fsarradin]

@fommil I think my new commit almost fulfills what you have specified. But still, I've the same failure with ZIO-streams transducer test.

BTW I'm hesitant to call this a bug. Certainly it's a nice quality of life feature. But if you take some of these JSON payloads that are being produced and try to parse them in any other library or on another platform, you'll be lucky if they decode the way you are expecting here.

TBH it is the source of my former confusion in this PR. Nevertheless, the problem comes from trying to match IEEE754 with numbers as described in RFC7159. As the one is not the subset of the other, I think that we have to consider two possibilities. 1/ Float or Double should not be allowed in JSON conversion. 2/ Double and Float are allowed, but developers should have to consider first the type BigDecimal to represent numbers, unless they know what they are doing :)

This said, IEEE754 is a long-time and widely adopted standard. I tend to think that there might have just a few libs and platforms that does not support it. Currently, I have done tests with JSON libs like in Java (Jackson) and Python (in its SDK) and they recognize it. BTW, it's an optimistic point of view :D

[fsvehla]

I can’t reproduce the flaky test — @fsarradin can you paste the test output?

[fsarradin]

Here is what I get:

[info]   - ZIO Streams integration
[info]     + decodes a stream of chars
[info]     + decodes an encoded stream of bytes
[info]     - decodeJsonTransducer
[info]       - Newline delimited
[info]         + decodes single elements
[info]         + decodes multiple elements
[info]         + decodes multiple elements when fed in smaller chunks
[info]         + accepts trailing NL
[info]         + errors
[info]         - is interruptible
[info] Timeout of 2 s exceeded.

[fsarradin]

@fsvehla Hi, did you reproduce the problem?

[fsvehla]

No, and it seems unrelated.

@fsarradin this looks fine, but will have to make it into 0.2 — I’m thinking about how to manage this and will come back to this shortly.

[guizmaii]

@fsvehla Any news maybe?