Skip to content

build: expand decorators validation rule to cover class members #17673

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Nov 18, 2019
Merged

build: expand decorators validation rule to cover class members #17673

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
180 changes: 118 additions & 62 deletions tools/tslint-rules/validateDecoratorsRule.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,16 @@ import * as minimatch from 'minimatch';
* ```
* "validate-decorators": [true, {
* "Component": {
* "encapsulation": "\\.None$",
* "!styles": ".*"
* "argument": 0,
* "properties": {
* "encapsulation": "\\.None$",
* "!styles": ".*"
* }
* },
* "NgModule": "^(?!\\s*$).+"
* "NgModule": {
* "argument": 0,
* "properties": "^(?!\\s*$).+"
* }
* }, "src/material"]
* ```
*/
Expand All @@ -24,15 +30,32 @@ export class Rule extends Lint.Rules.AbstractRule {
}
}

/**
* Token used to indicate that all properties of an object
* should be linted against a single pattern.
*/
const ALL_PROPS_TOKEN = '*';

/** Object that can be used to configured the rule. */
interface RuleConfig {
[key: string]: {
argument: number,
required?: boolean,
properties: {[key: string]: string}
};
}

/** Represents a set of required and forbidden decorator properties. */
type DecoratorRuleSet = {
required: {[key: string]: RegExp},
forbidden: {[key: string]: RegExp},
argument: number,
required: boolean,
requiredProps: {[key: string]: RegExp},
forbiddenProps: {[key: string]: RegExp},
};

/** Represents a map between decorator names and rule sets. */
type DecoratorRules = {
[decorator: string]: DecoratorRuleSet | RegExp
[decorator: string]: DecoratorRuleSet
};

class Walker extends Lint.RuleWalker {
Expand All @@ -57,8 +80,16 @@ class Walker extends Lint.RuleWalker {
}

visitClassDeclaration(node: ts.ClassDeclaration) {
if (this._enabled && node.decorators) {
node.decorators.forEach(decorator => this._validatedDecorator(decorator.expression));
if (this._enabled) {
if (node.decorators) {
node.decorators.forEach(decorator => this._validateDecorator(decorator));
}

node.members.forEach(member => {
if (member.decorators) {
member.decorators.forEach(decorator => this._validateDecorator(decorator));
}
});
}

super.visitClassDeclaration(node);
Expand All @@ -68,63 +99,76 @@ class Walker extends Lint.RuleWalker {
* Validates that a decorator matches all of the defined rules.
* @param decorator Decorator to be checked.
*/
private _validatedDecorator(decorator: any) {
private _validateDecorator(decorator: ts.Decorator) {
const expression = decorator.expression;

if (!expression || !ts.isCallExpression(expression)) {
return;
}

// Get the rules that are relevant for the current decorator.
const rules = this._rules[decorator.expression.getText()];
const rules = this._rules[expression.expression.getText()];
const args = expression.arguments;

// Don't do anything if there are no rules.
if (!rules) {
return;
}

// If the rule is a regex, extract the arguments as a string and run it against them.
if (rules instanceof RegExp) {
const decoratorText: string = decorator.getText();
const openParenthesisIndex = decoratorText.indexOf('(');
const closeParenthesisIndex = decoratorText.lastIndexOf(')');
const argumentsText = openParenthesisIndex > -1 ? decoratorText.substring(
openParenthesisIndex + 1,
closeParenthesisIndex > -1 ? closeParenthesisIndex : decoratorText.length) : '';

if (!rules.test(argumentsText)) {
this.addFailureAtNode(decorator.parent, `Expected decorator arguments to match "${rules}"`);
}
const allPropsRequirement = rules.requiredProps[ALL_PROPS_TOKEN];

// If we have a rule that applies to all properties, we just run it through once and we exit.
if (allPropsRequirement) {
const argumentText = args[rules.argument] ? args[rules.argument].getText() : '';
if (!allPropsRequirement.test(argumentText)) {
this.addFailureAtNode(expression.parent, `Expected decorator argument ${rules.argument} ` +
`to match "${allPropsRequirement}"`);
}
return;
}

const args = decorator.arguments;
if (!args[rules.argument]) {
if (rules.required) {
this.addFailureAtNode(expression.parent,
`Missing required argument at index ${rules.argument}`);
}
return;
}

if (!args || !args.length || !args[0].properties) {
if (!ts.isObjectLiteralExpression(args[rules.argument])) {
return;
}

// Extract the property names and values.
const props = args[0].properties
.filter((node: ts.PropertyAssignment) => node.name && node.initializer)
.map((node: ts.PropertyAssignment) => ({
name: node.name.getText(),
value: node.initializer.getText(),
node
}));
const props: {name: string, value: string, node: ts.PropertyAssignment}[] = [];

(args[rules.argument] as ts.ObjectLiteralExpression).properties.forEach(prop => {
if (ts.isPropertyAssignment(prop) && prop.name && prop.initializer) {
props.push({
name: prop.name.getText(),
value: prop.initializer.getText(),
node: prop
});
}
});

// Find all of the required rule properties that are missing from the decorator.
const missing = Object.keys(rules.required)
.filter(key => !props.find((prop: any) => prop.name === key));
const missing = Object.keys(rules.requiredProps)
.filter(key => !props.find(prop => prop.name === key));

if (missing.length) {
// Exit early if any of the properties are missing.
this.addFailureAtNode(decorator.expression,
this.addFailureAtNode(expression.expression,
'Missing required properties: ' + missing.join(', '));
} else {
// If all the necessary properties are defined, ensure that
// they match the pattern and aren't in the forbidden list.
props
.filter((prop: any) => rules.required[prop.name] || rules.forbidden[prop.name])
.forEach((prop: any) => {
.filter(prop => rules.requiredProps[prop.name] || rules.forbiddenProps[prop.name])
.forEach(prop => {
const {name, value, node} = prop;
const requiredPattern = rules.required[name];
const forbiddenPattern = rules.forbidden[name];
const requiredPattern = rules.requiredProps[name];
const forbiddenPattern = rules.forbiddenProps[name];

if (requiredPattern && !requiredPattern.test(value)) {
this.addFailureAtNode(node, `Invalid value for property. ` +
Expand All @@ -143,33 +187,45 @@ class Walker extends Lint.RuleWalker {
* @param config Config object passed in via the tslint.json.
* @returns Sanitized rules.
*/
private _generateRules(config: {[key: string]: string|{[key: string]: string}}): DecoratorRules {
private _generateRules(config: RuleConfig|null): DecoratorRules {
const output: DecoratorRules = {};

if (config) {
Object.keys(config)
.filter(decoratorName => Object.keys(config[decoratorName]).length > 0)
.forEach(decoratorName => {
const decoratorConfig = config[decoratorName];

if (typeof decoratorConfig === 'string') {
output[decoratorName] = new RegExp(decoratorConfig);
} else {
output[decoratorName] = Object.keys(decoratorConfig).reduce((rules, prop) => {
const isForbidden = prop.startsWith('!');
const cleanName = isForbidden ? prop.slice(1) : prop;
const pattern = new RegExp(decoratorConfig[prop]);

if (isForbidden) {
rules.forbidden[cleanName] = pattern;
} else {
rules.required[cleanName] = pattern;
}

return rules;
}, {required: {}, forbidden: {}} as DecoratorRuleSet);
}
});
Object.keys(config).forEach(decoratorName => {
const decoratorConfig = config[decoratorName];
const {argument, properties, required} = decoratorConfig;

// * is a special token which means to run the pattern across the entire object.
const allProperties = properties[ALL_PROPS_TOKEN];

if (allProperties) {
output[decoratorName] = {
argument,
required: !!required,
requiredProps: {[ALL_PROPS_TOKEN]: new RegExp(allProperties)},
forbiddenProps: {}
};
} else {
output[decoratorName] = Object.keys(decoratorConfig.properties).reduce((rules, prop) => {
const isForbidden = prop.startsWith('!');
const cleanName = isForbidden ? prop.slice(1) : prop;
const pattern = new RegExp(properties[prop]);

if (isForbidden) {
rules.forbiddenProps[cleanName] = pattern;
} else {
rules.requiredProps[cleanName] = pattern;
}

return rules;
}, {
argument,
required: !!required,
requiredProps: {} as {[key: string]: RegExp},
forbiddenProps: {} as {[key: string]: RegExp}
});
}
});
}

return output;
Expand Down
27 changes: 19 additions & 8 deletions tslint.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,17 +133,28 @@
}],
"validate-decorators": [true, {
"Component": {
"!host": "\\[class\\]",
"!preserveWhitespaces": ".*",
"!styles": ".*",
"!moduleId": ".*",
"changeDetection": "\\.OnPush$",
"encapsulation": "\\.None$"
"argument": 0,
"properties": {
"!host": "\\[class\\]",
"!preserveWhitespaces": ".*",
"!styles": ".*",
"!moduleId": ".*",
"changeDetection": "\\.OnPush$",
"encapsulation": "\\.None$"
}
},
"Directive": {
"!host": "\\[class\\]"
"argument": 0,
"properties": {
"!host": "\\[class\\]"
}
},
"NgModule": "^(?!\\s*$).+"
"NgModule": {
"argument": 0,
"properties": {
"*": "^(?!\\s*$).+"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It slightly feels confusing that * is part of properties but matches the actual decorator argument text (where it doesn't necessarily need to be a ts.ObjectLiteralExpression.

I understand why it is done that way, but it feels weird. Since it's just a lint rule though, it should be fine.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I felt weird about it as well, but I wasn't sure how else to express it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could have another property like argumentText, but I don't think it's too important.

}
}
}, "src/!(a11y-demo|e2e-app|components-examples|universal-app|dev-app)/**/!(*.spec).ts"],
"require-license-banner": [
true,
Expand Down