From fcda555d620acfe4146b4a2138d74acb9bdf1345 Mon Sep 17 00:00:00 2001 From: Dino Chiesa Date: Mon, 2 Oct 2023 16:49:07 -0700 Subject: [PATCH] fix: correct BN009 behavior for duplicate StatsCollector - correct error messages. - eliminate extraneous errors on the bundle. - introduce tests to verify expected behavior. The proxies were present but never included in tests. - refactor test proxies to be sensible. - remove one orphaned, duplicate test proxy --- lib/package/Bundle.js | 410 +++++++++--------- ...-checkForMultipleStatsCollectorPolicies.js | 251 +++++------ .../plugins/EP002-misplacedElements.js | 392 +++++++++++------ .../plugins/FL001-unconditionalFlows.js | 124 +++--- .../duplicates/apiproxy/duplicates.xml | 23 + .../apiproxy/policies/Stats-Address-1.xml} | 10 +- .../apiproxy/policies/Stats-Address-1a.xml} | 10 +- .../apiproxy/policies/Stats-Username-1.xml} | 10 +- .../apiproxy/policies/Stats-Username-1a.xml} | 10 +- .../apiproxy/proxies/default.xml | 15 +- .../apiproxy/targets/default.xml | 4 +- .../apiproxy/multiple_missing_conditions.xml | 22 + .../apiproxy/policies/Stats-1.xml} | 8 +- .../apiproxy/policies/Stats-2.xml} | 10 +- .../apiproxy/policies/Stats-3.xml} | 10 +- .../apiproxy/policies/Stats-4.xml} | 10 +- .../apiproxy/proxies/default.xml | 15 +- .../apiproxy/targets/default.xml | 0 .../policies/StatisticsCollector1.xml | 24 - .../policies/StatisticsCollector10.xml | 24 - .../no_condition/apiproxy/no_condition.xml | 22 + .../apiproxy/policies/Stats-1.xml | 22 + .../apiproxy/proxies/default.xml | 9 +- .../apiproxy/targets/default.xml | 4 +- .../apiproxy/proxies/default.xml | 48 -- .../policies/StatisticsCollector1.xml | 24 - .../policies/StatisticsCollector10.xml | 24 - .../apiproxy/targets/default.xml | 33 -- .../apiproxy/policies/Stats-1.xml | 22 + .../apiproxy/proxies/default.xml | 11 +- .../apiproxy/targets/default.xml | 4 +- .../apiproxy/twosteps_one_condition.xml | 23 + .../policies/StatisticsCollector1.xml | 3 +- .../policies/StatisticsCollector10.xml | 2 +- .../policies/StatisticsCollectorAddress.xml | 2 +- .../policies/StatisticsCollectorFlows.xml | 2 +- .../policies/StatisticsCollectorFlows2.xml | 2 +- .../policies/StatisticsCollectorZip.xml | 2 +- .../test_flow/apiproxy/proxies/default.xml | 2 +- .../test_flow/apiproxy/targets/default.xml | 2 +- .../test_flow/apiproxy/test_flow.xml | 6 + test/specs/BN009-MultipleStatsCollectors.js | 194 ++++++++- 42 files changed, 1009 insertions(+), 836 deletions(-) create mode 100644 test/fixtures/resources/statistics_collector/duplicates/apiproxy/duplicates.xml rename test/fixtures/resources/statistics_collector/{two_duplicate_stats_collector_on_conditions/apiproxy/policies/StatisticsCollectorZip.xml => duplicates/apiproxy/policies/Stats-Address-1.xml} (75%) rename test/fixtures/resources/statistics_collector/{two_duplicate_stats_collector_on_conditions/apiproxy/policies/StatisticsCollectorAddress.xml => duplicates/apiproxy/policies/Stats-Address-1a.xml} (75%) rename test/fixtures/resources/statistics_collector/{one_stats_collector_twosteps_one_condition/one_stats_collector_twosteps_one_condition/apiproxy/policies/StatisticsCollector1.xml => duplicates/apiproxy/policies/Stats-Username-1.xml} (71%) rename test/fixtures/resources/statistics_collector/{one_stats_collector_no_condition/apiproxy/policies/StatisticsCollector1.xml => duplicates/apiproxy/policies/Stats-Username-1a.xml} (71%) rename test/fixtures/resources/statistics_collector/{multiple_stats_collector_missing_conditions => duplicates}/apiproxy/proxies/default.xml (80%) rename test/fixtures/resources/statistics_collector/{one_stats_collector_no_condition => duplicates}/apiproxy/targets/default.xml (96%) create mode 100644 test/fixtures/resources/statistics_collector/multiple_missing_conditions/apiproxy/multiple_missing_conditions.xml rename test/fixtures/resources/statistics_collector/{one_stats_collector_twosteps_one_condition/one_stats_collector_twosteps_one_condition/apiproxy/policies/StatisticsCollector10.xml => multiple_missing_conditions/apiproxy/policies/Stats-1.xml} (74%) rename test/fixtures/resources/statistics_collector/{multiple_stats_collector_missing_conditions/apiproxy/policies/StatisticsCollectorZip.xml => multiple_missing_conditions/apiproxy/policies/Stats-2.xml} (73%) rename test/fixtures/resources/statistics_collector/{one_stats_collector_twosteps_one_condition/apiproxy/policies/StatisticsCollector1.xml => multiple_missing_conditions/apiproxy/policies/Stats-3.xml} (71%) rename test/fixtures/resources/statistics_collector/{multiple_stats_collector_missing_conditions/apiproxy/policies/StatisticsCollectorAddress.xml => multiple_missing_conditions/apiproxy/policies/Stats-4.xml} (75%) rename test/fixtures/resources/statistics_collector/{two_duplicate_stats_collector_on_conditions => multiple_missing_conditions}/apiproxy/proxies/default.xml (80%) rename test/fixtures/resources/statistics_collector/{multiple_stats_collector_missing_conditions => multiple_missing_conditions}/apiproxy/targets/default.xml (100%) delete mode 100644 test/fixtures/resources/statistics_collector/multiple_stats_collector_missing_conditions/apiproxy/policies/StatisticsCollector1.xml delete mode 100644 test/fixtures/resources/statistics_collector/multiple_stats_collector_missing_conditions/apiproxy/policies/StatisticsCollector10.xml create mode 100644 test/fixtures/resources/statistics_collector/no_condition/apiproxy/no_condition.xml create mode 100644 test/fixtures/resources/statistics_collector/no_condition/apiproxy/policies/Stats-1.xml rename test/fixtures/resources/statistics_collector/{one_stats_collector_no_condition => no_condition}/apiproxy/proxies/default.xml (87%) rename test/fixtures/resources/statistics_collector/{one_stats_collector_twosteps_one_condition => no_condition}/apiproxy/targets/default.xml (96%) delete mode 100644 test/fixtures/resources/statistics_collector/one_stats_collector_twosteps_one_condition/apiproxy/proxies/default.xml delete mode 100644 test/fixtures/resources/statistics_collector/two_duplicate_stats_collector_on_conditions/apiproxy/policies/StatisticsCollector1.xml delete mode 100644 test/fixtures/resources/statistics_collector/two_duplicate_stats_collector_on_conditions/apiproxy/policies/StatisticsCollector10.xml delete mode 100644 test/fixtures/resources/statistics_collector/two_duplicate_stats_collector_on_conditions/apiproxy/targets/default.xml create mode 100644 test/fixtures/resources/statistics_collector/twosteps_one_condition/apiproxy/policies/Stats-1.xml rename test/fixtures/resources/statistics_collector/{one_stats_collector_twosteps_one_condition/one_stats_collector_twosteps_one_condition => twosteps_one_condition}/apiproxy/proxies/default.xml (85%) rename test/fixtures/resources/statistics_collector/{one_stats_collector_twosteps_one_condition/one_stats_collector_twosteps_one_condition => twosteps_one_condition}/apiproxy/targets/default.xml (96%) create mode 100644 test/fixtures/resources/statistics_collector/twosteps_one_condition/apiproxy/twosteps_one_condition.xml create mode 100644 test/fixtures/resources/test_flow/apiproxy/test_flow.xml diff --git a/lib/package/Bundle.js b/lib/package/Bundle.js index 97d8d95e..2995c811 100644 --- a/lib/package/Bundle.js +++ b/lib/package/Bundle.js @@ -1,5 +1,5 @@ /* - Copyright 2019-2022 Google LLC + Copyright 2019-2023 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,36 +15,32 @@ */ const findFolders = require("./findFolders.js"), - decompress = require("decompress"), - fs = require("fs"), - path = require("path"), - Resource = require("./Resource.js"), - Policy = require("./Policy.js"), - Endpoint = require("./Endpoint.js"), - xpath = require("xpath"), - Dom = require("@xmldom/xmldom").DOMParser, - bundleType = require('./BundleTypes.js'), - debug = require("debug")("apigeelint:Bundle"), - myUtil = require("./myUtil.js"), - getcb = myUtil.curry(myUtil.diagcb, debug); + decompress = require("decompress"), + fs = require("fs"), + path = require("path"), + Resource = require("./Resource.js"), + Policy = require("./Policy.js"), + Endpoint = require("./Endpoint.js"), + xpath = require("xpath"), + Dom = require("@xmldom/xmldom").DOMParser, + bundleType = require("./BundleTypes.js"), + debug = require("debug")("apigeelint:Bundle"), + myUtil = require("./myUtil.js"), + getcb = myUtil.curry(myUtil.diagcb, debug); function _buildEndpoints(folder, tag, bundle, processFunction) { try { if (fs.existsSync(folder)) { - var files = fs.readdirSync(folder); - files.forEach(function(proxyFile) { + fs.readdirSync(folder).forEach(function (proxyFile) { //can't be a directory //must end in .xml - - var fname = folder + proxyFile; + const fname = path.join(folder, proxyFile); if (proxyFile.endsWith(".xml") && fs.lstatSync(fname).isFile()) { - var doc = xpath.select( + const doc = xpath.select( tag, new Dom().parseFromString(fs.readFileSync(fname).toString()) ); - doc.forEach(function(element) { - processFunction(element, fname, bundle); - }); + doc.forEach((element) => processFunction(element, fname, bundle)); } }); } @@ -57,7 +53,7 @@ function _buildEndpoints(folder, tag, bundle, processFunction) { function buildProxyEndpoints(bundle) { var folder = bundle.proxyRoot + "/proxies/", tag = "ProxyEndpoint", - processFunction = function(element, fname, bundle) { + processFunction = function (element, fname, bundle) { bundle.proxyEndpoints = bundle.proxyEndpoints || []; bundle.proxyEndpoints.push(new Endpoint(element, bundle, fname)); }; @@ -67,17 +63,19 @@ function buildProxyEndpoints(bundle) { function buildsharedflows(bundle) { var folder = bundle.proxyRoot + "/sharedflows/", tag = "SharedFlow", - processFunction = function(element, fname, bundle) { + processFunction = function (element, fname, bundle) { bundle.proxyEndpoints = bundle.proxyEndpoints || []; - bundle.proxyEndpoints.push(new Endpoint(element, bundle, fname, bundleType.BundleType.SHAREDFLOW)); + bundle.proxyEndpoints.push( + new Endpoint(element, bundle, fname, bundleType.BundleType.SHAREDFLOW) + ); }; _buildEndpoints(folder, tag, bundle, processFunction); } function buildTargetEndpoints(bundle) { - var folder = bundle.proxyRoot + "/targets/", + const folder = bundle.proxyRoot + "/targets/", tag = "TargetEndpoint", - processFunction = function(element, fname, bundle) { + processFunction = function (element, fname, bundle) { bundle.targetEndpoints = bundle.targetEndpoints || []; bundle.targetEndpoints.push(new Endpoint(element, bundle, fname)); }; @@ -88,29 +86,33 @@ function buildPolicies(bundle) { //get the list of policies and create the policy objects bundle.policies = []; if (fs.existsSync(bundle.proxyRoot + "/policies")) { - var files = fs.readdirSync(bundle.proxyRoot + "/policies"); - files.forEach(function(policyFile) { - var ext = policyFile.split(".").pop(); - if (ext === "xml") { - bundle.policies.push( - new Policy(bundle.proxyRoot + "/policies", policyFile, bundle) - ); + fs.readdirSync(bundle.proxyRoot + "/policies").forEach( + function (policyFile) { + const ext = policyFile.split(".").pop(); + if (ext === "xml") { + bundle.policies.push( + new Policy(bundle.proxyRoot + "/policies", policyFile, bundle) + ); + } } - }); + ); } } function _buildResources(parent, path, resources) { - //given the passed path append resources - //if the path is dir then recurse - var files = fs.readdirSync(path); - files.filter(file => file !== 'node_modules').forEach(function(policyFile) { - if (fs.statSync(path + "/" + policyFile).isDirectory()) { - _buildResources(parent, path + "/" + policyFile, resources); - } else if (policyFile !== ".DS_Store") { - resources.push(new Resource(parent, path + "/" + policyFile, policyFile)); - } - }); + // Append the resources found in the passed path. + // If the path is a directory, then recurse. + fs.readdirSync(path) + .filter((file) => !["node_modules", ".DS_Store"].includes(file)) + .forEach(function (policyFile) { + if (fs.statSync(path + "/" + policyFile).isDirectory()) { + _buildResources(parent, path + "/" + policyFile, resources); + } else if (!policyFile.endsWith("~")) { + resources.push( + new Resource(parent, path + "/" + policyFile, policyFile) + ); + } + }); } function buildResources(bundle) { @@ -121,21 +123,23 @@ function buildResources(bundle) { } } -Bundle.prototype.getElement = function() { - //read the contents of the file and return it raw +Bundle.prototype.getElement = function () { + // read the contents of the file and return it raw if (!this.element) { + const filePath = this.filePath; + debug(`getElement: filePath:${filePath}`); this.element = new Dom().parseFromString( - fs.readFileSync(this.filePath).toString() + fs.readFileSync(filePath).toString() ); } return this.element; }; -Bundle.prototype.getName = function() { +Bundle.prototype.getName = function () { if (!this.name) { //look at the root of the proxy for the .xml file and get name from there try { - var attr = xpath.select("/" + this.xPathName + "/@name", this.getElement()); + const attr = xpath.select(`/${this.xPathName}/@name`, this.getElement()); this.name = (attr[0] && attr[0].value) || "undefined"; } catch (e) { this.name = "undefined"; @@ -144,11 +148,14 @@ Bundle.prototype.getName = function() { return this.name; }; -Bundle.prototype.getRevision = function() { +Bundle.prototype.getRevision = function () { if (!this.revision) { //look at the root of the proxy for the .xml file and get name from there try { - var attr = xpath.select("/" + this.xPathName + "/@revision", this.getElement()); + const attr = xpath.select( + `/${this.xPathName}/@revision`, + this.getElement() + ); this.revision = (attr[0] && attr[0].value) || "undefined"; } catch (e) { this.revision = "undefined"; @@ -163,7 +170,7 @@ function buildAll(bundle, bundleName) { buildProxyEndpoints(bundle); //no targets for shared flow - if(bundleName !== bundleType.BundleType.SHAREDFLOW){ + if (bundleName !== bundleType.BundleType.SHAREDFLOW) { buildTargetEndpoints(bundle); } } @@ -173,12 +180,17 @@ function processFileSystem(config, bundle, cb) { // Normalize the root path. Convert all path separator to forward slash. // This is used in error and warning messages. - bundle.root = path.resolve(config.source.path).split(path.sep).join('/'); + bundle.root = path.resolve(config.source.path).split(path.sep).join("/"); // Populate the filepath by looking at the only .xml file // present at the bundle.root. - let files = fs.readdirSync(bundle.root) - .filter( file => fs.lstatSync(path.join(bundle.root, file)).isFile() && file.endsWith('.xml') ); + const files = fs + .readdirSync(bundle.root) + .filter( + (file) => + fs.lstatSync(path.join(bundle.root, file)).isFile() && + file.endsWith(".xml") + ); if (files.length > 1) { debug("More than one .xml file found at proxy root. Aborting."); @@ -186,7 +198,7 @@ function processFileSystem(config, bundle, cb) { bundle.filePath = bundle.root + "/" + files[0]; bundle.proxyRoot = bundle.root; - let bundleTypeName = config.source.bundleType; + const bundleTypeName = config.source.bundleType; //bundle.policies = []; //process.chdir(config.source.path); @@ -201,9 +213,12 @@ function processFileSystem(config, bundle, cb) { //only run for proxy or shared flow root if (!bundle.proxyRoot.endsWith(bundleTypeName)) { - let folders = findFolders(bundleTypeName); + const folders = findFolders(bundleTypeName); //potentially have (apiproxy and target/apiproxy) or shared flow - if (bundleTypeName == bundleType.BundleType.APIPROXY && folders.includes("target/apiproxy")) { + if ( + bundleTypeName == bundleType.BundleType.APIPROXY && + folders.includes("target/apiproxy") + ) { //we prefer the target bundle when we have both //refactor to contains then walk through options bundle.proxyRoot = "target/apiproxy"; @@ -214,7 +229,11 @@ function processFileSystem(config, bundle, cb) { } else { //warn and be done bundle.addMessage({ - message: "No " + bundleTypeName + " folder found in: " + JSON.stringify(folders) + message: + "No " + + bundleTypeName + + " folder found in: " + + JSON.stringify(folders) }); } } @@ -227,7 +246,7 @@ function processFileSystem(config, bundle, cb) { function Bundle(config, cb) { //if source is managementServer then download the bundle //rewrite the source to be filesystem - var bundle = this; + const bundle = this; this.xPathName = bundleType.getXPathName(config.source.bundleType); this.bundleTypeName = config.source.bundleType; @@ -236,11 +255,11 @@ function Bundle(config, cb) { if (config.source.type === "ManagementServer") { //shared flow not implemented for management server - if(config.source.bundleType === bundleType.BundleType.SHAREDFLOW){ + if (config.source.bundleType === bundleType.BundleType.SHAREDFLOW) { throw "SharedFlows for management server not supported"; } - var ManagementServer = require("./ManagementServer.js"), + const ManagementServer = require("./ManagementServer.js"), org = config.source.org, api = config.source.api, revision = config.source.revision, @@ -252,7 +271,7 @@ function Bundle(config, cb) { deleteFolderRecursive(tempFolder); fs.mkdirSync(tempFolder); - var fileStream = fs.createWriteStream(tempFolder + "/apiproxy.zip"); + const fileStream = fs.createWriteStream(tempFolder + "/apiproxy.zip"); ms.get( "Bundle", @@ -260,17 +279,16 @@ function Bundle(config, cb) { { api, revision, - onRes: function(res) { + onRes: function (res) { if (res.statusCode === 200) { - res.pipe(fileStream).on("close", function() { - decompress( - tempFolder + "/apiproxy.zip", - tempFolder - ).then(files => { - //add the info to the config object here - config.source.path = tempFolder + "/apiproxy"; - processFileSystem(config, bundle, cb); - }); + res.pipe(fileStream).on("close", function () { + decompress(tempFolder + "/apiproxy.zip", tempFolder).then( + (files) => { + //add the info to the config object here + config.source.path = tempFolder + "/apiproxy"; + processFileSystem(config, bundle, cb); + } + ); }); } else { if (cb) { @@ -282,7 +300,7 @@ function Bundle(config, cb) { } } }, - function(body) { + function (body) { //callback on bundle downloaded if (body.error) { console.log(body.error); @@ -296,12 +314,12 @@ function Bundle(config, cb) { } } -Bundle.prototype.getReport = function(cb) { +Bundle.prototype.getReport = function (cb) { //go out and getReport from endpoints, resources, policies - let result = [this.report]; - const appendCollection = - collection => collection.forEach( item => result.push(item.getReport()) ); + const result = [this.report]; + const appendCollection = (collection) => + collection.forEach((item) => result.push(item.getReport())); appendCollection(this.getEndpoints()); // for either apiproxy or sharedflow appendCollection(this.getPolicies()); @@ -312,36 +330,24 @@ Bundle.prototype.getReport = function(cb) { return result; }; -Bundle.prototype.addMessage = function(msg) { - if (msg.hasOwnProperty("plugin")) { +Bundle.prototype.addMessage = function (msg) { + if (msg.plugin) { msg.ruleId = msg.plugin.ruleId; if (!msg.severity) msg.severity = msg.plugin.severity; msg.nodeType = msg.plugin.nodeType; delete msg.plugin; } - if (!msg.hasOwnProperty("entity")) { + if (!msg.entity) { msg.entity = this; } - if ( - !msg.hasOwnProperty("source") && - msg.entity && - msg.entity.hasOwnProperty("getSource") - ) { + if (!msg.source && msg.entity && typeof msg.entity.getSource == "function") { msg.source = msg.entity.getSource(); } - if ( - !msg.hasOwnProperty("line") && - msg.entity && - msg.entity.hasOwnProperty("getElement") - ) { + if (!msg.line && msg.entity && typeof msg.entity.getElement == "function") { msg.line = msg.entity.getElement().lineNumber; } - if ( - !msg.hasOwnProperty("column") && - msg.entity && - msg.entity.hasOwnProperty("getElement") - ) { + if (!msg.column && msg.entity && typeof msg.entity.getElement == "function") { msg.column = msg.entity.getElement().columnNumber; } delete msg.entity; @@ -358,122 +364,137 @@ Bundle.prototype.addMessage = function(msg) { } }; -Bundle.prototype.getPolicyByName = function(pname) { - var result; - this.policies.some(function(policy) { - if (policy.getName() === pname) { - result = policy; - return true; - } - }); - return result; +Bundle.prototype.getPolicyByName = function (pname) { + return this.policies.find((policy) => policy.getName() === pname); }; -Bundle.prototype.onBundle = function(pluginFunction, cb) { +Bundle.prototype.onBundle = function (pluginFunction, cb) { pluginFunction(this, cb); }; -Bundle.prototype.onPolicies = function(pluginFunction, cb) { - this.getPolicies().forEach(policy => - pluginFunction(policy, getcb(`policy '${policy.getName()}'`))); +Bundle.prototype.onPolicies = function (pluginFunction, cb) { + this.getPolicies().forEach((policy) => + pluginFunction(policy, getcb(`policy '${policy.getName()}'`)) + ); cb(null, {}); }; -Bundle.prototype.onSteps = function(pluginFunction, callback) { +Bundle.prototype.onSteps = function (pluginFunction, callback) { // there is no need to run the checks in parallel const bundle = this, - proxies = bundle.getProxyEndpoints(), - targets = bundle.getTargetEndpoints(); + proxies = bundle.getProxyEndpoints(), + targets = bundle.getTargetEndpoints(); debug(`onSteps: bundle name: '${bundle.getName()}'`); try { - if (proxies && proxies.length>0) { - proxies.forEach(ep => ep.onSteps(pluginFunction, getcb(`proxyendpoint '${ep.getName()}'`))); + if (proxies && proxies.length > 0) { + proxies.forEach((ep) => + ep.onSteps( + pluginFunction, + getcb(`STEP proxyendpoint '${ep.getName()}'`) + ) + ); } else { debug("no proxyEndpoints"); } - if (targets && targets.length>0) { - targets.forEach(ep => ep.onSteps(pluginFunction, getcb(`targetendpoint '${ep.getName()}'`))); + if (targets && targets.length > 0) { + targets.forEach((ep) => + ep.onSteps( + pluginFunction, + getcb(`STEP targetendpoint '${ep.getName()}'`) + ) + ); } else { debug("no targetEndpoints"); } - } - catch (exc1) { - debug('exception: ' + exc1); + } catch (exc1) { + debug("exception: " + exc1); debug(exc1.stack); } callback(null, {}); }; -Bundle.prototype.onConditions = function(pluginFunction, callback) { +Bundle.prototype.onConditions = function (pluginFunction, callback) { const bundle = this, - proxies = bundle.getProxyEndpoints(), - targets = bundle.getTargetEndpoints(); + proxies = bundle.getProxyEndpoints(), + targets = bundle.getTargetEndpoints(); debug(`onConditions: bundle name: '${bundle.getName()}'`); try { - if (proxies && proxies.length>0) { - proxies.forEach(ep => - ep.onConditions(pluginFunction, getcb(`proxyendpoint '${ep.getName()}'`))); + if (proxies && proxies.length > 0) { + proxies.forEach((ep) => + ep.onConditions( + pluginFunction, + getcb(`COND proxyendpoint '${ep.getName()}'`) + ) + ); } if (targets && targets.length > 0) { - targets.forEach(ep => - ep.onConditions(pluginFunction, getcb(`targetendpoint '${ep.getName()}'`))); + targets.forEach((ep) => + ep.onConditions( + pluginFunction, + getcb(`COND targetendpoint '${ep.getName()}'`) + ) + ); } - } - catch (exc1) { - debug('exception: ' + exc1); + } catch (exc1) { + debug("exception: " + exc1); debug(exc1.stack); } callback(null, {}); }; -Bundle.prototype.onResources = function(pluginFunction, cb) { +Bundle.prototype.onResources = function (pluginFunction, cb) { const bundle = this; if (bundle.getResources()) { - bundle.getResources().forEach(re => - pluginFunction(re, getcb(`resource '${re.getFileName()}'`))); + bundle + .getResources() + .forEach((re) => + pluginFunction(re, getcb(`resource '${re.getFileName()}'`)) + ); } cb(null, {}); }; -Bundle.prototype.onFaultRules = function(pluginFunction, cb) { - const bundle = this; - if (bundle.getFaultRules()) { - bundle.getFaultRules().forEach(fr => - fr && pluginFunction(fr, getcb(`faultrule '${fr.getName()}'`))); +Bundle.prototype.onFaultRules = function (pluginFunction, cb) { + if (this.getFaultRules()) { + this.getFaultRules().forEach( + (fr) => fr && pluginFunction(fr, getcb(`faultrule '${fr.getName()}'`)) + ); } cb(null, {}); }; -Bundle.prototype.onDefaultFaultRules = function(pluginFunction, cb) { - const bundle = this; - if (bundle.getDefaultFaultRules()) { - bundle.getDefaultFaultRules().forEach(dfr => - dfr && pluginFunction(dfr, getcb('defaultfaultrule'))); +Bundle.prototype.onDefaultFaultRules = function (pluginFunction, cb) { + if (this.getDefaultFaultRules()) { + this.getDefaultFaultRules().forEach( + (dfr) => dfr && pluginFunction(dfr, getcb("defaultfaultrule")) + ); } cb(null, {}); }; -Bundle.prototype.onProxyEndpoints = function(pluginFunction, cb) { - let eps = this.getProxyEndpoints(); +Bundle.prototype.onProxyEndpoints = function (pluginFunction, cb) { + const eps = this.getProxyEndpoints(); if (eps && eps.length > 0) { - eps.forEach( ep => - pluginFunction(ep, getcb(`proxyendpoint '${ep.getName()}'`))); + eps.forEach((ep) => + pluginFunction(ep, getcb(`PEP proxyendpoint '${ep.getName()}'`)) + ); } cb(null, {}); }; -Bundle.prototype.onTargetEndpoints = function(pluginFunction, cb) { - let eps = this.getTargetEndpoints(); +Bundle.prototype.onTargetEndpoints = function (pluginFunction, cb) { + const eps = this.getTargetEndpoints(); if (eps && eps.length > 0) { - eps.forEach( ep => - pluginFunction(ep, getcb(`targetendpoint '${ep.getName()}'`))); + eps.forEach((ep) => + pluginFunction(ep, getcb(`TEP targetendpoint '${ep.getName()}'`)) + ); } cb(null, {}); }; -Bundle.prototype.getProxyEndpoints = function() { +Bundle.prototype.getProxyEndpoints = function () { if (!this.proxyEndpoints) { - if(this.bundleTypeName === bundleType.BundleType.SHAREDFLOW){ + if (this.bundleTypeName === bundleType.BundleType.SHAREDFLOW) { buildsharedflows(this); } else { buildProxyEndpoints(this); @@ -486,7 +507,7 @@ Bundle.prototype.getProxyEndpoints = function() { return this.proxyEndpoints; }; -Bundle.prototype.getTargetEndpoints = function() { +Bundle.prototype.getTargetEndpoints = function () { if (!this.targetEndpoints) { buildTargetEndpoints(this); if (!this.targetEndpoints) { @@ -496,99 +517,82 @@ Bundle.prototype.getTargetEndpoints = function() { return this.targetEndpoints; }; -Bundle.prototype.getEndpoints = function() { +Bundle.prototype.getEndpoints = function () { if (!this.endpoints) { this.endpoints = this.getProxyEndpoints(); //no targets for shared flows - if(this.bundleTypeName !== bundleType.BundleType.SHAREDFLOW){ + if (this.bundleTypeName !== bundleType.BundleType.SHAREDFLOW) { this.endpoints = this.endpoints.concat(this.getTargetEndpoints()); } } return this.endpoints; }; -Bundle.prototype.getResources = function() { +Bundle.prototype.getResources = function () { if (!this.resources) { buildResources(this); } return this.resources; }; -Bundle.prototype.getPolicies = function() { +Bundle.prototype.getPolicies = function () { if (!this.policies) { buildPolicies(this); } return this.policies; }; -Bundle.prototype.getFaultRules = function() { +Bundle.prototype.getFaultRules = function () { if (!this.faultRules) { - //get them from the endpoints - var faultRules = []; - this.getEndpoints().forEach(function(ep) { - faultRules = faultRules.concat(faultRules, ep.getFaultRules()); - }); - this.faultRules = faultRules; + this.faultRules = this.getEndpoints().reduce( + (a, ep) => [...a, ep.getFaultRules()], + [] + ); } return this.faultRules; }; -Bundle.prototype.getDefaultFaultRules = function() { +Bundle.prototype.getDefaultFaultRules = function () { if (!this.defaultFaultRules) { - var defaultFaultRules = []; - //get the defaultfaultrules from the endpoints - this.getEndpoints().forEach(function(ep) { - defaultFaultRules.push(ep.getDefaultFaultRule()); - }); - this.defaultFaultRules = defaultFaultRules; + this.defaultFaultRules = this.getEndpoints().reduce( + (a, ep) => [...a, ep.getDefaultFaultRule()], + [] + ); } return this.defaultFaultRules; }; -Bundle.prototype.summarize = function() { - let summary = { +Bundle.prototype.summarize = function () { + const summary = { report: this.getReport(), root: this.root, name: this.getName(), revision: this.getRevision(), - policies: this.getPolicies() - }; - - summary.proxyEndpoints = []; - if (this.getProxyEndpoints()) { - this.getProxyEndpoints().forEach(function(ep) { - summary.proxyEndpoints.push(ep.summarize()); - }); - } - summary.targetEndpoints = []; - if (this.getTargetEndpoints()) { - this.getTargetEndpoints().forEach(function(ep) { - summary.targetEndpoints.push(ep.summarize()); - }); - } + policies: this.getPolicies(), + proxyEndpoints: this.getProxyEndpoints() + ? this.getProxyEndpoints().map((ep) => ep.summarize()) + : [], - summary.resources = []; - if (this.getResources()) { - this.getResources().forEach(function(re) { - summary.resources.push(re.summarize()); - }); - } + targetEndpoints: this.getTargetEndpoints() + ? this.getTargetEndpoints().map((ep) => ep.summarize()) + : [], - summary.policies = []; - if (this.getPolicies()) { - this.getPolicies().forEach(function(po) { - summary.policies.push(po.summarize()); - }); - } + resources: this.getResources() + ? this.getResources().map((re) => re.summarize()) + : [], + policies: this.getPolicies() + ? this.getPolicies().map((po) => po.summarize()) + : [] + }; return summary; }; function deleteFolderRecursive(path) { if (fs.existsSync(path)) { - fs.readdirSync(path).forEach(function(file) { - var curPath = path + "/" + file; + fs.readdirSync(path).forEach(function (file) { + const curPath = path + "/" + file; if (fs.lstatSync(curPath).isDirectory()) { // recurse deleteFolderRecursive(curPath); diff --git a/lib/package/plugins/BN009-checkForMultipleStatsCollectorPolicies.js b/lib/package/plugins/BN009-checkForMultipleStatsCollectorPolicies.js index 7ccc1f30..29f2fb92 100644 --- a/lib/package/plugins/BN009-checkForMultipleStatsCollectorPolicies.js +++ b/lib/package/plugins/BN009-checkForMultipleStatsCollectorPolicies.js @@ -1,5 +1,5 @@ /* - Copyright 2019-2020 Google LLC + Copyright 2019-2020,2023 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,32 +15,40 @@ */ const ruleId = require("../myUtil.js").getRuleId(), - debug = require("debug")("apigeelint:" + ruleId); + debug = require("debug")("apigeelint:" + ruleId); const plugin = { - ruleId, - name: "Check for multiple statistics collector policies", - message: - "Only one Statistics Collector Policy will be executed. Therefore, if you include mulitiple Statistics Collector Policies then you must have conditions on each one.", - fatal: false, - severity: 1, //warn - nodeType: "Bundle", - enabled: true - }; + ruleId, + name: "Check for multiple statistics collector policies", + message: + "Only one Statistics Collector Policy will be executed. Therefore, if you include mulitiple Statistics Collector Policies then you should have conditions on all but one.", + fatal: false, + severity: 1, //warn + nodeType: "Bundle", + enabled: true +}; -let statsPolicies = [], hadWarnErr=false; +let hadWarnErr = false; -var onBundle = function(bundle, cb) { - statsPolicies = []; +const onBundle = function (bundle, cb) { + /* + * This is implemented with onBundle, so that the plugin can + * check for duplicates. It does check only single policies in isolation, + * but checks each Stats policy against others in the bundle. + **/ + let statsPolicies = []; if (bundle.policies) { debug("number of policies: " + bundle.policies.length); - bundle.policies.forEach(function(policy) { - if (isStatsCollectorPolicy(policy)) { - debug("statistics collector policy added: " + policy.getName()); - statsPolicies.push(policy); - } - }); + statsPolicies = bundle.policies.filter((policy) => + isStatsCollectorPolicy(policy) + ); debug("number of statistics collector policies: " + statsPolicies.length); + if (statsPolicies.length > 0) { + debug( + "statistics collector policies: " + + statsPolicies.map((p) => p.getName()) + ); + } if (statsPolicies.length > 1) { checkForDuplicatePolicies(statsPolicies); //checkForMoreThanOneStatsPolicyOnFlow(statsPolicies, bundle); @@ -48,34 +56,29 @@ var onBundle = function(bundle, cb) { } /* - This was a prior check to confirm that other Bundle features are working - such as the number of steps being pulled and the number of flows being - pulled correctly - */ - statsPolicies.forEach(function(policy) { + * This was a prior check to confirm that other Bundle features are working + * such as the number of steps being pulled and the number of flows being + * pulled correctly + **/ + statsPolicies.forEach(function (policy) { attachedToMoreThanOneStepAndNoConditions(policy); }); } - if (typeof(cb) == 'function') { - cb(null,hadWarnErr); + if (typeof cb == "function") { + cb(null, hadWarnErr); } }; /* -Determine if this is a statistics collector policy. -@returns true if it is or false otherwise -*/ + * Determine if this is a statistics collector policy. + * @returns true if it is or false otherwise + **/ function isStatsCollectorPolicy(policy) { - var policyType = policy.getType(); - if (policyType === "StatisticsCollector") { - return true; - } else { - return false; - } + return policy.getType() === "StatisticsCollector"; } function markPolicy(policy, msg) { - var result = { + const result = { ruleId: plugin.ruleId, severity: plugin.severity, source: policy.getSource(), @@ -85,116 +88,74 @@ function markPolicy(policy, msg) { message: msg }; policy.addMessage(result); - hadWarnErr=true; + hadWarnErr = true; } -function markBundle(bundle, msg) { - var result = { - ruleId: plugin.ruleId, - severity: plugin.severity, - nodeType: plugin.nodeType, - message: msg - }; - bundle.addMessage(result); - hadWarnErr=true; -} - -/* -*/ function checkForDuplicatePolicies(policies) { - var duplicates = getDuplicatePolicies(policies); - debug("there are " + duplicates.length + " duplicate policies."); + const duplicates = getDuplicatePolicies(policies); + debug("there are " + duplicates.length + " duplications."); if (duplicates.length > 0) { - var duplicatePolicyNames = concatenateDuplicatePolicyNames(policies); - debug("duplicate policies: " + duplicatePolicyNames); - policies.forEach(function(policy) { - debug("duplicate policy warning for " + policy.getName()); + duplicates.forEach((item) => { + const duplicatePolicyNames = item.dupes.map((p) => p.getName()).join(" "); + debug( + `original policy: ${item.policy.getName()} duplicates: ${duplicatePolicyNames}` + ); markPolicy( - policy, - "The following StatisticsCollectors are configured: " + + item.policy, + "The following StatisticsCollector policies are duplicates: " + duplicatePolicyNames ); }); } } -function concatenateDuplicatePolicyNames(policies) { - var names = ""; - policies.forEach(function(policy) { - names += policy.getName() + " "; - }); - return names.trim(); -} - function getDuplicatePolicies(policies) { - var duplicatePolicies = []; - for (var i = 0; i < policies.length - 1; i++) { - for (var j = i + 1; j < policies.length; j++) { - var p1 = policies[i].select("//Statistics").toString().trim(); - var p2 = policies[j].select("//Statistics").toString().trim(); - debug("comparing -> \n1st policy:\n" + p1 + "\n2nd policy:\n" + p2); - if (p1 === p2) { - if (duplicatePolicies.indexOf(policies[i]) === -1) { - duplicatePolicies.push(policies[i]); - debug(policies[i].getName() + " is a duplicate!"); - } - if (duplicatePolicies.indexOf(policies[j]) === -1) { - duplicatePolicies.push(policies[j]); - debug(policies[j].getName() + " is a duplicate!"); - } - } - } - } - return duplicatePolicies; + // A naive dupe finder - it looks for string equality in the XML. + // A better approach would be to check each attribute value. + const duplicateFinder = function (acc, c, ix, a) { + const p1 = c.select("//Statistics").toString().trim(); + const dupes = a + .slice(ix + 1) + .filter((p) => p.select("//Statistics").toString().trim() == p1); + return dupes.length ? [...acc, { policy: c, dupes }] : acc; + }; + + // return an array of hashes. + // each item is a hash {policy, dupes} + return policies.reduce(duplicateFinder, []); } /* -Check the policies for missng conditions, but make sure -that the policies are attached to the flow. -*/ -function checkPoliciesForMissingConditions(policies, bundle) { - var attachedPolicies = getAttachedPolicies(policies); + * Check the policies for missing conditions, but make sure + * that the policies are attached to the flow. + ***/ +function checkPoliciesForMissingConditions(policies, _bundle) { + const attachedPolicies = getAttachedPolicies(policies); if (attachedPolicies.length > 1) { - var doAllStepsHaveCondition = true; - attachedPolicies.forEach(function(policy) { + attachedPolicies.forEach(function (policy) { if (!allStepsHaveCondition(policy)) { - doAllStepsHaveCondition = false; markPolicy( policy, policy.getName() + - " is attached to a step without a condition. If you have more than two Statistics Collector policies, only the last one in the flow will execute. Include a condition to make sure the correct one executes." + " is attached to a step without a Condition. If you have more than two StatisticsCollector policies, only the last one in the flow will execute. Include a Condition to make sure the correct one executes." ); } }); - debug("doAllStepsHaveCondition: " + doAllStepsHaveCondition); - if (!doAllStepsHaveCondition) { - markBundle( - bundle, - "There are several Statistics Collector policies attached to a step without a condition. If you have more than two Statistics Collector policies, only the last one in the flow will execute. Include a condition to make sure the correct one executes." - ); - } } } /* -If there are two or more stats collector policies, then -make sure that they are all attached. -*/ + * If there are two or more stats collector policies, then + * make sure that they are all attached. + **/ function getAttachedPolicies(policies) { - var attachedPolicies = []; - policies.forEach(function(policy) { - if (isAttached(policy)) { - debug(policy.getName() + " is attached to the flow."); - attachedPolicies.push(policy); - } - }); - return attachedPolicies; + return policies.filter((policy) => isAttached(policy)); } /* -Check if there are any stats policies attached to multiple steps without conditions. -@returns true if a policy without a condition exists otherwise false. -*/ + * Check if there are any stats policies attached to multiple steps without conditions. + * @returns true if a policy without a condition exists otherwise false. + **/ function attachedToMoreThanOneStepAndNoConditions(policy) { if (isAttachedToMoreThanOneStep(policy) && !allStepsHaveCondition(policy)) { markPolicy( @@ -206,53 +167,37 @@ function attachedToMoreThanOneStepAndNoConditions(policy) { } /* -Is this policy attached to a step -*/ + * Is this policy attached to a step + **/ function isAttached(policy) { - var steps = policy.getSteps(); - if (steps) { - return true; - } else { - return false; - } + return !!policy.getSteps(); } /* -Is the policy attached to more than one step -@param policy -@returns true if the policy is attached to more than one step, false otherwise -*/ + * Is the policy attached to more than one step + * @param policy + * @returns true if the policy is attached to more than one step, false otherwise + **/ function isAttachedToMoreThanOneStep(policy) { - var steps = policy.getSteps(); + const steps = policy.getSteps(); if (steps) { - if (steps.length <= 1) { - debug("policy " + policy.getName() + " is attached to zero or one step."); - return false; - } - if (steps.length > 1) { - debug( - "policy " + - policy.getName() + - " is attached to " + - steps.length + - " steps." - ); - return true; - } + debug(`policy ${policy.getName()} is attached to ${steps.length} steps.`); + return steps.length > 1; } + return false; } /* -Check if all the steps have a condition. -@param policy -@return true if all steps have a condition, otherwise false -If there are no steps for the policy then return false -*/ + * Check if all the steps have a condition. + * @param policy + * @return true if all steps have a condition, otherwise false + * If there are no steps for the policy then return false + **/ function allStepsHaveCondition(policy) { - var steps = policy.getSteps(); - var result = true; + const steps = policy.getSteps(); + let result = true; if (steps) { - steps.forEach(function(step) { + steps.forEach(function (step) { if (!step.getCondition()) { debug(policy.getName() + " is attached to a step without a condition."); result = false; @@ -264,9 +209,7 @@ function allStepsHaveCondition(policy) { } if (result) { debug( - "all the steps to which " + - policy.getName() + - " is attached have a condition." + `all the steps to which ${policy.getName()} is attached have a condition.` ); } return result; diff --git a/lib/package/plugins/EP002-misplacedElements.js b/lib/package/plugins/EP002-misplacedElements.js index 647d2407..87c4396b 100644 --- a/lib/package/plugins/EP002-misplacedElements.js +++ b/lib/package/plugins/EP002-misplacedElements.js @@ -1,5 +1,5 @@ /* - Copyright 2019-2022 Google LLC + Copyright 2019-2023 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,103 +15,157 @@ */ const ruleId = require("../myUtil.js").getRuleId(), - debug = require("debug")("apigeelint:" + ruleId), - xpath = require("xpath"); + debug = require("debug")("apigeelint:" + ruleId), + xpath = require("xpath"); const plugin = { - ruleId, - name: "Check for commonly misplaced elements", - message: - "For example, a Flow element should be a child of Flows parent.", - fatal: false, - severity: 2, // error - nodeType: "Endpoint", - enabled: true - }; + ruleId, + name: "Check for commonly misplaced elements", + message: "For example, a Flow element should be a child of Flows parent.", + fatal: false, + severity: 2, // error + nodeType: "Endpoint", + enabled: true, +}; let bundleProfile = "apigee"; -const onBundle = function(bundle, cb) { - if (bundle.profile) { bundleProfile = bundle.profile; } +const onBundle = function (bundle, cb) { + if (bundle.profile) { + bundleProfile = bundle.profile; + } if (typeof cb == "function") { cb(null, false); } }; -const onProxyEndpoint = function(endpoint, cb) { - debug('onProxyEndpoint'); - let checker = new EndpointChecker(endpoint); - let flagged = checker.check(); - if (typeof(cb) == 'function') { - cb(null, flagged); - } - }; - -const onTargetEndpoint = function(endpoint, cb) { - debug('onTargetEndpoint'); - let checker = new EndpointChecker(endpoint); - let flagged = checker.check(); - if (typeof(cb) == 'function') { - cb(null, flagged); - } - }; +const onProxyEndpoint = function (endpoint, cb) { + debug("onProxyEndpoint"); + const checker = new EndpointChecker(endpoint); + const flagged = checker.check(); + if (typeof cb == "function") { + cb(null, flagged); + } + return flagged; +}; + +const onTargetEndpoint = function (endpoint, cb) { + debug("onTargetEndpoint"); + const checker = new EndpointChecker(endpoint); + const flagged = checker.check(); + if (typeof cb == "function") { + cb(null, flagged); + } + return flagged; +}; const _markEndpoint = (endpoint, message, line, column) => { - var result = { - ruleId: plugin.ruleId, - severity: plugin.severity, - nodeType: plugin.nodeType, - message, - line, - column, - }; - // discard duplicates - if ( !line || !column || !endpoint.report.messages.find(m => m.line == line && m.column == column)) { - endpoint.addMessage(result); - } - }; + var result = { + ruleId: plugin.ruleId, + severity: plugin.severity, + nodeType: plugin.nodeType, + message, + line, + column, + }; + // discard duplicates + if ( + !line || + !column || + !endpoint.report.messages.find((m) => m.line == line && m.column == column) + ) { + endpoint.addMessage(result); + } +}; const allowedParents = { - Step: ['Request', 'Response', 'FaultRule', 'DefaultFaultRule', 'SharedFlow'], - Request: ['PreFlow', 'PostFlow', 'Flow', 'PostClientFlow', 'HTTPMonitor'], - Response: ['PreFlow', 'PostFlow', 'Flow', 'PostClientFlow'], - Flows: ['ProxyEndpoint', 'TargetEndpoint'], - Flow: ['Flows'], - RouteRule: ['ProxyEndpoint'], - DefaultFaultRule: ['ProxyEndpoint', 'TargetEndpoint'], - HTTPTargetConnection: ['TargetEndpoint'], - LoadBalancer: ['HTTPTargetConnection'], - HealthMonitor: ['HTTPTargetConnection'], - HTTPMonitor: ['HealthMonitor'], - TCPMonitor: ['HealthMonitor'], - SuccessResponse: ['HTTPMonitor'] - }; + Step: ["Request", "Response", "FaultRule", "DefaultFaultRule", "SharedFlow"], + Request: ["PreFlow", "PostFlow", "Flow", "PostClientFlow", "HTTPMonitor"], + Response: ["PreFlow", "PostFlow", "Flow", "PostClientFlow"], + Flows: ["ProxyEndpoint", "TargetEndpoint"], + Flow: ["Flows"], + RouteRule: ["ProxyEndpoint"], + DefaultFaultRule: ["ProxyEndpoint", "TargetEndpoint"], + HTTPTargetConnection: ["TargetEndpoint"], + LoadBalancer: ["HTTPTargetConnection"], + HealthMonitor: ["HTTPTargetConnection"], + HTTPMonitor: ["HealthMonitor"], + TCPMonitor: ["HealthMonitor"], + SuccessResponse: ["HTTPMonitor"], +}; const allowedChildren = { - Step: ['Name', 'Condition', 'FaultRules'], - Request: ['Step'], - 'Request-child-of-HTTPMonitor': ['ConnectTimeoutInSec', 'SocketReadTimeoutInSec', 'Payload', 'IsSSL', 'TrustAllSSL', 'Port', 'Verb', 'Path', 'Header', 'IncludeHealthCheckIdHeader'], - Response: ['Step'], - Flows: ['Flow'], - Flow: ['Description', 'Request', 'Response', 'Condition'], - RouteRule: ['Condition', 'TargetEndpoint', 'URL', 'IntegrationEndpoint'], - DefaultFaultRule: ['Step', 'AlwaysEnforce', 'Condition'], - HTTPTargetConnection: ['LoadBalancer', 'Properties', 'Path', 'HealthMonitor', 'URL', 'SSLInfo', 'Authentication'], - LoadBalancer: ['Algorithm', 'Server', 'MaxFailures', 'ServerUnhealthyResponse', 'RetryEnabled', 'TargetDisableSecs'], - HealthMonitor: ['IsEnabled', 'IntervalInSec', 'HTTPMonitor', 'TCPMonitor'], - HTTPMonitor: ['Request', 'SuccessResponse'], - TCPMonitor: ['ConnectTimeoutInSec', 'Port'], - SuccessResponse: ['ResponseCode', 'Header'], - ProxyEndpoint: ['PreFlow', 'PostFlow', 'Flows', 'RouteRule', 'PostClientFlow', - 'Description', 'FaultRules', 'DefaultFaultRule', 'HTTPProxyConnection'], - TargetEndpoint: ['PreFlow', 'PostFlow', 'Flows', - 'Description', 'FaultRules', 'DefaultFaultRule', 'HostedTarget', - 'LocalTargetConnection', 'HTTPTargetConnection'], - SharedFlow: ['Step' ] - }; + Step: ["Name", "Condition", "FaultRules"], + Request: ["Step"], + "Request-child-of-HTTPMonitor": [ + "ConnectTimeoutInSec", + "SocketReadTimeoutInSec", + "Payload", + "IsSSL", + "TrustAllSSL", + "Port", + "Verb", + "Path", + "Header", + "IncludeHealthCheckIdHeader", + ], + Response: ["Step"], + Flows: ["Flow"], + Flow: ["Description", "Request", "Response", "Condition"], + RouteRule: ["Condition", "TargetEndpoint", "URL", "IntegrationEndpoint"], + DefaultFaultRule: ["Step", "AlwaysEnforce", "Condition"], + HTTPTargetConnection: [ + "LoadBalancer", + "Properties", + "Path", + "HealthMonitor", + "URL", + "SSLInfo", + "Authentication", + ], + LoadBalancer: [ + "Algorithm", + "Server", + "MaxFailures", + "ServerUnhealthyResponse", + "RetryEnabled", + "TargetDisableSecs", + ], + HealthMonitor: ["IsEnabled", "IntervalInSec", "HTTPMonitor", "TCPMonitor"], + HTTPMonitor: ["Request", "SuccessResponse"], + TCPMonitor: ["ConnectTimeoutInSec", "Port"], + SuccessResponse: ["ResponseCode", "Header"], + ProxyEndpoint: [ + "PreFlow", + "PostFlow", + "Flows", + "RouteRule", + "PostClientFlow", + "Description", + "FaultRules", + "DefaultFaultRule", + "HTTPProxyConnection", + ], + TargetEndpoint: [ + "PreFlow", + "PostFlow", + "Flows", + "Description", + "FaultRules", + "DefaultFaultRule", + "HostedTarget", + "LocalTargetConnection", + "HTTPTargetConnection", + ], + SharedFlow: ["Step"], +}; class EndpointChecker { constructor(endpoint) { - debug('EndpointChecker ctor (%s) (%s)', endpoint.getName(), endpoint.getType()); + debug( + "EndpointChecker ctor (%s) (%s)", + endpoint.getName(), + endpoint.getType(), + ); this.endpoint = endpoint; this.endpointType = endpoint.getType(); // ProxyEndpoint, TargetEndpoint, SharedFlow this.flagged = false; @@ -121,115 +175,183 @@ class EndpointChecker { try { const self = this; - let ep = self.endpointType; - let topLevelChildren = xpath.select("*", self.endpoint.element); - topLevelChildren.forEach(child => { - if (allowedChildren[ep].indexOf(child.tagName)< 0) { + const ep = self.endpointType, + topLevelChildren = xpath.select("*", self.endpoint.element); + topLevelChildren.forEach((child) => { + if (allowedChildren[ep].indexOf(child.tagName) < 0) { self.flagged = true; - _markEndpoint(self.endpoint, `Invalid ${child.tagName} element`, child.lineNumber, child.columnNumber); + _markEndpoint( + self.endpoint, + `Invalid ${child.tagName} element`, + child.lineNumber, + child.columnNumber, + ); } }); // 1st level children that must have at most one instance: Flows, DFR - ['Flows', 'DefaultFaultRule', 'FaultRules'].forEach( elementName => { - let elements = xpath.select(`${elementName}`, self.endpoint.element); + ["Flows", "DefaultFaultRule", "FaultRules"].forEach((elementName) => { + const elements = xpath.select(elementName, self.endpoint.element); if (elements.length != 0 && elements.length != 1) { self.flagged = true; - elements.slice(1) - .forEach(element => - _markEndpoint(self.endpoint, `Extra ${elementName} element`, - element.lineNumber, element.columnNumber)); + elements + .slice(1) + .forEach((element) => + _markEndpoint( + self.endpoint, + `Extra ${elementName} element`, + element.lineNumber, + element.columnNumber, + ), + ); } }); // Request, Response, Step, Flow, DefaultFaultRule, RouteRule - let condition = Object.keys(allowedParents).map(n => `self::${n}`).join(' or '); - let elements = xpath.select(`//*[${condition}]`, self.endpoint.element); - elements.forEach(element => { + const condition = Object.keys(allowedParents) + .map((n) => `self::${n}`) + .join(" or "); + const elements = xpath.select(`//*[${condition}]`, self.endpoint.element); + elements.forEach((element) => { let tagName = element.tagName; // misplaced children of toplevel elements are covered above - if (element.parentNode.tagName != 'ProxyEndpoint' && - element.parentNode.tagName != 'TargetEndpoint') { - if (allowedParents[tagName].indexOf(element.parentNode.tagName)<0) { + if ( + element.parentNode.tagName != "ProxyEndpoint" && + element.parentNode.tagName != "TargetEndpoint" + ) { + if (allowedParents[tagName].indexOf(element.parentNode.tagName) < 0) { self.flagged = true; - _markEndpoint(self.endpoint, `Misplaced ${tagName} element child of ${element.parentNode.tagName}`, element.lineNumber, element.columnNumber); + _markEndpoint( + self.endpoint, + `Misplaced ${tagName} element child of ${element.parentNode.tagName}`, + element.lineNumber, + element.columnNumber, + ); } } - let children = xpath.select("*", element); - children.forEach(child => { + const children = xpath.select("*", element); + children.forEach((child) => { // special case Request, there are two of them - let t = (tagName == 'Request' && element.parentNode.tagName == 'HTTPMonitor') ? - 'Request-child-of-HTTPMonitor' : tagName; + let t = + tagName == "Request" && element.parentNode.tagName == "HTTPMonitor" + ? "Request-child-of-HTTPMonitor" + : tagName; - if (allowedChildren[t].indexOf(child.tagName)<0) { + if (allowedChildren[t].indexOf(child.tagName) < 0) { self.flagged = true; - _markEndpoint(self.endpoint, `Misplaced '${child.tagName}' element child of ${tagName}`, - child.lineNumber, child.columnNumber); + _markEndpoint( + self.endpoint, + `Misplaced '${child.tagName}' element child of ${tagName}`, + child.lineNumber, + child.columnNumber, + ); } }); }); - // exactly one of LocalTarget and HTTPTarget is required - if (ep == 'TargetEndpoint') { + // in apigeex, exactly one of LocalTarget and HTTPTarget is required + if (ep == "TargetEndpoint") { let condition = []; - if(bundleProfile != "apigee") - condition = ['LocalTargetConnection', 'HTTPTargetConnection'].map(n => `self::${n}`).join(' or '); + if (bundleProfile != "apigee") + condition = ["LocalTargetConnection", "HTTPTargetConnection"] + .map((n) => `self::${n}`) + .join(" or "); else - condition = ['LocalTargetConnection', 'HTTPTargetConnection', 'HostedTarget'].map(n => `self::${n}`).join(' or '); //Apigee Edge can have without HTTPTargetConnection or LocalTargetConnection - let targetChildren = xpath.select(`*[${condition}]`, self.endpoint.element); + condition = [ + "LocalTargetConnection", + "HTTPTargetConnection", + "HostedTarget", + ] + .map((n) => `self::${n}`) + .join(" or "); //Apigee Edge can have without HTTPTargetConnection or LocalTargetConnection + let targetChildren = xpath.select( + `*[${condition}]`, + self.endpoint.element, + ); if (targetChildren.length == 0) { self.flagged = true; - if(bundleProfile != "apigee") - _markEndpoint(self.endpoint, `Missing a required *TargetConnection`, 1, 2); + if (bundleProfile != "apigee") + _markEndpoint( + self.endpoint, + `Missing a required *TargetConnection`, + 1, + 2, + ); else - _markEndpoint(self.endpoint, `Missing a required *TargetConnection or HostedTarget`, 1, 2); - } - else if (targetChildren.length != 1) { + _markEndpoint( + self.endpoint, + `Missing a required *TargetConnection or HostedTarget`, + 1, + 2, + ); + } else if (targetChildren.length != 1) { self.flagged = true; - targetChildren.slice(1) - .forEach(configElement => - _markEndpoint(self.endpoint, `${configElement.tagName} element conflicts with ${targetChildren[0].tagName} on line ${targetChildren[0].lineNumber}`, - configElement.lineNumber, configElement.columnNumber)); + targetChildren + .slice(1) + .forEach((configElement) => + _markEndpoint( + self.endpoint, + `${configElement.tagName} element conflicts with ${targetChildren[0].tagName} on line ${targetChildren[0].lineNumber}`, + configElement.lineNumber, + configElement.columnNumber, + ), + ); } } // in HealthMonitor, exactly one of HTTPMonitor or TCPMonitor is required - if (ep == 'TargetEndpoint') { - let healthMonitors = xpath.select(`HTTPTargetConnection/HealthMonitor`, self.endpoint.element); + if (ep == "TargetEndpoint") { + const healthMonitors = xpath.select( + `HTTPTargetConnection/HealthMonitor`, + self.endpoint.element, + ); if (healthMonitors.length > 1) { self.flagged = true; - healthMonitors.slice(1) - .forEach(elt => - _markEndpoint(self.endpoint, `Redundant HealthMonitor element`, elt.lineNumber, elt.columnNumber)); + healthMonitors + .slice(1) + .forEach((elt) => + _markEndpoint( + self.endpoint, + `Redundant HealthMonitor element`, + elt.lineNumber, + elt.columnNumber, + ), + ); } if (healthMonitors.length > 0) { - let condition = ['HTTP', 'TCP'].map(n => `self::${n}Monitor`).join(' or '); - let monitors = xpath.select(`*[${condition}]`, healthMonitors[0]); + const condition = ["HTTP", "TCP"] + .map((n) => `self::${n}Monitor`) + .join(" or "); + const monitors = xpath.select(`*[${condition}]`, healthMonitors[0]); if (monitors.length != 1) { self.flagged = true; - monitors.slice(1) - .forEach(configElement => - _markEndpoint(self.endpoint, `${configElement.tagName} element conflicts with ${monitors[0].tagName} on line ${monitors[0].lineNumber}`, - configElement.lineNumber, configElement.columnNumber)); + monitors + .slice(1) + .forEach((configElement) => + _markEndpoint( + self.endpoint, + `${configElement.tagName} element conflicts with ${monitors[0].tagName} on line ${monitors[0].lineNumber}`, + configElement.lineNumber, + configElement.columnNumber, + ), + ); } } } // future: add other checks here. return this.flagged; - } - catch(e) { + } catch (e) { console.log(e); return false; } } } - module.exports = { plugin, onBundle, onProxyEndpoint, - onTargetEndpoint + onTargetEndpoint, }; diff --git a/lib/package/plugins/FL001-unconditionalFlows.js b/lib/package/plugins/FL001-unconditionalFlows.js index 7becc093..ed86c3ba 100644 --- a/lib/package/plugins/FL001-unconditionalFlows.js +++ b/lib/package/plugins/FL001-unconditionalFlows.js @@ -1,5 +1,5 @@ /* - Copyright 2019-2021 Google LLC + Copyright 2019-2021,2023 Google LLC Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -17,78 +17,78 @@ const ruleId = require("../myUtil.js").getRuleId(); const plugin = { - ruleId, - name: "Unconditional Flows", - message: "Only one unconditional flow will be executed. Error if more than one detected.", - fatal: true, - severity: 2, //error - nodeType: "Endpoint", - enabled: true - }; + ruleId, + name: "Unconditional Flows", + message: + "Only one unconditional flow will be executed. Error if more than one detected.", + fatal: true, + severity: 2, //error + nodeType: "Endpoint", + enabled: true, +}; const debug = require("debug")(`apigeelint:${ruleId}`); -const searchUnconditionalFlowsInEndpoint = - function(endpoint) { - let flagged = false; +const searchUnconditionalFlowsInEndpoint = function (endpoint) { + let flagged = false; - let flows = endpoint.getFlows(); - debug(`found ${flows.length} Flows`); - let unconditionalFlows = - flows.filter( fl => (!fl.getCondition() || - fl.getCondition().getExpression() === "") ); - if (unconditionalFlows.length > 0) { - debug(`found ${unconditionalFlows.length} Flows with no Condition`); - let lastUncFlow = unconditionalFlows[unconditionalFlows.length - 1]; - // check if multiple - if (unconditionalFlows.length > 1) { - flagged = true; - endpoint.addMessage({ - source: lastUncFlow.getSource(), - line: lastUncFlow.getElement().lineNumber, - column: lastUncFlow.getElement().columnNumber, - plugin, - message: - `Endpoint has too many unconditional Flow elements (${unconditionalFlows.length}). Only one will be executed.` - }); - } + const flows = endpoint.getFlows(); + debug(`found ${flows.length} Flows`); + const unconditionalFlows = flows.filter( + (fl) => !fl.getCondition() || fl.getCondition().getExpression() === "", + ); + if (unconditionalFlows.length > 0) { + debug(`found ${unconditionalFlows.length} Flows with no Condition`); + const lastUncFlow = unconditionalFlows[unconditionalFlows.length - 1]; + // check if multiple + if (unconditionalFlows.length > 1) { + flagged = true; + endpoint.addMessage({ + source: lastUncFlow.getSource(), + line: lastUncFlow.getElement().lineNumber, + column: lastUncFlow.getElement().columnNumber, + plugin, + message: `Endpoint has too many unconditional Flow elements (${unconditionalFlows.length}). Only one will be executed.`, + }); + } - // check if unconditional is not final flow - let lastFlow = flows[flows.length - 1]; - if (lastFlow != lastUncFlow) { - flagged = true; - endpoint.addMessage({ - source: lastUncFlow.getSource(), - line: lastUncFlow.getElement().lineNumber, - column: lastUncFlow.getElement().columnNumber, - plugin, - message: - `Endpoint has an unconditional Flow that is not the final flow. It will be ignored.` - }); - } + // check if unconditional is not final flow + const lastFlow = flows[flows.length - 1]; + if (lastFlow != lastUncFlow) { + flagged = true; + endpoint.addMessage({ + source: lastUncFlow.getSource(), + line: lastUncFlow.getElement().lineNumber, + column: lastUncFlow.getElement().columnNumber, + plugin, + message: `Endpoint has an unconditional Flow that is not the final flow. It will be ignored.`, + }); } + } - return flagged; - }; + return flagged; +}; -const onProxyEndpoint = function(endpoint,cb) { - // The search function has side effects. Do not refactor - // to put it inside the conditional. - let f1 = searchUnconditionalFlowsInEndpoint(endpoint); - if (typeof cb == "function") { - cb(f1); - } - }; +const onProxyEndpoint = function (endpoint, cb) { + // The search function has side effects. Do not refactor + // to put it inside the conditional. + const flagged = searchUnconditionalFlowsInEndpoint(endpoint); + if (typeof cb == "function") { + cb(null, flagged); + } + return flagged; +}; -const onTargetEndpoint = function(endpoint,cb) { - let flagged = searchUnconditionalFlowsInEndpoint(endpoint); - if (typeof cb == "function") { - cb(null, flagged); - } - }; +const onTargetEndpoint = function (endpoint, cb) { + const flagged = searchUnconditionalFlowsInEndpoint(endpoint); + if (typeof cb == "function") { + cb(null, flagged); + } + return flagged; +}; module.exports = { plugin, onProxyEndpoint, - onTargetEndpoint + onTargetEndpoint, }; diff --git a/test/fixtures/resources/statistics_collector/duplicates/apiproxy/duplicates.xml b/test/fixtures/resources/statistics_collector/duplicates/apiproxy/duplicates.xml new file mode 100644 index 00000000..2b67fdf1 --- /dev/null +++ b/test/fixtures/resources/statistics_collector/duplicates/apiproxy/duplicates.xml @@ -0,0 +1,23 @@ + + + + + + 1491498008040 + + duplicates + diff --git a/test/fixtures/resources/statistics_collector/two_duplicate_stats_collector_on_conditions/apiproxy/policies/StatisticsCollectorZip.xml b/test/fixtures/resources/statistics_collector/duplicates/apiproxy/policies/Stats-Address-1.xml similarity index 75% rename from test/fixtures/resources/statistics_collector/two_duplicate_stats_collector_on_conditions/apiproxy/policies/StatisticsCollectorZip.xml rename to test/fixtures/resources/statistics_collector/duplicates/apiproxy/policies/Stats-Address-1.xml index f7e4c407..85948920 100644 --- a/test/fixtures/resources/statistics_collector/two_duplicate_stats_collector_on_conditions/apiproxy/policies/StatisticsCollectorZip.xml +++ b/test/fixtures/resources/statistics_collector/duplicates/apiproxy/policies/Stats-Address-1.xml @@ -1,5 +1,6 @@ + - - - StatisticsCollectorZip - + - value + value value value diff --git a/test/fixtures/resources/statistics_collector/two_duplicate_stats_collector_on_conditions/apiproxy/policies/StatisticsCollectorAddress.xml b/test/fixtures/resources/statistics_collector/duplicates/apiproxy/policies/Stats-Address-1a.xml similarity index 75% rename from test/fixtures/resources/statistics_collector/two_duplicate_stats_collector_on_conditions/apiproxy/policies/StatisticsCollectorAddress.xml rename to test/fixtures/resources/statistics_collector/duplicates/apiproxy/policies/Stats-Address-1a.xml index 5d534157..be163d35 100644 --- a/test/fixtures/resources/statistics_collector/two_duplicate_stats_collector_on_conditions/apiproxy/policies/StatisticsCollectorAddress.xml +++ b/test/fixtures/resources/statistics_collector/duplicates/apiproxy/policies/Stats-Address-1a.xml @@ -1,5 +1,6 @@ + - - - StatisticsCollectorAddress - + - value + value value value diff --git a/test/fixtures/resources/statistics_collector/one_stats_collector_twosteps_one_condition/one_stats_collector_twosteps_one_condition/apiproxy/policies/StatisticsCollector1.xml b/test/fixtures/resources/statistics_collector/duplicates/apiproxy/policies/Stats-Username-1.xml similarity index 71% rename from test/fixtures/resources/statistics_collector/one_stats_collector_twosteps_one_condition/one_stats_collector_twosteps_one_condition/apiproxy/policies/StatisticsCollector1.xml rename to test/fixtures/resources/statistics_collector/duplicates/apiproxy/policies/Stats-Username-1.xml index 040f3791..8a426831 100644 --- a/test/fixtures/resources/statistics_collector/one_stats_collector_twosteps_one_condition/one_stats_collector_twosteps_one_condition/apiproxy/policies/StatisticsCollector1.xml +++ b/test/fixtures/resources/statistics_collector/duplicates/apiproxy/policies/Stats-Username-1.xml @@ -1,5 +1,6 @@ + - - - StatisticsCollector1 - + - value + value diff --git a/test/fixtures/resources/statistics_collector/one_stats_collector_no_condition/apiproxy/policies/StatisticsCollector1.xml b/test/fixtures/resources/statistics_collector/duplicates/apiproxy/policies/Stats-Username-1a.xml similarity index 71% rename from test/fixtures/resources/statistics_collector/one_stats_collector_no_condition/apiproxy/policies/StatisticsCollector1.xml rename to test/fixtures/resources/statistics_collector/duplicates/apiproxy/policies/Stats-Username-1a.xml index 040f3791..29704ee6 100644 --- a/test/fixtures/resources/statistics_collector/one_stats_collector_no_condition/apiproxy/policies/StatisticsCollector1.xml +++ b/test/fixtures/resources/statistics_collector/duplicates/apiproxy/policies/Stats-Username-1a.xml @@ -1,5 +1,6 @@ + - - - StatisticsCollector1 - + - value + value diff --git a/test/fixtures/resources/statistics_collector/multiple_stats_collector_missing_conditions/apiproxy/proxies/default.xml b/test/fixtures/resources/statistics_collector/duplicates/apiproxy/proxies/default.xml similarity index 80% rename from test/fixtures/resources/statistics_collector/multiple_stats_collector_missing_conditions/apiproxy/proxies/default.xml rename to test/fixtures/resources/statistics_collector/duplicates/apiproxy/proxies/default.xml index 1b681967..7b8d8b8d 100644 --- a/test/fixtures/resources/statistics_collector/multiple_stats_collector_missing_conditions/apiproxy/proxies/default.xml +++ b/test/fixtures/resources/statistics_collector/duplicates/apiproxy/proxies/default.xml @@ -1,5 +1,6 @@ + - - StatisticsCollector1 + Stats-Username-1 - StatisticsCollectorZip + Stats-Username-1a @@ -34,12 +34,12 @@ - StatisticsCollector10 + Stats-Username-1a - StatisticsCollectorAddress + Stats-Address-1 @@ -47,8 +47,7 @@ default - /24solver - default + /duplicates secure diff --git a/test/fixtures/resources/statistics_collector/one_stats_collector_no_condition/apiproxy/targets/default.xml b/test/fixtures/resources/statistics_collector/duplicates/apiproxy/targets/default.xml similarity index 96% rename from test/fixtures/resources/statistics_collector/one_stats_collector_no_condition/apiproxy/targets/default.xml rename to test/fixtures/resources/statistics_collector/duplicates/apiproxy/targets/default.xml index 8815b416..5e9caf8e 100644 --- a/test/fixtures/resources/statistics_collector/one_stats_collector_no_condition/apiproxy/targets/default.xml +++ b/test/fixtures/resources/statistics_collector/duplicates/apiproxy/targets/default.xml @@ -1,5 +1,6 @@ + - diff --git a/test/fixtures/resources/statistics_collector/multiple_missing_conditions/apiproxy/multiple_missing_conditions.xml b/test/fixtures/resources/statistics_collector/multiple_missing_conditions/apiproxy/multiple_missing_conditions.xml new file mode 100644 index 00000000..22477c19 --- /dev/null +++ b/test/fixtures/resources/statistics_collector/multiple_missing_conditions/apiproxy/multiple_missing_conditions.xml @@ -0,0 +1,22 @@ + + + + + 1491498008040 + + multiple_missing_conditions + diff --git a/test/fixtures/resources/statistics_collector/one_stats_collector_twosteps_one_condition/one_stats_collector_twosteps_one_condition/apiproxy/policies/StatisticsCollector10.xml b/test/fixtures/resources/statistics_collector/multiple_missing_conditions/apiproxy/policies/Stats-1.xml similarity index 74% rename from test/fixtures/resources/statistics_collector/one_stats_collector_twosteps_one_condition/one_stats_collector_twosteps_one_condition/apiproxy/policies/StatisticsCollector10.xml rename to test/fixtures/resources/statistics_collector/multiple_missing_conditions/apiproxy/policies/Stats-1.xml index 040f3791..3b32a1c3 100644 --- a/test/fixtures/resources/statistics_collector/one_stats_collector_twosteps_one_condition/one_stats_collector_twosteps_one_condition/apiproxy/policies/StatisticsCollector10.xml +++ b/test/fixtures/resources/statistics_collector/multiple_missing_conditions/apiproxy/policies/Stats-1.xml @@ -1,3 +1,4 @@ + - - - StatisticsCollector1 - + - value + value diff --git a/test/fixtures/resources/statistics_collector/multiple_stats_collector_missing_conditions/apiproxy/policies/StatisticsCollectorZip.xml b/test/fixtures/resources/statistics_collector/multiple_missing_conditions/apiproxy/policies/Stats-2.xml similarity index 73% rename from test/fixtures/resources/statistics_collector/multiple_stats_collector_missing_conditions/apiproxy/policies/StatisticsCollectorZip.xml rename to test/fixtures/resources/statistics_collector/multiple_missing_conditions/apiproxy/policies/Stats-2.xml index 6a8acf2d..bdc6994c 100644 --- a/test/fixtures/resources/statistics_collector/multiple_stats_collector_missing_conditions/apiproxy/policies/StatisticsCollectorZip.xml +++ b/test/fixtures/resources/statistics_collector/multiple_missing_conditions/apiproxy/policies/Stats-2.xml @@ -1,5 +1,6 @@ + - - - StatisticsCollectorZip - + - value + value value diff --git a/test/fixtures/resources/statistics_collector/one_stats_collector_twosteps_one_condition/apiproxy/policies/StatisticsCollector1.xml b/test/fixtures/resources/statistics_collector/multiple_missing_conditions/apiproxy/policies/Stats-3.xml similarity index 71% rename from test/fixtures/resources/statistics_collector/one_stats_collector_twosteps_one_condition/apiproxy/policies/StatisticsCollector1.xml rename to test/fixtures/resources/statistics_collector/multiple_missing_conditions/apiproxy/policies/Stats-3.xml index 040f3791..def1ee30 100644 --- a/test/fixtures/resources/statistics_collector/one_stats_collector_twosteps_one_condition/apiproxy/policies/StatisticsCollector1.xml +++ b/test/fixtures/resources/statistics_collector/multiple_missing_conditions/apiproxy/policies/Stats-3.xml @@ -1,5 +1,6 @@ + - - - StatisticsCollector1 - + - value + value diff --git a/test/fixtures/resources/statistics_collector/multiple_stats_collector_missing_conditions/apiproxy/policies/StatisticsCollectorAddress.xml b/test/fixtures/resources/statistics_collector/multiple_missing_conditions/apiproxy/policies/Stats-4.xml similarity index 75% rename from test/fixtures/resources/statistics_collector/multiple_stats_collector_missing_conditions/apiproxy/policies/StatisticsCollectorAddress.xml rename to test/fixtures/resources/statistics_collector/multiple_missing_conditions/apiproxy/policies/Stats-4.xml index 594695b9..b5f37244 100644 --- a/test/fixtures/resources/statistics_collector/multiple_stats_collector_missing_conditions/apiproxy/policies/StatisticsCollectorAddress.xml +++ b/test/fixtures/resources/statistics_collector/multiple_missing_conditions/apiproxy/policies/Stats-4.xml @@ -1,5 +1,6 @@ + - - - StatisticsCollectorAddress - + - value + value value value diff --git a/test/fixtures/resources/statistics_collector/two_duplicate_stats_collector_on_conditions/apiproxy/proxies/default.xml b/test/fixtures/resources/statistics_collector/multiple_missing_conditions/apiproxy/proxies/default.xml similarity index 80% rename from test/fixtures/resources/statistics_collector/two_duplicate_stats_collector_on_conditions/apiproxy/proxies/default.xml rename to test/fixtures/resources/statistics_collector/multiple_missing_conditions/apiproxy/proxies/default.xml index 1b681967..6b421345 100644 --- a/test/fixtures/resources/statistics_collector/two_duplicate_stats_collector_on_conditions/apiproxy/proxies/default.xml +++ b/test/fixtures/resources/statistics_collector/multiple_missing_conditions/apiproxy/proxies/default.xml @@ -1,5 +1,6 @@ + - - StatisticsCollector1 + Stats-1 - StatisticsCollectorZip + Stats-2 @@ -34,12 +34,12 @@ - StatisticsCollector10 + Stats-3 - StatisticsCollectorAddress + Stats-4 @@ -47,8 +47,7 @@ default - /24solver - default + /multiple_missing_conditions secure diff --git a/test/fixtures/resources/statistics_collector/multiple_stats_collector_missing_conditions/apiproxy/targets/default.xml b/test/fixtures/resources/statistics_collector/multiple_missing_conditions/apiproxy/targets/default.xml similarity index 100% rename from test/fixtures/resources/statistics_collector/multiple_stats_collector_missing_conditions/apiproxy/targets/default.xml rename to test/fixtures/resources/statistics_collector/multiple_missing_conditions/apiproxy/targets/default.xml diff --git a/test/fixtures/resources/statistics_collector/multiple_stats_collector_missing_conditions/apiproxy/policies/StatisticsCollector1.xml b/test/fixtures/resources/statistics_collector/multiple_stats_collector_missing_conditions/apiproxy/policies/StatisticsCollector1.xml deleted file mode 100644 index 39948b29..00000000 --- a/test/fixtures/resources/statistics_collector/multiple_stats_collector_missing_conditions/apiproxy/policies/StatisticsCollector1.xml +++ /dev/null @@ -1,24 +0,0 @@ - - - - - StatisticsCollector1 - - - value - - diff --git a/test/fixtures/resources/statistics_collector/multiple_stats_collector_missing_conditions/apiproxy/policies/StatisticsCollector10.xml b/test/fixtures/resources/statistics_collector/multiple_stats_collector_missing_conditions/apiproxy/policies/StatisticsCollector10.xml deleted file mode 100644 index 2966754f..00000000 --- a/test/fixtures/resources/statistics_collector/multiple_stats_collector_missing_conditions/apiproxy/policies/StatisticsCollector10.xml +++ /dev/null @@ -1,24 +0,0 @@ - - - - - StatisticsCollector10 - - - value - - diff --git a/test/fixtures/resources/statistics_collector/no_condition/apiproxy/no_condition.xml b/test/fixtures/resources/statistics_collector/no_condition/apiproxy/no_condition.xml new file mode 100644 index 00000000..b833759f --- /dev/null +++ b/test/fixtures/resources/statistics_collector/no_condition/apiproxy/no_condition.xml @@ -0,0 +1,22 @@ + + + + + 1491498008040 + + no_condition + diff --git a/test/fixtures/resources/statistics_collector/no_condition/apiproxy/policies/Stats-1.xml b/test/fixtures/resources/statistics_collector/no_condition/apiproxy/policies/Stats-1.xml new file mode 100644 index 00000000..eaa68327 --- /dev/null +++ b/test/fixtures/resources/statistics_collector/no_condition/apiproxy/policies/Stats-1.xml @@ -0,0 +1,22 @@ + + + + + + value + + diff --git a/test/fixtures/resources/statistics_collector/one_stats_collector_no_condition/apiproxy/proxies/default.xml b/test/fixtures/resources/statistics_collector/no_condition/apiproxy/proxies/default.xml similarity index 87% rename from test/fixtures/resources/statistics_collector/one_stats_collector_no_condition/apiproxy/proxies/default.xml rename to test/fixtures/resources/statistics_collector/no_condition/apiproxy/proxies/default.xml index 8e7487de..9cd5bc2e 100644 --- a/test/fixtures/resources/statistics_collector/one_stats_collector_no_condition/apiproxy/proxies/default.xml +++ b/test/fixtures/resources/statistics_collector/no_condition/apiproxy/proxies/default.xml @@ -1,5 +1,6 @@ + - - StatisticsCollector1 + Stats-1 @@ -36,8 +36,7 @@ default - /24solver - default + /no_condition secure diff --git a/test/fixtures/resources/statistics_collector/one_stats_collector_twosteps_one_condition/apiproxy/targets/default.xml b/test/fixtures/resources/statistics_collector/no_condition/apiproxy/targets/default.xml similarity index 96% rename from test/fixtures/resources/statistics_collector/one_stats_collector_twosteps_one_condition/apiproxy/targets/default.xml rename to test/fixtures/resources/statistics_collector/no_condition/apiproxy/targets/default.xml index 8815b416..5e9caf8e 100644 --- a/test/fixtures/resources/statistics_collector/one_stats_collector_twosteps_one_condition/apiproxy/targets/default.xml +++ b/test/fixtures/resources/statistics_collector/no_condition/apiproxy/targets/default.xml @@ -1,5 +1,6 @@ + - diff --git a/test/fixtures/resources/statistics_collector/one_stats_collector_twosteps_one_condition/apiproxy/proxies/default.xml b/test/fixtures/resources/statistics_collector/one_stats_collector_twosteps_one_condition/apiproxy/proxies/default.xml deleted file mode 100644 index ff9fadbc..00000000 --- a/test/fixtures/resources/statistics_collector/one_stats_collector_twosteps_one_condition/apiproxy/proxies/default.xml +++ /dev/null @@ -1,48 +0,0 @@ - - - - - - - - - StatisticsCollector1 - - - - - - - - - - StatisticsCollector1 - request.queryparm.query == "test" - - - - - - - default - - - /24solver - default - secure - - diff --git a/test/fixtures/resources/statistics_collector/two_duplicate_stats_collector_on_conditions/apiproxy/policies/StatisticsCollector1.xml b/test/fixtures/resources/statistics_collector/two_duplicate_stats_collector_on_conditions/apiproxy/policies/StatisticsCollector1.xml deleted file mode 100644 index 040f3791..00000000 --- a/test/fixtures/resources/statistics_collector/two_duplicate_stats_collector_on_conditions/apiproxy/policies/StatisticsCollector1.xml +++ /dev/null @@ -1,24 +0,0 @@ - - - - - StatisticsCollector1 - - - value - - diff --git a/test/fixtures/resources/statistics_collector/two_duplicate_stats_collector_on_conditions/apiproxy/policies/StatisticsCollector10.xml b/test/fixtures/resources/statistics_collector/two_duplicate_stats_collector_on_conditions/apiproxy/policies/StatisticsCollector10.xml deleted file mode 100644 index 2966754f..00000000 --- a/test/fixtures/resources/statistics_collector/two_duplicate_stats_collector_on_conditions/apiproxy/policies/StatisticsCollector10.xml +++ /dev/null @@ -1,24 +0,0 @@ - - - - - StatisticsCollector10 - - - value - - diff --git a/test/fixtures/resources/statistics_collector/two_duplicate_stats_collector_on_conditions/apiproxy/targets/default.xml b/test/fixtures/resources/statistics_collector/two_duplicate_stats_collector_on_conditions/apiproxy/targets/default.xml deleted file mode 100644 index 8815b416..00000000 --- a/test/fixtures/resources/statistics_collector/two_duplicate_stats_collector_on_conditions/apiproxy/targets/default.xml +++ /dev/null @@ -1,33 +0,0 @@ - - - - - - - - - - - - - http://api.foo.com/ - - - - - - diff --git a/test/fixtures/resources/statistics_collector/twosteps_one_condition/apiproxy/policies/Stats-1.xml b/test/fixtures/resources/statistics_collector/twosteps_one_condition/apiproxy/policies/Stats-1.xml new file mode 100644 index 00000000..aede8a6e --- /dev/null +++ b/test/fixtures/resources/statistics_collector/twosteps_one_condition/apiproxy/policies/Stats-1.xml @@ -0,0 +1,22 @@ + + + + + + value + + diff --git a/test/fixtures/resources/statistics_collector/one_stats_collector_twosteps_one_condition/one_stats_collector_twosteps_one_condition/apiproxy/proxies/default.xml b/test/fixtures/resources/statistics_collector/twosteps_one_condition/apiproxy/proxies/default.xml similarity index 85% rename from test/fixtures/resources/statistics_collector/one_stats_collector_twosteps_one_condition/one_stats_collector_twosteps_one_condition/apiproxy/proxies/default.xml rename to test/fixtures/resources/statistics_collector/twosteps_one_condition/apiproxy/proxies/default.xml index ff9fadbc..00d3122f 100644 --- a/test/fixtures/resources/statistics_collector/one_stats_collector_twosteps_one_condition/one_stats_collector_twosteps_one_condition/apiproxy/proxies/default.xml +++ b/test/fixtures/resources/statistics_collector/twosteps_one_condition/apiproxy/proxies/default.xml @@ -1,5 +1,6 @@ + - - StatisticsCollector1 + Stats-1 @@ -30,7 +30,7 @@ - StatisticsCollector1 + Stats-1 request.queryparm.query == "test" @@ -41,8 +41,7 @@ default - /24solver - default + /twosteps_one_condition secure diff --git a/test/fixtures/resources/statistics_collector/one_stats_collector_twosteps_one_condition/one_stats_collector_twosteps_one_condition/apiproxy/targets/default.xml b/test/fixtures/resources/statistics_collector/twosteps_one_condition/apiproxy/targets/default.xml similarity index 96% rename from test/fixtures/resources/statistics_collector/one_stats_collector_twosteps_one_condition/one_stats_collector_twosteps_one_condition/apiproxy/targets/default.xml rename to test/fixtures/resources/statistics_collector/twosteps_one_condition/apiproxy/targets/default.xml index 8815b416..5e9caf8e 100644 --- a/test/fixtures/resources/statistics_collector/one_stats_collector_twosteps_one_condition/one_stats_collector_twosteps_one_condition/apiproxy/targets/default.xml +++ b/test/fixtures/resources/statistics_collector/twosteps_one_condition/apiproxy/targets/default.xml @@ -1,5 +1,6 @@ + - diff --git a/test/fixtures/resources/statistics_collector/twosteps_one_condition/apiproxy/twosteps_one_condition.xml b/test/fixtures/resources/statistics_collector/twosteps_one_condition/apiproxy/twosteps_one_condition.xml new file mode 100644 index 00000000..b4941b96 --- /dev/null +++ b/test/fixtures/resources/statistics_collector/twosteps_one_condition/apiproxy/twosteps_one_condition.xml @@ -0,0 +1,23 @@ + + + + + + 1491498008040 + + twosteps_one_condition + diff --git a/test/fixtures/resources/test_flow/apiproxy/policies/StatisticsCollector1.xml b/test/fixtures/resources/test_flow/apiproxy/policies/StatisticsCollector1.xml index 39948b29..8634a9f9 100644 --- a/test/fixtures/resources/test_flow/apiproxy/policies/StatisticsCollector1.xml +++ b/test/fixtures/resources/test_flow/apiproxy/policies/StatisticsCollector1.xml @@ -1,3 +1,4 @@ + - - StatisticsCollector1 diff --git a/test/fixtures/resources/test_flow/apiproxy/policies/StatisticsCollector10.xml b/test/fixtures/resources/test_flow/apiproxy/policies/StatisticsCollector10.xml index 2966754f..a1812197 100644 --- a/test/fixtures/resources/test_flow/apiproxy/policies/StatisticsCollector10.xml +++ b/test/fixtures/resources/test_flow/apiproxy/policies/StatisticsCollector10.xml @@ -1,3 +1,4 @@ + - StatisticsCollector10 diff --git a/test/fixtures/resources/test_flow/apiproxy/policies/StatisticsCollectorAddress.xml b/test/fixtures/resources/test_flow/apiproxy/policies/StatisticsCollectorAddress.xml index 594695b9..a13f5408 100644 --- a/test/fixtures/resources/test_flow/apiproxy/policies/StatisticsCollectorAddress.xml +++ b/test/fixtures/resources/test_flow/apiproxy/policies/StatisticsCollectorAddress.xml @@ -1,3 +1,4 @@ + - StatisticsCollectorAddress diff --git a/test/fixtures/resources/test_flow/apiproxy/policies/StatisticsCollectorFlows.xml b/test/fixtures/resources/test_flow/apiproxy/policies/StatisticsCollectorFlows.xml index 43ee2076..4fc9ff28 100644 --- a/test/fixtures/resources/test_flow/apiproxy/policies/StatisticsCollectorFlows.xml +++ b/test/fixtures/resources/test_flow/apiproxy/policies/StatisticsCollectorFlows.xml @@ -1,3 +1,4 @@ + - StatisticsCollectorFlows diff --git a/test/fixtures/resources/test_flow/apiproxy/policies/StatisticsCollectorFlows2.xml b/test/fixtures/resources/test_flow/apiproxy/policies/StatisticsCollectorFlows2.xml index 27fb8af6..d823b044 100644 --- a/test/fixtures/resources/test_flow/apiproxy/policies/StatisticsCollectorFlows2.xml +++ b/test/fixtures/resources/test_flow/apiproxy/policies/StatisticsCollectorFlows2.xml @@ -1,3 +1,4 @@ + - StatisticsCollectorFlows2 diff --git a/test/fixtures/resources/test_flow/apiproxy/policies/StatisticsCollectorZip.xml b/test/fixtures/resources/test_flow/apiproxy/policies/StatisticsCollectorZip.xml index 6a8acf2d..7b068ffe 100644 --- a/test/fixtures/resources/test_flow/apiproxy/policies/StatisticsCollectorZip.xml +++ b/test/fixtures/resources/test_flow/apiproxy/policies/StatisticsCollectorZip.xml @@ -1,3 +1,4 @@ + - StatisticsCollectorZip diff --git a/test/fixtures/resources/test_flow/apiproxy/proxies/default.xml b/test/fixtures/resources/test_flow/apiproxy/proxies/default.xml index 37f86eaa..8d8790c1 100644 --- a/test/fixtures/resources/test_flow/apiproxy/proxies/default.xml +++ b/test/fixtures/resources/test_flow/apiproxy/proxies/default.xml @@ -1,3 +1,4 @@ + - diff --git a/test/fixtures/resources/test_flow/apiproxy/targets/default.xml b/test/fixtures/resources/test_flow/apiproxy/targets/default.xml index 8815b416..c296a4bd 100644 --- a/test/fixtures/resources/test_flow/apiproxy/targets/default.xml +++ b/test/fixtures/resources/test_flow/apiproxy/targets/default.xml @@ -1,3 +1,4 @@ + - diff --git a/test/fixtures/resources/test_flow/apiproxy/test_flow.xml b/test/fixtures/resources/test_flow/apiproxy/test_flow.xml new file mode 100644 index 00000000..703ce072 --- /dev/null +++ b/test/fixtures/resources/test_flow/apiproxy/test_flow.xml @@ -0,0 +1,6 @@ + + + 1491498008040 + + test_flow + diff --git a/test/specs/BN009-MultipleStatsCollectors.js b/test/specs/BN009-MultipleStatsCollectors.js index 3dc82ace..19eed403 100644 --- a/test/specs/BN009-MultipleStatsCollectors.js +++ b/test/specs/BN009-MultipleStatsCollectors.js @@ -1,5 +1,5 @@ /* - Copyright 2019-2021 Google LLC + Copyright 2019-2021,2023 Google LLC Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -17,29 +17,191 @@ /* global describe, it, configuration */ const assert = require("assert"), - testID = "BN009", - debug = require("debug")("apigeelint:" + testID), - Bundle = require("../../lib/package/Bundle.js"), - bl = require("../../lib/package/bundleLinter.js"), - plugin = require(bl.resolvePlugin(testID)); + path = require("path"), + util = require("util"), + testID = "BN009", + debug = require("debug")(`apigeelint:${testID}-test`), + Bundle = require("../../lib/package/Bundle.js"), + bl = require("../../lib/package/bundleLinter.js"), + plugin = require(bl.resolvePlugin(testID)); -describe(`${testID} - ${plugin.plugin.name}`, function() { +const testOneProxy = (proxyName, expected) => { + const configuration = { + debug: true, + source: { + type: "filesystem", + path: path.resolve( + __dirname, + `../fixtures/resources/statistics_collector/${proxyName}/apiproxy` + ), + bundleType: "apiproxy" + }, + profile: "apigee", + excluded: {}, + setExitCode: false, + output: () => {} // suppress output + }; + bl.lint(configuration, (bundle) => { + const items = bundle.getReport(); + assert.ok(items); + assert.ok(items.length); + + const bn009Errors = items.filter((item) => + item.messages.find((m) => m.ruleId == "BN009") + ); + debug(`bn009: ${JSON.stringify(bn009Errors, null, 2)}`); + + const processed = []; + Object.keys(expected).forEach((policyName, px) => { + const policyItems = items.filter((m) => m.filePath.endsWith(policyName)); + assert.equal( + policyItems.length, + 1, + `No errors found for policy ${policyName}` + ); + + const bn009Messages = policyItems[0].messages.filter( + (m) => m.ruleId == testID + ); + assert.equal(bn009Messages.length, expected[policyName].length); + + debug(`expected[${policyName}]: ${util.format(expected[policyName])}`); + + expected[policyName].forEach((expectedItem, ix) => { + const matched = bn009Messages.find( + (item) => item.message == expectedItem.message + ); + assert.ok(matched, `did not find message like ${expectedItem.message}`); + Object.keys(expectedItem).find((key) => { + assert.ok( + matched[key], + expectedItem[key], + `case(${px},${ix}) key(${key})` + ); + }); + }); + processed.push(policyName); + }); + + const allFilesWithBn009Errors = bn009Errors.map((item) => + item.filePath.split(path.sep).pop() + ); + + debug(`allFilesWithBN009: ${allFilesWithBn009Errors}`); + const notProcessed = allFilesWithBn009Errors.filter( + (x) => !processed.includes(x) + ); + debug(`notProcessed: ${notProcessed}`); + assert.equal(notProcessed.length, 0); + }); +}; + +describe(`${testID} - MultipleStatsCollectors`, () => { + it("should generate errors for duplicates", () => { + const expected = { + "Stats-Address-1.xml": [ + { + message: + "The following StatisticsCollector policies are duplicates: Stats-Address-1a" + }, + { + message: + "Stats-Address-1 is attached to a step without a Condition. If you have more than two StatisticsCollector policies, only the last one in the flow will execute. Include a Condition to make sure the correct one executes." + } + ], + "Stats-Username-1.xml": [ + { + message: + "The following StatisticsCollector policies are duplicates: Stats-Username-1a" + }, + { + message: + "Stats-Username-1 is attached to a step without a Condition. If you have more than two StatisticsCollector policies, only the last one in the flow will execute. Include a Condition to make sure the correct one executes." + } + ], + "Stats-Username-1a.xml": [ + { + message: + "Stats-Username-1a is attached to multiple steps, but all the steps don't have a condition. This may result in unexpected behaviour." + }, + { + message: + "Stats-Username-1a is attached to a step without a Condition. If you have more than two StatisticsCollector policies, only the last one in the flow will execute. Include a Condition to make sure the correct one executes." + } + ] + }; + + testOneProxy("duplicates", expected); + }); + + it("should generate errors for no_condition", () => { + const expected = {}; + + testOneProxy("no_condition", expected); + }); + + it("should generate errors for twosteps_one_condition", () => { + const expected = { + "Stats-1.xml": [ + { + message: + "Stats-1 is attached to multiple steps, but all the steps don't have a condition. This may result in unexpected behaviour." + } + ] + }; + + testOneProxy("twosteps_one_condition", expected); + }); + + it("should generate errors for multiple_missing_conditions", () => { + const expected = { + "Stats-1.xml": [ + { + message: + "Stats-1 is attached to a step without a Condition. If you have more than two StatisticsCollector policies, only the last one in the flow will execute. Include a Condition to make sure the correct one executes." + } + ], + "Stats-2.xml": [ + { + message: + "Stats-2 is attached to a step without a Condition. If you have more than two StatisticsCollector policies, only the last one in the flow will execute. Include a Condition to make sure the correct one executes." + } + ], + "Stats-3.xml": [ + { + message: + "Stats-3 is attached to a step without a Condition. If you have more than two StatisticsCollector policies, only the last one in the flow will execute. Include a Condition to make sure the correct one executes." + } + ], + "Stats-4.xml": [ + { + message: + "Stats-4 is attached to a step without a Condition. If you have more than two StatisticsCollector policies, only the last one in the flow will execute. Include a Condition to make sure the correct one executes." + } + ] + }; + + testOneProxy("multiple_missing_conditions", expected); + }); +}); + +describe(`${testID} - print plugin results for ${plugin.plugin.name}`, function () { debug("test configuration: " + JSON.stringify(configuration)); - let bundle = new Bundle(configuration); + const bundle = new Bundle(configuration); bl.executePlugin(testID, bundle); - let report = bundle.getReport(); + const report = bundle.getReport(); - it("should create a report object with valid schema", function() { - let formatter = bl.getFormatter("json.js"); + it("should create a report object with valid schema", function () { + const formatter = bl.getFormatter("json.js"); if (!formatter) { assert.fail("formatter implementation not defined"); } - let schema = require("./../fixtures/reportSchema.js"), - Validator = require("jsonschema").Validator, - v = new Validator(), - jsonReport = JSON.parse(formatter(report)), - validationResult = v.validate(jsonReport, schema); + const schema = require("./../fixtures/reportSchema.js"), + Validator = require("jsonschema").Validator, + v = new Validator(), + jsonReport = JSON.parse(formatter(report)), + validationResult = v.validate(jsonReport, schema); assert.equal(validationResult.errors.length, 0, validationResult.errors); }); });