search

bignerdranch / Freddy

A reusable framework for parsing JSON in Swift.
MIT License
1.09k stars 120 forks source link

Fix bug where long Float numbers are parsed as JSON.String instead of JSON.Double #243

Closed dkokotov closed 7 years ago

dkokotov commented 7 years ago

The bug occurs on 32-bit platforms. This is because the parser treats numbers in the JSON data as Integers until encountering the decimal point, and using a Int to incrementally store them; on 32-bit platforms Int is 32-bit and thus it's easy to overflow while parsing a long floating point number. The current behavior is then to parse such numbers as JSON.String.

This patch fixes it in a quick-and-dirty manner by simply doing a final pass at the end to try and parse the string containing the number as a double in such a case.

dkokotov commented 7 years ago

This was meant to be created on my fork of this repo. I do intend to open an issue for this behavior.

lyricsboy commented 7 years ago

Take a look at https://github.com/bignerdranch/Freddy/issues/76 for motivation and reference on the reasoning behind the current implementation.

dkokotov commented 7 years ago

@lyricsboy I read the discussion, and while I don't necessarily agree (I dont see the big deal in using IntMax for integers), I can appreciate the reasoning.

However, this case is different - the overflow happens as part of parsing a floating point number, not an integer, and only happens as a side effect of how the parser happens to be implemented (parsing the number one character at a time, and using a Int to store the accumulating value). IMO this is just a bug.

My fix here is the simplest thing to do - at the end of the overflow fallback handler, just retry parsing the number.

A possibly cleaner fix is to use an Int64 to hold the temporary accumulating value while parsing. If the final number ends up being an integer, then we can convert this to a plain Int and handle the overflow as currently. If it ends up being floating-point, then it should not overflow, solving my problem.

FWIW, I dont think this is 100% correct still, as there would still be valid (representible) floating point numbers whose pre-decimal component would overflow a 64-bit integer, but they would be much much rarer and unlikely to happen in real-world. At the very least the behavior would be consistent between 32 and 64 bit platforms.

lyricsboy commented 7 years ago

Ah, thanks for the explanation. I clearly didn't look closely enough :)