From bb500b3b3dc086b3c9a99bec59269ce5bf6efe7a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ro=C5=BCek?= Date: Tue, 7 Apr 2020 20:12:55 +0200 Subject: [PATCH 01/11] fix: improve oas2/oas3-schema rules validation --- src/__tests__/linter.jest.test.ts | 7 ++ src/__tests__/linter.test.ts | 19 +++-- .../__fixtures__/draft-ref.oas2.json | 3 +- src/cli/services/__tests__/linter.test.ts | 54 ++++++++++++- src/functions/__tests__/schema.test.ts | 2 +- src/functions/schema.ts | 23 ++++-- src/rulesets/message.ts | 5 +- src/rulesets/oas/__tests__/oas2-schema.ts | 33 +++++--- .../__tests__/oasDocumentSchema.test.ts | 77 +++++++++++++++++++ .../oas/functions/oasDocumentSchema.ts | 29 +++++++ src/rulesets/oas/index.json | 7 +- test-harness/scenarios/oas3-schema.scenario | 42 ++++++++++ .../results-format-junit.oas3.scenario | 2 +- 13 files changed, 265 insertions(+), 38 deletions(-) create mode 100644 src/rulesets/oas/functions/__tests__/oasDocumentSchema.test.ts create mode 100644 src/rulesets/oas/functions/oasDocumentSchema.ts create mode 100644 test-harness/scenarios/oas3-schema.scenario diff --git a/src/__tests__/linter.jest.test.ts b/src/__tests__/linter.jest.test.ts index 232c4effd..ec36a3e12 100644 --- a/src/__tests__/linter.jest.test.ts +++ b/src/__tests__/linter.jest.test.ts @@ -5,7 +5,10 @@ import { isOpenApiv3 } from '../formats'; import { httpAndFileResolver } from '../resolvers/http-and-file'; import { Spectral } from '../spectral'; +import { functions } from '../functions'; import { readRuleset } from '../rulesets'; +import { setFunctionContext } from '../rulesets/evaluators'; +import oasDocumentSchema from '../rulesets/oas/functions/oasDocumentSchema'; import { IRuleset, RulesetExceptionCollection } from '../types/ruleset'; const customFunctionOASRuleset = path.join(__dirname, './__fixtures__/custom-functions-oas-ruleset.json'); @@ -361,6 +364,7 @@ describe('Linter', () => { }; spectral.setRuleset({ rules, exceptions: {}, functions: {} }); + spectral.setFunctions({ oasDocumentSchema: setFunctionContext({ functions }, oasDocumentSchema) }); const first = await spectral.run(document, opts); @@ -376,6 +380,7 @@ describe('Linter', () => { const exceptions = extractExceptionFrom(testRuleset, 'strings-maxLength', 0); spectral.setRuleset({ rules, exceptions, functions: {} }); + spectral.setFunctions({ oasDocumentSchema: setFunctionContext({ functions }, oasDocumentSchema) }); const second = await spectral.run(document, opts); @@ -396,6 +401,7 @@ describe('Linter', () => { }; spectral.setRuleset({ rules, exceptions: {}, functions: {} }); + spectral.setFunctions({ oasDocumentSchema: setFunctionContext({ functions }, oasDocumentSchema) }); const first = await spectral.run(document, opts); @@ -411,6 +417,7 @@ describe('Linter', () => { const exceptions = extractExceptionFrom(testRuleset, 'no-yaml-remote-reference', 1); spectral.setRuleset({ rules, exceptions, functions: {} }); + spectral.setFunctions({ oasDocumentSchema: setFunctionContext({ functions }, oasDocumentSchema) }); const second = await spectral.run(document, opts); diff --git a/src/__tests__/linter.test.ts b/src/__tests__/linter.test.ts index e1fdc221b..80a5d3905 100644 --- a/src/__tests__/linter.test.ts +++ b/src/__tests__/linter.test.ts @@ -158,13 +158,16 @@ describe('linter', () => { }) as RuleCollection, ); + // @ts-ignore + spectral.rules['oas3-schema'].then.function = 'oasDocumentSchema'; + const result = await spectral.run(invalidSchema); expect(result).toEqual([ expect.objectContaining({ code: 'oas3-schema', - message: `\`200\` property should have required property \`$ref\``, - path: ['paths', '/pets', 'get', 'responses', '200'], + message: 'Property `type` is not expected to be here', + path: ['paths', '/pets', 'get', 'responses', '200', 'headers', 'header-1'], }), expect.objectContaining({ code: 'invalid-ref', @@ -194,8 +197,8 @@ describe('linter', () => { expect.arrayContaining([ expect.objectContaining({ code: 'oas3-schema', - message: `\`200\` property should have required property \`$ref\``, - path: ['paths', '/pets', 'get', 'responses', '200'], + message: 'Property `type` is not expected to be here', + path: ['paths', '/pets', 'get', 'responses', '200', 'headers', 'header-1'], }), ]), ); @@ -621,8 +624,8 @@ responses:: !!foo }), expect.objectContaining({ code: 'oas3-schema', - message: `\`200\` property should have required property \`$ref\``, - path: ['paths', '/pets', 'get', 'responses', '200'], + message: 'Property `type` is not expected to be here', + path: ['paths', '/pets', 'get', 'responses', '200', 'headers', 'header-1'], }), expect.objectContaining({ code: 'invalid-ref', @@ -1028,8 +1031,8 @@ responses:: !!foo expect.arrayContaining([ expect.objectContaining({ code: 'oas3-schema', - message: 'Schema error at #/paths/~1pets/get/responses/200', - path: ['paths', '/pets', 'get', 'responses', '200'], + message: 'Schema error at #/paths/~1pets/get/responses/200/headers/header-1', + path: ['paths', '/pets', 'get', 'responses', '200', 'headers', 'header-1'], }), ]), ); diff --git a/src/cli/services/__tests__/__fixtures__/draft-ref.oas2.json b/src/cli/services/__tests__/__fixtures__/draft-ref.oas2.json index aed1ba3d7..594280d8d 100644 --- a/src/cli/services/__tests__/__fixtures__/draft-ref.oas2.json +++ b/src/cli/services/__tests__/__fixtures__/draft-ref.oas2.json @@ -5,6 +5,7 @@ "$ref": "./refs/info.json#/definitions/info" }, "paths": { - "test": {} + "/test": {}, + "foo": {} } } diff --git a/src/cli/services/__tests__/linter.test.ts b/src/cli/services/__tests__/linter.test.ts index bc978e5bd..a8c6a9060 100644 --- a/src/cli/services/__tests__/linter.test.ts +++ b/src/cli/services/__tests__/linter.test.ts @@ -225,7 +225,7 @@ describe('Linter service', () => { }), expect.objectContaining({ code: 'oas3-schema', - path: ['paths', '/pets', 'get', 'responses', '200'], + path: ['paths', '/pets', 'get', 'responses', '200', 'headers', 'header-1'], source: join(process.cwd(), 'src/__tests__/__fixtures__/petstore.invalid-schema.oas3.json'), }), expect.objectContaining({ @@ -397,7 +397,7 @@ describe('Linter service', () => { }), expect.objectContaining({ code: 'oas3-schema', - path: ['paths', '/pets', 'get', 'responses', '200'], + path: ['paths', '/pets', 'get', 'responses', '200', 'headers', 'header-1'], source: join(process.cwd(), 'src/__tests__/__fixtures__/petstore.invalid-schema.oas3.json'), }), ]), @@ -574,7 +574,23 @@ describe('Linter service', () => { }), expect.objectContaining({ code: 'oas2-schema', - message: 'property foo is not expected to be here', + message: 'Property `foo` is not expected to be here', + path: ['paths'], + range: { + end: { + character: 13, + line: 8, + }, + start: { + character: 10, + line: 6, + }, + }, + source: expect.stringContaining('__tests__/__fixtures__/draft-ref.oas2.json'), + }), + expect.objectContaining({ + code: 'oas2-schema', + message: 'Property `foo` is not expected to be here', path: ['definitions', 'info'], range: { end: { @@ -604,6 +620,22 @@ describe('Linter service', () => { }, source: expect.stringContaining('__tests__/__fixtures__/refs/info.json'), }), + expect.objectContaining({ + code: 'oas2-schema', + message: '`description` property type should be string', + path: ['definitions', 'info', 'description'], + range: { + end: { + character: 22, + line: 5, + }, + start: { + character: 21, + line: 5, + }, + }, + source: expect.stringContaining('__tests__/__fixtures__/refs/info.json'), + }), ]); }); @@ -655,6 +687,22 @@ describe('Linter service', () => { }, source: expect.stringContaining('__tests__/__fixtures__/refs/contact.json'), }), + expect.objectContaining({ + code: 'oas2-schema', + message: 'Property `response` is not expected to be here', + path: ['paths', '/test', 'get'], + range: { + end: { + character: 7, + line: 5, + }, + start: { + character: 13, + line: 3, + }, + }, + source: expect.stringContaining('__tests__/__fixtures__/refs/paths.json'), + }), expect.objectContaining({ code: 'operation-description', message: 'Operation `description` must be present and non-empty string.', diff --git a/src/functions/__tests__/schema.test.ts b/src/functions/__tests__/schema.test.ts index 68ba2e05e..d05421e38 100644 --- a/src/functions/__tests__/schema.test.ts +++ b/src/functions/__tests__/schema.test.ts @@ -158,7 +158,7 @@ describe('schema', () => { ), ).toEqual([ { - message: 'property baz is not expected to be here', + message: 'Property `baz` is not expected to be here', path: ['foo'], }, ]); diff --git a/src/functions/schema.ts b/src/functions/schema.ts index 03d1486a8..ae0843ebf 100644 --- a/src/functions/schema.ts +++ b/src/functions/schema.ts @@ -14,6 +14,10 @@ export interface ISchemaOptions { schema: object; // The oasVersion, either 2 or 3 for OpenAPI Spec versions, could also be 3.1 or a larger number if there's a need for it, otherwise JSON Schema oasVersion?: Optional; + allErrors?: boolean; + + // this is used by oasDocumentSchema function, to be removed once we sort out + prepareResults?(errors: AJV.ErrorObject[]): void; } export type SchemaRule = IRule; @@ -36,7 +40,7 @@ const logger = { const ajvInstances = {}; -function getAjv(oasVersion?: Optional): AJV.Ajv { +function getAjv(oasVersion: Optional, allErrors: Optional): AJV.Ajv { const type: string = oasVersion && oasVersion >= 2 ? 'oas' + oasVersion : 'jsonschema'; if (typeof ajvInstances[type] !== 'undefined') { return ajvInstances[type]; @@ -45,6 +49,7 @@ function getAjv(oasVersion?: Optional): AJV.Ajv { const ajvOpts: AJV.Options = { meta: true, // Add default meta schemas (draft 7 at the moment) schemaId: 'auto', + allErrors, jsonPointers: true, unknownFormats: 'ignore', nullable: oasVersion === 3, // Support nullable for OAS3 @@ -81,8 +86,8 @@ function getSchemaId(schemaObj: JSONSchema): void | string { } const validators = new (class extends WeakMap { - public get(schemaObj: JSONSchema, oasVersion?: Optional) { - const ajv = getAjv(oasVersion); + public get({ schema: schemaObj, oasVersion, allErrors }: ISchemaOptions) { + const ajv = getAjv(oasVersion, allErrors); const schemaId = getSchemaId(schemaObj); let validator = schemaId !== void 0 ? ajv.getSchema(schemaId) : void 0; if (validator !== void 0) { @@ -100,9 +105,9 @@ const validators = new (class extends WeakMap { } })(); -const replaceProperty = (substring: string, group1: string) => { - if (group1) { - return 'property '; +const replaceProperty = (substring: string, _: Optional, propertyName: Optional) => { + if (typeof _ === 'string' && propertyName !== void 0) { + return `Property \`${propertyName}\``; } return '{{property|gravis|append-property|optional-typeof}}'; @@ -113,7 +118,7 @@ const cleanAJVErrorMessage = (message: string, path: Optional, suggestio if (path) { cleanMessage = message.replace( - new RegExp(`^${escapeRegExp(decodePointerFragment(path))}:?\\s*(Property\\s+)?`), + new RegExp(`^${escapeRegExp(decodePointerFragment(path))}:?\\s*(?:(Property\\s+)([^\\s]+))?`), replaceProperty, ); } else if (cleanMessage.startsWith(':')) { @@ -145,8 +150,10 @@ export const schema: IFunction = (targetVal, opts, paths) => { try { // we used the compiled validation now, hence this lookup here (see the logic above for more info) - const validator = validators.get(schemaObj, opts.oasVersion); + const validator = validators.get(opts); if (!validator(targetVal) && validator.errors) { + opts.prepareResults?.(validator.errors); + try { results.push( ...(betterAjvErrors(schemaObj, targetVal, validator.errors, { format: 'js' }) as IAJVOutputError[]).map( diff --git a/src/rulesets/message.ts b/src/rulesets/message.ts index c09375326..8ca5ce825 100644 --- a/src/rulesets/message.ts +++ b/src/rulesets/message.ts @@ -1,5 +1,5 @@ import { Segment } from '@stoplight/types'; -import { isObject } from 'lodash'; +import { capitalize, isObject } from 'lodash'; import { Replacer } from '../utils/replacer'; export interface IMessageVars { @@ -14,9 +14,10 @@ export type MessageInterpolator = (str: string, values: IMessageVars) => string; const MessageReplacer = new Replacer(2); -MessageReplacer.addTransformer('double-quotes', (id, value) => (value ? `"${value}"` : '')); +MessageReplacer.addTransformer('double-quotes', (id, value) => (value ? `" ${value}"` : '')); MessageReplacer.addTransformer('single-quotes', (id, value) => (value ? `'${value}'` : '')); MessageReplacer.addTransformer('gravis', (id, value) => (value ? `\`${value}\`` : '')); +MessageReplacer.addTransformer('capitalize', (id, value) => capitalize(String(value))); MessageReplacer.addTransformer('append-property', (id, value) => (value ? `${value} property ` : '')); MessageReplacer.addTransformer('optional-typeof', (id, value, values) => diff --git a/src/rulesets/oas/__tests__/oas2-schema.ts b/src/rulesets/oas/__tests__/oas2-schema.ts index 12936ccb6..1c27646d4 100644 --- a/src/rulesets/oas/__tests__/oas2-schema.ts +++ b/src/rulesets/oas/__tests__/oas2-schema.ts @@ -1,21 +1,30 @@ +import { functions } from '../../../functions'; import { RuleType, Spectral } from '../../../spectral'; +import { setFunctionContext } from '../../evaluators'; +import oasDocumentSchema from '../functions/oasDocumentSchema'; import * as ruleset from '../index.json'; import * as oas2Schema from '../schemas/schema.oas2.json'; describe('oas2-schema', () => { - const s = new Spectral(); - s.registerFormat('oas2', () => true); - s.setRules({ - 'oas2-schema': Object.assign({}, ruleset.rules['oas2-schema'], { - recommended: true, - type: RuleType[ruleset.rules['oas2-schema'].type], - then: { - ...ruleset.rules['oas2-schema'].then, - functionOptions: { - schema: oas2Schema, + let s: Spectral; + + beforeEach(() => { + s = new Spectral(); + s.registerFormat('oas2', () => true); + s.setFunctions({ oasDocumentSchema: setFunctionContext({ functions }, oasDocumentSchema) }); + s.setRules({ + 'oas2-schema': Object.assign({}, ruleset.rules['oas2-schema'], { + recommended: true, + type: RuleType[ruleset.rules['oas2-schema'].type], + then: { + ...ruleset.rules['oas2-schema'].then, + functionOptions: { + ...ruleset.rules['oas2-schema'].then.functionOptions, + schema: oas2Schema, + }, }, - }, - }), + }), + }); }); test('annotates with correct paths', async () => { diff --git a/src/rulesets/oas/functions/__tests__/oasDocumentSchema.test.ts b/src/rulesets/oas/functions/__tests__/oasDocumentSchema.test.ts new file mode 100644 index 000000000..89ac104a7 --- /dev/null +++ b/src/rulesets/oas/functions/__tests__/oasDocumentSchema.test.ts @@ -0,0 +1,77 @@ +import { isOpenApiv2, isOpenApiv3, RuleType, Spectral } from '../../../..'; +import { functions } from '../../../../functions'; +import { setFunctionContext } from '../../../evaluators'; +import { rules } from '../../index.json'; +import oasDocumentSchema from '../oasDocumentSchema'; + +import { DiagnosticSeverity } from '@stoplight/types/dist'; +import * as oas2Schema from '../../schemas/schema.oas2.json'; +import * as oas3Schema from '../../schemas/schema.oas3.json'; + +describe('oasDocumentSchema', () => { + let s: Spectral; + + beforeEach(() => { + s = new Spectral(); + + s.registerFormat('oas2', isOpenApiv2); + s.registerFormat('oas3', isOpenApiv3); + s.setFunctions({ oasDocumentSchema: setFunctionContext({ functions }, oasDocumentSchema) }); + s.setRules({ + 'oas2-schema': { + ...rules['oas2-schema'], + type: RuleType[rules['oas2-schema'].type], + then: { + ...rules['oas2-schema'].then, + functionOptions: { + ...rules['oas2-schema'].then.functionOptions, + schema: oas2Schema, + }, + }, + }, + 'oas3-schema': { + ...rules['oas3-schema'], + type: RuleType[rules['oas3-schema'].type], + then: { + ...rules['oas3-schema'].then, + functionOptions: { + ...rules['oas3-schema'].then.functionOptions, + schema: oas3Schema, + }, + }, + }, + }); + }); + + describe('given OpenAPI 3 document', () => { + test('validate responses', async () => { + expect( + await s.run({ + openapi: '3.0.1', + info: { + title: 'response example', + version: '1.0', + }, + paths: { + '/user': { + get: { + operationId: 'd', + responses: { + 200: {}, + }, + }, + }, + }, + }), + ).toEqual([ + { + code: 'oas3-schema', + message: '`200` property should have required property `description`', + path: ['paths', '/user', 'get', 'responses', '200'], + severity: DiagnosticSeverity.Error, + range: expect.any(Object), + }, + ]); + }); + }); +}); diff --git a/src/rulesets/oas/functions/oasDocumentSchema.ts b/src/rulesets/oas/functions/oasDocumentSchema.ts new file mode 100644 index 000000000..b2940cb3c --- /dev/null +++ b/src/rulesets/oas/functions/oasDocumentSchema.ts @@ -0,0 +1,29 @@ +import * as AJV from 'ajv'; +import { ISchemaOptions } from '../../../functions/schema'; +import { IFunction, IFunctionContext } from '../../../types'; + +// /shrug +function prepareResults(errors: AJV.ErrorObject[]) { + for (let i = 0; i < errors.length; i++) { + const error = errors[i]; + + if (i + 1 < errors.length && errors[i + 1].dataPath === error.dataPath) { + errors.splice(i + 1, 1); + i--; + } else if ( + i > 0 && + error.keyword === 'required' && + (error.params as AJV.RequiredParams).missingProperty === '$ref' && + errors[i - 1].dataPath.startsWith(error.dataPath) + ) { + errors.splice(i, 1); + i--; + } + } +} + +export const oasDocumentSchema: IFunction = function(this: IFunctionContext, targetVal, opts, ...args) { + return this.functions.schema.call(this, targetVal, { ...opts, prepareResults }, ...args); +}; + +export default oasDocumentSchema; diff --git a/src/rulesets/oas/index.json b/src/rulesets/oas/index.json index 9bd16ce86..fa7af20d2 100644 --- a/src/rulesets/oas/index.json +++ b/src/rulesets/oas/index.json @@ -1,6 +1,7 @@ { "formats": ["oas2", "oas3"], "functions": [ + "oasDocumentSchema", "oasOp2xxResponse", "oasOpFormDataConsumeCheck", "oasOpIdUnique", @@ -586,8 +587,9 @@ "type": "validation", "given": "$", "then": { - "function": "schema", + "function": "oasDocumentSchema", "functionOptions": { + "allErrors": true, "schema": { "$ref": "./schemas/schema.oas2.json" } @@ -844,8 +846,9 @@ "type": "validation", "given": "$", "then": { - "function": "schema", + "function": "oasDocumentSchema", "functionOptions": { + "allErrors": true, "schema": { "$ref": "./schemas/schema.oas3.json" } diff --git a/test-harness/scenarios/oas3-schema.scenario b/test-harness/scenarios/oas3-schema.scenario new file mode 100644 index 000000000..393229060 --- /dev/null +++ b/test-harness/scenarios/oas3-schema.scenario @@ -0,0 +1,42 @@ +====test==== +The amount of enabled rules is printed in a parenthesis. +====document==== +openapi: 3.0.1 +info: + title: Example $ref error + version: 1.0.0 +paths: + /user: + get: + operationId: getUser + parameters: + - type: string + name: module_id + in: path + required: true + schema: + type: string + description: Module ID + responses: + "200": + description: An Example + content: + application/json: + schema: + type: object + properties: + user_id: 12345 +====asset:ruleset==== +extends: [[spectral:oas, off]] +rules: + oas3-schema: error +====command==== +{bin} lint {document} --ruleset {asset:ruleset} +====stdout==== +OpenAPI 3.x detected + +{document} + 10:11 error oas3-schema Property `type` is not expected to be here + 25:28 error oas3-schema `user_id` property type should be object + +✖ 2 problems (2 errors, 0 warnings, 0 infos, 0 hints) diff --git a/test-harness/scenarios/results-format-junit.oas3.scenario b/test-harness/scenarios/results-format-junit.oas3.scenario index 94765faa2..7cb4496e1 100644 --- a/test-harness/scenarios/results-format-junit.oas3.scenario +++ b/test-harness/scenarios/results-format-junit.oas3.scenario @@ -14,6 +14,6 @@ OpenAPI 3.x detected - + From af4eb9ae9cd2005f60aecdf122cb288c865daae9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ro=C5=BCek?= Date: Tue, 7 Apr 2020 20:35:16 +0200 Subject: [PATCH 02/11] fix(schema): more consistent casing --- src/cli/services/__tests__/linter.test.ts | 2 +- src/functions/__tests__/schema-path.test.ts | 12 +++---- src/functions/__tests__/schema.test.ts | 31 ++++++++++--------- src/functions/schema.ts | 11 ++++--- .../oas2-valid-definition-example.ts | 2 +- .../__tests__/oas2-valid-parameter-example.ts | 2 +- .../oas3-valid-oas-parameter-example.ts | 4 +-- .../oas3-valid-parameter-schema-example.ts | 2 +- .../__tests__/oas3-valid-schema-example.ts | 2 +- .../oas/__tests__/templates/_oas-example.ts | 2 +- .../__tests__/templates/_schema-example.ts | 2 +- .../external-schemas-ruleset.scenario | 4 +-- 12 files changed, 39 insertions(+), 37 deletions(-) diff --git a/src/cli/services/__tests__/linter.test.ts b/src/cli/services/__tests__/linter.test.ts index a8c6a9060..7009a5356 100644 --- a/src/cli/services/__tests__/linter.test.ts +++ b/src/cli/services/__tests__/linter.test.ts @@ -657,7 +657,7 @@ describe('Linter service', () => { }), expect.objectContaining({ code: 'oas2-schema', - message: `object should have required property \`title\``, + message: `Object should have required property \`title\``, path: [], range: { end: { diff --git a/src/functions/__tests__/schema-path.test.ts b/src/functions/__tests__/schema-path.test.ts index 3addc3596..309250d47 100644 --- a/src/functions/__tests__/schema-path.test.ts +++ b/src/functions/__tests__/schema-path.test.ts @@ -64,7 +64,7 @@ describe('schema-path', () => { expect(runSchemaPath(target, fieldToCheck, path)).toEqual([ { path: ['example'], - message: 'object should have required property `url`', + message: 'Object should have required property `url`', }, ]); }); @@ -79,7 +79,7 @@ describe('schema-path', () => { expect(runSchemaPath(target, fieldToCheck, path)).toEqual([ { path: ['example'], - message: '{{property|gravis|append-property|optional-typeof}}type should be string', + message: '{{property|gravis|append-property|optional-typeof|capitalize}}type should be string', }, ]); }); @@ -109,11 +109,11 @@ describe('schema-path', () => { }; expect(runSchemaPath(target, '$.examples.*', path)).toEqual([ { - message: '{{property|gravis|append-property|optional-typeof}}type should be string', + message: '{{property|gravis|append-property|optional-typeof|capitalize}}type should be string', path: ['examples', 'application/json', 'id'], }, { - message: '{{property|gravis|append-property|optional-typeof}}type should be string', + message: '{{property|gravis|append-property|optional-typeof|capitalize}}type should be string', path: ['examples', 'application/yaml', 'id'], }, ]); @@ -130,7 +130,7 @@ describe('schema-path', () => { expect(runSchemaPath(target, fieldToCheck, path)).toEqual([ { - message: '{{property|gravis|append-property|optional-typeof}}format should match format `url`', + message: '{{property|gravis|append-property|optional-typeof|capitalize}}format should match format `url`', path: ['example'], }, ]); @@ -148,7 +148,7 @@ describe('schema-path', () => { }; expect(runSchemaPath(target, invalidFieldToCheck, path)).toEqual([ { - message: '{{property|double-quotes|append-property}}does not exist', + message: '{{property|gravis|append-property}}does not exist', path: ['nonsense'], }, ]); diff --git a/src/functions/__tests__/schema.test.ts b/src/functions/__tests__/schema.test.ts index d05421e38..2e9608c40 100644 --- a/src/functions/__tests__/schema.test.ts +++ b/src/functions/__tests__/schema.test.ts @@ -15,7 +15,7 @@ describe('schema', () => { expect(runSchema('', testSchema)).toEqual([ { - message: '{{property|gravis|append-property|optional-typeof}}type should be number', + message: '{{property|gravis|append-property|optional-typeof|capitalize}}type should be number', path: [], }, ]); @@ -28,7 +28,7 @@ describe('schema', () => { expect(runSchema(0, testSchema)).toEqual([ { - message: `{{property|gravis|append-property|optional-typeof}}type should be string`, + message: `{{property|gravis|append-property|optional-typeof|capitalize}}type should be string`, path: [], }, ]); @@ -41,7 +41,7 @@ describe('schema', () => { expect(runSchema(false, testSchema)).toEqual([ { - message: `{{property|gravis|append-property|optional-typeof}}type should be string`, + message: `{{property|gravis|append-property|optional-typeof|capitalize}}type should be string`, path: [], }, ]); @@ -54,7 +54,7 @@ describe('schema', () => { expect(runSchema(null, testSchema)).toEqual([ { - message: `{{property|gravis|append-property|optional-typeof}}type should be string`, + message: `{{property|gravis|append-property|optional-typeof|capitalize}}type should be string`, path: [], }, ]); @@ -95,7 +95,7 @@ describe('schema', () => { const input = { foo: 'bar' }; expect(runSchema(input, testSchema)).toEqual([ expect.objectContaining({ - message: '{{property|gravis|append-property|optional-typeof}}type should be array', + message: '{{property|gravis|append-property|optional-typeof|capitalize}}type should be array', path: [], }), ]); @@ -105,7 +105,8 @@ describe('schema', () => { const input = ['1', '2']; expect(runSchema(input, testSchema)).toEqual([ expect.objectContaining({ - message: '{{property|gravis|append-property|optional-typeof}}maxItems should NOT have more than 1 items', + message: + '{{property|gravis|append-property|optional-typeof|capitalize}}maxItems should NOT have more than 1 items', path: [], }), ]); @@ -141,7 +142,7 @@ describe('schema', () => { ), ).toEqual([ { - message: `{{property|gravis|append-property|optional-typeof}}type should be string`, + message: `{{property|gravis|append-property|optional-typeof|capitalize}}type should be string`, path: ['foo', 'bar'], }, ]); @@ -175,7 +176,7 @@ describe('schema', () => { const input = 'not an email'; expect(runSchema(input, testSchema)).toEqual([ expect.objectContaining({ - message: '{{property|gravis|append-property|optional-typeof}}format should match format `email`', + message: '{{property|gravis|append-property|optional-typeof|capitalize}}format should match format `email`', path: [], }), ]); @@ -213,7 +214,7 @@ describe('schema', () => { expect(runSchema(2, testSchema)).toEqual([ { path: [], - message: `{{property|gravis|append-property|optional-typeof}}type should be string`, + message: `{{property|gravis|append-property|optional-typeof|capitalize}}type should be string`, }, ]); expect(runSchema('a', testSchema2)).toEqual([]); @@ -233,7 +234,7 @@ describe('schema', () => { expect(runSchema(2, testSchema)).toEqual([ { path: [], - message: `{{property|gravis|append-property|optional-typeof}}type should be string`, + message: `{{property|gravis|append-property|optional-typeof|capitalize}}type should be string`, }, ]); expect(runSchema('a', testSchema2)).toEqual([]); @@ -259,7 +260,7 @@ describe('schema', () => { it('reports pretty enum errors for a string', () => { expect(runSchema('baz', testSchema)).toEqual([ { - message: `string should be equal to one of the allowed values: foo, bar. Did you mean bar?`, + message: `String should be equal to one of the allowed values: foo, bar. Did you mean bar?`, path: [], }, ]); @@ -268,7 +269,7 @@ describe('schema', () => { it('reports pretty enum errors for a number', () => { expect(runSchema(2, testSchema)).toEqual([ { - message: `{{property|gravis|append-property|optional-typeof}}type should be string`, + message: `{{property|gravis|append-property|optional-typeof|capitalize}}type should be string`, path: [], }, ]); @@ -285,7 +286,7 @@ describe('schema', () => { it('reports pretty enum errors for a string', () => { expect(runSchema('baz', testSchema)).toEqual([ { - message: '{{property|gravis|append-property|optional-typeof}}type should be integer', + message: '{{property|gravis|append-property|optional-typeof|capitalize}}type should be integer', path: [], }, ]); @@ -294,7 +295,7 @@ describe('schema', () => { it('reports pretty enum errors for a number', () => { expect(runSchema(2, testSchema)).toEqual([ { - message: `number should be equal to one of the allowed values: 1, 3, 5, 10, 12`, + message: `Number should be equal to one of the allowed values: 1, 3, 5, 10, 12`, path: [], }, ]); @@ -311,7 +312,7 @@ describe('schema', () => { expect(runSchema('three', testSchema)).toEqual([ { - message: `string should be equal to one of the allowed values: foo, bar`, + message: `String should be equal to one of the allowed values: foo, bar`, path: [], }, ]); diff --git a/src/functions/schema.ts b/src/functions/schema.ts index ae0843ebf..ac8257c6c 100644 --- a/src/functions/schema.ts +++ b/src/functions/schema.ts @@ -5,7 +5,7 @@ import { ValidateFunction } from 'ajv'; import * as jsonSpecv4 from 'ajv/lib/refs/json-schema-draft-04.json'; import * as jsonSpecv6 from 'ajv/lib/refs/json-schema-draft-06.json'; import { IOutputError } from 'better-ajv-errors'; -import { escapeRegExp } from 'lodash'; +import { capitalize, escapeRegExp } from 'lodash'; import { IFunction, IFunctionResult, IRule, JSONSchema, RuleFunction } from '../types'; const oasFormatValidator = require('ajv-oai/lib/format-validator'); const betterAjvErrors = require('better-ajv-errors/lib/modern'); @@ -110,7 +110,7 @@ const replaceProperty = (substring: string, _: Optional, proper return `Property \`${propertyName}\``; } - return '{{property|gravis|append-property|optional-typeof}}'; + return '{{property|gravis|append-property|optional-typeof|capitalize}}'; }; const cleanAJVErrorMessage = (message: string, path: Optional, suggestion: Optional, type: string) => { @@ -124,7 +124,7 @@ const cleanAJVErrorMessage = (message: string, path: Optional, suggestio } else if (cleanMessage.startsWith(':')) { cleanMessage = cleanMessage.replace(/:\s*/, replaceProperty); } else { - cleanMessage = `${type} ${cleanMessage}`; + cleanMessage = `${capitalize(type)} ${cleanMessage}`; } return `${cleanMessage.replace(/['"]/g, '`')}${ @@ -137,13 +137,14 @@ export const schema: IFunction = (targetVal, opts, paths) => { const path = paths.target || paths.given; - if (targetVal === void 0) + if (targetVal === void 0) { return [ { path, - message: `{{property|double-quotes|append-property}}does not exist`, + message: `{{property|gravis|append-property}}does not exist`, }, ]; + } // we already access a resolved object in src/functions/schema-path.ts const { schema: schemaObj } = opts; diff --git a/src/rulesets/oas/__tests__/oas2-valid-definition-example.ts b/src/rulesets/oas/__tests__/oas2-valid-definition-example.ts index bc18d965d..e1e44c1b1 100644 --- a/src/rulesets/oas/__tests__/oas2-valid-definition-example.ts +++ b/src/rulesets/oas/__tests__/oas2-valid-definition-example.ts @@ -104,7 +104,7 @@ describe('oas2-valid-definition-example', () => { expect(results).toEqual([ expect.objectContaining({ code: 'oas2-valid-definition-example', - message: 'object should have required property `url`', + message: 'Object should have required property `url`', severity: DiagnosticSeverity.Error, }), ]); diff --git a/src/rulesets/oas/__tests__/oas2-valid-parameter-example.ts b/src/rulesets/oas/__tests__/oas2-valid-parameter-example.ts index e3cd24c88..cd7625810 100644 --- a/src/rulesets/oas/__tests__/oas2-valid-parameter-example.ts +++ b/src/rulesets/oas/__tests__/oas2-valid-parameter-example.ts @@ -127,7 +127,7 @@ describe('oas2-valid-parameter-example', () => { expect(results).toEqual([ expect.objectContaining({ code: 'oas2-valid-parameter-example', - message: 'object should have required property `url`', + message: 'Object should have required property `url`', severity: DiagnosticSeverity.Error, }), ]); diff --git a/src/rulesets/oas/__tests__/oas3-valid-oas-parameter-example.ts b/src/rulesets/oas/__tests__/oas3-valid-oas-parameter-example.ts index 839896a35..00aebdb12 100644 --- a/src/rulesets/oas/__tests__/oas3-valid-oas-parameter-example.ts +++ b/src/rulesets/oas/__tests__/oas3-valid-oas-parameter-example.ts @@ -119,7 +119,7 @@ describe('oas3-valid-oas-parameter-example', () => { expect(results).toEqual([ expect.objectContaining({ code: 'oas3-valid-oas-parameter-example', - message: 'object should have required property `abc`', + message: 'Object should have required property `abc`', severity: DiagnosticSeverity.Error, }), ]); @@ -156,7 +156,7 @@ describe('oas3-valid-oas-parameter-example', () => { expect(results).toEqual([ expect.objectContaining({ code: 'oas3-valid-oas-parameter-example', - message: 'object should have required property `url`', + message: 'Object should have required property `url`', severity: DiagnosticSeverity.Error, }), ]); diff --git a/src/rulesets/oas/__tests__/oas3-valid-parameter-schema-example.ts b/src/rulesets/oas/__tests__/oas3-valid-parameter-schema-example.ts index 8067d9b98..941ecdec2 100644 --- a/src/rulesets/oas/__tests__/oas3-valid-parameter-schema-example.ts +++ b/src/rulesets/oas/__tests__/oas3-valid-parameter-schema-example.ts @@ -284,7 +284,7 @@ describe('oas3-valid-parameter-schema-example', () => { expect(results).toEqual([ expect.objectContaining({ code: 'oas3-valid-parameter-schema-example', - message: 'object should have required property `url`', + message: 'Object should have required property `url`', severity: DiagnosticSeverity.Error, }), ]); diff --git a/src/rulesets/oas/__tests__/oas3-valid-schema-example.ts b/src/rulesets/oas/__tests__/oas3-valid-schema-example.ts index eff607011..95aef983a 100644 --- a/src/rulesets/oas/__tests__/oas3-valid-schema-example.ts +++ b/src/rulesets/oas/__tests__/oas3-valid-schema-example.ts @@ -226,7 +226,7 @@ describe('oas3-valid-schema-example', () => { expect(results).toEqual([ expect.objectContaining({ code: 'oas3-valid-schema-example', - message: 'object should have required property `url`', + message: 'Object should have required property `url`', severity: DiagnosticSeverity.Error, }), ]); diff --git a/src/rulesets/oas/__tests__/templates/_oas-example.ts b/src/rulesets/oas/__tests__/templates/_oas-example.ts index fd4b593ed..b72598bc9 100644 --- a/src/rulesets/oas/__tests__/templates/_oas-example.ts +++ b/src/rulesets/oas/__tests__/templates/_oas-example.ts @@ -158,7 +158,7 @@ export default (ruleName: string, path: string) => { expect(results).toEqual([ expect.objectContaining({ code: ruleName, - message: 'object should have required property `url`', + message: 'Object should have required property `url`', severity: DiagnosticSeverity.Error, }), ]); diff --git a/src/rulesets/oas/__tests__/templates/_schema-example.ts b/src/rulesets/oas/__tests__/templates/_schema-example.ts index 2f7d698b0..9c488455e 100644 --- a/src/rulesets/oas/__tests__/templates/_schema-example.ts +++ b/src/rulesets/oas/__tests__/templates/_schema-example.ts @@ -212,7 +212,7 @@ export default (ruleName: string, path: string) => { expect(results).toEqual([ expect.objectContaining({ code: ruleName, - message: 'object should have required property `url`', + message: 'Object should have required property `url`', severity: DiagnosticSeverity.Error, }), ]); diff --git a/test-harness/scenarios/external-schemas-ruleset.scenario b/test-harness/scenarios/external-schemas-ruleset.scenario index 3aa363c05..9b59fee91 100644 --- a/test-harness/scenarios/external-schemas-ruleset.scenario +++ b/test-harness/scenarios/external-schemas-ruleset.scenario @@ -11,7 +11,7 @@ info: OpenAPI 3.x detected {document} - 3:10 error info-title string should be equal to one of the allowed values: Stoplight, Stoplight.io, StoplightIO. Did you mean Stoplight? - 4:16 error info-description string should be equal to one of the allowed values: foo, foo-bar, bar-foo + 3:10 error info-title String should be equal to one of the allowed values: Stoplight, Stoplight.io, StoplightIO. Did you mean Stoplight? + 4:16 error info-description String should be equal to one of the allowed values: foo, foo-bar, bar-foo ✖ 2 problems (2 errors, 0 warnings, 0 infos, 0 hints) From 2d81c8b7c4875780da877ff74e1e1999669eba95 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ro=C5=BCek?= Date: Tue, 7 Apr 2020 21:00:01 +0200 Subject: [PATCH 03/11] fix(schema): pretty-print path-less property --- src/functions/__tests__/schema.test.ts | 10 ++++++++++ src/functions/schema.ts | 2 ++ 2 files changed, 12 insertions(+) diff --git a/src/functions/__tests__/schema.test.ts b/src/functions/__tests__/schema.test.ts index 2e9608c40..418864d30 100644 --- a/src/functions/__tests__/schema.test.ts +++ b/src/functions/__tests__/schema.test.ts @@ -317,4 +317,14 @@ describe('schema', () => { }, ]); }); + + test('pretty-prints path-less property', () => { + const input = { foo: true }; + expect(runSchema(input, { additionalProperties: false })).toEqual([ + expect.objectContaining({ + message: 'Property `foo` is not expected to be here', + path: [], + }), + ]); + }); }); diff --git a/src/functions/schema.ts b/src/functions/schema.ts index ac8257c6c..e54e0aaee 100644 --- a/src/functions/schema.ts +++ b/src/functions/schema.ts @@ -123,6 +123,8 @@ const cleanAJVErrorMessage = (message: string, path: Optional, suggestio ); } else if (cleanMessage.startsWith(':')) { cleanMessage = cleanMessage.replace(/:\s*/, replaceProperty); + } else if (cleanMessage.startsWith('Property ')) { + cleanMessage = cleanMessage.replace(/(Property\s+)([^\s]+)/, replaceProperty); } else { cleanMessage = `${capitalize(type)} ${cleanMessage}`; } From 0ed1294d1092d0cec681b255176493f58f0ec1bd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ro=C5=BCek?= Date: Tue, 7 Apr 2020 21:22:18 +0200 Subject: [PATCH 04/11] revert: accidently committed tab --- src/rulesets/message.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/rulesets/message.ts b/src/rulesets/message.ts index 8ca5ce825..969c1f5fd 100644 --- a/src/rulesets/message.ts +++ b/src/rulesets/message.ts @@ -14,7 +14,7 @@ export type MessageInterpolator = (str: string, values: IMessageVars) => string; const MessageReplacer = new Replacer(2); -MessageReplacer.addTransformer('double-quotes', (id, value) => (value ? `" ${value}"` : '')); +MessageReplacer.addTransformer('double-quotes', (id, value) => (value ? `"${value}"` : '')); MessageReplacer.addTransformer('single-quotes', (id, value) => (value ? `'${value}'` : '')); MessageReplacer.addTransformer('gravis', (id, value) => (value ? `\`${value}\`` : '')); MessageReplacer.addTransformer('capitalize', (id, value) => capitalize(String(value))); From 95f186f48bd36d8d9aac6aa5a0af60736a83a54a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ro=C5=BCek?= Date: Wed, 8 Apr 2020 15:43:06 +0200 Subject: [PATCH 05/11] style: rename _ to potentialProperty --- src/functions/schema.ts | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/functions/schema.ts b/src/functions/schema.ts index e54e0aaee..7eab525fb 100644 --- a/src/functions/schema.ts +++ b/src/functions/schema.ts @@ -105,8 +105,12 @@ const validators = new (class extends WeakMap { } })(); -const replaceProperty = (substring: string, _: Optional, propertyName: Optional) => { - if (typeof _ === 'string' && propertyName !== void 0) { +const replaceProperty = ( + substring: string, + potentialProperty: Optional, + propertyName: Optional, +) => { + if (typeof potentialProperty === 'string' && propertyName !== void 0) { return `Property \`${propertyName}\``; } From d287724ccad28060ed08d4fb0356d2cb621ab8b9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ro=C5=BCek?= Date: Wed, 8 Apr 2020 15:53:46 +0200 Subject: [PATCH 06/11] fix: add missing dot to the error message --- src/__tests__/linter.test.ts | 6 +++--- src/cli/services/__tests__/linter.test.ts | 10 +++++----- src/rulesets/oas/__tests__/oas2-schema.ts | 2 +- .../oas/functions/__tests__/oasDocumentSchema.test.ts | 2 +- src/rulesets/oas/index.json | 4 ++-- test-harness/scenarios/oas3-schema.scenario | 4 ++-- .../scenarios/results-format-junit.oas3.scenario | 2 +- 7 files changed, 15 insertions(+), 15 deletions(-) diff --git a/src/__tests__/linter.test.ts b/src/__tests__/linter.test.ts index 80a5d3905..a7b3b3562 100644 --- a/src/__tests__/linter.test.ts +++ b/src/__tests__/linter.test.ts @@ -166,7 +166,7 @@ describe('linter', () => { expect(result).toEqual([ expect.objectContaining({ code: 'oas3-schema', - message: 'Property `type` is not expected to be here', + message: 'Property `type` is not expected to be here.', path: ['paths', '/pets', 'get', 'responses', '200', 'headers', 'header-1'], }), expect.objectContaining({ @@ -197,7 +197,7 @@ describe('linter', () => { expect.arrayContaining([ expect.objectContaining({ code: 'oas3-schema', - message: 'Property `type` is not expected to be here', + message: 'Property `type` is not expected to be here.', path: ['paths', '/pets', 'get', 'responses', '200', 'headers', 'header-1'], }), ]), @@ -624,7 +624,7 @@ responses:: !!foo }), expect.objectContaining({ code: 'oas3-schema', - message: 'Property `type` is not expected to be here', + message: 'Property `type` is not expected to be here.', path: ['paths', '/pets', 'get', 'responses', '200', 'headers', 'header-1'], }), expect.objectContaining({ diff --git a/src/cli/services/__tests__/linter.test.ts b/src/cli/services/__tests__/linter.test.ts index 7009a5356..429c603a7 100644 --- a/src/cli/services/__tests__/linter.test.ts +++ b/src/cli/services/__tests__/linter.test.ts @@ -574,7 +574,7 @@ describe('Linter service', () => { }), expect.objectContaining({ code: 'oas2-schema', - message: 'Property `foo` is not expected to be here', + message: 'Property `foo` is not expected to be here.', path: ['paths'], range: { end: { @@ -590,7 +590,7 @@ describe('Linter service', () => { }), expect.objectContaining({ code: 'oas2-schema', - message: 'Property `foo` is not expected to be here', + message: 'Property `foo` is not expected to be here.', path: ['definitions', 'info'], range: { end: { @@ -622,7 +622,7 @@ describe('Linter service', () => { }), expect.objectContaining({ code: 'oas2-schema', - message: '`description` property type should be string', + message: '`description` property type should be string.', path: ['definitions', 'info', 'description'], range: { end: { @@ -657,7 +657,7 @@ describe('Linter service', () => { }), expect.objectContaining({ code: 'oas2-schema', - message: `Object should have required property \`title\``, + message: `Object should have required property \`title\`.`, path: [], range: { end: { @@ -689,7 +689,7 @@ describe('Linter service', () => { }), expect.objectContaining({ code: 'oas2-schema', - message: 'Property `response` is not expected to be here', + message: 'Property `response` is not expected to be here.', path: ['paths', '/test', 'get'], range: { end: { diff --git a/src/rulesets/oas/__tests__/oas2-schema.ts b/src/rulesets/oas/__tests__/oas2-schema.ts index 1c27646d4..de3284c48 100644 --- a/src/rulesets/oas/__tests__/oas2-schema.ts +++ b/src/rulesets/oas/__tests__/oas2-schema.ts @@ -44,7 +44,7 @@ describe('oas2-schema', () => { expect(results).toEqual([ { code: 'oas2-schema', - message: `\`get\` property should have required property \`responses\``, + message: `\`get\` property should have required property \`responses\`.`, path: ['paths', '/test', 'get'], range: { end: { diff --git a/src/rulesets/oas/functions/__tests__/oasDocumentSchema.test.ts b/src/rulesets/oas/functions/__tests__/oasDocumentSchema.test.ts index 89ac104a7..670f8064c 100644 --- a/src/rulesets/oas/functions/__tests__/oasDocumentSchema.test.ts +++ b/src/rulesets/oas/functions/__tests__/oasDocumentSchema.test.ts @@ -66,7 +66,7 @@ describe('oasDocumentSchema', () => { ).toEqual([ { code: 'oas3-schema', - message: '`200` property should have required property `description`', + message: '`200` property should have required property `description`.', path: ['paths', '/user', 'get', 'responses', '200'], severity: DiagnosticSeverity.Error, range: expect.any(Object), diff --git a/src/rulesets/oas/index.json b/src/rulesets/oas/index.json index fa7af20d2..7d5719b13 100644 --- a/src/rulesets/oas/index.json +++ b/src/rulesets/oas/index.json @@ -580,7 +580,7 @@ }, "oas2-schema": { "description": "Validate structure of OpenAPI v2 specification.", - "message": "{{error}}", + "message": "{{error}}.", "recommended": true, "formats": ["oas2"], "severity": 0, @@ -839,7 +839,7 @@ }, "oas3-schema": { "description": "Validate structure of OpenAPI v3 specification.", - "message": "{{error}}", + "message": "{{error}}.", "severity": 0, "formats": ["oas3"], "recommended": true, diff --git a/test-harness/scenarios/oas3-schema.scenario b/test-harness/scenarios/oas3-schema.scenario index 393229060..109812965 100644 --- a/test-harness/scenarios/oas3-schema.scenario +++ b/test-harness/scenarios/oas3-schema.scenario @@ -36,7 +36,7 @@ rules: OpenAPI 3.x detected {document} - 10:11 error oas3-schema Property `type` is not expected to be here - 25:28 error oas3-schema `user_id` property type should be object + 10:11 error oas3-schema Property `type` is not expected to be here. + 25:28 error oas3-schema `user_id` property type should be object. ✖ 2 problems (2 errors, 0 warnings, 0 infos, 0 hints) diff --git a/test-harness/scenarios/results-format-junit.oas3.scenario b/test-harness/scenarios/results-format-junit.oas3.scenario index 7cb4496e1..e0d3bf781 100644 --- a/test-harness/scenarios/results-format-junit.oas3.scenario +++ b/test-harness/scenarios/results-format-junit.oas3.scenario @@ -14,6 +14,6 @@ OpenAPI 3.x detected - + From 3853201d13e89ce3a5fdb54374da7f1cb0c7da1c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ro=C5=BCek?= Date: Wed, 8 Apr 2020 17:18:20 +0200 Subject: [PATCH 07/11] test: add more cases --- .../__tests__/oasDocumentSchema.test.ts | 43 ++++++++++++++++++- .../oas/functions/oasDocumentSchema.ts | 14 +++--- test-harness/scenarios/oas3-schema.scenario | 27 ++++++++++-- 3 files changed, 74 insertions(+), 10 deletions(-) diff --git a/src/rulesets/oas/functions/__tests__/oasDocumentSchema.test.ts b/src/rulesets/oas/functions/__tests__/oasDocumentSchema.test.ts index 670f8064c..b893d8777 100644 --- a/src/rulesets/oas/functions/__tests__/oasDocumentSchema.test.ts +++ b/src/rulesets/oas/functions/__tests__/oasDocumentSchema.test.ts @@ -1,10 +1,10 @@ +import { DiagnosticSeverity } from '@stoplight/types'; import { isOpenApiv2, isOpenApiv3, RuleType, Spectral } from '../../../..'; import { functions } from '../../../../functions'; import { setFunctionContext } from '../../../evaluators'; import { rules } from '../../index.json'; import oasDocumentSchema from '../oasDocumentSchema'; -import { DiagnosticSeverity } from '@stoplight/types/dist'; import * as oas2Schema from '../../schemas/schema.oas2.json'; import * as oas3Schema from '../../schemas/schema.oas3.json'; @@ -44,6 +44,47 @@ describe('oasDocumentSchema', () => { }); describe('given OpenAPI 3 document', () => { + test('validate parameters', async () => { + expect( + await s.run({ + openapi: '3.0.1', + info: { + title: 'response example', + version: '1.0', + }, + paths: { + '/user': { + get: { + responses: { + 200: { + description: 'dummy description', + }, + }, + parameters: [ + { + name: 'module_id', + in: 'bar', + required: true, + schema: { + type: ['string', 'number'], + }, + }, + ], + }, + }, + }, + }), + ).toEqual([ + { + code: 'oas3-schema', + message: '`type` property type should be string.', + path: ['paths', '/user', 'get', 'parameters', '0', 'schema', 'type'], + severity: DiagnosticSeverity.Error, + range: expect.any(Object), + }, + ]); + }); + test('validate responses', async () => { expect( await s.run({ diff --git a/src/rulesets/oas/functions/oasDocumentSchema.ts b/src/rulesets/oas/functions/oasDocumentSchema.ts index b2940cb3c..4c987f96e 100644 --- a/src/rulesets/oas/functions/oasDocumentSchema.ts +++ b/src/rulesets/oas/functions/oasDocumentSchema.ts @@ -2,6 +2,13 @@ import * as AJV from 'ajv'; import { ISchemaOptions } from '../../../functions/schema'; import { IFunction, IFunctionContext } from '../../../types'; +function shouldIgnoreError(error: AJV.ErrorObject) { + return ( + error.keyword === 'oneOf' || + (error.keyword === 'required' && (error.params as AJV.RequiredParams).missingProperty === '$ref') + ); +} + // /shrug function prepareResults(errors: AJV.ErrorObject[]) { for (let i = 0; i < errors.length; i++) { @@ -10,12 +17,7 @@ function prepareResults(errors: AJV.ErrorObject[]) { if (i + 1 < errors.length && errors[i + 1].dataPath === error.dataPath) { errors.splice(i + 1, 1); i--; - } else if ( - i > 0 && - error.keyword === 'required' && - (error.params as AJV.RequiredParams).missingProperty === '$ref' && - errors[i - 1].dataPath.startsWith(error.dataPath) - ) { + } else if (i > 0 && shouldIgnoreError(error) && errors[i - 1].dataPath.startsWith(error.dataPath)) { errors.splice(i, 1); i--; } diff --git a/test-harness/scenarios/oas3-schema.scenario b/test-harness/scenarios/oas3-schema.scenario index 109812965..798c34ffe 100644 --- a/test-harness/scenarios/oas3-schema.scenario +++ b/test-harness/scenarios/oas3-schema.scenario @@ -5,6 +5,8 @@ openapi: 3.0.1 info: title: Example $ref error version: 1.0.0 + contact: + foo: me paths: /user: get: @@ -17,6 +19,11 @@ paths: schema: type: string description: Module ID + - name: module_id + in: bar + required: true + schema: + type: [string, number] responses: "200": description: An Example @@ -26,6 +33,15 @@ paths: type: object properties: user_id: 12345 + application/yaml: + schema: + type: object + properties: + "400": + description: [] + /address: + get: + responses: {} ====asset:ruleset==== extends: [[spectral:oas, off]] rules: @@ -36,7 +52,12 @@ rules: OpenAPI 3.x detected {document} - 10:11 error oas3-schema Property `type` is not expected to be here. - 25:28 error oas3-schema `user_id` property type should be object. + 5:11 error oas3-schema Property `foo` is not expected to be here. + 12:11 error oas3-schema Property `type` is not expected to be here. + 23:18 error oas3-schema `type` property type should be string. + 32:28 error oas3-schema `user_id` property type should be object. + 36:28 error oas3-schema `properties` property type should be object. + 38:23 error oas3-schema `description` property type should be string. + 41:17 error oas3-schema `responses` property minProperties should NOT have fewer than 1 properties. -✖ 2 problems (2 errors, 0 warnings, 0 infos, 0 hints) +✖ 7 problems (7 errors, 0 warnings, 0 infos, 0 hints) From 8cf81f2be9a045529b2f8fb8e9c260ea6f5359ff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ro=C5=BCek?= Date: Wed, 8 Apr 2020 20:53:17 +0200 Subject: [PATCH 08/11] fix: handle security schemes edge case --- .../__tests__/oasDocumentSchema.test.ts | 74 +++++++++++++++++++ .../oas/functions/oasDocumentSchema.ts | 37 +++++++++- 2 files changed, 109 insertions(+), 2 deletions(-) diff --git a/src/rulesets/oas/functions/__tests__/oasDocumentSchema.test.ts b/src/rulesets/oas/functions/__tests__/oasDocumentSchema.test.ts index b893d8777..fee4df047 100644 --- a/src/rulesets/oas/functions/__tests__/oasDocumentSchema.test.ts +++ b/src/rulesets/oas/functions/__tests__/oasDocumentSchema.test.ts @@ -43,6 +43,42 @@ describe('oasDocumentSchema', () => { }); }); + describe('given OpenAPI 2 document', () => { + test('validate security definitions', async () => { + expect( + await s.run({ + swagger: '2.0', + info: { + title: 'response example', + version: '1.0', + }, + paths: { + '/user': { + get: { + responses: { + 200: { + description: 'dummy description', + }, + }, + }, + }, + }, + securityDefinitions: { + basic: null, + }, + }), + ).toEqual([ + { + code: 'oas2-schema', + message: 'Invalid security definition.', + path: ['securityDefinitions', 'basic'], + severity: DiagnosticSeverity.Error, + range: expect.any(Object), + }, + ]); + }); + }); + describe('given OpenAPI 3 document', () => { test('validate parameters', async () => { expect( @@ -85,6 +121,44 @@ describe('oasDocumentSchema', () => { ]); }); + test('validate security schemes', async () => { + expect( + await s.run({ + openapi: '3.0.1', + info: { + title: 'response example', + version: '1.0', + }, + paths: { + '/user': { + get: { + responses: { + 200: { + description: 'dummy description', + }, + }, + }, + }, + }, + components: { + securitySchemes: { + basic: { + foo: 2, + }, + }, + }, + }), + ).toEqual([ + { + code: 'oas3-schema', + message: 'Invalid security scheme.', + path: ['components', 'securitySchemes', 'basic'], + severity: DiagnosticSeverity.Error, + range: expect.any(Object), + }, + ]); + }); + test('validate responses', async () => { expect( await s.run({ diff --git a/src/rulesets/oas/functions/oasDocumentSchema.ts b/src/rulesets/oas/functions/oasDocumentSchema.ts index 4c987f96e..a218374e0 100644 --- a/src/rulesets/oas/functions/oasDocumentSchema.ts +++ b/src/rulesets/oas/functions/oasDocumentSchema.ts @@ -1,6 +1,6 @@ import * as AJV from 'ajv'; import { ISchemaOptions } from '../../../functions/schema'; -import { IFunction, IFunctionContext } from '../../../types'; +import { IFunction, IFunctionContext, IFunctionResult } from '../../../types'; function shouldIgnoreError(error: AJV.ErrorObject) { return ( @@ -9,6 +9,18 @@ function shouldIgnoreError(error: AJV.ErrorObject) { ); } +// this is supposed to cover edge cases, when it's impossible to detect the most appropriate error, i.e. oneOf consisting of more than 3 members, etc. +const ERROR_MAP = [ + { + path: /^components\/securitySchemes\/[^/]+$/, + message: 'Invalid security scheme', + }, + { + path: /^securityDefinitions\/[^/]+$/, + message: 'Invalid security definition', + }, +]; + // /shrug function prepareResults(errors: AJV.ErrorObject[]) { for (let i = 0; i < errors.length; i++) { @@ -24,8 +36,29 @@ function prepareResults(errors: AJV.ErrorObject[]) { } } +function applyManualReplacements(errors: IFunctionResult[]) { + for (const error of errors) { + if (error.path === void 0) continue; + + const joinedPath = error.path.join('/'); + + for (const mappedError of ERROR_MAP) { + if (mappedError.path.test(joinedPath)) { + error.message = mappedError.message; + break; + } + } + } +} + export const oasDocumentSchema: IFunction = function(this: IFunctionContext, targetVal, opts, ...args) { - return this.functions.schema.call(this, targetVal, { ...opts, prepareResults }, ...args); + const errors = this.functions.schema.call(this, targetVal, { ...opts, prepareResults }, ...args); + + if (Array.isArray(errors)) { + applyManualReplacements(errors); + } + + return errors; }; export default oasDocumentSchema; From 7fecad5f45be702400519a72c0e49bcf2c9e65ae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ro=C5=BCek?= Date: Thu, 9 Apr 2020 13:26:00 +0200 Subject: [PATCH 09/11] chore: rename test description --- test-harness/scenarios/oas3-schema.scenario | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test-harness/scenarios/oas3-schema.scenario b/test-harness/scenarios/oas3-schema.scenario index 798c34ffe..317c0468b 100644 --- a/test-harness/scenarios/oas3-schema.scenario +++ b/test-harness/scenarios/oas3-schema.scenario @@ -1,5 +1,5 @@ ====test==== -The amount of enabled rules is printed in a parenthesis. +Prints reasonable error messages for oas3-schema errors. ====document==== openapi: 3.0.1 info: From ebb97d4b8527063716a1f7b572d4704905450faf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ro=C5=BCek?= Date: Fri, 10 Apr 2020 20:48:16 +0200 Subject: [PATCH 10/11] test: add some unit tests --- src/__tests__/linter.jest.test.ts | 16 +- .../__tests__/oasDocumentSchema.test.ts | 157 +++++++++++++++++- .../oas/functions/oasDocumentSchema.ts | 2 +- 3 files changed, 171 insertions(+), 4 deletions(-) diff --git a/src/__tests__/linter.jest.test.ts b/src/__tests__/linter.jest.test.ts index ec36a3e12..ca9539c52 100644 --- a/src/__tests__/linter.jest.test.ts +++ b/src/__tests__/linter.jest.test.ts @@ -360,7 +360,13 @@ describe('Linter', () => { const rules = { 'strings-maxLength': testRuleset.rules['strings-maxLength'], - 'oas3-schema': testRuleset.rules['oas3-schema'], + 'oas3-schema': { + ...testRuleset.rules['oas3-schema'], + then: { + ...testRuleset.rules['oas3-schema'].then, + function: 'oasDocumentSchema', + }, + }, }; spectral.setRuleset({ rules, exceptions: {}, functions: {} }); @@ -397,7 +403,13 @@ describe('Linter', () => { const rules = { 'no-yaml-remote-reference': testRuleset.rules['no-yaml-remote-reference'], - 'oas3-schema': testRuleset.rules['oas3-schema'], + 'oas3-schema': { + ...testRuleset.rules['oas3-schema'], + then: { + ...testRuleset.rules['oas3-schema'].then, + function: 'oasDocumentSchema', + }, + }, }; spectral.setRuleset({ rules, exceptions: {}, functions: {} }); diff --git a/src/rulesets/oas/functions/__tests__/oasDocumentSchema.test.ts b/src/rulesets/oas/functions/__tests__/oasDocumentSchema.test.ts index fee4df047..1ff4ec2f9 100644 --- a/src/rulesets/oas/functions/__tests__/oasDocumentSchema.test.ts +++ b/src/rulesets/oas/functions/__tests__/oasDocumentSchema.test.ts @@ -3,8 +3,9 @@ import { isOpenApiv2, isOpenApiv3, RuleType, Spectral } from '../../../..'; import { functions } from '../../../../functions'; import { setFunctionContext } from '../../../evaluators'; import { rules } from '../../index.json'; -import oasDocumentSchema from '../oasDocumentSchema'; +import oasDocumentSchema, { prepareResults } from '../oasDocumentSchema'; +import { ErrorObject } from 'ajv'; import * as oas2Schema from '../../schemas/schema.oas2.json'; import * as oas3Schema from '../../schemas/schema.oas3.json'; @@ -189,4 +190,158 @@ describe('oasDocumentSchema', () => { ]); }); }); + + describe('prepareResults', () => { + test('given oneOf error one of which is required $ref property missing, picks only one error', () => { + const errors: ErrorObject[] = [ + { + keyword: 'type', + dataPath: '/paths/test/post/parameters/0/schema/type', + schemaPath: '#/properties/type/type', + params: { type: 'string' }, + message: 'should be string', + }, + { + keyword: 'required', + dataPath: '/paths/test/post/parameters/0/schema', + schemaPath: '#/definitions/Reference/required', + params: { missingProperty: '$ref' }, + message: "should have required property '$ref'", + }, + { + keyword: 'oneOf', + dataPath: '/paths/test/post/parameters/0/schema', + schemaPath: '#/properties/schema/oneOf', + params: { passingSchemas: null }, + message: 'should match exactly one schema in oneOf', + }, + ]; + + prepareResults(errors); + + expect(errors).toStrictEqual([ + { + keyword: 'type', + dataPath: '/paths/test/post/parameters/0/schema/type', + schemaPath: '#/properties/type/type', + params: { type: 'string' }, + message: 'should be string', + }, + ]); + }); + + test('given oneOf error one without any $ref property missing, picks all errors', () => { + const errors: ErrorObject[] = [ + { + keyword: 'type', + dataPath: '/paths/test/post/parameters/0/schema/type', + schemaPath: '#/properties/type/type', + params: { type: 'string' }, + message: 'should be string', + }, + { + keyword: 'type', + dataPath: '/paths/test/post/parameters/1/schema/type', + schemaPath: '#/properties/type/type', + params: { type: 'string' }, + message: 'should be string', + }, + { + keyword: 'oneOf', + dataPath: '/paths/test/post/parameters/0/schema', + schemaPath: '#/properties/schema/oneOf', + params: { passingSchemas: null }, + message: 'should match exactly one schema in oneOf', + }, + ]; + + prepareResults(errors); + + expect(errors).toStrictEqual([ + { + keyword: 'type', + dataPath: '/paths/test/post/parameters/0/schema/type', + schemaPath: '#/properties/type/type', + params: { type: 'string' }, + message: 'should be string', + }, + { + dataPath: '/paths/test/post/parameters/1/schema/type', + keyword: 'type', + message: 'should be string', + params: { + type: 'string', + }, + schemaPath: '#/properties/type/type', + }, + { + dataPath: '/paths/test/post/parameters/0/schema', + keyword: 'oneOf', + message: 'should match exactly one schema in oneOf', + params: { + passingSchemas: null, + }, + schemaPath: '#/properties/schema/oneOf', + }, + ]); + }); + + test('given errors with different data paths, picks all errors', () => { + const errors: ErrorObject[] = [ + { + keyword: 'type', + dataPath: '/paths/test/post/parameters/0/schema/type', + schemaPath: '#/properties/type/type', + params: { type: 'string' }, + message: 'should be string', + }, + { + keyword: 'required', + dataPath: '/paths/foo/post/parameters/0/schema', + schemaPath: '#/definitions/Reference/required', + params: { missingProperty: '$ref' }, + message: "should have required property '$ref'", + }, + { + keyword: 'oneOf', + dataPath: '/paths/baz/post/parameters/0/schema', + schemaPath: '#/properties/schema/oneOf', + params: { passingSchemas: null }, + message: 'should match exactly one schema in oneOf', + }, + ]; + + prepareResults(errors); + + expect(errors).toStrictEqual([ + { + dataPath: '/paths/test/post/parameters/0/schema/type', + keyword: 'type', + message: 'should be string', + params: { + type: 'string', + }, + schemaPath: '#/properties/type/type', + }, + { + dataPath: '/paths/foo/post/parameters/0/schema', + keyword: 'required', + message: "should have required property '$ref'", + params: { + missingProperty: '$ref', + }, + schemaPath: '#/definitions/Reference/required', + }, + { + dataPath: '/paths/baz/post/parameters/0/schema', + keyword: 'oneOf', + message: 'should match exactly one schema in oneOf', + params: { + passingSchemas: null, + }, + schemaPath: '#/properties/schema/oneOf', + }, + ]); + }); + }); }); diff --git a/src/rulesets/oas/functions/oasDocumentSchema.ts b/src/rulesets/oas/functions/oasDocumentSchema.ts index a218374e0..9759e0c54 100644 --- a/src/rulesets/oas/functions/oasDocumentSchema.ts +++ b/src/rulesets/oas/functions/oasDocumentSchema.ts @@ -22,7 +22,7 @@ const ERROR_MAP = [ ]; // /shrug -function prepareResults(errors: AJV.ErrorObject[]) { +export function prepareResults(errors: AJV.ErrorObject[]) { for (let i = 0; i < errors.length; i++) { const error = errors[i]; From e06c95414917bfe75ddec565295a280c28f5aa8b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ro=C5=BCek?= Date: Sun, 12 Apr 2020 19:07:55 +0200 Subject: [PATCH 11/11] chore: document oasDocumentSchema fn --- src/rulesets/oas/functions/oasDocumentSchema.ts | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/rulesets/oas/functions/oasDocumentSchema.ts b/src/rulesets/oas/functions/oasDocumentSchema.ts index 9759e0c54..25bb71bee 100644 --- a/src/rulesets/oas/functions/oasDocumentSchema.ts +++ b/src/rulesets/oas/functions/oasDocumentSchema.ts @@ -4,12 +4,15 @@ import { IFunction, IFunctionContext, IFunctionResult } from '../../../types'; function shouldIgnoreError(error: AJV.ErrorObject) { return ( + // oneOf is a fairly error as we have 2 options to choose from for most of the time. error.keyword === 'oneOf' || + // the required $ref is entirely useless, since oas-schema rules operate on resolved content, so there won't be any $refs in the document (error.keyword === 'required' && (error.params as AJV.RequiredParams).missingProperty === '$ref') ); } -// this is supposed to cover edge cases, when it's impossible to detect the most appropriate error, i.e. oneOf consisting of more than 3 members, etc. +// this is supposed to cover edge cases we need to cover manually, when it's impossible to detect the most appropriate error, i.e. oneOf consisting of more than 3 members, etc. +// note, more errors can be included if certain messages reported by AJV are not quite meaningful const ERROR_MAP = [ { path: /^components\/securitySchemes\/[^/]+$/, @@ -21,7 +24,12 @@ const ERROR_MAP = [ }, ]; -// /shrug +// The function removes irrelevant (aka misleading, confusing, useless, whatever you call it) errors. +// There are a few exceptions, i.e. security components I covered manually, +// yet apart from them we usually deal with a relatively simple scenario that can be literally expressed as: "either proper value of $ref property". +// The $ref part is never going to be interesting for us, because both oas-schema rules operate on resolved content, so we won't have any $refs left. +// As you can see, what we deal here wit is actually not really oneOf anymore - it's always the first member of oneOf we match against. +// That being said, we always strip both oneOf and $ref, since we are always interested in the first error. export function prepareResults(errors: AJV.ErrorObject[]) { for (let i = 0; i < errors.length; i++) { const error = errors[i];