mongodb / specifications

Specifications related to MongoDB
http://specifications.readthedocs.io/en/latest
Other
389 stars 242 forks source link

DRIVERS-2651 Add decimal128 clamped zeros tests with very large exponents. #1432

Closed matthewdale closed 1 year ago

matthewdale commented 1 year ago

DRIVERS-2651

Add decimal128 Extended JSON parse tests for clamped zeros with very large positive and negative exponents to catch any issues in iterative clamping logic.

Please complete the following before merging:

matthewdale commented 1 year ago

@jyemin that's really interesting! We could add these cases as expected parse errors instead of valid "degenerate_extjson". I'll check if there's an existing trend among drivers.

jmikola commented 1 year ago

Created https://github.com/mongodb/mongo-php-driver/pull/1433 to POC this using the PHP driver, which effectively tests libbson from the C driver.

There's no infinite loop (as mentioned in DRIVERS-2651), but I do see failures converting the degenerate extJSON to Canonical BSON. The PHP corpus tests exercise two code paths (one uses a BSON object as an intermediary representation) but this is really just one failure.

In the diffs below, the - segment is expected output and + is actual.


{
    "description": "clamped zeros with a large negative exponent",
    "canonical_bson": "180000001364000000000000000000000000000000000000",
    "degenerate_extjson": "{\"d\" : {\"$numberDecimal\" : \"0E-999999999999\"}}",
    "canonical_extjson": "{\"d\" : {\"$numberDecimal\" : \"0E-6176\"}}"
}
 // Degenerate extJSON -> Canonical BSON
-180000001364000000000000000000000000000000000000
+180000001364000000000000000000000000000000fe5f00
 // Degenerate extJSON -> BSON object -> Canonical BSON
-180000001364000000000000000000000000000000000000
+180000001364000000000000000000000000000000fe5f00

{
    "description": "clamped zeros with a large positive exponent",
    "canonical_bson": "180000001364000000000000000000000000000000FE5F00",
    "degenerate_extjson": "{\"d\" : {\"$numberDecimal\" : \"0E+999999999999\"}}",
    "canonical_extjson": "{\"d\" : {\"$numberDecimal\" : \"0E+6111\"}}"
}
 // Degenerate extJSON -> Canonical BSON
-180000001364000000000000000000000000000000fe5f00
+180000001364000000000000000000000000000000000000
 // Degenerate extJSON -> BSON object -> Canonical BSON
-180000001364000000000000000000000000000000fe5f00
+180000001364000000000000000000000000000000000000

{
    "description": "clamped negative zeros with a large negative exponent",
    "canonical_bson": "180000001364000000000000000000000000000000008000",
    "degenerate_extjson": "{\"d\" : {\"$numberDecimal\" : \"-0E-999999999999\"}}",
    "canonical_extjson": "{\"d\" : {\"$numberDecimal\" : \"-0E-6176\"}}"
}
 // Degenerate extJSON -> Canonical BSON
-180000001364000000000000000000000000000000008000
+180000001364000000000000000000000000000000fedf00
 // Degenerate extJSON -> BSON object -> Canonical BSON
-180000001364000000000000000000000000000000008000
+180000001364000000000000000000000000000000fedf00

{
    "description": "clamped negative zeros with a large positive exponent",
    "canonical_bson": "180000001364000000000000000000000000000000FEDF00",
    "degenerate_extjson": "{\"d\" : {\"$numberDecimal\" : \"-0E+999999999999\"}}",
    "canonical_extjson": "{\"d\" : {\"$numberDecimal\" : \"-0E+6111\"}}"
}
 // Degenerate extJSON -> Canonical BSON
-180000001364000000000000000000000000000000fedf00
+180000001364000000000000000000000000000000008000
 // Degenerate extJSON -> BSON object -> Canonical BSON
-180000001364000000000000000000000000000000fedf00
+180000001364000000000000000000000000000000008000
matthewdale commented 1 year ago

@jyemin doing some quick investigation into that exception message, it seems possible that the Java BigDecimal parser will throw an exception if the exponent has more than 10 digits. I've updated the tests to use a number with exactly 10 digits, which should hopefully pass the Java tests now.

matthewdale commented 1 year ago

@jyemin Hah, more validation! I've updated the test cases to use max/min int32 values for the exponents. It still triggers the problem in the old Go driver and hopefully passes the Java tests now.

matthewdale commented 1 year ago

@jyemin good suggestion. I adjusted the negative exponent by +1.

jmikola commented 1 year ago

From https://github.com/mongodb/specifications/pull/1432#issuecomment-1593893814

I've updated the tests to use a number with exactly 10 digits, which should hopefully pass the Java tests now.

From https://github.com/mongodb/specifications/pull/1432#issuecomment-1594012348

I've updated the test cases to use max/min int32 values for the exponents. It still triggers the problem in the old Go driver and hopefully passes the Java tests now.

@matthewdale: It looks like these changes resolved the previous failures I observed in libbson (via PHPC).