feat: improve our AJV usage (#215)

* feat: merge similar ajv errors

* fix: more reasonable stylish-cli logging for oas-schema rule

* feat: include OAI number formats

* chore: cleanup imports

* test: improve tests of formats

* chore: rename validator to oasFormatValidator

* refactor: deprecate summary property

* chore: add a comment

* test: summary field

* chore: comment on oclif & declare var

package.json

@@ -64,6 +64,7 @@
     "@stoplight/types": "^5.1.0",
     "@stoplight/yaml": "^2.3.0",
     "ajv": "6.x.x",
+    "ajv-oai": "^1.1.1",
     "chalk": "^2.4.2",
     "jsonpath": "https://github.com/stoplightio/jsonpath#f1c0e9e634da2d45671b7639fa0a99afc807da17",
     "lodash": ">=4.17.5",

src/__tests__/__fixtures__/petstore.invalid-schema.oas3.yaml

@@ -0,0 +1,58 @@
+openapi: 3.0.0
+info:
+  version: 1.0.0
+  title: Swagger Petstore
+  license:
+    name: MIT
+  contact:
+    email: bar@foo
+  description: test
+servers:
+  - url: 'http://petstore.swagger.io/v1'
+paths:
+  /pets:
+    get:
+      summary: List all pets
+      operationId: listPets
+      description: test
+      tags:
+        - pets
+      parameters:
+        - name: limit
+          in: query
+          description: How many items to return at one time (max 100)
+          required: false
+          schema:
+            type: integer
+            format: int32
+      responses:
+        '200':
+          description: A paged array of pets
+          headers:
+            x-next:
+              description: A link to the next page of responses
+              schema:
+                type: string
+            header-1:
+              type: string
+              op: foo
+          content:
+            application/json:
+              schema:
+                $ref: ./models/pet.yaml
+        default:
+          description: unexpected error
+          content:
+            application/json:
+              schema:
+                $ref: ../common/models/error.yaml
+components:
+  schemas:
+    Pets:
+      type: array
+      items:
+        $ref: ./models/pet.yaml
+      x-tags:
+        - Pets
+      title: Pets
+      description: A list of pets.

src/__tests__/linter.test.ts

@@ -1,8 +1,16 @@
 import { DiagnosticSeverity } from '@stoplight/types';
+import * as fs from 'fs';
+import * as path from 'path';
 
 import { oas2Functions, oas2Rules } from '../rulesets/oas2';
+import { oas3Functions, oas3Rules } from '../rulesets/oas3';
 import { Spectral } from '../spectral';
 
+const invalidSchema = fs.readFileSync(
+  path.join(__dirname, './__fixtures__/petstore.invalid-schema.oas3.yaml'),
+  'utf-8',
+);
+
 const fnName = 'fake';
 const fnName2 = 'fake2';
 const target = {
@@ -89,7 +97,7 @@ describe('linter', () => {
       },
     });
 
-    expect(result[0]).toEqual({
+    expect(result[0]).toMatchObject({
       code: 'rule1',
       message,
       severity: DiagnosticSeverity.Warning,
@@ -189,6 +197,34 @@ responses:: !!foo
     );
   });
 
+  test('should merge similar ajv errors', async () => {
+    spectral.addRules(oas3Rules());
+    spectral.addFunctions(oas3Functions());
+
+    const result = await spectral.run(invalidSchema);
+
+    expect(result).toEqual([
+      expect.objectContaining({
+        code: 'oas3-schema',
+        message: 'should NOT have additional properties: type',
+        summary: 'should NOT have additional properties: type',
+        path: ['paths', '/pets', 'get', 'responses', '200', 'headers', 'header-1'],
+      }),
+      expect.objectContaining({
+        code: 'oas3-schema',
+        message: 'should match exactly one schema in oneOf',
+        summary: 'should match exactly one schema in oneOf',
+        path: ['paths', '/pets', 'get', 'responses', '200', 'headers', 'header-1'],
+      }),
+      expect.objectContaining({
+        code: 'oas3-schema',
+        message: "should have required property '$ref'",
+        summary: "should have required property '$ref'",
+        path: ['paths', '/pets', 'get', 'responses', '200', 'headers', 'header-1'],
+      }),
+    ]);
+  });
+
   describe('functional tests for the given property', () => {
     let fakeLintingFunction: any;
 

src/cli/commands/__tests__/lint.test.ts

@@ -57,7 +57,7 @@ describe('lint', () => {
           .command([...args, '-f', 'json'])
           .it('outputs warnings in json format', ctx => {
             expect(ctx.stdout).toContain('OpenAPI `servers` must be present and non-empty array');
-            expect(ctx.stdout).toContain('"info.contact is not truthy"');
+            expect(ctx.stdout).toContain('"Info object should contain `contact` object."');
           });
       });
 
@@ -243,9 +243,9 @@ describe('lint', () => {
       .stdout()
       .command(['lint', invalidSpecPath, '-c', validConfigPath])
       .it('outputs warnings in json format', ctx => {
-        expect(ctx.stdout).toContain('"info.contact is not truthy"');
-        expect(ctx.stdout).toContain('"info.description is not truthy"');
-        expect(ctx.stdout).toContain('"servers does not exist"');
+        expect(ctx.stdout).toContain('"Info object should contain `contact` object."');
+        expect(ctx.stdout).toContain('"OpenAPI object info `description` must be present and non-empty string."');
+        expect(ctx.stdout).toContain('"OpenAPI `servers` must be present and non-empty array."');
       });
 
     test
@@ -281,9 +281,9 @@ describe('lint', () => {
       .stdout()
       .command(['lint', invalidSpecPath, '-c', validConfigPath, '--max-results', '1'])
       .it('setting --max-results to 1 will override config value of 5', ctx => {
-        expect(ctx.stdout).toContain('"info.contact is not truthy"');
-        expect(ctx.stdout).not.toContain('"info.description is not truthy"');
-        expect(ctx.stdout).not.toContain('"servers does not exist"');
+        expect(ctx.stdout).toContain('"Info object should contain `contact` object."');
+        expect(ctx.stdout).not.toContain('"OpenAPI object info `description` must be present and non-empty string."');
+        expect(ctx.stdout).not.toContain('"OpenAPI `servers` must be present and non-empty array."');
       });
   });
 
@@ -307,7 +307,7 @@ describe('lint', () => {
       .stdout()
       .command(['lint', invalidSpecPath])
       .it('outputs data in format from default config file', ctx => {
-        expect(ctx.stdout).toContain('"info.contact is not truthy"');
+        expect(ctx.stdout).toContain('"Info object should contain `contact` object."');
       });
   });
 });

src/formatters/__tests__/stylish.test.ts

@@ -0,0 +1,67 @@
+import { stylish } from '../stylish';
+
+const results = [
+  {
+    code: 'oas3-schema',
+    summary: 'Validate structure of OpenAPIv3 specification.',
+    message: 'should NOT have additional properties: type',
+    path: ['paths', '/pets', 'get', 'responses', '200', 'headers', 'header-1'],
+    severity: 0,
+    source: '/home/Stoplight/spectral/src/__tests__/__fixtures__/petstore.invalid-schema.oas3.yaml',
+    range: {
+      start: {
+        line: 35,
+        character: 21,
+      },
+      end: {
+        line: 37,
+        character: 21,
+      },
+    },
+  },
+  {
+    code: 'oas3-schema',
+    summary: 'Validate structure of OpenAPIv3 specification.',
+    message: 'should match exactly one schema in oneOf',
+    path: ['paths', '/pets', 'get', 'responses', '200', 'headers', 'header-1'],
+    severity: 0,
+    source: '/home/Stoplight/spectral/src/__tests__/__fixtures__/petstore.invalid-schema.oas3.yaml',
+    range: {
+      start: {
+        line: 35,
+        character: 21,
+      },
+      end: {
+        line: 37,
+        character: 21,
+      },
+    },
+  },
+  {
+    code: 'oas3-schema',
+    summary: 'Validate structure of OpenAPIv3 specification.',
+    message: "should have required property '$ref'",
+    path: ['paths', '/pets', 'get', 'responses', '200', 'headers', 'header-1'],
+    severity: 0,
+    source: '/home/Stoplight/spectral/src/__tests__/__fixtures__/petstore.invalid-schema.oas3.yaml',
+    range: {
+      start: {
+        line: 35,
+        character: 21,
+      },
+      end: {
+        line: 37,
+        character: 21,
+      },
+    },
+  },
+];
+
+describe('Stylish formatter', () => {
+  test('should prefer message for oas-schema errors', () => {
+    const result = stylish(results);
+    expect(result).toContain('oas3-schema  should NOT have additional properties: type\n');
+    expect(result).toContain('oas3-schema  should match exactly one schema in oneOf');
+    expect(result).toContain("oas3-schema  should have required property '$ref'\n");
+  });
+});

src/formatters/stylish.ts

@@ -86,12 +86,7 @@ export const stylish = (results: IRuleResult[]): string => {
         messageType = chalk.yellow('white');
       }
 
-      return [
-        formatRange(result.range),
-        messageType,
-        result.code !== undefined ? result.code : '',
-        result.summary ? result.summary.replace(/([^ ])\.$/u, '$1') : result.message,
-      ];
+      return [formatRange(result.range), messageType, result.code !== undefined ? result.code : '', result.message];
     });
 
     output += `${table(pathTableData, {

src/functions/schema.ts

@@ -1,6 +1,8 @@
 import { decodePointerFragment } from '@stoplight/json';
 import * as AJV from 'ajv';
 import * as jsonSpecv4 from 'ajv/lib/refs/json-schema-draft-04.json';
+const oasFormatValidator = require('ajv-oai/lib/format-validator');
+import { IFunction, IFunctionResult, ISchemaOptions } from '../types';
 
 const ajv = new AJV({
   meta: false,
@@ -14,7 +16,31 @@ ajv._opts.defaultMeta = jsonSpecv4.id;
 // @ts-ignore
 ajv._refs['http://json-schema.org/schema'] = 'http://json-schema.org/draft-04/schema';
 
-import { IFunction, IFunctionResult, ISchemaOptions } from '../types';
+ajv.addFormat('int32', { type: 'number', validate: oasFormatValidator.int32 });
+ajv.addFormat('int64', { type: 'number', validate: oasFormatValidator.int64 });
+ajv.addFormat('float', { type: 'number', validate: oasFormatValidator.float });
+ajv.addFormat('double', { type: 'number', validate: oasFormatValidator.double });
+ajv.addFormat('byte', { type: 'string', validate: oasFormatValidator.byte });
+
+const formatPath = (path: string) =>
+  path
+    .split('/')
+    .slice(1)
+    .map(decodePointerFragment);
+
+const mergeErrors = (existingError: IFunctionResult, newError: AJV.ErrorObject) => {
+  switch (newError.keyword) {
+    case 'additionalProperties': {
+      const { additionalProperty } = newError.params as AJV.AdditionalPropertiesParams;
+      if (!new RegExp(`[:,] ${additionalProperty}`).test(existingError.message)) {
+        existingError.message += `, ${(newError.params as AJV.AdditionalPropertiesParams).additionalProperty}`;
+      }
+      return true;
+    }
+    default:
+      return existingError.message === newError.message;
+  }
+};
 
 export const schema: IFunction<ISchemaOptions> = (targetVal, opts, paths) => {
   const results: IFunctionResult[] = [];
@@ -31,25 +57,33 @@ export const schema: IFunction<ISchemaOptions> = (targetVal, opts, paths) => {
 
   const { schema: schemaObj } = opts;
 
-  // TODO: potential performance improvements (compile, etc)?
   if (!ajv.validate(schemaObj, targetVal) && ajv.errors) {
-    ajv.errors.forEach((e: AJV.ErrorObject) => {
-      // @ts-ignore
-      if (e.params && e.params.additionalProperty) {
-        // @ts-ignore
-        e.message = e.message + ': ' + e.params.additionalProperty;
+    // TODO: potential performance improvements (compile, etc)?
+    const collectedErrors: string[] = [];
+
+    for (const error of ajv.errors) {
+      if (collectedErrors.length > 0) {
+        const index = collectedErrors.indexOf(error.keyword);
+        if (index !== -1) {
+          if (mergeErrors(results[index], error)) continue;
+        }
+      }
+
+      let message = error.message || '';
+
+      if (
+        error.keyword === 'additionalProperties' &&
+        (error.params as AJV.AdditionalPropertiesParams).additionalProperty
+      ) {
+        message += `: ${(error.params as AJV.AdditionalPropertiesParams).additionalProperty}`;
       }
 
+      collectedErrors.push(error.keyword);
       results.push({
-        path: path.concat(
-          e.dataPath
-            .split('/')
-            .slice(1)
-            .map(frag => decodePointerFragment(frag)),
-        ),
-        message: e.message ? e.message : '',
+        path: [...path, ...formatPath(error.dataPath)],
+        message,
       });
-    });
+    }
   }
 
   return results;

src/linter.ts

@@ -88,13 +88,28 @@ export const lintNode = (
 
     results = results.concat(
       targetResults.map(result => {
-        const location = parsedResult.getLocationForJsonPath(parsed, result.path || targetPath, true);
+        const path = result.path || targetPath;
+        const location = parsedResult.getLocationForJsonPath(parsed, path, true);
 
         return {
           code: rule.name,
-          summary: rule.summary,
-          message: result.message,
-          path: result.path || targetPath,
+
+          // @deprecated, points to message
+          get summary() {
+            return this.message;
+          },
+
+          message:
+            rule.message === undefined
+              ? rule.summary || result.message
+              : typeof rule.message === 'function'
+                ? rule.message({
+                    error: result.message,
+                    property: path.length > 0 ? path[path.length - 1] : '',
+                    description: rule.description,
+                  })
+                : rule.message,
+          path,
           severity,
           source: parsedResult.source,
           ...(location || {

src/rulesets/message.ts

@@ -0,0 +1,19 @@
+export interface IMessageVars {
+  property: string | number;
+  error: string;
+  description?: string;
+}
+
+export type MessageInterpolator = (values: IMessageVars) => string;
+
+export function message(strings: TemplateStringsArray, ...vars: Array<keyof IMessageVars>) {
+  return (values: IMessageVars) => {
+    const result = [strings[0]];
+
+    for (const [i, key] of vars.entries()) {
+      result.push(String(values[key] || ''), strings[i + 1]);
+    }
+
+    return result.join('');
+  };
+}