facebook-csharp-sdk / simple-json

JSON library for .NET 2.0+/SL4+/WP7/WindowsStore with optional support for dynamic and DataContract
MIT License
380 stars 143 forks source link

large long/int64 value being sent serialized incorrectly #7

Closed InsidiousForce closed 7 years ago

InsidiousForce commented 12 years ago

I'm building up an ad posting, which has an object_id in it. If I serialize the structure with newtonsoft's serializer, I get:

{"campaign_id":6003891916512,"name":"test","status":1,"bid_type":1,"max_bid":150,"targeting":{"countries":["FR","DE","NL"],"radius":10,"age_min":21,"age_max":35,"broad_age":0,"genders":[1,2],"education_statuses":[0],"keywords":["Instyle Magazine"]},"creative":{"body":"la la la la la. Check out and become a fan.","type":2,"object_id":10150098461530576,"name":"testy","image_hash":"ba24509710d162fcd7df6157638900f5"}}

However, If I look at what the SDK is sending over the wire when I pass in the same structure, the object_id is sent as:

"object_id":1.01500984615306E+16

So we can no longer use the SDK on newer pages that have these larger ids. Using 5.4.1.0 of FacebookSDK.

InsidiousForce commented 12 years ago

line 1040/1041 of SimpleJson.cs

        else if (IsNumeric(value))
            success = SerializeNumber(Convert.ToDouble(value), builder);

that needs to deal with longs and not convert to doubles if they're longs. can someone fix?

prabirshrestha commented 12 years ago

fixed in v0.12

InsidiousForce commented 12 years ago

a double has 15-16 digits of precision per http://msdn.microsoft.com/en-us/library/678hzkk9(v=vs.71).aspx

a long can be up to 20 digits per http://msdn.microsoft.com/en-us/library/ctetwysk(v=vs.71).aspx

Therefore I believe this has not fixed the problem. I can't test, but I believe you will start seeing errors with lack of precision on larger longs.

prabirshrestha commented 12 years ago

You could try sending a pull request too.

i think we will need to change

protected static bool SerializeNumber(double number, StringBuilder builder)

to

protected static bool SerializeNumber(object number, StringBuilder builder)

so we can parse correctly depending on long or double.

InsidiousForce commented 12 years ago

integer data should never enter the realm of floating point. decimal in this case should also not be converted to double. if you're dealing with money you're in for a world of hurt.

http://docs.oracle.com/cd/E19957-01/806-3568/ncg_goldberg.html

here's a paper on why that is, but straight up, integer and decimal data shouldn't be converted to double precision floats at all. decimal should be used for money, int32/int64 (int/long) should be used for integers.

prabirshrestha commented 12 years ago

in JSON everything is a number, there is no decimal/dobule/long/int.

So if the number contains . we should use decimal and if it doesn't use long?

prabirshrestha commented 12 years ago

so now it doesn't convert everything to double. If it is long it will use long, if decimal will use decimal if double it will use double.

InsidiousForce commented 12 years ago

sounds good. regarding your information above with deserializing (wouldn't be relevant to serializing), I would say yes, if it has a . then it's a decimal otherwise a long and force the consumer to cast to something with less precision or size if they like.

i'm not sure how, say, json.net handles this, I know all their integers are longs, not sure about decimals and what they do with that because I work mainly with facebook api and even their money is integer (money * 100).

prabirshrestha commented 12 years ago

We currently use either long or double. (as for decimal it will have to be another another issue. This would be a breaking change. For now I will leave it as double. You can start a new issue about this with reasons.)

https://github.com/facebook-csharp-sdk/simple-json/blob/03a67d66e190b626a62cb9accb1b77fa3a4b5dd7/src/SimpleJson/SimpleJson.cs#L892-916