feat(visitor): improve error messages (#20)

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

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',

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<string>): Promise<number> {
   }
 
   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<string>): Promise<number> {
 
   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);

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<any>;
+  definitions?: Dictionary<any>;
   name: string;
   rules: Array<RuleData>;
 }
@@ -55,7 +58,7 @@ export function makeSelector(options: Partial<RuleSelector>) {
   };
 }
 
-export async function loadRules(paths: Array<string>, ajv: any): Promise<Array<Rule>> {
+export async function loadRules(paths: Array<string>, ctx: VisitorContext): Promise<Array<Rule>> {
   const parser = new YamlParser();
   const rules = [];
 
@@ -68,10 +71,7 @@ export async function loadRules(paths: Array<string>, ajv: any): Promise<Array<R
 
     for (const data of docs) {
       if (!isNil(data.definitions)) {
-        ajv.addSchema({
-          '$id': data.name,
-          definitions: data.definitions,
-        });
+        ctx.addSchema(data.name, data.definitions);
       }
 
       rules.push(...data.rules.map((data: any) => new Rule(data)));
@@ -134,7 +134,7 @@ export async function visitRules(ctx: VisitorContext, rules: Array<Rule>, 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<RuleResult> {
   public async visit(ctx: VisitorContext, node: any): Promise<RuleResult> {
     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<VisitorError> = [];
     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<RuleResult> {
     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);
     }
   }
 }

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,
+  };
+}

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<any>;
+  protected readonly _errors: Array<VisitorError>;
+
+  public get changes(): ReadonlyArray<any> {
+    return this._changes;
+  }
+
+  public get errors(): ReadonlyArray<VisitorError> {
+    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<VisitorError>) {
+    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,
+    });
+  }
+}

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;
+}

src/visitor/VisitorResult.ts

@@ -0,0 +1,4 @@
+export interface VisitorResult {
+  changes: ReadonlyArray<any>;
+  errors: ReadonlyArray<any>;
+}

src/visitor/context.ts

@@ -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<any>;
-  public readonly coerce: boolean;
-  public readonly defaults: boolean;
-  public readonly errors: Array<any>;
-  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;
-  }
-}

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<TResult extends VisitorResult> {
   /**

src/visitor/result.ts

@@ -1,4 +0,0 @@
-export interface VisitorResult {
-  changes: Array<any>;
-  errors: Array<any>;
-}

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({

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({