diff --git a/.changeset/fine-cows-begin.md b/.changeset/fine-cows-begin.md new file mode 100644 index 0000000000..e490096e0f --- /dev/null +++ b/.changeset/fine-cows-begin.md @@ -0,0 +1,6 @@ +--- +"@redocly/openapi-core": patch +"@redocly/cli": patch +--- + +Fixed the `no-required-schema-properties-undefined` rule to report when a required property is not defined in every `oneOf`/`anyOf` branch. diff --git a/packages/core/src/rules/common/__tests__/no-required-schema-properties-undefined.test.ts b/packages/core/src/rules/common/__tests__/no-required-schema-properties-undefined.test.ts index a47bfb3699..04167ac42d 100644 --- a/packages/core/src/rules/common/__tests__/no-required-schema-properties-undefined.test.ts +++ b/packages/core/src/rules/common/__tests__/no-required-schema-properties-undefined.test.ts @@ -6,7 +6,7 @@ import { lintDocument } from '../../../lint.js'; import { BaseResolver } from '../../../resolve.js'; describe('no-required-schema-properties-undefined', () => { - it('should report if one or more of the required properties are undefined', async () => { + it('should report each undefined required property', async () => { const document = parseYamlToDocument( outdent` openapi: 3.0.0 @@ -18,6 +18,7 @@ describe('no-required-schema-properties-undefined', () => { - name - id - test + - test2 properties: id: type: integer @@ -45,7 +46,20 @@ describe('no-required-schema-properties-undefined', () => { "source": "foobar.yaml", }, ], - "message": "Required property 'test' is undefined.", + "message": "Required property 'test' is not defined.", + "ruleId": "no-required-schema-properties-undefined", + "severity": "error", + "suggest": [], + }, + { + "location": [ + { + "pointer": "#/components/schemas/Pet/required/3", + "reportOnKey": false, + "source": "foobar.yaml", + }, + ], + "message": "Required property 'test2' is not defined.", "ruleId": "no-required-schema-properties-undefined", "severity": "error", "suggest": [], @@ -54,7 +68,7 @@ describe('no-required-schema-properties-undefined', () => { `); }); - it('should report if one or more of the required properties are undefined, including allOf nested schemas', async () => { + it('should report undefined property not found in allOf', async () => { const document = parseYamlToDocument( outdent` openapi: 3.0.0 @@ -94,7 +108,7 @@ describe('no-required-schema-properties-undefined', () => { "source": "foobar.yaml", }, ], - "message": "Required property 'test' is undefined.", + "message": "Required property 'test' is not defined.", "ruleId": "no-required-schema-properties-undefined", "severity": "error", "suggest": [], @@ -103,7 +117,7 @@ describe('no-required-schema-properties-undefined', () => { `); }); - it('should not report if one or more of the required properties are defined when used in schema with allOf keyword', async () => { + it('should not report when required property is in allOf $ref sibling', async () => { const document = parseYamlToDocument( outdent` openapi: 3.0.0 @@ -142,7 +156,7 @@ describe('no-required-schema-properties-undefined', () => { expect(replaceSourceWithRef(results)).toMatchInlineSnapshot(`[]`); }); - it('should not report if one of more properties are defined in great-grandparent schema', async () => { + it('should not report when required property is in allOf sibling via anyOf ancestor', async () => { const document = parseYamlToDocument( outdent` openapi: 3.0.0 @@ -179,7 +193,7 @@ describe('no-required-schema-properties-undefined', () => { expect(replaceSourceWithRef(results)).toMatchInlineSnapshot(`[]`); }); - it('should report if one sub property is marked as required but undefined', async () => { + it('should report when parent property is required in a nested schema', async () => { const document = parseYamlToDocument( outdent` openapi: 3.0.0 @@ -217,7 +231,7 @@ describe('no-required-schema-properties-undefined', () => { "source": "foobar.yaml", }, ], - "message": "Required property 'foo' is undefined.", + "message": "Required property 'foo' is not defined.", "ruleId": "no-required-schema-properties-undefined", "severity": "error", "suggest": [], @@ -226,7 +240,7 @@ describe('no-required-schema-properties-undefined', () => { `); }); - it('should report if one or more of the required properties are undefined when used in schema with allOf keyword', async () => { + it('should report undefined property even when allOf $ref is present', async () => { const document = parseYamlToDocument( outdent` openapi: 3.0.0 @@ -272,109 +286,7 @@ describe('no-required-schema-properties-undefined', () => { "source": "foobar.yaml", }, ], - "message": "Required property 'surname' is undefined.", - "ruleId": "no-required-schema-properties-undefined", - "severity": "error", - "suggest": [], - }, - ] - `); - }); - - it('should not report required properties are present after resolving $refs when used in schema with allOf keyword', async () => { - const document = parseYamlToDocument( - outdent` - openapi: 3.0.0 - components: - schemas: - Cat: - description: A representation of a cat - allOf: - - $ref: '#/components/schemas/Pet' - - type: object - properties: - huntingSkill: - type: string - required: - - huntingSkill - required: - - name - Pet: - type: object - required: - - photoUrls - properties: - name: - type: string - photoUrls: - type: string - `, - 'foobar.yaml' - ); - - const results = await lintDocument({ - externalRefResolver: new BaseResolver(), - document, - config: await createConfig({ rules: { 'no-required-schema-properties-undefined': 'error' } }), - }); - - expect(replaceSourceWithRef(results)).toMatchInlineSnapshot(`[]`); - }); - - it('should report with few messages if more than one of the required properties are undefined', async () => { - const document = parseYamlToDocument( - outdent` - openapi: 3.0.0 - components: - schemas: - Pet: - type: object - required: - - name - - id - - test - - test2 - properties: - id: - type: integer - format: int64 - name: - type: string - example: doggie - `, - 'foobar.yaml' - ); - - const results = await lintDocument({ - externalRefResolver: new BaseResolver(), - document, - config: await createConfig({ rules: { 'no-required-schema-properties-undefined': 'error' } }), - }); - - expect(replaceSourceWithRef(results)).toMatchInlineSnapshot(` - [ - { - "location": [ - { - "pointer": "#/components/schemas/Pet/required/2", - "reportOnKey": false, - "source": "foobar.yaml", - }, - ], - "message": "Required property 'test' is undefined.", - "ruleId": "no-required-schema-properties-undefined", - "severity": "error", - "suggest": [], - }, - { - "location": [ - { - "pointer": "#/components/schemas/Pet/required/3", - "reportOnKey": false, - "source": "foobar.yaml", - }, - ], - "message": "Required property 'test2' is undefined.", + "message": "Required property 'surname' is not defined.", "ruleId": "no-required-schema-properties-undefined", "severity": "error", "suggest": [], @@ -460,7 +372,7 @@ describe('no-required-schema-properties-undefined', () => { expect(replaceSourceWithRef(results)).toMatchInlineSnapshot(`[]`); }); - it('should report if there is any required schema that is undefined, including checking nested allOf schemas', async () => { + it('should report undefined property in schema with nested allOf $ref chain', async () => { const document = parseYamlToDocument( outdent` openapi: 3.0.0 @@ -516,7 +428,7 @@ describe('no-required-schema-properties-undefined', () => { "source": "foobar.yaml", }, ], - "message": "Required property 'test' is undefined.", + "message": "Required property 'test' is not defined.", "ruleId": "no-required-schema-properties-undefined", "severity": "error", "suggest": [], @@ -525,7 +437,7 @@ describe('no-required-schema-properties-undefined', () => { `); }); - it('should not report if required property is present in one of the nested reference schemas', async () => { + it('should not report when the required property is in deeply nested allOf $ref chain', async () => { const document = parseYamlToDocument( outdent` openapi: 3.0.0 @@ -624,16 +536,16 @@ describe('no-required-schema-properties-undefined', () => { "source": "foobar.yaml", }, ], - "message": "Required property 'name' is undefined.", + "message": "Required property 'name' is not defined.", "ruleId": "no-required-schema-properties-undefined", "severity": "error", "suggest": [], }, - ] + ] `); }); - it('should report if one or more of the required properties are undefined when used in schema with anyOf keyword', async () => { + it('should NOT report if one or more of the required properties are defined when used in schema with anyOf keyword', async () => { const document = parseYamlToDocument( outdent` openapi: 3.0.0 @@ -642,22 +554,14 @@ describe('no-required-schema-properties-undefined', () => { Cat: description: A representation of a cat anyOf: - - $ref: '#/components/schemas/Pet' - - type: object - properties: - huntingSkill: - type: string - required: - - huntingSkill + - required: - name - Pet: - type: object - required: - - photoUrls + - required: + - huntingSkill properties: name: type: string - photoUrls: + huntingSkill: type: string `, 'foobar.yaml' @@ -669,52 +573,30 @@ describe('no-required-schema-properties-undefined', () => { config: await createConfig({ rules: { 'no-required-schema-properties-undefined': 'error' } }), }); - expect(replaceSourceWithRef(results)).toMatchInlineSnapshot(` - [ - { - "location": [ - { - "pointer": "#/components/schemas/Cat/anyOf/1/required/1", - "reportOnKey": false, - "source": "foobar.yaml", - }, - ], - "message": "Required property 'name' is undefined.", - "ruleId": "no-required-schema-properties-undefined", - "severity": "error", - "suggest": [], - }, - ] - `); + expect(replaceSourceWithRef(results)).toMatchInlineSnapshot(`[]`); }); - it('should NOT report if one or more of the required properties are undefined when used in schema with allOf keyword', async () => { + it('should NOT report when required property exists in all anyOf branches', async () => { const document = parseYamlToDocument( outdent` openapi: 3.0.0 components: schemas: Cat: - description: A representation of a cat - allOf: - - $ref: '#/components/schemas/Pet' - - type: object - properties: + type: object + anyOf: + - properties: huntingSkill: type: string - required: - - huntingSkill + name: + type: string + - properties: + name: + type: string + curiosityLevel: + type: number required: - name - Pet: - type: object - required: - - photoUrls - properties: - name: - type: string - photoUrls: - type: string `, 'foobar.yaml' ); @@ -728,24 +610,92 @@ describe('no-required-schema-properties-undefined', () => { expect(replaceSourceWithRef(results)).toMatchInlineSnapshot(`[]`); }); - it('should NOT report if one or more of the required properties are defined when used in schema with anyOf keyword', async () => { + it('should report when required properties are not declared in all anyOf branches', async () => { const document = parseYamlToDocument( outdent` openapi: 3.0.0 components: schemas: Cat: - description: A representation of a cat + type: object anyOf: - - required: - - name - - required: - - huntingSkill + - properties: + huntingSkill: + type: string + - properties: + name: + type: string + required: + - name + - huntingSkill + `, + 'foobar.yaml' + ); + + const results = await lintDocument({ + externalRefResolver: new BaseResolver(), + document, + config: await createConfig({ rules: { 'no-required-schema-properties-undefined': 'error' } }), + }); + + expect(replaceSourceWithRef(results)).toMatchInlineSnapshot(` + [ + { + "location": [ + { + "pointer": "#/components/schemas/Cat/required/0", + "reportOnKey": false, + "source": "foobar.yaml", + }, + ], + "message": "Required property 'name' is not defined.", + "ruleId": "no-required-schema-properties-undefined", + "severity": "error", + "suggest": [], + }, + { + "location": [ + { + "pointer": "#/components/schemas/Cat/required/1", + "reportOnKey": false, + "source": "foobar.yaml", + }, + ], + "message": "Required property 'huntingSkill' is not defined.", + "ruleId": "no-required-schema-properties-undefined", + "severity": "error", + "suggest": [], + }, + ] + `); + }); + + it('should NOT report when anyOf branches share a $ref that defines the required property', async () => { + const document = parseYamlToDocument( + outdent` + openapi: 3.0.0 + components: + schemas: + Animal: + type: object + required: + - name + anyOf: + - allOf: + - $ref: '#/components/schemas/Base' + - properties: + kind: + enum: [cat] + - allOf: + - $ref: '#/components/schemas/Base' + - properties: + kind: + enum: [dog] + Base: + type: object properties: name: type: string - huntingSkill: - type: string `, 'foobar.yaml' ); @@ -759,24 +709,26 @@ describe('no-required-schema-properties-undefined', () => { expect(replaceSourceWithRef(results)).toMatchInlineSnapshot(`[]`); }); - it('should NOT report if one or more of the required properties are defined when used in schema with oneOf keyword', async () => { + it('should NOT report when required property is in allOf sibling reached via oneOf parent', async () => { const document = parseYamlToDocument( outdent` openapi: 3.0.0 components: schemas: Cat: - description: A representation of a cat - oneOf: - - required: - - name - - required: - - huntingSkill - properties: - name: - type: string - huntingSkill: - type: string + allOf: + - properties: + name: + type: string + - oneOf: + - required: + - name + - huntingSkill + properties: + huntingSkill: + type: string + - required: + - name `, 'foobar.yaml' ); @@ -844,7 +796,7 @@ describe('no-required-schema-properties-undefined', () => { "source": "foobar.yaml", }, ], - "message": "Required property 'missing-required-prop' is undefined.", + "message": "Required property 'missing-required-prop' is not defined.", "ruleId": "no-required-schema-properties-undefined", "severity": "error", "suggest": [], @@ -889,7 +841,7 @@ describe('no-required-schema-properties-undefined', () => { "source": "foobar.yaml", }, ], - "message": "Required property 'missingRequired' is undefined.", + "message": "Required property 'missingRequired' is not defined.", "ruleId": "no-required-schema-properties-undefined", "severity": "error", "suggest": [], @@ -937,7 +889,7 @@ describe('no-required-schema-properties-undefined', () => { "source": "foobar.yaml", }, ], - "message": "Required property 'missing-required' is undefined.", + "message": "Required property 'missing-required' is not defined.", "ruleId": "no-required-schema-properties-undefined", "severity": "error", "suggest": [], @@ -977,7 +929,7 @@ describe('no-required-schema-properties-undefined', () => { "source": "foobar.yaml", }, ], - "message": "Required property 'name' is undefined.", + "message": "Required property 'name' is not defined.", "ruleId": "no-required-schema-properties-undefined", "severity": "error", "suggest": [], diff --git a/packages/core/src/rules/common/no-required-schema-properties-undefined.ts b/packages/core/src/rules/common/no-required-schema-properties-undefined.ts index d3272312ac..c8751b2622 100644 --- a/packages/core/src/rules/common/no-required-schema-properties-undefined.ts +++ b/packages/core/src/rules/common/no-required-schema-properties-undefined.ts @@ -1,11 +1,15 @@ -import { isRef } from '../../ref-utils.js'; import type { Oas3Schema, Oas3_1Schema } from '../../typings/openapi.js'; import type { Oas2Schema } from '../../typings/swagger.js'; import { getOwn } from '../../utils/get-own.js'; +import { isNotEmptyArray } from '../../utils/is-not-empty-array.js'; import type { Async2Rule, Async3Rule, Arazzo1Rule, Oas2Rule, Oas3Rule } from '../../visitors.js'; import type { UserContext } from '../../walk.js'; +import { resolveSchema } from '../utils.js'; -type AnySchema = Oas3Schema | Oas3_1Schema | Oas2Schema; +type AnySchema = + | Oas3Schema + | Oas3_1Schema + | (Oas2Schema & { anyOf?: undefined; oneOf?: undefined }); export const NoRequiredSchemaPropertiesUndefined: | Oas3Rule @@ -19,91 +23,72 @@ export const NoRequiredSchemaPropertiesUndefined: leave(_: AnySchema) { parents.pop(); }, - enter(schema: AnySchema, { location, report, resolve }: UserContext) { - parents.push(schema); - if (!schema.required) return; - const visitedSchemas: Set = new Set(); + enter(currentSchema: AnySchema, ctx: UserContext) { + parents.push(currentSchema); + if (!currentSchema.required) return; - const elevateProperties = ( - schema: AnySchema, - from?: string, - includeFirstLevelExclusiveSchemas: boolean = true - ): Record => { - // Check if the schema has been visited before processing it - if (visitedSchemas.has(schema)) { - return {}; + const hasProperty = ( + schemaOrRef: AnySchema | undefined, + propertyName: string, + visited: Set, + resolveFrom?: string + ): boolean => { + const { schema, location } = resolveSchema(schemaOrRef, ctx, resolveFrom); + if (!schema || visited.has(schema)) return false; + visited.add(schema); + + if (schema.properties && getOwn(schema.properties, propertyName) !== undefined) { + return true; } - visitedSchemas.add(schema); - if (isRef(schema)) { - const resolved = resolve(schema, from); - if (!resolved.node) { - return {}; - } - return elevateProperties(resolved.node, resolved.location?.source.absoluteRef); + if (schema.allOf?.some((s) => hasProperty(s, propertyName, visited, location))) { + return true; } - return Object.assign( - {}, - schema.properties, - ...(schema.allOf?.map((s) => elevateProperties(s, from)) ?? []), - ...(('anyOf' in schema && includeFirstLevelExclusiveSchemas - ? schema.anyOf?.map((s) => elevateProperties(s, from)) - : undefined) ?? []), - ...(('oneOf' in schema && includeFirstLevelExclusiveSchemas - ? schema.oneOf?.map((s) => elevateProperties(s, from)) - : undefined) ?? []) - ); + if ( + isNotEmptyArray(schema.anyOf) && + schema.anyOf.every((s) => hasProperty(s, propertyName, new Set(visited), location)) + ) { + return true; + } + + if ( + isNotEmptyArray(schema.oneOf) && + schema.oneOf.every((s) => hasProperty(s, propertyName, new Set(visited), location)) + ) { + return true; + } + + return false; }; - /** - * The index to use to lookup grand parent schemas when dealing with composed schemas. - * @summary The current schema should always end up with under ../anyOf/1, if we get multiple ancestors, they should always be a multiple. - */ - const locationLookupIndex = 2; - const getParentSchema = (lookupIndex: number): AnySchema | undefined => { - lookupIndex++; - if (!parents || parents.length < lookupIndex) return undefined; - const parent = parents[parents.length - lookupIndex]; - return parent; + const isCompositionChild = (parent: AnySchema, child: AnySchema): boolean => { + const matchesChild = (s: AnySchema) => resolveSchema(s, ctx).schema === child; + return !!( + parent.allOf?.some(matchesChild) || + parent.anyOf?.some(matchesChild) || + parent.oneOf?.some(matchesChild) + ); }; - const recursivelyGetParentProperties = ( - splitLocation: string[], - parentLookupIndex: number = 0 - ): Record | undefined => { - const isMemberOfComposedType = - splitLocation.length > locationLookupIndex && - !isNaN(parseInt(splitLocation[splitLocation.length - 1])) && - !!/(allOf|oneOf|anyOf)/.exec(splitLocation[splitLocation.length - locationLookupIndex]); - const parentSchema = isMemberOfComposedType - ? getParentSchema(++parentLookupIndex) - : undefined; - const grandParentProperties = - splitLocation.length >= locationLookupIndex + locationLookupIndex - ? recursivelyGetParentProperties( - splitLocation.slice(0, -locationLookupIndex), - parentLookupIndex - ) - : {}; - return parentSchema - ? { - ...elevateProperties(parentSchema, undefined, false), - ...grandParentProperties, - } + + const findCompositionRoot = (i: number, child: AnySchema): AnySchema | undefined => { + if (i < 0) return undefined; + const parent = parents[i]; + return isCompositionChild(parent, child) + ? (findCompositionRoot(i - 1, parent) ?? parent) : undefined; }; - const allProperties = elevateProperties(schema); - const parentProperties = recursivelyGetParentProperties(location.pointer.split('/')); + const compositionRoot = findCompositionRoot(parents.length - 2, currentSchema); - for (const [i, requiredProperty] of schema.required.entries()) { + for (const [i, requiredProperty] of currentSchema.required.entries()) { if ( - (!allProperties || getOwn(allProperties, requiredProperty) === undefined) && - (!parentProperties || getOwn(parentProperties, requiredProperty) === undefined) + !hasProperty(currentSchema, requiredProperty, new Set()) && + !hasProperty(compositionRoot, requiredProperty, new Set()) ) { - report({ - message: `Required property '${requiredProperty}' is undefined.`, - location: location.child(['required', i]), + ctx.report({ + message: `Required property '${requiredProperty}' is not defined.`, + location: ctx.location.child(['required', i]), }); } }