-
Notifications
You must be signed in to change notification settings - Fork 1.2k
[SR-7054] JSONDecoder Decimal precision error #4255
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
Comments
cc @itaiferber |
This is a known issue that's been discussed at length in the past — I'll try to dig up some prior discussion about this. The core of the matter is that the implementations of At the You can see this with the following ObjC code: #import <Foundation/Foundation.h>
NS_FORMAT_FUNCTION(1, 2)
static void print(NSString *format, ...) {
va_list arguments;
va_start(arguments, format);
puts([[NSString alloc] initWithFormat:format arguments:arguments].UTF8String);
va_end(arguments);
}
int main(int argc, char *argv[]) {
@autoreleasepool {
double d = 46.984765;
print(@"%.*f\n", DBL_DECIMAL_DIG, d); // => 46.98476500000000300
NSDecimalNumber *number = [NSDecimalNumber decimalNumberWithString:@"46.984765"];
print(@"%@", number); // => 46.984765
print(@"%.*f\n", DBL_DECIMAL_DIG, number.doubleValue); // => 46.98476499999999589
number = [NSDecimalNumber numberWithDouble:d];
print(@"%@", number); // => 46.98476500000001024
print(@"%.*f\n", DBL_DECIMAL_DIG, number.doubleValue); // => 46.98476500000002432
}
} All of these results are "correct" for some definition of "correct". If At the end of the day, we preferred to always accurately round-trip |
There are a few ways we could potentially address this, potentially requiring changes in
Each of these approaches have their tradeoffs that we'd need to weigh if we want to address this. |
Comment by Tibor Bödecs (JIRA) @itaiferber thank you for the deep explanation, I'm aware of the root cause of the issue. If I take the following example:
Both values are "correct", but from my point of view, however if I encode, decode, encode the second decimal number to JSON, the final value is going to be an "incorrect" (46.98476500000001024) decimal number value. That's clearly not the expected representation because of the underlying JSONSerialization problem. I would love to see a better solution, because the current behaviour of the coder classes can be misleading if you are working with decimals. Also it would be nice to save decimals to JSON as numbers instead of strings, because currently that's the only alternative to hold the original value. |
tiborbodecs (JIRA User) I agree on all points — this should be something that "just works" and it's unfortunate that we leak the implementation details here in this way. This is one of the many improvements to our implementations we'd like to make. |
Comment by Sergej Jaskiewicz (JIRA) @itaiferber I see the following in the _JSONDecoder current implementation: fileprivate func unbox(_ value: Any, as type: Decimal.Type) throws -> Decimal? {
guard !(value is NSNull) else { return nil }
// Attempt to bridge from NSDecimalNumber.
if let decimal = value as? Decimal {
return decimal
} else {
let doubleValue = try self.unbox(value, as: Double.self)!
return Decimal(doubleValue)
}
} If we change it to fileprivate func unbox(_ value: Any, as type: Decimal.Type) throws -> Decimal? {
guard !(value is NSNull) else { return nil }
// Attempt to bridge from NSDecimalNumber.
if let decimal = value as? Decimal {
return decimal
} else if let number = value as? NSNumber,
number !== kCFBooleanTrue,
number !== kCFBooleanFalse {
return number.decimalValue
} else {
let doubleValue = try self.unbox(value, as: Double.self)!
return Decimal(doubleValue)
}
} then everything is fine — the JSON data is decoded correctly. For example, this test from the test suite: lazy var decimalValues: [Decimal] = [
Decimal.leastFiniteMagnitude,
Decimal.greatestFiniteMagnitude,
Decimal.leastNormalMagnitude,
Decimal.leastNonzeroMagnitude,
Decimal(),
Decimal(string: "67.52")!, // This was added
// Decimal.pi does not round-trip at the moment.
// See rdar://problem/33165355
// Decimal.pi,
]
func test_Decimal_JSON() {
for decimal in decimalValues {
// Decimal encodes as a number in JSON and cannot be encoded at the top level.
expectRoundTripEqualityThroughJSON(for: TopLevelWrapper(decimal))
}
} was failing after I had added `Decimal(string: "67.52")!` to the `decimalValues` array. However, after the aforementioned fix, it passes and no other test fails. Or am I missing something? I would be happy to create a PR and write some tests for it. |
Comment by Tibor Bödecs (JIRA) broadway_lamb (JIRA User) sounds good, it'd be good to se a possible fix for this issue! 😉 |
broadway_lamb (JIRA User) I meant to respond to this earlier — this fix might work for some numbers, but not all: import Foundation
let decimal = Decimal(string: "46.984765000000003")!
print(decimal) // => 46.984765000000003
let number = NSNumber(value: 46.984765000000003)
print(number.stringValue) // => 46.984765
print(String(format: "%0.17f", number.doubleValue)) // => 46.98476500000000300
print(number.decimalValue == decimal) // => false
I think this gets us closer to the right answer, but is inefficient and still doesn't provide the correctness we need, unfortunately. |
Comment by Petr Dvorak (JIRA) Hello @itaiferber, I was briefly looking at the source code (I admit that building Swift is more than I can currently chew - you guys are doing a great job!) and I was wondering: Why the [Decimal type uses the Double conversion for unboxing|https://github.com/apple/swift/blob/master/stdlib/public/SDK/Foundation/JSONEncoder.swift#L2417] and not String? Is this for the performance reason (and if yes, is the performance hit so unacceptable in typical scenarios)? Solving this issue would help us a lot. We have APIs that send correct double values in JSON and all our other projects (usually built in Java, leveraging the veteran JSON mapper Jackson) work correctly (they map the JSON value to BigDecimal via String representation). We only encounter issues with Swift... |
petrdvorak (JIRA User) I think that my original comment on this explains things best — specifically, that there is unfortunately no correct answer here because In a perfect world, my preferred solution would be to truly use a /cc @stephentyrone for some visibility on this in light of some of our previous discussions. |
Comment by Petr Dvorak (JIRA) @itaiferber Thank you for the comment - I completely forgot there is no To provide yet another hint of possible direction... Maybe having some "hooks" - alongside the Jackson Streaming API would allow for a more precise (de)serialization? (EDIT: I am not suggesting making a streaming API - instead, I would like to have the callback when reading attributes so that I have some control over the writing / parsing...) |
petrdvorak (JIRA User) That's certainly another way of solving the issue that we've considered — but it quickly hits the fact that parsing is done up-front by |
Comment by Eddie LukeAtmey (JIRA) Hi @itaiferber, I just saw this pull request that has been merged into `apple:master`. I'd like to know if the pull request has fixed the issue. If so, when will the fix be distributed? |
eddiemarvin (JIRA User) I believe that change should make it into the Swift 5 release. However, please keep in mind that (a) this change is in swift-corelibs-foundation, and thus will not affect Darwin platforms, and (b) even that fix will not resolve the issue; although it improves parsing of values on non-Darwin platforms, the base issue remains: parsing numbers as |
Comment by Charles Maria Tor (JIRA) Anyone adversely affected by this issue like I was (I work for a bank, precision is very important) can use this custom JSONDecoder for the time being. Attached are tests written with SwiftCheck that prove the issue exists in JSONDecoder and does not exist in my replacement Decoder. |
Comment by Charles Maria Tor (JIRA) @itaiferber is anything being done to replace JSONSerialization with a non-lossy parser or is this issue going to be left indefinitely because it requires huge changes to legacy code? |
I am a bit puzzled by this as well. It is clearly a bug that using these types basically corrupts your data and no one seems to care really. I would have imagined that this is something that requires an urgent fix in a the next possible dot-release even. |
@tkrajacic I don't think it's fair to characterize this as nobody caring — I'd love to get this resolved, but replacing the implementation of chazza (JIRA User) We have no intention of leaving this indefinitely; given bandwidth (or contributions), we should absolutely work to fix this in a generalized way. Your JSON decoder is an excellent workaround for this in the meantime. |
@itaiferber You are absolutely right, that was unfair. I humbly and truly apologize. You are giving so much helpful feedback on the forums as well and I would say that you (and basically most other people concerned with Swift) are incredibly helpful and supportive. I spoke out of frustration. I am writing an application that handles monetary amounts, with quite a large data model and even "unit count"s are decimal of course. Double and Float should not even exist in programming languages anymore because they are broken for most anything that doesn't allow for "meh, if it is approximately this value that's fine". Now I have to write EVERY SINGLE Codable conformance myself because the encoders/decoder that Swift provides are broken. That's where my frustration comes from. And simply encoding/decoding Decimals as String would solve all of this. (Well, the other thing is that the compiler is stupid enough to treat literals when initializing Decimals as Doubles - so never correctly initializes a Decimal value, but that's another story… and bug) Now I do my best to help out. I write plenty of bug reports here and include sample projects with almost all of them which takes a considerable amount distilling them from a big project. Sadly I don't have the C++ expertise to fix bugs in the compiler and related projects. I do really not understand how this particular bug though can be ranked anything but top-priority as it basically corrupts user data. This is a critical bug in my humble opinion. |
Comment by Tiago Janela (JIRA) We also faced this issue on an iOS application we are working with which involves monetary amounts. We are currently using the referenced JSON Decoder with satisfactory results. I'm currently inclined to say that, from our use case perspective, a number of hooks at the JSONDecoder level would help overcome this issue with an acceptable tradeoff. This is a pattern that works well, given my experience with other JSON Coders/Decoders such as Newtonsoft's JSON.NET or FasterXML's Jackson. Happy to contribute in any way possible. |
Comment by Boris Yurkevich (JIRA) tiborbodecs (JIRA User) thank you for your ticket. I have ran your snippet in Xcode 10.2.1 Playground and what I got is different. DoubleItem(name: "Gum ball", price: 46.984765)
DecimalItem(name: "Gum ball", price: 46.98476500000001024) Looks like Double doesn't have this problem any more and we can use it. The problem now affects only Decimal. Update: I've been thinking a lot about this. I don't believe this is a platform issue, but an issue of us using Swift platform in an incorrect way. When dealing with financial problems, following this simple rule will keep your code accurate: use String for encoding and decoding. I have not found any issues converting between String and Decimal. I have tried "broken decimals" from chazza (JIRA User)'s workaround. If your server API does not uses String, encode with Double, do not attempt to encode with Decimal. You still can do math using Decimal. Then, when you ready to send it to server, encode it to Double. Apply `NSDecimalRound` when dealing with long numbers. If server gives you a numeric JSON value, this means they are already using primitive type on backend. The right solution in avoiding data loss, is changing code on the server to use String type for JSON models. |
Comment by Tiago Janela (JIRA) boris.y (JIRA User) I disagree with the premise of "using Swift platform in an incorrect way". I believe the correct way to handle this would be to provide functionality much like the DateDecodingStrategy only for Numbers, such as a NumberDecodingStrategy. This functionality could provide a default strategy that keeps backward compatibility and provides a way for those who need control to have it. Happy to discuss further. |
Comment by Boris Yurkevich (JIRA) tjanela (JIRA User) I am aware of the issue with Decimal encoding and decoding. Ideally it should not conform to the Codable protocol. This would prevent wrong assumptions. I am sure there’s a good reason to allow this. |
Comment by Charles Maria Tor (JIRA) boris.y (JIRA User) Removing Codable support isn’t going to happen for obvious reasons, and I’ve already demonstrated that this problem can be solved with different methods of JSON parsing, so I don’t know why you’re advocating for the worst solution to this problem. |
Comment by Tiago Janela (JIRA) And on that note, chazza (JIRA User), how can one go about integrating your suggested changes to the JSON parsing method into the codebase? What are the drawbacks of the current implementation of MonadicJSON from your point of view? |
Comment by Boris Yurkevich (JIRA) I have been doing more testing, and I have to retract some of my previous statements. Plain short value Double also suffers from this behaviour. It seams like MonadicJSON is necessary workaorund. import Foundation
// This is correct number which we send
let package: Double = 7067.51
let input: [String: Any] = ["Key": package]
let data = try! JSONSerialization.data(withJSONObject: input, options: [])
print(String(data: data, encoding: .utf8)!) Output: {"Key":7067.5100000000002} |
boris.y (JIRA User) What you're seeing for |
Comment by Charles Maria Tor (JIRA) tjanela (JIRA User) My suggested changes require a huge change to how numbers are currently being treated by `JSONSerializalion`. At the moment they're being first initialised into an NSNumber, but we need an intermediary container that preserves the original characters until they need to be converted into their final numeric representation. Going from String to NSNumber to Decimal is where we get precision issues, going directly from String to Decimal is the solution. This could theoretically be implemented without changing the API surface of JSONSerialization, but would require changes to NSNumber (to store the raw string) and would have huge implications for the entire Apple ecosystem by requiring conversion at extraction time. Memoization of the call would be needed but even then the performance implications can't be overlooked. |
Comment by Tiago Janela (JIRA) EDIT: Added the fact that the author suggested using a strategy approach to handle this |
I also have a hard time grasping how such an obvious case of data corruption can be ignored for so long. 7067.5100000000002 is NOT excess precision. It is NOT "more correct" than the 7067.51 that was fed into the serialization… That attitude is mind-boggling. |
CC @millenomi |
Comment by Tiago Janela (JIRA) Hey bendjones (JIRA User), let me know if you want to debate possible approaches or need any help. |
For Linux (swift-corelibs-foundation) there is a potential solution here: #2980 |
Comment by Adriano Gonçalves (JIRA) Hello. I was wondering if there's any update on this? We are facing the same issue on our current project and will try to fix it using chazza (JIRA User)'s implementation of a custom decoder, but would love to see this being fixed in the default JSONDecoder. |
Comment by Jason Bobier (JIRA) This is now biting me too. In particular, I'm converting JSON numbers to a rational number via a library that I wrote. I was expecting to read them in as Decimals and convert to my rationals, easy... no problem. Except I can't read numbers in as Decimals accurately. sigh 28.35 becomes 28 49258120924365/140737488355328 instead of 28 7/20. This is for display to an end user and no one wants to see that. Obviously, this is even more dangerous for monetary transactions. |
Comment by Doug Sjoquist (JIRA) It is not JSONDecoder's fault (or not just its fault). The conversion from double to decimal is flawed. This: import Foundation print(Decimal(46.984765)) produces this output: 46.98476500000001024 |
Comment by Tiago Janela (JIRA) Following your train of thought, dwsjoquist (JIRA User), this would make it the compiler's fault, since parsing and storing a Double literal is its responsibility. We write a double literal, the compiler parses and stores a double. The Decimal constructor that receives a double can't do much more than what it is doing right now. The information it gets is of a double value that, due to the way they are coded in the computer structure, is not a precise number in certain cases. Therefore, that particular constructor has no issues and is behaving accordingly. Furthermore, if you use Decimal("46.984765") you'll notice that you get adequate results. In fact, I believe the problem lies exactly there. Somewhere in JSONDecoder's implementation the receiving type should be inspected. If it is a Decimal then the constructor that receives a string should be used. |
Comment by Jason Bobier (JIRA) I do believe that it is the JSONDecoder's issue. A number in JSON is an arbitrary length signed decimal number. The decoder needs to be able to handle this without loss of accuracy or if it is unable to handle the given number (for example if it is too many digits), throw an error. If requested, this arbitrary length decimal number could be converted to a given output (such as Double or Decimal) with loss of precision if it would be required to store it in that format. But that loss of precision needs to be explicitly asked for by the caller. |
This is neither the compiler’s nor JSONDecoder’s fault (or at least not directly). The problem is caused by NSJSONSerialization parsing all floating point numbers as Double and JSONDecoder using NSJSONSerialization under the hood. If JSONDecoder was more than a simple wrapper around the legacy NSJSONSerialization type, it could be written properly to fix the Decimal parsing problem by parsing floating point values directly into Decimal when asked for (rather than using Double as the intermediate type). Or as an alternative, the issue could be fixed at the root cause, in NSJSONSerialization itself. |
Comment by Leonardo Savio Dabus (JIRA) If you need to preserve precision when initializing a Decimal you need to use the string initializer otherwise you would be using the double initializer and that's the problem. init?(string: String, locale: Locale) init(_ value: Double) What you need is: print(Decimal(string: "46.984765")!) |
Starting with commit 5ed74d3, the decoder is no longer using I'm not quite sure when this commit landed in the SDKs, but using Xcode 14.2 today, I don't get any loss of precision from the Gum Ball example: DoubleItem(name: "Gum ball", price: 46.984765)
DecimalItem(name: "Gum ball", price: 46.984765) Initializing |
Sadly but this issue is still actual. Edited: my opinion based on Gum Ball example, but with another number
|
@guillaumealgis @scoreyou bear in mind this is corelibs-foundation, not Apple Foundation. It might've been fixed in this one but not Apple's. You could try running Gum Ball on Linux to verify. Soon enough it could be fixed in both though, as Foundation is being rewritten in Swift, open sourced, and shared across all platforms. |
@scoreyou @davdroman thank you both for clarifying :) Indeed, its doesn't seems to be fixed in Apple Foundation yet, sadly :( |
This seems to be fixed on iOS 15 and newer. Deserializing on iOS 14, the result is Sadly, we target iOS 13+ in our applications and SDKs, so we need to wait a few years, and then we can drop the "string from the backend" approach 😬 |
@kober32 I've tested a few decimals on iOS 15 and 16 and some still seem to fail.
|
@bencallis I tried to reproduce this issue on Input: let jsonString = #"{ "name": "Gum ball", "price": 16684.24 }"# Output: DoubleItem(name: "Gum ball", price: 16684.24)
DecimalItem(name: "Gum ball", price: 16684.24) Does it seem alright? |
This issue seems to be fixed, it can be closed. |
Environment
Xcode 9.2 (9C40b)
Apple Swift version 4.0.3 (swiftlang-900.0.74.1 clang-900.0.39.2)
Same output with 4.1 swift-DEVELOPMENT-SNAPSHOT-2018-02-20-a-ubuntu16.04.
Additional Detail from JIRA
md5: 4d586dd9920250512d8b01ea1b3597c9
Issue Description:
Decoding decimals from a JSON structure returns incorrect numbers.
See the following example:
Output:
Expected result for the DecimalItem should be: 46.984765
I know that floating point values can not be represented precisely, but decimals should keep their original values, am I wrong? Is this a Swift foundation bug or an intended behavior? Note that encoding the same value with the JSONEncoder will provide the correct value in the JSON.
My actual problem is that the JSONEncoder and JSONDecoder classes are working inconsistently.
If the encoder will output the value as a number, I'd expect that the decoder can decode the exact same value from that if I use the decimal type in my model. My other idea is that the encoder should transform and save the value into a string type inside the JSON instead of the current representation, this could be supported with a decimal coding strategy.
Please share your thoughts about this idea.
The text was updated successfully, but these errors were encountered: