Merge pull request #342 from DinoChiesa/issue339

migrate PO00{1,2,3,4,5} to ST00{3,4,5,6,7}

Author: ssvaidyanathan

README.md

@@ -81,9 +81,14 @@ This example uses the "externalPlugins" directory with a plugin for alternate po
 
 ### Listing plugins
 List plugins and formatters, with or without --externalPluginsDirectory.
-```
+```sh
 apigeelint --list
-apigeelint --list -x ./externalPlugins or apigeelint --list --externalPluginsDirectory ./externalPlugins 
+apigeelint --list -x ./externalPlugins
+
+# or
+
+apigeelint --list --externalPluginsDirectory ./externalPlugins
+
  ```
 
 ## Does this tool just lint or does it also check style?
@@ -122,7 +127,7 @@ You can also contribute by reporting issues, asking for new features.
 ## Rules
 
 The list of rules is a work in progress. We expect it to increase over time. As
-product features change (new policies, etc), we will change rules as
+product features change (new policies, deprecated policies, etc), we will change rules as
 well.
 
 This is the current list:
@@ -141,32 +146,32 @@ This is the current list:
 |   |:white_check_mark:| BN009 | Statistics Collector - duplicate policies |  Warn on duplicate policies when no conditions are present or conditions are duplicates. |
 |   |:white_check_mark:| BN010 | Missing policies | Issue an error if a referenced policy is not present in the bundle. |
 | Proxy Definition |   |   |   |   |
-|   |:white_check_mark:| PD001 | RouteRules to Targets | RouteRules should map to defined Targets |
-|   |:white_check_mark:| PD002 | Unreachable Route Rules - defaults |  Only one RouteRule should be present without a condition |
+|   |:white_check_mark:| PD001 | RouteRules to Targets | RouteRules should map to defined Targets. |
+|   |:white_check_mark:| PD002 | Unreachable Route Rules - defaults |  Only one RouteRule should be present without a condition. |
 |   |:white_check_mark:| PD003 | Unreachable Route Rules |  RouteRule without a condition should be last. |
 |   |:white_check_mark:| PD004 | ProxyEndpoint name | ProxyEndpoint name should match basename of filename. |
 |   |:white_check_mark:| PD005 | VirtualHost | ProxyEndpoint should have one HTTPProxyConnection, and in the case of profile=apigeex, no VirtualHost. |
 | Target Definition |   |   |   |   |
 |   |:white_check_mark:| TD001 | Mgmt Server as Target |  Discourage calls to the Management Server from a Proxy via target. |
-|   |:white_check_mark:| TD002 | Use Target Servers |  Encourage the use of target servers |
+|   |:white_check_mark:| TD002 | Use Target Servers |  Encourage the use of target servers. |
 |   |:white_check_mark:| TD003 | TargetEndpoint name | TargetEndpoint name should match basename of filename. |
 |   |:white_check_mark:| TD004 | TargetEndpoint SSLInfo | TargetEndpoint HTTPTargetConnection should enable TLS/SSL. |
 |   |:white_check_mark:| TD005 | TargetEndpoint SSLInfo references | TargetEndpoint SSLInfo should use references for KeyStore and TrustStore. |
 | Flow |   |   |   |   |
 |   |:white_check_mark:| FL001 | Unconditional Flows |  Only one unconditional flow will get executed. Error if more than one was detected. |
 | Step |   |   |   |   |
-|   |:white_check_mark:| ST001 | Empty Step |  Empty steps clutter the bundle. |
+|   |:white_check_mark:| ST001 | Empty Step | Empty steps clutter the bundle. |
 |   |:white_check_mark:| ST002 | Step Structure | each Step should have at most one Name element, one Condition element, no others. |
+|   |:white_check_mark:| ST003 | Extract Variables Step with JSONPayload | A check for message content should be performed before policy execution. |
+|   |:white_check_mark:| ST004 | Extract Variables Step with XMLPayload | A check for message content should be performed before policy execution. |
+|   |:white_check_mark:| ST005 | Extract Variables Step with FormParam | A check for message content should be performed before policy execution. |
+|   |:white_check_mark:| ST006 | JSON Threat Protection Step | A check for message content should be performed before policy execution. |
+|   |:white_check_mark:| ST007 | XML Threat Protection Step | A check for message content should be performed before policy execution. |
 | Policy |   |   |   |   |
-|   |:white_check_mark:| PO001 | JSON Threat Protection |  A check for a body element must be performed before policy execution. |
-|   |:white_check_mark:| PO002 | XML Threat Protection |  A check for a body element must be performed before policy execution. |
-|   |:white_check_mark:| PO003 | Extract Variables with JSONPayload |  A check for a body element must be performed before policy execution. |
-|   |:white_check_mark:| PO004 | Extract Variables with XMLPayload |  A check for a body element must be performed before policy execution. |
-|   |:white_check_mark:| PO005 | Extract Variables with FormParam |  A check for a body element must be performed before policy execution. |
 |   |:white_check_mark:| PO006 | Policy Name & filename agreement |  Policy name attribute should coincide with the policy filename. |
 |   |:white_check_mark:| PO007 | Policy Naming Conventions - type indication |  It is recommended that the policy name use a prefix or follow a pattern that indicates the policy type. |
 |   |:white_check_mark:| PO008 | Policy DisplayName & DisplayName agreement |  Check that the policy filename matches the display name of the policy. |
-|   |:white_medium_square:| PO009 | Service Callout Target - Mgmt Server |  Targeting management server may result in higher than expected latency use with caution. |
+|   |:white_medium_square:| PO009 | Service Callout Target - Mgmt Server |  Targeting management server may result in higher than expected latency; use with caution. |
 |   |:white_medium_square:| PO010 | Service Callout Target - Target Server |  Encourage use of target servers. |
 |   |:white_medium_square:| PO011 | Service Callout Target - Dynamic URLs |  Error on dynamic URLs in target server URL tag. |
 |   |:white_check_mark:| PO012 | AssignMessage/AssignTo | Warn on unnecessary AssignTo in AssignMessage when createNew is false and no destination variable. |
@@ -210,6 +215,28 @@ This is the current list:
 
 From an implementation perspective, the focus is on plugin support and flexibility over performance. Compute is cheap.
 
+## Release Notes
+
+### Release v2.31.0
+
+#### Condition checks around policies
+
+In release v2.31.0, the plugins PO001, PO002, PO003, PO004, and PO005 have been
+converted to ST006, ST007, ST003, ST004, and ST005, respectively.  These plugins
+move from the "Policy" category to the "Step" category because the plugin
+analyzes the attachment of the policy in a Step element, rather than the policy
+itself. Also these plugins will now generate warnings, rather than errors.
+
+If previously you excluded PO003 via the `--excluded` option, you must now
+exclude ST003, and so on.
+
+#### Sharedflows
+
+Starting with release v2.31.0, using apigeelint against Sharedflows will
+generate a correct report. Previously the report on a sharedflow was truncated
+and omitted some warnings and errors.
+
+
 
 ## Support
 

lib/package/Bundle.js

@@ -306,29 +306,20 @@ function Bundle(config, cb) {
 Bundle.prototype.getReport = function(cb) {
   //go out and getReport from endpoints, resources, policies
 
-  var result = [this.report],
-    append = function(a) {
-      a.forEach(function(e) {
-        result.push(e.getReport());
-      });
-    };
+  let result = [this.report];
+  const appendCollection =
+    collection => collection.forEach( item => result.push(item.getReport()) );
 
-  //no endpoints in shared flow
-  if(this.bundleTypeName !== bundleType.BundleType.SHAREDFLOW){
-    append(this.getEndpoints());
-  }
-  append(this.getPolicies());
-  append(this.getResources());
+  appendCollection(this.getEndpoints()); // for either apiproxy or sharedflow
+  appendCollection(this.getPolicies());
+  appendCollection(this.getResources());
   if (cb) {
     cb(result);
   }
   return result;
 };
 
 Bundle.prototype.addMessage = function(msg) {
-  //lets inspect what we got
-  //if it includes a plugin field
-  //then process new style
   if (msg.hasOwnProperty("plugin")) {
     msg.ruleId = msg.plugin.ruleId;
     if (!msg.severity) msg.severity = msg.plugin.severity;
@@ -563,7 +554,7 @@ Bundle.prototype.getDefaultFaultRules = function() {
 };
 
 Bundle.prototype.summarize = function() {
-  var summary = {
+  let summary = {
     report: this.getReport(),
     root: this.root,
     name: this.getName(),

lib/package/Endpoint.js

@@ -31,7 +31,7 @@ function Endpoint(element, parent, fname, bundletype) {
   debug(`> new Endpoint() ${fname}`);
   this.bundleType = bundletype ? bundletype : "apiproxy" //default to apiproxy if bundletype not passed
   this.fileName = fname;
-  this.parent = parent;
+  this.parent = parent; // the bundle
   this.element = element;
   this.report = {
     filePath: fname.substring(fname.indexOf("/" + this.bundleType)),
@@ -169,7 +169,6 @@ Endpoint.prototype.getPostClientFlow = function() {
 
 Endpoint.prototype.getSharedFlow = function() {
   if (!this.sharedFlow) {
-    //find the preflow tag
     var doc = xpath.select(".", this.element);
     if (doc && doc[0]) {
       this.sharedFlow = new Flow(doc[0], this);
@@ -230,7 +229,9 @@ Endpoint.prototype.getFlows = function() {
             else {
               debug(`Flow`);
             }
-            flows.push(new Flow(elt, ep));
+            let f = new Flow(elt, ep);
+            debug(`getFlows() flow= ${f.element.tagName}`);
+            flows.push(f);
           });
       });
     }
@@ -428,14 +429,15 @@ Endpoint.prototype.getParent = function() {
 };
 
 Endpoint.prototype.addMessage = function(msg) {
+  debug(`> addMessage`);
   //Severity should be one of the following: 0 = off, 1 = warning, 2 = error
   if (msg.hasOwnProperty("plugin")) {
     msg.ruleId = msg.plugin.ruleId;
     if (!msg.severity) msg.severity = msg.plugin.severity;
     msg.nodeType = msg.plugin.nodeType;
     delete msg.plugin;
   }
-  debug(`Endpoint.addMessage ${msg.ruleId}: ${msg.message}`);
+  debug(`addMessage ${msg.ruleId}: ${msg.message}`);
 
   if (!msg.hasOwnProperty("entity")) {
     msg.entity = this;
@@ -466,37 +468,42 @@ Endpoint.prototype.addMessage = function(msg) {
 };
 
 Endpoint.prototype.summarize = function() {
-  var summary = {};
+  debug('> summarize');
+  let summary = {
+        name : this.getName(),
+        type : this.getType(),
+      };
+
+  if(this.bundleType === bundleTypes.BundleType.SHAREDFLOW){
+    debug('summarize - is SharedFlow');
+    summary.flows =
+      this.getSharedFlow().summarize();
+  }
+  else {
+    debug('summarize - is Apiproxy');
+    summary.proxyName = this.getProxyName();
+
+    let faultRules = this.getFaultRules();
+    if (faultRules) {
+      summary.faultRules = [];
+      faultRules.forEach(function(fr) {
+        summary.faultRules.push(fr.summarize());
+      });
+    }
 
-  summary.name = this.getName();
-  summary.type = this.getType();
-  summary.proxyName = this.getProxyName();
+    summary.defaultFaultRule =
+      (this.getDefaultFaultRule() && this.getDefaultFaultRule().summarize()) ||
+      {};
 
-  var faultRules = this.getFaultRules();
-  if (faultRules) {
-    summary.faultRules = [];
-    faultRules.forEach(function(fr) {
-      summary.faultRules.push(fr.summarize());
-    });
-  }
+    summary.preFlow = (this.getPreFlow() && this.getPreFlow().summarize()) || {};
 
-  summary.defaultFaultRule =
-    (this.getDefaultFaultRule() && this.getDefaultFaultRule().summarize()) ||
-    {};
+    summary.flows = this.getFlows().map(flow => flow.summarize());
 
-  summary.preFlow = (this.getPreFlow() && this.getPreFlow().summarize()) || {};
-
-  summary.flows = [];
-  this.getFlows().forEach(function(flow) {
-    summary.flows.push(flow.summarize());
-  });
-  summary.postFlow =
-    (this.getPostFlow() && this.getPostFlow().summarize()) || {};
-  summary.routeRules = [];
-  this.getRouteRules().forEach(function(rr) {
-    summary.routeRules.push(rr.summarize());
-  });
+    summary.postFlow =
+      (this.getPostFlow() && this.getPostFlow().summarize()) || {};
+    summary.routeRules = this.getRouteRules().map(rr => rr.summarize());
 
+  }
   return summary;
 };
 

lib/package/Flow.js

@@ -22,19 +22,22 @@ const xpath = require("xpath"),
       getcb = myUtil.curry(myUtil.diagcb, debug);
 
 function Flow(element, parent) {
-  this.parent = parent;
+  this.parent = parent; // like ProxyEndpoint , TargetEndpoint
   this.element = element;
   this.condition = null;
   this.flowResponse = null;
   this.sharedflow = null;
   this.description = null;
+  let self = this;
+  debug(`Flow ctor: self: ${self.element.tagName}`);
 }
 
 Flow.prototype.getName = function() {
   if (!this.name) {
     var attr = xpath.select("./@name", this.element);
     this.name = (attr[0] && attr[0].value) || "";
   }
+  debug(`getName(): self: ${self.element.tagName}, name: ${this.name}`);
   return this.name;
 };
 
@@ -43,9 +46,10 @@ Flow.prototype.getMessages = function() {
 };
 
 Flow.prototype.getType = function() {
-  //lets go ahead and look this up
   if (!this.type) {
-    this.type = xpath.select("name(/*)", this.element);
+    // Flow | PreFlow || PostFlow || PostClientFlow
+    //this.type = xpath.select("name(/*)", this.element);
+    this.type = this.element.tagName;
   }
   return this.type;
 };
@@ -93,7 +97,8 @@ Flow.prototype.getFlowRequest = function() {
 };
 
 Flow.prototype.getFlowResponse = function() {
-  if (this.flowResponse == null) {
+  let self = this;
+  if ( ! this.flowResponse) {
     let doc = xpath.select("./Response", this.element);
     this.flowResponse = (doc && doc[0]) ? new FlowPhase(doc[0], this) : false;
   }

lib/package/FlowPhase.js

@@ -23,6 +23,7 @@ const xpath = require("xpath"),
 function FlowPhase(element, parent) {
   this.parent = parent;
   this.element = element;
+  debug(`parent is ${parent.getType()}`);
 }
 
 FlowPhase.prototype.getType = function() {
@@ -62,7 +63,7 @@ FlowPhase.prototype.onSteps = function(pluginFunction, cb) {
   let steps = this.getSteps();
   if (steps && steps.length > 0) {
     steps.forEach( (step, ix) =>
-        pluginFunction(step, getcb(`step ${ix}}`)));
+        pluginFunction(step, getcb(`step ${ix}`)));
   }
   cb(null, {});
 };
@@ -71,7 +72,7 @@ FlowPhase.prototype.onConditions = function(pluginFunction, cb) {
   let steps = this.getSteps();
   if (steps && steps.length > 0) {
     steps.forEach( (step, ix) =>
-        step.onConditions(pluginFunction, getcb(`step ${ix}}`)));
+        step.onConditions(pluginFunction, getcb(`step ${ix}`)));
   }
   cb(null, {});
 };