From ff5b1cf833442662c8eeba898bf26a3e6a3c6c06 Mon Sep 17 00:00:00 2001 From: Phil Adams Date: Tue, 21 Feb 2023 19:09:31 -0600 Subject: [PATCH] fix(rulesets): avoid false errors from ajv This commit modifies the oasExample function so that example fields are removed from the schema to be used for validation. This is needed because the presence of an "example" field in a schema confuses ajv in certain scenarios. References: - https://github.com/stoplightio/spectral/issues/2081 - https://github.com/stoplightio/spectral/issues/2140 - https://github.com/ajv-validator/ajv/issues/1426 --- package.json | 3 +- packages/functions/src/schema/index.ts | 5 +- .../functions/__tests__/oasExample.test.ts | 451 ++++++++++++++++++ .../rulesets/src/oas/functions/oasExample.ts | 54 +++ 4 files changed, 511 insertions(+), 2 deletions(-) create mode 100644 packages/rulesets/src/oas/functions/__tests__/oasExample.test.ts diff --git a/package.json b/package.json index 840d9544f..7488675a1 100644 --- a/package.json +++ b/package.json @@ -39,7 +39,8 @@ "test.karma": "karma start", "prepare": "husky install", "prerelease": "patch-package", - "release": "yarn prerelease && yarn workspaces foreach run release" + "release": "yarn prerelease && yarn workspaces foreach run release", + "jest": "jest" }, "workspaces": { "packages": [ diff --git a/packages/functions/src/schema/index.ts b/packages/functions/src/schema/index.ts index a7262517d..955c799b9 100644 --- a/packages/functions/src/schema/index.ts +++ b/packages/functions/src/schema/index.ts @@ -69,7 +69,10 @@ export default createRulesetFunction( // let's ignore any $ref errors if schema fn is provided with already resolved content, // if our resolver fails to resolve them, // ajv is unlikely to do it either, since it won't have access to the whole document, but a small portion of it - if (!rule.resolved || !(ex instanceof MissingRefError)) { + // We specifically check that "rule" is truthy below because "rule" might be undefined/null if this + // code is called from testcases. + const ignoreError = rule?.resolved && ex instanceof MissingRefError; + if (!ignoreError) { results.push({ message: ex.message, path, diff --git a/packages/rulesets/src/oas/functions/__tests__/oasExample.test.ts b/packages/rulesets/src/oas/functions/__tests__/oasExample.test.ts new file mode 100644 index 000000000..899e5b517 --- /dev/null +++ b/packages/rulesets/src/oas/functions/__tests__/oasExample.test.ts @@ -0,0 +1,451 @@ +import { oas3, oas3_0 } from '@stoplight/spectral-formats'; +import { DeepPartial } from '@stoplight/types'; +import oasExample, { Options as ExampleOptions } from '../oasExample'; +import { RulesetFunctionContext } from '@stoplight/spectral-core/src'; + +const schemaOpts: ExampleOptions = { + schemaField: '$', + oasVersion: 3, + type: 'schema', +}; +const mediaOpts: ExampleOptions = { + schemaField: 'schema', + oasVersion: 3, + type: 'media', +}; +const docFormats = { + formats: new Set([oas3, oas3_0]), +}; + +/** + * Runs the oasExample() custom rule function to perform a single test. + * @param target the object (media type or schema) containing an example/default value + * @param ruleOptions the options to be passed to oasExample() + * @param context the spectral context object to pass to oasExample() + * @returns an array of errors, or [] if no errors occurred + */ +function runRule(testData: Record, ruleOptions: ExampleOptions) { + const context: DeepPartial = { + path: [], + documentInventory: {}, + document: docFormats, + }; + + return oasExample(testData, ruleOptions, context as RulesetFunctionContext); +} + +describe('oasExample', () => { + describe('should return no errors', () => { + describe('example/default value in schema', () => { + test('valid "example" object', () => { + const schema = { + type: 'object', + properties: { + foo: { + type: 'number', + }, + bar: { + type: 'string', + }, + }, + required: ['foo'], + example: { + foo: 38, + bar: 'foo', + }, + }; + + const results = runRule(schema, schemaOpts); + expect(results).toHaveLength(0); + }); + test('valid "default" string', () => { + const schema = { + type: 'string', + pattern: 'xyz-.*', + minLength: 4, + maxLength: 6, + default: 'xyz-99', + }; + + const results = runRule(schema, schemaOpts); + expect(results).toHaveLength(0); + }); + test('valid "example" integer', () => { + const schema = { + type: 'integer', + example: 74, + }; + + const results = runRule(schema, schemaOpts); + expect(results).toHaveLength(0); + }); + test('scenario: "resolves to more than one schema"', () => { + // This test data is from https://github.com/stoplightio/spectral/issues/2081 and + // demonstrates a scenario in which ajv returns the dreaded + // "reference <...> resolves to more than one schema" false error. + // Without the fix to the oasExample() function, this test will fail. + // The reason that it fails is due to the way in which ajv handles unknown + // properties found in the schema (e.g. "example" - it's not actually part of JSONSchema), + // and the way it gives special treatment to the "id" property. Ajv gets confused by + // the fact that there are multiple example objects that each contain a property named "id" + // with the value 'bf23bc970b78d27691e8' (repeating example values is probably not an uncommon + // use-case for openapi authors if you think about it). + // So, without the fix to oasExample(), the test below will fail with this result: + // [ + // { + // "message": "reference \"bf23bc970b78d27691e8\" resolves to more than one schema", + // "path": ["example"] + // } + // ] + // However, if you rename the "id" properties to something else, the rule returns []. + // Likewise, if you change the value of "id" in one of the examples (so they are no longer equal) + // the rule returns []. + // And of course, with the fix to oasExample() in place, the rule will also return []. + const schema = { + type: 'object', + required: ['items'], + allOf: [ + { + type: 'object', + properties: { + items: { + type: 'array', + items: { + type: 'object', + required: ['id', 'url'], + properties: { + id: { + type: 'string', + }, + url: { + type: 'string', + format: 'uri', + }, + }, + example: { + id: 'bf23bc970b78d27691e8', + url: 'https://api.example.com/banking/accounts/bf23bc970b78d27691e8', + }, + }, + }, + }, + }, + ], + example: { + items: [ + { + id: 'bf23bc970b78d27691e8', + url: 'https://api.example.com/banking/accounts/bf23bc970b78d27691e8', + }, + { + id: '8d27691e8bf23bc970b7', + url: 'https://api.example.com/banking/accounts/8d27691e8bf23bc970b7', + }, + ], + }, + }; + + const results = runRule(schema, schemaOpts); + expect(results).toHaveLength(0); + }); + }); + describe('example/examples value in mediatype', () => { + test('valid "example" object', () => { + const mediaType = { + schema: { + type: 'object', + properties: { + foo: { + type: 'number', + }, + bar: { + type: 'string', + }, + }, + required: ['foo'], + }, + example: { + foo: 38, + bar: 'foo', + }, + }; + + const results = runRule(mediaType, mediaOpts); + expect(results).toHaveLength(0); + }); + test('valid "examples" object', () => { + const mediaType = { + schema: { + type: 'object', + properties: { + foo: { + type: 'number', + }, + bar: { + type: 'string', + }, + }, + required: ['foo'], + }, + examples: { + first: { + value: { + foo: 38, + bar: 'foo', + }, + }, + second: { + value: { + foo: 26, + bar: 'baz', + }, + }, + }, + }; + + const results = runRule(mediaType, mediaOpts); + expect(results).toHaveLength(0); + }); + test('valid "example" string', () => { + const mediaType = { + schema: { + type: 'string', + pattern: 'xyz-.*', + minLength: 4, + maxLength: 8, + }, + example: 'xyz-9999', + }; + + const results = runRule(mediaType, mediaOpts); + expect(results).toHaveLength(0); + }); + test('valid "examples" string', () => { + const mediaType = { + schema: { + type: 'string', + pattern: 'id-.*', + minLength: 4, + maxLength: 8, + }, + examples: { + first: { + value: 'id-1', + }, + second: { + value: 'id-99999', + }, + third: { + value: 'id-38', + }, + }, + }; + + const results = runRule(mediaType, mediaOpts); + expect(results).toHaveLength(0); + }); + test('scenario: "resolves to more than one schema"', () => { + // This test data was adapted from https://github.com/stoplightio/spectral/issues/2140. + const mediaType = { + schema: { + properties: { + bars: { + description: 'Array of bars!', + type: 'array', + items: { + oneOf: [ + { + type: 'object', + description: 'a real bar!', + required: ['id'], + properties: { + id: { + description: 'The ID for this real bar', + type: 'string', + }, + }, + example: { + id: '6d353a0f-aeb1-4ae1-832e-1110d10981bb', + }, + }, + { + description: 'not a real bar!', + not: { + type: 'object', + description: 'a real bar!', + required: ['id'], + properties: { + id: { + description: 'The ID for this real bar', + type: 'string', + }, + }, + example: { + id: '6d353a0f-aeb1-4ae1-832e-1110d10981bb', + }, + }, + }, + ], + }, + }, + }, + }, + example: { + bars: [{ id: '6d353a0f-aeb1-4ae1-832e-1110d10981bb' }], + }, + }; + + const results = runRule(mediaType, mediaOpts); + expect(results).toHaveLength(0); + }); + }); + }); + describe('should return errors', () => { + describe('example/default value in schema', () => { + test('invalid "example" object', () => { + const schema = { + type: 'object', + properties: { + foo: { + type: 'number', + }, + bar: { + type: 'string', + }, + }, + required: ['foo', 'bar'], + example: { + foo: 38, + bar: 26, + }, + }; + + const results = runRule(schema, schemaOpts); + expect(results).toHaveLength(1); + + expect(results[0].path.join('.')).toBe('example.bar'); + expect(results[0].message).toBe(`"bar" property type must be string`); + }); + test('invalid "default" string', () => { + const schema = { + type: 'string', + pattern: 'xyz-.*', + minLength: 4, + maxLength: 8, + default: 'xyz-99999', + }; + + const results = runRule(schema, schemaOpts); + expect(results).toHaveLength(1); + expect(results[0].message).toBe(`"default" property must not have more than 8 characters`); + expect(results[0].path.join('.')).toBe('default'); + }); + }); + describe('example/examples value in mediatype', () => { + test('invalid "example" object', () => { + const mediaType = { + schema: { + type: 'object', + properties: { + foo: { + type: 'number', + }, + bar: { + type: 'string', + }, + }, + required: ['foo', 'bar'], + }, + example: { + foo: 38, + }, + }; + + const results = runRule(mediaType, mediaOpts); + expect(results).toHaveLength(1); + expect(results[0].message).toBe(`"example" property must have required property "bar"`); + expect(results[0].path.join('.')).toBe('example'); + }); + test('invalid "examples" object', () => { + const mediaType = { + schema: { + type: 'object', + properties: { + foo: { + type: 'number', + }, + bar: { + type: 'string', + }, + }, + required: ['foo', 'bar'], + }, + examples: { + first: { + value: { + foo: 38, + }, + }, + second: { + value: { + foo: 'bar', + bar: 'foo', + }, + }, + }, + }; + + const results = runRule(mediaType, mediaOpts); + expect(results).toHaveLength(2); + + expect(results[0].message).toBe(`"value" property must have required property "bar"`); + expect(results[0].path.join('.')).toBe('examples.first.value'); + + expect(results[1].message).toBe(`"foo" property type must be number`); + expect(results[1].path.join('.')).toBe('examples.second.value.foo'); + }); + test('invalid "example" string', () => { + const mediaType = { + schema: { + type: 'string', + pattern: 'xyz-.*', + minLength: 4, + maxLength: 8, + }, + example: 'xyz-99999', + }; + + const results = runRule(mediaType, mediaOpts); + expect(results).toHaveLength(1); + expect(results[0].message).toBe(`"example" property must not have more than 8 characters`); + expect(results[0].path.join('.')).toBe('example'); + }); + test('invalid "examples" string', () => { + const mediaType = { + schema: { + type: 'string', + pattern: 'xyz-.*', + minLength: 4, + maxLength: 8, + default: 'xyz-99', + }, + examples: { + first: { + value: 'xyz-99999', + }, + second: { + value: 38, + }, + }, + }; + + const results = runRule(mediaType, mediaOpts); + expect(results).toHaveLength(2); + expect(results[0].message).toBe(`"value" property must not have more than 8 characters`); + expect(results[0].path.join('.')).toBe('examples.first.value'); + expect(results[1].message).toBe(`"value" property type must be string`); + expect(results[1].path.join('.')).toBe('examples.second.value'); + }); + }); + }); +}); diff --git a/packages/rulesets/src/oas/functions/oasExample.ts b/packages/rulesets/src/oas/functions/oasExample.ts index 1ad1dcd9b..1f0e62cb7 100644 --- a/packages/rulesets/src/oas/functions/oasExample.ts +++ b/packages/rulesets/src/oas/functions/oasExample.ts @@ -110,6 +110,54 @@ function* getSchemaValidationItems( } } +/** + * Modifies 'schema' (and all its sub-schemas) to remove all "example" fields. + * In this context, "sub-schemas" refers to all schemas reachable from 'schema' + * (e.g. properties, additionalProperties, allOf/anyOf/oneOf, not, items, etc.) + * @param schema the schema to be "de-examplified" + * @returns 'schema' with example fields removed + */ +function deExamplify(schema: Record): Record { + if ('items' in schema && typeof schema.items === 'object') { + schema.items = deExamplify(schema.items as Record); + } + + if (Array.isArray(schema.allOf)) { + for (let i = 0; i < schema.allOf.length; i++) { + schema.allOf[i] = deExamplify(schema.allOf[i] as Record); + } + } + if (Array.isArray(schema.anyOf)) { + for (let i = 0; i < schema.anyOf.length; i++) { + schema.anyOf[i] = deExamplify(schema.anyOf[i] as Record); + } + } + if (Array.isArray(schema.oneOf)) { + for (let i = 0; i < schema.oneOf.length; i++) { + schema.oneOf[i] = deExamplify(schema.oneOf[i] as Record); + } + } + if ('not' in schema && typeof schema.not === 'object') { + schema.not = deExamplify(schema.not as Record); + } + + if ('additionalProperties' in schema && typeof schema.additionalProperties === 'object') { + schema.additionalProperties = deExamplify(schema.additionalProperties as Record); + } + + if ('properties' in schema && typeof schema.properties === 'object') { + for (const propName in schema.properties) { + schema.properties[propName] = deExamplify(schema.properties[propName] as Record); + } + } + + if ('example' in schema) { + delete schema.example; + } + + return schema; +} + export default createRulesetFunction, Options>( { input: { @@ -149,6 +197,12 @@ export default createRulesetFunction, Options>( delete schemaOpts.schema.required; } + // Make a deep copy of the schema and then remove all the "example" fields from it. + // This is to avoid problems down in "ajv" which does the actual schema validation. + // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment + schemaOpts.schema = JSON.parse(JSON.stringify(schemaOpts.schema)); + schemaOpts.schema = deExamplify(schemaOpts.schema); + for (const validationItem of validationItems) { const result = oasSchema(validationItem.value, schemaOpts, { ...context,