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

Closed
2 of 10 tasks
Stas-Buzunko opened this issue Nov 18, 2024 · 26 comments · Fixed by #8269
Closed
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 · 26 comments · Fixed by #8269
Assignees
Labels
platform: ios plugin: database Firebase Realtime Database type: bug New bug report

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

@Stas-Buzunko
Copy link
Contributor Author

@mikehardy

@jpancotti
Copy link

I found that disabling bridgeless mode prevents null values from being dropped in update() calls. Adding this to AppDelegate.mm resolves the issue:
(BOOL)bridgelessEnabled { return NO; }
This allows update() to work as expected with null values without requiring refactoring to set() or remove(). While not an ideal long-term solution, it provides a workable temporary fix until the underlying bridge issue is resolved.

@mikehardy
Copy link
Collaborator

That explains why we couldn't reproduce this in repo with #8146 - that was really confusing me and made it hard to figure out + fix - our e2e test app is a bit behind in react-native terms and doesn't have new arch + bridgeless mode enabled yet (I've been doing verification of basic compatibility with my make-demo script)

Great insight @jpancotti thank you

@mikehardy
Copy link
Collaborator

We have progress here! Took a while but our e2e test app is using new arch now, which perfectly exposes this (real!) problem, as expected after the insight that it was new arch specific from @jpancotti

I've opened #8269 as a branch / PR which doesn't have a fix yet but it has test failures because of this issue (and some similar null-handling-in-new-arch issues...), and the expectation is that PR will gain a couple commits that fix it.

cipolleschi added a commit to cipolleschi/react-native that referenced this issue Feb 7, 2025
Summary:
The TurboModule System decided to ignore the Null values when they are coming to JS. However, in iOS, null value can be mapped to `[NSNull null];` and this value is a valid value that can be used on the native side.

In the old architecture, when the user were sending a null value from JS to a native module, the Native side was receiving the value.

In the New Architecture, the value was stripped away.

This change allow us to handle the `null` value properly and reduce one of the Gaps between the old and the new architecture.

See discussion happening here: invertase/react-native-firebase#8144 (comment)

This behavior was breaking some libraries that relies on checking wheter some values were null or not.

## Changelog:
[iOS][Changed] - Properly handle `null` values coming from NativeModules.

Differential Revision: D69301396
@mikehardy mikehardy added blocked: react-native Issue or PR blocked by a React Native issue or change. and removed Workflow: Needs Review Pending feedback or review from a maintainer. labels Feb 7, 2025
cipolleschi added a commit to cipolleschi/react-native that referenced this issue Feb 7, 2025
Summary:

The TurboModule System decided to ignore the Null values when they are coming to JS. However, in iOS, null value can be mapped to `[NSNull null];` and this value is a valid value that can be used on the native side.

In the old architecture, when the user were sending a null value from JS to a native module, the Native side was receiving the value.

In the New Architecture, the value was stripped away.

This change allow us to handle the `null` value properly in the interop layer, to restore the usage of legacy modules in the New Arch.

I also tried with a more radical approach, but several tests were crashing because some modules do not know how to handle `NSNull`.

See discussion happening here: invertase/react-native-firebase#8144 (comment)

## Changelog:
[iOS][Changed] - Properly handle `null` values coming from NativeModules.

Differential Revision: D69301396
@mikehardy
Copy link
Collaborator

@Stas-Buzunko just wanted to give a thanks for taking the time to work with Cursor and really dig in here, then share the results. It's definitely an upstream issue and the info you provided made generating a reproducer app really easy for me, which then makes it much easier for upstream to create a fix. We're close on this one thanks to that help

cipolleschi added a commit to cipolleschi/react-native that referenced this issue Feb 7, 2025
Summary:

The TurboModule System decided to ignore the Null values when they are coming to JS. However, in iOS, null value can be mapped to `[NSNull null];` and this value is a valid value that can be used on the native side.

In the old architecture, when the user were sending a null value from JS to a native module, the Native side was receiving the value.

In the New Architecture, the value was stripped away.

This change allow us to handle the `null` value properly in the interop layer, to restore the usage of legacy modules in the New Arch.

I also tried with a more radical approach, but several tests were crashing because some modules do not know how to handle `NSNull`.

See discussion happening here: invertase/react-native-firebase#8144 (comment)

## Changelog:
[iOS][Changed] - Properly handle `null` values coming from NativeModules.

Differential Revision: D69301396
cipolleschi added a commit to cipolleschi/react-native that referenced this issue Feb 7, 2025
Summary:

The TurboModule System decided to ignore the Null values when they are coming to JS. However, in iOS, null value can be mapped to `[NSNull null];` and this value is a valid value that can be used on the native side.

In the old architecture, when the user were sending a null value from JS to a native module, the Native side was receiving the value.

In the New Architecture, the value was stripped away.

This change allow us to handle the `null` value properly in the interop layer, to restore the usage of legacy modules in the New Arch.

I also tried with a more radical approach, but several tests were crashing because some modules do not know how to handle `NSNull`.

See discussion happening here: invertase/react-native-firebase#8144 (comment)

## Changelog:
[iOS][Changed] - Properly handle `null` values coming from NativeModules.

Differential Revision: D69301396
@mikehardy
Copy link
Collaborator

Upstream has fixes in review now

This patch for react-native 0.77 will fix things for you https://github.com/mikehardy/react-native-new-arch-ios-null-handling/blob/main/ReproducerApp/patches/react-native%2B0.77.0.patch

This patch is the same but setup for react-native 0.76 (just a tiny bit different): 2d1ee33#diff-ae8dcc40dc6123a31257293bd95f339ce52baad4f42621fd749f262d4ee89d4a

facebook-github-bot pushed a commit to facebook/react-native that referenced this issue Feb 10, 2025
Summary:
Pull Request resolved: #49250

The TurboModule System decided to ignore the Null values when they are coming to JS. However, in iOS, null value can be mapped to `[NSNull null];` and this value is a valid value that can be used on the native side.

In the old architecture, when the user were sending a null value from JS to a native module, the Native side was receiving the value.

In the New Architecture, the value was stripped away.

This change allow us to handle the `null` value properly in the interop layer, to restore the usage of legacy modules in the New Arch.

I also tried with a more radical approach, but several tests were crashing because some modules do not know how to handle `NSNull`.

See discussion happening here: invertase/react-native-firebase#8144 (comment)

## Changelog:
[iOS][Changed] - Properly handle `null` values coming from NativeModules.

Reviewed By: sammy-SC

Differential Revision: D69301396

fbshipit-source-id: be275185e2643092f6c3dc2481fe9381bbcf69e9
@mikehardy
Copy link
Collaborator

mikehardy commented Feb 10, 2025

upstream has merged the patch as-is from my last comment, I would be comfortable incorporating the patch into my release builds until a new release from react-native has them. (we are doing so internally in our e2e app)

I project that 0.77.1 and 0.78.0 will be the first releases to incorporate the fix, unsure if the 0.76.x series will see it.

@mikehardy mikehardy removed blocked: react-native Issue or PR blocked by a React Native issue or change. Keep Open avoids the stale bot Needs Attention labels Feb 10, 2025
robhogan pushed a commit to facebook/react-native that referenced this issue Feb 11, 2025
Summary:
Pull Request resolved: #49250

The TurboModule System decided to ignore the Null values when they are coming to JS. However, in iOS, null value can be mapped to `[NSNull null];` and this value is a valid value that can be used on the native side.

In the old architecture, when the user were sending a null value from JS to a native module, the Native side was receiving the value.

In the New Architecture, the value was stripped away.

This change allow us to handle the `null` value properly in the interop layer, to restore the usage of legacy modules in the New Arch.

I also tried with a more radical approach, but several tests were crashing because some modules do not know how to handle `NSNull`.

See discussion happening here: invertase/react-native-firebase#8144 (comment)

## Changelog:
[iOS][Changed] - Properly handle `null` values coming from NativeModules.

Reviewed By: sammy-SC

Differential Revision: D69301396

fbshipit-source-id: be275185e2643092f6c3dc2481fe9381bbcf69e9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
platform: ios plugin: database Firebase Realtime Database type: bug New bug report
Projects
None yet
5 participants