Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix #894: oas3-valid-(content-)schema-example has problem with nullable #914

Merged
merged 10 commits into from
Jan 20, 2020
4 changes: 2 additions & 2 deletions src/functions/__tests__/schema.test.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import { JSONSchema4, JSONSchema6 } from 'json-schema';
import { schema } from '../schema';

function runSchema(target: any, schemaObj: object) {
return schema(target, { schema: schemaObj }, { given: [] }, { given: null, original: null } as any);
function runSchema(target: any, schemaObj: object, oasVersion: number = 0) {
return schema(target, { schema: schemaObj, oasVersion }, { given: [] }, { given: null, original: null } as any);
}

describe('schema', () => {
Expand Down
4 changes: 3 additions & 1 deletion src/functions/schema-path.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ export interface ISchemaPathOptions {
schemaPath: string;
// the `path.to.prop` to field, or special `@key` value to target keys for matched `given` object
field?: string;
// The oasVersion, either 2 or 3 for OpenAPI Spec versions, or undefined / 0 for JSON Schema
oasVersion?: number;
}

export type SchemaPathRule = IRule<RuleFunction.SCHEMAPATH, ISchemaPathOptions>;
Expand Down Expand Up @@ -45,5 +47,5 @@ export const schemaPath: IFunction<ISchemaPathOptions> = (targetVal, opts, paths
}
}

return schema(relevantObject, { schema: schemaObject }, paths, otherValues);
return schema(relevantObject, { schema: schemaObject, oasVersion: opts.oasVersion }, paths, otherValues);
};
62 changes: 39 additions & 23 deletions src/functions/schema.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import * as AJV from 'ajv';
import { ValidateFunction } from 'ajv';
import * as jsonSpecv4 from 'ajv/lib/refs/json-schema-draft-04.json';
import * as jsonSpecv6 from 'ajv/lib/refs/json-schema-draft-06.json';
import * as jsonSpecv7 from 'ajv/lib/refs/json-schema-draft-07.json';
import { IOutputError } from 'better-ajv-errors';
import { escapeRegExp } from 'lodash';
import { IFunction, IFunctionResult, IRule, JSONSchema, RuleFunction } from '../types';
Expand All @@ -13,6 +12,8 @@ const betterAjvErrors = require('better-ajv-errors/lib/modern');

export interface ISchemaOptions {
schema: object;
// The oasVersion, either 2 or 3 for OpenAPI Spec versions, or undefined / 0 for JSON Schema
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// The oasVersion, either 2 or 3 for OpenAPI Spec versions, or undefined / 0 for JSON Schema
// The oasVersion, either 2 or 3 for OpenAPI Spec versions, or null / 0 for JSON Schema

Let's go with null instead of 0.
Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I chose 0 so that it can be used in the ajvInstances object as key (and because I was confused with how to enforce null as default value in TS).

Copy link
Contributor Author

@m-mohr m-mohr Jan 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@P0lip I couldn't get your suggestions working and I'm not familiar with how Optional works, so I tried a bit and the solution committed now works without 0. Please check whether that's okay for you.

Now I need to get #915 fixed so that my CI doesn't fail all the time. ;-)

oasVersion?: number;
}

export type SchemaRule = IRule<RuleFunction.SCHEMA, ISchemaOptions>;
Expand All @@ -33,26 +34,40 @@ const logger = {
error: console.error,
};

const ajv = new AJV({
meta: false,
schemaId: 'auto',
jsonPointers: true,
unknownFormats: 'ignore',
logger,
});
ajv.addMetaSchema(jsonSpecv4);
ajv.addMetaSchema(jsonSpecv6);
ajv.addMetaSchema(jsonSpecv7);
// @ts-ignore
ajv._opts.defaultMeta = jsonSpecv4.id;
// @ts-ignore
ajv._refs['http://json-schema.org/schema'] = 'http://json-schema.org/draft-04/schema';

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 ajvInstances = {};
P0lip marked this conversation as resolved.
Show resolved Hide resolved

function getAjv(oasVersion: number = 0): AJV.Ajv {
m-mohr marked this conversation as resolved.
Show resolved Hide resolved
if (typeof ajvInstances[oasVersion] !== 'undefined') {
return ajvInstances[oasVersion];
}

const ajvOpts: object = {
m-mohr marked this conversation as resolved.
Show resolved Hide resolved
meta: true, // Add default meta schemas (draft 7 at the moment)
schemaId: 'auto',
jsonPointers: true,
unknownFormats: 'ignore',
nullable: oasVersion >= 2, // Support nullable for OAS
m-mohr marked this conversation as resolved.
Show resolved Hide resolved
logger,
};
const ajv = new AJV(ajvOpts);
// We need v4 for OpenAPI and it doesn't hurt to have v6 as well.
ajv.addMetaSchema(jsonSpecv4);
ajv.addMetaSchema(jsonSpecv6);

// @ts-ignore
ajv._opts.defaultMeta = jsonSpecv4.id;
// @ts-ignore
ajv._refs['http://json-schema.org/schema'] = 'http://json-schema.org/draft-04/schema';

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 });

ajvInstances[oasVersion] = ajv;
return ajv;
}

function getSchemaId(schemaObj: JSONSchema): void | string {
if ('$id' in schemaObj) {
Expand All @@ -65,7 +80,8 @@ function getSchemaId(schemaObj: JSONSchema): void | string {
}

const validators = new (class extends WeakMap<JSONSchema, ValidateFunction> {
public get(schemaObj: JSONSchema) {
public get(schemaObj: JSONSchema, oasVersion: number = 0) {
m-mohr marked this conversation as resolved.
Show resolved Hide resolved
const ajv = getAjv(oasVersion);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@P0lip how cheap is it to recreate a new instance in that layer?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only three instances are created, one for each "schema version" (i.e. oas2, oas3 and json schema). So overhead should be minimal and better than creating one ajv instance per example validation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it should be cheap.

const schemaId = getSchemaId(schemaObj);
let validator = schemaId !== void 0 ? ajv.getSchema(schemaId) : void 0;
if (validator !== void 0) {
Expand Down Expand Up @@ -128,7 +144,7 @@ export const schema: IFunction<ISchemaOptions> = (targetVal, opts, paths) => {

try {
// we used the compiled validation now, hence this lookup here (see the logic above for more info)
const validator = validators.get(schemaObj);
const validator = validators.get(schemaObj, opts.oasVersion);
if (!validator(targetVal) && validator.errors) {
try {
results.push(
Expand Down
27 changes: 18 additions & 9 deletions src/rulesets/oas/index.json
Original file line number Diff line number Diff line change
Expand Up @@ -531,7 +531,8 @@
"function": "schemaPath",
"functionOptions": {
"field": "example",
"schemaPath": "$"
"schemaPath": "$",
"oasVersion": 2
}
}
},
Expand All @@ -547,7 +548,8 @@
"function": "schemaPath",
"functionOptions": {
"field": "example",
"schemaPath": "$"
"schemaPath": "$",
"oasVersion": 2
}
}
},
Expand Down Expand Up @@ -708,7 +710,8 @@
"function": "schemaPath",
"functionOptions": {
"field": "example",
"schemaPath": "$.schema"
"schemaPath": "$.schema",
"oasVersion": 3
}
}
},
Expand All @@ -724,7 +727,8 @@
"function": "schemaPath",
"functionOptions": {
"field": "example",
"schemaPath": "$.schema"
"schemaPath": "$.schema",
"oasVersion": 3
}
}
},
Expand All @@ -740,7 +744,8 @@
"function": "schemaPath",
"functionOptions": {
"field": "example",
"schemaPath": "$.schema"
"schemaPath": "$.schema",
"oasVersion": 3
}
}
},
Expand All @@ -756,7 +761,8 @@
"function": "schemaPath",
"functionOptions": {
"field": "example",
"schemaPath": "$"
"schemaPath": "$",
"oasVersion": 3
}
}
},
Expand All @@ -772,7 +778,8 @@
"function": "schemaPath",
"functionOptions": {
"field": "example",
"schemaPath": "$"
"schemaPath": "$",
"oasVersion": 3
}
}
},
Expand All @@ -788,7 +795,8 @@
"function": "schemaPath",
"functionOptions": {
"field": "example",
"schemaPath": "$"
"schemaPath": "$",
"oasVersion": 3
}
}
},
Expand All @@ -804,7 +812,8 @@
"function": "schemaPath",
"functionOptions": {
"field": "example",
"schemaPath": "$"
"schemaPath": "$",
"oasVersion": 3
}
}
},
Expand Down