From f0b5109689cf5ccd3c81e7f4b2d5dce03338d3e0 Mon Sep 17 00:00:00 2001 From: ssube Date: Sun, 3 Nov 2019 16:11:25 -0600 Subject: [PATCH] feat(rules): add item index to rule error (fixes #116) --- src/rule/SchemaRule.ts | 19 ++++-- src/rule/index.ts | 9 ++- src/visitor/VisitorContext.ts | 26 +++++++- src/visitor/index.ts | 5 +- test/examples/kubernetes-resources-multi.yml | 27 ++++++++ test/rule/TestRule.ts | 1 + test/rule/TestSchemaRule.ts | 68 +++++++++++++++----- test/visitor/TestContext.ts | 13 +++- 8 files changed, 139 insertions(+), 29 deletions(-) create mode 100644 test/examples/kubernetes-resources-multi.yml diff --git a/src/rule/SchemaRule.ts b/src/rule/SchemaRule.ts index 1baf124..63ad691 100644 --- a/src/rule/SchemaRule.ts +++ b/src/rule/SchemaRule.ts @@ -56,7 +56,7 @@ export class SchemaRule implements Rule, RuleData, Visitor { if (filter(node)) { ctx.logger.debug({ item: node }, 'checking item'); if (!check(node) && hasItems(check.errors)) { - errors.push(...check.errors.map((err) => friendlyError(ctx, err, this))); + errors.push(...check.errors.map((err) => friendlyError(ctx, err))); } } else { ctx.logger.debug({ errors: filter.errors, item: node }, 'skipping item'); @@ -74,21 +74,26 @@ export class SchemaRule implements Rule, RuleData, Visitor { } } -export function friendlyError(ctx: VisitorContext, err: ErrorObject, rule: SchemaRule): VisitorError { +export function friendlyError(ctx: VisitorContext, err: ErrorObject): VisitorError { return { data: { err, - rule, }, level: 'error', - msg: friendlyErrorMessage(err, rule), + msg: friendlyErrorMessage(ctx, err), }; } -export function friendlyErrorMessage(err: ErrorObject, rule: SchemaRule): string { +export function friendlyErrorMessage(ctx: VisitorContext, err: ErrorObject): string { + const msg = [err.dataPath]; if (isNil(err.message)) { - return `${err.dataPath} ${err.keyword} at ${rule.select} for ${rule.name}`; + msg.push(err.keyword); } else { - return `${err.dataPath} ${err.message} at ${rule.select} for ${rule.name}`; + msg.push(err.message); } + msg.push('at', 'item', ctx.visitData.itemIndex.toString()); + msg.push('of', ctx.visitData.rule.select); + msg.push('for', ctx.visitData.rule.name); + + return msg.join(' '); } diff --git a/src/rule/index.ts b/src/rule/index.ts index c60fe3f..7eb4251 100644 --- a/src/rule/index.ts +++ b/src/rule/index.ts @@ -202,13 +202,18 @@ export async function resolveRules(rules: Array, selector: RuleSelector): export async function visitRules(ctx: VisitorContext, rules: Array, data: any): Promise { for (const rule of rules) { const items = await rule.pick(ctx, data); + let itemIndex = 0; for (const item of items) { + ctx.visitData = { + itemIndex, + rule, + }; const itemResult = cloneDeep(item); const ruleResult = await rule.visit(ctx, itemResult); if (hasItems(ruleResult.errors)) { ctx.logger.warn({ count: ruleResult.errors.length, rule }, 'rule failed'); - ctx.mergeResult(ruleResult); + ctx.mergeResult(ruleResult, ctx.visitData); continue; } @@ -226,6 +231,8 @@ export async function visitRules(ctx: VisitorContext, rules: Array, data: } else { ctx.logger.info({ rule: rule.name }, 'rule passed'); } + + itemIndex += 1; } } diff --git a/src/visitor/VisitorContext.ts b/src/visitor/VisitorContext.ts index 4873b61..66b0532 100644 --- a/src/visitor/VisitorContext.ts +++ b/src/visitor/VisitorContext.ts @@ -25,6 +25,7 @@ export class VisitorContext implements VisitorContextOptions, VisitorResult { protected readonly ajv: Ajv.Ajv; protected readonly changeBuffer: Array; protected readonly errorBuffer: Array; + protected data: any; public get changes(): ReadonlyArray { return this.changeBuffer; @@ -64,9 +65,17 @@ export class VisitorContext implements VisitorContextOptions, VisitorResult { return this.ajv.compile(schema); } - public mergeResult(other: VisitorResult): this { + public mergeResult(other: VisitorResult, data: any = {}): this { this.changeBuffer.push(...other.changes); - this.errorBuffer.push(...other.errors); + this.errorBuffer.push(...other.errors.map((err) => { + return { + ...err, + data: { + ...err.data, + ...data, + }, + }; + })); return this; } @@ -87,4 +96,17 @@ export class VisitorContext implements VisitorContextOptions, VisitorResult { return []; } } + + /** + * store some flash data. this is very much not the right way to do it. + * + * @TODO: fix this + */ + public get visitData(): any { + return this.data; + } + + public set visitData(value: any) { + this.data = value; + } } diff --git a/src/visitor/index.ts b/src/visitor/index.ts index ea27d8a..95186c7 100644 --- a/src/visitor/index.ts +++ b/src/visitor/index.ts @@ -1,3 +1,4 @@ +import { Diff } from 'deep-diff'; import { LogLevel } from 'noicejs'; import { VisitorContext } from './VisitorContext'; @@ -12,8 +13,8 @@ export interface VisitorError { } export interface VisitorResult { - changes: ReadonlyArray; - errors: ReadonlyArray; + changes: ReadonlyArray>; + errors: ReadonlyArray; } export interface Visitor { diff --git a/test/examples/kubernetes-resources-multi.yml b/test/examples/kubernetes-resources-multi.yml new file mode 100644 index 0000000..03c2589 --- /dev/null +++ b/test/examples/kubernetes-resources-multi.yml @@ -0,0 +1,27 @@ +# test rules kubernetes +# test tags kubernetes +# test exit-status 1 + +metadata: + name: example + labels: {} +spec: + template: + spec: + containers: + - name: test + resources: + limits: + cpu: 4000m + memory: 5Gi + requests: + cpu: 4000m + memory: 5Gi + + - name: other + resources: + limits: + cpu: 2000m + requests: + cpu: 2000m + diff --git a/test/rule/TestRule.ts b/test/rule/TestRule.ts index 6af47ed..8f8baa8 100644 --- a/test/rule/TestRule.ts +++ b/test/rule/TestRule.ts @@ -227,6 +227,7 @@ describeLeaks('rule visitor', async () => { changes: [], errors: [{ data: {}, + level: 'error', msg: 'kaboom!', }], })); diff --git a/test/rule/TestSchemaRule.ts b/test/rule/TestSchemaRule.ts index a321df3..ec2158f 100644 --- a/test/rule/TestSchemaRule.ts +++ b/test/rule/TestSchemaRule.ts @@ -160,26 +160,64 @@ describeLeaks('schema rule', async () => { describe('friendly errors', () => { it('should have a message', () => { - const err = friendlyError(new VisitorContext({ - innerOptions: { - coerce: false, - defaults: false, - mutate: false, - }, - logger: NullLogger.global, - }), { - dataPath: 'test-path', - keyword: TEST_NAME, - params: { /* ? */ }, - schemaPath: 'test-path', - }, new SchemaRule({ + const rule = new SchemaRule({ check: {}, desc: TEST_NAME, level: 'info', name: TEST_NAME, select: '', tags: [TEST_NAME], - })); - expect(err.msg).to.not.equal(''); + }); + const ctx = new VisitorContext({ + innerOptions: { + coerce: false, + defaults: false, + mutate: false, + }, + logger: NullLogger.global, + }); + ctx.visitData = { + itemIndex: 0, + rule, + }; + const err = friendlyError(ctx, { + dataPath: 'test-path', + keyword: TEST_NAME, + params: { /* ? */ }, + schemaPath: 'test-path', + }); + expect(err.msg).to.include(TEST_NAME); + }); + + it('should handle errors with an existing message', () => { + const TEST_MESSAGE = 'test-message'; + const rule = new SchemaRule({ + check: {}, + desc: TEST_NAME, + level: 'info', + name: TEST_NAME, + select: '', + tags: [TEST_NAME], + }); + const ctx = new VisitorContext({ + innerOptions: { + coerce: false, + defaults: false, + mutate: false, + }, + logger: NullLogger.global, + }); + ctx.visitData = { + itemIndex: 0, + rule, + }; + const err = friendlyError(ctx, { + dataPath: 'test-path', + keyword: TEST_NAME, + message: TEST_MESSAGE, + params: { /* ? */ }, + schemaPath: 'test-path', + }); + expect(err.msg).to.include(TEST_MESSAGE); }); }); diff --git a/test/visitor/TestContext.ts b/test/visitor/TestContext.ts index b3cccab..e36a01f 100644 --- a/test/visitor/TestContext.ts +++ b/test/visitor/TestContext.ts @@ -15,8 +15,17 @@ describe('visitor context', () => { }); const nextCtx = firstCtx.mergeResult({ - changes: [{bar: 3}], - errors: [{foo: 2}], + changes: [{ + kind: 'N', + rhs: {}, + }], + errors: [{ + data: { + foo: 2, + }, + level: 'info', + msg: 'uh oh', + }], }); expect(nextCtx).to.equal(firstCtx);