refactor: validators return errors, not throw them (#691)

---------

Signed-off-by: Jan Kowalleck <[email protected]>

Author: jkowalleck

HISTORY.md

@@ -5,14 +5,13 @@ All notable changes to this project will be documented in this file.
 ## unreleased
 
 * Added
-  * Formal validators for JSON string and XML string ([#620] via [#652])  
+  * Formal validators for JSON string and XML string ([#620] via [#652], [#691])  
     Currently available only for _Node.js_. Requires [optional dependencies](README.md#optional-dependencies).
     * Related new validator classes:
       * `Validation.JsonValidator`
       * `Validation.JsonStrictValidator`
       * `Validation.XmlValidator`
     * Related new error classes:
-      * `Validation.ValidationError`
       * `Validation.NotImplementedError`
       * `Validation.MissingOptionalDependencyError`
 * Build
@@ -24,6 +23,7 @@ All notable changes to this project will be documented in this file.
 [#644]: https://github.com/CycloneDX/cyclonedx-javascript-library/pull/644
 [#652]: https://github.com/CycloneDX/cyclonedx-javascript-library/pull/652
 [#686]: https://github.com/CycloneDX/cyclonedx-javascript-library/pull/686
+[#691]: https://github.com/CycloneDX/cyclonedx-javascript-library/pull/691
 
 ## 1.13.3 - 2023-04-05
 

src/validation/baseValidator.ts

@@ -18,18 +18,7 @@ Copyright (c) OWASP Foundation. All Rights Reserved.
 */
 
 import type { Version } from '../spec'
-
-export interface Validator {
-  /**
-   * validate the data.
-   *
-   * Promise may reject with one of the following:
-   * - {@link Validation.NotImplementedError | NotImplementedError} when there is no validator available for `this.version`
-   * - {@link Validation.MissingOptionalDependencyError | MissingOptionalDependencyError} when a required dependency was not installed
-   * - {@link Validation.ValidationError | ValidationError} when `data` was invalid to the schema
-   */
-  validate: (data: string) => Promise<void>
-}
+import type { ValidationError, Validator } from './types'
 
 export abstract class BaseValidator implements Validator {
   readonly #version: Version
@@ -42,6 +31,6 @@ export abstract class BaseValidator implements Validator {
     return this.#version
   }
 
-  /** {@inheritDoc Validator.validate} */
-  abstract validate (data: string): Promise<void>
+  /** {@inheritDoc Validation.Types.Validator.validate} */
+  abstract validate (data: string): Promise<null | ValidationError>
 }

src/validation/errors.ts

@@ -19,15 +19,6 @@ Copyright (c) OWASP Foundation. All Rights Reserved.
 
 import { type Version } from '../spec'
 
-export class ValidationError extends Error {
-  readonly details: any | undefined
-
-  constructor (message: string, details?: any) {
-    super(message)
-    this.details = details
-  }
-}
-
 export class NotImplementedError extends Error {
   constructor (version: Version) {
     super(`not implemented for CycloneDX version: ${version}`)

src/validation/index.common.ts

@@ -18,4 +18,4 @@ Copyright (c) OWASP Foundation. All Rights Reserved.
 */
 
 export * from './errors'
-export * from './types'
+export * as Types from './types'

src/validation/jsonValidator.node.ts

@@ -23,7 +23,8 @@ import { readFile } from 'fs/promises'
 
 import { FILES } from '../resources.node'
 import { BaseValidator } from './baseValidator'
-import { MissingOptionalDependencyError, NotImplementedError, ValidationError } from './errors'
+import { MissingOptionalDependencyError, NotImplementedError } from './errors'
+import type { ValidationError } from './types'
 
 let _ajv: Ajv | undefined
 
@@ -101,19 +102,22 @@ abstract class BaseJsonValidator extends BaseValidator {
   /**
    * Validate the data against CycloneDX spec of `this.version`.
    *
-   * Promise may reject with one of the following:
-   * - {@link Validation.NotImplementedError | NotImplementedError} when there is no validator available for `this.version`
-   * - {@link Validation.MissingOptionalDependencyError | MissingOptionalDependencyError} when a required dependency was not installed
-   * - {@link Validation.ValidationError | ValidationError} when `data` was invalid to the schema
+   * Promise completes with one of the following:
+   * - `null`, when data was valid
+   * - {@link Validation.Types.ValidationError | something} representing the error details, when data was invalid representing the error details, when data was invalid
+   *
+   * Promise rejects with one of the following:
+   * - {@link Validation.NotImplementedError | NotImplementedError}, when there is no validator available for `this.version`
+   * - {@link Validation.MissingOptionalDependencyError | MissingOptionalDependencyError}, when a required dependency was not installed
    */
-  async validate (data: string): Promise<void> {
+  async validate (data: string): Promise<null | ValidationError> {
     const [doc, validator] = await Promise.all([
       (async () => JSON.parse(data))(),
       this.#getValidator()
     ])
-    if (!validator(doc)) {
-      throw new ValidationError(`invalid to CycloneDX ${this.version}`, validator.errors)
-    }
+    return validator(doc)
+      ? null
+      : validator.errors
   }
 }
 export class JsonValidator extends BaseJsonValidator {

src/validation/types.ts

@@ -17,12 +17,20 @@ SPDX-License-Identifier: Apache-2.0
 Copyright (c) OWASP Foundation. All Rights Reserved.
 */
 
+/**
+ * Details and information describing a validation error.
+ */
+export type ValidationError = NonNullable<any>
+
 export interface Validator {
   /**
-   * Promise rejects with one of the following
-   * - {@link Validation.NotImplementedError | NotImplementedError} when there is no validator available for `this.version`
-   * - {@link Validation.MissingOptionalDependencyError | MissingOptionalDependencyError} when a required dependency was not installed
-   * - {@link Validation.ValidationError | ValidationError} when `data` was invalid to the schema
+   * Promise completes with one of the following:
+   * - `null`, when data was valid
+   * - {@link ValidationError | something} representing the error details, when data was invalid
+   *
+   * Promise rejects with one of the following:
+   * - {@link Validation.NotImplementedError | NotImplementedError}, when there is no validator available
+   * - {@link Validation.MissingOptionalDependencyError | MissingOptionalDependencyError}, when a required dependency was not installed
    */
-  validate: (data: string) => Promise<void>
+  validate: (data: string) => Promise<null | ValidationError>
 }

src/validation/xmlValidator.node.ts

@@ -23,7 +23,8 @@ import { pathToFileURL } from 'url'
 
 import { FILES } from '../resources.node'
 import { BaseValidator } from './baseValidator'
-import { MissingOptionalDependencyError, NotImplementedError, ValidationError } from './errors'
+import { MissingOptionalDependencyError, NotImplementedError } from './errors'
+import type { ValidationError } from './types'
 
 let _parser: typeof parseXml | undefined
 
@@ -75,19 +76,22 @@ export class XmlValidator extends BaseValidator {
   /**
    * Validate the data against CycloneDX spec of `this.version`.
    *
-   * Promise may reject with one of the following:
-   * - {@link Validation.NotImplementedError | NotImplementedError} when there is no validator available for `this.version`
-   * - {@link Validation.MissingOptionalDependencyError | MissingOptionalDependencyError} when a required dependency was not installed
-   * - {@link Validation.ValidationError | ValidationError} when `data` was invalid to the schema
+   * Promise completes with one of the following:
+   * - `null`, when data was valid
+   * - {@link Validation.Types.ValidationError | something} representing the error details, when data was invalid representing the error details, when data was invalid
+   *
+   * Promise rejects with one of the following:
+   * - {@link Validation.NotImplementedError | NotImplementedError}, when there is no validator available for `this.version`
+   * - {@link Validation.MissingOptionalDependencyError | MissingOptionalDependencyError}, when a required dependency was not installed
    */
-  async validate (data: string): Promise<void> {
+  async validate (data: string): Promise<null | ValidationError> {
     const [parse, schema] = await Promise.all([
       getParser(),
       this.#getSchema()
     ])
     const doc = parse(data, xmlParseOptions)
-    if (!doc.validate(schema)) {
-      throw new ValidationError(`invalid to CycloneDX ${this.version}`, doc.validationErrors)
-    }
+    return doc.validate(schema)
+      ? null
+      : doc.validationErrors
   }
 }

tests/integration/Serialize.JsonSerialize.test.js

@@ -31,7 +31,7 @@ const {
   },
   Spec: { Spec1dot2, Spec1dot3, Spec1dot4 },
   Validation: {
-    ValidationError, MissingOptionalDependencyError,
+    MissingOptionalDependencyError,
     JsonStrictValidator
   }
 } = require('../../')
@@ -65,11 +65,9 @@ describe('Serialize.JsonSerialize', function () {
 
       const validator = new JsonStrictValidator(spec.version)
       try {
-        await validator.validate(serialized)
+        const validationError = await validator.validate(serialized)
+        assert.strictEqual(validationError, null)
       } catch (err) {
-        if (err instanceof ValidationError) {
-          assert.fail(`unexpected ValidationError: ${err.message}\n` + JSON.stringify(err.details))
-        }
         if (!(err instanceof MissingOptionalDependencyError)) {
           // unexpected error
           assert.fail(err)

tests/integration/Serialize.XmlSerialize.test.js

@@ -31,7 +31,7 @@ const {
   },
   Spec: { Spec1dot2, Spec1dot3, Spec1dot4 },
   Validation: {
-    ValidationError, MissingOptionalDependencyError,
+    MissingOptionalDependencyError,
     XmlValidator
   }
 } = require('../../')
@@ -72,11 +72,9 @@ describe('Serialize.XmlSerialize', function () {
 
       const validator = new XmlValidator(spec.version)
       try {
-        await validator.validate(serialized)
+        const validationError = await validator.validate(serialized)
+        assert.strictEqual(validationError, null)
       } catch (err) {
-        if (err instanceof ValidationError) {
-          assert.fail(`unexpected ValidationError: ${err.message}\n` + JSON.stringify(err.details))
-        }
         if (!(err instanceof MissingOptionalDependencyError)) {
           // unexpected error
           assert.fail(err)

tests/integration/Validation.JsonStrictValidator.test.js

@@ -20,8 +20,6 @@ Copyright (c) OWASP Foundation. All Rights Reserved.
 const assert = require('assert')
 const { describe, it } = require('mocha')
 
-const { escapeRegExp } = require('../_helpers/stringFunctions')
-
 let hasDep = true
 try {
   require('ajv')
@@ -32,7 +30,7 @@ try {
 const {
   Spec: { Version },
   Validation: {
-    ValidationError, NotImplementedError, MissingOptionalDependencyError,
+    NotImplementedError, MissingOptionalDependencyError,
     JsonStrictValidator
   }
 } = require('../../')
@@ -45,7 +43,7 @@ describe('Validation.JsonStrictValidator', () => {
     it('throws not implemented', async () => {
       const validator = new JsonStrictValidator(version)
       await assert.rejects(
-        () => validator.validate('{}'),
+        validator.validate('{}'),
         (err) => err instanceof NotImplementedError
       )
     })
@@ -60,7 +58,7 @@ describe('Validation.JsonStrictValidator', () => {
       it('throws MissingOptionalDependencyError', async () => {
         const validator = new JsonStrictValidator(version)
         await assert.rejects(
-          () => validator.validate('{}'),
+          validator.validate('{}'),
           (err) => err instanceof MissingOptionalDependencyError
         )
       })
@@ -78,15 +76,8 @@ describe('Validation.JsonStrictValidator', () => {
           unknown: 'undefined' // << undefined/additional property
         }]
       })
-      await assert.rejects(
-        () => validator.validate(input),
-        (err) => {
-          assert.ok(err instanceof ValidationError)
-          assert.match(err.message, new RegExp(`invalid.* CycloneDX ${escapeRegExp(version)}`, 'i'))
-          assert.notStrictEqual(err.details, undefined)
-          return true
-        }
-      )
+      const validationError = await validator.validate(input)
+      assert.notStrictEqual(validationError, null)
     })
 
     it('valid passes', async () => {
@@ -100,7 +91,8 @@ describe('Validation.JsonStrictValidator', () => {
           version: '1.337'
         }]
       })
-      await validator.validate(input)
+      const validationError = await validator.validate(input)
+      assert.strictEqual(validationError, null)
     })
   }))
 })