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

Align DataSnapshot methods to @firebase/database DataSnapshot #926

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
143 changes: 134 additions & 9 deletions spec/v1/providers/database.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -541,17 +541,52 @@ describe('Database Functions', () => {
expect(subject.val()).to.equal(0);
populate({ myKey: 0 });
expect(subject.val()).to.deep.equal({ myKey: 0 });

// Null values are still reported as null.
populate({ myKey: null });
expect(subject.val()).to.deep.equal({ myKey: null });
Comment on lines -544 to -547
Copy link
Contributor Author

@rhodgkins rhodgkins Jul 31, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

});

// Regression test: .val() was returning array of nulls when there's a property called length (BUG#37683995)
it('should return correct values when data has "length" property', () => {
populate({ length: 3, foo: 'bar' });
expect(subject.val()).to.deep.equal({ length: 3, foo: 'bar' });
});

it('should deal with null-values appropriately', () => {
populate(null);
expect(subject.val()).to.be.null;

populate({ myKey: null });
expect(subject.val()).to.be.null;
});

it('should deal with empty object values appropriately', () => {
populate({});
expect(subject.val()).to.be.null;

populate({ myKey: {} });
expect(subject.val()).to.be.null;

populate({ myKey: { child: null } });
expect(subject.val()).to.be.null;
});

it('should deal with empty array values appropriately', () => {
populate([]);
expect(subject.val()).to.be.null;

populate({ myKey: [] });
expect(subject.val()).to.be.null;

populate({ myKey: [null] });
expect(subject.val()).to.be.null;

populate({ myKey: [{}] });
expect(subject.val()).to.be.null;

populate({ myKey: [{ myKey: null }] });
expect(subject.val()).to.be.null;

populate({ myKey: [{ myKey: {} }] });
expect(subject.val()).to.be.null;
});
});

describe('#child(): DataSnapshot', () => {
Expand All @@ -578,14 +613,37 @@ describe('Database Functions', () => {
});

it('should be false for a non-existent value', () => {
populate({ a: { b: 'c' } });
populate({ a: { b: 'c', nullChild: null } });
expect(subject.child('d').exists()).to.be.false;
expect(subject.child('nullChild').exists()).to.be.false;
});

it('should be false for a value pathed beyond a leaf', () => {
populate({ a: { b: 'c' } });
expect(subject.child('a/b/c').exists()).to.be.false;
});

it('should be false for an empty object value', () => {
populate({ a: {} });
expect(subject.child('a').exists()).to.be.false;

populate({ a: { child: null } });
expect(subject.child('a').exists()).to.be.false;

populate({ a: { child: {} } });
expect(subject.child('a').exists()).to.be.false;
});

it('should be false for an empty array value', () => {
populate({ a: [] });
expect(subject.child('a').exists()).to.be.false;

populate({ a: [null] });
expect(subject.child('a').exists()).to.be.false;

populate({ a: [{}] });
expect(subject.child('a').exists()).to.be.false;
});
});

describe('#forEach(action: (a: DataSnapshot) => boolean): boolean', () => {
Expand Down Expand Up @@ -614,6 +672,17 @@ describe('Database Functions', () => {

expect(subject.forEach(counter)).to.equal(false);
expect(count).to.eq(0);

populate({
a: 'foo',
nullChild: null,
emptyObjectChild: {},
emptyArrayChild: [],
});
count = 0;

expect(subject.forEach(counter)).to.equal(false);
expect(count).to.eq(1);
});

it('should cancel further enumeration if callback returns true', () => {
Expand Down Expand Up @@ -653,13 +722,51 @@ describe('Database Functions', () => {

describe('#numChildren()', () => {
it('should be key count for objects', () => {
populate({ a: 'b', c: 'd' });
populate({
a: 'b',
c: 'd',
nullChild: null,
emptyObjectChild: {},
emptyArrayChild: [],
});
expect(subject.numChildren()).to.eq(2);
});

it('should be 0 for non-objects', () => {
populate(23);
expect(subject.numChildren()).to.eq(0);

populate({
nullChild: null,
emptyObjectChild: {},
emptyArrayChild: [],
});
expect(subject.numChildren()).to.eq(0);
});
});

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;
Comment on lines +748 to +769
Copy link
Contributor Author

@rhodgkins rhodgkins Jul 31, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

});
});

Expand All @@ -671,9 +778,17 @@ describe('Database Functions', () => {
});

it('should return false if a child is missing', () => {
populate({ a: 'b' });
populate({
a: 'b',
nullChild: null,
emptyObjectChild: {},
emptyArrayChild: [],
});
expect(subject.hasChild('c')).to.be.false;
expect(subject.hasChild('a/b')).to.be.false;
expect(subject.hasChild('nullChild')).to.be.false;
expect(subject.hasChild('emptyObjectChild')).to.be.false;
expect(subject.hasChild('emptyArrayChild')).to.be.false;
});
});

Expand Down Expand Up @@ -703,11 +818,21 @@ describe('Database Functions', () => {

describe('#toJSON(): Object', () => {
it('should return the current value', () => {
populate({ a: 'b' });
populate({
a: 'b',
nullChild: null,
emptyObjectChild: {},
emptyArrayChild: [],
});
expect(subject.toJSON()).to.deep.equal(subject.val());
});
it('should be stringifyable', () => {
populate({ a: 'b' });
populate({
a: 'b',
nullChild: null,
emptyObjectChild: {},
emptyArrayChild: [],
});
expect(JSON.stringify(subject)).to.deep.equal('{"a":"b"}');
});
});
Expand Down
29 changes: 24 additions & 5 deletions src/v1/providers/database.ts
Original file line number Diff line number Diff line change
Expand Up @@ -471,7 +471,16 @@ export class DataSnapshot {
* @return `true` if this `DataSnapshot` contains any data; otherwise, `false`.
*/
exists(): boolean {
return !_.isNull(this.val());
const val = this.val();
if (_.isNull(val)) {
// Null value
return false;
}
if (_.isObjectLike(val) && _.isEmpty(val)) {
// Empty object/array
return false;
}
return true;
}

/**
Expand Down Expand Up @@ -512,7 +521,7 @@ export class DataSnapshot {
*/
forEach(action: (a: DataSnapshot) => boolean | void): boolean {
const val = this.val();
if (_.isPlainObject(val)) {
if (_.isObjectLike(val)) {
return _.some(
val,
(value, key: string) => action(this.child(key)) === true
Expand Down Expand Up @@ -546,7 +555,7 @@ export class DataSnapshot {
*/
hasChildren(): boolean {
const val = this.val();
return _.isPlainObject(val) && _.keys(val).length > 0;
return _.isObjectLike(val) && !_.isEmpty(val);
}

/**
Expand All @@ -556,7 +565,7 @@ export class DataSnapshot {
*/
numChildren(): number {
const val = this.val();
return _.isPlainObject(val) ? Object.keys(val).length : 0;
return _.isObjectLike(val) ? _.keys(val).length : 0;
}

/**
Expand Down Expand Up @@ -588,7 +597,12 @@ export class DataSnapshot {
continue;
}
const childNode = node[key];
obj[key] = this._checkAndConvertToArray(childNode);
const v = this._checkAndConvertToArray(childNode);
if (v === null) {
// Empty child node
continue;
}
obj[key] = v;
numKeys++;
const integerRegExp = /^(0|[1-9]\d*)$/;
if (allIntegerKeys && integerRegExp.test(key)) {
Expand All @@ -598,6 +612,11 @@ export class DataSnapshot {
}
}

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

Comment on lines +615 to +619
Copy link
Contributor Author

@rhodgkins rhodgkins Jul 31, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (allIntegerKeys && maxKey < 2 * numKeys) {
// convert to array.
const array: any = [];
Expand Down