Number serialization of java types outside of IEEE754 double precisio…

[bravehorsie]

…n range.

Signed-off-by: Roman Grigoriadi [email protected]

[bravehorsie]

Fixes #160

[m0mus]

I think that this functionality should be implemented on a higher level (JSONB). JSONP is a pure parser and should not contain this kind of logic.

[bravehorsie]

@m0mus JSONP is a Java API to serialize Java to JSON. This API includes types such as BigDecimal, Long and BigInteger and should be aware of how to handle them properly. Otherwise its clients (which are not using JSONB) would have no control over it.

[m0mus]

@bravehorsie JSONP is not an API to serialize Java Objects to JSON, it's JSONB. JSONP is a low level parser. It's used to write and read JSON data. If you need to write a number as string you should convert it to String and use JSONP methods to write strings.

[rmannibucau]

Well, this issue is still open and JsonGenerator inconsistent.
Issue is this method (and its "friends"): https://github.com/eclipse-ee4j/jsonp/blob/master/api/src/main/java/jakarta/json/stream/JsonGenerator.java#L288
if you read the java doc it says:

 * Writes a JSON name/number value pair in the current object context.
* The specified value is written as a JSON number value. The string
* {@code new BigDecimal(value).toString()}
* is used as the text value for writing.

So "JSON number value" is the result of "toString()" and "is used as the text value". Means the generator is designed to write not JSON portable outputs.

Just to comment on your last message @m0mus , issue is not on parsing side (we support more, it is great) but on writing side (we write things consumers can't read).

Being said both yasson and johnzon write BigDecimal as string, I guess the spec (javadoc here) can be clarified with "The specified value is written as a JSON string value.". Then there is no more issue since shape of the output json is constant for the same input types so consumers can deal with it.

Hope it makes sense.

[keilw]

The last communication was a little while ago, but unless @m0mus objects, do you @rmannibucau already have a working committer agreement for Eclipse or is there still a problem with your employer, because if you have an ECA then why not propose a PR for the spec?

[rmannibucau]

@keilw it is ok now, I'm fine doing the PR once we decide if we

1. Drop these methods (since they are redundant with String/double ones),
2. Fix the javadoc,
3. Option 3 (likely clarify the number/string ambiguity if any if it is not a mistake)

I'm for 2 since both impl picked the same behavior.

[m0mus]

I agree with @rmannibucau . We should fix the javadoc. I would appreciate if you provide a PR.

[leadpony]

Many people are confusing JSON and JavaScript or ECMAScript.
JSON specification does not say there is limits on the range and precision of numbers in JSON.

[rmannibucau]

@leadpony well you can also consider that:

1. the J of JSON is JavaScript ;),
2. 80% of the usage is about interoperability between languages (not only js/java but also ruby, python, go etc...),
3. that JSON spec mention that interoperability as having an undefined behavior (https://tools.ietf.org/html/rfc7159#section-6).

edit: here is the PR #237

[leadpony]

@rmannibucau
RFC 7159 says:

This specification allows implementations to set limits on the range and precision of numbers accepted.

I am not a native English speaker, but I believe allow means may, it does not mean must or should.
If write(String, BigDecimal) writes the value as a JSON string, we lose forever the only way to write a super big number as a JSON number. Using which of write(String, BigDecimal) or write(String, String) should be determined by API users.

[rmannibucau]

@leadpony "allow implementations" means "outside of the spec" so yes it is not forbidden, all implementations can have a toogle to enable that (passed in JsonGeneratorFactory properties) but spec can't specify that as a supported behavior. Rephrased: implementations are allowed to not be spec compliant with some specific configuration while they respect the spec by default. Here to embrace the best we can RFC7159 and encourage interoperability, default must be a deterministic typing with a good interoperability level, i.e. String cause all others options have more pitfalls than this one (not saying I'm happy with that btw, but it is the "least worse" I see in our current ecosystem).

[m0mus]

@leadpony @rmannibucau I re-read the javadoc and think that it may be OK. It means that the written value is JSONNumber (it means without quotes) and it's text value is the same as output of BigDecimal.toString() method.
So, if we are writing new BigDecimal("123.456") , 123.456 will be written, not "123.456". I cannot test it now, will do it later.
I guess that if we need to write a String value, one can pass yourBigDecimal.toString() instead of 'yourBigDecimal' to JsonGenerator.write method.

[rmannibucau]

@m0mus hmm, then we are back to the fact it is polymorphic and not as deterministic for consumers as expected so then this issue (and its friends) must be reopened.

[leadpony]

@m0mus
Unfortunately, all implementations including Jakarta, Johnzon and Joy, besides my JSON-P Test Suite, output it as a quoted string.
We should have named it writeAsString(String, BigDecimal)...

[m0mus]

@leadpony @rmannibucau Why you don't accept the "BigDecimal.toString()" workaround if JsonString is really needed. It's flexible. By default BigDecimal is serialized as JsonNumber, if user wants to serialize it as JsonString just use 'BigDecimal.toString`. What's wrong with this?

[m0mus]

@leadpony Are you saying that the current Jakarta JSONP implementation (this project) serializes BigDecimals as Strings?

[rmannibucau]

@m0mus I accepted the workaround but it does not solve the issue that we have an API which does not do what the users expect, or rephrased, it is an API user must not use generally. As mentioned, 80% of the time you will have this interoperability requirement so by default it should not overflow as a number making current API almost unusable. Another way to see it is to follow you reasonning: why don't we only have write([String,],JsonValue) method? So if we type we must enable the user to have his most common use cases working at least IMHO. This is super impacting for JSON-B because users start to share JSON-P components and JSON-B (to save some memory when there are pooling strategies or just reuse some global configs) and if JSON-B (or any layer on top of JSON-P) use that API then the behavior is encapsulated for users making it quite hard to change.

Since all impl use a String since 1.0 I think it makes sense to keep it specified as writing it as a String (and from an API point of view it is consistent with what the Java Big wrappers are so it is user friendly). About the remark on the naming, everything is named write so I guess this part is not a big deal and worse of everything would be to break working cases so at the end I guess we must keep current behavior.
Other choices are breaking changes today so guess the javadoc fix is the actual only option.

[leadpony]

@m0mus
Sorry, I was wrong. The current implementations serialize BigDecimals as JSON numbers , not quoted.
The following code

try (JsonGenerator g = Json.createGenerator(out)) {
    g.writeStartObject();
    g.write("amount", new BigDecimal("123.456"));
    g.writeEnd();
}

outputted the following JSON:

{"amount":123.456}

[jjspiegel]

Logic to write Java numbers as a JSON string does not belong inside the serializer.

[rmannibucau]

@jjspiegel we don't speak of the parser but generator. Question is really should BigDecimal (BigInteger) be represented in JSON as they are in java (by strings) and enabled a stable representation type or should they violate their contract and try to be represented as number (no quote) and break most JSON parsers out there which will try to parse it as a double (including in java ecosystem, there are a tons of minijson/simple-json forks doing that).

[m0mus]

About the remark on the naming, everything is named write so I guess this part is not a big deal and worse of everything would be to break working cases so at the end I guess we must keep current behavior.
Other choices are breaking changes today so guess the javadoc fix is the actual only option.

@rmannibucau I agree that we should not introduce any backwards incompatible changes. What do you want to change in javadoc? Do you want to have a note there that for better compatibility user may consider writing it as Sting using toStong() method?

[rmannibucau]

@m0mus originally did the same error than @leadpony and through RI and other impls were writing real JSON string values so wanted to change JSON number to JSON string, but since they all write the string not quoted (seems it is validated by TCKs ;)), we should document at least that this method is not portable and should be used with caution at least. I agree that pointing out the write(decimal.toString()) is a solution we must explicit probably.