diff --git a/tools/spectral/ipa/CONTRIBUTING.md b/tools/spectral/ipa/CONTRIBUTING.md index 3c9c2df596..d7b90b31d0 100644 --- a/tools/spectral/ipa/CONTRIBUTING.md +++ b/tools/spectral/ipa/CONTRIBUTING.md @@ -24,6 +24,9 @@ Instead of using the [Spectral overrides approach](https://docs.stoplight.io/doc "xgen-IPA-104-resource-has-GET": "Legacy API, not used by infrastructure-as-code tooling", } ``` + +Some rules allow an exception to be inherited from a parent component. For example, an exception for `xgen-IPA-112-boolean-field-names-avoid-is-prefix` can be set on a schema instead of on each of its fields, and applies to all fields defined directly on that schema. Fields in nested inline schemas or in `allOf`, `anyOf` or `oneOf` composition need their own exception. Inherited exceptions are still counted in the IPA metrics. See the rule's implementation details in the [rulesets README](https://github.com/mongodb/openapi/tree/main/tools/spectral/ipa/rulesets/README.md) to check whether it supports inherited exceptions. + --- ## Testing diff --git a/tools/spectral/ipa/__tests__/IPA112BooleanFieldNamesAvoidIsPrefix.test.js b/tools/spectral/ipa/__tests__/IPA112BooleanFieldNamesAvoidIsPrefix.test.js index a353e018aa..dc7b6c97cd 100644 --- a/tools/spectral/ipa/__tests__/IPA112BooleanFieldNamesAvoidIsPrefix.test.js +++ b/tools/spectral/ipa/__tests__/IPA112BooleanFieldNamesAvoidIsPrefix.test.js @@ -177,6 +177,193 @@ testRule('xgen-IPA-112-boolean-field-names-avoid-is-prefix', [ }, errors: [], }, + { + name: 'schema with schema-level exception - applies to all fields of the schema', + document: { + components: { + schemas: { + SchemaName: { + type: 'object', + 'x-xgen-IPA-exception': { + 'xgen-IPA-112-boolean-field-names-avoid-is-prefix': 'Reason', + }, + properties: { + isEnabled: { type: 'boolean' }, + isActive: { type: 'boolean' }, + visible: { type: 'boolean' }, + nested: { + type: 'object', + properties: { + isNested: { type: 'boolean' }, + }, + }, + }, + }, + OtherSchema: { + type: 'object', + properties: { + isOther: { type: 'boolean' }, + }, + }, + }, + }, + paths: { + '/users': { + post: { + requestBody: { + content: { + 'application/vnd.atlas.2024-01-01+json': { + schema: { + type: 'object', + 'x-xgen-IPA-exception': { + 'xgen-IPA-112-boolean-field-names-avoid-is-prefix': 'Reason', + }, + properties: { + isAvailable: { type: 'boolean' }, + }, + }, + }, + }, + }, + }, + }, + }, + }, + errors: [ + { + code: 'xgen-IPA-112-boolean-field-names-avoid-is-prefix', + message: 'Boolean field "isNested" should not use the "is" prefix. Use "nested" instead.', + path: ['components', 'schemas', 'SchemaName', 'properties', 'nested', 'properties', 'isNested'], + severity: DiagnosticSeverity.Error, + }, + { + code: 'xgen-IPA-112-boolean-field-names-avoid-is-prefix', + message: 'Boolean field "isOther" should not use the "is" prefix. Use "other" instead.', + path: ['components', 'schemas', 'OtherSchema', 'properties', 'isOther'], + severity: DiagnosticSeverity.Error, + }, + ], + }, + { + name: 'schema with both schema-level and field-level exception - field-level exception is unnecessary', + document: { + components: { + schemas: { + SchemaName: { + type: 'object', + 'x-xgen-IPA-exception': { + 'xgen-IPA-112-boolean-field-names-avoid-is-prefix': 'Reason', + }, + properties: { + isEnabled: { + type: 'boolean', + 'x-xgen-IPA-exception': { + 'xgen-IPA-112-boolean-field-names-avoid-is-prefix': 'Reason', + }, + }, + }, + }, + }, + }, + }, + errors: [ + { + code: 'xgen-IPA-112-boolean-field-names-avoid-is-prefix', + message: + 'This component is covered by an exception on its containing schema and does not need its own exception. Please remove the exception.', + path: [ + 'components', + 'schemas', + 'SchemaName', + 'properties', + 'isEnabled', + 'x-xgen-IPA-exception', + 'xgen-IPA-112-boolean-field-names-avoid-is-prefix', + ], + severity: DiagnosticSeverity.Error, + }, + ], + }, + { + name: 'schema with unnecessary schema-level exception - no violating fields', + document: { + components: { + schemas: { + SchemaName: { + type: 'object', + 'x-xgen-IPA-exception': { + 'xgen-IPA-112-boolean-field-names-avoid-is-prefix': 'Reason', + }, + properties: { + enabled: { type: 'boolean' }, + active: { type: 'boolean' }, + name: { type: 'string' }, + }, + }, + }, + }, + }, + errors: [ + { + code: 'xgen-IPA-112-boolean-field-names-avoid-is-prefix', + message: + 'This component does not need an exception because none of its fields violate the rule. Please remove the exception.', + path: [ + 'components', + 'schemas', + 'SchemaName', + 'x-xgen-IPA-exception', + 'xgen-IPA-112-boolean-field-names-avoid-is-prefix', + ], + severity: DiagnosticSeverity.Error, + }, + ], + }, + { + name: 'schema with schema-level exception whose only violating field is nested - exception is unnecessary', + document: { + components: { + schemas: { + SchemaName: { + type: 'object', + 'x-xgen-IPA-exception': { + 'xgen-IPA-112-boolean-field-names-avoid-is-prefix': 'Reason', + }, + properties: { + name: { type: 'string' }, + nested: { + type: 'object', + properties: { + isNested: { type: 'boolean' }, + }, + }, + }, + }, + }, + }, + }, + errors: [ + { + code: 'xgen-IPA-112-boolean-field-names-avoid-is-prefix', + message: + 'This component does not need an exception because none of its fields violate the rule. Please remove the exception.', + path: [ + 'components', + 'schemas', + 'SchemaName', + 'x-xgen-IPA-exception', + 'xgen-IPA-112-boolean-field-names-avoid-is-prefix', + ], + severity: DiagnosticSeverity.Error, + }, + { + code: 'xgen-IPA-112-boolean-field-names-avoid-is-prefix', + message: 'Boolean field "isNested" should not use the "is" prefix. Use "nested" instead.', + path: ['components', 'schemas', 'SchemaName', 'properties', 'nested', 'properties', 'isNested'], + severity: DiagnosticSeverity.Error, + }, + ], + }, { name: 'schema with referenced property types', document: { @@ -320,4 +507,29 @@ testRule('xgen-IPA-112-boolean-field-names-avoid-is-prefix', [ }, ], }, + { + name: 'non-boolean field with its own exception and a schema-level exception', + document: { + components: { + schemas: { + SchemaName: { + type: 'object', + 'x-xgen-IPA-exception': { + 'xgen-IPA-112-boolean-field-names-avoid-is-prefix': 'Reason', + }, + properties: { + isEnabled: { type: 'boolean' }, + name: { + type: 'string', + 'x-xgen-IPA-exception': { + 'xgen-IPA-112-boolean-field-names-avoid-is-prefix': 'Reason', + }, + }, + }, + }, + }, + }, + }, + errors: [], + }, ]); diff --git a/tools/spectral/ipa/__tests__/utils/collectionUtils.test.js b/tools/spectral/ipa/__tests__/utils/collectionUtils.test.js index 6e98fa5042..57de22163d 100644 --- a/tools/spectral/ipa/__tests__/utils/collectionUtils.test.js +++ b/tools/spectral/ipa/__tests__/utils/collectionUtils.test.js @@ -1,6 +1,7 @@ import { afterEach, describe, expect, it, jest } from '@jest/globals'; import { evaluateAndCollectAdoptionStatus, + evaluateAndCollectAdoptionStatusWithInheritedException, evaluateAndCollectAdoptionStatusWithoutExceptions, } from '../../rulesets/functions/utils/collectionUtils.js'; import collector from '../../metrics/collector.js'; @@ -162,4 +163,52 @@ describe('tools/spectral/ipa/rulesets/functions/utils/collectionUtils.js', () => expect(collector.add).toHaveBeenCalledWith(TEST_ENTRY_TYPE.ADOPTION, testPath, testRuleName); }); }); + + describe('evaluateAndCollectAdoptionStatusWithInheritedException', () => { + const testRuleName = 'xgen-IPA-XXX-rule'; + const testPath = ['components', 'schemas', 'Schema', 'properties', 'field']; + const testParentObject = { + properties: { field: {} }, + 'x-xgen-IPA-exception': { + 'xgen-IPA-XXX-rule': 'reason', + }, + }; + + afterEach(() => { + jest.clearAllMocks(); + }); + + it('returns empty and collects exceptions for the object when there are errors', () => { + const testErrors = [ + { + path: testPath, + message: TEST_ERROR_MESSAGE, + }, + ]; + + const result = evaluateAndCollectAdoptionStatusWithInheritedException( + testErrors, + testRuleName, + testParentObject, + testPath + ); + + expect(result).toEqual(undefined); + expect(collector.add).toHaveBeenCalledTimes(1); + expect(collector.add).toHaveBeenCalledWith(TEST_ENTRY_TYPE.EXCEPTION, testPath, testRuleName, 'reason'); + }); + + it('returns empty and collects adoptions when there are no errors', () => { + const result = evaluateAndCollectAdoptionStatusWithInheritedException( + [], + testRuleName, + testParentObject, + testPath + ); + + expect(result).toEqual(undefined); + expect(collector.add).toHaveBeenCalledTimes(1); + expect(collector.add).toHaveBeenCalledWith(TEST_ENTRY_TYPE.ADOPTION, testPath, testRuleName); + }); + }); }); diff --git a/tools/spectral/ipa/__tests__/utils/exceptions.test.js b/tools/spectral/ipa/__tests__/utils/exceptions.test.js index 6646e861a6..1ee27555d1 100644 --- a/tools/spectral/ipa/__tests__/utils/exceptions.test.js +++ b/tools/spectral/ipa/__tests__/utils/exceptions.test.js @@ -1,5 +1,5 @@ import { describe, expect, it } from '@jest/globals'; -import { hasException } from '../../rulesets/functions/utils/exceptions'; +import { findExceptionInParentSchema, hasException } from '../../rulesets/functions/utils/exceptions'; const TEST_RULE_NAME_100 = 'xgen-IPA-100'; @@ -52,4 +52,57 @@ describe('tools/spectral/ipa/rulesets/functions/utils/exceptions.js', () => { expect(hasException(objectWithNestedIpa100Exception, TEST_RULE_NAME_100)).toBe(false); }); }); + describe('findExceptionInParentSchema', () => { + const oas = { + components: { + schemas: { + SchemaWithException: { + ...objectWithIpa100Exception, + properties: { + field: {}, + fieldWithException: { ...objectWithIpa100Exception }, + }, + }, + SchemaWithoutException: { + properties: { + field: {}, + }, + }, + }, + }, + }; + + it('returns the schema if the parent schema has an exception', () => { + const path = ['components', 'schemas', 'SchemaWithException', 'properties', 'field']; + expect(findExceptionInParentSchema(oas, {}, path, TEST_RULE_NAME_100)).toEqual({ + schema: oas.components.schemas.SchemaWithException, + }); + }); + it('returns an error if both the property and the parent schema have an exception', () => { + const path = ['components', 'schemas', 'SchemaWithException', 'properties', 'fieldWithException']; + expect(findExceptionInParentSchema(oas, objectWithIpa100Exception, path, TEST_RULE_NAME_100)).toEqual({ + error: [ + { + path: [...path, 'x-xgen-IPA-exception', TEST_RULE_NAME_100], + message: + 'This component is covered by an exception on its containing schema and does not need its own exception. Please remove the exception.', + }, + ], + }); + }); + it('returns the schema if the property is undefined and the parent schema has an exception', () => { + const path = ['components', 'schemas', 'SchemaWithException', 'properties', 'field']; + expect(findExceptionInParentSchema(oas, undefined, path, TEST_RULE_NAME_100)).toEqual({ + schema: oas.components.schemas.SchemaWithException, + }); + }); + it('returns null if the parent schema does not have an exception', () => { + const path = ['components', 'schemas', 'SchemaWithoutException', 'properties', 'field']; + expect(findExceptionInParentSchema(oas, {}, path, TEST_RULE_NAME_100)).toBeNull(); + expect(findExceptionInParentSchema(oas, {}, path, 'xgen-IPA-101')).toBeNull(); + }); + it('returns null if the path does not point to a schema property', () => { + expect(findExceptionInParentSchema(oas, {}, ['components', 'schemas'], TEST_RULE_NAME_100)).toBeNull(); + }); + }); }); diff --git a/tools/spectral/ipa/rulesets/IPA-112.yaml b/tools/spectral/ipa/rulesets/IPA-112.yaml index 3b1115aae8..ab3f6e1df6 100644 --- a/tools/spectral/ipa/rulesets/IPA-112.yaml +++ b/tools/spectral/ipa/rulesets/IPA-112.yaml @@ -64,6 +64,10 @@ rules: - Applies only to properties with type 'boolean' - Identifies property names that start with "is" followed by an uppercase letter - Suggests using the direct adjective form instead (e.g., "disabled" instead of "isDisabled") + - Exceptions can be set on the field or on the schema containing the field. An exception on the schema applies to all fields defined directly on that schema and is counted in the IPA exception metrics + - A field-level exception is unnecessary when the schema containing the field already has one + - A schema-level exception is unnecessary when no field defined directly on the schema violates the rule + - Fields in nested inline schemas or in allOf, anyOf or oneOf composition need their own exception message: '{{error}} https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-112-boolean-field-names-avoid-is-prefix' severity: error resolved: false diff --git a/tools/spectral/ipa/rulesets/README.md b/tools/spectral/ipa/rulesets/README.md index 0816e17fb6..64d07a86a7 100644 --- a/tools/spectral/ipa/rulesets/README.md +++ b/tools/spectral/ipa/rulesets/README.md @@ -863,6 +863,10 @@ Rule checks for the following conditions: - Applies only to properties with type 'boolean' - Identifies property names that start with "is" followed by an uppercase letter - Suggests using the direct adjective form instead (e.g., "disabled" instead of "isDisabled") + - Exceptions can be set on the field or on the schema containing the field. An exception on the schema applies to all fields defined directly on that schema and is counted in the IPA exception metrics + - A field-level exception is unnecessary when the schema containing the field already has one + - A schema-level exception is unnecessary when no field defined directly on the schema violates the rule + - Fields in nested inline schemas or in allOf, anyOf or oneOf composition need their own exception diff --git a/tools/spectral/ipa/rulesets/functions/IPA112BooleanFieldNamesAvoidIsPrefix.js b/tools/spectral/ipa/rulesets/functions/IPA112BooleanFieldNamesAvoidIsPrefix.js index ae7567ed13..62b8371500 100644 --- a/tools/spectral/ipa/rulesets/functions/IPA112BooleanFieldNamesAvoidIsPrefix.js +++ b/tools/spectral/ipa/rulesets/functions/IPA112BooleanFieldNamesAvoidIsPrefix.js @@ -1,23 +1,74 @@ -import { evaluateAndCollectAdoptionStatus, handleInternalError } from './utils/collectionUtils.js'; +import { + evaluateAndCollectAdoptionStatus, + evaluateAndCollectAdoptionStatusWithInheritedException, + handleInternalError, +} from './utils/collectionUtils.js'; import { resolveObject } from './utils/componentUtils.js'; +import { findExceptionInParentSchema, getUnnecessarySchemaExceptionError } from './utils/exceptions.js'; const IS_PREFIX_REGEX = /^is[A-Z]/; +/** + * Checks that boolean field names do not use the "is" prefix + * + * The exception can be set on the field or on the schema containing the field. An exception on the schema + * applies to all fields defined directly on that schema and is reported as unnecessary when no field needs it. + * + * @param {string} input - The field name + * @param {object} options - Rule configuration options + * @param {object} context - The context object containing the path and documentInventory + */ export default (input, options, { path, documentInventory, rule }) => { const ruleName = rule.name; const oas = documentInventory.unresolved; const property = resolveObject(oas, path); - // Skip schema references ($ref) and non-boolean fields: - // Referenced schemas are validated separately to prevent duplicate violations - if (!property || property.type !== 'boolean') { + if (!property) { return; } + const result = findExceptionInParentSchema(oas, property, path, ruleName); + + // A schema-level exception is unnecessary if no field defined directly on the schema needs it. + // Every field of the schema reports the same error, which Spectral deduplicates into a single result. + const unnecessarySchemaException = + result?.schema && !schemaNeedsException(result.schema) + ? getUnnecessarySchemaExceptionError(path.slice(0, -2), ruleName) + : undefined; + + // Skip schema references ($ref) and non-boolean fields: referenced schemas are validated + // separately to prevent duplicate violations. The schema-level exception is still checked + // for non-boolean fields, since it applies to the whole schema. + if (property.type !== 'boolean') { + return unnecessarySchemaException; + } + + if (result?.error) { + return result.error; + } + const errors = checkViolationsAndReturnErrors(input, path, ruleName); - return evaluateAndCollectAdoptionStatus(errors, ruleName, property, path); + if (!result) { + return evaluateAndCollectAdoptionStatus(errors, ruleName, property, path); + } + + const inheritedResult = evaluateAndCollectAdoptionStatusWithInheritedException(errors, ruleName, result.schema, path); + return unnecessarySchemaException ?? inheritedResult; }; +/** + * Checks whether the schema-level exception is needed by at least one field defined directly on the schema. + * A field needs the exception if it is a boolean field using the "is" prefix. + * + * @param {object} schema - The schema containing the exception + * @returns {boolean} true if at least one field of the schema violates the rule, false otherwise + */ +function schemaNeedsException(schema) { + return Object.entries(schema.properties ?? {}).some( + ([fieldName, fieldSchema]) => fieldSchema?.type === 'boolean' && IS_PREFIX_REGEX.test(fieldName) + ); +} + function checkViolationsAndReturnErrors(input, path, ruleName) { try { if (IS_PREFIX_REGEX.test(input)) { diff --git a/tools/spectral/ipa/rulesets/functions/utils/collectionUtils.js b/tools/spectral/ipa/rulesets/functions/utils/collectionUtils.js index eba0e27625..7c292549d1 100644 --- a/tools/spectral/ipa/rulesets/functions/utils/collectionUtils.js +++ b/tools/spectral/ipa/rulesets/functions/utils/collectionUtils.js @@ -27,6 +27,29 @@ export function evaluateAndCollectAdoptionStatus(validationErrors, ruleName, obj } } +/** + * Evaluates and collects the adoption or the inherited exception for an object whose exception lives on a parent + * object, for example a schema property inheriting an exception from its schema. The inherited exception is not + * reported as unnecessary, since it may still be needed by other objects inheriting it. + * + * @param {Array<{path: Array, message: string}>} validationErrors the error results from the rule + * @param {string} ruleName the name of the rule + * @param {*} parentObject the parent object containing the exception + * @param {Array} objectPath the JSON path to the evaluated object + */ +export function evaluateAndCollectAdoptionStatusWithInheritedException( + validationErrors, + ruleName, + parentObject, + objectPath +) { + if (validationErrors.length !== 0) { + collectException(parentObject, ruleName, objectPath); + return; + } + collectAdoption(objectPath, ruleName); +} + /** * Evaluates and collects adoptions and violations based on the rule, evaluated object and the validation errors. * No exceptions are allowed. diff --git a/tools/spectral/ipa/rulesets/functions/utils/exceptions.js b/tools/spectral/ipa/rulesets/functions/utils/exceptions.js index 1edf519735..ffd812ea2a 100644 --- a/tools/spectral/ipa/rulesets/functions/utils/exceptions.js +++ b/tools/spectral/ipa/rulesets/functions/utils/exceptions.js @@ -1,3 +1,5 @@ +import { resolveObject } from './componentUtils.js'; + export const EXCEPTION_EXTENSION = 'x-xgen-IPA-exception'; /** @@ -14,13 +16,27 @@ export function hasException(object, ruleName) { return false; } +const UNNECESSARY_EXCEPTION_MESSAGE = + 'This component adopts the rule and does not need an exception. Please remove the exception.'; +const UNNECESSARY_INHERITED_EXCEPTION_MESSAGE = + 'This component is covered by an exception on its containing schema and does not need its own exception. Please remove the exception.'; +const UNNECESSARY_SCHEMA_EXCEPTION_MESSAGE = + 'This component does not need an exception because none of its fields violate the rule. Please remove the exception.'; + +function unnecessaryExceptionError(objectPath, ruleName, message) { + return [{ path: [...objectPath, EXCEPTION_EXTENSION, ruleName], message }]; +} + export function getUnnecessaryExceptionError(objectPath, ruleName) { - return [ - { - path: [...objectPath, EXCEPTION_EXTENSION, ruleName], - message: 'This component adopts the rule and does not need an exception. Please remove the exception.', - }, - ]; + return unnecessaryExceptionError(objectPath, ruleName, UNNECESSARY_EXCEPTION_MESSAGE); +} + +export function getUnnecessaryInheritedExceptionError(objectPath, ruleName) { + return unnecessaryExceptionError(objectPath, ruleName, UNNECESSARY_INHERITED_EXCEPTION_MESSAGE); +} + +export function getUnnecessarySchemaExceptionError(objectPath, ruleName) { + return unnecessaryExceptionError(objectPath, ruleName, UNNECESSARY_SCHEMA_EXCEPTION_MESSAGE); } /** @@ -53,3 +69,29 @@ export function findExceptionInPathHierarchy(oas, currentPath, ruleName, jsonPat return null; } + +/** + * Finds an exception for a specified rule name in the schema directly containing the given property. + * The exception applies to all fields defined directly on that schema. + * + * @param {Object} oas - The OpenAPI Specification object. + * @param {Object} property - The property object being checked. + * @param {Array} propertyPath - The JSON path to the property, ending with ['properties', propertyName]. + * @param {string} ruleName - The name of the rule to check for exceptions. + * @return {null|Object} the schema with the exception, an error if both the property and the schema have one, or null + */ +export function findExceptionInParentSchema(oas, property, propertyPath, ruleName) { + if (propertyPath.length < 2 || propertyPath[propertyPath.length - 2] !== 'properties') { + return null; + } + + const schema = resolveObject(oas, propertyPath.slice(0, -2)); + if (!schema || !hasException(schema, ruleName)) { + return null; + } + + if (property && hasException(property, ruleName)) { + return { error: getUnnecessaryInheritedExceptionError(propertyPath, ruleName) }; + } + return { schema: schema }; +}