🔥[🐛] Wrong empty check in the main DatabaseReference.update call

[david-gregorian]

Issue

There is a difference between the main Firebase SDK & this implementation.
It is about the DatabaseReference.update method.
Currently this library is throwing an error when calling this function with an empty object, due to this condition:

if (!Object.keys(values).length) { throw new Error(...) }

I guess this has been implemented because of the parameter description on the firebase docs

But I think it has just been understood wrongly. The main firebase sdks are not checking for empty objects in the update call. They don't throw an error when calling update with an empty object.
This is an unexpected behavior and can lead to big issues, when sharing code between a web & react-native project.

Would be great if you could have a look :)

---

Project Files

Javascript

Click To Expand

packages/database/lib/DatabaseReference.js

 if (!Object.keys(values).length) {
      throw new Error(
        "firebase.database().ref().update(*) 'values' must be an object containing multiple values.",
     );
  }

[mikehardy]

Interesting - not saying you're wrong but it sounds a little like a pointless operation, if update only updates the key/value pairs passed...and you pass no keys...what exactly are you updating? Seems like there would be no side effect then and thus no point in calling the method?

So I guess I'm struggling to see the use case

But! If firebase-js-sdk let's empty objects through and our goal is to be a drop-in replacement I certainly want to copy their behavior.

I traced it through to this point where the update is finally about to happen, to this point no validation of the values arg has happened:

https://github.com/firebase/firebase-js-sdk/blob/d47fb414bfd9508f611a9adbc253fb360c88fbd3/packages/database/src/exp/Reference_impl.ts#L785

That will pass through most of the method if it's an empty object, at which point merge paths will be built from the keys, which will be an empty array sent through

https://github.com/firebase/firebase-js-sdk/blob/d47fb414bfd9508f611a9adbc253fb360c88fbd3/packages/database/src/core/util/validation.ts#L288

...and that will pass. https://github.com/firebase/firebase-js-sdk/blob/d47fb414bfd9508f611a9adbc253fb360c88fbd3/packages/database/src/core/util/validation.ts#L204

[david-gregorian]

Hi @mikehardy ,

thanks for the quick reply! I agree, it is pointless. We just went through a big bug-tracing mission in our project, as we have a web app, which uses the firebase-js-sdk and a RN App which uses the react-native-firebase sdk 😄

The exact same code, that we share between those projects, lead to a different behavior when connecting to firebase.
I just thought, it might be the right way, to just adapt the firebase-js-sdk way, so there is no unexpected behaviour between these sdks. Even though, sending an empty object actually has no effect.

I am pretty sure, that firebase-js-sdk allows calling the update function with an empty object. We used update with empty objects already for a long time now and also could not find any point in the firebase-js-sdk or the docs, where this is being prevented / forbidden.

[mikehardy]

Yeah - I'll post up a patch here that allows the empties through as soon as my machine recovers from updating Xcode 12.5 etc

We have plans for react-native-firebase to support react-native-web in our next major rev, delegating to firebase-js-sdk under the covers but with no reference-swapping or transpile-processing (which I do in my web+RN app right now...) so keep an eye on this space ;-)

I'll tag you in the PR once it's ready and you can grab the patch-package patches artifact off the PR Github Actions of it for testing+confirmation

[david-gregorian]

Haha alright 👍 Great, thanks a lot!

[mikehardy]

Release 11.4.1 just went out with the change related to this among other fixes + features. Enjoy! 🚀