Workaround for NSDecimalNumber longLongValue issue. (#4109)

Author: mortenbekditlevsen

Example/Database/Tests/Unit/FLevelDBStorageEngineTests.m

@@ -515,12 +515,40 @@ - (void)testDoublesAreRoundedProperly {
   XCTAssertEqualObjects([[engine serverCacheAtPath:PATH(@"foo")] dataHash], hashFor247);
 }
 
+// NOTE: This deals with part of the NSDecimalNumber issue: namely that
+// [NSDecimalNumber longLongValue] is completely bonkers for decimals with
+// high precision. The decimal value below (close to 1000) is returned as -844!
+// http://www.openradar.me/radar?id=5007005597040640
+// This does not deal with the fact that this is not the same behavior as the
+// RTDB server. Given an NSDecimalNumber, the server stores decimals with the
+// full precision and returns these as NSDecimalNumbers too.
+// This means that the RTDB server can store the double value 2.47 as 2.47.
+// But if the double is wrapped in an NSDecimalNumber, the server apparently
+// stores the value 2.470000000000000512.
+// This means that the persistence layer will have a hard time determining whether
+// rounding is appropriate or not.
+// Similarly, as an NSDecimalNumber, the RTDB gladly stores and returns
+// 999.9999999999999487 without any rounding, although considered as a double,
+// the value will be 1000.
+- (void)testNSDecimalsAreRoundedProperly {
+  FLevelDBStorageEngine *engine = [self cleanStorageEngine];
+  id decimalValue = [NSDecimalNumber decimalNumberWithString:@"999.9999999999999487"];
+  id expectedDecimalValue = [NSDecimalNumber decimalNumberWithString:@"1000"];
+  [engine updateServerCache:NODE(decimalValue) atPath:PATH(@"foo") merge:NO];
+
+  id<FNode> actualData = [engine serverCacheAtPath:PATH(@"foo")];
+  NSNumber *actualDecimal = [actualData val];
+  XCTAssertEqualObjects([actualDecimal stringValue], [expectedDecimalValue stringValue]);
+  XCTAssertEqual(CFNumberGetType((CFNumberRef)actualDecimal), kCFNumberFloat64Type);
+}
+
 - (void)testIntegersAreReturnedsAsIntegers {
   id intValue = @247;
   id longValue = @1542405709418655810;
   id doubleValue = @0xFFFFFFFFFFFFFFFFUL;  // This number can't be represented as a signed long.
 
-  id<FNode> expectedData = NODE((@{@"int" : @247, @"long" : longValue, @"double" : doubleValue}));
+  id<FNode> expectedData =
+      NODE((@{@"int" : intValue, @"long" : longValue, @"double" : doubleValue}));
   FLevelDBStorageEngine *engine = [self cleanStorageEngine];
   [engine updateServerCache:expectedData atPath:PATH(@"foo") merge:NO];
   id<FNode> actualData = [engine serverCacheAtPath:PATH(@"foo")];
@@ -532,18 +560,7 @@ - (void)testIntegersAreReturnedsAsIntegers {
   XCTAssertEqual(CFNumberGetType((CFNumberRef)actualInt), kCFNumberSInt64Type);
   XCTAssertEqualObjects([actualLong stringValue], [longValue stringValue]);
   XCTAssertEqual(CFNumberGetType((CFNumberRef)actualLong), kCFNumberSInt64Type);
-#ifdef TARGET_OS_IOS
-  // Catalyst and iOS 13 use int128_t but CFNumber still calls it 64 bits.
-  if ([[NSProcessInfo processInfo] operatingSystemVersion].majorVersion >= 13) {
-    XCTAssertEqual(CFNumberGetType((CFNumberRef)actualDouble), kCFNumberSInt64Type);
-  } else {
-    XCTAssertEqual(CFNumberGetType((CFNumberRef)actualDouble), kCFNumberFloat64Type);
-  }
-#elif TARGET_OS_MACCATALYST
   XCTAssertEqual(CFNumberGetType((CFNumberRef)actualDouble), kCFNumberSInt64Type);
-#else
-  XCTAssertEqual(CFNumberGetType((CFNumberRef)actualDouble), kCFNumberFloat64Type);
-#endif
 }
 
 // TODO[offline]: Somehow test estimated server size?

Firebase/Database/Persistence/FLevelDBStorageEngine.m

@@ -873,12 +873,47 @@ - (NSData *)serializePrimitive:(id)value {
 
 - (id)fixDoubleParsing:(id)value
     __attribute__((no_sanitize("float-cast-overflow"))) {
-    // The parser for double values in JSONSerialization at the root takes some
-    // short-cuts and delivers wrong results (wrong rounding) for some double
-    // 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]]) {
+        // In case the value is an NSDecimalNumber, we may be dealing with
+        // precisions that are higher than what can be represented in a double.
+        // In this case it does not suffice to check for integral numbers by
+        // casting the [value doubleValue] to an int64_t, because this will
+        // cause the compared values to be rounded to double precision.
+        // Coupled with a bug in [NSDecimalNumber longLongValue] that triggers
+        // when converting values with high precision, this would cause
+        // values of high precision, but with an integral 'doubleValue'
+        // representation to be converted to bogus values.
+        // A radar for the NSDecimalNumber issue can be found here:
+        // http://www.openradar.me/radar?id=5007005597040640
+        // Consider the NSDecimalNumber value: 999.9999999999999487
+        // This number has a 'doubleValue' of 1000. Using the previous version
+        // of this method would cause the value to be interpreted to be integral
+        // and then the resulting value would be based on the longLongValue
+        // which due to the NSDecimalNumber issue would turn out as -844.
+        // By using NSDecimal logic to test for integral values,
+        // 999.9999999999999487 will not be considered integral, and instead
+        // of triggering the 'longLongValue' issue, it will be returned as
+        // the 'doubleValue' representation (1000).
+        // Please note, that even without the NSDecimalNumber issue, the
+        // 'correct' longLongValue of 999.9999999999999487 is 999 and not 1000,
+        // so the previous code would cause issues even without the bug
+        // referenced in the radar.
+        NSDecimal original = [(NSDecimalNumber *)value decimalValue];
+        NSDecimal rounded;
+        NSDecimalRound(&rounded, &original, 0, NSRoundPlain);
+        if (NSDecimalCompare(&original, &rounded) != NSOrderedSame) {
+            NSString *doubleString = [value stringValue];
+            return [NSNumber numberWithDouble:[doubleString doubleValue]];
+        } else {
+            return [NSNumber numberWithLongLong:[value longLongValue]];
+        }
+    } else if ([value isKindOfClass:[NSNumber class]]) {
+        // The parser for double values in JSONSerialization at the root takes
+        // some short-cuts and delivers wrong results (wrong rounding) for some
+        // double 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
         CFNumberType type = CFNumberGetType((CFNumberRef)value);
         if (type == kCFNumberDoubleType || type == kCFNumberFloatType) {
             // The NSJSON parser returns all numbers as double values, even