From cc7cf5c9a192c59715bfd126c25fa9492bb977d7 Mon Sep 17 00:00:00 2001 From: ssube Date: Tue, 19 Nov 2019 08:10:31 -0600 Subject: [PATCH 1/9] feat(visitor): emit rule visit, error, and pass events --- src/rule/RuleVisitor.ts | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/src/rule/RuleVisitor.ts b/src/rule/RuleVisitor.ts index b9c292d4..08c0b04f 100644 --- a/src/rule/RuleVisitor.ts +++ b/src/rule/RuleVisitor.ts @@ -1,4 +1,5 @@ import { applyDiff, diff } from 'deep-diff'; +import { EventEmitter } from 'events'; import { cloneDeep } from 'lodash'; import { Rule } from '.'; @@ -12,10 +13,18 @@ export interface RuleVisitorOptions { rules: ReadonlyArray; } -export class RuleVisitor implements RuleVisitorOptions, Visitor { +export enum RuleVisitorEvents { + RULE_VISIT = 'rule-visit', + RULE_ERROR = 'rule-error', + RULE_PASS = 'rule-pass', +} + +export class RuleVisitor extends EventEmitter implements RuleVisitorOptions, Visitor { public readonly rules: ReadonlyArray; constructor(options: RuleVisitorOptions) { + super(); + this.rules = Array.from(options.rules); } @@ -25,6 +34,10 @@ export class RuleVisitor implements RuleVisitorOptions, Visitor { public async visit(ctx: VisitorContext, root: any): Promise { for (const rule of this.rules) { + this.emit(RuleVisitorEvents.RULE_VISIT, { + rule, + }); + const items = await rule.pick(ctx, root); let itemIndex = 0; for (const item of items) { @@ -48,6 +61,10 @@ export class RuleVisitor implements RuleVisitorOptions, Visitor { if (hasItems(ruleResult.errors)) { ctx.logger.warn({ count: ruleResult.errors.length, rule }, 'rule failed'); ctx.mergeResult(ruleResult, ctx.visitData); + this.emit(RuleVisitorEvents.RULE_ERROR, { + item, + rule, + }); return; } @@ -64,6 +81,10 @@ export class RuleVisitor implements RuleVisitorOptions, Visitor { } } else { ctx.logger.info({ rule: rule.name }, 'rule passed'); + this.emit(RuleVisitorEvents.RULE_PASS, { + item, + rule, + }); } } } From b23bdd9c0f6789c653497fe841cbd4bb8e3a52d2 Mon Sep 17 00:00:00 2001 From: ssube Date: Tue, 19 Nov 2019 08:23:10 -0600 Subject: [PATCH 2/9] feat(visitor): emit item visit, diff, error, and pass events --- src/rule/RuleVisitor.ts | 58 +++++++++++++++++++++++++++++------------ 1 file changed, 42 insertions(+), 16 deletions(-) diff --git a/src/rule/RuleVisitor.ts b/src/rule/RuleVisitor.ts index 08c0b04f..302cfd69 100644 --- a/src/rule/RuleVisitor.ts +++ b/src/rule/RuleVisitor.ts @@ -14,9 +14,13 @@ export interface RuleVisitorOptions { } export enum RuleVisitorEvents { - RULE_VISIT = 'rule-visit', + ITEM_DIFF = 'item-diff', + ITEM_ERROR = 'item-error', + ITEM_PASS = 'item-pass', + ITEM_VISIT = 'item-visit', RULE_ERROR = 'rule-error', RULE_PASS = 'rule-pass', + RULE_VISIT = 'rule-visit', } export class RuleVisitor extends EventEmitter implements RuleVisitorOptions, Visitor { @@ -38,6 +42,8 @@ export class RuleVisitor extends EventEmitter implements RuleVisitorOptions, Vis rule, }); + const errorsBefore = ctx.errors.length; + const items = await rule.pick(ctx, root); let itemIndex = 0; for (const item of items) { @@ -49,42 +55,62 @@ export class RuleVisitor extends EventEmitter implements RuleVisitorOptions, Vis await this.visitItem(ctx, item, itemIndex, rule); itemIndex += 1; } + + if (ctx.errors.length > errorsBefore) { + ctx.logger.info({ rule: rule.name }, 'rule failed'); + this.emit(RuleVisitorEvents.RULE_ERROR, { + rule, + }); + } else { + ctx.logger.info({ rule: rule.name }, 'rule passed'); + this.emit(RuleVisitorEvents.RULE_PASS, { + rule, + }); + } } return ctx; } - public async visitItem(ctx: VisitorContext, item: any, itemIndex: number, rule: Rule): Promise { + public async visitItem(ctx: VisitorContext, item: any, itemIndex: number, rule: Rule): Promise { 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.visitData); - this.emit(RuleVisitorEvents.RULE_ERROR, { + const errorData = { + count: ruleResult.errors.length, item, rule, - }); - return; + }; + ctx.logger.warn(errorData, 'item failed'); + this.emit(RuleVisitorEvents.ITEM_ERROR, errorData); + + ctx.mergeResult(ruleResult, ctx.visitData); + return ctx; } const itemDiff = diff(item, itemResult); if (hasItems(itemDiff)) { - ctx.logger.info({ + const diffData = { diff: itemDiff, item, - rule: rule.name, - }, 'rule passed with modifications'); + rule, + }; + ctx.logger.info(diffData, 'item could pass rule with changes'); + this.emit(RuleVisitorEvents.ITEM_DIFF, diffData); if (ctx.schemaOptions.mutate) { applyDiff(item, itemResult); } - } else { - ctx.logger.info({ rule: rule.name }, 'rule passed'); - this.emit(RuleVisitorEvents.RULE_PASS, { - item, - rule, - }); } + + const passData = { + item, + rule, + }; + ctx.logger.debug(passData, 'item passed'); + this.emit(RuleVisitorEvents.ITEM_PASS, passData); + + return ctx; } } From 8c5eab232726ab2d2c37f31332806f1d19c019eb Mon Sep 17 00:00:00 2001 From: ssube Date: Tue, 19 Nov 2019 08:44:49 -0600 Subject: [PATCH 3/9] fix: type rule visitor flash data (fixes #137) --- src/rule/RuleVisitor.ts | 1 + src/utils/index.ts | 9 +++++++++ src/visitor/VisitorContext.ts | 21 ++++++++++++++------- test/rule/TestSchemaRule.ts | 1 + 4 files changed, 25 insertions(+), 7 deletions(-) diff --git a/src/rule/RuleVisitor.ts b/src/rule/RuleVisitor.ts index 302cfd69..3f48bd94 100644 --- a/src/rule/RuleVisitor.ts +++ b/src/rule/RuleVisitor.ts @@ -48,6 +48,7 @@ export class RuleVisitor extends EventEmitter implements RuleVisitorOptions, Vis let itemIndex = 0; for (const item of items) { ctx.visitData = { + item, itemIndex, rule, }; diff --git a/src/utils/index.ts b/src/utils/index.ts index 4213c030..1bf26a9e 100644 --- a/src/utils/index.ts +++ b/src/utils/index.ts @@ -1,9 +1,18 @@ import { isNil } from 'lodash'; +import { NotFoundError } from '../error/NotFoundError'; + export function doesExist(val: T | null | undefined): val is T { return !isNil(val); } +export function mustExist(val: T | null | undefined): T { + if (isNil(val)) { + throw new NotFoundError(); + } + return val; +} + /** * Test if a value is an array with some items (length > 0). * diff --git a/src/visitor/VisitorContext.ts b/src/visitor/VisitorContext.ts index 2231afb5..9067a525 100644 --- a/src/visitor/VisitorContext.ts +++ b/src/visitor/VisitorContext.ts @@ -3,7 +3,8 @@ import { JSONPath } from 'jsonpath-plus'; import { Logger } from 'noicejs'; import { VisitorError, VisitorResult } from '.'; -import { doesExist, hasItems } from '../utils'; +import { Rule } from '../rule'; +import { doesExist, hasItems, mustExist } from '../utils'; /* eslint-disable @typescript-eslint/no-explicit-any */ @@ -18,6 +19,12 @@ export interface VisitorContextOptions { schemaOptions: RuleOptions; } +export interface VisitorContextFlash { + item: unknown; + itemIndex: number; + rule: Rule; +} + export class VisitorContext implements VisitorContextOptions, VisitorResult { public readonly logger: Logger; public readonly schemaOptions: RuleOptions; @@ -25,7 +32,7 @@ export class VisitorContext implements VisitorContextOptions, VisitorResult { protected readonly ajv: Ajv.Ajv; protected readonly changeBuffer: Array; protected readonly errorBuffer: Array; - protected data: any; + protected data?: VisitorContextFlash; public get changes(): ReadonlyArray { return this.changeBuffer; @@ -96,15 +103,15 @@ export class VisitorContext implements VisitorContextOptions, VisitorResult { } /** - * store some flash data. this is very much not the right way to do it. + * Store some flash data about the item and rule being visited. * - * @TODO: fix this + * TODO: This is not the best way to do it and could use work. */ - public get visitData(): any { - return this.data; + public get visitData(): VisitorContextFlash { + return mustExist(this.data); } - public set visitData(value: any) { + public set visitData(value: VisitorContextFlash) { this.data = value; } } diff --git a/test/rule/TestSchemaRule.ts b/test/rule/TestSchemaRule.ts index bbb566a3..436d7900 100644 --- a/test/rule/TestSchemaRule.ts +++ b/test/rule/TestSchemaRule.ts @@ -201,6 +201,7 @@ function createErrorContext() { }, }); ctx.visitData = { + item: {}, itemIndex: 0, rule, }; From 938ae44bf8579c2bc25bfdf0ad91ec19996fd576 Mon Sep 17 00:00:00 2001 From: ssube Date: Tue, 19 Nov 2019 08:59:20 -0600 Subject: [PATCH 4/9] cover must exist helper --- test/utils/TestNil.ts | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) create mode 100644 test/utils/TestNil.ts diff --git a/test/utils/TestNil.ts b/test/utils/TestNil.ts new file mode 100644 index 00000000..9e0446f1 --- /dev/null +++ b/test/utils/TestNil.ts @@ -0,0 +1,21 @@ +import { expect } from 'chai'; + +import { NotFoundError } from '../../src/error/NotFoundError'; +import { mustExist } from '../../src/utils'; + +describe('nil helpers', () => { + describe('must exist helper', () => { + it('should return set values', () => { + const TEST_NUMBER = 3; + const TEST_STRING = '3'; + expect(mustExist(TEST_NUMBER)).to.equal(TEST_NUMBER); + expect(mustExist(TEST_STRING)).to.equal(TEST_STRING); + }); + + it('should throw on nil values', () => { + /* eslint-disable-next-line no-null/no-null */ + expect(() => mustExist(null)).to.throw(NotFoundError); + expect(() => mustExist(undefined)).to.throw(NotFoundError); + }); + }); +}); From 1ea1cf8b26c2952915d7f98d5a3786d1b150692b Mon Sep 17 00:00:00 2001 From: ssube Date: Wed, 20 Nov 2019 08:06:45 -0600 Subject: [PATCH 5/9] make visitor data and error type explicit --- src/app.ts | 4 +- src/rule/RuleVisitor.ts | 29 ++++++++++++--- src/rule/SchemaRule.ts | 12 +++--- src/rule/index.ts | 20 +++++----- src/visitor/VisitorContext.ts | 15 ++------ src/visitor/index.ts | 16 ++++---- test/helpers/context.ts | 16 ++++++++ test/rule/TestLoadRule.ts | 64 ++++---------------------------- test/rule/TestRule.ts | 24 ++---------- test/rule/TestRuleVisitor.ts | 49 ++++--------------------- test/rule/TestSchemaRule.ts | 69 +++++------------------------------ 11 files changed, 99 insertions(+), 219 deletions(-) create mode 100644 test/helpers/context.ts diff --git a/src/app.ts b/src/app.ts index efbd60e9..9a2d3963 100644 --- a/src/app.ts +++ b/src/app.ts @@ -5,7 +5,7 @@ import { loadConfig } from './config'; import { CONFIG_ARGS_NAME, CONFIG_ARGS_PATH, MODE, parseArgs } from './config/args'; import { YamlParser } from './parser/YamlParser'; import { createRuleSelector, createRuleSources, loadRules, resolveRules, validateConfig } from './rule'; -import { RuleVisitor } from './rule/RuleVisitor'; +import { RuleVisitor, RuleVisitorData, RuleVisitorError } from './rule/RuleVisitor'; import { readSource, writeSource } from './source'; import { VERSION_INFO } from './version'; import { VisitorContext } from './visitor/VisitorContext'; @@ -29,7 +29,7 @@ export async function main(argv: Array): Promise { logger.info({ args, mode }, 'main arguments'); // load rules - const ctx = new VisitorContext({ + const ctx = new VisitorContext({ logger, schemaOptions: { coerce: args.coerce, diff --git a/src/rule/RuleVisitor.ts b/src/rule/RuleVisitor.ts index 3f48bd94..be414b07 100644 --- a/src/rule/RuleVisitor.ts +++ b/src/rule/RuleVisitor.ts @@ -4,11 +4,23 @@ import { cloneDeep } from 'lodash'; import { Rule } from '.'; import { hasItems } from '../utils'; -import { Visitor } from '../visitor'; +import { Visitor, VisitorResult } from '../visitor'; import { VisitorContext } from '../visitor/VisitorContext'; /* eslint-disable @typescript-eslint/no-explicit-any */ +export interface RuleVisitorData { + item: unknown; + itemIndex: number; + rule: Rule; + ruleIndex: number; +} + +export interface RuleVisitorError extends RuleVisitorData { + key: string; + path: string; +} + export interface RuleVisitorOptions { rules: ReadonlyArray; } @@ -23,7 +35,10 @@ export enum RuleVisitorEvents { RULE_VISIT = 'rule-visit', } -export class RuleVisitor extends EventEmitter implements RuleVisitorOptions, Visitor { +export type RuleVisitorContext = VisitorContext; +export type RuleVisitorResult = VisitorResult; + +export class RuleVisitor extends EventEmitter implements RuleVisitorOptions, Visitor { public readonly rules: ReadonlyArray; constructor(options: RuleVisitorOptions) { @@ -32,11 +47,12 @@ export class RuleVisitor extends EventEmitter implements RuleVisitorOptions, Vis this.rules = Array.from(options.rules); } - public async pick(ctx: VisitorContext, root: any): Promise> { + public async pick(ctx: RuleVisitorContext, root: any): Promise> { return []; // TODO: why is this part of visitor rather than rule? } - public async visit(ctx: VisitorContext, root: any): Promise { + public async visit(ctx: RuleVisitorContext, root: any): Promise { + let ruleIndex = 0; for (const rule of this.rules) { this.emit(RuleVisitorEvents.RULE_VISIT, { rule, @@ -51,6 +67,7 @@ export class RuleVisitor extends EventEmitter implements RuleVisitorOptions, Vis item, itemIndex, rule, + ruleIndex, }; await this.visitItem(ctx, item, itemIndex, rule); @@ -68,12 +85,14 @@ export class RuleVisitor extends EventEmitter implements RuleVisitorOptions, Vis rule, }); } + + ruleIndex += 1; } return ctx; } - public async visitItem(ctx: VisitorContext, item: any, itemIndex: number, rule: Rule): Promise { + public async visitItem(ctx: RuleVisitorContext, item: any, itemIndex: number, rule: Rule): Promise { const itemResult = cloneDeep(item); const ruleResult = await rule.visit(ctx, itemResult); diff --git a/src/rule/SchemaRule.ts b/src/rule/SchemaRule.ts index 8ae52492..9c60af1e 100644 --- a/src/rule/SchemaRule.ts +++ b/src/rule/SchemaRule.ts @@ -5,7 +5,7 @@ import { LogLevel } from 'noicejs'; import { Rule, RuleData } from '.'; import { doesExist, hasItems } from '../utils'; import { Visitor, VisitorError, VisitorResult } from '../visitor'; -import { VisitorContext } from '../visitor/VisitorContext'; +import { RuleVisitorContext } from './RuleVisitor'; /* eslint-disable @typescript-eslint/no-explicit-any, @typescript-eslint/strict-boolean-expressions */ @@ -34,7 +34,7 @@ export class SchemaRule implements Rule, RuleData, Visitor { } } - public async pick(ctx: VisitorContext, root: any): Promise> { + public async pick(ctx: RuleVisitorContext, root: any): Promise> { const items = ctx.pick(this.select, root); if (items.length === 0) { @@ -44,7 +44,7 @@ export class SchemaRule implements Rule, RuleData, Visitor { return items; } - public async visit(ctx: VisitorContext, node: any): Promise { + public async visit(ctx: RuleVisitorContext, node: any): Promise { ctx.logger.debug({ item: node, rule: this }, 'visiting node'); const check = ctx.compile(this.check); @@ -67,7 +67,7 @@ export class SchemaRule implements Rule, RuleData, Visitor { return result; } - protected compileFilter(ctx: VisitorContext): ValidateFunction { + protected compileFilter(ctx: RuleVisitorContext): ValidateFunction { if (isNil(this.filter)) { return DEFAULT_FILTER; } else { @@ -76,7 +76,7 @@ export class SchemaRule implements Rule, RuleData, Visitor { } } -export function friendlyError(ctx: VisitorContext, err: ErrorObject): VisitorError { +export function friendlyError(ctx: RuleVisitorContext, err: ErrorObject): VisitorError { return { data: { err, @@ -86,7 +86,7 @@ export function friendlyError(ctx: VisitorContext, err: ErrorObject): VisitorErr }; } -export function friendlyErrorMessage(ctx: VisitorContext, err: ErrorObject): string { +export function friendlyErrorMessage(ctx: RuleVisitorContext, err: ErrorObject): string { const msg = [err.dataPath]; if (isNil(err.message)) { msg.push(err.keyword); diff --git a/src/rule/index.ts b/src/rule/index.ts index f64d913b..de7eea80 100644 --- a/src/rule/index.ts +++ b/src/rule/index.ts @@ -10,6 +10,7 @@ import { readFile } from '../source'; import { doesExist, ensureArray } from '../utils'; import { VisitorResult } from '../visitor'; import { VisitorContext } from '../visitor/VisitorContext'; +import { RuleVisitorContext } from './RuleVisitor'; import { SchemaRule } from './SchemaRule'; /* eslint-disable @typescript-eslint/no-explicit-any */ @@ -27,6 +28,7 @@ export interface RuleData { /* tslint:enable:no-any */ export type Validator = ValidateFunction; + export interface Rule { check: Validator; desc?: string; @@ -36,8 +38,8 @@ export interface Rule { select: string; tags: Array; - pick(ctx: VisitorContext, root: any): Promise>; - visit(ctx: VisitorContext, item: any): Promise; + pick(ctx: RuleVisitorContext, root: any): Promise>; + visit(ctx: RuleVisitorContext, item: any): Promise; } /** @@ -100,7 +102,7 @@ export function isPOJSO(val: any): val is RuleData { return Reflect.getPrototypeOf(val) === Reflect.getPrototypeOf({}); } -export async function loadRuleSource(data: RuleSourceModule, ctx: VisitorContext): Promise> { +export async function loadRuleSource(data: RuleSourceModule, ctx: RuleVisitorContext): Promise> { if (doesExist(data.definitions)) { ctx.addSchema(data.name, data.definitions); } @@ -114,7 +116,7 @@ export async function loadRuleSource(data: RuleSourceModule, ctx: VisitorContext }); } -export async function loadRuleFiles(paths: Array, ctx: VisitorContext): Promise> { +export async function loadRuleFiles(paths: Array, ctx: RuleVisitorContext): Promise> { const parser = new YamlParser(); const rules = []; @@ -141,7 +143,7 @@ export async function loadRuleFiles(paths: Array, ctx: VisitorContext): return rules; } -export async function loadRulePaths(paths: Array, ctx: VisitorContext): Promise> { +export async function loadRulePaths(paths: Array, ctx: RuleVisitorContext): Promise> { const match = new Minimatch('**/*.+(json|yaml|yml)', { nocase: true, }); @@ -163,7 +165,7 @@ export async function loadRulePaths(paths: Array, ctx: VisitorContext): return rules; } -export async function loadRuleModules(modules: Array, ctx: VisitorContext, r = require): Promise> { +export async function loadRuleModules(modules: Array, ctx: RuleVisitorContext, r = require): Promise> { const rules = []; for (const name of modules) { @@ -186,7 +188,7 @@ export async function loadRuleModules(modules: Array, ctx: VisitorContex return rules; } -export async function loadRules(sources: RuleSources, ctx: VisitorContext): Promise> { +export async function loadRules(sources: RuleSources, ctx: RuleVisitorContext): Promise> { return [ ...await loadRuleFiles(sources.ruleFile, ctx), ...await loadRulePaths(sources.rulePath, ctx), @@ -218,7 +220,7 @@ export async function resolveRules(rules: Array, selector: RuleSelector): return Array.from(activeRules); } -export function validateRules(ctx: VisitorContext, root: any): boolean { +export function validateRules(ctx: RuleVisitorContext, root: any): boolean { const { definitions, name } = ruleSchemaData as any; const validCtx = new VisitorContext(ctx); @@ -233,7 +235,7 @@ export function validateRules(ctx: VisitorContext, root: any): boolean { } } -export function validateConfig(ctx: VisitorContext, root: any): boolean { +export function validateConfig(ctx: RuleVisitorContext, root: any): boolean { const { definitions, name } = ruleSchemaData as any; const validCtx = new VisitorContext(ctx); diff --git a/src/visitor/VisitorContext.ts b/src/visitor/VisitorContext.ts index 9067a525..79e39345 100644 --- a/src/visitor/VisitorContext.ts +++ b/src/visitor/VisitorContext.ts @@ -3,7 +3,6 @@ import { JSONPath } from 'jsonpath-plus'; import { Logger } from 'noicejs'; import { VisitorError, VisitorResult } from '.'; -import { Rule } from '../rule'; import { doesExist, hasItems, mustExist } from '../utils'; /* eslint-disable @typescript-eslint/no-explicit-any */ @@ -19,20 +18,14 @@ export interface VisitorContextOptions { schemaOptions: RuleOptions; } -export interface VisitorContextFlash { - item: unknown; - itemIndex: number; - rule: Rule; -} - -export class VisitorContext implements VisitorContextOptions, VisitorResult { +export class VisitorContext implements VisitorContextOptions, VisitorResult { public readonly logger: Logger; public readonly schemaOptions: RuleOptions; protected readonly ajv: Ajv.Ajv; protected readonly changeBuffer: Array; protected readonly errorBuffer: Array; - protected data?: VisitorContextFlash; + protected data?: TData; public get changes(): ReadonlyArray { return this.changeBuffer; @@ -107,11 +100,11 @@ export class VisitorContext implements VisitorContextOptions, VisitorResult { * * TODO: This is not the best way to do it and could use work. */ - public get visitData(): VisitorContextFlash { + public get visitData(): TData { return mustExist(this.data); } - public set visitData(value: VisitorContextFlash) { + public set visitData(value: TData) { this.data = value; } } diff --git a/src/visitor/index.ts b/src/visitor/index.ts index 95186c71..3742cbdd 100644 --- a/src/visitor/index.ts +++ b/src/visitor/index.ts @@ -6,25 +6,25 @@ import { VisitorContext } from './VisitorContext'; /** * This is a runtime error, not an exception. */ -export interface VisitorError { - data: any; +export interface VisitorError { + data: TData; level: LogLevel; msg: string; } -export interface VisitorResult { - changes: ReadonlyArray>; - errors: ReadonlyArray; +export interface VisitorResult { + changes: ReadonlyArray>; + errors: ReadonlyArray>; } -export interface Visitor { +export interface Visitor> { /** * Select nodes eligible to be visited. **/ - pick(ctx: VisitorContext, root: any): Promise>; + pick(ctx: VisitorContext, root: any): Promise>; /** * Visit a node. */ - visit(ctx: VisitorContext, node: any): Promise; + visit(ctx: VisitorContext, node: any): Promise; } diff --git a/test/helpers/context.ts b/test/helpers/context.ts new file mode 100644 index 00000000..b5f6c3c2 --- /dev/null +++ b/test/helpers/context.ts @@ -0,0 +1,16 @@ +import { NullLogger } from 'noicejs'; + +import { RuleVisitorData, RuleVisitorError } from '../../src/rule/RuleVisitor'; +import { VisitorContext } from '../../src/visitor/VisitorContext'; + +export function testContext() { + return new VisitorContext({ + logger: NullLogger.global, + schemaOptions: { + coerce: false, + defaults: false, + mutate: false, + }, + }); +} + diff --git a/test/rule/TestLoadRule.ts b/test/rule/TestLoadRule.ts index 669a6c4d..c913e9ff 100644 --- a/test/rule/TestLoadRule.ts +++ b/test/rule/TestLoadRule.ts @@ -1,12 +1,12 @@ import { expect } from 'chai'; import mockFS from 'mock-fs'; -import { LogLevel, NullLogger } from 'noicejs'; +import { LogLevel } from 'noicejs'; import { spy, stub } from 'sinon'; import { loadRuleFiles, loadRuleModules, loadRulePaths, loadRuleSource } from '../../src/rule'; import { SchemaRule } from '../../src/rule/SchemaRule'; -import { VisitorContext } from '../../src/visitor/VisitorContext'; import { describeLeaks, itLeaks } from '../helpers/async'; +import { testContext } from '../helpers/context'; const EXAMPLE_EMPTY = '{name: foo, definitions: {}, rules: []}'; const EXAMPLE_RULES = `{ @@ -21,31 +21,13 @@ const EXAMPLE_RULES = `{ }] }`; -function testContext() { - return new VisitorContext({ - logger: NullLogger.global, - schemaOptions: { - coerce: false, - defaults: false, - mutate: false, - }, - }); -} - describeLeaks('load rule file helper', async () => { itLeaks('should add schema', async () => { mockFS({ test: EXAMPLE_EMPTY, }); - const ctx = new VisitorContext({ - logger: NullLogger.global, - schemaOptions: { - coerce: false, - defaults: false, - mutate: false, - }, - }); + const ctx = testContext(); const schemaSpy = spy(ctx, 'addSchema'); const rules = await loadRuleFiles([ @@ -63,15 +45,7 @@ describeLeaks('load rule file helper', async () => { test: EXAMPLE_RULES, }); - const ctx = new VisitorContext({ - logger: NullLogger.global, - schemaOptions: { - coerce: false, - defaults: false, - mutate: false, - }, - }); - + const ctx = testContext(); const rules = await loadRuleFiles([ 'test', ], ctx); @@ -90,15 +64,7 @@ describeLeaks('load rule file helper', async () => { }` }); - const ctx = new VisitorContext({ - logger: NullLogger.global, - schemaOptions: { - coerce: false, - defaults: false, - mutate: false, - }, - }); - + const ctx = testContext(); const rules = await loadRuleFiles([ 'test', ], ctx); @@ -118,15 +84,7 @@ describeLeaks('load rule path helper', async () => { }, }); - const ctx = new VisitorContext({ - logger: NullLogger.global, - schemaOptions: { - coerce: false, - defaults: false, - mutate: false, - }, - }); - + const ctx = testContext(); const rules = await loadRulePaths([ 'test', ], ctx); @@ -149,15 +107,7 @@ describeLeaks('load rule path helper', async () => { }, }); - const ctx = new VisitorContext({ - logger: NullLogger.global, - schemaOptions: { - coerce: false, - defaults: false, - mutate: false, - }, - }); - + const ctx = testContext(); const rules = await loadRulePaths([ 'test', ], ctx); diff --git a/test/rule/TestRule.ts b/test/rule/TestRule.ts index 143ef5ef..96766085 100644 --- a/test/rule/TestRule.ts +++ b/test/rule/TestRule.ts @@ -1,10 +1,10 @@ import { expect } from 'chai'; -import { ConsoleLogger, LogLevel, NullLogger } from 'noicejs'; +import { LogLevel } from 'noicejs'; import { createRuleSelector, createRuleSources, resolveRules, validateRules } from '../../src/rule'; import { SchemaRule } from '../../src/rule/SchemaRule'; -import { VisitorContext } from '../../src/visitor/VisitorContext'; import { describeLeaks, itLeaks } from '../helpers/async'; +import { testContext } from '../helpers/context'; const TEST_RULES = [new SchemaRule({ check: {}, @@ -143,15 +143,7 @@ describe('create rule selector helper', () => { describeLeaks('validate rule helper', async () => { itLeaks('should accept valid modules', async () => { - const ctx = new VisitorContext({ - logger: ConsoleLogger.global, - schemaOptions: { - coerce: false, - defaults: false, - mutate: false, - }, - }); - + const ctx = testContext(); expect(validateRules(ctx, { name: 'test', rules: [], @@ -159,15 +151,7 @@ describeLeaks('validate rule helper', async () => { }); itLeaks('should reject partial modules', async () => { - const ctx = new VisitorContext({ - logger: NullLogger.global, - schemaOptions: { - coerce: false, - defaults: false, - mutate: false, - }, - }); - + const ctx = testContext(); expect(validateRules(ctx, {})).to.equal(false); expect(validateRules(ctx, { name: '', diff --git a/test/rule/TestRuleVisitor.ts b/test/rule/TestRuleVisitor.ts index 4f0c5577..489da584 100644 --- a/test/rule/TestRuleVisitor.ts +++ b/test/rule/TestRuleVisitor.ts @@ -1,22 +1,15 @@ import { expect } from 'chai'; -import { LogLevel, NullLogger } from 'noicejs'; +import { LogLevel } from 'noicejs'; import { mock, spy, stub } from 'sinon'; import { RuleVisitor } from '../../src/rule/RuleVisitor'; import { SchemaRule } from '../../src/rule/SchemaRule'; -import { VisitorContext } from '../../src/visitor/VisitorContext'; import { describeLeaks, itLeaks } from '../helpers/async'; +import { testContext } from '../helpers/context'; describeLeaks('rule visitor', async () => { itLeaks('should only call visit for selected items', async () => { - const ctx = new VisitorContext({ - logger: NullLogger.global, - schemaOptions: { - coerce: false, - defaults: false, - mutate: false, - }, - }); + const ctx = testContext(); const data = {}; const rule = new SchemaRule({ check: {}, @@ -44,14 +37,7 @@ describeLeaks('rule visitor', async () => { }); itLeaks('should call visit for each selected item', async () => { - const ctx = new VisitorContext({ - logger: NullLogger.global, - schemaOptions: { - coerce: false, - defaults: false, - mutate: false, - }, - }); + const ctx = testContext(); const data = {}; const rule = new SchemaRule({ check: {}, @@ -82,14 +68,7 @@ describeLeaks('rule visitor', async () => { }); itLeaks('should visit individual items', async () => { - const ctx = new VisitorContext({ - logger: NullLogger.global, - schemaOptions: { - coerce: false, - defaults: false, - mutate: false, - }, - }); + const ctx = testContext(); const data = { foo: [Math.random(), Math.random(), Math.random()], }; @@ -118,14 +97,7 @@ describeLeaks('rule visitor', async () => { }); itLeaks('should visit individual items', async () => { - const ctx = new VisitorContext({ - logger: NullLogger.global, - schemaOptions: { - coerce: false, - defaults: false, - mutate: false, - }, - }); + const ctx = testContext(); const data = { foo: [Math.random(), Math.random(), Math.random()], }; @@ -158,14 +130,7 @@ describeLeaks('rule visitor', async () => { }); itLeaks('should not pick items', async () => { - const ctx = new VisitorContext({ - logger: NullLogger.global, - schemaOptions: { - coerce: false, - defaults: false, - mutate: false, - }, - }); + const ctx = testContext(); const visitor = new RuleVisitor({ rules: [], }); diff --git a/test/rule/TestSchemaRule.ts b/test/rule/TestSchemaRule.ts index 436d7900..600ebcf5 100644 --- a/test/rule/TestSchemaRule.ts +++ b/test/rule/TestSchemaRule.ts @@ -1,10 +1,10 @@ import { expect } from 'chai'; -import { LogLevel, NullLogger } from 'noicejs'; +import { LogLevel } from 'noicejs'; import { stub } from 'sinon'; import { friendlyError, SchemaRule } from '../../src/rule/SchemaRule'; -import { VisitorContext } from '../../src/visitor/VisitorContext'; import { describeLeaks, itLeaks } from '../helpers/async'; +import { testContext } from '../helpers/context'; /* eslint-disable @typescript-eslint/unbound-method */ @@ -12,14 +12,7 @@ const TEST_NAME = 'test-rule'; describeLeaks('schema rule', async () => { itLeaks('should pick items from the scope', async () => { - const ctx = new VisitorContext({ - logger: NullLogger.global, - schemaOptions: { - coerce: false, - defaults: false, - mutate: false, - }, - }); + const ctx = testContext(); const data = { foo: 3, }; @@ -37,14 +30,7 @@ describeLeaks('schema rule', async () => { }); itLeaks('should pick no items', async () => { - const ctx = new VisitorContext({ - logger: NullLogger.global, - schemaOptions: { - coerce: false, - defaults: false, - mutate: false, - }, - }); + const ctx = testContext(); const data = { bar: 3, }; @@ -62,15 +48,7 @@ describeLeaks('schema rule', async () => { }); itLeaks('should filter out items', async () => { - const ctx = new VisitorContext({ - logger: NullLogger.global, - schemaOptions: { - coerce: false, - defaults: false, - mutate: false, - }, - }); - + const ctx = testContext(); const data = { foo: 3, }; @@ -96,14 +74,7 @@ describeLeaks('schema rule', async () => { }); itLeaks('should pick items from the root', async () => { - const ctx = new VisitorContext({ - logger: NullLogger.global, - schemaOptions: { - coerce: false, - defaults: false, - mutate: false, - }, - }); + const ctx = testContext(); const rule = new SchemaRule({ check: undefined, desc: TEST_NAME, @@ -119,14 +90,7 @@ describeLeaks('schema rule', async () => { }); itLeaks('should visit selected items', async () => { - const ctx = new VisitorContext({ - logger: NullLogger.global, - schemaOptions: { - coerce: false, - defaults: false, - mutate: false, - }, - }); + const ctx = testContext(); const check = {}; const checkSpy = stub().returns(true); @@ -152,14 +116,7 @@ describeLeaks('schema rule', async () => { }); itLeaks('should skip filtered items', async () => { - const ctx = new VisitorContext({ - logger: NullLogger.global, - schemaOptions: { - coerce: false, - defaults: false, - mutate: false, - }, - }); + const ctx = testContext(); const checkSpy = stub().throws(new Error('check spy error')); const filterSpy = stub().returns(false); @@ -192,18 +149,12 @@ function createErrorContext() { select: '', tags: [TEST_NAME], }); - const ctx = new VisitorContext({ - logger: NullLogger.global, - schemaOptions: { - coerce: false, - defaults: false, - mutate: false, - }, - }); + const ctx = testContext(); ctx.visitData = { item: {}, itemIndex: 0, rule, + ruleIndex: 0, }; return { ctx, rule }; From c85fa93645fab93fda431ee202b8c1e74d7d949b Mon Sep 17 00:00:00 2001 From: ssube Date: Wed, 20 Nov 2019 08:14:44 -0600 Subject: [PATCH 6/9] lint validation schemas --- src/rule/index.ts | 25 ++++++++++++++----------- src/visitor/VisitorContext.ts | 2 -- test/visitor/TestContext.ts | 14 +++----------- 3 files changed, 17 insertions(+), 24 deletions(-) diff --git a/src/rule/index.ts b/src/rule/index.ts index de7eea80..ffe708b4 100644 --- a/src/rule/index.ts +++ b/src/rule/index.ts @@ -220,32 +220,35 @@ export async function resolveRules(rules: Array, selector: RuleSelector): return Array.from(activeRules); } -export function validateRules(ctx: RuleVisitorContext, root: any): boolean { +export function compileValidators(ctx: RuleVisitorContext) { const { definitions, name } = ruleSchemaData as any; const validCtx = new VisitorContext(ctx); validCtx.addSchema(name, definitions); - const ruleSchema = validCtx.compile(definitions.source); + const configSchema = validCtx.compile(definitions.config); + const sourceSchema = validCtx.compile(definitions.source); + + return { configSchema, sourceSchema }; +} + +export function validateRules(ctx: RuleVisitorContext, root: any): boolean { + const { sourceSchema } = compileValidators(ctx); - if (ruleSchema(root) === true) { + if (sourceSchema(root) === true) { return true; } else { - ctx.logger.error({ errors: ruleSchema.errors }, 'error validating rules'); + ctx.logger.error({ errors: sourceSchema.errors }, 'error validating rules'); return false; } } export function validateConfig(ctx: RuleVisitorContext, root: any): boolean { - const { definitions, name } = ruleSchemaData as any; - - const validCtx = new VisitorContext(ctx); - validCtx.addSchema(name, definitions); - const ruleSchema = validCtx.compile(definitions.config); + const { configSchema } = compileValidators(ctx); - if (ruleSchema(root) === true) { + if (configSchema(root) === true) { return true; } else { - ctx.logger.error({ errors: ruleSchema.errors }, 'error validating rules'); + ctx.logger.error({ errors: configSchema.errors }, 'error validating config'); return false; } } diff --git a/src/visitor/VisitorContext.ts b/src/visitor/VisitorContext.ts index 79e39345..03049ef1 100644 --- a/src/visitor/VisitorContext.ts +++ b/src/visitor/VisitorContext.ts @@ -97,8 +97,6 @@ export class VisitorContext implements VisitorConte /** * Store some flash data about the item and rule being visited. - * - * TODO: This is not the best way to do it and could use work. */ public get visitData(): TData { return mustExist(this.data); diff --git a/test/visitor/TestContext.ts b/test/visitor/TestContext.ts index a30adf34..4f1ff0c8 100644 --- a/test/visitor/TestContext.ts +++ b/test/visitor/TestContext.ts @@ -1,19 +1,11 @@ import { expect } from 'chai'; -import { LogLevel, NullLogger } from 'noicejs'; +import { LogLevel } from 'noicejs'; -import { VisitorContext } from '../../src/visitor/VisitorContext'; +import { testContext } from '../helpers/context'; describe('visitor context', () => { it('should merge results', () => { - const firstCtx = new VisitorContext({ - logger: NullLogger.global, - schemaOptions: { - coerce: false, - defaults: false, - mutate: false, - }, - }); - + const firstCtx = testContext(); const nextCtx = firstCtx.mergeResult({ changes: [{ kind: 'N', From 8eaf9a6e57a0f9bf96ff687c1e94e43cc5d42ab7 Mon Sep 17 00:00:00 2001 From: ssube Date: Wed, 20 Nov 2019 08:34:55 -0600 Subject: [PATCH 7/9] merge rule results after each item --- src/rule/RuleVisitor.ts | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/src/rule/RuleVisitor.ts b/src/rule/RuleVisitor.ts index be414b07..ee835cba 100644 --- a/src/rule/RuleVisitor.ts +++ b/src/rule/RuleVisitor.ts @@ -58,7 +58,7 @@ export class RuleVisitor extends EventEmitter implements RuleVisitorOptions, Vis rule, }); - const errorsBefore = ctx.errors.length; + let ruleErrors = 0; const items = await rule.pick(ctx, root); let itemIndex = 0; @@ -70,12 +70,15 @@ export class RuleVisitor extends EventEmitter implements RuleVisitorOptions, Vis ruleIndex, }; - await this.visitItem(ctx, item, itemIndex, rule); + const result = await this.visitItem(ctx, item, itemIndex, rule); + ctx.mergeResult(result); + + ruleErrors += result.errors.length; itemIndex += 1; } - if (ctx.errors.length > errorsBefore) { - ctx.logger.info({ rule: rule.name }, 'rule failed'); + if (ruleErrors > 0) { + ctx.logger.warn({ rule: rule.name }, 'rule failed'); this.emit(RuleVisitorEvents.RULE_ERROR, { rule, }); @@ -105,8 +108,7 @@ export class RuleVisitor extends EventEmitter implements RuleVisitorOptions, Vis ctx.logger.warn(errorData, 'item failed'); this.emit(RuleVisitorEvents.ITEM_ERROR, errorData); - ctx.mergeResult(ruleResult, ctx.visitData); - return ctx; + return ruleResult; } const itemDiff = diff(item, itemResult); @@ -131,6 +133,6 @@ export class RuleVisitor extends EventEmitter implements RuleVisitorOptions, Vis ctx.logger.debug(passData, 'item passed'); this.emit(RuleVisitorEvents.ITEM_PASS, passData); - return ctx; + return ruleResult; } } From 405948faf7bba47613d40a0158a3a224dcdd7037 Mon Sep 17 00:00:00 2001 From: ssube Date: Fri, 22 Nov 2019 08:59:24 -0600 Subject: [PATCH 8/9] lint: isolate rule visitor diff handling --- src/rule/RuleVisitor.ts | 70 +++++++++++++++++++---------------------- 1 file changed, 32 insertions(+), 38 deletions(-) diff --git a/src/rule/RuleVisitor.ts b/src/rule/RuleVisitor.ts index ee835cba..c0e4a435 100644 --- a/src/rule/RuleVisitor.ts +++ b/src/rule/RuleVisitor.ts @@ -1,4 +1,4 @@ -import { applyDiff, diff } from 'deep-diff'; +import { applyDiff, Diff, diff } from 'deep-diff'; import { EventEmitter } from 'events'; import { cloneDeep } from 'lodash'; @@ -54,23 +54,21 @@ export class RuleVisitor extends EventEmitter implements RuleVisitorOptions, Vis public async visit(ctx: RuleVisitorContext, root: any): Promise { let ruleIndex = 0; for (const rule of this.rules) { - this.emit(RuleVisitorEvents.RULE_VISIT, { + const ruleData = { rule, - }); + }; + this.emit(RuleVisitorEvents.RULE_VISIT, ruleData); + let itemIndex = 0; let ruleErrors = 0; - const items = await rule.pick(ctx, root); - let itemIndex = 0; for (const item of items) { - ctx.visitData = { + const result = await this.visitItem(ctx, { item, itemIndex, rule, ruleIndex, - }; - - const result = await this.visitItem(ctx, item, itemIndex, rule); + }); ctx.mergeResult(result); ruleErrors += result.errors.length; @@ -79,14 +77,10 @@ export class RuleVisitor extends EventEmitter implements RuleVisitorOptions, Vis if (ruleErrors > 0) { ctx.logger.warn({ rule: rule.name }, 'rule failed'); - this.emit(RuleVisitorEvents.RULE_ERROR, { - rule, - }); + this.emit(RuleVisitorEvents.RULE_ERROR, ruleData); } else { ctx.logger.info({ rule: rule.name }, 'rule passed'); - this.emit(RuleVisitorEvents.RULE_PASS, { - rule, - }); + this.emit(RuleVisitorEvents.RULE_PASS, ruleData); } ruleIndex += 1; @@ -95,15 +89,16 @@ export class RuleVisitor extends EventEmitter implements RuleVisitorOptions, Vis return ctx; } - public async visitItem(ctx: RuleVisitorContext, item: any, itemIndex: number, rule: Rule): Promise { - const itemResult = cloneDeep(item); - const ruleResult = await rule.visit(ctx, itemResult); + public async visitItem(ctx: RuleVisitorContext, data: RuleVisitorData): Promise { + ctx.visitData = data; + + const itemResult = cloneDeep(data.item); + const ruleResult = await data.rule.visit(ctx, itemResult); if (hasItems(ruleResult.errors)) { const errorData = { + ...data, count: ruleResult.errors.length, - item, - rule, }; ctx.logger.warn(errorData, 'item failed'); this.emit(RuleVisitorEvents.ITEM_ERROR, errorData); @@ -111,28 +106,27 @@ export class RuleVisitor extends EventEmitter implements RuleVisitorOptions, Vis return ruleResult; } - const itemDiff = diff(item, itemResult); + const itemDiff = diff(data.item, itemResult); if (hasItems(itemDiff)) { - const diffData = { - diff: itemDiff, - item, - rule, - }; - ctx.logger.info(diffData, 'item could pass rule with changes'); - this.emit(RuleVisitorEvents.ITEM_DIFF, diffData); - - if (ctx.schemaOptions.mutate) { - applyDiff(item, itemResult); - } + await this.visitDiff(ctx, data, itemDiff, itemResult); } - const passData = { - item, - rule, - }; - ctx.logger.debug(passData, 'item passed'); - this.emit(RuleVisitorEvents.ITEM_PASS, passData); + ctx.logger.debug(data, 'item passed'); + this.emit(RuleVisitorEvents.ITEM_PASS, data); return ruleResult; } + + public async visitDiff(ctx: RuleVisitorContext, data: RuleVisitorData, itemDiff: Array>, result: any): Promise { + const diffData = { + ...data, + diff: itemDiff, + }; + ctx.logger.info(diffData, 'item could pass rule with changes'); + this.emit(RuleVisitorEvents.ITEM_DIFF, diffData); + + if (ctx.schemaOptions.mutate) { + applyDiff(data.item, result); + } + } } From 78d11d947ddbdb5cbc1c3253e5d509c4ac8f8cce Mon Sep 17 00:00:00 2001 From: ssube Date: Wed, 11 Dec 2019 06:58:44 -0600 Subject: [PATCH 9/9] lint: qualify schema options within rule --- src/visitor/VisitorContext.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/visitor/VisitorContext.ts b/src/visitor/VisitorContext.ts index 03049ef1..ff6472ce 100644 --- a/src/visitor/VisitorContext.ts +++ b/src/visitor/VisitorContext.ts @@ -7,7 +7,7 @@ import { doesExist, hasItems, mustExist } from '../utils'; /* eslint-disable @typescript-eslint/no-explicit-any */ -export interface RuleOptions { +export interface RuleSchemaOptions { coerce: boolean; defaults: boolean; mutate: boolean; @@ -15,12 +15,12 @@ export interface RuleOptions { export interface VisitorContextOptions { logger: Logger; - schemaOptions: RuleOptions; + schemaOptions: RuleSchemaOptions; } export class VisitorContext implements VisitorContextOptions, VisitorResult { public readonly logger: Logger; - public readonly schemaOptions: RuleOptions; + public readonly schemaOptions: RuleSchemaOptions; protected readonly ajv: Ajv.Ajv; protected readonly changeBuffer: Array;