Workaround for NSDecimalNumber longLongValue issue. #4109
Conversation
…th [NSDecimalNumber longLongValue]. Still does not deal with the fact that a double and an NSDecimalNumber wrapping a double can be represented in different ways on the server.
|
@mortenbekditlevsen Thanks for the PR! It looks like our new Danger integration is breaking CI for external PRs. cc: @morganchen12 We'll investigate that and get this reviewed. |
|
@mortenbekditlevsen The danger issue has been fixed in master, if you merge/rebase it should go away. Sorry for the inconvenience! |
| @@ -878,7 +878,17 @@ - (id)fixDoubleParsing:(id)value | |||
| // values, including 2.47. Because we use the exact bytes for hashing on the | |||
| // server this will lead to hash mismatches. The parser of NSNumber seems to | |||
| // be more in line with what the server expects, so we use that here | |||
| if ([value isKindOfClass:[NSNumber class]]) { | |||
| if ([value isKindOfClass:[NSDecimalNumber class]]) { | |||
| NSDecimal d = [(NSDecimalNumber*)value decimalValue]; | |||
There was a problem hiding this comment.
What do you think of renaming "d" to "original" and "result" to "rounded"?
| @@ -878,7 +878,17 @@ - (id)fixDoubleParsing:(id)value | |||
| // values, including 2.47. Because we use the exact bytes for hashing on the | |||
| // server this will lead to hash mismatches. The parser of NSNumber seems to | |||
| // be more in line with what the server expects, so we use that here | |||
There was a problem hiding this comment.
Can you move this existing comment into the else if if ([value isKindOfClass:[NSNumber class]]) block?
There was a problem hiding this comment.
Certainly, but the essential idea of what is going on inside both the NSDecimalNumber case and the NSNumber case is the same, so does this comment not apply to both?
I am updating the PR with the moved comment, but I think that perhaps it belongs to the entire method.
| @@ -878,7 +878,17 @@ - (id)fixDoubleParsing:(id)value | |||
| // values, including 2.47. Because we use the exact bytes for hashing on the | |||
| // server this will lead to hash mismatches. The parser of NSNumber seems to | |||
| // be more in line with what the server expects, so we use that here | |||
| if ([value isKindOfClass:[NSNumber class]]) { | |||
| if ([value isKindOfClass:[NSDecimalNumber class]]) { | |||
| NSDecimal d = [(NSDecimalNumber*)value decimalValue]; | |||
There was a problem hiding this comment.
Please add a comment with a link to the underlying Radar here.
|
Thank you very much for the review. I have updated the PR with requested changes and added additional comments. Have you had a chance to consider the other issue described in: #4108? |
There was a problem hiding this comment.
Thanks for updating the PR. Your comments are extremely helpful and I would like to merge this as is.
If you see excessive network traffic even with this fix due to hash mismatches, then let's go back and see if there are other fixes we need to make. I am bit afraid that this might lead us to a slippery slope, where in the end we have to do so much post-processing that we are essentially implementing our own JSON number parser. Fortunately, we do compute our Node hashes based on deserialized data, so we should already have the same hash value (at least when the backend and the SDK agree on the long representation).
Discussion
The discussion of the issue can be found here:
#4108
This PR only provides a workaround for a bug with [NSDecimalNumber longLongValue] on values with high precision.
It does not deal with the fact that the RTDB server appears to store a double wrapped in an NSNumber differently than the same value wrapped in an NSDecimalNumber.
As an NSDecimalNumber the higher precision is stored, but the persistence layer has no knowledge about whether we are in one case or the other.
Testing
The added logic changes the CFNumber type of one of the existing tests. Otherwise all existing tests passed.
API Changes
No API changes in the PR.