Skip to content

Commit

Permalink
fix: correct BN009 behavior for duplicate StatsCollector
Browse files Browse the repository at this point in the history 
- 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
  • Loading branch information
@DinoChiesa
DinoChiesa committed Oct 2, 2023
1 parent 8668bf3 commit fcda555
Show file tree
Hide file tree
Showing 42 changed files with 1,009 additions and 836 deletions.
410 changes: 207 additions & 203 deletions lib/package/Bundle.js
View file

Large diffs are not rendered by default.

251 changes: 97 additions & 154 deletions lib/package/plugins/BN009-checkForMultipleStatsCollectorPolicies.js
View file
Original file line number Diff line number Diff line change
@@ -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.
Expand All @@ -15,67 +15,70 @@
*/

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);
checkPoliciesForMissingConditions(statsPolicies, bundle);
}

/*
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(),
Expand All @@ -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(
Expand All @@ -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;
Expand All @@ -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;
Expand Down
Loading

0 comments on commit fcda555

Please sign in to comment.