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

[Yonom]

Fixes #3004.

Test Plan

• call firestore.collection('myCollection').where('myField', 'in', [0]) and confirm that correct results are being returned

[CLAassistant]

All committers have signed the CLA.

[vercel]

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployments, click below or on the icon next to each commit.

react-native-firebase – ./

🔍 Inspect: https://vercel.com/invertase/react-native-firebase/JE4wXhAYQXG86vF5Pe5QBvpS4D7C
✅ Preview: https://react-native-firebase-git-fork-yonom-fix-ios-w-c43697-invertase.vercel.app

react-native-firebase-next – ./website_modular

🔍 Inspect: https://vercel.com/invertase/react-native-firebase-next/3qjCTL3jtHpXTaoRLZH2n7mZcjGr
✅ Preview: Canceled

[Deployment for 18160b0 canceled]

[mikehardy]

Oh my - that's one of those fixes that's so obvious when viewed as a diff in a PR.
There is a mystery embedded in the issues/PRs/tests where the question is: with #3895 we are testing numbers in where clauses, and it's working for us in our e2e rig, but obviously it is failing in practice. Why? I believe I have the answer, our e2e rig is a bit special and it's running the bundle in the e2e app via the javascript debugging interface on a node instance, on linux. Stated differently, the "isAndroid" checks are querying the node VM not the react-native app container, and since those run on unix (Darwin or Ubuntu) and android is also linux, they're answering "yes" to "isAndroid" in the javascript check.

So my hypothesis is that our test was always doing a string conversion. Remains to be proven that is happening, and if so how the backend is handling a string query on number contents (maybe it's auto-converting?) such that it works, but that's my hypothesis for why the test we already have is false-negative.

I'm preliminarily approving this (and really really appreciate it!) but I need to poke at it a little from the testing side to see if I can get tests to fail before merging.

That shouldn't be a blocker though, we release very frequently so the diffs are small, and we generate a patch-package patch set as one of our CI actions, so you may trivially integrate this fix in a reliable (via patch-package) way and not have to maintain a fork or worry about updates etc.

I won't delay long on merge+release either way

[codecov]

Codecov Report

Merging #5840 (596d9b6) into main (f7313b9) will increase coverage by 0.47%.
The diff coverage is 66.67%.

❗ Current head 596d9b6 differs from pull request most recent head 18160b0. Consider uploading reports for the commit 18160b0 to get more accurate results

@@             Coverage Diff              @@
##               main    #5840      +/-   ##
============================================
+ Coverage     52.56%   53.02%   +0.47%     
- Complexity      620      627       +7     
============================================
  Files           208      208              
  Lines         10154    10157       +3     
  Branches       1612     1614       +2     
============================================
+ Hits           5336     5385      +49     
+ Misses         4560     4520      -40     
+ Partials        258      252       -6     

[Yonom]

@mikehardy

So my hypothesis is that our test was always doing a string conversion. Remains to be proven that is happening, and if so how the backend is handling a string query on number contents (maybe it's auto-converting?) such that it works, but that's my hypothesis for why the test we already have is false-negative.

The (firebase) backend never received strings on either platform. Before this PR, the old code return @([typeMap[1] doubleValue]); took care of converting strings to doubles if they were passed as such. (Hence my note on the fact that the JS change is backwards compatible with existing native library versions).

In my tests, the firebase backend is stricter and does not accept neither a string nor a double as equal to an integer value due to the different type. So that shouldn't be a reason for tests not failing.

Idk why your tests aren't failing, but here are some thoughts:

• This only happens if the object fields are integers. Does the iOS SDK save integer object fields as int64? The JS SDK certainly does.
• It silently fails and returns an empty array.

---

To be honest, I consider this a bug in the Firebase iOS SDK though. The JS SDK must do some automatic integer detection to save 0.0 as an integer instead of double. This automatic conversion is left as an exercise for the user (in this case, react-native-firebase) for the iOS SDK.

I don't care enough about the iOS SDK to file a bug though, so this is an exercise for the reader. 😊

(same can be said about the Android SDK)

[Yonom]

Oh look your tests are failing! 😄

---

Jokes aside; I've made up my mind on why your test's aren't failing on the mentioned issue though:

await ref.add({ status: 1 });

This saves 1 (double) as the value for status. Therefore, a query for double succeeds.

How to verify:

1. set rules for a firestore object to allow ALL: allow read, write: if true;
2. write to the object to firestore await ref.add({ status: 1 });
3. go to https://firestore.googleapis.com/v1/projects/$PORJECT_ID/databases/(default)/documents/$COLLECTION_ID/$DOCUMENT_ID

View the output:

{
  "name": "projects/<redacted>/databases/(default)/documents/config/test",
  "fields": {
    "status": {
      "doubleValue": 1
    }
  },
  "createTime": "2021-10-14T13:15:10.763946Z",
  "updateTime": "2021-11-07T19:25:48.364106Z"
}

[mikehardy]

Ah, so if I understand correctly / just restating: our tests are passing because setting a double then checking a double of course works. But there is no ability in the current react-native-firebase to set an int then check an int - but if we had (for instance) a test where there was already an int in there (not set via our android implementation as it exists, since it will never set an int) then it would fail. And that makes sense and would explain it completely.

Our tests are quite flaky at the moment unfortunately - android is a rarer flake but iOS has a serious problem right now with Detox that unfortunately causes the app to crash quickly. I restarted both those test runs though and I'm pretty sure they'll roll through if the flakiness doesn't halt them

[Yonom]

No worries!

Some thing to keep in mind:

If you have a case where there was already an integer saved as double (set by the iOS implementation as it currently exists) then the same object will no longer be found via .where after this patch is applied.

This makes this change a minor breaking change 😱. Mitigation would be to go through your whole DB and load and re-save every object (I've been there, done that)

EDIT: does this change also handle saving integers? I am not sure if there are alternative code paths to take care of

[mikehardy]

I'm okay with major version releases, as a consumer I actually prefer them to be conservative (implying this would be a major version) just with full details on exactly how to know if it affects you and what to do if it does. This will likely be one of those, now that you mention it. I handle that when I do the merge via special commit message tokens for conventional commits to work through

https://firebase.google.com/docs/firestore/manage-data/data-types indicates there is "floating point number" and "integer". I'm still getting the work bench set up to really look at this, and I'll inspect it from that perspective. Based on the related android PR I think this handles all the branches but I'll look at android and ios and have a think about it

I really appreciate all the thought you've put into it and the details you've commented on, they are very helpful

[Yonom]

One more aspect that I thought about - there are multiple ways to detect double vs integer:

• toString() and check for the existence of "." (current approach)
• pass double and check for n % 1 !== 0

I chose the first because the android version used the same approach and it makes sense to apply the same design over to iOS. However, I was worried that the conversion to string and back to double would cause bits to get lost over the wire, so I wrote this quick and dirty test:

for (let i = 0; i < 100000000; i++) {
  const n = Number.EPSILON * i;
  console.assert(n === parseFloat(n.toString()));
}
// no logs encountered

So I assume it's safe. Just documenting it here for your review, in case I'm wrong.

[mikehardy]

Sorry for the delay on this! My work queue unexpectedly got very deep, very quickly. Mentally the delay is mostly about how to handle + message the breaking change, the change itself looks good to me still. For interested people this is the exact reason we generate patch-package patch-sets in CI though - if you need this, please grab it, and if you do that and use it please report any success or failure.

Thanks, and thanks for your patience

[Yonom]

There are still several cases that are handled incorrectly (differently than the JS SDK) by react-native-firebase:

Value	JS SDK	react-native-firebase
-0	doubleValue	integerValue
Number.MAX_SAFE_INTEGER + 2	doubleValue	integerValue
Number.MIN_SAFE_INTEGER - 2	doubleValue	integerValue

Here are the relevant code lines in the JS SDK:
https://github.com/firebase/firebase-js-sdk/blob/master/packages/firestore/src/remote/number_serializer.ts#L56
https://github.com/firebase/firebase-js-sdk/blob/master/packages/firestore/src/util/types.ts#L44

---

Off topic:
I just realized that the nodejs sdk does not handle negative zero correctly either: https://github.com/googleapis/nodejs-firestore/blob/main/dev/src/serializer.ts#L121
I'll create create a bug report for that...

[mikehardy]

Github does not offer me the 😨 emoji as a reaction on your comment but it's appropriate. Really hard to get things right, but also it's math with an example implementation - there is a right answer - and we should get it right. Any further commits on this PR happily accepted, or a separate PR if you have time. Can stage them all as one "Hey everyone! We're getting numbers typed correctly in firebase now..." breaking release

[Yonom]

Yeah I'll add a fix to this PR (probably get to doing it on Monday)

[mikehardy]

This literally just bit me on a project, and now I'm using this patch-package too 🙈 - I need to do the needful on the release here and get it out. I would have been stumped if I hadn't known about the issue and known about this PR.

https://github.com/invertase/react-native-firebase/suites/4281049278/artifacts/111469567

(I was only able to make it fail when I queried via where with an in, not an == and sent the number in the array, it would work if I did an ==...)

[mikehardy]

That didn't quite go as planned, it looks like the item is not actually a string as it pops out of the type map for some reason:

[mikehardy]

Really strange, in our e2e test harness I do not reproduce the crash but I've gone back and forth on my work project with the patch twice and it reproduces with the patch each time. I'm not certain what is wrong but still suspect something about my integration via the patch s incorrect, just haven't found it yet.

I did successfully probe the failure though, so we finally have a test case that correctly breaks with our existing broken implementation (and passes with your fix!)

f9ce428#diff-b7d5d6ae9890f87025278e8b804153bd92e1a0c6a6c9ab78acdf9ac2a8015855R178-R184

[mikehardy]

It was my patch integration, the patch checks out 💯 in a test integration with a real project.
The local problem I had is that I'm testing in a distributed cluster of emulators and browsers and simulators with them all pointing to bundler on one machine over a ssh port forwards. My native build (on a different machine) was out of sync with the bundle build (on my local machine) with regard to whether the patch was integrated or not, so the types were going in to native from JS without your change to string from number on the JS side. Totally my fault.

Okay, this is still on deck to integrate after a few more cross-SDK compatibility checks to see if I can narrow down the guidance for release notes, but now it's making a contract project of mine work correctly ;-). Many thanks.

[Yonom]

You can expect my fixes for the edge cases within the next 12 hours.

I have decided to remove the toString conversion and pass double on both platforms. On the native side I check for bounds around MAX_SAFE_NUMBER and if the double value is an integer (analogous to how the JS SDK does it)

I have yet to implement negative zero handling but I can confirm that negative zero crosses the bridge without issues 👍

[mikehardy]

Fantastic!

[mikehardy]

@chiendv I used this added to the e2e tests in order to test it: f9ce428#diff-b7d5d6ae9890f87025278e8b804153bd92e1a0c6a6c9ab78acdf9ac2a8015855R178-R184

That test definitely fails with existing released code, but was fixed by the original version of this patch. I haven't really exercised the current version of the patch but I do have it integrated in a work project now (unreleased) and was hoping to test it shortly.

If you are not sure you integrated it correctly via git reference, you can do like I do and try using patch-package (https://github.com/ds300/patch-package) and integrating the patch set including all our unreleased changes (they are minor, and safe - just waiting for release) along with this change: https://github.com/invertase/react-native-firebase/actions/runs/1503114495

[chiendv]

@mikehardy thank for your help, I got it work on my project now.

[mikehardy]

Fantastic, and I'm happy to hear the success report (though I'm not surprised: @Yonom appears to have done great work here and I really need to get it released!). Good luck with your project

[Yonom]

Finally got around to writing tests! I caught three issues

large numbers on Android were being truncated to int32 (fixed)

This was causing existing tests to fail. Now doing (long) typeArray.getDouble(1) instead of typeArray.getInt(1)

-0 did not cross the bridge safely in the test environment (fixed)

The native side was receiving 0 instead of -0 on both Android and iOS, which resulted in the following being stored:

"number": {
  "doubleValue": 0.0
}

It works on my other app, I can't figure out why... JSI? TurboModules? Hermes? 🤷
I assume the bridge payload is getting stringified somewhere, since: -0.toString() === '0' and JSON.stringify(-0) === '0'

To be sure that it works everywhere, I added an extra data type INT_NEGATIVE_ZERO, similar to the other edge cases (NaN, Inf, -Inf).

can't query where-in NaN (wontfix)

Querying .where('x', 'in', [NaN]) does not return any results
It doesn't return anything in the JS SDK either, so we are on parity with the official client, good enough IMO!

[mikehardy]

Your experience matches my experience with regard to testing: there is nothing so cleansing as an automated test...the corner cases discovered, the surprise "enjoyed" by what is found ;-)

This looks fantastic. One tiny comment

[mikehardy]

It's a thing of beauty. I imagine you are now the master of all-thing-number-related when it comes to how a) Javascript deals with them and b) how the RN bridge deals with them and c) their native representations. Believe it or not that'll probably come in handy for life. I knew much of this stuff but not all of it, and learned quite a bit as you made this PR, thanks!

[mikehardy]

My work stack is finally unwinding! Preparing to merge this. Looking for guidance on the changelog breaking change note. Currently thought:

BREAKING CHANGE: Previous versions had an issue with number type handling during save on iOS. Integers would be saved as doubles, incorrectly. Previously you had to save numbers as strings if you wanted .where queries to work cross-platform. Number types are now 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.

[mikehardy]

I think I've mitigated iOS E2E CI flakiness to a large degree with #5932 - I'm going to re-base and repush this branch to hopefully get CI green pre-merge

[mikehardy]

Just wrestling with CI at this point, my hoped-for iOS E2E flakiness mitigation is unfortunately not the fix I was hoping for. Anyway, this is on deck, RNFB's getting a release with a bunch of pent-up fixes shortly and this will be in

[Yonom]

Awesome! :)