Repository: goldibex/targaryen
Pull request #52 (2016-10-19 13:29:03)

Node should be replaced not merged
`Ruleset.prototype.tryWrite` and `Ruleset.prototype.tryPatch` should replace node(s) not merge them.

dinoboff: @mhuebert @goldibex ready for review

Patch:
diff --git a/lib/helpers.js b/lib/helpers.js
index d812939..4b0b428 100644
--- a/lib/helpers.js
+++ b/lib/helpers.js
@@ -1,60 +1,36 @@
 'use strict';
 
-var RuleDataSnapshot = require('./rule-data-snapshot');
-
-function trimLeft(path) {
+exports.trimLeft = function(path) {
   path = path || '';
 
   return path.replace(/^\/+/, '');
 }
 
-function trimRight(path) {
+exports.trimRight = function(path) {
   path = path || '';
 
   return path.replace(/\/+$/, '');
 }
 
-function trim(path) {
-  return trimLeft(trimRight(path));
+exports.trim = function(path) {
+  return exports.trimLeft(exports.trimRight(path));
 }
 
 exports.pathMerger = function(root, path) {
-  root = trim(root);
-  path = trimLeft(path);
+  root = exports.trim(root);
+  path = exports.trimLeft(path);
 
   return root + '/' + path;
 }
 
 exports.pathSplitter = function(path) {
-  return trimLeft(path).split('/');
+  return exports.trimLeft(path).split('/');
 };
 
 exports.makeNewDataSnap = function(path, newData) {
+  var RuleDataSnapshot = require('./rule-data-snapshot');
 
-  path = trimLeft(path);
-  newData = RuleDataSnapshot.convert(newData)
-
-  var result;
-
-  if (path.length === 0) {
-    result = newData;
-  } else {
-
-    var workObject = {};
-    result = workObject;
-
-    exports.pathSplitter(path).forEach(function(pathPart, i, pathParts) {
-
-      if (pathParts.length - i === 1) {
-        workObject[pathPart] = newData;
-      } else {
-        workObject[pathPart] = {};
-      }
-      workObject = workObject[pathPart];
-
-    });
-
-  }
+  console.log('makeNewDataSnap is deprecated. Use RuleDataSnapshot.create instead.');
 
-  return new RuleDataSnapshot(result);
+  return RuleDataSnapshot.create(path, newData);
 };
diff --git a/lib/rule-data-snapshot.js b/lib/rule-data-snapshot.js
index a2c392e..a0568ae 100644
--- a/lib/rule-data-snapshot.js
+++ b/lib/rule-data-snapshot.js
@@ -2,6 +2,7 @@
 'use strict';
 
 var merge = require('lodash.mergewith');
+var helpers = require('./helpers');
 
 function isEmptyObj (obj) {
   return obj && typeof obj == 'object' && Object.keys(obj).length === 0 && obj.constructor === Object;
@@ -23,6 +24,12 @@ function RuleDataSnapshot(data, path) {
 
 }
 
+RuleDataSnapshot.create = function(path, newData) {
+  const empty = new RuleDataSnapshot();
+
+  return empty.set(path, newData);
+};
+
 RuleDataSnapshot.convert = function(data) {
 
   return (function firebaseify(node) {
@@ -105,6 +112,41 @@ RuleDataSnapshot.prototype.merge = function(other) {
   return new RuleDataSnapshot(data);
 }
 
+RuleDataSnapshot.prototype.set = function(path, newData) {
+
+  path = helpers.trim(path);
+  newData = RuleDataSnapshot.convert(newData)
+
+  if (path.length === 0) {
+    return new RuleDataSnapshot(newData);
+  }
+
+  let data = merge({}, this._data);
+  let currentNode = data;
+
+  helpers.pathSplitter(path).forEach(function(key, i, pathParts) {
+    const isLast = pathParts.length - i === 1;
+
+
+    if (isLast) {
+      const nodePriority = currentNode[key] && currentNode[key]['.priority'] || null;
+
+      currentNode[key] = newData;
+      currentNode[key]['.priority'] = nodePriority;
+
+      return;
+    }
+
+    if (!currentNode[key] || currentNode[key].hasOwnProperty('.value')) {
+      currentNode[key] = {};
+    }
+
+    currentNode = currentNode[key];
+  });
+
+  return new RuleDataSnapshot(data);
+}
+
 RuleDataSnapshot.prototype._getVal = function() {
 
   var pathParts;
diff --git a/lib/ruleset.js b/lib/ruleset.js
index 3aa167e..6a51992 100644
--- a/lib/ruleset.js
+++ b/lib/ruleset.js
@@ -4,8 +4,7 @@
 var Rule = require('./parser/rule'),
   RuleDataSnapshot = require('./rule-data-snapshot'),
   pathSplitter = require('./helpers').pathSplitter,
-  pathMerger = require('./helpers').pathMerger,
-  makeNewDataSnap = require('./helpers').makeNewDataSnap;
+  pathMerger = require('./helpers').pathMerger;
 
 var validRuleKinds = {
   '.read': true,
@@ -238,7 +237,7 @@ Ruleset.prototype.tryWrite = function(path, root, newData, auth, skipWrite, skip
   // write encompasses both the cascading write rules and the
   // non-cascading validate rules
 
-  var newDataRoot = root.merge(makeNewDataSnap(path, newData));
+  var newDataRoot = root.set(path, newData);
 
   var result = {
     path: path,
@@ -263,7 +262,7 @@ Ruleset.prototype.tryPatch = function(path, root, newData, auth, skipWrite, skip
   Object.keys(newData).forEach(function(endPath){
     var pathToNode = pathMerger(path, endPath)
 
-    newDataRoot = newDataRoot.merge(makeNewDataSnap(pathToNode, newData[endPath]));
+    newDataRoot = newDataRoot.set(pathToNode, newData[endPath]);
     pathsToTest.push(pathToNode);
   });
 


Test patch:
diff --git a/lib/test-helpers.js b/lib/test-helpers.js
index 6f4e535..fc447cc 100644
--- a/lib/test-helpers.js
+++ b/lib/test-helpers.js
@@ -4,8 +4,7 @@
 var root, rules,
   debug = true,
   Ruleset = require('./ruleset'),
-  RuleDataSnapshot = require('./rule-data-snapshot'),
-  makeNewDataSnap = require('./helpers').makeNewDataSnap;
+  RuleDataSnapshot = require('./rule-data-snapshot');
 
 var userDefinitions = exports.userDefinitions = {
 
@@ -167,4 +166,6 @@ exports.getFirebaseRules = function() {
   return rules;
 };
 
-exports.makeNewDataSnap = makeNewDataSnap;
+exports.makeNewDataSnap = function() {
+  return RuleDataSnapshot.create.apply(null, arguments)
+};
diff --git a/test/spec/lib/rule-data-snapshot.js b/test/spec/lib/rule-data-snapshot.js
index 8bfe02f..a1ace2f 100644
--- a/test/spec/lib/rule-data-snapshot.js
+++ b/test/spec/lib/rule-data-snapshot.js
@@ -32,6 +32,20 @@ var root = new RuleDataSnapshot(rootObj);
 
 describe('RuleDataSnapshot', function() {
 
+  describe('create', function() {
+
+    it('should create a new snapshot', function() {
+      expect(RuleDataSnapshot.create('foo/bar/baz', 1).val()).to.eql({
+        foo: {
+          bar: {
+            baz: 1
+          }
+        }
+      });
+    });
+
+  });
+
   describe('convert', function() {
 
     it('converts plain Javascript objects into Firebase data format', function() {
@@ -149,6 +163,51 @@ describe('RuleDataSnapshot', function() {
 
   });
 
+  describe('#set', function() {
+
+    it('should replace the root data', function() {
+      expect(root.set('/', {foo: 1}).val()).to.eql({foo: 1});
+    });
+
+    it('should replace a node', function() {
+      const path = 'users/password:c7ec6752-45b3-404f-a2b9-7df07b78d28e';
+      const newRoot = root.set(path, {
+        name: 'Sherlock Holmes',
+        genius: true
+      });
+
+      expect(newRoot.child(path).val()).to.eql({
+        name: 'Sherlock Holmes',
+        genius: true
+      });
+    });
+
+    it('should replace a literal node', function() {
+      const path = 'users/password:c7ec6752-45b3-404f-a2b9-7df07b78d28e/arrests';
+      const newRoot = root.set(path, {first: 1887, second: 1887});
+
+      expect(newRoot.child(path).val()).to.eql({first: 1887, second: 1887});
+    });
+
+    it('should replace node with a literal', function() {
+      const path = 'users/password:c7ec6752-45b3-404f-a2b9-7df07b78d28e/arrests';
+      const newRoot = root.set(path, null);
+
+      expect(newRoot.child(path).val()).to.equal(null);
+    });
+
+    it('should preserve the priority', function() {
+      const path = 'users/password:c7ec6752-45b3-404f-a2b9-7df07b78d28e';
+      const newRoot = root.set(path, {
+        name: 'Sherlock Holmes',
+        genius: true
+      });
+
+      expect(newRoot.child(path).getPriority()).to.equal(1);
+    });
+
+  });
+
   describe('#val', function() {
 
     it('gets the value at the specified path', function() {
diff --git a/test/spec/lib/ruleset.js b/test/spec/lib/ruleset.js
index 8bdad30..e326321 100644
--- a/test/spec/lib/ruleset.js
+++ b/test/spec/lib/ruleset.js
@@ -30,6 +30,21 @@ function getRuleset() {
             }
           }
         }
+      },
+      mixedType: {
+        $item: {
+          '.write': true,
+          '.validate': 'newData.hasChildren(["type", "a"]) || newData.hasChildren(["type", "b"])',
+          type: {
+            '.validate': 'newData.val() == "a" || newData.val() == "b"'
+          },
+          a: {
+            '.validate': 'newData.parent().child("type").val() == "a" \n&& newData.parent().child("b").exists() == false'
+          },
+          b: {
+            '.validate': 'newData.parent().child("type").val() == "b" \n&& newData.parent().child("a").exists() == false'
+          }
+        }
       }
     }
   });
@@ -59,6 +74,12 @@ function getRoot() {
         'two': {id: {'.value': 'two'}}
       }
     },
+    'mixedType': {
+      'first': {
+        type: {'.value': 'a'},
+        a: {'.value': 1}
+      }
+    },
     'users': {
       'firstChild': {
         '.value': true
@@ -261,6 +282,25 @@ describe('Ruleset', function() {
       expect(rules.tryWrite('nested/one/one', root, {id: {'.value': 'two'}}, auth).allowed).to.be.false;
     });
 
+    it('should replace a node, not merge it', function() {
+      var root = getRoot(),
+        auth = null,
+        result;
+
+      result = rules.tryWrite('mixedType/first', root, {
+        type: {'.value': 'b'},
+        b: {'.value': 1}
+      }, auth);
+      expect(result.newData).to.eql({type: 'b', b: 1});
+      expect(result.allowed).to.be.true;
+
+      result = rules.tryWrite('mixedType/first', root, {
+        type: {'.value': 'a'},
+        b: {'.value': 1}
+      }, auth)
+      expect(result.allowed).to.be.false;
+    });
+
   });
 
   describe('#tryPatch', function() {