From e31da7ff5605b628a6a9056e2f6411e68e84dce1 Mon Sep 17 00:00:00 2001 From: Theo Truong Date: Thu, 4 Jul 2024 12:57:57 -0500 Subject: [PATCH] AjvErrorsParser - Collapse multiple `required` errors into one - Collapse multiple `additionalProperties` and `unevaluatedProperties` errors into one, and print out which properties are prohibited. - Make `enum` errors print out the supported enum values. Signed-off-by: Theo Truong --- CHANGELOG.md | 1 + tools/src/_utils/AjvErrorsParser.ts | 98 +++++++++++++++++++ tools/src/linter/SchemasValidator.ts | 32 +++--- .../linter/components/base/FileValidator.ts | 7 +- tools/src/tester/SchemaValidator.ts | 7 +- tools/src/tester/StoryValidator.ts | 11 ++- tools/tests/_utils/AjvErrorsParser.test.ts | 62 ++++++++++++ tools/tests/linter/SchemasValidator.test.ts | 12 +-- .../linter/SupersededOperationsFile.test.ts | 21 +--- tools/tests/tester/StoryValidator.test.ts | 5 +- .../fixtures/evals/failed/invalid_data.yaml | 5 +- tools/tests/tester/test.test.ts | 2 +- 12 files changed, 205 insertions(+), 58 deletions(-) create mode 100644 tools/src/_utils/AjvErrorsParser.ts create mode 100644 tools/tests/_utils/AjvErrorsParser.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 55248104d..4504da50a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -32,6 +32,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) - Added support for `application/yaml` responses ([#363](https://github.com/opensearch-project/opensearch-api-specification/pull/363)) - Added test for search with seq_no_primary_term ([#367](https://github.com/opensearch-project/opensearch-api-specification/pull/367)) - Added a linter for parameter sorting ([#369](https://github.com/opensearch-project/opensearch-api-specification/pull/369)) +- Added AjvErrorsParser to print more informative error messages ([#364](https://github.com/opensearch-project/opensearch-api-specification/issues/364)) - Added support for `application/cbor` responses ([#371](https://github.com/opensearch-project/opensearch-api-specification/pull/371)) ### Changed diff --git a/tools/src/_utils/AjvErrorsParser.ts b/tools/src/_utils/AjvErrorsParser.ts new file mode 100644 index 000000000..4be4b052e --- /dev/null +++ b/tools/src/_utils/AjvErrorsParser.ts @@ -0,0 +1,98 @@ +/* +* Copyright OpenSearch Contributors +* SPDX-License-Identifier: Apache-2.0 +* +* The OpenSearch Contributors require contributions made to +* this file be licensed under the Apache-2.0 license or a +* compatible open source license. +*/ + +import { Ajv2019, ErrorsTextOptions, ErrorObject } from 'ajv/dist/2019' +import _ from 'lodash' + +export default class AjvErrorsParser { + private readonly ajv: Ajv2019 + private readonly options: ErrorsTextOptions + + constructor(ajv: Ajv2019, options: ErrorsTextOptions = {}) { + this.ajv = ajv + this.options = { separator: ' --- ', ...options } + } + + parse(errors: ErrorObject[] | undefined | null): string { + const error_groups = this.#group_errors(errors ?? []) + const parsed_errors = [ + this.#prohibited_property_error(error_groups.prohibited), + this.#required_property_error(error_groups.required), + this.#enum_error(error_groups.enum), + ...error_groups.others + ].filter(e => e != null) as ErrorObject[] + return this.ajv.errorsText(parsed_errors, this.options) + } + + #group_errors(errors: ErrorObject[]): { required: ErrorObject[], prohibited: ErrorObject[], enum: ErrorObject[], others: ErrorObject[] } { + const categories = { + required: [] as ErrorObject[], + prohibited: [] as ErrorObject[], + enum: [] as ErrorObject[], + others: [] as ErrorObject[] + } + _.values(_.groupBy(errors, 'instancePath')).forEach((path_errors) => { + for (const error of path_errors) { + switch (error.keyword) { + case 'required': + categories.required.push(error) + break + case 'unevaluatedProperties': + case 'additionalProperties': + categories.prohibited.push(error) + break + case 'enum': + categories.enum.push(error) + break + default: + categories.others.push(error) + } + } + }) + return categories + } + + #prohibited_property_error(errors: ErrorObject[]): ErrorObject | undefined { + if (errors.length === 0) return + const properties = errors.map((error) => error.params.additionalProperty ?? error.params.unevaluatedProperty).join(', ') + return { + keyword: 'prohibited', + instancePath: errors[0].instancePath, + schemaPath: errors[0].schemaPath, + params: { additionalProperty: errors[0].instancePath.split('/').pop() }, + message: `contains unsupported properties: ${properties}` + } + } + + #required_property_error(errors: ErrorObject[]): ErrorObject | undefined { + if (errors.length === 0) return + const properties = errors.map((error) => error.params.missingProperty).join(', ') + return { + keyword: 'required', + instancePath: errors[0].instancePath, + schemaPath: errors[0].schemaPath, + params: { missingProperty: errors[0].instancePath.split('/').pop() }, + message: `MUST contain the missing properties: ${properties}` + } + } + + #enum_error(errors: ErrorObject[]): ErrorObject | undefined { + if (errors.length === 0) return + const allowed_values = errors[0].params.allowedValues.join(', ') + return { + keyword: 'enum', + instancePath: errors[0].instancePath, + schemaPath: errors[0].schemaPath, + params: errors[0].params, + message: `MUST be equal to one of the allowed values: ${allowed_values}` + } + } + + +} \ No newline at end of file diff --git a/tools/src/linter/SchemasValidator.ts b/tools/src/linter/SchemasValidator.ts index 9390f09a1..78fcd680a 100644 --- a/tools/src/linter/SchemasValidator.ts +++ b/tools/src/linter/SchemasValidator.ts @@ -7,16 +7,12 @@ * compatible open source license. */ -import AJV from 'ajv' +import { Ajv2019 as AJV } from 'ajv/dist/2019' import addFormats from 'ajv-formats' +import AjvErrorsParser from "../_utils/AjvErrorsParser"; import OpenApiMerger from '../merger/OpenApiMerger' import { type ValidationError } from '../types' -import { type Logger } from '../Logger' - -const IGNORED_ERROR_PREFIXES = [ - 'can\'t resolve reference', // errors in referenced schemas will also cause reference errors - 'discriminator: oneOf subschemas' // known bug in ajv: https://github.com/ajv-validator/ajv/issues/2281 -] +import { Logger, LogLevel } from '../Logger' const ADDITIONAL_KEYWORDS = [ 'x-version-added', @@ -30,6 +26,7 @@ export default class SchemasValidator { root_folder: string spec: Record = {} ajv: AJV + errors_parser: AjvErrorsParser constructor (root_folder: string, logger: Logger) { this.logger = logger @@ -37,10 +34,11 @@ export default class SchemasValidator { this.ajv = new AJV({ strict: true, discriminator: true }) addFormats(this.ajv) for (const keyword of ADDITIONAL_KEYWORDS) this.ajv.addKeyword(keyword) + this.errors_parser = new AjvErrorsParser(this.ajv, {}) } validate (): ValidationError[] { - this.spec = new OpenApiMerger(this.root_folder, this.logger).merge().components as Record + this.spec = new OpenApiMerger(this.root_folder, new Logger(LogLevel.error)).merge().components as Record const named_schemas_errors = this.validate_named_schemas() if (named_schemas_errors.length > 0) return named_schemas_errors return [ @@ -53,7 +51,7 @@ export default class SchemasValidator { validate_named_schemas (): ValidationError[] { return Object.entries(this.spec.schemas as Record).map(([key, _schema]) => { const schema = _schema as Record - const error = this.validate_schema(schema, `#/components/schemas/${key}`) + const error = this.validate_schema(schema) if (error == null) return const file = `schemas/${key.split(':')[0]}.yaml` @@ -100,16 +98,10 @@ export default class SchemasValidator { }).filter(e => e != null) as ValidationError[] } - validate_schema (schema: Record, key: string | undefined = undefined): Error | undefined { + validate_schema (schema: Record): string | undefined { if (schema == null || schema.$ref != null) return - try { - if (key != null) this.ajv.addSchema(schema, key) - this.ajv.compile(schema) - } catch (_e: any) { - const error = _e as Error - for (const prefix of IGNORED_ERROR_PREFIXES) if (error.message.startsWith(prefix)) return - return error - } + const valid = this.ajv.validateSchema(schema) + if (valid !== true) return this.errors_parser.parse(this.ajv.errors) } group_to_namespace (group: string): string { @@ -118,7 +110,7 @@ export default class SchemasValidator { return namespace ?? '_core' } - error (file: string, location: string, error: Error): ValidationError { - return { file, location, message: error.message } + error (file: string, location: string, message: string): ValidationError { + return { file, location, message } } } diff --git a/tools/src/linter/components/base/FileValidator.ts b/tools/src/linter/components/base/FileValidator.ts index a6f9da82e..dcd6159d6 100644 --- a/tools/src/linter/components/base/FileValidator.ts +++ b/tools/src/linter/components/base/FileValidator.ts @@ -10,9 +10,10 @@ import ValidatorBase from './ValidatorBase' import { type ValidationError } from 'types' import { type OpenAPIV3 } from 'openapi-types' -import { read_yaml, to_json } from '../../../helpers' +import { read_yaml } from '../../../helpers' import AJV from 'ajv' import addFormats from 'ajv-formats' +import AjvErrorsParser from '../../../_utils/AjvErrorsParser' export default class FileValidator extends ValidatorBase { file_path: string @@ -63,9 +64,11 @@ export default class FileValidator extends ValidatorBase { const schema = read_yaml(json_schema_path) const ajv = new AJV({ schemaId: 'id' }) addFormats(ajv) + const errors_parser = new AjvErrorsParser(ajv) const validator = ajv.compile(schema) if (!validator(this.spec())) { - return this.error(`File content does not match JSON schema found in '${json_schema_path}':\n ${to_json(validator.errors)}`) + const message = errors_parser.parse(validator.errors) + return this.error(`File content does not match JSON schema found in '${json_schema_path}': ${message}`) } } } diff --git a/tools/src/tester/SchemaValidator.ts b/tools/src/tester/SchemaValidator.ts index 9fdc0dfb6..07802517a 100644 --- a/tools/src/tester/SchemaValidator.ts +++ b/tools/src/tester/SchemaValidator.ts @@ -7,9 +7,10 @@ * compatible open source license. */ -import AJV from 'ajv' +import AJV from 'ajv/dist/2019' import ajv_errors from 'ajv-errors' import addFormats from 'ajv-formats' +import AjvErrorsParser from '../_utils/AjvErrorsParser' import { type OpenAPIV3 } from 'openapi-types' import { type Evaluation, Result } from './types/eval.types' import { Logger } from 'Logger' @@ -25,6 +26,7 @@ const ADDITIONAL_KEYWORDS = [ export default class SchemaValidator { private readonly ajv: AJV + private readonly errors_parser: AjvErrorsParser private readonly logger: Logger constructor (spec: OpenAPIV3.Document, logger: Logger) { @@ -35,6 +37,7 @@ export default class SchemaValidator { ajv_errors(this.ajv, { singleError: true }) const schemas = spec.components?.schemas ?? {} for (const key in schemas) this.ajv.addSchema(schemas[key], `#/components/schemas/${key}`) + this.errors_parser = new AjvErrorsParser(this.ajv) } validate (schema: OpenAPIV3.SchemaObject, data: any): Evaluation { @@ -47,7 +50,7 @@ export default class SchemaValidator { } return { result: valid ? Result.PASSED : Result.FAILED, - message: valid ? undefined : this.ajv.errorsText(validate.errors) + message: valid ? undefined : this.errors_parser.parse(validate.errors) } } } diff --git a/tools/src/tester/StoryValidator.ts b/tools/src/tester/StoryValidator.ts index 6b15228d5..996535453 100644 --- a/tools/src/tester/StoryValidator.ts +++ b/tools/src/tester/StoryValidator.ts @@ -9,20 +9,23 @@ import { Result, StoryEvaluation, StoryFile } from "./types/eval.types"; import * as path from "path"; -import { Ajv2019, ValidateFunction } from 'ajv/dist/2019' +import { Ajv2019 as AJV, ValidateFunction } from 'ajv/dist/2019' import addFormats from 'ajv-formats' import { read_yaml } from "../helpers"; +import AjvErrorsParser from "../_utils/AjvErrorsParser"; export default class StoryValidator { private static readonly SCHEMA_FILE = path.resolve("./json_schemas/test_story.schema.yaml") - private readonly ajv: Ajv2019 + private readonly ajv: AJV + private readonly errors_parser: AjvErrorsParser private readonly validate_schema: ValidateFunction constructor() { - this.ajv = new Ajv2019({ allErrors: true, strict: false }) + this.ajv = new AJV({ allErrors: true, strict: false }) addFormats(this.ajv) const schema = read_yaml(StoryValidator.SCHEMA_FILE) this.validate_schema = this.ajv.compile(schema) + this.errors_parser = new AjvErrorsParser(this.ajv) } validate(story_file: StoryFile): StoryEvaluation | undefined { @@ -40,7 +43,7 @@ export default class StoryValidator { #validate_schema(story_file: StoryFile): StoryEvaluation | undefined { const valid = this.validate_schema(story_file.story) - if (!valid) return this.#invalid(story_file, this.ajv.errorsText(this.validate_schema.errors)) + if (!valid) return this.#invalid(story_file, this.errors_parser.parse(this.validate_schema.errors)) } #invalid({ story, display_path, full_path }: StoryFile, message: string): StoryEvaluation { diff --git a/tools/tests/_utils/AjvErrorsParser.test.ts b/tools/tests/_utils/AjvErrorsParser.test.ts new file mode 100644 index 000000000..60b16bb89 --- /dev/null +++ b/tools/tests/_utils/AjvErrorsParser.test.ts @@ -0,0 +1,62 @@ +/* +* Copyright OpenSearch Contributors +* SPDX-License-Identifier: Apache-2.0 +* +* The OpenSearch Contributors require contributions made to +* this file be licensed under the Apache-2.0 license or a +* compatible open source license. +*/ + +import { Ajv2019 } from "ajv/dist/2019"; +import AjvErrorsParser from "../../src/_utils/AjvErrorsParser"; + +describe('AjvErrorsParser', () => { + const ajv = new Ajv2019({ allErrors: true, strict: false }) + const parser = new AjvErrorsParser(ajv, { separator: ' | ' , dataVar: 'Obj' }) + const schema = { + type: 'object', + additionalProperties: false, + required: [ 'a_boolean', 'a_number', 'an_array', 'an_enum', 'a_compound_object'], + properties: { + a_boolean: { type: 'boolean' }, + a_number: { type: 'number' }, + a_nullable_string: { type: ['string', 'null'] }, + a_non_nullable_string: { type: 'string' }, + an_array: { type: 'array', items: { type: 'string' }, minItems: 1 }, + an_enum: { type: 'string', enum: ['a', 'b', 'c'] }, + a_compound_object: { + unevaluatedProperties: false, + allOf: [ + { type: 'object', properties: { a: { type: 'string' } } }, + { type: 'object', properties: { b: { type: 'number' } } }, + ], + } + } + } + + const data = { + an_array: [], + an_enum: 'd', + a_compound_object: { stranger: 'danger', hello: 'world' }, + space_odyssey: 42, + thirteen_sentinels: 426 + } + + const func = ajv.compile(schema) + func(data) + + it('can parse multiple errors', () => { + expect(parser.parse(func.errors)).toEqual( + 'Obj contains unsupported properties: space_odyssey, thirteen_sentinels, stranger, hello | ' + + 'Obj MUST contain the missing properties: a_boolean, a_number | ' + + 'Obj/an_enum MUST be equal to one of the allowed values: a, b, c | ' + + 'Obj/an_array must NOT have fewer than 1 items' + ) + }); + + it('can parse empty errors', () => { + const empty_func = ajv.compile({ type: 'object' }) + empty_func({}) + expect(parser.parse(empty_func.errors)).toEqual(ajv.errorsText(undefined)) + }); +}); \ No newline at end of file diff --git a/tools/tests/linter/SchemasValidator.test.ts b/tools/tests/linter/SchemasValidator.test.ts index c2d48dc4f..c0929b6ec 100644 --- a/tools/tests/linter/SchemasValidator.test.ts +++ b/tools/tests/linter/SchemasValidator.test.ts @@ -16,12 +16,12 @@ test('validate() - named_schemas', () => { { file: 'schemas/actions.yaml', location: '#/components/schemas/Bark', - message: 'schema is invalid: data/type must be equal to one of the allowed values, data/type must be array, data/type must match a schema in anyOf' + message: 'data/type MUST be equal to one of the allowed values: array, boolean, integer, null, number, object, string --- data/type must be array --- data/type must match a schema in anyOf' }, { file: 'schemas/animals.yaml', location: '#/components/schemas/Dog', - message: 'schema is invalid: data/type must be equal to one of the allowed values, data/type must be array, data/type must match a schema in anyOf' + message: 'data/type MUST be equal to one of the allowed values: array, boolean, integer, null, number, object, string --- data/type must be array --- data/type must match a schema in anyOf' } ]) }) @@ -32,22 +32,22 @@ test('validate() - anonymous_schemas', () => { { file: '_global_parameters.yaml', location: 'human', - message: 'schema is invalid: data/type must be equal to one of the allowed values, data/type must be array, data/type must match a schema in anyOf' + message: 'data/type MUST be equal to one of the allowed values: array, boolean, integer, null, number, object, string --- data/type must be array --- data/type must match a schema in anyOf' }, { file: 'namespaces/_core.yaml', location: '#/components/parameters/adopt::path.docket', - message: 'schema is invalid: data/type must be equal to one of the allowed values, data/type must be array, data/type must match a schema in anyOf' + message: 'data/type MUST be equal to one of the allowed values: array, boolean, integer, null, number, object, string --- data/type must be array --- data/type must match a schema in anyOf' }, { file: 'namespaces/adopt.yaml', location: '#/components/requestBodies/adopt/content/application/json', - message: 'schema is invalid: data/type must be equal to one of the allowed values, data/type must be array, data/type must match a schema in anyOf' + message: 'data/type MUST be equal to one of the allowed values: array, boolean, integer, null, number, object, string --- data/type must be array --- data/type must match a schema in anyOf' }, { file: 'namespaces/_core.yaml', location: '#/components/responses/adopt@200/content/application/json', - message: 'schema is invalid: data/type must be equal to one of the allowed values, data/type must be array, data/type must match a schema in anyOf' + message: 'data/type MUST be equal to one of the allowed values: array, boolean, integer, null, number, object, string --- data/type must be array --- data/type must match a schema in anyOf' } ]) }) diff --git a/tools/tests/linter/SupersededOperationsFile.test.ts b/tools/tests/linter/SupersededOperationsFile.test.ts index 423b68520..db68fdc1b 100644 --- a/tools/tests/linter/SupersededOperationsFile.test.ts +++ b/tools/tests/linter/SupersededOperationsFile.test.ts @@ -15,26 +15,7 @@ describe('validate()', () => { expect(validator.validate()).toEqual([ { file: 'superseded_operations/invalid_schema.yaml', - message: "File content does not match JSON schema found in './json_schemas/_superseded_operations.schema.yaml':\n " + - JSON.stringify([ - { - "instancePath": "/~1hello~1world/operations/1", - "schemaPath": "#/patternProperties/%5E~1/properties/operations/items/enum", - "keyword": "enum", - "params": { - "allowedValues": [ - "GET", - "POST", - "PUT", - "DELETE", - "HEAD", - "OPTIONS", - "PATCH" - ] - }, - "message": "must be equal to one of the allowed values" - } - ], null, 2), + message: "File content does not match JSON schema found in './json_schemas/_superseded_operations.schema.yaml': data/~1hello~1world/operations/1 MUST be equal to one of the allowed values: GET, POST, PUT, DELETE, HEAD, OPTIONS, PATCH" }, ]) }) diff --git a/tools/tests/tester/StoryValidator.test.ts b/tools/tests/tester/StoryValidator.test.ts index c3a42d372..4a755b7a2 100644 --- a/tools/tests/tester/StoryValidator.test.ts +++ b/tools/tests/tester/StoryValidator.test.ts @@ -29,7 +29,10 @@ describe('StoryValidator', () => { test('invalid story', () => { const evaluation = validate('tools/tests/tester/fixtures/invalid_story.yaml') expect(evaluation?.result).toBe('ERROR') - expect(evaluation?.message).toBe("Invalid Story: data/epilogues/0 must NOT have unevaluated properties, data/chapters/0 must have required property 'method', data/chapters/1/method must be equal to one of the allowed values") + expect(evaluation?.message).toBe("Invalid Story: " + + "data/epilogues/0 contains unsupported properties: response --- " + + "data/chapters/0 MUST contain the missing properties: method --- " + + "data/chapters/1/method MUST be equal to one of the allowed values: GET, PUT, POST, DELETE, PATCH, HEAD, OPTIONS") }) test('valid story', () => { diff --git a/tools/tests/tester/fixtures/evals/failed/invalid_data.yaml b/tools/tests/tester/fixtures/evals/failed/invalid_data.yaml index 3bf4abbac..7cdaf2973 100644 --- a/tools/tests/tester/fixtures/evals/failed/invalid_data.yaml +++ b/tools/tests/tester/fixtures/evals/failed/invalid_data.yaml @@ -33,7 +33,7 @@ chapters: result: PASSED request_body: result: FAILED - message: data must NOT have additional properties + message: 'data contains unsupported properties: aliases' response: status: result: PASSED @@ -58,7 +58,8 @@ chapters: message: expected acknowledged='false', got 'true', missing shards_acknowledged='true' payload_schema: result: FAILED - message: data must NOT have additional properties + message: 'data contains unsupported properties: acknowledged' + epilogues: - title: DELETE /books diff --git a/tools/tests/tester/test.test.ts b/tools/tests/tester/test.test.ts index bab385367..b82e2abd7 100644 --- a/tools/tests/tester/test.test.ts +++ b/tools/tests/tester/test.test.ts @@ -41,7 +41,7 @@ test('displays story filename', () => { test('invalid story', () => { expect(spec(['--tests', 'tools/tests/tester/fixtures/invalid_story.yaml']).stdout).toContain( - `${ansi.gray("(Invalid Story: data/epilogues/0 must NOT have unevaluated properties, data/chapters/0 must have required property 'method', data/chapters/1/method must be equal to one of the allowed values)")}` + `\x1b[90m(Invalid Story:` ) })