Make the rule configurable to the usage of utility methods

For details: #1
ssutar · Oct 12, 2018 · 73f3ea6 · 73f3ea6
1 parent fa90434
commit 73f3ea6
Show file tree

Hide file tree

Showing 3 changed files with 242 additions and 16 deletions.
diff --git a/lib/index.js b/lib/index.js
@@ -1,6 +1,6 @@
 /**
- * @fileoverview Eslint plugin for enforcing usages of ember es6 classes.
- * @author scalvert
+ * @fileoverview Eslint plugin to prevent the record update after removal
+ * @author ssutar
  */
 "use strict";
 

diff --git a/lib/rules/no-object-update.js b/lib/rules/no-object-update.js
@@ -1,14 +1,46 @@
 /**
- * Prevent the code to use `EmberObject.extend` favoring the ES6 native class `extends`
+ * Prevent the record update after removal
  */
 "use strict";
+
 //------------------------------------------------------------------------------
-// Rule Definition
+// Utility methods
 //------------------------------------------------------------------------------
-function getReportMessage(objName = "obj") {
-  return `Please wrap '${objName}.set' in 'if (!${objName}.isDestroying) { ${objName}.set(...) }'`;
+
+/**
+ * Construct the error message to be report when rule fails
+ *
+ * @param {String} objName
+ * @param {String[]} utilityMethods
+ */
+function getReportMessage(objName = "obj", utilityMethods = []) {
+  let msg = `Wrap '${objName}.set' in 'if (!${objName}.isDestroying) { ${objName}.set(...) }'`;
+  if (utilityMethods.length) {
+    msg = `Use the utility method(s) '${utilityMethods.join(
+      "' OR '"
+    )}' to verify the '${objName}' is not being destroyed before calling '${objName}.set(...)'`;
+  }
+  return msg;
 }
 
+/**
+ * Get a property from and object, useful to get nested props without checking for null values
+ *
+ * @param {Object} obj
+ * @param {String} path
+ * @returns {Any}
+ */
+function get(obj, path) {
+  return path.split(".").reduce(function(currentObject, pathSegment) {
+    return typeof currentObject == "undefined" || currentObject === null
+      ? currentObject
+      : currentObject[pathSegment];
+  }, obj);
+}
+
+//------------------------------------------------------------------------------
+// Rule Definition
+//------------------------------------------------------------------------------
 module.exports = {
   meta: {
     docs: {
@@ -18,36 +50,98 @@ module.exports = {
     },
     reportMessage: getReportMessage()
   },
+
   create: function create(context) {
+    /**
+     * Iterates recursively over the test node of conditional and extracts the nodes matching
+     * either call expressions or member expressions
+     * @param {Expression} testNode
+     * @returns {Expression[]}
+     */
+    function getConditionalParams(testNode) {
+      const type = get(testNode, "type");
+      if (type === "LogicalExpression") {
+        return []
+          .concat(getConditionalParams(get(testNode, "left")))
+          .concat(getConditionalParams(get(testNode, "right")));
+      }
+      if (type === "UnaryExpression") {
+        return [].concat(getConditionalParams(get(testNode, "argument")));
+      }
+      if (type === "MemberExpression" || type === "CallExpression") {
+        return [testNode];
+      }
+      return [];
+    }
+
+    /**
+     * Checks if the `statement` is wrapped in the destroy conditional
+     *
+     * @param {Scope} scope
+     * @param {String} objName
+     * @returns boolean
+     */
+    function isWrappedInDestroyConditional(scope, objName) {
+      if (get(scope, "block.parent.type") !== "IfStatement") {
+        return false;
+      }
+      return getConditionalParams(get(scope, "block.parent.test")).some(
+        param => {
+          const type = get(param, "type");
+          if (type === "MemberExpression") {
+            return (
+              get(param, "object.name") === objName &&
+              get(param, "property.name") === "isDestroying"
+            );
+          } else {
+            return (
+              context.options.includes(get(param, "callee.name")) &&
+              get(param, "arguments").some(arg => get(arg, "name") === objName)
+            );
+          }
+        }
+      );
+    }
+
+    /**
+     * Main entry point for rule
+     *
+     * Parses all the `set` Identifier expressions in the code and report if it can fail in tests
+     *
+     * @param {Identifier} node
+     */
     function checkForObjectUpdate(node) {
       if (
-        node.name === "set" &&
-        node.parent.type !== "CallExpression" &&
-        node.parent.object.type !== "ThisExpression"
+        get(node, "name") === "set" &&
+        get(node, "parent.type") !== "CallExpression" &&
+        get(node, "parent.object.type") !== "ThisExpression"
       ) {
+        const objName = get(node, "parent.object.name");
         let scope = context.getScope(node);
-        while (scope && scope.type !== "function") {
-          scope = scope.upper;
+        while (scope && get(scope, "type") !== "function") {
+          if (isWrappedInDestroyConditional(scope, objName)) {
+            return;
+          }
+          scope = get(scope, "upper");
         }
 
-        const pNodeScope = scope && scope.block.parent;
+        const pNodeScope = get(scope, "block.parent");
         const pNodeScopeCallee =
-          pNodeScope && pNodeScope.type === "CallExpression"
+          get(pNodeScope, "type") === "CallExpression"
             ? pNodeScope.callee
             : null;
 
         if (!pNodeScopeCallee) {
           return;
         } else {
-          const propName = pNodeScopeCallee.property.name;
+          const propName = get(pNodeScopeCallee, "property.name");
           const isSetWrappedInPromise =
             propName === "then" ||
             propName === "catch" ||
             propName === "finally";
 
           if (isSetWrappedInPromise) {
-            const objName = node.parent.object.name;
-            context.report(node, getReportMessage(objName));
+            context.report(node, getReportMessage(objName, context.options));
           }
         }
       }

diff --git a/tests/rules/no-object-update-test.js b/tests/rules/no-object-update-test.js
@@ -8,6 +8,8 @@ const ruleTester = new RuleTester({
   }
 });
 
+const MESSAGE_WITH_OPTIONS = `Use the utility method(s) 'isAlive' OR 'isRemoved' to verify the 'obj' is not being destroyed before calling 'obj.set(...)'`;
+
 ruleTester.run("no-object-update", rule, {
   valid: [
     {
@@ -23,6 +25,63 @@ ruleTester.run("no-object-update", rule, {
             this.set('prop3', false);
           });
         `
+    },
+    {
+      code: `
+        somePromise
+          .then(() => {
+            if(!obj.isDestroying) {
+              obj.set('prop1', true);
+            }
+          })
+          .catch(() => {
+            if(!obj.isDestroying) {
+              if(someOtherCondition()) {
+                if(someOtherNestedCondition) {
+                  obj.set('prop2', true);
+                }
+              }
+            }
+          })
+          .finally(() => {
+            if(someOtherCondition()) {
+              if(!obj.isDestroying) {
+                if(someOtherNestedCondition) {
+                  obj.set('prop2', true);
+                }
+              }
+            }
+          });
+        `
+    },
+    {
+      code: `
+        somePromise
+          .then(() => {
+            if(!isRemoved(obj)) {
+              obj.set('prop1', true);
+            }
+          })
+          .catch(() => {
+            if(isAlive(obj)) {
+              if(someOtherCondition()) {
+                if(someOtherNestedCondition) {
+                  obj.set('prop2', true);
+                }
+              }
+            }
+          })
+          .finally(() => {
+            if(someOtherCondition()) {
+              if(!isMarkedForDelete(obj)) {
+                if(someOtherNestedCondition) {
+                  obj.set('prop2', true);
+                }
+              }
+            }
+          });
+        `,
+      options: ["isAlive", "isRemoved", "isMarkedForDelete"]
     }
   ],
   invalid: [
@@ -50,6 +109,79 @@ ruleTester.run("no-object-update", rule, {
           message: MESSAGE
         }
       ]
+    },
+    {
+      code: `
+        somePromise
+          .then(() => {
+            if(!isRemoved(obj)) {
+              obj.set('prop1', true);
+            }
+          })
+          .catch(() => {
+            if(isAlive(obj)) {
+              if(someOtherCondition()) {
+                if(someOtherNestedCondition) {
+                  obj.set('prop2', true);
+                }
+              }
+            }
+          })
+          .finally(() => {
+            if(someOtherCondition()) {
+              if(!isMarkedForDelete(obj)) {
+                if(someOtherNestedCondition) {
+                  obj.set('prop2', true);
+                }
+              }
+            }
+          });
+        `,
+      errors: [
+        {
+          message: MESSAGE
+        },
+        {
+          message: MESSAGE
+        },
+        {
+          message: MESSAGE
+        }
+      ]
+    },
+    {
+      code: `
+        somePromise
+          .then(() => {
+            obj.set('prop1', true);
+          })
+          .catch(() => {
+            if(someOtherCondition()) {
+              if(someOtherNestedCondition) {
+                obj.set('prop2', true);
+              }
+            }
+          })
+          .finally(() => {
+            if(someOtherCondition()) {
+              if(someOtherNestedCondition) {
+                obj.set('prop2', true);
+              }
+            }
+          });
+        `,
+      options: ["isAlive", "isRemoved"],
+      errors: [
+        {
+          message: MESSAGE_WITH_OPTIONS
+        },
+        {
+          message: MESSAGE_WITH_OPTIONS
+        },
+        {
+          message: MESSAGE_WITH_OPTIONS
+        }
+      ]
     }
   ]
 });