fix(firestore)!: fix Long/Double conversion issues #3004 (#5840)

* feat(firestore): fix iOS Long/Double conversion #3004 * Add special -0 handling * Add tests Co-Authored-By: Mike Hardy <github@mikehardy.net> BREAKING CHANGE: Previous versions of firestore here incorrectly saved integers as doubles on iOS, so they did not show up in `where`/`in` queries. You had to save numbers as strings if you wanted `where`/`in` queries to work cross-platform. Number types will now be handled correctly. However, If you have integers saved (incorrectly!) as double (from previous versions) and you use where / in style queries on numbers, then the same document will no longer be found via .where. Mitigation could be to go through your whole DB and load and re-save the integers correctly, or alter queries. Please test your where / in queries that use number types if this affects you.
invertase · Dec 14, 2021 · 910d4e4 · 910d4e4 · vercel · Dec 14, 2021
1 parent 3ebe5bb
commit 910d4e4
Show file tree

Hide file tree

Showing 5 changed files with 99 additions and 22 deletions.
diff --git a/...actnative/java/io/invertase/firebase/firestore/ReactNativeFirebaseFirestoreSerialize.java b/...actnative/java/io/invertase/firebase/firestore/ReactNativeFirebaseFirestoreSerialize.java
@@ -60,7 +60,7 @@ public class ReactNativeFirebaseFirestoreSerialize {
   private static final int INT_DOCUMENTID = 4;
   private static final int INT_BOOLEAN_TRUE = 5;
   private static final int INT_BOOLEAN_FALSE = 6;
-  private static final int INT_NUMBER = 7;
+  private static final int INT_DOUBLE = 7;
   private static final int INT_STRING = 8;
   private static final int INT_STRING_EMPTY = 9;
   private static final int INT_ARRAY = 10;
@@ -70,6 +70,8 @@ public class ReactNativeFirebaseFirestoreSerialize {
   private static final int INT_BLOB = 14;
   private static final int INT_FIELDVALUE = 15;
   private static final int INT_OBJECT = 16;
+  private static final int INT_INTEGER = 17;
+  private static final int INT_NEGATIVE_ZERO = 18;
   private static final int INT_UNKNOWN = -999;
 
   // Keys
@@ -300,7 +302,7 @@ private static WritableArray buildTypeMap(Object value) {
     }
 
     if (value instanceof Integer) {
-      typeArray.pushInt(INT_NUMBER);
+      typeArray.pushInt(INT_DOUBLE);
       typeArray.pushDouble(((Integer) value).doubleValue());
       return typeArray;
     }
@@ -325,19 +327,19 @@ private static WritableArray buildTypeMap(Object value) {
         return typeArray;
       }
 
-      typeArray.pushInt(INT_NUMBER);
+      typeArray.pushInt(INT_DOUBLE);
       typeArray.pushDouble(doubleValue);
       return typeArray;
     }
 
     if (value instanceof Float) {
-      typeArray.pushInt(INT_NUMBER);
+      typeArray.pushInt(INT_DOUBLE);
       typeArray.pushDouble(((Float) value).doubleValue());
       return typeArray;
     }
 
     if (value instanceof Long) {
-      typeArray.pushInt(INT_NUMBER);
+      typeArray.pushInt(INT_DOUBLE);
       typeArray.pushDouble(((Long) value).doubleValue());
       return typeArray;
     }
@@ -461,14 +463,12 @@ static Object parseTypeMap(FirebaseFirestore firestore, ReadableArray typeArray)
         return true;
       case INT_BOOLEAN_FALSE:
         return false;
-      case INT_NUMBER:
-        // https://github.com/invertase/react-native-firebase/issues/3004
-        // Number values come from JS as Strings on Android so we can check for floating points
-        String numberStringValue = Objects.requireNonNull(typeArray.getString(1));
-        if (numberStringValue.contains(".")) {
-          return Double.valueOf(numberStringValue);
-        }
-        return Long.valueOf(numberStringValue, 10);
+      case INT_NEGATIVE_ZERO:
+        return -0.0;
+      case INT_INTEGER:
+        return (long) typeArray.getDouble(1);
+      case INT_DOUBLE:
+        return typeArray.getDouble(1);
       case INT_STRING:
         return typeArray.getString(1);
       case INT_STRING_EMPTY:

diff --git a/packages/firestore/e2e/issues.e2e.js b/packages/firestore/e2e/issues.e2e.js
@@ -16,6 +16,22 @@
  */
 
 const COLLECTION = 'firestore';
+const { getE2eEmulatorHost } = require('@react-native-firebase/app/e2e/helpers');
+const jsFirebase = require('firebase/compat/app');
+require('firebase/compat/firestore');
+
+const testNumbers = {
+  zero: 0, // int
+  negativeZero: -0, // double
+  half: 0.5, // double
+  unsafeInt: Number.MAX_SAFE_INTEGER + 1, // double
+  nagativeUnsafeInt: Number.MIN_SAFE_INTEGER - 1, // double
+  safeInt: Number.MAX_SAFE_INTEGER, // int
+  nagativeSafeInt: Number.MIN_SAFE_INTEGER, // int
+  inf: Infinity, // double
+  negativeInf: -Infinity, // double
+  // nan: NaN, // double -- where-in queries on NaN do not work
+};
 
 describe('firestore()', function () {
   describe('issues', function () {
@@ -92,4 +108,51 @@ describe('firestore()', function () {
       });
     });
   });
+
+  describe('number type consistency', function () {
+    before(async function () {
+      jsFirebase.initializeApp(FirebaseHelpers.app.config());
+      jsFirebase.firestore().useEmulator(getE2eEmulatorHost(), 8080);
+
+      // Put one example of each number in our collection using JS SDK
+      await Promise.all(
+        Object.entries(testNumbers).map(([testName, testValue]) => {
+          return jsFirebase
+            .firestore()
+            .doc(`${COLLECTION}/numberTestsJS/cases/${testName}`)
+            .set({ number: testValue });
+        }),
+      );
+
+      // Put one example of each number in our collection using Native SDK
+      await Promise.all(
+        Object.entries(testNumbers).map(([testName, testValue]) => {
+          return firebase
+            .firestore()
+            .doc(`${COLLECTION}/numberTestsNative/cases/${testName}`)
+            .set({ number: testValue });
+        }),
+      );
+    });
+
+    it('types inserted by JS may be queried by native with filters', async function () {
+      const testValues = Object.values(testNumbers);
+      const ref = firebase
+        .firestore()
+        .collection(`${COLLECTION}/numberTestsJS/cases`)
+        .where('number', 'in', testValues);
+      typesSnap = await ref.get();
+      should.deepEqual(typesSnap.docs.map(d => d.id).sort(), Object.keys(testNumbers).sort());
+    });
+
+    it('types inserted by native may be queried by JS with filters', async function () {
+      const testValues = Object.values(testNumbers);
+      const ref = jsFirebase
+        .firestore()
+        .collection(`${COLLECTION}/numberTestsNative/cases`)
+        .where('number', 'in', testValues);
+      typesSnap = await ref.get();
+      should.deepEqual(typesSnap.docs.map(d => d.id).sort(), Object.keys(testNumbers).sort());
+    });
+  });
 });
diff --git a/packages/firestore/ios/RNFBFirestore/RNFBFirestoreSerialize.m b/packages/firestore/ios/RNFBFirestore/RNFBFirestoreSerialize.m
@@ -46,7 +46,7 @@ @implementation RNFBFirestoreSerialize
   INT_DOCUMENTID,
   INT_BOOLEAN_TRUE,
   INT_BOOLEAN_FALSE,
-  INT_NUMBER,
+  INT_DOUBLE,
   INT_STRING,
   INT_STRING_EMPTY,
   INT_ARRAY,
@@ -56,6 +56,8 @@ @implementation RNFBFirestoreSerialize
   INT_BLOB,
   INT_FIELDVALUE,
   INT_OBJECT,
+  INT_INTEGER,
+  INT_NEGATIVE_ZERO,
   INT_UNKNOWN = -999,
 };
 
@@ -336,7 +338,7 @@ + (NSArray *)buildTypeMap:(id)value {
     }
 
     // Number
-    typeArray[0] = @(INT_NUMBER);
+    typeArray[0] = @(INT_DOUBLE);
     typeArray[1] = value;
     return typeArray;
   }
@@ -400,7 +402,11 @@ + (id)parseTypeMap:(FIRFirestore *)firestore typeMap:(NSArray *)typeMap {
       return @(YES);
     case INT_BOOLEAN_FALSE:
       return @(NO);
-    case INT_NUMBER:
+    case INT_NEGATIVE_ZERO:
+      return @(-0.0);
+    case INT_INTEGER:
+      return @([typeMap[1] longLongValue]);
+    case INT_DOUBLE:
       return @([typeMap[1] doubleValue]);
     case INT_STRING:
       return typeMap[1];

diff --git a/packages/firestore/lib/utils/serialize.js b/packages/firestore/lib/utils/serialize.js
@@ -16,7 +16,6 @@
  */
 
 import {
-  isAndroid,
   isArray,
   isBoolean,
   isDate,
@@ -139,10 +138,15 @@ export function generateNativeData(value, ignoreUndefined) {
   }
 
   if (isNumber(value)) {
-    if (isAndroid) {
-      return getTypeMapInt('number', value.toString());
+    // mirror the JS SDK's integer detection algorithm
+    // https://github.com/firebase/firebase-js-sdk/blob/086df7c7e0299cedd9f3cff9080f46ca25cab7cd/packages/firestore/src/remote/number_serializer.ts#L56
+    if (value === 0 && 1 / value === -Infinity) {
+      return getTypeMapInt('negativeZero');
     }
-    return getTypeMapInt('number', value);
+    if (Number.isSafeInteger(value)) {
+      return getTypeMapInt('integer', value);
+    }
+    return getTypeMapInt('double', value);
   }
 
   if (isString(value)) {
@@ -254,7 +258,9 @@ export function parseNativeData(firestore, nativeArray) {
       return true;
     case 'booleanFalse':
       return false;
-    case 'number':
+    case 'double':
+    case 'integer':
+    case 'negativeZero':
     case 'string':
       return value;
     case 'stringEmpty':

diff --git a/packages/firestore/lib/utils/typemap.js b/packages/firestore/lib/utils/typemap.js
@@ -25,7 +25,7 @@ const MAP = {
   documentid: 4, // to native only
   booleanTrue: 5,
   booleanFalse: 6,
-  number: 7,
+  double: 7,
   string: 8,
   stringEmpty: 9,
   array: 10,
@@ -35,6 +35,8 @@ const MAP = {
   blob: 14,
   fieldvalue: 15,
   object: 16,
+  integer: 17,
+  negativeZero: 18,
   unknown: -999,
 };