From ad7134db5dbd2c17b1a15fa7bb7988b470406a09 Mon Sep 17 00:00:00 2001 From: Dustin Popp Date: Fri, 10 Jan 2025 15:55:57 -0600 Subject: [PATCH] fix(ibm-well-defined-dictionaries): include patternProperties in validation (#713) Currently, the rule only considers dictionaries defined with `additionalProperties`. OpenAPI 3.1.x supports defining dictionaries with `patternProperties`, so this commit adds consideration for this field in its validation, in addition to `additionalProperties`. Signed-off-by: Dustin Popp --- .secrets.baseline | 2 +- docs/ibm-cloud-rules.md | 4 +- .../functions/well-defined-dictionaries.js | 80 ++++++--- .../rules/well-defined-dictionaries.test.js | 167 ++++++++++++++---- 4 files changed, 186 insertions(+), 67 deletions(-) diff --git a/.secrets.baseline b/.secrets.baseline index 97e9df08..cb496540 100644 --- a/.secrets.baseline +++ b/.secrets.baseline @@ -3,7 +3,7 @@ "files": "package-lock.json|^.secrets.baseline$", "lines": null }, - "generated_at": "2024-12-19T16:14:03Z", + "generated_at": "2025-01-09T19:49:59Z", "plugins_used": [ { "name": "AWSKeyDetector" diff --git a/docs/ibm-cloud-rules.md b/docs/ibm-cloud-rules.md index 09c5d0bf..c0f01da2 100644 --- a/docs/ibm-cloud-rules.md +++ b/docs/ibm-cloud-rules.md @@ -7328,8 +7328,8 @@ paths: This rule validates that any dictionary schemas are well defined and that all values share a single type. Dictionaries are defined as object type schemas that have variable key names. They are distinct from model types, which are objects with pre-defined properties. A schema must not define both concrete properties and variable key names. - Practically, this means a schema must explicitly define a `properties` object or an `additionalProperties` schema, but not both. - If used, the `additionalProperties` schema must define a concrete type. The concrete type of the values must not be a dictionary itself. See the IBM Cloud API Handbook documentation on types for more info. + Practically, this means a schema must explicitly define a `properties` object or an `(additional|pattern)Properties` schema, but not both. + If used, the `(additional|pattern)Properties` schema must define a concrete type. The concrete type of the values must not be a dictionary itself. See the IBM Cloud API Handbook documentation on types for more info. diff --git a/packages/ruleset/src/functions/well-defined-dictionaries.js b/packages/ruleset/src/functions/well-defined-dictionaries.js index e8661e59..cde853c5 100644 --- a/packages/ruleset/src/functions/well-defined-dictionaries.js +++ b/packages/ruleset/src/functions/well-defined-dictionaries.js @@ -1,5 +1,5 @@ /** - * Copyright 2024 IBM Corporation. + * Copyright 2024 - 2025 IBM Corporation. * SPDX-License-Identifier: Apache2.0 */ @@ -7,7 +7,6 @@ const { isObject, isObjectSchema, schemaHasConstraint, - schemaLooselyHasConstraint, validateNestedSchemas, } = require('@ibm-cloud/openapi-ruleset-utilities'); const { LoggerFactory } = require('../utils'); @@ -15,6 +14,13 @@ const { LoggerFactory } = require('../utils'); let ruleId; let logger; +/** + * The implementation for this rule makes assumptions that are dependent on the + * presence of the following other rules: + * + * - ibm-pattern-properties: patternProperties isn't empty or the wrong type + */ + module.exports = function (schema, _opts, context) { if (!logger) { ruleId = context.rule.name; @@ -32,7 +38,7 @@ function wellDefinedDictionaries(schema, path) { // We will flag dictionaries of dictionaries, so we can skip // providing guidance for directly nested dictionaries. - if (path.at(-1) === 'additionalProperties') { + if (isDictionaryValueSchema(path)) { return []; } @@ -40,15 +46,15 @@ function wellDefinedDictionaries(schema, path) { `${ruleId}: checking object schema at location: ${path.join('.')}` ); - // Dictionaries should have additionalProperties defined on them. - // If the schema doesn't, make sure it has properties and then - // abandon the check. - if (!schemaDefinesField(schema, 'additionalProperties')) { + // Dictionaries should have additionalProperties or patternProperties + // defined on them. If the schema doesn't, make sure it has properties + // and then abandon the check. + if (!isDictionarySchema(schema)) { if (!schemaDefinesField(schema, 'properties')) { return [ { message: - 'Object schemas must define either properties, or additionalProperties with a concrete type', + 'Object schemas must define either properties, or (additional/pattern)Properties with a concrete type', path, }, ]; @@ -79,8 +85,7 @@ function wellDefinedDictionaries(schema, path) { // more strict in the future but this meets our current purposes. if (schemaHasConstraint(schema, isAmbiguousDictionary)) { errors.push({ - message: - 'Dictionary schemas must have a single, well-defined value type in `additionalProperties`', + message: 'Dictionary schemas must have a single, well-defined value type', path, }); } @@ -89,7 +94,7 @@ function wellDefinedDictionaries(schema, path) { // should not be dictionaries themselves. if (schemaHasConstraint(schema, isDictionaryOfDictionaries)) { errors.push({ - message: 'Dictionaries must not have values that are also dictionaries.', + message: 'Dictionaries must not have values that are also dictionaries', path, }); } @@ -102,29 +107,46 @@ function schemaDefinesField(schema, field) { } function isAmbiguousDictionary(schema) { - if (!schema.additionalProperties) { - return false; - } - - // additionalProperties must be an object (not a boolean) value - // and must define a `type` field in order to be considered an - // unambiguous dictionary. - return ( - !isObject(schema.additionalProperties) || - !schemaDefinesField(schema.additionalProperties, 'type') + return dictionaryValuesHaveConstraint( + schema, + valueSchema => + !isObject(valueSchema) || !schemaDefinesField(valueSchema, 'type') ); } function isDictionaryOfDictionaries(schema) { - if (!isObject(schema.additionalProperties)) { + return dictionaryValuesHaveConstraint( + schema, + valueSchema => isObject(valueSchema) && isDictionarySchema(valueSchema) + ); +} + +function dictionaryValuesHaveConstraint(schema, hasConstraint) { + return schemaHasConstraint(schema, s => { + if (s.additionalProperties !== undefined) { + return hasConstraint(s.additionalProperties); + } + + if (s.patternProperties !== undefined) { + return Object.values(s.patternProperties).some(p => hasConstraint(p)); + } + return false; - } + }); +} - // We don't want any schema that may define the dictionary values - // to also be a dictionary, so we use a looser constraint that - // checks against any oneOf/anyOf schema. - return schemaLooselyHasConstraint( - schema.additionalProperties, - s => !!s['additionalProperties'] +// Check, *by path*, if the current schema is a dictionary value schema. +function isDictionaryValueSchema(path) { + return ( + path.at(-1) === 'additionalProperties' || + path.at(-2) === 'patternProperties' + ); +} + +// Check, *by object fields* if the current schema is a dictionary or not. +function isDictionarySchema(schema) { + return ( + schemaDefinesField(schema, 'additionalProperties') || + schemaDefinesField(schema, 'patternProperties') ); } diff --git a/packages/ruleset/test/rules/well-defined-dictionaries.test.js b/packages/ruleset/test/rules/well-defined-dictionaries.test.js index b5517b0d..9da5c17d 100644 --- a/packages/ruleset/test/rules/well-defined-dictionaries.test.js +++ b/packages/ruleset/test/rules/well-defined-dictionaries.test.js @@ -1,5 +1,5 @@ /** - * Copyright 2024 IBM Corporation. + * Copyright 2024 - 2025 IBM Corporation. * SPDX-License-Identifier: Apache2.0 */ @@ -24,7 +24,7 @@ describe(`Spectral rule: ${ruleId}`, () => { expect(results).toHaveLength(0); }); - it('Includes a well-defined dictionary with scalar values', async () => { + it('Includes a well-defined dictionary with scalar values (additionalProperties)', async () => { const testDocument = makeCopy(rootDocument); testDocument.components.schemas.Movie.properties.metadata = { @@ -39,6 +39,23 @@ describe(`Spectral rule: ${ruleId}`, () => { expect(results).toHaveLength(0); }); + it('Includes a well-defined dictionary with scalar values (patternProperties)', async () => { + const testDocument = makeCopy(rootDocument); + + testDocument.components.schemas.Movie.properties.metadata = { + description: 'a dictionary mapping keys to string values', + type: 'object', + patternProperties: { + '^[a-zA-Z]+$': { + type: 'string', + }, + }, + }; + + const results = await testRule(ruleId, rule, testDocument); + expect(results).toHaveLength(0); + }); + it('Includes a well-defined dictionary with composed scalar values', async () => { const testDocument = makeCopy(rootDocument); @@ -139,14 +156,14 @@ describe(`Spectral rule: ${ruleId}`, () => { for (const i in results) { expect(results[i].code).toBe(ruleId); expect(results[i].message).toMatch( - 'Object schemas must define either properties, or additionalProperties with a concrete type' + 'Object schemas must define either properties, or (additional/pattern)Properties with a concrete type' ); expect(results[i].severity).toBe(expectedSeverity); expect(results[i].path.join('.')).toBe(expectedPaths[i]); } }); - it('Includes a model/dictionary hybrid, which is not allowed', async () => { + it('Includes a model/dictionary hybrid, which is not allowed (additionalProperties)', async () => { const testDocument = makeCopy(rootDocument); testDocument.components.schemas.Movie.properties.metadata = { @@ -175,6 +192,37 @@ describe(`Spectral rule: ${ruleId}`, () => { } }); + it('Includes a model/dictionary hybrid, which is not allowed (patternProperties)', async () => { + const testDocument = makeCopy(rootDocument); + + testDocument.components.schemas.Movie.properties.metadata = { + description: 'a dictionary with no definition', + type: 'object', + properties: { + rating: { + type: 'integer', + }, + }, + patternProperties: { + '^[a-zA-Z]+$': { + type: 'string', + }, + }, + }; + + const results = await testRule(ruleId, rule, testDocument); + expect(results).toHaveLength(4); + + for (const i in results) { + expect(results[i].code).toBe(ruleId); + expect(results[i].message).toMatch( + 'Object schemas must be either a model or a dictionary - they cannot be both' + ); + expect(results[i].severity).toBe(expectedSeverity); + expect(results[i].path.join('.')).toBe(expectedPaths[i]); + } + }); + it('Includes a dictionary with additionalProperties set to "true"', async () => { const testDocument = makeCopy(rootDocument); @@ -190,7 +238,36 @@ describe(`Spectral rule: ${ruleId}`, () => { for (const i in results) { expect(results[i].code).toBe(ruleId); expect(results[i].message).toMatch( - 'Dictionary schemas must have a single, well-defined value type in `additionalProperties`' + 'Dictionary schemas must have a single, well-defined value type' + ); + expect(results[i].severity).toBe(expectedSeverity); + expect(results[i].path.join('.')).toBe(expectedPaths[i]); + } + }); + + it('Includes a dictionary with patternProperties value that has no type', async () => { + const testDocument = makeCopy(rootDocument); + + testDocument.components.schemas.Movie.properties.metadata = { + description: 'a dictionary with no definition', + type: 'object', + patternProperties: { + '^[a-zA-Z]+$': { + type: 'string', + }, + '^wrong_[A-Z][a-z]+$': { + description: 'no type definition', + }, + }, + }; + + const results = await testRule(ruleId, rule, testDocument); + expect(results).toHaveLength(4); + + for (const i in results) { + expect(results[i].code).toBe(ruleId); + expect(results[i].message).toMatch( + 'Dictionary schemas must have a single, well-defined value type' ); expect(results[i].severity).toBe(expectedSeverity); expect(results[i].path.join('.')).toBe(expectedPaths[i]); @@ -212,7 +289,7 @@ describe(`Spectral rule: ${ruleId}`, () => { for (const i in results) { expect(results[i].code).toBe(ruleId); expect(results[i].message).toMatch( - 'Dictionary schemas must have a single, well-defined value type in `additionalProperties`' + 'Dictionary schemas must have a single, well-defined value type' ); expect(results[i].severity).toBe(expectedSeverity); expect(results[i].path.join('.')).toBe(expectedPaths[i]); @@ -244,7 +321,7 @@ describe(`Spectral rule: ${ruleId}`, () => { for (const i in results) { expect(results[i].code).toBe(ruleId); expect(results[i].message).toMatch( - 'Dictionaries must not have values that are also dictionaries.' + 'Dictionaries must not have values that are also dictionaries' ); expect(results[i].severity).toBe(expectedSeverity); expect(results[i].path.join('.')).toBe(expectedRulePaths[i]); @@ -272,14 +349,14 @@ describe(`Spectral rule: ${ruleId}`, () => { for (const i in results) { expect(results[i].code).toBe(ruleId); expect(results[i].message).toMatch( - 'Dictionary schemas must have a single, well-defined value type in `additionalProperties`' + 'Dictionary schemas must have a single, well-defined value type' ); expect(results[i].severity).toBe(expectedSeverity); expect(results[i].path.join('.')).toBe(expectedPaths[i]); } }); - it('Includes a dictionary of dictionaries, which is not allowed', async () => { + it('Includes a dictionary of dictionaries, which is not allowed (additionalProperties)', async () => { const testDocument = makeCopy(rootDocument); testDocument.components.schemas.Movie.properties.metadata = { @@ -299,22 +376,28 @@ describe(`Spectral rule: ${ruleId}`, () => { for (const i in results) { expect(results[i].code).toBe(ruleId); expect(results[i].message).toMatch( - 'Dictionaries must not have values that are also dictionaries.' + 'Dictionaries must not have values that are also dictionaries' ); expect(results[i].severity).toBe(expectedSeverity); expect(results[i].path.join('.')).toBe(expectedPaths[i]); } }); - it('Flags dictionary of dictionaries but ignores dictionary value issue', async () => { + it('Includes a dictionary of dictionaries, which is not allowed (patternProperties)', async () => { const testDocument = makeCopy(rootDocument); testDocument.components.schemas.Movie.properties.metadata = { description: 'a dictionary with no definition', type: 'object', - additionalProperties: { - type: 'object', - additionalProperties: true, + patternProperties: { + '^[a-zA-Z]+$': { + type: 'object', + patternProperties: { + '^[a-zA-Z]+$': { + type: 'string', + }, + }, + }, }, }; @@ -324,14 +407,41 @@ describe(`Spectral rule: ${ruleId}`, () => { for (const i in results) { expect(results[i].code).toBe(ruleId); expect(results[i].message).toMatch( - 'Dictionaries must not have values that are also dictionaries.' + 'Dictionaries must not have values that are also dictionaries' ); expect(results[i].severity).toBe(expectedSeverity); expect(results[i].path.join('.')).toBe(expectedPaths[i]); } }); - it('Includes a composed dictionary of dictionaries, which is not allowed', async () => { + it('Includes a dictionary of dictionaries, which is not allowed (hybrid)', async () => { + const testDocument = makeCopy(rootDocument); + + testDocument.components.schemas.Movie.properties.metadata = { + description: 'a dictionary with no definition', + type: 'object', + patternProperties: { + '^[a-zA-Z]+$': { + type: 'object', + additionalProperties: true, + }, + }, + }; + + const results = await testRule(ruleId, rule, testDocument); + expect(results).toHaveLength(4); + + for (const i in results) { + expect(results[i].code).toBe(ruleId); + expect(results[i].message).toMatch( + 'Dictionaries must not have values that are also dictionaries' + ); + expect(results[i].severity).toBe(expectedSeverity); + expect(results[i].path.join('.')).toBe(expectedPaths[i]); + } + }); + + it('Flags dictionary of dictionaries but ignores dictionary value issue', async () => { const testDocument = makeCopy(rootDocument); testDocument.components.schemas.Movie.properties.metadata = { @@ -339,20 +449,7 @@ describe(`Spectral rule: ${ruleId}`, () => { type: 'object', additionalProperties: { type: 'object', - allOf: [ - { - additionalProperties: { - type: 'string', - }, - }, - { - properties: { - name: { - type: 'string', - }, - }, - }, - ], + additionalProperties: true, }, }; @@ -362,14 +459,14 @@ describe(`Spectral rule: ${ruleId}`, () => { for (const i in results) { expect(results[i].code).toBe(ruleId); expect(results[i].message).toMatch( - 'Dictionaries must not have values that are also dictionaries.' + 'Dictionaries must not have values that are also dictionaries' ); expect(results[i].severity).toBe(expectedSeverity); expect(results[i].path.join('.')).toBe(expectedPaths[i]); } }); - it('Includes a single oneOf element that creates a nested dictionary, which is not allowed', async () => { + it('Includes a composed dictionary of dictionaries, which is not allowed', async () => { const testDocument = makeCopy(rootDocument); testDocument.components.schemas.Movie.properties.metadata = { @@ -377,7 +474,7 @@ describe(`Spectral rule: ${ruleId}`, () => { type: 'object', additionalProperties: { type: 'object', - oneOf: [ + allOf: [ { additionalProperties: { type: 'string', @@ -400,7 +497,7 @@ describe(`Spectral rule: ${ruleId}`, () => { for (const i in results) { expect(results[i].code).toBe(ruleId); expect(results[i].message).toMatch( - 'Dictionaries must not have values that are also dictionaries.' + 'Dictionaries must not have values that are also dictionaries' ); expect(results[i].severity).toBe(expectedSeverity); expect(results[i].path.join('.')).toBe(expectedPaths[i]); @@ -437,7 +534,7 @@ describe(`Spectral rule: ${ruleId}`, () => { for (const i in results) { expect(results[i].code).toBe(ruleId); expect(results[i].message).toMatch( - 'Dictionary schemas must have a single, well-defined value type in `additionalProperties`' + 'Dictionary schemas must have a single, well-defined value type' ); expect(results[i].severity).toBe(expectedSeverity); expect(results[i].path.join('.')).toBe(expectedRulePaths[i]);