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

Allow deserialization of very large integers #874

Conversation

logiclrd
Copy link

@logiclrd logiclrd commented Sep 7, 2016

While I was working on PR #873, I was reading through the SimpleJson code and discovered that the way in which it parses integers might reject some syntactically-valid JSON documents. In particular, JSON allows for very large integer values to be specified (values large enough that a 64-bit integer cannot represent them) with no punctuation or exponentiation. These values would, in typical implementations, need to be represented as floating-point values (double), but because they lack periods and e signifiers, the SimpleJson code will only attempt to parse them as long. This means that the (valid) JSON string produced by (long.MaxValue + 1M).ToString() cannot be parsed. This PR addresses the issue (using decimal and then double as fallbacks where long fails), and adds a corresponding unit test.

…epeatedly building a System.String of the number substring.

Reworked the integer parsing path in RestSharp/SimpleJson.cs to try decimal and double parsers if long fails. This allows numbers such as long.MaxValue + 1M to be parsed (yielding a decimal instead of a long). Numbers too large for System.Decimal become System.Double (at, of course, a loss of precision).
Added test Can_Deserialize_Very_Large_Integers to RestSharp.Tests/JsonTests.cs.
@behoyh
Copy link

behoyh commented Oct 6, 2016

SimpleJson is a dependency, we can't modify it.

@logiclrd
Copy link
Author

logiclrd commented Oct 6, 2016

Ahh, okay. Where does it live? I can try pushing this patch upstream. :-)

@logiclrd
Copy link
Author

logiclrd commented Oct 6, 2016

I believe I have found it: https://github.com/facebook-csharp-sdk/simple-json

I'll create a PR there. Looks like SimpleJson.cs is brought in via NuGet? Kind of strange, but I assume that means if my PR is accepted, then I can come back to RestSharp and create a PR consisting of updating the NuGet reference?

@logiclrd
Copy link
Author

logiclrd commented Oct 6, 2016

@logiclrd logiclrd closed this Oct 6, 2016
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.

2 participants