TCK Challenge - incorrectly testing if big number marshaling fits IEEE754

[shighbar]

Specification version
JSONB 1.0

Coordinates of the challenged test(s)
https://github.com/eclipse-ee4j/jakartaee-tck/tree/master/src/com/sun/ts/tests/jsonb/defaultmapping

TCK version & exclude list versions
build.name=jakartaeetck-8.0.0_07-Aug-2019
build.date.time=Wed, 7 Aug 2019 08:52:52
build.comment=\

Exclude list default with above build. Last comit 5c0de06 on March 27, 2019.

Implementation under test
Open Liberty, IBM Corporation
https://openliberty.io/
OpenJDK 1.8.0_222

Full test names:
jsonb/defaultmapping/basictypes/BasicJavaTypesMappingTest_testLongMapping_from_servlet.jtr
jsonb/defaultmapping/basictypes/BasicJavaTypesMappingTest_testLongMapping_from_jsp.jtr
jsonb/defaultmapping/basictypes/BasicJavaTypesMappingTest_testLongMapping_from_ejb.jtr
jsonb/defaultmapping/basictypes/BasicJavaTypesMappingTest_testLongMapping_from_appclient.jtr
jsonb/defaultmapping/bignumbers/BigNumbersMappingTest_testBigNumberMarshalling_from_ejb.jtr
jsonb/defaultmapping/bignumbers/BigNumbersMappingTest_testBigNumberMarshalling_from_servlet.jtr
jsonb/defaultmapping/bignumbers/BigNumbersMappingTest_testBigNumberMarshalling_from_appclient.jtr
jsonb/defaultmapping/bignumbers/BigNumbersMappingTest_testBigNumberMarshalling_from_jsp.jtr

Description
The problem is that these tests are expecting: {"number":"0.10000000000000001"} but it gets: {"number":0.10000000000000001}.
Here is the justification for why it is the way it is now: eclipse-ee4j/yasson#269 (i.e. without quotes around the number).
With Yasson 1.0.4 being updated to support this change, the tests are incorrect in how they are testing these values.
Given we don't want to change tests in the TCK for the initial Jakarta release I think these tests should be ignored until they can be updated in the TCK in the next release.

Supporting materials:

eclipse-ee4j/yasson#269

excerpt from test case log;
08-14-2019 13:55:35: SVR-TRACE: ABOUT TO INVOKE SETUP METHOD!
08-14-2019 13:55:35: SVR: setup ok
08-14-2019 13:55:35: SVR-TRACE: INVOKED SETUP METHOD!
08-14-2019 13:55:35: SVR-TRACE: ABOUT TO INVOKE EETEST RUN METHOD!
08-14-2019 13:55:35: SVR-ERROR: Failed to correctly marshal number of greater precision than IEEE 754 as string.
08-14-2019 13:55:35: SVR-ERROR: Test case throws exception: Failed to correctly marshal number of greater precision than IEEE 754 as string.
08-14-2019 13:55:35: SVR-ERROR: Exception at:
08-14-2019 13:55:35: SVR-ERROR: com.sun.ts.lib.harness.EETest$Fault: Failed to correctly marshal number of greater precision than IEEE 754 as string.
at com.sun.ts.tests.jsonb.defaultmapping.bignumbers.BigNumbersMappingTest.testBigNumberMarshalling(BigNumbersMappingTest.java:74)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:498)
at com.sun.ts.lib.harness.EETest.run(EETest.java:596)
at com.sun.ts.lib.harness.ServiceEETest.run(ServiceEETest.java:115)
at com.ibm._jsp._jsp_5F_vehicle.runTest(_jsp_5F_vehicle.java:62)
at com.ibm._jsp._jsp_5F_vehicle._jspService(_jsp_5F_vehicle.java:177)
at com.ibm.ws.jsp.runtime.HttpJspBase.service(HttpJspBase.java:100)
at javax.servlet.http.HttpServlet.service(HttpServlet.java:791)
at com.ibm.ws.webcontainer.servlet.ServletWrapper.service(ServletWrapper.java:1255)
at com.ibm.ws.webcontainer.servlet.ServletWrapper.handleRequest(ServletWrapper.java:743)
at com.ibm.ws.webcontainer.servlet.ServletWrapper.handleRequest(ServletWrapper.java:440)
at com.ibm.wsspi.webcontainer.servlet.GenericServletWrapper.handleRequest(GenericServletWrapper.java:118)
at com.ibm.ws.jsp.webcontainerext.AbstractJSPExtensionServletWrapper.handleRequest(AbstractJSPExtensionServletWrapper.java:207)
at com.ibm.ws.webcontainer.filter.WebAppFilterChain.invokeTarget(WebAppFilterChain.java:182)
at com.ibm.ws.webcontainer.filter.WebAppFilterChain.doFilter(WebAppFilterChain.java:93)
at com.ibm.ws.security.jaspi.JaspiServletFilter.doFilter(JaspiServletFilter.java:56)
at com.ibm.ws.webcontainer.filter.FilterInstanceWrapper.doFilter(FilterInstanceWrapper.java:201)
at com.ibm.ws.webcontainer.filter.WebAppFilterChain.doFilter(WebAppFilterChain.java:90)
at com.ibm.ws.webcontainer.filter.WebAppFilterManager.doFilter(WebAppFilterManager.java:996)
at com.ibm.ws.webcontainer.filter.WebAppFilterManager.invokeFilters(WebAppFilterManager.java:1134)
at com.ibm.ws.webcontainer.webapp.WebApp.handleRequest(WebApp.java:4975)
at com.ibm.ws.webcontainer.osgi.DynamicVirtualHost$2.handleRequest(DynamicVirtualHost.java:314)
at com.ibm.ws.webcontainer.WebContainer.handleRequest(WebContainer.java:1007)
at com.ibm.ws.webcontainer.osgi.DynamicVirtualHost$2.run(DynamicVirtualHost.java:279)
at com.ibm.ws.http.dispatcher.internal.channel.HttpDispatcherLink$TaskWrapper.run(HttpDispatcherLink.java:1061)
at com.ibm.ws.http.dispatcher.internal.channel.HttpDispatcherLink.wrapHandlerAndExecute(HttpDispatcherLink.java:417)
at com.ibm.ws.http.dispatcher.internal.channel.HttpDispatcherLink.ready(HttpDispatcherLink.java:376)
at com.ibm.ws.http.channel.internal.inbound.HttpInboundLink.handleDiscrimination(HttpInboundLink.java:532)
at com.ibm.ws.http.channel.internal.inbound.HttpInboundLink.handleNewRequest(HttpInboundLink.java:466)
at com.ibm.ws.http.channel.internal.inbound.HttpInboundLink.processRequest(HttpInboundLink.java:331)
at com.ibm.ws.http.channel.internal.inbound.HttpInboundLink.ready(HttpInboundLink.java:302)
at com.ibm.ws.tcpchannel.internal.NewConnectionInitialReadCallback.sendToDiscriminators(NewConnectionInitialReadCallback.java:165)
at com.ibm.ws.tcpchannel.internal.NewConnectionInitialReadCallback.complete(NewConnectionInitialReadCallback.java:74)
at com.ibm.ws.tcpchannel.internal.WorkQueueManager.requestComplete(WorkQueueManager.java:503)
at com.ibm.ws.tcpchannel.internal.WorkQueueManager.attemptIO(WorkQueueManager.java:573)
at com.ibm.ws.tcpchannel.internal.WorkQueueManager.workerRun(WorkQueueManager.java:954)
at com.ibm.ws.tcpchannel.internal.WorkQueueManager$Worker.run(WorkQueueManager.java:1043)
at com.ibm.ws.threading.internal.ExecutorServiceImpl$RunnableWrapper.run(ExecutorServiceImpl.java:239)
at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
at java.lang.Thread.run(Thread.java:748)

08-14-2019 13:55:35: SVR: cleanup ok
08-14-2019 13:55:35: SVR: Test running in jsp vehicle failed
08-14-2019 13:55:36: TRACE: SLEPT FOR: 1000
STATUS:Failed.Test case throws exception: Failed to correctly marshal number of greater precision than IEEE 754 as string.
result: Failed. Test case throws exception: Failed to correctly marshal number of greater precision than IEEE 754 as string.

test result: Failed. Test case throws exception: Failed to correctly marshal number of greater precision than IEEE 754 as string.

[bravehorsie]

I suppose this test should be removed for now and reworked after jakartaee/jsonp-api#160 is integrated.

The thing with "fits IEEE754" in the spec is that it lacks definition. Any of the floating point decimal values can be converted to IEEE754 double precision but most of them would be inprecise after conversion. For example a decimal value of 0.1

In my opinion it should be also excluded from the next release of the spec.

[kwsutter]

Just to clarify, this is a similar issue that WebSphere Liberty raised during the Java EE 8 testing. We're just trying to make it Jakarta EE 8 official. Since we can not change testcases for Jakarta EE 8, can we just get these excluded for now? We will have to do the real fix post Jakarta EE 8. Thanks.

[jansupol]

Can you please file the bug in the Jakarta EE TCK issue tracker? You can also create a PR to exclude the tests.

The test exclusion is needed to be done for:

• full JakartaEETCK
• JSON-B TCK

Note the name of the tests. For full Jakarta EE TCK, 4 vehicles are used:

• _from_appclient
• _from_ejb
• _from_jsp
• _from_servlet

The JSON-B uses one vehicle:

• _from_standalone or _from_servlet

See the example for JakartaEE TCK

[Tomas-Kraus]

Clallenge evaluation:

JSON-B Spec 1.0, chapter 3.16: Big numbers

JSON Binding implementation MUST serialize/deserialize numbers that express greater magnitude or precision than an IEEE 754 double precision number as strings.

Challenged test

Decimal number 0.10000000000000001 is expected to be serialized as String

Does 0.10000000000000001 number fit into IEEE 754 double precision?

IEEE 754 double precision number is 64 bits long format where significan part has 53 bits and exponent has 11 bits.
Number 0.10000000000000001 converted to binary format needs much more. Here are first 1024 bits of the conversion: 0.0001100110011001100110011001100110011001100110011001101001010010000100010100001111001011110100000011111001001101111000101010001101011000100110000101001110010100011011100010000101011100100110010010011110001001101011110001011101010011001001101101000011101001011100001010100100000110101001001001000111110011101110111101111100011001101010001010010111111111101110010111001010011011100111100000001000010000101000011011100001100010010100011011111000101100111011110111001011010110001001100110010010010011111110000111110100010000011110000101101100110000110000000011000010001111011110001100010000011101000000111010001001001110110100000011001100110110011011011100001000000111110101110110101001110001110110111110010100000001110001011000011011111110100000100001100101010010001011101000011101001001101101101101000001011101010101110100010110010011001100000001100001010001011011101110111110001000110101000111100011111100010111011111101000100101111100110100001001101100011110111110001011001010010101100100110000111101011101111110011100111011
Looks like this number may even have infinite number of decimal places in binary form.

Answer is: number 0.10000000000000001 does not fit into IEEE 754 double precision without loss of precision.

Conclusion

This TCK test works properly. According to JSR 367 v1.0 number 0.10000000000000001 must be serialized as String.

[Tomas-Kraus]

The thing with "fits IEEE754" in the spec is that it lacks definition. Any of the floating point decimal values can be converted to IEEE754 double precision but most of them would be inprecise after conversion. For example a decimal value of 0.1

In my opinion it should be also excluded from the next release of the spec.

Specification defines this good enough, see chapter 3.16: Big numbers. It also explains what does "fit into" mean here: Number can be stored into EEE 754 double precision without loss of precision.

Currently this TCK test is consistent with spec and I don't see any reason to remove it. The only solution I see here is to change spec and define number storage mapping in another way.

[kwsutter]

I think it was premature to close this issue. Per the Jakarta TCK Process, there should be more discussion with the JSON-B API Specification team before deciding on a resolution. Technically, since the Jakarta TCKs are not final yet, we really can't be challenging any testcases. But, we (IBM) ran into this same issue when we were testing Java EE 8. This issue was created to document that problem and get it resolved post Jakarta EE 8. In the mean time, application servers that are trying to be Jakarta EE 8 compatible should be allowed to exclude this test. Other posts to this issue seem to support that position as well.

[rmannibucau]

Think #180 (comment) is accurate, i.e. currently the test is valid but it will need to be refined for next specification. In the meantime, implementations can use JsonbConfig#properties to disable that behavior when desired.

[aguibert]

While I agree with @Tomas-Kraus citations of the spec, we should also keep in mind that this particular issue has been discussed and agreed on by committers from both JSON-B implementations, and will become the new behavior in the next version of JSON-B.

[m0mus]

I agree that we should exclude this test. I disagree that this functionality must be a part of JSONP as it's proposed in #180 (comment). JSONP is a pure parser and shouldn't contain any special conversion/binding logic..

In JSONB all numbers by default must be serialized as JSON Numbers. Big numbers chapter [3.16] must be removed from JSONB spec. To give users an ability to deal with this issue without implementing custom logic in their application we can come out with @JsonbAsString annotation which forces JSONB to serialize an annotated number as string.

[rmannibucau]

@m0mus the feature, if any, belongs to jsonp and not jsonb - purely on the ownership side due to the api responsability. This is what the comment was about and I think we all agreed we dont want jsonb to build concurrent rules to jsonp for the same types.
Now solution to this issue is not yet released but seems everyone agreed to drop this rule and even provided workarounds in impls so if there is a strong agreement and user reject we can probably just make a quick maintenance release of the spec (pdf) dropping that part. Would likely be a 1.0.1? Wdyt @m0mus?

[bravehorsie]

@Tomas-Kraus

Number 0.10000000000000001 converted to binary format needs much more. Here are first 1024 bits of the conversion:
...
Looks like this number may even have infinite number of decimal places in binary form.
Answer is: number 0.10000000000000001 does not fit into IEEE 754 double precision without loss of precision.

Correct, decimal number "0.10000000000000001" cannot be converted to IEEE 754 without loosing precision. But guess what? Neither can be decimal number "0.1". It results in similar infinite loop of binary fractions as "0.10000000000000001" does in your example. In fact most of the decimal numbers besides those obvious like (0.5, 0.25, 0.125) and so on will result in infinite binary fractions. It is similar to dividing 1/3 in decimal however in binary it occur much more often. After trimming such binary fraction result to specific number of bits such as for example 64 or 32 the precision is lost.

Considering the fact above and your spec explanation it sounds like the spec also states that 0.1 and all other numbers which cannot be converted to IEEE 754 double precision without loosing precision should be written to JSON as string values. Is that what is intended in 3.16 - Big Numbers?

[Tomas-Kraus]

@bravehorsie Yes, current specification states that decimal number 0.1 should be written to JSON as string values. Welcome to the real world. :)

[rmannibucau]

Yes, current specification states that decimal number 0.1 should be written to JSON as string values. Welcome to the real world. :)

This is actually wrong for three reasons:

1. the link between IEEE754 and the JVM representation is not stated in the spec so 3.16 part is quite undefined at the moment,
2. 3.3.2 and 3.3.4 define other rules for numbers,
3. TCK tests rounds trip for numbers work even when they are outside IEEE754 space which is opposed to the test of that issue.

Therefore IEEE754 tests are inaccurate in spec v1.0

side note: since 3.16 will likely be dropped, there is no reason to keep broken tests IMHO.

[Tomas-Kraus]

Anyway, many people here would like to change serialization rules for numbers and remove IEEE 754 double precision conditions. It makes sense to mee too. We shall consider several documents in this process:

• RFC 8259
  Chapter 6: Numbers
  It does not define any limits to the number in the document. There is just an option that implementation can define limits like IEEE 754 binary64.
• JSR 374 - defined by JSON-P javadoc
  https://static.javadoc.io/javax.json/javax.json-api/1.1.4/index.html?overview-summary.html
  In our case important class is JsonParser for deserialization. It has getBigDecimal​() so it shall return RFC 8259 number of any size from the JSON input stream.
  For serialization, JsonNumber Json.createValue​(BigDecimal value) gives us RFC 8259 number of any size and builders/JsonWriter allows to write it into JSON outout stream.

There is not any restriction to store any number of any size as RFC 8259 number and get it back.
I agree with @m0mus proposal of the spec change.

@rmannibucau Yes, if we agree that this part of the spec shall be changed, we can ignore IEEE 754 related tests.

[Tomas-Kraus]

@m0mus the feature, if any, belongs to jsonp and not jsonb - purely on the ownership side due to the api responsability. This is what the comment was about and I think we all agreed we dont want jsonb to build concurrent rules to jsonp for the same types.

JSON-P is RFC 8259 parser. It provides tokens based on grammar defined there and shall not define anything else by default. Things discussed here (like IEEE 754 and whether to store number as number or string) are logic above that. It shall be defined and implemented in JSON-B spec/API.
Also JSON-B shall work with any default JSON-P API implementation.

[m0mus]

We should stop discussion here. We agreed to exclude this test and remove [3.16] from the next version of the spec.

@kwsutter @bshannon We have the first issue with exclude list in Jakarta EE. We need guidance how to solve it. I guess new version of TCK bundle should be produced and IBM will use it for certification. Am I right? Do we have the process written somewhere?

[kwsutter]

@kwsutter @bshannon We have the first issue with exclude list in Jakarta EE. We need guidance how to solve it. I guess new version of TCK bundle should be produced and IBM will use it for certification. Am I right? Do we have the process written somewhere?

@m0mus No changes to the TCK for Jakarta EE 8. At this point, it sounds like the workaround is to exclude this testcase. Compatible Implementations can do that as part of their testing for Jakarta EE 8. Post Jakarta EE 8, then a new revision of the TCK should be generated. Here's the link for the TCK Process.

[jansupol]

@kwsutter Can you please point to exclusions done by compatible implementation? I can only see that:

• A service TCK bundle "release which updates the exclude list and does not have test additions or changes to tests that affect behavior" can be performed.
• The TCK project maintains the exclude list.

Is it enough to update the exclude list from the TCK repository (after the PR with exclusion is merged there) in the TCK bundle downloaded from Eclipse download site?

[bshannon]

Right, we should follow the new TCK test challenge process after the completion of Jakarta EE 8.

[kwsutter]

@jansupol We're in a weird state right now since officially the TCKs are not final yet, so we can't do formal challenges. Under normal conditions, if this Issue was deemed "accepted" for the TCK challenge, then the process asks for an "immediate" release of a service release for the TCK with the proper exclude list. It may not be practical to do an immediate release for a variety of reasons. In that case, then documenting what the resolution would eventually entail should be sufficient for the Compatible Implementation to make adjustments for their execution of the tests. Whatever approach is selected would need to be documented in the Certification Request.