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

Fix problems deserializing Bson long into int #913

Merged
merged 2 commits into from Dec 11, 2014

Conversation

Projects
None yet
3 participants
@dmonagle
Contributor

dmonagle commented Dec 4, 2014

Adds the ability to deserialize a Bson long into a native int. This could be problematic for numbers outside the range of int but it should probably be checked by the developer if they are taking Bson input. This caused quite a headache when we have data coming from a Json api and wanted to merge with the Bson from MongoDB before deserializing.

This code previously did not work:

    int dest;

    auto jsonSource = Json(2048); // Imagine this came from a web request
    assert(jsonSource.type == Json.Type.int_); // The Json type is int

    auto source = Bson.fromJson(jsonSource); // We could do this so we can merge with a Bson object loaded from the database
    assert(source.type == Bson.Type.long_); // The Bson object now has a long

    dest.deserializeBson(source); // This caused an error as it was conversion from long to int
Fix problems deserializing Bson long into int
Adds the ability to deserialize a Bson long into a native int. This could be problematic for numbers outside the range of int but it should probably be checked by the developer if they are taking Bson input. This caused quite a headache when we have data coming from a Json api and wanted to merge with the Bson from MongoDB before deserializing.

This code previously did not work:

```D
	int dest;

	auto jsonSource = Json(2048); // Imagine this came from a web request
	assert(jsonSource.type == Json.Type.int_); // The Json type is int

	auto source = Bson.fromJson(jsonSource); // We could do this so we can merge with a Bson object loaded from the database
	assert(source.type == Bson.Type.long_); // The Bson object now has a long

	dest.deserializeBson(source); // This caused an error as there is was conversion from long to int

```
@Geod24

This comment has been minimized.

Show comment
Hide comment
@Geod24

Geod24 Dec 5, 2014

Contributor

I think you should check if the long is in an int boundary before returning a (possibly) invalid value to the user.

Contributor

Geod24 commented Dec 5, 2014

I think you should check if the long is in an int boundary before returning a (possibly) invalid value to the user.

@dmonagle

This comment has been minimized.

Show comment
Hide comment
@dmonagle

dmonagle Dec 5, 2014

Contributor

@Geod24 I've updated the code to add an assertion for that situation. Frankly I'm torn as to whether the serializer should be checking this. You mention returning an invalid value to the user, but realistically a developer should be using long if there is any chance that the data is going to go outside of int range. We do our range checking in the controllers, well before the point of attempting to coerce it into a native variable.

I've only patched this because it was becoming seriously problematic for us and we are already looking forward with the std_data_json library and figure that the bson library will be replaced in a similar fashion in the near(ish) future.

Contributor

dmonagle commented Dec 5, 2014

@Geod24 I've updated the code to add an assertion for that situation. Frankly I'm torn as to whether the serializer should be checking this. You mention returning an invalid value to the user, but realistically a developer should be using long if there is any chance that the data is going to go outside of int range. We do our range checking in the controllers, well before the point of attempting to coerce it into a native variable.

I've only patched this because it was becoming seriously problematic for us and we are already looking forward with the std_data_json library and figure that the bson library will be replaced in a similar fashion in the near(ish) future.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Dec 11, 2014

Member

Throwing an exception here I think makes sense, since the developer usually otherwise has no way to do the same check (other than manually deserializing the data a second time).

Member

s-ludwig commented Dec 11, 2014

Throwing an exception here I think makes sense, since the developer usually otherwise has no way to do the same check (other than manually deserializing the data a second time).

s-ludwig added a commit that referenced this pull request Dec 11, 2014

Merge pull request #913 from intrica/fix-bson-deserialize-long
Fix problems deserializing Bson long into int

@s-ludwig s-ludwig merged commit 596c4de into vibe-d:master Dec 11, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment