Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
2 of 10 tasks
Stas-Buzunko opened this issue Nov 18, 2024 · 19 comments · May be fixed by #8146
Open
2 of 10 tasks

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

Stas-Buzunko opened this issue Nov 18, 2024 · 19 comments · May be fixed by #8146
Assignees
Labels
Keep Open avoids the stale bot Needs Attention platform: ios plugin: database Firebase Realtime Database type: bug New bug report Workflow: Needs Review Pending feedback or review from a maintainer.

Comments

@Stas-Buzunko
Copy link
Contributor

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:

# N/A

firebase.json for react-native-firebase v6:

# N/A

iOS

Click To Expand

ios/Podfile:

  • I'm not using Pods
  • I'm using Pods and my Podfile looks like:
# N/A

AppDelegate.m:

// 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:

// N/A

android/app/build.gradle:

// N/A

android/settings.gradle:

// N/A

MainApplication.java:

// N/A

AndroidManifest.xml:

<!-- N/A -->


Environment

Click To Expand

react-native info output:

 OUTPUT GOES HERE
  • Platform that you're experiencing the issue on:
    • 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
Copy link
Contributor Author

Stas-Buzunko commented Nov 18, 2024

firebase
    .database()
    .ref('some-path')
    .update({ someKey: null }); // Doesn't work as expected

firebase
    .database()
    .ref('some-path/someKey')
    .set(null); // Works fine

@russellwheatley russellwheatley self-assigned this Nov 19, 2024
@russellwheatley
Copy link
Member

Hey @Stas-Buzunko - I just wrote a test for this use case and it passed across all platforms: #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
Copy link
Contributor Author

Stas-Buzunko commented Nov 20, 2024

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
Copy link
Contributor Author

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

@mikehardy
Copy link
Collaborator

🤔 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

@Stas-Buzunko
Copy link
Contributor Author

Stas-Buzunko commented Dec 15, 2024

Facing this issue on TestFlight using two of our testing devices:

  • iOS 16.7.10 - iPhone 8
  • iOS 17.6.1 - iPhone 11

Can reproduce locally using our repo:

  • Sonoma 15.1.1
  • Xcode 16.2
  • iOS 18.2
Screenshot 2024-12-15 at 9 49 28 PM Screenshot 2024-12-15 at 9 52 57 PM

both result in

Screenshot 2024-12-15 at 9 49 37 PM

changing value to something else - works fine

Screenshot 2024-12-15 at 9 53 50 PM Screenshot 2024-12-15 at 9 53 54 PM

@Stas-Buzunko
Copy link
Contributor Author

i can also reproduce using bare https://github.com/mikehardy/rnfbdemo

Screenshot 2024-12-15 at 11 14 11 PM Screenshot 2024-12-15 at 11 14 35 PM

@Stas-Buzunko
Copy link
Contributor Author

by adding


const testFn = async () => {
  const testingData = {
    boolean: true,
    number: 12345,
    string: 'safsdf',
  };

  try {
    await firebase.database().ref('testing-null-removal').set(testingData);
    console.log('data set');

    const updates = {};

    updates['testing-null-removal/boolean'] = null;
    updates['testing-null-removal/number'] = null;
    updates['testing-null-removal/string'] = null;

    await firebase.database().ref().update(updates);

    console.log('data updated', updates);

    const dataLeft = await firebase
      .database()
      .ref('testing-null-removal')
      .once('value');

    console.log(dataLeft.val());
  } catch (error) {
    console.log(error);
  }
};

testFn();

before
AppRegistry.registerComponent(appName, () => App);

@Stas-Buzunko
Copy link
Contributor Author

@Stas-Buzunko
Copy link
Contributor Author

i've pushed the reproduction repo
https://github.com/Stas-Buzunko/rnfbdemo/tree/main/rnfbdemo
https://github.com/Stas-Buzunko/rnfbdemo/blob/main/rnfbdemo/index.js

@Stas-Buzunko
Copy link
Contributor Author

Stas-Buzunko commented Dec 15, 2024

Ruby 3.1.1p18
CocoaPods - your reproduction repo 1.15.2, our repo 1.16.2
Node.js v18.20.4
Macbook air m1 / also happens on m2

@Stas-Buzunko
Copy link
Contributor Author

We are going to be refactoring all places where we set nulls via updates. Good news: there are not many of them! 😅

@Stas-Buzunko
Copy link
Contributor Author

Let me know if there is a way I can provide more details that might help.

@mikehardy
Copy link
Collaborator

@Stas-Buzunko the reproducer you posted is great - reproducible examples are usually the key to unlock a fix here. There is a deep queue of work in the repo though unfortunately (new architecture support, typescript, deprecate the old non-modular APIs, add support for the new Vertex AI module, etc...), so I will mark this as keep open so it doesn't get auto-closed but an actual fix will require patience unless you pursue it yourself and post a PR. With a reproducer there is hope though either way, much appreciated

@mikehardy mikehardy added Keep Open avoids the stale bot Workflow: Needs Review Pending feedback or review from a maintainer. labels Dec 16, 2024
@Stas-Buzunko
Copy link
Contributor Author

Cursor AI seems to have found the root cause! @mikehardy

It asked me to add the following logs:

// In RNFBDatabaseReferenceModule.m
NSDictionary *values = [props valueForKey:@"values"];
NSLog(@"Update values received: %@", values);

// In FSyncTree.m or FRepo.m where the compound write is created
NSLog(@"Compound write values: %@", changedChildren);

// In FSyncPoint.m where operations are applied
NSLog(@"Operation type: %@, values: %@", operation, values);

And here are the logs from Xcode:

Operation type: FOverwrite, source: FOperationSource { fromUser=0, fromServer=1, queryId=(null), tagged=0 }
data set
Update values received: {
    "testing-null-removal/number" = 123456;
}
Compound write values: {
    "testing-null-removal/number" = 123456;
}
'data updated', { 'testing-null-removal/boolean': null,
  'testing-null-removal/number': 123456,
  'testing-null-removal/string': null }
Operation type: FOverwrite, source: FOperationSource { fromUser=0, fromServer=1, queryId=(null), tagged=0 }
{ boolean: true, number: 123456, string: 'safsdf' }

Here are Cursor AI's thoughts on these logs:

This log output reveals something very interesting about what's happening:

  1. When the update is sent from JS:
updates['testing-null-removal/boolean'] = null;
updates['testing-null-removal/number'] = 123456;
updates['testing-null-removal/string'] = null;
  1. But in the native bridge, only the non-null value makes it through:
Update values received: {
    "testing-null-removal/number" = 123456;
}
  1. Similarly, in the compound write:
Compound write values: {
    "testing-null-removal/number" = 123456;
}

This strongly suggests that the null values are being filtered out somewhere between the JavaScript layer and the native bridge. The fact that only the non-null value (123456) makes it through explains why the null updates aren't working.

The issue appears to be in the bridge layer, specifically in how null values are handled when converting from JavaScript to native code. This explains why:

  • set(null) works (different code path)
  • Only non-null values in updates get through
  • The final state still has all values

====

AI:

Based on these files and our earlier findings, let's modify the update method in RNFBDatabaseReferenceModule.m. Here's a more complete solution that should properly handle null values:

The key changes are:

  1. We're explicitly handling both [NSNull null] and nil values by converting them to [NSNull null].
  2. We're using a mutable dictionary to build up our processed values.
  3. We're adding logging to track the values before and after processing.
  4. We're ensuring the processed values maintain the null values when passed to updateChildValues:.

Me: This resulted in the same logs.

AI:

This suggests the null values are being filtered out before they even reach our native code. The issue is likely in how React Native's bridge handles the conversion of the JavaScript object to native values.

Let's try a different approach. Instead of sending null values directly, we could:

  1. Modify the JavaScript code to use a special marker value that won't get filtered by the bridge, then convert it back to null in native code, or
  2. Split the update into two operations: one for non-null values and one specifically for nulls.
const testFn = async () => {
  const testingData = {
    boolean: true,
    number: 12345,
    string: 'safsdf'
  }

  try {
    await firebase.database().ref('testing-null-removal').set(testingData);
    console.log('data set');

    const updates = {};

    // Use "__FIREBASE_NULL__" as a marker for null values
    updates['testing-null-removal/boolean'] = '__FIREBASE_NULL__';
    updates['testing-null-removal/number'] = 123456;
    updates['testing-null-removal/string'] = null;

    await firebase.database().ref().update(updates);

    console.log('data updated', updates);

    const dataLeft = await firebase
      .database()
      .ref('testing-null-removal')
      .once('value');

    console.log(dataLeft.val());
  } catch (error) {
    console.log(error);
  }
}

testFn();

In RNFBDatabaseReferenceModule.m:

RCT_EXPORT_METHOD(update
                  : (FIRApp *)firebaseApp
                  : (NSString *)dbURL
                  : (NSString *)path
                  : (NSDictionary *)props
                  : (RCTPromiseResolveBlock)resolve
                  : (RCTPromiseRejectBlock)reject) {
  NSDictionary *originalValues = [props valueForKey:@"values"];
  NSLog(@"Original update values received: %@", originalValues);
  
  // Create a mutable dictionary that will preserve null values
  NSMutableDictionary *processedValues = [NSMutableDictionary dictionaryWithCapacity:[originalValues count]];
  
  // Explicitly process each value, converting markers to null
  [originalValues enumerateKeysAndObjectsUsingBlock:^(id key, id obj, BOOL *stop) {
      if ([obj isKindOfClass:[NSString class]] && [obj isEqualToString:@"__FIREBASE_NULL__"]) {
          [processedValues setObject:[NSNull null] forKey:key];
      } else {
          [processedValues setObject:obj forKey:key];
      }
  }];
  
  NSLog(@"Processed update values: %@", processedValues);
  
  FIRDatabase *firDatabase = [RNFBDatabaseCommon getDatabaseForApp:firebaseApp dbURL:dbURL];
  FIRDatabaseReference *firDatabaseReference =
      [RNFBDatabaseCommon getReferenceForDatabase:firDatabase path:path];

  [firDatabaseReference updateChildValues:processedValues
                      withCompletionBlock:^(NSError *error, FIRDatabaseReference *ref) {
                        if (error != nil) {
                          [RNFBDatabaseCommon promiseRejectDatabaseException:reject error:error];
                        } else {
                          resolve([NSNull null]);
                        }
                      }];
}

Me: This worked!

AI: You should report this issue to @react-native-firebase/database since it's a bug in how their bridge handles null values in updates. Here's why:

  1. The issue is specific to the React Native Firebase Database implementation.
  2. The workaround needed to be implemented in both the JavaScript layer and the native module of this package.
  3. Other Firebase operations (like set(null)) work correctly, suggesting it's not a core React Native or Firebase iOS SDK issue.

@Stas-Buzunko
Copy link
Contributor Author

Stas-Buzunko commented Dec 17, 2024

both .update(obj) and .set(obj) seem to be affected on iOS

await firebase.database().ref('testing-null-removal').set({
    boolean: '__FIREBASE_NULL__',
    number: 123456,
    string: null
  })

Operation type: FOverwrite, source: FOperationSource { fromUser=0, fromServer=1, queryId=(null), tagged=0 }
data set
Original update values received: {
    "testing-null-removal/boolean" = "__FIREBASE_NULL__";
    "testing-null-removal/number" = 123456;
}
Processed update values: {
    "testing-null-removal/boolean" = "<null>";
    "testing-null-removal/number" = 123456;
}
Compound write values: {
    "testing-null-removal/boolean" = "<null>";
    "testing-null-removal/number" = 123456;
}
'data updated', { 'testing-null-removal/boolean': '__FIREBASE_NULL__',
  'testing-null-removal/number': 123456,
  'testing-null-removal/string': null }
Operation type: FOverwrite, source: FOperationSource { fromUser=0, fromServer=1, queryId=(null), tagged=0 }
{ number: 123456, string: 'safsdf' }

@Stas-Buzunko
Copy link
Contributor Author

Stas-Buzunko commented Dec 17, 2024

Changing to update on a child node instead of a parent node has no effect. The same issue

 const updates = {
      boolean: null,
      number: null,
      string: null
    }

    await firebase.database().ref('testing-null-removal').update(updates)

@BrandonHowe
Copy link

I am getting this issue as well. Is there a workaround in place besides using .set(null)? Or do we have to wait for @react-native-firebase/database to fix their bridge?

@Stas-Buzunko
Copy link
Contributor Author

@BrandonHowe
no, there isn't :(
luckily we only had around 10 places on the mobile where we used null with update(), so it didn't take much time to refactor them

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Keep Open avoids the stale bot Needs Attention platform: ios plugin: database Firebase Realtime Database type: bug New bug report Workflow: Needs Review Pending feedback or review from a maintainer.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants