Skip to content
This repository has been archived by the owner on Aug 15, 2021. It is now read-only.

Value i128 serializer breaks cbor <=> json Value to Value conversions #140

Open
rslife opened this issue Aug 4, 2019 · 7 comments
Open

Comments

@rslife
Copy link

rslife commented Aug 4, 2019

Hello.

Starting with 8cfcea9, direct conversions between cbor Values and json Values is no longer possible.

I fully understand the rationale behind using one integer type to cover the full integer value range supported by cbor. But I wonder what are your thoughts on bringing back, what was admittedly accidental, support for this type of conversion.

@pyfisch
Copy link
Owner

pyfisch commented Aug 5, 2019

Hello,

that is unfortuate. Can you post a complete example with code of a failed conversion?
I'd like to bring it back, but it may be easier to change serde-json to accept all number types.

@rslife
Copy link
Author

rslife commented Aug 7, 2019

use serde::{Serialize, Deserialize};
use serde_cbor::value::Value as CborValue;

#[derive(Deserialize, Serialize)]
struct Foo { x: i32 }

fn main() {
    let foo = Foo { x: -13 };

    let cbor_bytes = serde_cbor::ser::to_vec(&foo)
        .expect("failed to serialize to cbor bytes");
    let cbor_val: CborValue = serde_cbor::from_slice(&*cbor_bytes)
        .expect("failed to deserialize to cbor Value");

    println!("{:?}", cbor_val);

    let json_val = serde_json::to_value(cbor_val)
        .expect("failed conversion");

    println!("{:?}", json_val);
}

This obviously happens whenever integer values of any type (not just i32) are involved.

@pyfisch
Copy link
Owner

pyfisch commented Sep 7, 2019

Thanks for the example code. Would you mind opening an issue for serde-rs/json and ask them to accept i128 integers for values if they are in the 64-bit range?

@pyfisch
Copy link
Owner

pyfisch commented Nov 18, 2019

Would it help if serde-cbor Values emit an u64 for positive numbers and i64 for negative number whenever possible?

@rslife
Copy link
Author

rslife commented Nov 18, 2019

Would it help if serde-cbor Values emit an u64 for positive numbers and i64 for negative number whenever possible?

Probably yes.

@pyfisch pyfisch added this to the v0.11 milestone Nov 21, 2019
@pyfisch
Copy link
Owner

pyfisch commented Nov 21, 2019

@rslife would you like to submit a PR?

@rslife
Copy link
Author

rslife commented Nov 24, 2019

I will send a PR from my home account later.

MoSal added a commit to MoSal/cbor that referenced this issue Nov 24, 2019
 UnsignedInteger(u64): for all non-negative values

 SignedInteger(i64): for negative values that fit in a i64.

 LargeSignedInteger(i128): for negative values that can't be represented
 by i64.

 This should fix pyfisch#140.

Signed-off-by: Mohammad AlSaleh <[email protected]>
MoSal added a commit to MoSal/cbor that referenced this issue Nov 24, 2019
 UnsignedInteger(u64): for all non-negative values

 SignedInteger(i64): for negative values that fit in a i64.

 LargeSignedInteger(i128): for negative values that can't be represented
 by i64.

 This should fix pyfisch#140.

Signed-off-by: Mohammad AlSaleh <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants