chore: optimize schema creation

.gitignore

@@ -1,5 +1,6 @@
 **/node_modules
 **/package-lock.json
+**/pnpm-lock.yaml
 
 coverage.*
 

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

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',
         () =>

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;

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

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

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)) {

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()', () => {