From d7f29e7363ab7439a75897f21e2b6f1fdd98ac2f Mon Sep 17 00:00:00 2001 From: Julien Fontanet Date: Thu, 11 Aug 2022 15:50:04 +0200 Subject: [PATCH] chore(xo-server/api): use Ajv instead of schema-inspector - standard JSON schema - faster - maintained New implementation also pre-compile schemas which means that params validation for each call is faster and incorrect schemas are detected at startup. --- packages/xo-server/package.json | 1 - packages/xo-server/src/api/backup-ng.mjs | 6 +- packages/xo-server/src/api/disk.mjs | 2 +- packages/xo-server/src/api/host.mjs | 2 +- packages/xo-server/src/api/network.mjs | 6 +- packages/xo-server/src/api/vm.mjs | 11 +- packages/xo-server/src/api/xosan.mjs | 5 +- packages/xo-server/src/xo-mixins/api.mjs | 147 ++++++++++++++++++----- yarn.lock | 9 +- 9 files changed, 130 insertions(+), 59 deletions(-) diff --git a/packages/xo-server/package.json b/packages/xo-server/package.json index 24e322956..40a7be805 100644 --- a/packages/xo-server/package.json +++ b/packages/xo-server/package.json @@ -113,7 +113,6 @@ "pw": "^0.0.4", "readable-stream": "^4.1.0", "redis": "^4.2.0", - "schema-inspector": "^2.0.1", "semver": "^7.3.2", "serve-static": "^1.13.1", "set-cookie-parser": "^2.3.5", diff --git a/packages/xo-server/src/api/backup-ng.mjs b/packages/xo-server/src/api/backup-ng.mjs index 0176d3ba0..a3fc14cd6 100644 --- a/packages/xo-server/src/api/backup-ng.mjs +++ b/packages/xo-server/src/api/backup-ng.mjs @@ -14,7 +14,7 @@ const SCHEMA_SETTINGS = { properties: { concurrency: { type: 'number', - gt: 0, + minimum: 0, optional: true, }, }, @@ -210,7 +210,7 @@ getLogs.params = { after: { type: ['number', 'string'], optional: true }, before: { type: ['number', 'string'], optional: true }, limit: { type: 'number', optional: true }, - '*': { type: 'any' }, + '*': {}, } // ----------------------------------------------------------------------------- @@ -364,7 +364,7 @@ fetchFiles.params = { }, paths: { items: { type: 'string' }, - minLength: 1, + minItems: 1, type: 'array', }, remote: { diff --git a/packages/xo-server/src/api/disk.mjs b/packages/xo-server/src/api/disk.mjs index e0d9bf091..08e6f3421 100644 --- a/packages/xo-server/src/api/disk.mjs +++ b/packages/xo-server/src/api/disk.mjs @@ -123,7 +123,7 @@ export async function exportContent({ vdi, format = VHD }) { exportContent.description = 'export the content of a VDI' exportContent.params = { id: { type: 'string' }, - format: { eq: [VMDK, VHD], optional: true }, + format: { enum: [VMDK, VHD], optional: true }, } exportContent.resolve = { vdi: ['id', ['VDI', 'VDI-snapshot'], 'view'], diff --git a/packages/xo-server/src/api/host.mjs b/packages/xo-server/src/api/host.mjs index fac8b562e..794b23fab 100644 --- a/packages/xo-server/src/api/host.mjs +++ b/packages/xo-server/src/api/host.mjs @@ -171,7 +171,7 @@ restartAgent.resolve = { } // TODO: remove deprecated alias -export { restartAgent as restart_agent } // eslint-disable-line camelcase +export const restart_agent = 'restartAgent' // eslint-disable-line camelcase // ------------------------------------------------------------------- diff --git a/packages/xo-server/src/api/network.mjs b/packages/xo-server/src/api/network.mjs index 23cdc1f13..0d5dbafb4 100644 --- a/packages/xo-server/src/api/network.mjs +++ b/packages/xo-server/src/api/network.mjs @@ -52,11 +52,7 @@ createBonded.params = { }, }, mtu: { type: ['integer', 'string'], optional: true }, - // RegExp since schema-inspector does not provide a param check based on an enumeration - bondMode: { - type: 'string', - pattern: new RegExp(`^(${getBondModes().join('|')})$`), - }, + bondMode: { enum: getBondModes() }, } createBonded.resolve = { diff --git a/packages/xo-server/src/api/vm.mjs b/packages/xo-server/src/api/vm.mjs index 8ccdc2c4b..b04c84d44 100644 --- a/packages/xo-server/src/api/vm.mjs +++ b/packages/xo-server/src/api/vm.mjs @@ -303,9 +303,7 @@ create.params = { existingDisks: { optional: true, type: 'object', - - // Do not for a type object. - items: { + additionalProperties: { type: 'object', properties: { size: { @@ -325,7 +323,7 @@ create.params = { copyHostBiosStrings: { type: 'boolean', optional: true }, // other params are passed to `editVm` - '*': { type: 'any' }, + '*': {}, } create.resolve = { @@ -594,8 +592,7 @@ set.params = { high_availability: { optional: true, - pattern: new RegExp(`^(${getHaValues().join('|')})$`), - type: 'string', + enum: getHaValues(), }, // Number of virtual CPUs to allocate. @@ -803,7 +800,7 @@ convertToTemplate.resolve = { } // TODO: remove when no longer used. -export { convertToTemplate as convert } +export const convert = 'convertToTemplate' // ------------------------------------------------------------------- diff --git a/packages/xo-server/src/api/xosan.mjs b/packages/xo-server/src/api/xosan.mjs index 8402ecca6..b12f45bd6 100644 --- a/packages/xo-server/src/api/xosan.mjs +++ b/packages/xo-server/src/api/xosan.mjs @@ -201,8 +201,7 @@ getVolumeInfo.params = { type: 'string', }, infoType: { - type: 'string', - eq: Object.keys(VOLUME_INFO_TYPES), + enum: Object.keys(VOLUME_INFO_TYPES), }, } getVolumeInfo.resolve = { @@ -230,7 +229,7 @@ profileStatus.params = { type: 'string', }, changeStatus: { - type: 'bool', + type: 'boolean', optional: true, }, } diff --git a/packages/xo-server/src/xo-mixins/api.mjs b/packages/xo-server/src/xo-mixins/api.mjs index 83146e6de..127f08ef4 100644 --- a/packages/xo-server/src/xo-mixins/api.mjs +++ b/packages/xo-server/src/xo-mixins/api.mjs @@ -1,10 +1,10 @@ import emitAsync from '@xen-orchestra/emit-async' import { createLogger } from '@xen-orchestra/log' +import Ajv from 'ajv' import forEach from 'lodash/forEach.js' import kindOf from 'kindof' import ms from 'ms' -import schemaInspector from 'schema-inspector' import { AsyncLocalStorage } from 'async_hooks' import { format, JsonRpcError, MethodNotFound } from 'json-rpc-peer' @@ -55,23 +55,14 @@ const XAPI_ERROR_TO_XO_ERROR = { const hasPermission = (actual, expected) => PERMISSIONS[actual] >= PERMISSIONS[expected] +const ajv = new Ajv({ allErrors: true, allowUnionTypes: true }) + function checkParams(method, params) { - const schema = method.params - if (!schema) { - return - } - - const result = schemaInspector.validate( - { - properties: schema, - strict: true, - type: 'object', - }, - params - ) - - if (!result.valid) { - throw errors.invalidParameters(result.error) + const { validate } = method + if (validate !== undefined) { + if (!validate(params)) { + throw errors.invalidParameters(validate.errors) + } } } @@ -97,6 +88,73 @@ function checkPermission(method) { } } +function adaptJsonSchema(schema) { + if (schema.enum !== undefined) { + return schema + } + + const is = (({ type }) => { + if (typeof type === 'string') { + return t => t === type + } + const types = new Set(type) + return t => types.has(t) + })(schema) + + if (is('array')) { + const { items } = schema + if (items !== undefined) { + if (Array.isArray(items)) { + for (let i = 0, n = items.length; i < n; ++i) { + items[i] = adaptJsonSchema(items[i]) + } + } else { + schema.items = adaptJsonSchema(items) + } + } + } + + if (is('object')) { + const { properties = {} } = schema + let keys = Object.keys(properties) + + for (const key of keys) { + properties[key] = adaptJsonSchema(properties[key]) + } + + const { additionalProperties } = schema + if (schema === undefined) { + const wildCard = properties['*'] + if (wildCard === undefined) { + // we want additional properties to be disabled by default + schema.additionalProperties = false + } else { + delete properties['*'] + keys = Object.keys(properties) + schema.additionalProperties = wildCard + } + } else if (typeof additionalProperties === 'object') { + schema.additionalProperties = adaptJsonSchema(additionalProperties) + } + + // we want properties to be required by default unless explicitly marked so + // we use property `optional` instead of object `required` + if (schema.required === undefined) { + const required = keys.filter(key => { + const value = properties[key] + const required = !value.optional + delete value.optional + return required + }) + if (required.length !== 0) { + schema.required = required + } + } + } + + return schema +} + async function resolveParams(method, params) { const resolve = method.resolve if (!resolve) { @@ -177,13 +235,40 @@ export default class Api { throw new Error(`API method ${name} already exists`) } - Object.keys(method).forEach(prop => { - if (!(prop in ALLOWED_METHOD_PROPS)) { - throw new Error(`invalid prop ${prop} for API method ${name}`) - } - }) + // alias + if (typeof method === 'string') { + Object.defineProperty(methods, name, { + configurable: true, + enumerable: true, + get() { + return methods[method] + }, + }) + } else { + Object.keys(method).forEach(prop => { + if (!(prop in ALLOWED_METHOD_PROPS)) { + throw new Error(`invalid prop ${prop} for API method ${name}`) + } + }) - methods[name] = method + const { params } = method + if (params !== undefined) { + let schema = { type: 'object', properties: params } + try { + schema = adaptJsonSchema(schema) + method.validate = ajv.compile(schema) + } catch (error) { + log.warn('failed to compile method params schema', { + error, + method: name, + schema, + }) + throw error + } + } + + methods[name] = method + } let remove = () => { delete methods[name] @@ -199,15 +284,17 @@ export default class Api { const addMethod = (method, name) => { name = base + name - if (typeof method === 'function') { + const type = typeof method + if (type === 'string') { + removes.push(this.addApiMethod(name, base + method)) + } else if (type === 'function') { removes.push(this.addApiMethod(name, method)) - return + } else { + const oldBase = base + base = name + '.' + forEach(method, addMethod) + base = oldBase } - - const oldBase = base - base = name + '.' - forEach(method, addMethod) - base = oldBase } try { diff --git a/yarn.lock b/yarn.lock index f9cd6a232..89d0855c0 100644 --- a/yarn.lock +++ b/yarn.lock @@ -4095,7 +4095,7 @@ async-settle@^1.0.0: dependencies: async-done "^1.2.2" -async@^2.6.2, async@~2.6.3: +async@^2.6.2: version "2.6.4" resolved "https://registry.npmjs.org/async/-/async-2.6.4.tgz" integrity sha512-mzo5dfJYwAn29PeiJ0zvwTo04zj8HDJj0Mn8TD7sno7q12prdbnasKJHhkm2c1LgrhlJ0teaea8860oxi51mGA== @@ -16392,13 +16392,6 @@ scheduler@^0.20.2: loose-envify "^1.1.0" object-assign "^4.1.1" -schema-inspector@^2.0.1: - version "2.0.1" - resolved "https://registry.npmjs.org/schema-inspector/-/schema-inspector-2.0.1.tgz" - integrity sha512-lqR4tOVfoqf9Z8cgX/zvXuWPnTWCqrc4WSgeSPDDc1bWbMABaqdSTY98xj7iRKHOIRtKjc4M8EWCgUu5ASlHkg== - dependencies: - async "~2.6.3" - schema-utils@^1.0.0: version "1.0.0" resolved "https://registry.npmjs.org/schema-utils/-/schema-utils-1.0.0.tgz"