Repository: goldibex/targaryen
Pull request #56 (2016-10-21 15:38:58)

Remove null values
Prune (nested) null values after a set/update operation.

mhuebert: Removed extraneous changes related to result.data
mhuebert: Looks like `.set` is the only place it needs to be. I think we need to retain null values in `convert` so that when we merge we know what to overwrite.
dinoboff: 👍 
dinoboff: I refactor `prune()` and made the exposed method return a copy.

https://github.com/mhuebert/targaryen/compare/prune-null-values...dinoboff:prune-null-values?expand=1

I will rebase before merging the PR.

Patch:
diff --git a/lib/rule-data-snapshot.js b/lib/rule-data-snapshot.js
index a3f96ff..150046a 100644
--- a/lib/rule-data-snapshot.js
+++ b/lib/rule-data-snapshot.js
@@ -1,17 +1,60 @@
 
 'use strict';
 
-var merge = require('lodash.mergewith');
-var helpers = require('./helpers');
+var merge = require('lodash.mergewith'),
+    helpers = require('./helpers');
+
+function literal(value, priority) {
+  return {
+    '.value': value,
+     '.priority': priority !== undefined ? priority : null
+  };
+}
+
+function isObject(obj) {
+  return (
+    obj &&
+    typeof obj == 'object' &&
+    obj.constructor === Object
+  );
+}
 
 function isEmptyObj (obj) {
-  return obj && typeof obj == 'object' && Object.keys(obj).length === 0 && obj.constructor === Object;
+  if (!isObject(obj)) {
+    return false;
+  }
+
+  const length = Object.keys(obj).length;
+
+  return length === 0 || length === 1 && obj.hasOwnProperty('.priority');
 }
 
 function coerce(v) {
   return isEmptyObj(v) ? null : v;
 }
 
+function isNullNode(node) {
+  return node == null || node['.value'] === null || isEmptyObj(node);
+}
+
+function prune(obj) {
+  if (!isObject(obj)) {
+    return obj;
+  }
+
+  const dest = Object.keys(obj).reduce(function(dest, key) {
+    const value = prune(obj[key]);
+
+    if (!isNullNode(value)) {
+      dest[key] = value;
+    }
+
+    return dest;
+  }, {});
+
+  return isNullNode(dest) ? literal(null) : dest;
+}
+
 function RuleDataSnapshot(data, pathOrNow, path) {
 
   var now = pathOrNow;
@@ -48,10 +91,7 @@ RuleDataSnapshot.convert = function(data, now) {
 
     if (typeof node !== 'object' || node === null) {
 
-      return {
-        '.value': node,
-        '.priority': null
-      };
+      return literal(node);
 
     } else if (node.hasOwnProperty('.value')) {
       return node;
@@ -60,10 +100,7 @@ RuleDataSnapshot.convert = function(data, now) {
       // server value. right now that just means timestamp
       if (node['.sv'] === 'timestamp') {
 
-        return {
-          '.value': ts,
-          '.priority': node.hasOwnProperty('.priority') ? node['.priority'] : null
-        };
+        return literal(ts, node['.priority']);
 
       } else {
         throw new Error('Unrecognized server value "' + node['.sv'] + '"');
@@ -89,6 +126,9 @@ RuleDataSnapshot.convert = function(data, now) {
 
 };
 
+RuleDataSnapshot.prototype.prune = function() {
+  return new RuleDataSnapshot(prune(this._data), this._timestamp, this._path);
+};
 
 RuleDataSnapshot.prototype.merge = function(other) {
   if (this._path || other._path) {
@@ -119,8 +159,8 @@ RuleDataSnapshot.prototype.merge = function(other) {
     return merge({'.priority': oldPriority}, newNode);
   });
 
-  return new RuleDataSnapshot(data, other._timestamp);
-}
+  return new RuleDataSnapshot(prune(data), other._timestamp);
+};
 
 RuleDataSnapshot.prototype.set = function(path, newData, now) {
 
@@ -129,7 +169,7 @@ RuleDataSnapshot.prototype.set = function(path, newData, now) {
   newData = RuleDataSnapshot.convert(newData, now);
 
   if (path.length === 0) {
-    return new RuleDataSnapshot(newData, now);
+    return new RuleDataSnapshot(prune(newData), now);
   }
 
   let data = merge({}, this._data);
@@ -155,7 +195,7 @@ RuleDataSnapshot.prototype.set = function(path, newData, now) {
     currentNode = currentNode[key];
   });
 
-  return new RuleDataSnapshot(data, now);
+  return new RuleDataSnapshot(prune(data), now)
 }
 
 RuleDataSnapshot.prototype._getVal = function() {
@@ -175,10 +215,7 @@ RuleDataSnapshot.prototype._getVal = function() {
       if (tree.hasOwnProperty(nextKey)) {
         return traverse(tree[nextKey]);
       } else {
-        return {
-          '.value': null,
-          '.priority': null
-        };
+        return literal(null);
       }
 
     } else {
@@ -187,10 +224,7 @@ RuleDataSnapshot.prototype._getVal = function() {
       if (tree) {
         return Object.assign({}, tree);
       } else {
-        return {
-          '.value': null,
-          '.priority': null
-        };
+        return literal(null);
       }
 
     }


Test patch:
diff --git a/test/spec/lib/rule-data-snapshot.js b/test/spec/lib/rule-data-snapshot.js
index 2ebac0c..3786f0b 100644
--- a/test/spec/lib/rule-data-snapshot.js
+++ b/test/spec/lib/rule-data-snapshot.js
@@ -201,7 +201,7 @@ describe('RuleDataSnapshot', function() {
       var patch = new RuleDataSnapshot({
         users: {
           'password:c7ec6752-45b3-404f-a2b9-7df07b78d28e': {
-            '.value': null
+            '.value': 'xyz'
           }
         }
       });
diff --git a/test/spec/lib/ruleset.js b/test/spec/lib/ruleset.js
index 718055e..675c1f4 100644
--- a/test/spec/lib/ruleset.js
+++ b/test/spec/lib/ruleset.js
@@ -322,6 +322,30 @@ describe('Ruleset', function() {
       expect(rules.tryWrite('nested/one/one', root, {id: {'.value': 'two'}}, auth).allowed).to.be.false;
     });
 
+    it('should prune null keys', function(){
+
+      var root = new RuleDataSnapshot(RuleDataSnapshot.convert({'a': 1, 'b': 2})),
+        rules = new Ruleset({rules: {'.write': true}});
+
+      expect(rules.tryWrite('/a', root, null, null).newRoot.val()).to.be.deep.equal({'b': 2});
+      expect(rules.tryWrite('/', root, {'a': 1, 'b': {}}, null).newRoot.val()).to.be.deep.equal({'a': 1});
+
+    });
+
+    it('should prune null keys deeply', function(){
+
+      var root = new RuleDataSnapshot(RuleDataSnapshot.convert({'a': {'b': 2}})),
+          rules = new Ruleset({rules: {'.write': true}}),
+          result = rules.tryWrite('/a/b', root, null, null);
+
+      expect(result.newRoot.val()).to.be.deep.equal(null);
+      expect(result.newRoot.child('a').val()).to.be.null;
+      expect(result.newRoot.child('a').exists()).to.be.false;
+      expect(result.newRoot.val()).to.be.null;
+      expect(result.newRoot.exists()).to.be.false;
+
+    });
+
     it('should replace a node, not merge it', function() {
       var root = getRoot(),
         auth = null,
@@ -456,6 +480,20 @@ describe('Ruleset', function() {
       expect(result.newData.val()).to.eql({foo: 1});
     });
 
+    it('should prune null keys deeply', function(){
+
+      var root = new RuleDataSnapshot(RuleDataSnapshot.convert({'a': {'b': 2}})),
+          rules = new Ruleset({rules: {'.write': true}}),
+          result = rules.tryPatch('/', root, {'/a/b': {}}, null);
+
+      expect(result.newRoot.val()).to.be.deep.equal(null);
+      expect(result.newRoot.child('a').val()).to.be.null;
+      expect(result.newRoot.child('a').exists()).to.be.false;
+      expect(result.newRoot.val()).to.be.null;
+      expect(result.newRoot.exists()).to.be.false;
+
+    });
+
   });
 
 });