invertase / react-native-firebase

🔥 A well-tested feature-rich modular Firebase implementation for React Native. Supports both iOS & Android platforms for all Firebase services.
https://rnfirebase.io
Other
11.72k stars 2.22k forks source link

🔥 update() with null does not remove the value from Firebase Database in iOS #8144

Open Stas-Buzunko opened 4 days ago

Stas-Buzunko commented 4 days ago

Issue

"@react-native-firebase/database": "^21.4.0"

import { firebase } from '@react-native-firebase/database'

firebase
    .database()
    .ref()
    .update({
      'any-path': null
    })
    .then(() => {
      console.log('Update succeeded')
    })
    .catch((error) => {
      console.error('Update failed: ', error);
    });

On iOS, using null with the update() method doesn't remove the key's value from the database as expected; instead, the old value remains. However, on Android, Web, and Admin SDK, this behavior works as intended, and the key is removed.

Updating the value to false works as expected, and the value gets updated correctly in the database.

I face the issue every time on the iOS simulator (v18.1, Xcode 16), but there were also cases in production with the older version of @react-native-firebase/database: ^18.3.2. I believe that not all iOS production users are affected; otherwise, the issue would have been noticed earlier.

Describe your issue here


Project Files

Javascript

Click To Expand

#### `package.json`: ```json # N/A ``` #### `firebase.json` for react-native-firebase v6: ```json # N/A ```

iOS

Click To Expand

#### `ios/Podfile`: - [ ] I'm not using Pods - [x] I'm using Pods and my Podfile looks like: ```ruby # N/A ``` #### `AppDelegate.m`: ```objc // N/A ```


Android

Click To Expand

#### Have you converted to AndroidX? - [ ] my application is an AndroidX application? - [ ] I am using `android/gradle.settings` `jetifier=true` for Android compatibility? - [ ] I am using the NPM package `jetifier` for react-native compatibility? #### `android/build.gradle`: ```groovy // N/A ``` #### `android/app/build.gradle`: ```groovy // N/A ``` #### `android/settings.gradle`: ```groovy // N/A ``` #### `MainApplication.java`: ```java // N/A ``` #### `AndroidManifest.xml`: ```xml ```


Environment

Click To Expand

**`react-native info` output:** ``` OUTPUT GOES HERE ``` - **Platform that you're experiencing the issue on**: - [x] iOS - [ ] Android - [ ] **iOS** but have not tested behavior on Android - [ ] **Android** but have not tested behavior on iOS - [ ] Both - **`react-native-firebase` version you're using that has this issue:** - `18.3.2-21.4.0` - **`Firebase` module(s) you're using that has the issue:** - `Database` - **Are you using `TypeScript`?** - `Y` & `^5.0.4`


Stas-Buzunko commented 4 days ago
firebase
    .database()
    .ref('some-path')
    .update({ someKey: null }); // Doesn't work as expected

firebase
    .database()
    .ref('some-path')
    .set(null); // Works fine
russellwheatley commented 3 days ago

Hey @Stas-Buzunko - I just wrote a test for this use case and it passed across all platforms: https://github.com/invertase/react-native-firebase/pull/8146

Also tested locally and it worked. Not sure why it doesn't work for you, it is strange. I had a look at JS code/iOS code and we do nothing to null value properties.

I did notice in your first example, it is executing on the parent node, not sure if that makes a difference but something to double check.

Stas-Buzunko commented 2 days ago

hi @russellwheatley

  1. This is a narrow issue. We've had ~1500 new users in the last 2 months, with 70% on iOS (~1050), and only 9 (0.85%) faced the problem.

  2. We validated this by creating a Firebase function that logged those 9 affected users.

Now, another developer and I can reproduce the issue (different machines) on iOS simulator 18.1 (Version 16.0 (1038), SimulatorKit 942, CoreSimulator 987.2, Xcode 16.1). I believe it’s related to the iOS version, not RN Firebase or Firebase iOS SDK; otherwise, more users would be impacted.

What’s concerning is seeing this issue on the latest simulator. The key takeaway is that we can't rely on "null" to delete keys in critical situations. We'll refactor all "null" usages to use set() instead.

Screenshot 2024-11-20 at 2 33 25 PM
Stas-Buzunko commented 2 days ago

I can record a video demonstrating the problem if you'd like, and if it would be helpful.

mikehardy commented 2 days ago

🤔 hmm - what platform are you all on where you reproduce it? Do you reproduce it every time or is there something you have to do to reproduce it reliably?

Our CI (where this test ran, but once and only once) is running:

If we can reliably reproduce this we may be able to fix it but reproduction here seems like it should be the focus first