Repository: bpmn-io/bpmnlint
Pull request #54 (2020-12-29 22:11:51)

Implement <enter> and <leave> hooks as well as sub-tree skipping
A rule can now be explicit about when to check a given node, on _enter_, on _leave_ or during both occasions:

```javascript
module.exports = function someRule() {

  return {
    check: {
      enter: function(node, reporter) { ... },
      leave: function(node, reporter) { ... }
    }
  };
};
```

A rule may also return false from the _enter_ hook to stop traversing deeper into the elements children.

---

Closes https://github.com/bpmn-io/bpmnlint/issues/52
Closes https://github.com/bpmn-io/bpmnlint/issues/53

adroste: I am testing right now.

### Use case 1 

Search for matching types & perform checks after every element was visited.

Works by doing sth. like this:

```js
function leave(node, reporter) {
    if (!is(node, 'bpmn:Definitions'))
        return;

   // calc & write reports
}

function enter(node, reporter) {
    // collect relevant elements
}

return { check: { enter, leave } };
```

### Use case 2

Traversing/searching for inner element.

Works, example:

```js
let startNode = null;
let innerElements = [];

function leave(node, reporter) {
    if (node !== startNode) 
        return;

    // do the checks

    startNode = null;
    innerElements = [];
}

function enter(node, reporter) {
    if (startNode) { 
        if (is(node, 'my:innerType')) {
            innerElements.push(node);
        }
    } 
    else if (is(node, 'my:outerType')) {
        startNode = node;
    }
}

return { check: { enter, leave } };
```

### Use case 3

Early exit. 

Works somewhat by short circuiting every enter. Example:

```js
let finished = false;

function enter(node, reporter) {
    if (finished)
        return false;

    if (is(node, 'my:type')) {
        
        // do stuff...

        finished = true;
        return false;
    }
}

return { check: { enter } };
```

## Conclusion

Works for me. Seems to be a good compromise between versatility and the need to write boilerplate code for certain use cases.

My tests did not encounter any unwanted behaviour.

As it does not break existing implementations, I think a merge should be safe.

Patch:
diff --git a/lib/support/compile-config.js b/lib/support/compile-config.js
index bf5bd3d..e673215 100644
--- a/lib/support/compile-config.js
+++ b/lib/support/compile-config.js
@@ -34,7 +34,7 @@ async function compileConfig(config) {
   const serializedRules = JSON.stringify(enabledRules, null, '  ');
 
   const preambleCode = `
-var cache = {};
+const cache = {};
 
 /**
  * A resolver that caches rules and configuration as part of the bundle,
@@ -61,15 +61,15 @@ Resolver.prototype.resolveConfig = function(pkg, configName) {
   );
 };
 
-var resolver = new Resolver();
+const resolver = new Resolver();
 
-var rules = ${serializedRules};
+const rules = ${serializedRules};
 
-var config = {
+const config = {
   rules: rules
 };
 
-var bundle = {
+const bundle = {
   resolver: resolver,
   config: config
 };
diff --git a/lib/traverse.js b/lib/traverse.js
index 767d7d8..b6b7d60 100644
--- a/lib/traverse.js
+++ b/lib/traverse.js
@@ -3,32 +3,37 @@
  * and call the passed visitor fn.
  *
  * @param {ModdleElement} element
- * @param {Function} fn
+ * @param {{ enter?: Function; leave?: Function }} options
  */
-module.exports = function traverse(element, fn) {
-  fn(element);
+module.exports = function traverse(element, options) {
 
-  var descriptor = element.$descriptor;
+  const enter = options.enter || null;
+  const leave = options.leave || null;
 
-  if (descriptor.isGeneric) {
-    return;
-  }
+  const enterSubTree = enter && enter(element);
+
+  const descriptor = element.$descriptor;
+
+  if (enterSubTree !== false && !descriptor.isGeneric) {
 
-  var containedProperties = descriptor.properties.filter(p => {
-    return !p.isAttr && !p.isReference && p.type !== 'String';
-  });
+    const containedProperties = descriptor.properties.filter(p => {
+      return !p.isAttr && !p.isReference && p.type !== 'String';
+    });
 
-  containedProperties.forEach(p => {
-    if (p.name in element) {
-      const propertyValue = element[p.name];
+    containedProperties.forEach(p => {
+      if (p.name in element) {
+        const propertyValue = element[p.name];
 
-      if (p.isMany) {
-        propertyValue.forEach(child => {
-          traverse(child, fn);
-        });
-      } else {
-        traverse(propertyValue, fn);
+        if (p.isMany) {
+          propertyValue.forEach(child => {
+            traverse(child, options);
+          });
+        } else {
+          traverse(propertyValue, options);
+        }
       }
-    }
-  });
+    });
+  }
+
+  leave && leave(element);
 };


Test patch:
diff --git a/lib/test-rule.js b/lib/test-rule.js
index 5de59a3..9044369 100644
--- a/lib/test-rule.js
+++ b/lib/test-rule.js
@@ -15,6 +15,20 @@ class Reporter {
 
 module.exports = function testRule({ moddleRoot, rule }) {
   const reporter = new Reporter({ rule, moddleRoot });
-  traverse(moddleRoot, node => rule.check(node, reporter));
+
+  const check = rule.check;
+
+  const enter = check && check.enter || check;
+  const leave = check && check.leave;
+
+  if (!enter && !leave) {
+    throw new Error('no check implemented');
+  }
+
+  traverse(moddleRoot, {
+    enter: enter ? (node) => enter(node, reporter) : null,
+    leave: leave ? (node) => leave(node, reporter) : null
+  });
+
   return reporter.messages;
 };
diff --git a/test/spec/linter-spec.js b/test/spec/linter-spec.js
index 1c03df0..c19cf2f 100644
--- a/test/spec/linter-spec.js
+++ b/test/spec/linter-spec.js
@@ -54,11 +54,29 @@ describe('linter', function() {
       );
 
       // then
-      expect(results).to.eql(buildResults('error'));
+      expect(results).to.eql(buildFakeResults('error'));
     });
 
 
-    it('should fail', function() {
+    it('should succeed with <enter> and <leave> hooks', function() {
+
+      // when
+      const results = linter.applyRule(
+        moddleRoot,
+        {
+          name: 'test-rule',
+          config: { },
+          rule: createRule(fakeEnterLeaveRule),
+          category: 'error'
+        }
+      );
+
+      // then
+      expect(results).to.eql(buildFakeEnterLeaveResults('error'));
+    });
+
+
+    it('should fail without check', function() {
 
       const failingRule = {};
 
@@ -77,12 +95,36 @@ describe('linter', function() {
       expect(results).to.eql([
         {
           category: 'error',
-          message: 'Rule error: rule.check is not a function'
+          message: 'Rule error: no check implemented'
         }
       ]);
     });
 
 
+    it('should fail without <enter> or <leave> hook', function() {
+
+      const failingRule = { check: {} };
+
+      // when
+      const results = linter.applyRule(
+        moddleRoot,
+        {
+          name: 'test-rule',
+          config: { },
+          rule: failingRule,
+          category: 'warn'
+        }
+      );
+
+      // then
+      expect(results).to.eql([
+        {
+          category: 'error',
+          message: 'Rule error: enter is not a function'
+        }
+      ]);
+    });
+
   });
 
 
@@ -173,7 +215,7 @@ describe('linter', function() {
 
         // then
         expect(lintResults).to.eql({
-          testRule: buildResults('warn')
+          testRule: buildFakeResults('warn')
         });
       });
 
@@ -198,7 +240,7 @@ describe('linter', function() {
 
         // then
         expect(lintResults).to.eql({
-          testRule: buildResults('warn')
+          testRule: buildFakeResults('warn')
         });
       });
 
@@ -873,7 +915,8 @@ function fakeRule() {
   };
 }
 
-function buildResults(category) {
+
+function buildFakeResults(category) {
   const results = [
     {
       id: 'sid-38422fae-e03e-43a3-bef4-bd33b32041b2',
@@ -885,6 +928,59 @@ function buildResults(category) {
     return results;
   }
 
+  return results.map((result) => {
+    return {
+      ...result,
+      category
+    };
+  });
+}
+
+
+function fakeEnterLeaveRule() {
+
+  const seen = {};
+
+  function enter(node, reporter) {
+
+    if (node.id) {
+      seen[node.id] = node;
+    }
+
+  }
+
+  function leave(node, reporter) {
+
+    if (node.id) {
+      expect(seen[node.id]).to.equal(node);
+    }
+
+    if (is(node, 'Definitions') && seen[node.id]) {
+      reporter.report(node.id, 'Definitions seen');
+    }
+  }
+
+  return {
+    check: {
+      enter,
+      leave
+    }
+  };
+}
+
+
+function buildFakeEnterLeaveResults(category) {
+  const results = [
+    {
+      id: 'sid-38422fae-e03e-43a3-bef4-bd33b32041b2',
+      message: 'Definitions seen'
+    }
+  ];
+
+  if (typeof category === 'undefined') {
+    return results;
+  }
+
   return results.map((result) => {
     return {
       ...result,
diff --git a/test/spec/test-rule-spec.js b/test/spec/test-rule-spec.js
index 1468be5..13986fb 100644
--- a/test/spec/test-rule-spec.js
+++ b/test/spec/test-rule-spec.js
@@ -15,7 +15,8 @@ describe('test-rule', function() {
     moddleRoot = result.root;
   });
 
-  it('should return reported messages', () => {
+
+  it('should return check function reported messages', () => {
 
     // given
     const expectedMessages = [
@@ -26,7 +27,30 @@ describe('test-rule', function() {
     ];
     const messages = testRule({
       moddleRoot,
-      rule: createRule(fakeRuleWithReports)
+      rule: createRule(fakeCheckRuleWithReports)
+    });
+
+    // then
+    expect(messages).to.eql(expectedMessages);
+  });
+
+
+  it('should return { enter, leave } hook reported messages', () => {
+
+    // given
+    const expectedMessages = [
+      {
+        id: 'sid-38422fae-e03e-43a3-bef4-bd33b32041b2',
+        message: 'Definitions enter'
+      },
+      {
+        id: 'sid-38422fae-e03e-43a3-bef4-bd33b32041b2',
+        message: 'Definitions leave'
+      }
+    ];
+    const messages = testRule({
+      moddleRoot,
+      rule: createRule(fakeEnterLeaveRuleWithReports)
     });
 
     // then
@@ -42,7 +66,7 @@ describe('test-rule', function() {
     // when
     const messages = testRule({
       moddleRoot,
-      rule: createRule(fakeRuleWithoutReports)
+      rule: createRule(fakeCheckRuleWithoutReports)
     });
 
     // then
@@ -51,7 +75,7 @@ describe('test-rule', function() {
 
 });
 
-function fakeRuleWithReports() {
+function fakeCheckRuleWithReports() {
   function check(node, reporter) {
     if (is(node, 'Definitions')) {
       reporter.report(node.id, 'Definitions detected');
@@ -61,6 +85,27 @@ function fakeRuleWithReports() {
   return { check };
 }
 
-function fakeRuleWithoutReports() {
+function fakeEnterLeaveRuleWithReports() {
+  function enter(node, reporter) {
+    if (is(node, 'Definitions')) {
+      reporter.report(node.id, 'Definitions enter');
+    }
+  }
+
+  function leave(node, reporter) {
+    if (is(node, 'Definitions')) {
+      reporter.report(node.id, 'Definitions leave');
+    }
+  }
+
+  return {
+    check: {
+      enter,
+      leave
+    }
+  };
+}
+
+function fakeCheckRuleWithoutReports() {
   return { check: () => {} };
 }
diff --git a/test/spec/traverse-spec.js b/test/spec/traverse-spec.js
index eb3b5c7..149195c 100644
--- a/test/spec/traverse-spec.js
+++ b/test/spec/traverse-spec.js
@@ -9,9 +9,9 @@ import {
 
 describe('traverse', function() {
 
-  describe('should visit each node', function() {
+  describe('traversal', function() {
 
-    it('diagram with one node', async function() {
+    it('should traverse diagram with one node', async function() {
 
       // given
       const xmlStr = `
@@ -37,12 +37,12 @@ describe('traverse', function() {
 
       // then
       expect(nodes).to.eql([
-        'bpmn:Definitions#Definitions'
+        'bpmn:Definitions#Definitions - enter'
       ]);
     });
 
 
-    it('diagram with generic element', async function() {
+    it('should traverse diagram with generic element', async function() {
 
       // given
       const xmlStr = `
@@ -72,14 +72,14 @@ describe('traverse', function() {
 
       // then
       expect(nodes).to.eql([
-        'bpmn:Definitions#Definitions',
-        'bpmn:ExtensionElements',
-        'asd:foo'
+        'bpmn:Definitions#Definitions - enter',
+        'bpmn:ExtensionElements - enter',
+        'asd:foo - enter'
       ]);
     });
 
 
-    it('diagram with collaboration', async function() {
+    it('should traverse diagram with collaboration', async function() {
 
       // given
       const {
@@ -96,26 +96,26 @@ describe('traverse', function() {
 
       // then
       expect(nodes).to.eql([
-        'bpmn:Definitions#sid-38422fae-e03e-43a3-bef4-bd33b32041b2',
-        'bpmn:Collaboration#Collaboration_0wzd2dx',
-        'bpmn:Participant#Participant_03hz6qm',
-        'bpmn:Participant#Participant_1w6hx42',
-        'bpmn:MessageFlow#MessageFlow_1ofxm38',
-        'bpmn:Process#Process_1',
-        'bpmndi:BPMNDiagram#BpmnDiagram_1',
-        'bpmndi:BPMNPlane#BpmnPlane_1',
-        'bpmndi:BPMNShape#Participant_03hz6qm_di',
-        'dc:Bounds',
-        'bpmndi:BPMNEdge#MessageFlow_1ofxm38_di',
-        'dc:Point',
-        'dc:Point',
-        'bpmndi:BPMNShape#Participant_1sh3ce3_di',
-        'dc:Bounds'
+        'bpmn:Definitions#sid-38422fae-e03e-43a3-bef4-bd33b32041b2 - enter',
+        'bpmn:Collaboration#Collaboration_0wzd2dx - enter',
+        'bpmn:Participant#Participant_03hz6qm - enter',
+        'bpmn:Participant#Participant_1w6hx42 - enter',
+        'bpmn:MessageFlow#MessageFlow_1ofxm38 - enter',
+        'bpmn:Process#Process_1 - enter',
+        'bpmndi:BPMNDiagram#BpmnDiagram_1 - enter',
+        'bpmndi:BPMNPlane#BpmnPlane_1 - enter',
+        'bpmndi:BPMNShape#Participant_03hz6qm_di - enter',
+        'dc:Bounds - enter',
+        'bpmndi:BPMNEdge#MessageFlow_1ofxm38_di - enter',
+        'dc:Point - enter',
+        'dc:Point - enter',
+        'bpmndi:BPMNShape#Participant_1sh3ce3_di - enter',
+        'dc:Bounds - enter'
       ]);
     });
 
 
-    it('diagram with simple process', async function() {
+    it('should traverse diagram with simple process', async function() {
 
       // given
       const {
@@ -132,22 +132,194 @@ describe('traverse', function() {
 
       // then
       expect(nodes).to.eql([
-        'bpmn:Definitions#Definitions_1',
-        'bpmn:Process#Process_1',
-        'bpmn:StartEvent#Event_1',
-        'bpmn:Task#Activity_1',
-        'bpmn:SequenceFlow#Flow_1',
-        'bpmndi:BPMNDiagram#BpmnDiagram_1',
-        'bpmndi:BPMNPlane#BpmnPlane_1',
-        'bpmndi:BPMNEdge#Flow_1_di',
-        'dc:Point',
-        'dc:Point',
-        'bpmndi:BPMNShape#Event_1_di',
-        'dc:Bounds',
-        'bpmndi:BPMNLabel',
-        'dc:Bounds',
-        'bpmndi:BPMNShape#Activity_1_di',
-        'dc:Bounds'
+        'bpmn:Definitions#Definitions_1 - enter',
+        'bpmn:Process#Process_1 - enter',
+        'bpmn:StartEvent#Event_1 - enter',
+        'bpmn:Task#Activity_1 - enter',
+        'bpmn:SequenceFlow#Flow_1 - enter',
+        'bpmndi:BPMNDiagram#BpmnDiagram_1 - enter',
+        'bpmndi:BPMNPlane#BpmnPlane_1 - enter',
+        'bpmndi:BPMNEdge#Flow_1_di - enter',
+        'dc:Point - enter',
+        'dc:Point - enter',
+        'bpmndi:BPMNShape#Event_1_di - enter',
+        'dc:Bounds - enter',
+        'bpmndi:BPMNLabel - enter',
+        'dc:Bounds - enter',
+        'bpmndi:BPMNShape#Activity_1_di - enter',
+        'dc:Bounds - enter'
+      ]);
+    });
+
+  });
+
+
+  describe('enter/leave hooks', function() {
+
+    it('should <enter> and <leave>', async function() {
+
+      // given
+      const xmlStr = `
+        <?xml version="1.0" encoding="UTF-8"?>
+        <definitions
+            xmlns="http://www.omg.org/spec/BPMN/20100524/MODEL"
+            id="Definitions"
+            targetNamespace="http://bpmn.io/bpmn">
+          <process id="Process_1"/>
+        </definitions>
+      `;
+
+      const {
+        root
+      } = await createModdle(xmlStr);
+
+      const {
+        nodes,
+        log
+      } = createLogger([ 'enter', 'leave' ]);
+
+      // when
+      traverse(root, log);
+
+      // then
+      expect(nodes).to.eql([
+        'bpmn:Definitions#Definitions - enter',
+        'bpmn:Process#Process_1 - enter',
+        'bpmn:Process#Process_1 - leave',
+        'bpmn:Definitions#Definitions - leave'
+      ]);
+    });
+
+
+    it('should <leave> only', async function() {
+
+      // given
+      const xmlStr = `
+        <?xml version="1.0" encoding="UTF-8"?>
+        <definitions
+            xmlns="http://www.omg.org/spec/BPMN/20100524/MODEL"
+            id="Definitions"
+            targetNamespace="http://bpmn.io/bpmn">
+          <process id="Process_1"/>
+        </definitions>
+      `;
+
+      const {
+        root
+      } = await createModdle(xmlStr);
+
+      const {
+        nodes,
+        log
+      } = createLogger([ 'leave' ]);
+
+      // when
+      traverse(root, log);
+
+      // then
+      expect(nodes).to.eql([
+        'bpmn:Process#Process_1 - leave',
+        'bpmn:Definitions#Definitions - leave'
+      ]);
+    });
+
+
+    it('should <enter> and <leave> generic element', async function() {
+
+      // given
+      const xmlStr = `
+        <?xml version="1.0" encoding="UTF-8"?>
+        <definitions
+            xmlns="http://www.omg.org/spec/BPMN/20100524/MODEL"
+            xmlns:asd="http://asd"
+            id="Definitions"
+            targetNamespace="http://bpmn.io/bpmn">
+          <extensionElements>
+            <asd:foo bar="BAR" />
+          </extensionElements>
+        </definitions>
+      `;
+
+      const {
+        root
+      } = await createModdle(xmlStr);
+
+      const {
+        nodes,
+        log
+      } = createLogger([ 'enter', 'leave' ]);
+
+      // when
+      traverse(root, log);
+
+      // then
+      expect(nodes).to.eql([
+        'bpmn:Definitions#Definitions - enter',
+        'bpmn:ExtensionElements - enter',
+        'asd:foo - enter',
+        'asd:foo - leave',
+        'bpmn:ExtensionElements - leave',
+        'bpmn:Definitions#Definitions - leave'
+      ]);
+    });
+
+  });
+
+
+  describe('sub-tree skipping', function() {
+
+    it('should skip if <enter> returns false', async function() {
+
+      // given
+      const xmlStr = `
+        <?xml version="1.0" encoding="UTF-8"?>
+        <definitions
+            xmlns="http://www.omg.org/spec/BPMN/20100524/MODEL"
+            id="Definitions"
+            targetNamespace="http://bpmn.io/bpmn">
+          <bpmn:process id="Process_1">
+            <bpmn:task id="Task_1" />
+          </bpmn:process>
+          <bpmn:process id="Process_2">
+            <bpmn:task id="Task_2" />
+          </bpmn:process>
+        </definitions>
+      `;
+
+      const {
+        root
+      } = await createModdle(xmlStr);
+
+      const {
+        nodes,
+        log
+      } = createLogger([ 'enter', 'leave' ]);
+
+      // when
+      traverse(root, {
+        enter: (node) => {
+          log.enter(node);
+
+          if (node.id === 'Process_1') {
+            return false;
+          }
+
+          // does nothing
+          return null;
+        },
+        leave: log.leave
+      });
+
+      // then
+      expect(nodes).to.eql([
+        'bpmn:Definitions#Definitions - enter',
+        'bpmn:Process#Process_1 - enter',
+        'bpmn:Process#Process_1 - leave',
+        'bpmn:Process#Process_2 - enter',
+        'bpmn:Task#Task_2 - enter',
+        'bpmn:Task#Task_2 - leave',
+        'bpmn:Process#Process_2 - leave',
+        'bpmn:Definitions#Definitions - leave'
       ]);
     });
 
@@ -156,13 +328,20 @@ describe('traverse', function() {
 });
 
 
-function createLogger() {
+// helpers ///////////////
+
+function createLogger(hooks = [ 'enter' ]) {
 
   const nodes = [];
 
-  const log = (node) => {
-    nodes.push(node.$type + (node.id ? '#' + node.id : ''));
-  };
+  const log = hooks.reduce((log, hook) => {
+
+    log[hook] = (node) => {
+      nodes.push(node.$type + (node.id ? '#' + node.id : '') + ' - ' + hook);
+    };
+
+    return log;
+  }, {});
 
   return {
     nodes,