Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disallow serde serialize/deserialize unsigned integer types #72

Merged
merged 3 commits into from
May 30, 2017

Conversation

zonyitoo
Copy link
Contributor

  • Disallow u16, u32 and u64 to be serialize to / deserialize from BSON

@zonyitoo zonyitoo self-assigned this May 22, 2017
@zonyitoo zonyitoo requested a review from kyeah May 22, 2017 14:15
@kyeah
Copy link
Contributor

kyeah commented May 26, 2017

Should we replace these with functions that users can refer to, so people can still keep the original functionality if necessary?

// something like this -- pseudocode!

#[inline]
fn serialize_u64_into_f64(serializer: Encoder, value: u64) -> EncoderResult<Bson> {
    serializer.serialize_f64(value as f64)
}

#[derive(Serialize, Debug)]
struct Thing {
    #[serde(serialize_with = "bson::encoder::serialize_u64_into_f64")]
    s: u64,
}

reference

@zonyitoo
Copy link
Contributor Author

Hmm, maybe. But I don't think it is correct to serialize u64 to f64.

It is reasonable to do this for compatibility.

@zonyitoo
Copy link
Contributor Author

Added backward compatibility support mod u2f

#[test]
fn test_compat_u2f() {
    #[derive(Serialize, Deserialize, Eq, PartialEq, Debug)]
    struct Foo {
        #[serde(with = "bson::compat::u2f")]
        x: u32
    }

    let foo = Foo { x: 20 };
    let b = bson::to_bson(&foo).unwrap();
    assert_eq!(b, Bson::Document(doc! { "x" => (Bson::FloatingPoint(20.0)) }));

    let de_foo = bson::from_bson::<Foo>(b).unwrap();
    assert_eq!(de_foo, foo);
}

Copy link
Contributor

@kyeah kyeah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice, LGTM. unfortunate that we don't have a changelog to document this 😅 Maybe we should add a section in the README to document the fact that mongodb doesn't support unsigned integers, and this module is provided for serialization compatibility.

@zonyitoo
Copy link
Contributor Author

@kyeah How about this?

Copy link
Contributor

@kyeah kyeah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some small grammatical comments but looks good otherwise. Thanks for adding it!

README.md Outdated

## Breaking Changes

In BSON specification, it doesn't support any kind of _unsigned integer types_, for example, `u32`. In the older version of this crate (< `v0.8.0`), if you uses `serde` to serialize _unsigned integer types_ into BSON, it will store them with `Bson::FloatingPoint` type. From `v0.8.0`, we removed this behavior and simply returned an error when you want to serialize _unsigned integer types_ to BSON. [#72](https://github.com/zonyitoo/bson-rs/pull/72)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"In the BSON specification, unsigned integer types are unsupported; for example"...

README.md Outdated

In BSON specification, it doesn't support any kind of _unsigned integer types_, for example, `u32`. In the older version of this crate (< `v0.8.0`), if you uses `serde` to serialize _unsigned integer types_ into BSON, it will store them with `Bson::FloatingPoint` type. From `v0.8.0`, we removed this behavior and simply returned an error when you want to serialize _unsigned integer types_ to BSON. [#72](https://github.com/zonyitoo/bson-rs/pull/72)

For backward compatibility, we provided a mod `bson::compat::u2f` to explicitly serialize _unsigned integer types_ into BSON's floating point value as follows:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we => we've

README.md Outdated
}
```

In this example, we added an attribute `#[serde(with = "bson::compat::u2f")]` on field `x`, which will tell `serde` to uses the `bson::compat::u2f::serialize` and `bson::compat::u2f::deserialize` to process this field.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uses => use

"the bson::compat::u2f::serialize and bson::compat::u2f::deserialize methods"

@zonyitoo
Copy link
Contributor Author

Already done.

@kyeah
Copy link
Contributor

kyeah commented May 30, 2017

awesome, LGTM!

@zonyitoo zonyitoo merged commit 8a1b94a into master May 30, 2017
@zonyitoo zonyitoo deleted the fix-unsigned-serde branch May 30, 2017 15:58
@zonyitoo
Copy link
Contributor Author

zonyitoo commented May 30, 2017

Already published to crates.io.

lrlna pushed a commit to lrlna/bson-rs that referenced this pull request Feb 27, 2019
* [mongodb#70] Disallow serde serialize/deserialize unsigned integer types

* add compat mod for backward compatibility

* Added breaking changes in README
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants