fix: PO012 (AssignTo hygiene) now handles missing attributes

apigee · Apr 10, 2024 · 2d6973e · 2d6973e
1 parent d2a52a6
commit 2d6973e
Show file tree

Hide file tree

Showing 7 changed files with 213 additions and 144 deletions.
diff --git a/lib/package/Policy.js b/lib/package/Policy.js
@@ -39,15 +39,14 @@ function Policy(path, fn, parent, doc) {
 
 Policy.prototype.getName = function () {
   if (!this.name) {
-    var attr = xpath.select("//@name", this.getElement());
+    let attr = xpath.select("//@name", this.getElement());
     this.name = (attr[0] && attr[0].value) || "";
   }
   return this.name;
 };
 
 Policy.prototype.getLines = function (start, stop) {
   //actually parse the source into lines if we haven't already and return the requested subset
-  var result = "";
   if (!this.lines) {
     this.lines = fs.readFileSync(this.filePath).toString().split("\n");
   }
@@ -68,16 +67,12 @@ Policy.prototype.getLines = function (start, stop) {
     start = stop;
   }
 
-  for (var i = start; i <= stop; i++) {
-    result += this.lines[i] + "\n";
-  }
-
-  return result;
+  return this.lines.join("\n");
 };
 
 Policy.prototype.getSource = function () {
   if (!this.source) {
-    var start = this.getElement().lineNumber - 1,
+    const start = this.getElement().lineNumber - 1,
       stop = Number.MAX_SAFE_INTEGER;
     if (
       this.getElement().nextSibling &&
@@ -93,7 +88,7 @@ Policy.prototype.getSource = function () {
 Policy.prototype.getDisplayName = function () {
   if (typeof this.displayName === "undefined") {
     this.getName();
-    let nodes = xpath.select("//DisplayName", this.getElement());
+    const nodes = xpath.select("//DisplayName", this.getElement());
     if (nodes && nodes[0]) {
       if (nodes[0].childNodes && nodes[0].childNodes[0]) {
         this.displayName = nodes[0].childNodes[0].nodeValue;
@@ -117,11 +112,10 @@ Policy.prototype.select = function (xs) {
 };
 
 Policy.prototype.getElement = function () {
-  //read the contents of the file and return it raw
   if (!this.element) {
     this.element = new Dom().parseFromString(
       fs.readFileSync(this.filePath).toString()
-    );
+    ).documentElement;
   }
   return this.element;
 };
@@ -132,13 +126,14 @@ Policy.prototype.getFileName = function () {
 
 Policy.prototype.getType = function () {
   if (!this.type) {
-    var doc = xpath.select("/", this.getElement());
+    const root = xpath.select("/", this.getElement());
     this.type =
-      (doc &&
-        doc[0] &&
-        doc[0].documentElement &&
-        doc[0].documentElement.tagName) ||
+      (root &&
+        root[0] &&
+        root[0].documentElement &&
+        root[0].documentElement.tagName) ||
       "";
+    debug(`getType() policyType: ${this.type}`);
     if (this.type === "DisplayName") {
       this.type = "";
     }

diff --git a/lib/package/plugins/PO012-assignMessageAssignTo.js b/lib/package/plugins/PO012-assignMessageAssignTo.js
@@ -1,5 +1,5 @@
 /*
-  Copyright 2019-2020 Google LLC
+  Copyright 2019-2024 Google LLC
 
   Licensed under the Apache License, Version 2.0 (the "License");
   you may not use this file except in compliance with the License.
@@ -15,69 +15,104 @@
 */
 
 const xpath = require("xpath"),
-      util = require('util'),
-      ruleId = require("../myUtil.js").getRuleId(),
-      debug = require("debug")("apigeelint:" + ruleId);
+  util = require("util"),
+  ruleId = require("../myUtil.js").getRuleId(),
+  debug = require("debug")("apigeelint:" + ruleId);
 
 const plugin = {
-        ruleId,
-        name: "AssignTo",
-        fatal: false,
-        severity: 1, // warning
-        nodeType: "AssignMessage",
-        enabled: true
-      };
+  ruleId,
+  name: "AssignTo",
+  fatal: false,
+  severity: 1, // warning
+  nodeType: "AssignMessage",
+  enabled: true
+};
+
+const onPolicy = function (policy, cb) {
+  let flagged = false;
+  const addMessage = (line, column, message) => {
+    policy.addMessage({ plugin, message, line, column });
+    flagged = true;
+  };
+  try {
+    if (policy.getType() === "AssignMessage") {
+      const atNodes = xpath.select(
+        "/AssignMessage/AssignTo",
+        policy.getElement()
+      );
+      const formatAttrs = (node) =>
+        node.attributes && node.attributes.length
+          ? `(attrs: ${Array.prototype.map
+              .call(node.attributes, (at) => at.localName)
+              .join(", ")})`
+          : "(no attributes)";
 
-const onPolicy = function(policy, cb) {
-        let flagged = false;
-        const addMessage = (line, column, message) => {
-                policy.addMessage({plugin, message, line, column});
-                flagged = true;
-              };
-        try {
-          if (policy.getType() === "AssignMessage") {
-            const atNodes = xpath.select("/AssignMessage/AssignTo", policy.getElement());
-            if (atNodes && atNodes.length>0) {
-              debug(`${policy.fileName} found ${atNodes.length} AssignTo elements`);
-              if (atNodes.length>1) {
-                atNodes.slice(1).forEach( (node, ix) =>
-                                          addMessage(node.lineNumber, node.columnNumber, "extraneous AssignTo element"));
-              }
-              else {
-                const node = atNodes[0];
-                debug(`node: ` + util.format(node));
-                const textNodes = xpath.select('text()', node);
-                const text = textNodes[0] && textNodes[0].data;
-                debug(`text: ` + text);
-                const attrs = ['transport', 'createNew', 'type']
-                  .reduce((map, item) => {
-                    map[item] = xpath.select1('@' + item, node).value;
-                    return map;
-                  }, {});
-                debug(`attrs: ` + JSON.stringify(attrs));
-                if (attrs.transport && attrs.transport != 'http') {
-                  addMessage(node.lineNumber, node.columnNumber, "unsupported transport attribute in AssignTo element");
-                }
-                if (attrs.createNew && attrs.createNew == 'false' && !text) {
-                  addMessage(node.lineNumber, node.columnNumber, "unnecessary AssignTo with no named message");
-                }
-                if (attrs.type && attrs.type != 'request' && attrs.type != 'response') {
-                  addMessage(node.lineNumber, node.columnNumber, "unrecognized type attribute in AssignTo");
-                }
-              }
-            }
-            else {
-              debug(`${policy.fileName} found no AssignTo elements`);
-            }
+      debug(`policy ${policy.fileName} parent ${policy.parent.root}`);
+      debug(`${policy.fileName} atNodes ${util.format(atNodes)}`);
+      if (atNodes && atNodes.length > 0) {
+        debug(`${policy.fileName} found ${atNodes.length} AssignTo elements`);
+        if (atNodes.length > 1) {
+          atNodes
+            .slice(1)
+            .forEach((node, _ix) =>
+              addMessage(
+                node.lineNumber,
+                node.columnNumber,
+                "extraneous AssignTo element"
+              )
+            );
+        } else {
+          const node = atNodes[0];
+          debug(`node: ${node.tagName} ${formatAttrs(node)}`);
+          const textNodes = xpath.select("text()", node);
+          const text = textNodes[0] && textNodes[0].data;
+          debug(`text: ` + text);
+          const attrs = ["transport", "createNew", "type"].reduce(
+            (map, item) => {
+              const attr = xpath.select1("@" + item, node);
+              map[item] = attr ? attr.value : null;
+              return map;
+            },
+            {}
+          );
+          debug(`attrs: ` + JSON.stringify(attrs));
+          if (attrs.transport && attrs.transport != "http") {
+            addMessage(
+              node.lineNumber,
+              node.columnNumber,
+              "unsupported transport attribute in AssignTo element"
+            );
           }
-          if (typeof(cb) == 'function') {
-            cb(null, flagged);
+          if (attrs.createNew && attrs.createNew == "false" && !text) {
+            addMessage(
+              node.lineNumber,
+              node.columnNumber,
+              "unnecessary AssignTo with no named message"
+            );
+          }
+          if (
+            attrs.type &&
+            attrs.type != "request" &&
+            attrs.type != "response"
+          ) {
+            addMessage(
+              node.lineNumber,
+              node.columnNumber,
+              "unrecognized type attribute in AssignTo"
+            );
           }
         }
-        catch(e) {
-          debug(util.format(e));
-        }
-      };
+      } else {
+        debug(`${policy.fileName} found no AssignTo elements`);
+      }
+    }
+    if (typeof cb == "function") {
+      cb(null, flagged);
+    }
+  } catch (e) {
+    debug(util.format(e));
+  }
+};
 
 module.exports = {
   plugin,

diff --git a/lib/package/plugins/PO034-am-hygiene.js b/lib/package/plugins/PO034-am-hygiene.js
@@ -126,7 +126,7 @@ const onPolicy = function (policy, cb) {
         "AssignVariable"
       ].reduce(
         (a, action, _ix) =>
-          a + xpath.select(`AssignMessage/${action}`, policyRoot).length,
+          a + xpath.select(`/AssignMessage/${action}`, policyRoot).length,
         0
       );
 
@@ -141,7 +141,10 @@ const onPolicy = function (policy, cb) {
       }
 
       // check for unknown/unsupported elements at the top level
-      const foundTopLevelChildren = xpath.select("AssignMessage/*", policyRoot);
+      const foundTopLevelChildren = xpath.select(
+        "/AssignMessage/*",
+        policyRoot
+      );
       debug(`found ${foundTopLevelChildren.length} toplevel children...`);
       foundTopLevelChildren.forEach((child) => {
         debug(`toplevel child: ${child.tagName}...`);

diff --git a/...res/resources/PO012-assignToHygiene/apiproxy/policies/AM-Inject-Proxy-Revision-Header.xml b/...res/resources/PO012-assignToHygiene/apiproxy/policies/AM-Inject-Proxy-Revision-Header.xml
@@ -0,0 +1,8 @@
+<AssignMessage name='AM-Inject-Proxy-Revision-Header'>
+  <Set>
+    <Headers>
+      <Header name='APIProxy'>{apiproxy.name} r{apiproxy.revision}</Header>
+    </Headers>
+  </Set>
+  <AssignTo createNew='false' type='request'/>
+</AssignMessage>
diff --git a/...policies/AM-ModifyRequestRemoveApiKey.xml → ...icies/AM-Modify-Request-Remove-ApiKey.xml b/...policies/AM-ModifyRequestRemoveApiKey.xml → ...icies/AM-Modify-Request-Remove-ApiKey.xml
@@ -1,11 +1,10 @@
 <?xml version="1.0" encoding="UTF-8" standalone="yes"?>
-<AssignMessage async="false" continueOnError="false" enabled="true" name="AM-ModifyRequestRemoveApiKey">
+<AssignMessage async="false" continueOnError="false" enabled="true" name="AM-Modify-Request-Remove-ApiKey">
     <!--
        here you can use this policy to remove, add, set request parameters before sending the request
        to backend. Here we are just removing x-api-key as it is not needed by backend.
     -->
-    <DisplayName>AM-ModifyRequestRemoveApiKey</DisplayName>
-    <Properties/>
+    <DisplayName>AM-Modify-Request-Remove-ApiKey</DisplayName>
     <Remove>
         <Headers>
             <Header name="x-api-key"/>
@@ -18,4 +17,4 @@
     </AssignVariable>
     <IgnoreUnresolvedVariables>true</IgnoreUnresolvedVariables>
     <AssignTo createNew="false" transport="http" type="request"/>
-</AssignMessage>
+</AssignMessage>
diff --git a/test/fixtures/resources/PO012-assignToHygiene/apiproxy/proxies/default.xml b/test/fixtures/resources/PO012-assignToHygiene/apiproxy/proxies/default.xml
@@ -4,25 +4,25 @@
 ProxyEndpoint
 =============
 
-The ProxyEndpoint configuration defines the inbound (client-facing) interface for an API proxy. 
+The ProxyEndpoint configuration defines the inbound (client-facing) interface for an API proxy.
 When you configure a ProxyEndpoint, you are setting up a network configuration that defines how client applications ('apps') should invoke the proxied API.
 While you can configure multiple ProxyEndpoints in a single API proxy, it is antipattern and should be avoided as it makes the proxy difficult to read and troubleshoot.
 
 ProxyEndpoint Pipeline can be summarized in the below graph:
 
 Front-End App ===> apigee (PreFlow  (Request)  ==> Flows (Request)  ==> Target-Endpoint(Request))  ==> Backend APIs
-              <=== apigee (PostFlow (Response) <== Flows (Response) <== Target-Endpoint(Response)) <==   
-              
+              <=== apigee (PostFlow (Response) <== Flows (Response) <== Target-Endpoint(Response)) <==
+
 ProxyEndpoint has the following elements to be configured:
     - PreFlow: Defines the policies in the PreFlow flow of a request or response.
     - Flows: Defines the policies in the conditional flows of a request or response.
     - PostFlow: Defines the policies in the PostFlow flow of a request or response.
     - DefaultFaultRule: Handles all errors raised by apigee or backend
     - HTTPProxyConnection: Defines the network address and URI path associated with the API proxy
         - BasePath: A required string that uniquely identifies the URI path used by Apigee Edge to route incoming messages to the proper API proxy.
-        - VirtualHost: defines which apigee virtual host would be used for this proxy. 
-    - RouteRule: Defines the destination of inbound request messages after processing by the ProxyEndpoint request pipeline. 
-    
+        - VirtualHost: defines which apigee virtual host would be used for this proxy.
+    - RouteRule: Defines the destination of inbound request messages after processing by the ProxyEndpoint request pipeline.
+
 -->
 <ProxyEndpoint name="default">
     <PreFlow name="PreFlow">
@@ -77,7 +77,7 @@ ProxyEndpoint has the following elements to be configured:
                     <Name>JS-ValidateAndSanitizeRequest</Name>
                 </Step>
                 <Step>
-                    <Name>AM-ModifyRequestRemoveApiKey</Name>
+                    <Name>AM-Modify-Request-Remove-ApiKey</Name>
                 </Step>
             </Request>
             <Response/>
@@ -109,6 +109,9 @@ ProxyEndpoint has the following elements to be configured:
         </Response>
     </PostFlow>
     <DefaultFaultRule name="default-fault">
+        <Step>
+            <Name>AM-Inject-Proxy-Revision-Header</Name>
+        </Step>
         <Step>
             <Name>AM-AddCORS</Name>
         </Step>
@@ -133,4 +136,4 @@ ProxyEndpoint has the following elements to be configured:
     <RouteRule name="default">
         <TargetEndpoint>default</TargetEndpoint>
     </RouteRule>
-</ProxyEndpoint>
+</ProxyEndpoint>