From 47b53add9ff9a0c3830799013c7381146ef1baa4 Mon Sep 17 00:00:00 2001 From: Robert Sese <734194+rsese@users.noreply.github.com> Date: Tue, 28 Mar 2023 14:12:59 -0500 Subject: [PATCH] ajv lib/frontmatter (#35487) Co-authored-by: Sarah Schneider --- lib/frontmatter.js | 46 +++-- lib/read-frontmatter.js | 79 +++++---- .../schemas/feature-versions-schema.js | 21 --- .../helpers/schemas/learning-tracks-schema.js | 21 --- .../helpers/schemas/secret-scanning-schema.js | 22 --- tests/unit/read-frontmatter.js | 159 ++++++------------ 6 files changed, 128 insertions(+), 220 deletions(-) diff --git a/lib/frontmatter.js b/lib/frontmatter.js index d4b07e17c6..8d476a2508 100644 --- a/lib/frontmatter.js +++ b/lib/frontmatter.js @@ -1,5 +1,4 @@ import parse from './read-frontmatter.js' -import semver from 'semver' import { allVersions } from './all-versions.js' import { allTools } from './all-tools.js' import { getDeepDataByLanguage } from './get-data.js' @@ -16,10 +15,12 @@ const layoutNames = [ const guideTypes = ['overview', 'quick_start', 'tutorial', 'how_to', 'reference'] export const schema = { + type: 'object', + required: ['title', 'versions'], + additionalProperties: false, properties: { title: { type: 'string', - required: true, translatable: true, }, shortTitle: { @@ -69,7 +70,7 @@ export const schema = { layout: { type: ['string', 'boolean'], enum: layoutNames, - message: 'must be the filename of an existing layout file, or `false` for no layout', + errorMessage: 'must be the filename of an existing layout file, or `false` for no layout', }, redirect_from: { type: 'array', @@ -120,8 +121,12 @@ export const schema = { items: { type: 'object', properties: { - title: 'string', - href: 'string', + title: { + type: 'string', + }, + href: { + type: 'string', + }, }, }, }, @@ -170,8 +175,12 @@ export const schema = { communityRedirect: { type: 'object', properties: { - name: 'string', - href: 'string', + name: { + type: 'string', + }, + href: { + type: 'string', + }, }, }, // Platform-specific content preference @@ -196,15 +205,16 @@ export const schema = { // External products specified on the homepage externalProducts: { type: 'object', + required: ['electron'], properties: { electron: { type: 'object', - required: true, + required: ['id', 'name', 'href', 'external'], properties: { - id: { type: 'string', required: true }, - name: { type: 'string', required: true }, - href: { type: 'string', format: 'url', required: true }, - external: { type: 'boolean', required: true }, + id: { type: 'string' }, + name: { type: 'string' }, + href: { type: 'string', format: 'url' }, + external: { type: 'boolean' }, }, }, }, @@ -252,20 +262,22 @@ const featureVersionsProp = { items: { type: 'string', }, - message: + errorMessage: 'must be the name (or names) of a feature that matches "filename" in data/features/_filename_.yml', }, } const semverRange = { type: 'string', - conform: semver.validRange, - message: 'Must be a valid SemVer range', + format: 'semver', + // This is JSON pointer syntax with ajv so we can specify the bad version + // in the error message. + // eslint-disable-next-line no-template-curly-in-string + errorMessage: 'Must be a valid SemVer range: ${0}', } schema.properties.versions = { type: ['object', 'string'], // allow a '*' string to indicate all versions - required: true, additionalProperties: false, // don't allow any versions in FM that aren't defined in lib/all-versions properties: Object.values(allVersions).reduce((acc, versionObj) => { acc[versionObj.plan] = semverRange @@ -277,8 +289,6 @@ schema.properties.versions = { export function frontmatter(markdown, opts = {}) { const defaults = { schema, - validateKeyNames: true, - validateKeyOrder: false, // TODO: enable this once we've sorted all the keys. See issue 9658 } return parse(markdown, Object.assign({}, defaults, opts)) diff --git a/lib/read-frontmatter.js b/lib/read-frontmatter.js index 588660185d..712a2a347d 100644 --- a/lib/read-frontmatter.js +++ b/lib/read-frontmatter.js @@ -1,9 +1,21 @@ import matter from 'gray-matter' -import revalidator from 'revalidator' -import { difference, intersection } from 'lodash-es' +import Ajv from 'ajv' +import addErrors from 'ajv-errors' +import addFormats from 'ajv-formats' +import semver from 'semver' -function readFrontmatter(markdown, opts = { validateKeyNames: false, validateKeyOrder: false }) { - const schema = opts.schema || { properties: {} } +const ajv = new Ajv({ allErrors: true, allowUnionTypes: true }) +ajv.addKeyword({ + keyword: 'translatable', +}) +ajv.addFormat('semver', { + validate: (x) => semver.validRange(x), +}) +addErrors(ajv) +addFormats(ajv) + +function readFrontmatter(markdown, opts = {}) { + const schema = opts.schema || { type: 'object', properties: {} } const filepath = opts.filepath || null let content, data @@ -33,42 +45,41 @@ function readFrontmatter(markdown, opts = { validateKeyNames: false, validateKey return { errors } } - const allowedKeys = Object.keys(schema.properties) - const existingKeys = Object.keys(data) - const expectedKeys = intersection(allowedKeys, existingKeys) + const ajvValidate = ajv.compile(schema) + const valid = ajvValidate(data) - ;({ errors } = revalidator.validate(data, schema)) - - // add filepath property to each error object - if (errors.length && filepath) { - errors = errors.map((error) => Object.assign(error, { filepath })) + if (!valid) { + errors = ajvValidate.errors } - // validate key names - if (opts.validateKeyNames) { - const invalidKeys = difference(existingKeys, allowedKeys) - invalidKeys.forEach((key) => { - const error = { - property: key, - message: `not allowed. Allowed properties are: ${allowedKeys.join(', ')}`, - } - if (filepath) error.filepath = filepath - errors.push(error) + // Combine the AJV-supplied `instancePath` and `params` into a more user-friendly frontmatter path. + // For example, given: + // "instancePath": "/versions", + // "params": { "additionalProperty": "ftp" } + // return: + // property: 'versions.ftp' + // + // The purpose is to help users understand that the error is on the `fpt` key within the `versions` object. + // Note if the error is on a top-level FM property like `title`, the `instancePath` will be empty. + const cleanPropertyPath = (params, instancePath) => { + const mainProps = Object.values(params)[0] + if (!instancePath) return mainProps + + const prefixProps = instancePath.replace('/', '').replace(/\//g, '.') + return typeof mainProps !== 'object' ? `${prefixProps}.${mainProps}` : prefixProps + } + + if (!valid && filepath) { + errors = ajvValidate.errors.map((error) => { + const userFriendly = {} + userFriendly.property = cleanPropertyPath(error.params, error.instancePath) + userFriendly.message = error.message + userFriendly.reason = error.keyword + userFriendly.filepath = filepath + return userFriendly }) } - // validate key order - if (opts.validateKeyOrder && existingKeys.join('') !== expectedKeys.join('')) { - const error = { - property: 'keys', - message: `keys must be in order. Current: ${existingKeys.join( - ',' - )}; Expected: ${expectedKeys.join(',')}`, - } - if (filepath) error.filepath = filepath - errors.push(error) - } - return { content, data, errors } } diff --git a/tests/helpers/schemas/feature-versions-schema.js b/tests/helpers/schemas/feature-versions-schema.js index 1696c8fd29..99f5b82da0 100644 --- a/tests/helpers/schemas/feature-versions-schema.js +++ b/tests/helpers/schemas/feature-versions-schema.js @@ -20,25 +20,4 @@ featureVersions.additionalProperties = false // avoid ajv strict warning featureVersions.type = 'object' -// *** TODO: We can drop the following once the frontmatter.js schema has been updated to work with AJV. *** -const properties = {} -Object.keys(featureVersions.properties.versions.properties).forEach((key) => { - const value = Object.assign({}, featureVersions.properties.versions.properties[key]) - - // AJV supports errorMessage, not message. - value.errorMessage = value.message - delete value.message - - // AJV doesn't support conform, so we'll add semver validation in the lint-versioning test. - if (value.conform) { - value.format = 'semver' - delete value.conform - } - properties[key] = value -}) - -featureVersions.properties.versions.properties = properties -delete featureVersions.properties.versions.required -// *** End TODO *** - export default featureVersions diff --git a/tests/helpers/schemas/learning-tracks-schema.js b/tests/helpers/schemas/learning-tracks-schema.js index 8262b46bbb..4025e18ea7 100644 --- a/tests/helpers/schemas/learning-tracks-schema.js +++ b/tests/helpers/schemas/learning-tracks-schema.js @@ -4,27 +4,6 @@ import { schema } from '../../../lib/frontmatter.js' // so we can import that part of the FM schema. const versionsProps = Object.assign({}, schema.properties.versions) -// Tweak the imported versions schema so it works with AJV. -// *** TODO: We can drop the following once the frontmatter.js schema has been updated to work with AJV. *** -const properties = {} -Object.keys(versionsProps.properties).forEach((key) => { - const value = Object.assign({}, versionsProps.properties[key]) - - // AJV supports errorMessage, not message. - value.errorMessage = value.message - delete value.message - - // AJV doesn't support conform, so we'll add semver validation in the lint-files test. - if (value.conform) { - value.format = 'semver' - delete value.conform - } - properties[key] = value -}) - -versionsProps.properties = properties -// *** End TODO *** - // `versions` are not required in learning tracks the way they are in FM. delete versionsProps.required diff --git a/tests/helpers/schemas/secret-scanning-schema.js b/tests/helpers/schemas/secret-scanning-schema.js index 944ea365c9..07f69bbb30 100644 --- a/tests/helpers/schemas/secret-scanning-schema.js +++ b/tests/helpers/schemas/secret-scanning-schema.js @@ -4,28 +4,6 @@ import { schema } from '../../../lib/frontmatter.js' // so we can import that part of the FM schema. const versionsProps = Object.assign({}, schema.properties.versions) -// Tweak the imported versions schema so it works with AJV. -// *** TODO: We can drop the following once the frontmatter.js schema has been updated to work with AJV. *** -const properties = {} -Object.keys(versionsProps.properties).forEach((key) => { - const value = Object.assign({}, versionsProps.properties[key]) - - // AJV supports errorMessage, not message. - value.errorMessage = value.message - delete value.message - - // AJV doesn't support conform, so we'll add semver validation in the lint-secret-scanning-data test. - if (value.conform) { - value.format = 'semver' - delete value.conform - } - properties[key] = value -}) - -versionsProps.properties = properties -delete versionsProps.required -// *** End TODO *** - // The secret-scanning.json contains an array of objects that look like this: // { // "provider": "Azure", diff --git a/tests/unit/read-frontmatter.js b/tests/unit/read-frontmatter.js index 8adafbf654..1583cbf964 100644 --- a/tests/unit/read-frontmatter.js +++ b/tests/unit/read-frontmatter.js @@ -1,4 +1,6 @@ import parse from '../../lib/read-frontmatter.js' +import { schema as frontmatterSchema } from '../../lib/frontmatter.js' + const filepath = 'path/to/file.md' const fixture1 = `--- title: Hello, World @@ -7,6 +9,13 @@ meaning_of_life: 42 I am content. ` +const fixture2 = `--- +versions: + fpt: '*' + ghec: '*' + ghes: 'BAD_VERSION' +--- +` describe('frontmatter', () => { it('parses frontmatter and content in a given string (no options required)', () => { @@ -96,21 +105,54 @@ I am content. expect(content.trim()).toBe('I am content.') expect(errors.length).toBe(1) const expectedError = { - attribute: 'minimum', - property: 'meaning_of_life', - expected: 50, - actual: 42, - message: 'must be greater than or equal to 50', + instancePath: '/meaning_of_life', + schemaPath: '#/properties/meaning_of_life/minimum', + keyword: 'minimum', + params: { + comparison: '>=', + limit: 50, + }, + message: 'must be >= 50', } expect(errors[0]).toEqual(expectedError) }) + it('creates errors if versions frontmatter does not match semver format', () => { + const schema = { type: 'object', required: ['versions'], properties: {} } + schema.properties.versions = Object.assign({}, frontmatterSchema.properties.versions) + + const { errors } = parse(fixture2, { schema }) + const expectedError = { + instancePath: '/versions/ghes', + schemaPath: '#/properties/versions/properties/ghes/errorMessage', + keyword: 'errorMessage', + params: { + errors: [ + { + instancePath: '/versions/ghes', + schemaPath: '#/properties/versions/properties/ghes/format', + keyword: 'format', + params: { + format: 'semver', + }, + message: 'must match format "semver"', + emUsed: true, + }, + ], + }, + message: 'Must be a valid SemVer range: "BAD_VERSION"', + } + + expect(errors[0]).toEqual(expectedError) + }) + it('creates errors if required frontmatter is not present', () => { const schema = { + type: 'object', + required: ['yet_another_key'], properties: { yet_another_key: { type: 'string', - required: true, }, }, } @@ -118,106 +160,15 @@ I am content. const { errors } = parse(fixture1, { schema }) expect(errors.length).toBe(1) const expectedError = { - attribute: 'required', - property: 'yet_another_key', - expected: true, - actual: undefined, - message: 'is required', + instancePath: '', + schemaPath: '#/required', + keyword: 'required', + params: { + missingProperty: 'yet_another_key', + }, + message: "must have required property 'yet_another_key'", } expect(errors[0]).toEqual(expectedError) }) }) - - describe('validateKeyNames', () => { - const schema = { - properties: { - age: { - type: 'number', - }, - }, - } - - it('creates errors for undocumented keys if `validateKeyNames` is true', () => { - const { errors } = parse(fixture1, { schema, validateKeyNames: true, filepath }) - expect(errors.length).toBe(2) - const expectedErrors = [ - { - property: 'title', - message: 'not allowed. Allowed properties are: age', - filepath: 'path/to/file.md', - }, - { - property: 'meaning_of_life', - message: 'not allowed. Allowed properties are: age', - filepath: 'path/to/file.md', - }, - ] - expect(errors).toEqual(expectedErrors) - }) - - it('does not create errors for undocumented keys if `validateKeyNames` is false', () => { - const { errors } = parse(fixture1, { schema, validateKeyNames: false }) - expect(errors.length).toBe(0) - }) - }) - - describe('validateKeyOrder', () => { - it('creates errors if `validateKeyOrder` is true and keys are not in order', () => { - const schema = { - properties: { - meaning_of_life: { - type: 'number', - }, - title: { - type: 'string', - }, - }, - } - const { errors } = parse(fixture1, { schema, validateKeyOrder: true, filepath }) - const expectedErrors = [ - { - property: 'keys', - message: - 'keys must be in order. Current: title,meaning_of_life; Expected: meaning_of_life,title', - filepath: 'path/to/file.md', - }, - ] - expect(errors).toEqual(expectedErrors) - }) - - it('does not create errors if `validateKeyOrder` is true and keys are in order', () => { - const schema = { - properties: { - title: { - type: 'string', - }, - meaning_of_life: { - type: 'number', - }, - }, - } - const { errors } = parse(fixture1, { schema, validateKeyOrder: true }) - expect(errors.length).toBe(0) - }) - - it('does not create errors if `validateKeyOrder` is true and expected keys are in order', () => { - const schema = { - properties: { - title: { - type: 'string', - required: true, - }, - yet_another_key: { - type: 'string', - }, - meaning_of_life: { - type: 'number', - required: true, - }, - }, - } - const { errors } = parse(fixture1, { schema, validateKeyOrder: true }) - expect(errors.length).toBe(0) - }) - }) })