Skip to content
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

feat(core): improve ruleset validation #2026

Merged
merged 2 commits into from
Aug 3, 2022
Merged

feat(core): improve ruleset validation #2026

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f53ee36
feat(core): improve validation
P0lip Jan 18, 2022
1329113
Merge remote-tracking branch 'origin/develop' into feat/core/validati…
P0lip Aug 3, 2022
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
4 changes: 1 addition & 3 deletions jest.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,7 @@ const projectDefault = {
preset: 'ts-jest',
moduleNameMapper: {
...mapValues(pathsToModuleNameMapper(compilerOptions.paths), v => path.join(__dirname, v)),
'^@stoplight/spectral-test-utils$': '<rootDir>/test-utils/node/index.ts',
'^nimma/fallbacks$': '<rootDir>/node_modules/nimma/dist/cjs/fallbacks/index.js',
'^nimma/legacy$': '<rootDir>/node_modules/nimma/dist/legacy/cjs/index.js',
'^@stoplight/spectral\\-test\\-utils$': '<rootDir>/test-utils/node/index.ts',
},
testEnvironment: 'node',
globals: {
Expand Down
5 changes: 4 additions & 1 deletion karma.conf.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Karma configuration
// Generated on Tue Jul 02 2019 17:18:30 GMT+0200 (Central European Summer Time)

import * as path from 'path';
import type { TransformCallback, TransformContext } from 'karma-typescript';
import type { Config } from 'karma';

Expand All @@ -14,7 +15,7 @@ module.exports = (config: Config): void => {
frameworks: ['jasmine', 'karma-typescript'],

// list of files / patterns to load in the browser
files: ['./__karma__/jest.ts', 'packages/*/src/**/*.ts'],
files: ['./__karma__/jest.ts', './test-utils/*.ts', 'packages/*/src/**/*.ts'],

// list of files / patterns to exclude
exclude: ['packages/cli/**', 'packages/ruleset-bundler/src/plugins/commonjs.ts', '**/*.jest.test.ts'],
Expand All @@ -24,6 +25,7 @@ module.exports = (config: Config): void => {
preprocessors: {
'packages/*/src/**/*.ts': ['karma-typescript'],
'./__karma__/**/*.ts': ['karma-typescript'],
'./test-utils/*.ts': ['karma-typescript'],
},

// @ts-expect-error: non-standard - karmaTypeScriptConfig
Expand All @@ -35,6 +37,7 @@ module.exports = (config: Config): void => {
resolve: {
alias: {
'@stoplight/spectral-test-utils': require.resolve('./test-utils/browser/index.js'),
'@stoplight/spectral-test-utils/matchers': path.join(__dirname, './test-utils/matchers.ts'),
nimma: require.resolve('./node_modules/nimma/dist/legacy/cjs/index.js'),
'nimma/fallbacks': require.resolve('./node_modules/nimma/dist/legacy/cjs/fallbacks/index.js'),
'nimma/legacy': require.resolve('./node_modules/nimma/dist/legacy/cjs/index.js'),
Expand Down
1 change: 0 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,6 @@
"jest": "^27.4.3",
"jest-mock": "^27.4.2",
"jest-when": "^3.4.2",
"json-schema": "^0.4.0",
"karma": "^6.1.1",
"karma-chrome-launcher": "^3.1.0",
"karma-jasmine": "^3.3.1",
Expand Down
58 changes: 54 additions & 4 deletions packages/cli/src/services/__tests__/linter.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,12 @@
import '@stoplight/spectral-test-utils/matchers';

import { join, resolve } from '@stoplight/path';
import nock from 'nock';
import * as yargs from 'yargs';
import { DiagnosticSeverity } from '@stoplight/types';
import { RulesetValidationError } from '@stoplight/spectral-core';
import '@stoplight/spectral-test-utils/matchers';
import AggregateError = require('es-aggregate-error');
import * as process from 'process';

import lintCommand from '../../commands/lint';
Expand Down Expand Up @@ -221,8 +225,32 @@ describe('Linter service', () => {
});

it('fails trying to extend an invalid relative ruleset', () => {
return expect(run(`lint ${validCustomOas3SpecPath} -r ${invalidNestedRulesetPath}`)).rejects.toThrowError(
RulesetValidationError,
return expect(
run(`lint ${validCustomOas3SpecPath} -r ${invalidNestedRulesetPath}`),
).rejects.toThrowAggregateError(
new AggregateError([
new RulesetValidationError('must be equal to one of the allowed values', [
'rules',
'rule-with-invalid-enum',
]),
new RulesetValidationError('the rule must have at least "given" and "then" properties', [
'rules',
'rule-without-given-nor-them',
]),
new RulesetValidationError('allowed types are "style" and "validation"', [
'rules',
'rule-with-invalid-enum',
'type',
]),
new RulesetValidationError('must be equal to one of the allowed values', [
'rules',
'rule-without-given-nor-them',
]),
new RulesetValidationError(
'the value has to be one of: 0, 1, 2, 3 or "error", "warn", "info", "hint", "off"',
['rules', 'rule-with-invalid-enum', 'severity'],
),
]),
);
});
});
Expand All @@ -235,8 +263,30 @@ describe('Linter service', () => {
});

it('outputs "invalid ruleset" error', () => {
return expect(run(`lint ${validOas3SpecPath} -r ${invalidRulesetPath}`)).rejects.toThrowError(
RulesetValidationError,
return expect(run(`lint ${validOas3SpecPath} -r ${invalidRulesetPath}`)).rejects.toThrowAggregateError(
new AggregateError([
new RulesetValidationError('must be equal to one of the allowed values', [
'rules',
'rule-with-invalid-enum',
]),
new RulesetValidationError('the rule must have at least "given" and "then" properties', [
'rules',
'rule-without-given-nor-them',
]),
new RulesetValidationError('allowed types are "style" and "validation"', [
'rules',
'rule-with-invalid-enum',
'type',
]),
new RulesetValidationError('must be equal to one of the allowed values', [
'rules',
'rule-without-given-nor-them',
]),
new RulesetValidationError(
'the value has to be one of: 0, 1, 2, 3 or "error", "warn", "info", "hint", "off"',
['rules', 'rule-with-invalid-enum', 'severity'],
),
]),
);
});

Expand Down
2 changes: 2 additions & 0 deletions packages/core/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,11 +45,13 @@
"@stoplight/spectral-ref-resolver": "^1.0.0",
"@stoplight/spectral-runtime": "^1.0.0",
"@stoplight/types": "~13.2.0",
"@types/es-aggregate-error": "^1.0.2",
"@types/json-schema": "^7.0.11",
"ajv": "^8.6.0",
"ajv-errors": "~3.0.0",
"ajv-formats": "~2.1.0",
"blueimp-md5": "2.18.0",
"es-aggregate-error": "^1.0.7",
"jsonpath-plus": "6.0.1",
"lodash": "~4.17.21",
"lodash.topath": "^4.5.2",
Expand Down
8 changes: 6 additions & 2 deletions packages/core/src/ruleset/__tests__/__fixtures__/overrides/formats.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,9 @@ const ruleset: RulesetDefinition = {
then: {
function: schema,
functionOptions: {
type: 'number',
schema: {
type: 'number',
},
},
},
},
Expand All @@ -39,7 +41,9 @@ const ruleset: RulesetDefinition = {
then: {
function: schema,
functionOptions: {
type: 'boolean',
schema: {
type: 'boolean',
},
},
},
},
Expand Down
2 changes: 1 addition & 1 deletion packages/core/src/ruleset/__tests__/ruleset.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,7 @@ describe('Ruleset', () => {
describe('error handling', () => {
it('given empty ruleset, should throw a user friendly error', () => {
expect(() => new Ruleset({})).toThrowError(
new RulesetValidationError('Ruleset must have rules or extends or overrides defined'),
new RulesetValidationError('Ruleset must have rules or extends or overrides defined', []),
);
});
});
Expand Down
98 changes: 51 additions & 47 deletions packages/core/src/ruleset/function.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,56 +8,58 @@ import type { JSONSchema7 } from 'json-schema';

import { printPath, PrintStyle, printValue } from '@stoplight/spectral-runtime';

import { RulesetValidationError } from './validation';
import { RulesetValidationError } from './validation/index';
import { IFunctionResult, JSONSchema, RulesetFunction, RulesetFunctionWithValidator } from '../types';
import { isObject } from 'lodash';
import AggregateError = require('es-aggregate-error');

const ajv = new Ajv({ allErrors: true, allowUnionTypes: true, strict: true, keywords: ['x-internal'] });
ajvErrors(ajv);
addFormats(ajv);

export class RulesetFunctionValidationError extends RulesetValidationError {
constructor(fn: string, errors: ErrorObject[]) {
const messages = errors.map(error => {
switch (error.keyword) {
case 'type': {
const path = printPath(error.instancePath.slice(1).split('/'), PrintStyle.Dot);
const values = Array.isArray(error.params.type) ? error.params.type.join(', ') : String(error.params.type);

return `"${fn}" function and its "${path}" option accepts only the following types: ${values}`;
}

case 'required': {
const missingProperty = (error as RequiredError).params.missingProperty;
const missingPropertyPath =
error.instancePath === ''
? missingProperty
: printPath([...error.instancePath.slice(1).split('/'), missingProperty], PrintStyle.Dot);

return `"${fn}" function is missing "${missingPropertyPath}" option`;
}

case 'additionalProperties': {
const additionalProperty = (error as AdditionalPropertiesError).params.additionalProperty;
const additionalPropertyPath =
error.instancePath === ''
? additionalProperty
: printPath([...error.instancePath.slice(1).split('/'), additionalProperty], PrintStyle.Dot);

return `"${fn}" function does not support "${additionalPropertyPath}" option`;
}

case 'enum': {
const path = printPath(error.instancePath.slice(1).split('/'), PrintStyle.Dot);
const values = (error as EnumError).params.allowedValues.map(printValue).join(', ');

return `"${fn}" function and its "${path}" option accepts only the following values: ${values}`;
}
default:
return error.message;
constructor(fn: string, error: ErrorObject) {
super(RulesetFunctionValidationError.printMessage(fn, error), error.instancePath.slice(1).split('/'));
}

private static printMessage(fn: string, error: ErrorObject): string {
switch (error.keyword) {
case 'type': {
const path = printPath(error.instancePath.slice(1).split('/'), PrintStyle.Dot);
const values = Array.isArray(error.params.type) ? error.params.type.join(', ') : String(error.params.type);

return `"${fn}" function and its "${path}" option accepts only the following types: ${values}`;
}

case 'required': {
const missingProperty = (error as RequiredError).params.missingProperty;
const missingPropertyPath =
error.instancePath === ''
? missingProperty
: printPath([...error.instancePath.slice(1).split('/'), missingProperty], PrintStyle.Dot);

return `"${fn}" function is missing "${missingPropertyPath}" option`;
}
});

super(messages.join('\n'));
case 'additionalProperties': {
const additionalProperty = (error as AdditionalPropertiesError).params.additionalProperty;
const additionalPropertyPath =
error.instancePath === ''
? additionalProperty
: printPath([...error.instancePath.slice(1).split('/'), additionalProperty], PrintStyle.Dot);

return `"${fn}" function does not support "${additionalPropertyPath}" option`;
}

case 'enum': {
const path = printPath(error.instancePath.slice(1).split('/'), PrintStyle.Dot);
const values = (error as EnumError).params.allowedValues.map(printValue).join(', ');

return `"${fn}" function and its "${path}" option accepts only the following values: ${values}`;
}
default:
return error.message ?? 'unknown error';
}
}
}

Expand Down Expand Up @@ -138,25 +140,27 @@ export function createRulesetFunction<I extends unknown, O extends unknown>(

Reflect.defineProperty(wrappedFn, 'name', { value: fn.name });

const validOpts = new Set<unknown>();
const validOpts = new WeakSet();
wrappedFn.validator = function (o: unknown): asserts o is O {
if (validOpts.has(o)) return; // I don't like this.
if (isObject(o) && validOpts.has(o)) return; // I don't like this.

if (validateOptions(o)) {
validOpts.add(o);
if (isObject(o)) validOpts.add(o);
return;
}

if (options === null) {
throw new TypeError(`"${fn.name || '<unknown>'}" function does not accept any options`);
throw new RulesetValidationError(`"${fn.name || '<unknown>'}" function does not accept any options`, []);
} else if (
'errors' in validateOptions &&
Array.isArray(validateOptions.errors) &&
validateOptions.errors.length > 0
) {
throw new RulesetFunctionValidationError(fn.name || '<unknown>', validateOptions.errors);
throw new AggregateError(
validateOptions.errors.map(error => new RulesetFunctionValidationError(fn.name || '<unknown>', error)),
);
} else {
throw new Error(`"functionOptions" of "${fn.name || '<unknown>'}" function must be valid`);
throw new RulesetValidationError(`"functionOptions" of "${fn.name || '<unknown>'}" function must be valid`, []);
}
};

Expand Down
2 changes: 1 addition & 1 deletion packages/core/src/ruleset/mergers/rules.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ export function mergeRule(
owner: existingRule.owner,
});
} else {
assertValidRule(rule);
assertValidRule(rule, name);
return new Rule(name, rule, ruleset);
}

Expand Down
4 changes: 2 additions & 2 deletions packages/core/src/ruleset/meta/js-extensions.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,12 +40,12 @@
},
"Format": {
"$anchor": "format",
"spectral-runtime": "format",
"x-spectral-runtime": "format",
"errorMessage": "must be a valid format"
},
"Function": {
"$anchor": "function",
"spectral-runtime": "function",
"x-spectral-runtime": "ruleset-function",
"type": "object",
"properties": {
"function": true
Expand Down
Loading