From 7a4796af5688665269d84f1b9ff8cbf89c002edf Mon Sep 17 00:00:00 2001 From: Dustin Popp Date: Thu, 14 Nov 2024 13:33:47 -0600 Subject: [PATCH] fix(ibm-well-defined-dictionaries): flag dictionaries with dictionary-type values The API Handbook states that the values for dictionaries should not themselves be dictionaries (no dictionaries of dictionaries). Previously, this rule would not catch that scenario. Now, it will. Additionally, more robust logic is used to check for the presence of ambiguous dictionaries. The old logic would miss flagging valid violations in complex, composed schemas without the fix. Signed-off-by: Dustin Popp Co-authored-by: Phil Adams --- .secrets.baseline | 2 +- docs/ibm-cloud-rules.md | 2 +- .../ruleset/src/functions/api-symmetry.js | 36 +-- .../functions/well-defined-dictionaries.js | 36 ++- .../rules/well-defined-dictionaries.test.js | 220 +++++++++++++++++- 5 files changed, 247 insertions(+), 49 deletions(-) diff --git a/.secrets.baseline b/.secrets.baseline index e54d3a83..5cb6afb6 100644 --- a/.secrets.baseline +++ b/.secrets.baseline @@ -3,7 +3,7 @@ "files": "package-lock.json|^.secrets.baseline$", "lines": null }, - "generated_at": "2024-11-08T16:48:42Z", + "generated_at": "2024-11-14T19:33:14Z", "plugins_used": [ { "name": "AWSKeyDetector" diff --git a/docs/ibm-cloud-rules.md b/docs/ibm-cloud-rules.md index 1d5dafc2..7193f980 100644 --- a/docs/ibm-cloud-rules.md +++ b/docs/ibm-cloud-rules.md @@ -7262,7 +7262,7 @@ paths: 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. See the IBM Cloud API Handbook documentation on types for more info. + 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. diff --git a/packages/ruleset/src/functions/api-symmetry.js b/packages/ruleset/src/functions/api-symmetry.js index 680ab257..dfbe43da 100644 --- a/packages/ruleset/src/functions/api-symmetry.js +++ b/packages/ruleset/src/functions/api-symmetry.js @@ -9,6 +9,7 @@ const { isObjectSchema, isArraySchema, schemaHasConstraint, + schemaLooselyHasConstraint, } = require('@ibm-cloud/openapi-ruleset-utilities'); const { @@ -310,7 +311,7 @@ function checkForGraphFragmentPattern( for (const [name, prop] of Object.entries(c.properties)) { logger.debug(`${ruleId}: checking canonical property '${name}'`); - const included = schemaHasLooseConstraint( + const included = schemaLooselyHasConstraint( variant, s => 'properties' in s && @@ -439,36 +440,3 @@ function canonicalSchemaMeetsConstraint( return false; } - -/** - * This function is a looser adaptation of the "schemaHasConstraint()" function in the utilities package. - * Here we process oneOf and anyOf lists the same as allOf, where we return true if one (or more) - * of the oneOf/anyOf elements has the constraint (rather than all of the elements). - */ -function schemaHasLooseConstraint(schema, hasConstraint) { - if (!isObject(schema)) { - return false; - } - - if (hasConstraint(schema)) { - return true; - } - - const anySchemaHasConstraintReducer = (previousResult, currentSchema) => { - return ( - previousResult || schemaHasLooseConstraint(currentSchema, hasConstraint) - ); - }; - - for (const applicator of ['allOf', 'oneOf', 'anyOf']) { - if ( - Array.isArray(schema[applicator]) && - schema[applicator].length > 0 && - schema[applicator].reduce(anySchemaHasConstraintReducer, false) - ) { - return true; - } - } - - return false; -} diff --git a/packages/ruleset/src/functions/well-defined-dictionaries.js b/packages/ruleset/src/functions/well-defined-dictionaries.js index bfb3c354..e8661e59 100644 --- a/packages/ruleset/src/functions/well-defined-dictionaries.js +++ b/packages/ruleset/src/functions/well-defined-dictionaries.js @@ -7,6 +7,7 @@ const { isObject, isObjectSchema, schemaHasConstraint, + schemaLooselyHasConstraint, validateNestedSchemas, } = require('@ibm-cloud/openapi-ruleset-utilities'); const { LoggerFactory } = require('../utils'); @@ -29,6 +30,12 @@ function wellDefinedDictionaries(schema, path) { return []; } + // We will flag dictionaries of dictionaries, so we can skip + // providing guidance for directly nested dictionaries. + if (path.at(-1) === 'additionalProperties') { + return []; + } + logger.debug( `${ruleId}: checking object schema at location: ${path.join('.')}` ); @@ -78,6 +85,15 @@ function wellDefinedDictionaries(schema, path) { }); } + // The Handbook guidelines state that dictionary types + // should not be dictionaries themselves. + if (schemaHasConstraint(schema, isDictionaryOfDictionaries)) { + errors.push({ + message: 'Dictionaries must not have values that are also dictionaries.', + path, + }); + } + return errors; } @@ -90,7 +106,25 @@ function isAmbiguousDictionary(schema) { 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) || !schema.additionalProperties.type + !isObject(schema.additionalProperties) || + !schemaDefinesField(schema.additionalProperties, 'type') + ); +} + +function isDictionaryOfDictionaries(schema) { + if (!isObject(schema.additionalProperties)) { + 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'] ); } diff --git a/packages/ruleset/test/rules/well-defined-dictionaries.test.js b/packages/ruleset/test/rules/well-defined-dictionaries.test.js index 4d9b93f0..b5517b0d 100644 --- a/packages/ruleset/test/rules/well-defined-dictionaries.test.js +++ b/packages/ruleset/test/rules/well-defined-dictionaries.test.js @@ -12,7 +12,7 @@ const { } = require('../test-utils'); const rule = wellDefinedDictionaries; -const ruleId = 'ibm-valid-path-segments'; +const ruleId = 'ibm-well-defined-dictionaries'; const expectedSeverity = severityCodes.warning; describe(`Spectral rule: ${ruleId}`, () => { @@ -39,17 +39,25 @@ describe(`Spectral rule: ${ruleId}`, () => { expect(results).toHaveLength(0); }); - it('Includes a dictionary that nests a well-defined dictionary with scalar values', async () => { + it('Includes a well-defined dictionary with composed scalar values', async () => { const testDocument = makeCopy(rootDocument); testDocument.components.schemas.Movie.properties.metadata = { - description: 'a dictionary mapping keys to string values', + description: 'a dictionary mapping keys to integer values', type: 'object', additionalProperties: { - type: 'object', - additionalProperties: { - type: 'string', - }, + oneOf: [ + { + type: 'integer', + minimum: 0, + maximum: 10, + }, + { + type: 'integer', + minimum: 20, + maximum: 30, + }, + ], }, }; @@ -84,6 +92,29 @@ describe(`Spectral rule: ${ruleId}`, () => { const results = await testRule(ruleId, rule, testDocument); expect(results).toHaveLength(0); }); + + it('Includes a well-defined dictionary of models that include a dictionary property', async () => { + const testDocument = makeCopy(rootDocument); + + testDocument.components.schemas.Movie.properties.metadata = { + description: 'a dictionary mapping keys to string values', + type: 'object', + additionalProperties: { + type: 'object', + properties: { + nested: { + type: 'object', + additionalProperties: { + type: 'string', + }, + }, + }, + }, + }; + + const results = await testRule(ruleId, rule, testDocument); + expect(results).toHaveLength(0); + }); }); describe('Should yield errors', () => { @@ -204,16 +235,16 @@ describe(`Spectral rule: ${ruleId}`, () => { expect(results).toHaveLength(4); const expectedRulePaths = [ - 'paths./v1/movies.get.responses.200.content.application/json.schema.allOf.1.properties.movies.items.properties.metadata.additionalProperties', - 'paths./v1/movies.post.responses.201.content.application/json.schema.properties.metadata.additionalProperties', - 'paths./v1/movies/{movie_id}.get.responses.200.content.application/json.schema.properties.metadata.additionalProperties', - 'paths./v1/movies/{movie_id}.put.responses.200.content.application/json.schema.properties.metadata.additionalProperties', + 'paths./v1/movies.get.responses.200.content.application/json.schema.allOf.1.properties.movies.items.properties.metadata', + 'paths./v1/movies.post.responses.201.content.application/json.schema.properties.metadata', + 'paths./v1/movies/{movie_id}.get.responses.200.content.application/json.schema.properties.metadata', + 'paths./v1/movies/{movie_id}.put.responses.200.content.application/json.schema.properties.metadata', ]; 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`' + 'Dictionaries must not have values that are also dictionaries.' ); expect(results[i].severity).toBe(expectedSeverity); expect(results[i].path.join('.')).toBe(expectedRulePaths[i]); @@ -247,5 +278,170 @@ describe(`Spectral rule: ${ruleId}`, () => { expect(results[i].path.join('.')).toBe(expectedPaths[i]); } }); + + it('Includes a dictionary of dictionaries, which is not allowed', async () => { + const testDocument = makeCopy(rootDocument); + + testDocument.components.schemas.Movie.properties.metadata = { + description: 'a dictionary with no definition', + type: 'object', + additionalProperties: { + type: 'object', + additionalProperties: { + 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( + '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 = { + description: 'a dictionary with no definition', + type: 'object', + additionalProperties: { + 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('Includes a composed dictionary of dictionaries, which is not allowed', async () => { + const testDocument = makeCopy(rootDocument); + + testDocument.components.schemas.Movie.properties.metadata = { + description: 'a dictionary with no definition', + type: 'object', + additionalProperties: { + type: 'object', + allOf: [ + { + additionalProperties: { + type: 'string', + }, + }, + { + properties: { + name: { + 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( + '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 () => { + const testDocument = makeCopy(rootDocument); + + testDocument.components.schemas.Movie.properties.metadata = { + description: 'a dictionary with no definition', + type: 'object', + additionalProperties: { + type: 'object', + oneOf: [ + { + additionalProperties: { + type: 'string', + }, + }, + { + properties: { + name: { + 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( + '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 well-defined dictionary of models that include a poorly-defined dictionary property', async () => { + const testDocument = makeCopy(rootDocument); + + testDocument.components.schemas.Movie.properties.metadata = { + description: 'a dictionary mapping keys to string values', + type: 'object', + additionalProperties: { + type: 'object', + properties: { + nested: { + type: 'object', + additionalProperties: true, + }, + }, + }, + }; + + const results = await testRule(ruleId, rule, testDocument); + expect(results).toHaveLength(4); + + const expectedRulePaths = [ + 'paths./v1/movies.get.responses.200.content.application/json.schema.allOf.1.properties.movies.items.properties.metadata.additionalProperties.properties.nested', + 'paths./v1/movies.post.responses.201.content.application/json.schema.properties.metadata.additionalProperties.properties.nested', + 'paths./v1/movies/{movie_id}.get.responses.200.content.application/json.schema.properties.metadata.additionalProperties.properties.nested', + 'paths./v1/movies/{movie_id}.put.responses.200.content.application/json.schema.properties.metadata.additionalProperties.properties.nested', + ]; + + 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`' + ); + expect(results[i].severity).toBe(expectedSeverity); + expect(results[i].path.join('.')).toBe(expectedRulePaths[i]); + } + }); }); });