From c9c1a5840710927b7c455e71e5770ab9e5e17080 Mon Sep 17 00:00:00 2001 From: ssube Date: Tue, 10 Sep 2019 20:08:57 -0500 Subject: [PATCH] feat(visitor): improve error messages (#20) --- Makefile | 2 +- config/rollup.js | 1 + src/index.ts | 14 ++++--- src/rule.ts | 39 ++++++++---------- src/utils/ajv/index.ts | 11 +++++ src/visitor/VisitorContext.ts | 76 +++++++++++++++++++++++++++++++++++ src/visitor/VisitorError.ts | 10 +++++ src/visitor/VisitorResult.ts | 4 ++ src/visitor/context.ts | 45 --------------------- src/visitor/index.ts | 4 +- src/visitor/result.ts | 4 -- test/TestRule.ts | 18 +++++---- test/visitor/TestContext.ts | 10 +++-- 13 files changed, 147 insertions(+), 91 deletions(-) create mode 100644 src/utils/ajv/index.ts create mode 100644 src/visitor/VisitorContext.ts create mode 100644 src/visitor/VisitorError.ts create mode 100644 src/visitor/VisitorResult.ts delete mode 100644 src/visitor/context.ts delete mode 100644 src/visitor/result.ts diff --git a/Makefile b/Makefile index 0ccec16..e903db7 100755 --- a/Makefile +++ b/Makefile @@ -42,7 +42,7 @@ RELEASE_OPTS ?= --commit-all export NODE_VERSION := $(shell node -v) export RUNNER_VERSION := $(CI_RUNNER_VERSION) -all: build +all: build test @echo Success! clean: ## clean up everything added by the default target diff --git a/config/rollup.js b/config/rollup.js index 5a1b298..7655fc9 100644 --- a/config/rollup.js +++ b/config/rollup.js @@ -80,6 +80,7 @@ const bundle = { 'node_modules/noicejs/out/main-bundle.js': [ 'BaseError', 'ConsoleLogger', + 'logWithLevel', ], 'node_modules/js-yaml/index.js': [ 'DEFAULT_SAFE_SCHEMA', diff --git a/src/index.ts b/src/index.ts index de62182..e3f135f 100644 --- a/src/index.ts +++ b/src/index.ts @@ -6,7 +6,7 @@ import { YamlParser } from './parser/YamlParser'; import { loadRules, resolveRules, visitRules } from './rule'; import { loadSource, writeSource } from './source'; import { VERSION_INFO } from './version'; -import { VisitorContext } from './visitor/context'; +import { VisitorContext } from './visitor/VisitorContext'; enum MODES { check = 'check', @@ -34,13 +34,15 @@ export async function main(argv: Array): Promise { } const ctx = new VisitorContext({ - coerce: args.coerce, - defaults: args.defaults, + innerOptions: { + coerce: args.coerce, + defaults: args.defaults, + mutate: mode === 'fix', + }, logger, - mutate: mode === 'fix', }); - const rules = await loadRules(args.rules, ctx.ajv); + const rules = await loadRules(args.rules, ctx); const activeRules = await resolveRules(rules, args as any); if (mode === 'list') { @@ -50,7 +52,7 @@ export async function main(argv: Array): Promise { const parser = new YamlParser(); const source = await loadSource(args.source); - let docs = parser.parse(source); + const docs = parser.parse(source); for (const data of docs) { await visitRules(ctx, activeRules, data); diff --git a/src/rule.ts b/src/rule.ts index f34c44d..924bf9b 100644 --- a/src/rule.ts +++ b/src/rule.ts @@ -1,13 +1,16 @@ +import { ValidateFunction } from 'ajv'; import { applyDiff, diff } from 'deep-diff'; import { JSONPath } from 'jsonpath-plus'; -import { cloneDeep, intersection, isNil } from 'lodash'; +import { cloneDeep, Dictionary, intersection, isNil } from 'lodash'; import { LogLevel } from 'noicejs'; import { YamlParser } from './parser/YamlParser'; import { readFileSync } from './source'; +import { friendlyError } from './utils/ajv'; import { Visitor } from './visitor'; -import { VisitorContext } from './visitor/context'; -import { VisitorResult } from './visitor/result'; +import { VisitorContext } from './visitor/VisitorContext'; +import { VisitorError } from './visitor/VisitorError'; +import { VisitorResult } from './visitor/VisitorResult'; export interface RuleData { // metadata @@ -31,7 +34,7 @@ export interface RuleSelector { } export interface RuleSource { - definitions?: Array; + definitions?: Dictionary; name: string; rules: Array; } @@ -55,7 +58,7 @@ export function makeSelector(options: Partial) { }; } -export async function loadRules(paths: Array, ajv: any): Promise> { +export async function loadRules(paths: Array, ctx: VisitorContext): Promise> { const parser = new YamlParser(); const rules = []; @@ -68,10 +71,7 @@ export async function loadRules(paths: Array, ajv: any): Promise new Rule(data))); @@ -134,7 +134,7 @@ export async function visitRules(ctx: VisitorContext, rules: Array, data: rule: rule.name, }, 'rule passed with modifications'); - if (ctx.mutate) { + if (ctx.innerOptions.mutate) { applyDiff(item, itemCopy); } } else { @@ -191,25 +191,20 @@ export class Rule implements RuleData, Visitor { public async visit(ctx: VisitorContext, node: any): Promise { ctx.logger.debug({ item: node, rule: this }, 'visiting node'); - const check = ctx.ajv.compile(this.check); + const check = ctx.compile(this.check); const filter = this.compileFilter(ctx); + const errors: Array = []; const result: RuleResult = { changes: [], - errors: [], + errors, rule: this, }; if (filter(node)) { ctx.logger.debug({ item: node }, 'checking item'); - if (!check(node)) { + if (!check(node) && check.errors && check.errors.length) { const errors = Array.from(check.errors); - ctx.logger.warn({ - errors, - name: this.name, - item: node, - rule: this, - }, 'rule failed on item'); - result.errors.push(...errors); + ctx.error(...errors.map(friendlyError)); } } else { ctx.logger.debug({ errors: filter.errors, item: node }, 'skipping item'); @@ -218,11 +213,11 @@ export class Rule implements RuleData, Visitor { return result; } - protected compileFilter(ctx: VisitorContext): any { + protected compileFilter(ctx: VisitorContext): ValidateFunction { if (isNil(this.filter)) { return () => true; } else { - return ctx.ajv.compile(this.filter); + return ctx.compile(this.filter); } } } diff --git a/src/utils/ajv/index.ts b/src/utils/ajv/index.ts new file mode 100644 index 0000000..6410a64 --- /dev/null +++ b/src/utils/ajv/index.ts @@ -0,0 +1,11 @@ +import { ErrorObject } from 'ajv'; + +import { VisitorError } from '../../visitor/VisitorError'; + +export function friendlyError(err: ErrorObject): VisitorError { + return { + data: {}, + level: 'error', + msg: err.message || err.keyword, + }; +} diff --git a/src/visitor/VisitorContext.ts b/src/visitor/VisitorContext.ts new file mode 100644 index 0000000..f3a4e31 --- /dev/null +++ b/src/visitor/VisitorContext.ts @@ -0,0 +1,76 @@ +import Ajv from 'ajv'; +import { Logger, logWithLevel } from 'noicejs'; + +import { VisitorError } from './VisitorError'; +import { VisitorResult } from './VisitorResult'; + +export interface RuleOptions { + coerce: boolean; + defaults: boolean; + mutate: boolean; +} + +export interface VisitorContextOptions { + logger: Logger; + innerOptions: RuleOptions; +} + +export class VisitorContext implements VisitorContextOptions, VisitorResult { + public readonly logger: Logger; + public readonly innerOptions: RuleOptions; + + protected readonly ajv: Ajv.Ajv; + protected readonly _changes: Array; + protected readonly _errors: Array; + + public get changes(): ReadonlyArray { + return this._changes; + } + + public get errors(): ReadonlyArray { + return this._errors; + } + + constructor(options: VisitorContextOptions) { + this._changes = []; + this._errors = []; + + this.ajv = new Ajv({ + coerceTypes: options.innerOptions.coerce, + useDefaults: options.innerOptions.defaults, + }); + + this.logger = options.logger; + this.innerOptions = options.innerOptions; + } + + public error(...errors: Array) { + for (const err of errors) { + logWithLevel(this.logger, err.level, err.data, err.msg); + } + + this._errors.push(...errors); + } + + public mergeResult(other: VisitorResult): this { + this._changes.push(...other.changes); + this._errors.push(...other.errors); + return this; + } + + public compile(schema: any): Ajv.ValidateFunction { + return this.ajv.compile(schema); + } + + public addSchema(name: string, schema: any): void { + this.logger.debug('adding ajv schema', { + name, + schema, + }); + + this.ajv.addSchema({ + '$id': name, + definitions: schema, + }); + } +} diff --git a/src/visitor/VisitorError.ts b/src/visitor/VisitorError.ts new file mode 100644 index 0000000..5263083 --- /dev/null +++ b/src/visitor/VisitorError.ts @@ -0,0 +1,10 @@ +import { LogLevel } from 'noicejs'; + +/** + * This is an runtime error, not an exception. + */ +export interface VisitorError { + data: any; + level: LogLevel; + msg: string; +} diff --git a/src/visitor/VisitorResult.ts b/src/visitor/VisitorResult.ts new file mode 100644 index 0000000..d6a429f --- /dev/null +++ b/src/visitor/VisitorResult.ts @@ -0,0 +1,4 @@ +export interface VisitorResult { + changes: ReadonlyArray; + errors: ReadonlyArray; +} diff --git a/src/visitor/context.ts b/src/visitor/context.ts deleted file mode 100644 index af683db..0000000 --- a/src/visitor/context.ts +++ /dev/null @@ -1,45 +0,0 @@ -import * as Ajv from 'ajv'; -import { Logger } from 'noicejs'; - -import { VisitorResult } from './result'; - -export interface VisitorContextOptions { - coerce: boolean; - defaults: boolean; - logger: Logger; - mutate: boolean; -} - -export class VisitorContext implements VisitorContextOptions, VisitorResult { - public readonly ajv: any; - public readonly changes: Array; - public readonly coerce: boolean; - public readonly defaults: boolean; - public readonly errors: Array; - public readonly logger: Logger; - public readonly mutate: boolean; - - constructor(options: VisitorContextOptions) { - this.ajv = new ((Ajv as any).default)({ - coerceTypes: options.coerce, - useDefaults: options.defaults, - }); - this.changes = []; - this.coerce = options.coerce; - this.defaults = options.defaults; - this.errors = []; - this.logger = options.logger; - this.mutate = options.mutate; - } - - public error(options: any, msg: string) { - this.logger.error(options, msg); - this.errors.push(options || msg); - } - - public mergeResult(other: VisitorResult): this { - this.changes.push(...other.changes); - this.errors.push(...other.errors); - return this; - } -} diff --git a/src/visitor/index.ts b/src/visitor/index.ts index 6ede915..2aa55c7 100644 --- a/src/visitor/index.ts +++ b/src/visitor/index.ts @@ -1,5 +1,5 @@ -import { VisitorContext } from './context'; -import { VisitorResult } from './result'; +import { VisitorContext } from './VisitorContext'; +import { VisitorResult } from './VisitorResult'; export interface Visitor { /** diff --git a/src/visitor/result.ts b/src/visitor/result.ts deleted file mode 100644 index 7bb2dce..0000000 --- a/src/visitor/result.ts +++ /dev/null @@ -1,4 +0,0 @@ -export interface VisitorResult { - changes: Array; - errors: Array; -} diff --git a/test/TestRule.ts b/test/TestRule.ts index 788086b..b12f58b 100644 --- a/test/TestRule.ts +++ b/test/TestRule.ts @@ -3,7 +3,7 @@ import { ConsoleLogger } from 'noicejs'; import { mock } from 'sinon'; import { makeSelector, resolveRules, Rule, visitRules } from '../src/rule'; -import { VisitorContext } from '../src/visitor/context'; +import { VisitorContext } from '../src/visitor/VisitorContext'; const TEST_RULES = [new Rule({ name: 'foo', @@ -102,10 +102,12 @@ describe('rule resolver', () => { describe('rule visitor', () => { it('should only call visit for selected items', async () => { const ctx = new VisitorContext({ - coerce: false, - defaults: false, logger: new ConsoleLogger(), - mutate: false, + innerOptions: { + coerce: false, + defaults: false, + mutate: false, + } }); const data = {}; const rule = new Rule({ @@ -132,10 +134,12 @@ describe('rule visitor', () => { it('should call visit for each selected item', async () => { const ctx = new VisitorContext({ - coerce: false, - defaults: false, + innerOptions: { + coerce: false, + defaults: false, + mutate: false, + }, logger: new ConsoleLogger(), - mutate: false, }); const data = {}; const rule = new Rule({ diff --git a/test/visitor/TestContext.ts b/test/visitor/TestContext.ts index 6aed394..b3cccab 100644 --- a/test/visitor/TestContext.ts +++ b/test/visitor/TestContext.ts @@ -1,15 +1,17 @@ import { expect } from 'chai'; import { ConsoleLogger } from 'noicejs'; -import { VisitorContext } from '../../src/visitor/context'; +import { VisitorContext } from '../../src/visitor/VisitorContext'; describe('visitor context', () => { it('should merge results', () => { const firstCtx = new VisitorContext({ - coerce: false, - defaults: false, + innerOptions: { + coerce: false, + defaults: false, + mutate: false, + }, logger: new ConsoleLogger(), - mutate: false, }); const nextCtx = firstCtx.mergeResult({