feat: add new spectral discriminator rule and remove old rule (#367)

ruleset: add 'discriminator' rule to ruleset

This rule verifies that the 'propertyName' value of the discriminator
is present in each subschema.

validator: remove broken discriminator rule in favor of new spectral rule

This rule was buggy and not previously configurable. The same rule with
identical (albeit fixed) behavior is now available in our Spectral ruleset
under the name 'discriminator' and is configurable.

README.md

@@ -64,7 +64,7 @@ as well as IBM-defined best practices.
 ## Getting Started
 The validator analyzes your API definition and reports any problems within. The validator is highly customizable, and supports both OpenAPI 3.0 and OpenAPI 2.0 (Swagger 2.0) formats. The tool also supports a number of rules from [Spectral](https://stoplight.io/open-source/spectral/). You can easily extend the tool with custom rules to meet your specific needs and ensure compliance to your standards.
 
-The default configuration uses both OpenAPI 3.0 rules as well as Spectral rules. The [default mode](#default-mode) section decscribes these rules. Get started by [installing the tool](#installation), then [run the tool](#usage) on your API definition.  
+The default configuration uses both OpenAPI 3.0 rules as well as Spectral rules. The [default mode](#default-mode) section describes these rules. Get started by [installing the tool](#installation), then [run the tool](#usage) on your API definition.  
 
 ### Customization
 
@@ -496,32 +496,32 @@ The default values for each rule are described below.
 Currently the validator configures Spectral to check the following rules from its
 [“oas" ruleset](https://meta.stoplight.io/docs/spectral/docs/reference/openapi-rules.md):
 ```
+oas2-operation-formData-consume-check: true
+operation-operationId-unique: true
+operation-parameters: true
+operation-tag-defined: true
 no-eval-in-markdown: true
 no-script-tags-in-markdown: true
 openapi-tags: true
 operation-description: true
-operation-operationId-unique: true
-operation-parameters: true
 operation-tags: true
-operation-tag-defined: true
 path-keys-no-trailing-slash: true
 path-not-include-query: true
-request-body-object: true
 typed-enum: true
 oas2-api-host: true
 oas2-api-schemes: true
 oas2-host-trailing-slash: true
-oas2-valid-example: true
-oas2-valid-definition-example: true
+oas2-valid-schema-example: 'warn'
 oas2-anyOf: true
 oas2-oneOf: true
-oas2-operation-formData-consume-check: true
+oas2-unused-definition: true
 oas3-api-servers: true
 oas3-examples-value-or-externalValue: true
 oas3-server-trailing-slash: true
+oas3-valid-media-example: 'warn'
+oas3-valid-schema-example: 'warn'
 oas3-schema: true
-oas3-valid-example: true
-oas3-valid-schema-example: true
+oas3-unused-component: true
 ```
 
 This ruleset has the name `@ibm-cloud/openapi-ruleset`, and you can "extend" this ruleset or specify your own custom ruleset

packages/ruleset/src/collections/index.js

@@ -0,0 +1,19 @@
+// a group of predefined "collections" of OpenAPI locations to validate
+// helpful when the same group of locations needs to be used by multiple rules
+
+// a collection of locations where a JSON Schema object can be *used*.
+//
+// note that this does not include "components.schemas" to avoid duplication.
+// this collection should be used in a rule that has "resolved" set to "true".
+// we separately validate that all schemas in "components" need to be used.
+const schemas = [
+  '$.paths[*][parameters][*].schema',
+  '$.paths[*][parameters][*].content[*].schema',
+  '$.paths[*][*][parameters][*].schema',
+  '$.paths[*][*][parameters,responses][*].content[*].schema',
+  '$.paths[*][*][requestBody].content[*].schema'
+];
+
+module.exports = {
+  schemas
+};

packages/ruleset/src/functions/discriminator.js

@@ -0,0 +1,40 @@
+// Assertation 1:
+// if discriminator exist inside schema object, it must be of type Object
+// enforced by Spectral's oas3-schema rule
+
+// Assertion 2:
+// discriminator object must have a field name propertyName
+// enforced by Spectral's oas3-schema rule
+
+// Assertation 3:
+// propertyName is of type string
+// enforced by Spectral's oas3-schema rule
+
+// Assertation 4:
+// properties inside a schema object must include propertyName from discriminator object
+
+const { checkSubschemasForProperty, validateSubschemas } = require('../utils');
+
+module.exports = function(schema, _opts, { path }) {
+  return validateSubschemas(schema, path, validateDiscriminators);
+};
+
+function validateDiscriminators(schema, path) {
+  const errors = [];
+
+  const { discriminator } = schema;
+  if (!discriminator || !typeof discriminator === 'object') {
+    return errors;
+  }
+
+  const { propertyName } = discriminator;
+  if (!checkSubschemasForProperty(schema, propertyName)) {
+    errors.push({
+      message:
+        'The discriminator property name used must be defined in this schema',
+      path: [...path, 'discriminator', 'propertyName']
+    });
+  }
+
+  return errors;
+}

packages/ruleset/src/functions/index.js

@@ -1,4 +1,5 @@
 const checkMajorVersion = require('./check-major-version');
+const discriminator = require('./discriminator');
 const errorResponseSchema = require('./error-response-schema');
 const requiredProperty = require('./required-property');
 const responseExampleProvided = require('./response-example-provided');
@@ -7,6 +8,7 @@ const stringBoundary = require('./string-boundary');
 
 module.exports = {
   checkMajorVersion,
+  discriminator,
   errorResponseSchema,
   requiredProperty,
   responseExampleProvided,

packages/ruleset/src/functions/required-property.js

@@ -1,35 +1,14 @@
+const { checkSubschemasForProperty, validateSubschemas } = require('../utils');
+
 module.exports = function(schema, _opts, { path }) {
-  return getErrorsForMissingRequiredProperties(schema, path);
+  return validateSubschemas(schema, path, checkRequiredProperties);
 };
 
-function getErrorsForMissingRequiredProperties(schema, path) {
-  const errors = [];
-  errors.push(...checkRequiredProperties(schema, path));
-  if (schema.properties) {
-    Object.entries(schema.properties).forEach(function(prop) {
-      const propName = prop[0];
-      const propSchema = prop[1];
-      errors.push(
-        ...getErrorsForMissingRequiredProperties(propSchema, [
-          ...path,
-          'properties',
-          propName
-        ])
-      );
-    });
-  } else if (schema.items) {
-    errors.push(
-      ...getErrorsForMissingRequiredProperties(schema.items, [...path, 'items'])
-    );
-  }
-  return errors;
-}
-
 function checkRequiredProperties(schema, path) {
   const errors = [];
   if (Array.isArray(schema.required)) {
     schema.required.forEach(function(requiredPropName) {
-      if (!checkSchemaForProp(requiredPropName, schema)) {
+      if (!checkSubschemasForProperty(schema, requiredPropName)) {
         let message;
         if (schema.allOf) {
           message = `Required property, ${requiredPropName}, must be defined in at least one of the allOf schemas`;
@@ -47,27 +26,3 @@ function checkRequiredProperties(schema, path) {
   }
   return errors;
 }
-
-function checkSchemaForProp(requiredProp, schema) {
-  if (schema.properties && schema.properties[requiredProp]) {
-    return true;
-  } else if (Array.isArray(schema.allOf)) {
-    let reqPropDefined = false;
-    schema.allOf.forEach(childObj => {
-      if (checkSchemaForProp(requiredProp, childObj)) {
-        reqPropDefined = true;
-      }
-    });
-    return reqPropDefined;
-  } else if (Array.isArray(schema.anyOf) || Array.isArray(schema.oneOf)) {
-    const childList = schema.anyOf || schema.oneOf;
-    let reqPropDefined = true;
-    childList.forEach(childObj => {
-      if (!checkSchemaForProp(requiredProp, childObj)) {
-        reqPropDefined = false;
-      }
-    });
-    return reqPropDefined;
-  }
-  return false;
-}

packages/ruleset/src/functions/string-boundary.js

@@ -1,72 +1,40 @@
+const { validateSubschemas } = require('../utils');
+
 module.exports = function(schema, _opts, { path }) {
-  return traverseSchema(schema, path);
+  return validateSubschemas(schema, path, stringBoundaryErrors);
 };
 
-function traverseSchema(schema, path) {
-  if (schema.type === 'string') {
-    return stringBoundaryErrors(schema, path);
-  }
+function stringBoundaryErrors(schema, path) {
   const errors = [];
-  if (schema.properties) {
-    Object.entries(schema.properties).forEach(function(prop) {
-      const propName = prop[0];
-      const propSchema = prop[1];
-      errors.push(
-        ...traverseSchema(propSchema, [...path, 'properties', propName])
-      );
-    });
-  } else if (schema.items) {
-    errors.push(...traverseSchema(schema.items, [...path, 'items']));
-  } else if (schema.allOf || schema.anyOf || schema.oneOf) {
-    const whichComposedSchemaType = schema.allOf
-      ? 'allOf'
-      : schema.anyOf
-      ? 'anyOf'
-      : 'oneOf';
-    const composedSchemas = schema[whichComposedSchemaType];
-    if (Array.isArray(composedSchemas)) {
-      composedSchemas.forEach(function(composedSchema, index) {
-        errors.push(
-          ...traverseSchema(composedSchema, [
-            ...path,
-            whichComposedSchemaType,
-            index
-          ])
-        );
-      });
-    }
+  if (schema.type !== 'string') {
+    return errors;
   }
-  return errors;
-}
-
-function stringBoundaryErrors(stringSchema, path) {
-  const errors = [];
-  if (isUndefinedOrNull(stringSchema.enum)) {
+  if (isUndefinedOrNull(schema.enum)) {
     if (
-      isUndefinedOrNull(stringSchema.pattern) &&
-      !['binary', 'date', 'date-time'].includes(stringSchema.format)
+      isUndefinedOrNull(schema.pattern) &&
+      !['binary', 'date', 'date-time'].includes(schema.format)
     ) {
       errors.push({
         message: 'Should define a pattern for a valid string',
         path
       });
     }
-    if (isUndefinedOrNull(stringSchema.minLength)) {
+    if (isUndefinedOrNull(schema.minLength)) {
       errors.push({
         message: 'Should define a minLength for a valid string',
         path
       });
     }
-    if (isUndefinedOrNull(stringSchema.maxLength)) {
+    if (isUndefinedOrNull(schema.maxLength)) {
       errors.push({
         message: 'Should define a maxLength for a valid string',
         path
       });
     }
     if (
-      !isUndefinedOrNull(stringSchema.minLength) &&
-      !isUndefinedOrNull(stringSchema.maxLength) &&
-      stringSchema.minLength > stringSchema.maxLength
+      !isUndefinedOrNull(schema.minLength) &&
+      !isUndefinedOrNull(schema.maxLength) &&
+      schema.minLength > schema.maxLength
     ) {
       errors.push({
         message: 'minLength must be less than maxLength',

packages/ruleset/src/ibm-oas.js

@@ -90,6 +90,7 @@ module.exports = {
     // IBM Custom Rules
 
     'content-entry-provided': ibmRules.contentEntryProvided,
+    discriminator: ibmRules.discriminator,
     'content-entry-contains-schema': ibmRules.contentEntryContainsSchema,
     'ibm-content-type-is-specific': ibmRules.ibmContentTypeIsSpecific,
     'ibm-error-content-type-is-json': ibmRules.ibmErrorContentTypeIsJson,

packages/ruleset/src/rules/discriminator.js

@@ -0,0 +1,15 @@
+const { oas3 } = require('@stoplight/spectral-formats');
+const { discriminator } = require('../functions');
+const { schemas } = require('../collections');
+
+module.exports = {
+  description: 'The discriminator property name must be defined in this schema',
+  message: '{{error}}',
+  given: schemas,
+  severity: 'error',
+  formats: [oas3],
+  resolved: true,
+  then: {
+    function: discriminator
+  }
+};

packages/ruleset/src/rules/index.js

@@ -1,4 +1,5 @@
 const contentEntryContainsSchema = require('./content-entry-contains-schema');
+const discriminator = require('./discriminator');
 const ibmErrorContentTypeIsJson = require('./ibm-error-content-type-is-json');
 const missingRequiredProperty = require('./missing-required-property');
 const responseErrorResponseSchema = require('./response-error-response-schema');
@@ -16,6 +17,7 @@ const stringBoundary = require('./string-boundary');
 
 module.exports = {
   contentEntryContainsSchema,
+  discriminator,
   ibmErrorContentTypeIsJson,
   missingRequiredProperty,
   responseErrorResponseSchema,

packages/ruleset/src/rules/missing-required-property.js

@@ -1,15 +1,12 @@
 const { oas2, oas3 } = require('@stoplight/spectral-formats');
 const { requiredProperty } = require('../functions');
+const { schemas } = require('../collections');
 
 module.exports = {
   description: 'A required property is not in the schema',
   message: '{{error}}',
   formats: [oas2, oas3],
-  given: [
-    '$.paths[*][*][parameters][*].schema',
-    '$.paths[*][*][parameters,responses][*].content[*].schema',
-    '$.paths[*][*][requestBody].content[*].schema'
-  ],
+  given: schemas,
   severity: 'error',
   then: {
     function: requiredProperty

packages/ruleset/src/utils/check-subschemas-for-prop.js

@@ -0,0 +1,36 @@
+const checkSubschemasForProperty = (schema, name) => {
+  if (!schema) {
+    return false;
+  }
+
+  let propertyIsDefined = false;
+
+  // first check the properties
+  if (schema.properties) {
+    propertyIsDefined = name in schema.properties;
+  } else if (schema.oneOf || schema.anyOf) {
+    // every schema in a oneOf or anyOf must contain the property
+    const subschemas = schema.oneOf || schema.anyOf;
+    if (Array.isArray(subschemas)) {
+      propertyIsDefined = true;
+      for (const s of subschemas) {
+        if (!checkSubschemasForProperty(s, name)) {
+          propertyIsDefined = false;
+          break;
+        }
+      }
+    }
+  } else if (Array.isArray(schema.allOf)) {
+    // at least one schema in an allOf must contain the property
+    for (const s of schema.allOf) {
+      if (checkSubschemasForProperty(s, name)) {
+        propertyIsDefined = true;
+        break;
+      }
+    }
+  }
+
+  return propertyIsDefined;
+};
+
+module.exports = checkSubschemasForProperty;

packages/ruleset/src/utils/index.js

@@ -0,0 +1,7 @@
+const checkSubschemasForProperty = require('./check-subschemas-for-prop');
+const validateSubschemas = require('./validate-subschemas');
+
+module.exports = {
+  checkSubschemasForProperty,
+  validateSubschemas
+};