Repository: goldibex/targaryen
Pull request #59 (2016-10-22 10:48:59)

Standardize result data
tryRead, tryWrite, and tryPatch return data & root as snapshots (no need to run .val() on these).
tryWrite and tryPatch also return newData and newRoot.


Patch:
diff --git a/.gitignore b/.gitignore
index f4adf91..59d842b 100644
--- a/.gitignore
+++ b/.gitignore
@@ -26,6 +26,3 @@ node_modules
 
 # Users Environment Variables
 .lock-wscript
-
-# Editors
-*.sublime-*
\ No newline at end of file
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 5fac0bc..4e1d1bd 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -3,13 +3,12 @@
 - [Expose Ruleset, DataSnapshot and helpers](https://github.com/goldibex/targaryen/pull/50); thanks to @mhuebert.
 - [Write operations now replace nodes instead of merging them](https://github.com/goldibex/targaryen/pull/52).
 - [Fix initial timestamps](https://github.com/goldibex/targaryen/pull/41); The timestamp used to replace a timestamp server value will now match the `now` variable in rule tests.
-- [Fix paring rules with unknown identifers](https://github.com/goldibex/targaryen/pull/55).
-- `setFirebaseData` gains a `now` parameter: `targaryen.setFirebaseData(data[, now]);`.
-- jasmine matchers gain an optional `now` parameter.
-- chai plugin `write` and `patch` methods gain an optional `now` parameter.
+- `setFirebaseData` gains a `now parameter: `targaryen.setFirebaseData(data[, now]);`.
+- jasmine matchers gain an optional now parameter.
+- chai plugin `write` and `patch` methods gain an optional now parameter.
 - chai plugin gains a `readAt(now)` method.
 
-## 2.2.1 (2016-10-21)
+## 2.2.1 (upcoming)
 
 - [Properly merge literal value nodes (including null)](https://github.com/goldibex/targaryen/pull/44);
   a node would have been set to null but it's - deleted - children would still have been validated with their old value.
diff --git a/lib/parser/rule.js b/lib/parser/rule.js
index d21b61a..a3735b9 100644
--- a/lib/parser/rule.js
+++ b/lib/parser/rule.js
@@ -51,7 +51,6 @@ function Rule(ruleStr, wildchildren, isWrite) {
   })(ast);
 
   scope.inferType(this._ast);
-  scope.assertIndentifiersExist(this._ast);
 
   if (this._ast.expression.inferredType !== 'boolean') {
     throw new Error('Expression does not evaluate to a boolean');
diff --git a/lib/parser/scope.js b/lib/parser/scope.js
index f8d02ce..40f38da 100644
--- a/lib/parser/scope.js
+++ b/lib/parser/scope.js
@@ -136,50 +136,6 @@ Scope.prototype.inferType = function(node) {
 
 };
 
-Scope.prototype.assertIndentifiersExist = function(node) {
-  var stack = [node];
-
-  while (stack.length > 0) {
-    let currNode = stack.pop();
-
-    switch(currNode.type) {
-
-    case 'LogicalExpression':
-    case 'BinaryExpression':
-      stack.push(currNode.right, currNode.left);
-      break;
-
-    case 'ConditionalExpression':
-      stack.push(currNode.test, currNode.consequent, currNode.alternate);
-      break;
-
-    case 'ExpressionStatement':
-      stack.push(currNode.expression);
-      break;
-
-    case 'CallExpression':
-      stack.push(currNode.callee);
-      break;
-
-    case 'MemberExpression':
-      stack.push(currNode.object);
-      break;
-
-    case 'UnaryExpression':
-      stack.push(currNode.argument);
-      break;
-
-    case 'Identifier':
-      this._assertIdentifier(currNode);
-      break;
-
-    default:
-      break;
-
-    }
-  }
-};
-
 Scope.prototype._inferLiteral = function(node) {
 
   // one hiccup: is this a regular expression literal? esparse does not help with this
@@ -262,16 +218,10 @@ Scope.prototype._inferBinaryExpression = function(node) {
 
 Scope.prototype._inferIdentifier = function(node) {
 
-  this._assertIdentifier(node);
-  node.inferredType = this.types[node.name];
-
-};
-
-Scope.prototype._assertIdentifier = function(node) {
-
   if (!this.types[node.name]) {
     throw nodeError(node, 'Unknown variable "' + node.name + '"');
   }
+  node.inferredType = this.types[node.name];
 
 };
 
diff --git a/lib/ruleset.js b/lib/ruleset.js
index be7641a..164eb10 100644
--- a/lib/ruleset.js
+++ b/lib/ruleset.js
@@ -184,7 +184,9 @@ Ruleset.prototype.tryRead = function(path, root, auth, now) {
     auth: state.auth,
     type: 'read',
     info: '',
-    allowed: false
+    allowed: false,
+    data: state.data,
+    root: state.root
   };
 
   for (var i = 0; i < rules.length; i++) {
@@ -249,8 +251,10 @@ Ruleset.prototype.tryWrite = function(path, root, newData, auth, skipWrite, skip
     auth: auth === undefined ? null : auth,
     writePermitted: skipWrite || false,
     validated: true,
-    data: root.child(path).val(),
-    newData: newDataRoot.child(path).val()
+    data: root.child(path),
+    newData: newDataRoot.child(path),
+    root: root,
+    newRoot: newDataRoot
   };
 
   return this._tryWrite(path, root, newDataRoot, result, skipWrite, skipValidate, skipOnNoValue);
@@ -259,7 +263,8 @@ Ruleset.prototype.tryWrite = function(path, root, newData, auth, skipWrite, skip
 Ruleset.prototype.tryPatch = function(path, root, newData, auth, skipWrite, skipValidate, skipOnNoValue, now) {
   var newDataRoot = root,
       pathsToTest = [],
-      results;
+      pathResults,
+      result;
 
   now = now || Date.now();
 
@@ -270,46 +275,50 @@ Ruleset.prototype.tryPatch = function(path, root, newData, auth, skipWrite, skip
     pathsToTest.push(pathToNode);
   });
 
-  results = pathsToTest.map(function(pathToNode) {
-    var result = {
+  pathResults = pathsToTest.map(function(pathToNode) {
+    var pathResult = {
       path: pathToNode,
       type: 'patch',
       info: '',
       allowed: false,
       auth: auth === undefined ? null : auth,
       writePermitted: skipWrite || false,
-      validated: true,
-      data: root.child(pathToNode),
-      newData: newDataRoot.child(pathToNode).val()
+      validated: true
     };
 
-    return this._tryWrite(pathToNode, root, newDataRoot, result, skipWrite, skipValidate, skipOnNoValue);
+    return this._tryWrite(pathToNode, root, newDataRoot, pathResult, skipWrite, skipValidate, skipOnNoValue);
   }, this);
 
-  if (results.length === 0) {
-    return {
+  if (pathResults.length === 0) {
+    result = {
       path: path,
       type: 'patch',
       info: '',
       allowed: true,
       auth: auth === undefined ? null : auth,
       writePermitted: skipWrite || false,
-      validated: true,
-      data: root.child(path),
-      newData: newDataRoot.child(path).val()
+      validated: true
     };
+  } else {
+    result = pathResults.reduce(function(result, pathResult) {
+      result.path = path;
+      result.newData = newData;
+      result.info += pathResult.info;
+      result.allowed = result.allowed && pathResult.allowed;
+      result.writePermitted = result.writePermitted && pathResult.writePermitted;
+      result.validated = result.validated && pathResult.validated;
+
+      return result;
+    });
   }
 
-  return results.reduce(function(result, pathResult) {
-    result.path = path;
-    result.newData = newData;
-    result.info += pathResult.info;
-    result.allowed = result.allowed && pathResult.allowed;
-    result.writePermitted = result.writePermitted && pathResult.writePermitted;
-    result.validated = result.validated && pathResult.validated;
+  result.data = root.child(path);
+  result.newData = newDataRoot.child(path);
+  result.root = root;
+  result.newRoot = newDataRoot;
+
+  return result
 
-    return result;
-  });
 };
 
 


Test patch:
diff --git a/test/spec/lib/ruleset.js b/test/spec/lib/ruleset.js
index e2093fd..39cdbb8 100644
--- a/test/spec/lib/ruleset.js
+++ b/test/spec/lib/ruleset.js
@@ -95,11 +95,11 @@ function getRoot() {
 
 }
 
-var invalidRulesets = {
-  'are null': null,
-  'are a string': 'wut',
-  'are missing': { noTopLevelRules: true },
-  'include extra props': {
+var invalidRulesets = [
+  null,
+  'wut',
+  { noTopLevelRules: true },
+  {
     rules: {
       '.read': true,
       '.write': true,
@@ -108,41 +108,29 @@ var invalidRulesets = {
     },
     otherStuff: true
   },
-  'include invalid index': {
+  {
     rules: {
       '.read': true,
       '.indexOn': true
     }
   },
-  'include unknown props': {
+  {
     rules: {
       '.read': true,
       '.woops': true
     }
   },
-  'include unknown wildchild': {
+  {
     rules: {
       '.read': '$somewhere === true'
     }
   },
-  'set index to an object': {
+  {
     rules: {
       '.indexOn': {}
     }
-  },
-  'include unknown variables': {
-    rules: {
-      ".validate": "something.val() + 1 === date.val()",
-      ".write": "something.val() / 2 > 0"
-    }
-  },
-  'include rules composed with unknown variables': {
-    rules: {
-      ".validate": "auth != null && something.val() + 1 === date.val()",
-      ".write": "auth != null && something.val() / 2 > 0"
-    }
   }
-};
+];
 
 var validRulesets = [{
   rules: {}
@@ -171,12 +159,14 @@ describe('Ruleset', function() {
 
   describe('constructor', function() {
 
-    Object.keys(invalidRulesets).forEach(function(reason) {
-      it(`rejects when rulesets ${reason}`, function() {
+    it('rejects invalid rulesets', function() {
+
+      invalidRulesets.forEach(function(rules) {
         expect(function() {
-          return new Ruleset(invalidRulesets[reason]);
+          return new Ruleset(rules);
         }).to.throw();
       });
+
     });
 
     it('accepts valid rulesets', function() {
@@ -331,7 +321,7 @@ describe('Ruleset', function() {
         type: {'.value': 'b'},
         b: {'.value': 1}
       }, auth);
-      expect(result.newData).to.eql({type: 'b', b: 1});
+      expect(result.newData.val()).to.eql({type: 'b', b: 1});
       expect(result.allowed).to.be.true;
 
       result = rules.tryWrite('mixedType/first', root, {
@@ -453,7 +443,7 @@ describe('Ruleset', function() {
       const result = rules.tryPatch('nested/one/one', root, {}, auth)
 
       expect(result.allowed).to.be.true;
-      expect(result.newData).to.eql({foo: 1});
+      expect(result.newData.val()).to.eql({foo: 1});
     });
 
   });