Skip to content

Commit

Permalink
fix(rule): template-conditional-complexity not reporting failures for…
Browse files Browse the repository at this point in the history 
… '[ngIf]' (#611)
  • Loading branch information
@rafaelss95 @wKoza
rafaelss95 authored and wKoza committed May 8, 2018
1 parent fedd331 commit 7fc3b09
Show file tree
Hide file tree
Showing 2 changed files with 142 additions and 114 deletions.
141 changes: 77 additions & 64 deletions src/templateConditionalComplexityRule.ts
View file
Original file line number Diff line number Diff line change
@@ -1,36 +1,33 @@
import * as Lint from 'tslint';
import * as ts from 'typescript';
import * as ast from '@angular/compiler';
import { AST, ASTWithSource, Binary, BoundDirectivePropertyAst, Lexer, Parser } from '@angular/compiler';
import { sprintf } from 'sprintf-js';
import { BasicTemplateAstVisitor } from './angular/templates/basicTemplateAstVisitor';
import { IRuleMetadata, RuleFailure, Rules } from 'tslint/lib';
import { SourceFile } from 'typescript/lib/typescript';
import { NgWalker } from './angular/ngWalker';
import * as compiler from '@angular/compiler';
import { Binary } from '@angular/compiler';
import { BasicTemplateAstVisitor } from './angular/templates/basicTemplateAstVisitor';

export class Rule extends Lint.Rules.AbstractRule {
public static metadata: Lint.IRuleMetadata = {
ruleName: 'template-conditional-complexity',
type: 'functionality',
export class Rule extends Rules.AbstractRule {
static readonly metadata: IRuleMetadata = {
description: "The condition complexity shouldn't exceed a rational limit in a template.",
rationale: 'An important complexity complicates the tests and the maintenance.',
optionExamples: ['true', '[true, 4]'],
options: {
type: 'array',
items: {
type: 'string'
},
maxLength: 2,
minLength: 0,
maxLength: 2
type: 'array'
},
optionExamples: ['true', '[true, 4]'],
optionsDescription: 'Determine the maximum number of Boolean operators allowed.',
rationale: 'An important complexity complicates the tests and the maintenance.',
ruleName: 'template-conditional-complexity',
type: 'maintainability',
typescriptOnly: true
};

static COMPLEXITY_FAILURE_STRING = "The condition complexity (cost '%s') exceeded the defined limit (cost '%s'). The conditional expression should be moved into the component.";

static COMPLEXITY_MAX = 3;
static readonly FAILURE_STRING = "The condition complexity (cost '%s') exceeded the defined limit (cost '%s'). The conditional expression should be moved into the component.";
static readonly DEFAULT_MAX_COMPLEXITY = 3;

public apply(sourceFile: ts.SourceFile): Lint.RuleFailure[] {
apply(sourceFile: SourceFile): RuleFailure[] {
return this.applyWithWalker(
new NgWalker(sourceFile, this.getOptions(), {
templateVisitorCtrl: TemplateConditionalComplexityVisitor
Expand All @@ -39,53 +36,69 @@ export class Rule extends Lint.Rules.AbstractRule {
}
}

class TemplateConditionalComplexityVisitor extends BasicTemplateAstVisitor {
visitDirectiveProperty(prop: ast.BoundDirectivePropertyAst, context: BasicTemplateAstVisitor): any {
if (prop.sourceSpan) {
const directive = (<any>prop.sourceSpan).toString();

if (directive.startsWith('*ngIf')) {
// extract expression and drop characters new line and quotes
const expr = directive
.split(/\*ngIf\s*=\s*/)[1]
.slice(1, -1)
.replace(/[\n\r]/g, '');

const expressionParser = new compiler.Parser(new compiler.Lexer());
const ast = expressionParser.parseAction(expr, null);

let complexity = 0;
let conditions: Array<Binary> = [];
let condition = ast.ast as Binary;
if (condition.operation) {
complexity++;
conditions.push(condition);
}

while (conditions.length > 0) {
condition = conditions.pop();
if (condition.operation) {
if (condition.left instanceof Binary) {
complexity++;
conditions.push(condition.left as Binary);
}

if (condition.right instanceof Binary) {
conditions.push(condition.right as Binary);
}
}
}
const options = this.getOptions();
const complexityMax: number = options.length ? options[0] : Rule.COMPLEXITY_MAX;

if (complexity > complexityMax) {
const span = prop.sourceSpan;
let failureConfig: string[] = [String(complexity), String(complexityMax)];
failureConfig.unshift(Rule.COMPLEXITY_FAILURE_STRING);
this.addFailure(this.createFailure(span.start.offset, span.end.offset - span.start.offset, sprintf.apply(this, failureConfig)));
}
}
export const getFailureMessage = (totalComplexity: number, maxComplexity = Rule.DEFAULT_MAX_COMPLEXITY): string => {
return sprintf(Rule.FAILURE_STRING, totalComplexity, maxComplexity);
};

const getTotalComplexity = (ast: AST): number => {
const expr = (ast as ASTWithSource).source.replace(/\s/g, '');
const expressionParser = new Parser(new Lexer());
const astWithSource = expressionParser.parseAction(expr, null);
const conditions: Binary[] = [];
let totalComplexity = 0;
let condition = astWithSource.ast as Binary;

if (condition.operation) {
totalComplexity++;
conditions.push(condition);
}

while (conditions.length > 0) {
condition = conditions.pop();

if (!condition.operation) {
continue;
}

if (condition.left instanceof Binary) {
totalComplexity++;
conditions.push(condition.left);
}

if (condition.right instanceof Binary) {
conditions.push(condition.right);
}
}

return totalComplexity;
};

class TemplateConditionalComplexityVisitor extends BasicTemplateAstVisitor {
visitDirectiveProperty(prop: BoundDirectivePropertyAst, context: BasicTemplateAstVisitor): any {
this.validateDirective(prop);
super.visitDirectiveProperty(prop, context);
}

private validateDirective(prop: BoundDirectivePropertyAst): void {
const { templateName, value } = prop;

if (templateName !== 'ngIf') {
return;
}

const maxComplexity: number = this.getOptions()[0] || Rule.DEFAULT_MAX_COMPLEXITY;
const totalComplexity = getTotalComplexity(value);

if (totalComplexity <= maxComplexity) {
return;
}

const {
sourceSpan: {
end: { offset: endOffset },
start: { offset: startOffset }
}
} = prop;
this.addFailureFromStartToEnd(startOffset, endOffset, getFailureMessage(totalComplexity, maxComplexity));
}
}
115 changes: 65 additions & 50 deletions test/templateConditionalComplexityRule.spec.ts
View file
Original file line number Diff line number Diff line change
@@ -1,122 +1,137 @@
import { assertSuccess, assertAnnotated } from './testHelper';
import { getFailureMessage, Rule } from '../src/templateConditionalComplexityRule';
import { assertAnnotated, assertSuccess } from './testHelper';

describe('complexity', () => {
describe('success', () => {
it('should work with a lower level of complexity', () => {
let source = `
const {
metadata: { ruleName }
} = Rule;

describe(ruleName, () => {
describe('failure', () => {
it('should fail with a higher level of complexity', () => {
const source = `
@Component({
template: \`
<div *ngIf="a === '1' || (b === '2' && c.d !== e)">
<div *ngIf="a === '3' || (b === '3' && c.d !== '1' && e.f !== '6' && q !== g)">
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Enter your card details
</div>
\`
})
class Bar {}
`;
assertSuccess('template-conditional-complexity', source);
assertAnnotated({ message: getFailureMessage(5, 4), options: [4], ruleName, source });
});

it('should work with a lower level of complexity', () => {
let source = `
it('should fail with a higher level of complexity and a carrier return', () => {
const source = `
@Component({
template: \`
<div *ngIf="a === '1' || b === '2' && c.d !== e">
<div *ngIf="a === '3' || (b === '3' && c.d !== '1'
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
&& e.f !== '6' && q !== g)">
~~~~~~~~~~~~~~~~~~~~~~~~~~~
Enter your card details
</div>
\`
})
class Bar {}
`;
assertSuccess('template-conditional-complexity', source);
assertAnnotated({ message: getFailureMessage(5, 3), ruleName, source });
});

it('should work with a level of complexity customisable', () => {
let source = `
it('should fail with a higher level of complexity with ng-template', () => {
const source = `
@Component({
template: \`
<div *ngIf="a === '3' || (b === '3' && c.d !== '1' && e.f !== '6' && q !== g)">
<ng-template [ngIf]="a === '3' || (b === '3' && c.d !== '1'
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
&& e.f !== '6' && q !== g && x === '1')">
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Enter details
</ng-template>
\`
})
class Bar {}
`;
assertAnnotated({ message: getFailureMessage(6, 3), ruleName, source });
});
});

describe('success', () => {
it('should succeed with a lower level of complexity', () => {
const source = `
@Component({
template: \`
<div *ngIf="a === '1' || b === '2' && c.d !== e">
Enter your card details
</div>
\`
})
class Bar {}
`;
assertSuccess('template-conditional-complexity', source, [5]);
assertSuccess(ruleName, source);
});

it('should work with a level of complexity customisable', () => {
let source = `
it('should succeed with a lower level of complexity with separated statements', () => {
const source = `
@Component({
template: \`
<div *ngIf="(b === '3' && c.d !== '1' && e.f !== '6' && q !== g) || a === '3'">
<div *ngIf="a === '1' || (b === '2' && c.d !== e)">
Enter your card details
</div>
\`
})
class Bar {}
`;
assertSuccess('template-conditional-complexity', source, [5]);
assertSuccess(ruleName, source);
});

it('should work with something else', () => {
let source = `
it('should succeed with a level of complexity customizable', () => {
const source = `
@Component({
template: \`
<div *ngIf="isValid;then content else other_content">
<div *ngIf="a === '3' || (b === '3' && c.d !== '1' && e.f !== '6' && q !== g)">
Enter your card details
</div>
\`
})
class Bar {}
`;
assertSuccess('template-conditional-complexity', source, [5]);
assertSuccess(ruleName, source, [5]);
});
});

describe('failure', () => {
it('should fail with a higher level of complexity', () => {
let source = `
it('should succeed with a level of complexity customizable', () => {
const source = `
@Component({
template: \`
<div *ngIf="a === '3' || (b === '3' && c.d !== '1' && e.f !== '6' && q !== g)">
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
<div *ngIf="(b === '3' && c.d !== '1' && e.f !== '6' && q !== g) || a === '3'">
Enter your card details
</div>
\`
})
class Bar {}
`;
assertAnnotated({
ruleName: 'template-conditional-complexity',
message:
"The condition complexity (cost '5') exceeded the defined limit (cost '4'). The conditional expression should be moved into the component.",
source,
options: [4]
});
assertSuccess(ruleName, source, [5]);
});
});

describe('failure', () => {
it('should fail with a higher level of complexity and a carrier return', () => {
let source = `
it('should succeed with non-inlined then template', () => {
const source = `
@Component({
template: \`
<div *ngIf="a === '3' || (b === '3' && c.d !== '1'
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
&& e.f !== '6' && q !== g)">
~~~~~~~~~~~~~~~~~~~~~~~~~~~
<div *ngIf="isValid; then thenBlock; else elseBlock">
Enter your card details
</div>
<ng-template #thenBlock>
thenBlock
</ng-template>
<ng-template #elseBlock>
elseBlock
</ng-template>
\`
})
class Bar {}
`;
assertAnnotated({
ruleName: 'template-conditional-complexity',
message:
"The condition complexity (cost '5') exceeded the defined limit (cost '3'). The conditional expression should be moved into the component.",
source
});
assertSuccess(ruleName, source, [5]);
});
});
});

0 comments on commit 7fc3b09

Please sign in to comment.