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 <[email protected]> Co-authored-by: Phil Adams <[email protected]>
IBM · Nov 15, 2024 · 7a4796a · 7a4796a
1 parent 8ae0e07
commit 7a4796a
Show file tree

Hide file tree

Showing 5 changed files with 247 additions and 49 deletions.
diff --git 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
@@ -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 <a href="https://cloud.ibm.com/docs/api-handbook?topic=api-handbook-types">IBM Cloud API Handbook documentation on types</a> 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 <a href="https://cloud.ibm.com/docs/api-handbook?topic=api-handbook-types">IBM Cloud API Handbook documentation on types</a> for more info.
 </td>
 </tr>
 <tr>

diff --git 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
@@ -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
@@ -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]);
+      }
+    });
   });
 });