Align DataSnapshot methods to @firebase/database DataSnapshot #926

rhodgkins · 2021-07-31T13:50:42Z

Description

I've noticed[1] a difference between DataSnapshots methods in this SDK compared to the one used when fetching data (when using the admin SDK).

When setting values that are (or contain) empty object/arrays and null values using the admin SDK, when fetching these values back, they're always null when calling .val().
However when creating a DataSnapshot with these values and calling .val() the results don't match.
I'm only raising this as an issue because when the functions are being called, I'd expect the DataSnapshot provided to the function handler to match the DataSnapshot when fetched from the same location - currently this isn't the case.

Code sample

Using the following data: { child: 'foo', nullChild: null, emptyChild: {}, emptyArray: [], child: { nullVal: null, key: 'bar' } }

Setting and then getting this value using the admin SDK [2] results in { child1: 'foo', child2: { key: 'bar } } when calling .val(), which is what you'd expect - all the empty/null nodes are dropped.

However using the firebase-functions database.DataSnapshot constructor the result includes these empty/null values { child: 'foo', nullChild: null, emptyChild: {}, emptyArray: {}, child: { nullVal: null, key: 'bar' } } when calling .val().

I think this is mostly due to the lack of this check the @firebase/database does when calling .val().

The discrepancies are mainly when using .val() but I've updated the other methods and their tests (.exists(), .numChildren(), .hasChildren(), etc.) for completeness...

[1] noticed when using firebase-functions-test makeDataSnapshot method (which calls this repos database.DataSnapshot constructor) for mocking parameters for testing a Firebase Function.

[2] By using:

const firebase = require('firebase-admin')
firebase.initializeApp({ databaseURL: 'https://<databaseName>.firebaseio.com' })

const setAndGet = async val => {
   const ref = firebase.database().ref('node')
   
   await ref.set(val)
   return await ref.get()
}

rhodgkins · 2021-07-31T13:58:48Z

spec/v1/providers/database.spec.ts

-
-        // Null values are still reported as null.
-        populate({ myKey: null });
-        expect(subject.val()).to.deep.equal({ myKey: null });


I've removed this test case as its incorrect compared to the Firebase SDK, the correct version is now in should deal with null-values appropriately.

rhodgkins · 2021-07-31T14:01:48Z

src/v1/providers/database.ts

+    if (numKeys === 0) {
+      // Empty node
+      return null;
+    }
+


This simulates the previously mentioned this.isEmpty() check the Firebase SDK does.

rhodgkins · 2021-07-31T14:02:40Z

spec/v1/providers/database.spec.ts

+    describe('#hasChildren()', () => {
+      it('should true for objects', () => {
+        populate({
+          a: 'b',
+          c: 'd',
+          nullChild: null,
+          emptyObjectChild: {},
+          emptyArrayChild: [],
+        });
+        expect(subject.hasChildren()).to.be.true;
+      });
+
+      it('should be false for non-objects', () => {
+        populate(23);
+        expect(subject.hasChildren()).to.be.false;
+
+        populate({
+          nullChild: null,
+          emptyObjectChild: {},
+          emptyArrayChild: [],
+        });
+        expect(subject.hasChildren()).to.be.false;


.hasChildren() had no tests so added some by mirroring the .numChildren() tests above.

…base

rhodgkins · 2021-08-13T09:42:34Z

@inlined due to #943 this PR needs a bit of an update - shall I rebase off master and do it against the old non-v1 directory structure?

inlined · 2021-08-13T17:55:46Z

Let me get back to you. We got rid of the v1 directory temporarily to unbreak the emulator and people depending on undocumented behavior. We want to move everything back ASAP by releasing a breaking change. That breaking change might actually be the best time to accept your change since it's a good change but changes behavior notably.

rhodgkins · 2021-08-25T12:03:49Z

Let me get back to you. We got rid of the v1 directory temporarily to unbreak the emulator and people depending on undocumented behavior. We want to move everything back ASAP by releasing a breaking change. That breaking change might actually be the best time to accept your change since it's a good change but changes behavior notably.

Any update on this @inlined ?

rhodgkins · 2021-10-25T08:21:24Z

Bump @inlined

inlined · 2021-10-25T23:40:49Z

Haven't forgotten about you. We just haven't made a breaking change yet. Your PR is actually listed on an internal checklist for changes to be made before our next breaking change.

rhodgkins · 2021-10-26T06:37:56Z

Thanks (and sorry for the pestering then)! Shall I rebase (/keep rebasing) or wait till you guys are ready and give me a bump?

inlined · 2021-10-27T17:33:40Z

I don't want to waste your time chasing after a moving target. I'll reach out or rebase myself.

inlined · 2022-06-28T04:16:55Z

Closing this since the changes have been ported to the launch.next branch and will be included in the v4 SDK. Thank you very much for both your technical contribution and your patience.

admin-01-web · 2022-11-06T09:38:33Z

[gh pr checkout 926]

admin-01-web · 2022-11-06T09:39:12Z

@admin-01-web

admin-01-web · 2022-11-06T09:39:38Z

#1298

google-cla bot added the cla: yes label Jul 31, 2021

rhodgkins force-pushed the datasnapshot-null-children branch from 9ec3562 to ec514e2 Compare July 31, 2021 13:55

rhodgkins commented Jul 31, 2021

View reviewed changes

rhodgkins force-pushed the datasnapshot-null-children branch 2 times, most recently from 3dde778 to fae1bf0 Compare August 11, 2021 08:17

rhodgkins added 3 commits August 12, 2021 13:02

Test cases for DataSnapshot to align methods closer to @firebase/data…

dfd1708

…base

Aligned DataSnapshot methods to @firebase/database counterpart

92597f0

Removed unneeded use of _.isArray with _.isObjectLike

f08be25

rhodgkins force-pushed the datasnapshot-null-children branch from fae1bf0 to f08be25 Compare August 12, 2021 12:02

rhodgkins added a commit to bookcreator/firebase-functions-test that referenced this pull request Aug 13, 2021

Failing test case for firebase/firebase-functions#926

3648691

rhodgkins mentioned this pull request Aug 13, 2021

Failing test case for firebase/firebase-functions#926 firebase/firebase-functions-test#116

Draft

rhodgkins added a commit to bookcreator/firebase-functions-test that referenced this pull request Aug 19, 2021

Failing test case for firebase/firebase-functions#926

f98db28

rhodgkins referenced this pull request Jun 8, 2022

adding in changes from @rhodgkins to match the admin SDK

1fc1d33

inlined closed this Jun 28, 2022

rhodgkins added a commit to bookcreator/firebase-functions-test that referenced this pull request Aug 9, 2022

Failing test case for firebase/firebase-functions#926

5753334

taeold added this to the v4 milestone Aug 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Align DataSnapshot methods to @firebase/database DataSnapshot #926

Align DataSnapshot methods to @firebase/database DataSnapshot #926

rhodgkins commented Jul 31, 2021 •

edited

Loading

rhodgkins Jul 31, 2021 •

edited

Loading

rhodgkins Jul 31, 2021 •

edited

Loading

rhodgkins Jul 31, 2021 •

edited

Loading

rhodgkins commented Aug 13, 2021

inlined commented Aug 13, 2021

rhodgkins commented Aug 25, 2021

rhodgkins commented Oct 25, 2021

inlined commented Oct 25, 2021

rhodgkins commented Oct 26, 2021

inlined commented Oct 27, 2021

inlined commented Jun 28, 2022

admin-01-web commented Nov 6, 2022

admin-01-web commented Nov 6, 2022

admin-01-web commented Nov 6, 2022

Align DataSnapshot methods to @firebase/database DataSnapshot #926

Align DataSnapshot methods to @firebase/database DataSnapshot #926

Conversation

rhodgkins commented Jul 31, 2021 • edited Loading

Description

Code sample

rhodgkins Jul 31, 2021 • edited Loading

Choose a reason for hiding this comment

rhodgkins Jul 31, 2021 • edited Loading

Choose a reason for hiding this comment

rhodgkins Jul 31, 2021 • edited Loading

Choose a reason for hiding this comment

rhodgkins commented Aug 13, 2021

inlined commented Aug 13, 2021

rhodgkins commented Aug 25, 2021

rhodgkins commented Oct 25, 2021

inlined commented Oct 25, 2021

rhodgkins commented Oct 26, 2021

inlined commented Oct 27, 2021

inlined commented Jun 28, 2022

admin-01-web commented Nov 6, 2022

admin-01-web commented Nov 6, 2022

admin-01-web commented Nov 6, 2022

rhodgkins commented Jul 31, 2021 •

edited

Loading

rhodgkins Jul 31, 2021 •

edited

Loading

rhodgkins Jul 31, 2021 •

edited

Loading

rhodgkins Jul 31, 2021 •

edited

Loading