diff --git a/.gitignore b/.gitignore index c9e47e49..a913759e 100644 --- a/.gitignore +++ b/.gitignore @@ -1,5 +1,6 @@ **/node_modules **/package-lock.json +**/pnpm-lock.yaml coverage.* diff --git a/benchmarks/bench.js b/benchmarks/bench.js index aa625c68..4598132d 100755 --- a/benchmarks/bench.js +++ b/benchmarks/bench.js @@ -61,23 +61,28 @@ const versionPick = (o) => { return o; } - for (const k of Object.keys(o)) { - if (Joi.version.startsWith(k)) { - return o[k]; - } - } - - throw new Error(`Unsupported version ${Joi.version}`); + const major = parseInt(Joi.version, 10); + const match = Object.keys(o).map(Number).sort((a, b) => b - a).find((version) => version <= major); + assert(match !== undefined, `Unsupported version ${Joi.version}`); + return o[match]; }; const test = ([name, initFn, testFn]) => { - const [schema, valid, invalid] = versionPick(initFn)(); + initFn = versionPick(initFn); + testFn = versionPick(testFn); + + if (!initFn || + !testFn) { + + return; + } + + const [schema, valid, invalid] = initFn(); assert(valid === undefined || !testFn(schema, valid).error, 'validation must not fail for: ' + name); assert(invalid === undefined || testFn(schema, invalid).error, 'validation must fail for: ' + name); - testFn = versionPick(testFn); Suite.add(name + (valid !== undefined ? ' (valid)' : ''), () => { testFn(schema, valid); diff --git a/benchmarks/suite.js b/benchmarks/suite.js index 0f9d73da..ce028277 100755 --- a/benchmarks/suite.js +++ b/benchmarks/suite.js @@ -164,6 +164,92 @@ module.exports = (Joi) => [ }, (schema, value) => schema.validate(value()) ], + [ + 'Schema creation with many keys', + () => { + + const keys = {}; + for (let i = 0; i < 200; ++i) { + keys[`key${i}`] = Joi.string().min(1).max(10); + } + + return [keys]; + }, + (keys) => Joi.object(keys) + ], + [ + 'Schema creation with many keys and sibling references', + () => { + + const keys = {}; + for (let i = 0; i < 200; ++i) { + keys[`key${i}`] = i && i % 10 === 0 ? Joi.number().min(Joi.ref(`key${i - 1}`)) : Joi.number(); + } + + return [keys]; + }, + (keys) => Joi.object(keys) + ], + [ + 'Incremental schema creation', + () => { + + const parts = []; + for (let i = 0; i < 50; ++i) { + parts.push({ [`key${i}`]: Joi.string().min(1).max(10) }); + } + + return [parts]; + }, + (parts) => { + + let schema = Joi.object(); + for (const part of parts) { + schema = schema.keys(part); + } + + return schema; + } + ], + [ + 'Schema creation by rule chaining', + () => [], + () => Joi.string().min(1).max(100).lowercase().trim().required().description('a string') + ], + [ + 'Schema concatenation', + () => { + + const build = (prefix) => { + + const keys = {}; + for (let i = 0; i < 25; ++i) { + keys[`${prefix}${i}`] = Joi.string().min(1).max(10); + } + + return Joi.object(keys); + }; + + return [[build('left'), build('right')]]; + }, + ([left, right]) => left.concat(right) + ], + [ + 'Schema fork', + { + 15: false, // fork() was added in 16 + 16: () => { + + const keys = {}; + for (let i = 0; i < 50; ++i) { + keys[`key${i}`] = Joi.string().min(1).max(10); + } + + return [Joi.object(keys)]; + } + }, + (schema) => schema.fork('key25', (key) => key.required()) + ], [ 'Complex object', () => diff --git a/lib/base.js b/lib/base.js index db85b737..be47d955 100644 --- a/lib/base.js +++ b/lib/base.js @@ -781,9 +781,10 @@ internals.Base = class { const obj = this.clone(); if (args) { - assert(Object.keys(args).length === 1 || Object.keys(args).length === this._definition.rules[rule.name].args.length, 'Invalid rule definition for', this.type, rule.name); + const argKeys = Object.keys(args); + assert(argKeys.length === 1 || argKeys.length === this._definition.rules[rule.name].args.length, 'Invalid rule definition for', this.type, rule.name); - for (const key of Object.keys(args)) { + for (const key of argKeys) { let arg = args[key]; if (definition.argsByName) { @@ -987,7 +988,7 @@ internals.Base = class { target._valids = this._valids && this._valids.clone(); target._invalids = this._invalids && this._invalids.clone(); target._rules = this._rules.slice(); - target._singleRules = clone(this._singleRules, { shallow: true }); + target._singleRules = new Map(this._singleRules); target._refs = this._refs.clone(); target._flags = Object.assign({}, this._flags); target._cache = null; diff --git a/lib/modify.js b/lib/modify.js index 0aad9449..a2842cfe 100755 --- a/lib/modify.js +++ b/lib/modify.js @@ -22,8 +22,17 @@ exports.Ids = internals.Ids = class { clone() { const clone = new internals.Ids(); - clone._byId = new Map(this._byId); - clone._byKey = new Map(this._byKey); + + // Most schemas have no ids nor keys, so we keep the empty maps the constructor already made + + if (this._byId.size) { + clone._byId = new Map(this._byId); + } + + if (this._byKey.size) { + clone._byKey = new Map(this._byKey); + } + clone._schemaChain = this._schemaChain; return clone; } diff --git a/lib/ref.js b/lib/ref.js index b834e871..5d614dde 100755 --- a/lib/ref.js +++ b/lib/ref.js @@ -1,6 +1,6 @@ 'use strict'; -const { assert, clone, reach } = require('@hapi/hoek'); +const { assert, reach } = require('@hapi/hoek'); const Common = require('./common'); @@ -396,7 +396,7 @@ exports.Manager = class { clone() { const copy = new exports.Manager(); - copy.refs = clone(this.refs); + copy.refs = this.refs.slice(); // Entries are never mutated once registered, so a shallow copy is enough return copy; } diff --git a/lib/types/keys.js b/lib/types/keys.js index 47e69201..7d83e53d 100755 --- a/lib/types/keys.js +++ b/lib/types/keys.js @@ -600,12 +600,7 @@ module.exports = Any.extend({ rebuild(schema) { if (schema.$_terms.keys) { - const topo = new Topo.Sorter(); - for (const child of schema.$_terms.keys) { - Common.tryWithPath(() => topo.add(child, { after: child.schema.$_rootReferences(), group: child.key }), child.key); - } - - schema.$_terms.keys = new internals.Keys(...topo.nodes); + schema.$_terms.keys = new internals.Keys(...internals.sortKeys(schema.$_terms.keys)); } }, @@ -902,6 +897,25 @@ internals.dependencies = { }; +internals.sortKeys = function (keys, manual = true) { + + const topo = new Topo.Sorter(); + for (const child of keys) { + Common.tryWithPath(() => topo.add(child, { after: child.schema.$_rootReferences(), group: child.key, manual }), child.key); + } + + try { + return topo.sort(); + } + catch { + + // Sorting once at the end doesn't tell which key created the cycle, so replay the adds sorting each time to get the detailed error + + return internals.sortKeys(keys, false); + } +}; + + internals.keysToLabels = function (schema, keys) { if (Array.isArray(keys)) { diff --git a/test/types/object.js b/test/types/object.js index d9dccbfc..a2ce46c4 100755 --- a/test/types/object.js +++ b/test/types/object.js @@ -1638,6 +1638,31 @@ describe('object', () => { [{ type: 'a', set: true, flag: true }, false, '"flag" must be [false]'] ]); }); + + it('errors on circular references between sibling keys', () => { + + const err = expect(() => { + + Joi.object({ + a: Joi.ref('b'), + b: Joi.ref('a') + }); + }).to.throw('item added into group b created a dependencies error'); + + expect(err.path).to.equal('b'); + }); + + it('errors on circular references introduced by rebuild', () => { + + const schema = Joi.object({ + a: Joi.any(), + b: Joi.any() + }) + .fork('a', (s) => s.default(Joi.ref('b'))); + + const err = expect(() => schema.fork('b', (s) => s.default(Joi.ref('a')))).to.throw('item added into group a created a dependencies error'); + expect(err.path).to.equal('a'); + }); }); describe('length()', () => {