Add support for serde_json's arbitrary precision

[tenuous-guidance]

Resolves #113 and #117

[tenuous-guidance]

I think we had a limit of serde_json = "<1.0.101" due to incompatibilities with minimum-supported-rust-version 1.43. If that's an issue we'll merge this into an 0.5.0 branch. I'm trying not to stray too far from the num_ crates' MSRV of 1.31.

I thought I'd give it a shot, but no dice. We need at least .108 and no more than .100. How do you want to proceed?

[probablykasper]

Hey, any word on this one @akubera?

[probablykasper]

Is there any simple way to make it work with other types that use BigDecimal, like Option<BigDecimal>?

[tenuous-guidance]

Is there any simple way to make it work with other types that use BigDecimal, like Option<BigDecimal>?

I added Option<_> support in the MR, unfortunately the solution has to be manual for each wrapper we want to support, from what I understand of how serde works here.

[akubera]

Ok, I'm working on this again. Going to split the first commit into move-existing-serde-to-impl-file, (I'll macro-tize the test) then apply your others and work something out.

[akubera]

I found a way to have my cake and eat it too. By aliasing the old version of serde_json for just tests, we can simultaneously support both versions: serde_json_testing restricted to version supported by all versions of Rust, and serde_json for anything "modern" that actually requires it (serde_json is just used for testing otherwise). So now if the serde_json feature is enabled, the tests ignore serde_json_testing and test against the proper version. Otherwise the testing version is used in the tests.

There's a little macro-magic to duplicate the implementation of the traits between the testing and non-testing versions.

I don't know if anyone is actually using BigDecimal with Rust versions as old as 1.43, but as long as num-bigint supports 1.31 I want to keep as much backwards support for as long as possible.

I'm working on this in the feature/serde-arbprec branch.

[akubera]

I don't like the name of the feature "arbitrary-precision", it only affects serialization and should reflect that. Otherwise it sounds like the library is fixed-precision unless otherwise enabled.

Maybe json-arbitrary-precision, to hint that it's a feature passed to serde-json?

Also should investigate float_roundtrip

serde_json features : https://docs.rs/crate/serde_json/latest/features

[akubera]

Other option would be... just don't test with serde_json on versions older than they support.

which they CLAIM is 1.36, but they only test back to 1.56.

[akubera]

Going further and replacing serde_json with serde_test, which probably should have been used from the start. This gives us access to the Tokens serde uses, and we won't rely on json types directly.

This has the same problem with the dependency requiring the 2021 edition of Rust, which isn't supported on versions of Rust older than 1.56, so I think I'll have to keep my hack around, but we won't have to compile serde_json twice.

[akubera]

Okay. The serde_json tests have been removed and replaced with similar implementations using serde_test, which supported 1.43.1 until the most recent version.

To simplify things I'm going to say arbitrary-precision doesn't work on 1.43, and I'm quite sure nobody will have a problem with that.

I thank you for trying to get it to work. I know it can be frustrating.

I looked around and the rust-decimal crate has a similar feature they named serde-with-arbitrary-precision. I like that; it's very clear. Much better than our "string-only" feature which I never got around to understanding.

It's too late for me to work on this anymore. I'll push new feature/serde-arbprec soon, rebased onto the new trunk.

[akubera]

I'm turning my attention to this one again, now that the formatting issues are resolved. I'm currently rebasing the arbitrary_precision branch onto trunk and will proceed from there.

Significantly, the serialization tests have been switched from serde_json to the more appropriate serde_test, so hopefully that will allow more straightforward inputs and outputs (no json strings of floats in json structs in a Rust string).

[tenuous-guidance]

arbitrary-precision is explicitly a serde-json feature. If we want to test our compatibility with the feature (which is the closest there is to a spec for it), then we need to test using serde-json.

[akubera]

You're right. If serde-json is the thing "generating" the tokens, trying to bypass that isn't going to help us here.

Regardless, arbitrary-precision feature includes the serde_json feature, so we're able to use it in tests.

I'm seeing an error from this test, for the case when serde_json is enabled and arbitrary_precision is not enabled:

    #[test]
    #[cfg(not(feature = "arbitrary-precision"))]
    fn test_normal_precision() {
        let json = r#"0.1"#;
        let expected = BigDecimal::from_str("0.1").expect("should parse 0.1 as BigDecimal");
        let deser: BigDecimal = serde_json::from_str(json).expect("should parse JSON");

        // 0.1 is directly representable in `BigDecimal`, but not `f64` so the default deserialization fails.
        assert_ne!(expected, deser);
    }

$ cargo test --features=serde,serde_json test_normal_precision 
running 1 test
test impl_serde::test::test_normal_precision ... FAILED

failures:

---- impl_serde::test::test_normal_precision stdout ----
thread 'impl_serde::test::test_normal_precision' panicked at src/impl_serde.rs:276:60:
should parse JSON: Error("invalid type: map, expected a number or formatted decimal string", line: 1, column: 3)
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

I don't understand why json 0.1 is being parsed as a map.

[akubera]

But getting to what matters, this test-suite works, and I think that's the actual use case here:

    #[cfg(feature = "arbitrary-precision")]
    mod serde_struct_decimals {
        use super::*;
        use crate as bigdecimal;
        use serde::{Serialize, Deserialize};

        #[derive(Serialize, Deserialize)]
        pub struct TestExample {
            #[serde(with = "bigdecimal::serde_arbitrary_precision")]
            value: BigDecimal,
        }

        macro_rules! impl_case {
            ($name:ident : $input:expr => (error)) => {
                #[test]
                fn $name() {
                    let res = serde_json::from_str::<TestExample>($input);
                    assert!(res.is_err());
                }
            };
            ($name:ident : $input:expr => $expected:expr) => {
                #[test]
                fn $name() {
                    let obj: TestExample = serde_json::from_str($input).unwrap();
                    let expected: BigDecimal = $expected.parse().unwrap();

                    assert_eq!(expected, obj.value);

                    // expect output to be input with all spaces removed
                    let s = serde_json::to_string(&obj).unwrap();
                    let input_stripped = $input.replace(" ", "");

                    assert_eq!(&s, &input_stripped);                }
            };
        }

        impl_case!(case_1d0: r#"{ "value": 1.0 }"# => "1.0" );
        impl_case!(case_2d01: r#"{ "value": 2.01 }"# => "2.01" );
        impl_case!(case_high_prec: r#"{ "value": 64771126779.35857825871133263810255301911 }"# => "64771126779.35857825871133263810255301911" );

        impl_case!(case_nan: r#"{ "value": nan }"# => (error) );
    }

[akubera]

Would it be more or less confusing if we dropped arbitrary-precision and just used the serde_json feature/optional-dependency to provide the serialize/deserialize routines?

[akubera]

And along with naming things, I think the path to the serializer modules can be improved (certainly from the direct bigdecimal::impl_serde, we definitely dont want to do that), but I'd like to know if there's a convention in the community where to define such things.

I think right now it looks like this if we expose that module directly:

        #[derive(Serialize, Deserialize)]
        pub struct TestExample {
            #[serde(with = "bigdecimal::arbitrary_precision")]
            value: BigDecimal,
        }

But that name again feels redundant.

I'm thinking of namespacing the

        #[derive(Serialize, Deserialize)]
        pub struct TestExample {
            #[serde(with = "bigdecimal::serde::json_num")]
            value: BigDecimal,
            // explicitly map options
            #[serde(with = "bigdecimal::serde::json_num_option")]
            maybe_value: Option<BigDecimal>,

            // would it be nice to do one for Vec, too? 
            #[serde(with = "bigdecimal::serde::json_num_list")]
            maybe_value: Vec<BigDecimal>,
       }

bigdecimal::serde_json::num is an option, too.

[akubera]

Current state of things on my branch:

trunk...arbitrary_precision

[tenuous-guidance]

I don't understand why json 0.1 is being parsed as a map

The way arbitrary precision works inside serde-json is it puts the String corresponding to the number into a serde map with a magic key.

[tenuous-guidance]

https://github.com/serde-rs/json/blob/f346308cda4cd5a7dae0327a757fe41f064a2161/src/number.rs#L369-L395