Report error if 'discriminator' is not required (#439)

* Report error if 'discriminator' is not required

Issue #386 OAV report error if the property for 'discriminator' is not a
requried property.

* Update error object.

* Add an unittest.

* Add suppression process for discriminator.

* Fix review comment and bump up package version.

ChangeLog.md

@@ -1,5 +1,9 @@
 # Changelog
 
+## 07/12/2019 0.19.2
+
+- Add support for validating discriminator is required and also the support for suppression.[Issue#386](https://github.com/Azure/oav/issues/386).
+
 ## 07/02/2019 0.19.1
 
 - Update TypeScript to 3.5

lib/util/constants.ts

@@ -100,6 +100,10 @@ export const ErrorCodes = {
   ResponseBodyNotInExample: {
     name: "RESPONSE_BODY_NOT_IN_EXAMPLE",
     id: "OAV130"
+  },
+  DiscriminatorNotRequired: {
+    name: "DISCRIMINATOR_NOT_REQUIRED",
+    id: "OAV131"
   }
 }
 

lib/validate.ts

@@ -122,9 +122,10 @@ export async function validateSpec(
     o.shouldResolveNullableTypes = false
 
     const validator = new SemanticValidator(specPath, null, o)
-    await validator.initialize()
+    const suppression = await getSuppressions(specPath)
+    await validator.initialize(suppression)
     log.info(`Semantically validating  ${specPath}:\n`)
-    const validationResults = await validator.validateSpec()
+    const validationResults = await validator.validateSpec(specPath, suppression)
     updateEndResultOfSingleValidation(validator)
     logDetailedInfo(validator)
     if (o.pretty) {

lib/validators/semanticValidator.ts

@@ -1,7 +1,8 @@
 // Copyright (c) Microsoft Corporation. All rights reserved.
 // Licensed under the MIT License. See License.txt in the project root for license information.
 
-import { keys } from "@ts-common/string-map"
+import * as amd from "@azure/openapi-markdown"
+import * as sm from "@ts-common/string-map"
 import * as util from "util"
 import * as Sway from "yasway"
 
@@ -11,7 +12,9 @@ import { log } from "../util/logging"
 import { processErrors } from "../util/processErrors"
 import { validateResponse } from "../util/validationResponse"
 
+import { TitleObject } from "./specTransformer"
 import { CommonValidationResult, SpecValidator } from "./specValidator"
+import { existSuppression } from "./suppressions"
 
 export interface Result {
   isValid?: unknown
@@ -28,7 +31,10 @@ export interface SemanticValidationResult extends CommonValidationResult {
 }
 
 export class SemanticValidator extends SpecValidator<SemanticValidationResult> {
-  public async validateSpec(): Promise<Sway.ValidationResults> {
+  public async validateSpec(
+    specPath?: string,
+    suppression?: amd.Suppression
+  ): Promise<Sway.ValidationResults> {
     this.specValidationResult.validateSpec = {
       isValid: true,
       errors: [],
@@ -50,6 +56,19 @@ export class SemanticValidator extends SpecValidator<SemanticValidationResult> {
     try {
       const validationResult = this.swaggerApi.validate()
       if (validationResult) {
+        if (
+          (suppression !== undefined &&
+            specPath !== undefined &&
+            !existSuppression(specPath, suppression, C.ErrorCodes.DiscriminatorNotRequired.id)) ||
+          suppression === undefined ||
+          specPath === undefined
+        ) {
+          const discriminatorValidationResult = this.validateDiscriminator()
+          if (discriminatorValidationResult) {
+            validationResult.errors = validationResult.errors.concat(discriminatorValidationResult)
+          }
+        }
+
         if (validationResult.errors && validationResult.errors.length) {
           this.specValidationResult.validateSpec.isValid = false
           processErrors(validationResult.errors)
@@ -99,7 +118,7 @@ export class SemanticValidator extends SpecValidator<SemanticValidationResult> {
     const re = /^(.*)\/providers\/(\w+\.\w+)\/(.*)$/gi
     if (this.specInJson) {
       if (this.specInJson.paths) {
-        for (const pathStr of keys(this.specInJson.paths)) {
+        for (const pathStr of sm.keys(this.specInJson.paths)) {
           const res = re.exec(pathStr)
           if (res && res[2]) {
             return res[2]
@@ -109,4 +128,38 @@ export class SemanticValidator extends SpecValidator<SemanticValidationResult> {
     }
     return null
   }
+
+  /**
+   * Validate discriminator must be a required property
+   */
+  private validateDiscriminator(): Sway.ValidationEntry[] {
+    const spec = this.specInJson
+    const definitions = spec.definitions as Sway.DefinitionsObject
+    const validationEntries: Sway.ValidationEntry[] = []
+    for (const modelEntry of sm.entries(definitions)) {
+      const model = modelEntry[1]
+      const discriminator = model.discriminator
+      if (discriminator) {
+        if (!model.required || !model.required.includes(discriminator)) {
+          let path = modelEntry[0]
+          if (model.title) {
+            const title: TitleObject = JSON.parse(model.title)
+            if (title.path) {
+              path = title.path.join()
+            }
+          }
+          const validateEntry: Sway.ValidationEntry = {
+            code: C.ErrorCodes.DiscriminatorNotRequired.id,
+            error: C.ErrorCodes.DiscriminatorNotRequired.name,
+            name: "discriminator",
+            params: [discriminator],
+            message: "discriminator must be a required property.",
+            path: [path]
+          } as any
+          validationEntries.push(validateEntry)
+        }
+      }
+    }
+    return validationEntries
+  }
 }

lib/validators/specValidator.ts

@@ -179,13 +179,14 @@ export class SpecValidator<T extends CommonValidationResult> {
    * Initializes the spec validator. Resolves the spec on different counts using the SpecResolver
    * and initializes the internal api validator.
    */
-  public async initialize(): Promise<Sway.SwaggerApi> {
+  public async initialize(suppression?: amd.Suppression): Promise<Sway.SwaggerApi> {
     const errors: jsonParser.ParseError[] = []
     const reportError = (e: jsonParser.ParseError) => errors.push(e)
     try {
-      let suppression: amd.Suppression | undefined
       if (this.specInJson === undefined || this.specInJson === null) {
-        suppression = await getSuppressions(this.specPath)
+        if (suppression === undefined) {
+          suppression = await getSuppressions(this.specPath)
+        }
         const result = await jsonUtils.parseJson(
           suppression,
           this.specPath,

lib/validators/suppressions.ts

@@ -3,9 +3,10 @@
 
 import * as amd from "@azure/openapi-markdown"
 import * as md from "@ts-common/commonmark-to-markdown"
-import { isArray } from "@ts-common/iterator"
+import * as it from "@ts-common/iterator"
 import * as vfs from "@ts-common/virtual-fs"
 import * as path from "path"
+import { isSubPath, splitPathAndReverse } from "../util/path"
 
 export const getSuppressions = async (specPath: string): Promise<undefined | amd.Suppression> => {
   // find readme.md
@@ -20,8 +21,32 @@ export const getSuppressions = async (specPath: string): Promise<undefined | amd
     return undefined
   }
   const suppression = amd.getYamlFromNode(suppressionCodeBlock) as amd.Suppression
-  if (!isArray(suppression.directive)) {
+  if (!it.isArray(suppression.directive)) {
     return undefined
   }
   return suppression
 }
+
+export function existSuppression(
+  specPath: string,
+  suppression: amd.Suppression,
+  id: string
+): boolean {
+  if (suppression.directive !== undefined) {
+    const suppressionArray = getSuppressionArray(specPath, suppression.directive)
+    return it.some(suppressionArray, s => s.suppress === id)
+  }
+  return false
+}
+
+const getSuppressionArray = (
+  specPath: string,
+  suppressionItems: ReadonlyArray<amd.SuppressionItem>
+): ReadonlyArray<amd.SuppressionItem> => {
+  const urlReversed = splitPathAndReverse(specPath)
+  return suppressionItems.filter(s =>
+    it.some(it.isArray(s.from) ? s.from : [s.from], from =>
+      isSubPath(urlReversed, splitPathAndReverse(from))
+    )
+  )
+}

package.json

@@ -1,6 +1,6 @@
 {
   "name": "oav",
-  "version": "0.19.1",
+  "version": "0.19.2",
   "author": {
     "name": "Microsoft Corporation",
     "email": "azsdkteam@microsoft.com",

test/semanticValidation/specification/invalid/notRequiredDiscriminator.json

@@ -0,0 +1,52 @@
+{
+  "swagger": "2.0",
+  "info": {
+    "title": "title",
+    "description": "",
+    "version": "2016-11-01"
+  },
+  "host": "host",
+  "schemes": [
+    "https"
+  ],
+  "paths": {
+    "/hello": {
+      "get": {
+        "tags": [
+          "Insipid"
+        ],
+        "operationId": "Insipid_Hello",
+        "description": "Very boring operation",
+        "responses": {
+          "200": {
+            "description": "OK",
+            "schema": {
+              "$ref": "#/definitions/Artifact"
+            }
+          }
+        }
+      }
+    }
+  },
+  "definitions": {
+    "Artifact": {
+      "type": "object",
+      "discriminator": "kind",
+      "description": "Represents an artifact.",
+      "properties": {
+        "kind": {
+          "type": "string",
+          "description": "Specifies the kind of artifact.",
+          "enum": [
+            "template",
+            "role"
+          ],
+          "x-ms-enum": {
+            "name": "ArtifactKind",
+            "modelAsString": true
+          }
+        }
+      }
+    }
+  }
+}

test/semanticValidation/specification/invalid/notRequiredDiscriminatorWithSuppression.json

@@ -0,0 +1,52 @@
+{
+  "swagger": "2.0",
+  "info": {
+    "title": "title",
+    "description": "",
+    "version": "2016-11-01"
+  },
+  "host": "host",
+  "schemes": [
+    "https"
+  ],
+  "paths": {
+    "/hello": {
+      "get": {
+        "tags": [
+          "Insipid"
+        ],
+        "operationId": "Insipid_Hello",
+        "description": "Very boring operation",
+        "responses": {
+          "200": {
+            "description": "OK",
+            "schema": {
+              "$ref": "#/definitions/Artifact"
+            }
+          }
+        }
+      }
+    }
+  },
+  "definitions": {
+    "Artifact": {
+      "type": "object",
+      "discriminator": "kind",
+      "description": "Represents an artifact.",
+      "properties": {
+        "kind": {
+          "type": "string",
+          "description": "Specifies the kind of artifact.",
+          "enum": [
+            "template",
+            "role"
+          ],
+          "x-ms-enum": {
+            "name": "ArtifactKind",
+            "modelAsString": true
+          }
+        }
+      }
+    }
+  }
+}

test/semanticValidation/specification/invalid/readme.md

@@ -0,0 +1,9 @@
+# notRequiredDiscriminatorWithSuppression
+
+## Suppression
+
+``` yaml
+directive:
+  - from: notRequiredDiscriminatorWithSuppression.json
+    suppress: OAV131 
+    reason: Testing purpose.

test/semanticValidatorTests.ts

@@ -36,4 +36,16 @@ describe("Semantic validation", () => {
     assert(result.validityStatus === false)
     assert.strictEqual((result.resolveSpec as any).code, constants.ErrorCodes.JsonParsingError.name)
   })
+
+  it("should fail when discriminator is not a required property", async () => {
+    const specPath = `${testPath}/semanticValidation/specification/invalid/notRequiredDiscriminator.json`
+    const result = await validate.validateSpec(specPath, undefined)
+    assert(result.validityStatus === false)
+  })
+
+  it("should succeed when discriminator is not a required property and the error is suppressed", async () => {
+    const specPath = `${testPath}/semanticValidation/specification/invalid/notRequiredDiscriminatorWithSuppression.json`
+    const result = await validate.validateSpec(specPath, undefined)
+    assert(result.validityStatus === true)
+  })
 })