From 7a466e8e54b47f07826710bdb4addf9c20853108 Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Sun, 30 Jun 2024 22:40:18 -0400 Subject: [PATCH 1/4] feat: merge rule.meta.defaultOptions before validation --- lib/shared/config-validator.js | 6 +- lib/shared/deep-merge-arrays.js | 58 +++++++++++ tests/lib/shared/config-validator.js | 30 ++++++ tests/lib/shared/deep-merge-arrays.js | 138 ++++++++++++++++++++++++++ 4 files changed, 231 insertions(+), 1 deletion(-) create mode 100644 lib/shared/deep-merge-arrays.js create mode 100644 tests/lib/shared/deep-merge-arrays.js diff --git a/lib/shared/config-validator.js b/lib/shared/config-validator.js index 531ccbf..0829bf9 100644 --- a/lib/shared/config-validator.js +++ b/lib/shared/config-validator.js @@ -19,6 +19,7 @@ import util from "util"; import * as ConfigOps from "./config-ops.js"; import { emitDeprecationWarning } from "./deprecation-warnings.js"; import ajvOrig from "./ajv.js"; +import { deepMergeArrays } from "./deep-merge-arrays.js"; import configSchema from "../../conf/config-schema.js"; import BuiltInEnvironments from "../../conf/environments.js"; @@ -148,7 +149,10 @@ export default class ConfigValidator { const validateRule = ruleValidators.get(rule); if (validateRule) { - validateRule(localOptions); + const mergedOptions = deepMergeArrays(rule.meta?.defaultOptions, localOptions); + + validateRule(mergedOptions); + if (validateRule.errors) { throw new Error(validateRule.errors.map( error => `\tValue ${JSON.stringify(error.data)} ${error.message}.\n` diff --git a/lib/shared/deep-merge-arrays.js b/lib/shared/deep-merge-arrays.js new file mode 100644 index 0000000..29c245e --- /dev/null +++ b/lib/shared/deep-merge-arrays.js @@ -0,0 +1,58 @@ +/** + * @fileoverview Applies default rule options + * @author JoshuaKGoldberg + */ + +/** + * Check if the variable contains an object strictly rejecting arrays + * @param {unknown} value an object + * @returns {boolean} Whether value is an object + */ +function isObjectNotArray(value) { + return typeof value === "object" && value !== null && value !== void 0 && !Array.isArray(value); +} + +/** + * Deeply merges second on top of first, creating a new {} object if needed. + * @param {T} first Base, default value. + * @param {U} second User-specified value. + * @returns {T | U | (T & U)} Merged equivalent of second on top of first. + */ +function deepMergeObjects(first, second) { + if (second === void 0) { + return first; + } + + if (!isObjectNotArray(first) || !isObjectNotArray(second)) { + return second; + } + + const result = { ...first, ...second }; + + for (const key of Object.keys(second)) { + if (Object.prototype.propertyIsEnumerable.call(first, key)) { + result[key] = deepMergeObjects(first[key], second[key]); + } + } + + return result; +} + +/** + * Deeply merges second on top of first, creating a new [] array if needed. + * @param {T[]} first Base, default values. + * @param {U[]} second User-specified values. + * @returns {(T | U | (T & U))[]} Merged equivalent of second on top of first. + */ +function deepMergeArrays(first, second) { + if (!first || !second) { + return second || first || []; + } + + return [ + ...first.map((value, i) => deepMergeObjects(value, i < second.length ? second[i] : void 0)), + ...second.slice(first.length) + ]; +} + +export { deepMergeArrays }; diff --git a/tests/lib/shared/config-validator.js b/tests/lib/shared/config-validator.js index abed983..fbd92d9 100644 --- a/tests/lib/shared/config-validator.js +++ b/tests/lib/shared/config-validator.js @@ -73,6 +73,21 @@ const mockInvalidJSONSchemaRule = { } }; +const mockMaxPropertiesSchema = { + meta: { + defaultOptions: [{ + foo: 42 + }], + schema: [{ + type: "object", + maxProperties: 2 + }] + }, + create() { + return {}; + } +}; + //------------------------------------------------------------------------------ // Tests //------------------------------------------------------------------------------ @@ -253,4 +268,19 @@ describe("ConfigValidator", () => { }); }); + + describe("validateRuleSchema", () => { + + it("should throw when rule options are invalid after defaults are applied", () => { + const fn = validator.validateRuleSchema.bind(validator, mockMaxPropertiesSchema, [{ bar: 6, baz: 7 }]); + + nodeAssert.throws( + fn, + { + message: '\tValue {"foo":42,"bar":6,"baz":7} should NOT have more than 2 properties.\n' + } + ); + }); + + }); }); diff --git a/tests/lib/shared/deep-merge-arrays.js b/tests/lib/shared/deep-merge-arrays.js new file mode 100644 index 0000000..ad3a3b2 --- /dev/null +++ b/tests/lib/shared/deep-merge-arrays.js @@ -0,0 +1,138 @@ +/* eslint-disable no-undefined -- `null` and `undefined` are different in options */ + +//------------------------------------------------------------------------------ +// Requirements +//------------------------------------------------------------------------------ + +import assert from "node:assert"; + +import { deepMergeArrays } from "../../../lib/shared/deep-merge-arrays.js"; + +//------------------------------------------------------------------------------ +// Tests +//------------------------------------------------------------------------------ + +/** + * Turns a value into its string equivalent for a test name. + * @param {unknown} value Value to be stringified. + * @returns {string} String equivalent of the value. + */ +function toTestCaseName(value) { + return typeof value === "object" ? JSON.stringify(value) : `${value}`; +} + +describe("deepMerge", () => { + for (const [first, second, result] of [ + [undefined, undefined, []], + [[], undefined, []], + [["abc"], undefined, ["abc"]], + [undefined, ["abc"], ["abc"]], + [[], ["abc"], ["abc"]], + [[undefined], ["abc"], ["abc"]], + [[undefined, undefined], ["abc"], ["abc", undefined]], + [[undefined, undefined], ["abc", "def"], ["abc", "def"]], + [[undefined, null], ["abc"], ["abc", null]], + [[undefined, null], ["abc", "def"], ["abc", "def"]], + [[null], ["abc"], ["abc"]], + [[123], [undefined], [123]], + [[123], [null], [null]], + [[123], [{ a: 0 }], [{ a: 0 }]], + [["abc"], [undefined], ["abc"]], + [["abc"], [null], [null]], + [["abc"], ["def"], ["def"]], + [["abc"], [{ a: 0 }], [{ a: 0 }]], + [[["abc"]], [null], [null]], + [[["abc"]], ["def"], ["def"]], + [[["abc"]], [{ a: 0 }], [{ a: 0 }]], + [[{ abc: true }], ["def"], ["def"]], + [[{ abc: true }], [["def"]], [["def"]]], + [[null], [{ abc: true }], [{ abc: true }]], + [[{ a: undefined }], [{ a: 0 }], [{ a: 0 }]], + [[{ a: null }], [{ a: 0 }], [{ a: 0 }]], + [[{ a: null }], [{ a: { b: 0 } }], [{ a: { b: 0 } }]], + [[{ a: 0 }], [{ a: 1 }], [{ a: 1 }]], + [[{ a: 0 }], [{ a: null }], [{ a: null }]], + [[{ a: 0 }], [{ a: undefined }], [{ a: 0 }]], + [[{ a: 0 }], ["abc"], ["abc"]], + [[{ a: 0 }], [123], [123]], + [[[{ a: 0 }]], [123], [123]], + [ + [{ a: ["b"] }], + [{ a: ["c"] }], + [{ a: ["c"] }] + ], + [ + [{ a: [{ b: "c" }] }], + [{ a: [{ d: "e" }] }], + [{ a: [{ d: "e" }] }] + ], + [ + [{ a: { b: "c" }, d: true }], + [{ a: { e: "f" } }], + [{ a: { b: "c", e: "f" }, d: true }] + ], + [ + [{ a: { b: "c" } }], + [{ a: { e: "f" }, d: true }], + [{ a: { b: "c", e: "f" }, d: true }] + ], + [ + [{ a: { b: "c" } }, { d: true }], + [{ a: { e: "f" } }, { f: 123 }], + [{ a: { b: "c", e: "f" } }, { d: true, f: 123 }] + ], + [ + [{ hasOwnProperty: true }], + [{}], + [{ hasOwnProperty: true }] + ], + [ + [{ hasOwnProperty: false }], + [{}], + [{ hasOwnProperty: false }] + ], + [ + [{ hasOwnProperty: null }], + [{}], + [{ hasOwnProperty: null }] + ], + [ + [{ hasOwnProperty: undefined }], + [{}], + [{ hasOwnProperty: undefined }] + ], + [ + [{}], + [{ hasOwnProperty: null }], + [{ hasOwnProperty: null }] + ], + [ + [{}], + [{ hasOwnProperty: undefined }], + [{ hasOwnProperty: undefined }] + ], + [ + [{ + allow: [], + ignoreDestructuring: false, + ignoreGlobals: false, + ignoreImports: false, + properties: "always" + }], + [], + [{ + allow: [], + ignoreDestructuring: false, + ignoreGlobals: false, + ignoreImports: false, + properties: "always" + }] + ] + ]) { + it(`${toTestCaseName(first)}, ${toTestCaseName(second)}`, () => { + assert.deepStrictEqual(deepMergeArrays(first, second), result); + }); + } +}); + +/* eslint-enable no-undefined -- `null` and `undefined` are different in options */ From d76f47e6c7af72ae4353cd00814a09baea7cdde1 Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Wed, 3 Jul 2024 11:11:29 -0400 Subject: [PATCH 2/4] Switch from ?. to && --- lib/shared/config-validator.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/shared/config-validator.js b/lib/shared/config-validator.js index 0829bf9..bce7674 100644 --- a/lib/shared/config-validator.js +++ b/lib/shared/config-validator.js @@ -149,7 +149,7 @@ export default class ConfigValidator { const validateRule = ruleValidators.get(rule); if (validateRule) { - const mergedOptions = deepMergeArrays(rule.meta?.defaultOptions, localOptions); + const mergedOptions = deepMergeArrays(rule.meta && rule.meta.defaultOptions, localOptions); validateRule(mergedOptions); From c9a60fd941c1c067c6b5be81b5c6f410a22fa5ab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Josh=20Goldberg=20=E2=9C=A8?= Date: Fri, 26 Jul 2024 07:46:01 -0400 Subject: [PATCH 3/4] Apply suggestions from code review Co-authored-by: Francesco Trotta --- lib/shared/config-validator.js | 2 +- lib/shared/deep-merge-arrays.js | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/shared/config-validator.js b/lib/shared/config-validator.js index bce7674..0829bf9 100644 --- a/lib/shared/config-validator.js +++ b/lib/shared/config-validator.js @@ -149,7 +149,7 @@ export default class ConfigValidator { const validateRule = ruleValidators.get(rule); if (validateRule) { - const mergedOptions = deepMergeArrays(rule.meta && rule.meta.defaultOptions, localOptions); + const mergedOptions = deepMergeArrays(rule.meta?.defaultOptions, localOptions); validateRule(mergedOptions); diff --git a/lib/shared/deep-merge-arrays.js b/lib/shared/deep-merge-arrays.js index 29c245e..0c7ec34 100644 --- a/lib/shared/deep-merge-arrays.js +++ b/lib/shared/deep-merge-arrays.js @@ -9,7 +9,7 @@ * @returns {boolean} Whether value is an object */ function isObjectNotArray(value) { - return typeof value === "object" && value !== null && value !== void 0 && !Array.isArray(value); + return typeof value === "object" && value !== null && !Array.isArray(value); } /** @@ -40,8 +40,8 @@ function deepMergeObjects(first, second) { /** * Deeply merges second on top of first, creating a new [] array if needed. - * @param {T[]} first Base, default values. - * @param {U[]} second User-specified values. + * @param {T[] | undefined} first Base, default values. + * @param {U[] | undefined} second User-specified values. * @returns {(T | U | (T & U))[]} Merged equivalent of second on top of first. */ function deepMergeArrays(first, second) { @@ -50,7 +50,7 @@ function deepMergeArrays(first, second) { } return [ - ...first.map((value, i) => deepMergeObjects(value, i < second.length ? second[i] : void 0)), + ...first.map((value, i) => deepMergeObjects(value, second[i])), ...second.slice(first.length) ]; } From c6c136a4074cb37b15ac9eefb411f6ccd383ff41 Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Fri, 26 Jul 2024 11:41:40 -0400 Subject: [PATCH 4/4] switch from undefined to void 0 in deep-merge-arrays --- tests/lib/shared/deep-merge-arrays.js | 38 ++++++++++++--------------- 1 file changed, 17 insertions(+), 21 deletions(-) diff --git a/tests/lib/shared/deep-merge-arrays.js b/tests/lib/shared/deep-merge-arrays.js index ad3a3b2..55cc272 100644 --- a/tests/lib/shared/deep-merge-arrays.js +++ b/tests/lib/shared/deep-merge-arrays.js @@ -1,5 +1,3 @@ -/* eslint-disable no-undefined -- `null` and `undefined` are different in options */ - //------------------------------------------------------------------------------ // Requirements //------------------------------------------------------------------------------ @@ -23,21 +21,21 @@ function toTestCaseName(value) { describe("deepMerge", () => { for (const [first, second, result] of [ - [undefined, undefined, []], - [[], undefined, []], - [["abc"], undefined, ["abc"]], - [undefined, ["abc"], ["abc"]], + [void 0, void 0, []], + [[], void 0, []], + [["abc"], void 0, ["abc"]], + [void 0, ["abc"], ["abc"]], [[], ["abc"], ["abc"]], - [[undefined], ["abc"], ["abc"]], - [[undefined, undefined], ["abc"], ["abc", undefined]], - [[undefined, undefined], ["abc", "def"], ["abc", "def"]], - [[undefined, null], ["abc"], ["abc", null]], - [[undefined, null], ["abc", "def"], ["abc", "def"]], + [[void 0], ["abc"], ["abc"]], + [[void 0, void 0], ["abc"], ["abc", void 0]], + [[void 0, void 0], ["abc", "def"], ["abc", "def"]], + [[void 0, null], ["abc"], ["abc", null]], + [[void 0, null], ["abc", "def"], ["abc", "def"]], [[null], ["abc"], ["abc"]], - [[123], [undefined], [123]], + [[123], [void 0], [123]], [[123], [null], [null]], [[123], [{ a: 0 }], [{ a: 0 }]], - [["abc"], [undefined], ["abc"]], + [["abc"], [void 0], ["abc"]], [["abc"], [null], [null]], [["abc"], ["def"], ["def"]], [["abc"], [{ a: 0 }], [{ a: 0 }]], @@ -47,12 +45,12 @@ describe("deepMerge", () => { [[{ abc: true }], ["def"], ["def"]], [[{ abc: true }], [["def"]], [["def"]]], [[null], [{ abc: true }], [{ abc: true }]], - [[{ a: undefined }], [{ a: 0 }], [{ a: 0 }]], + [[{ a: void 0 }], [{ a: 0 }], [{ a: 0 }]], [[{ a: null }], [{ a: 0 }], [{ a: 0 }]], [[{ a: null }], [{ a: { b: 0 } }], [{ a: { b: 0 } }]], [[{ a: 0 }], [{ a: 1 }], [{ a: 1 }]], [[{ a: 0 }], [{ a: null }], [{ a: null }]], - [[{ a: 0 }], [{ a: undefined }], [{ a: 0 }]], + [[{ a: 0 }], [{ a: void 0 }], [{ a: 0 }]], [[{ a: 0 }], ["abc"], ["abc"]], [[{ a: 0 }], [123], [123]], [[[{ a: 0 }]], [123], [123]], @@ -97,9 +95,9 @@ describe("deepMerge", () => { [{ hasOwnProperty: null }] ], [ - [{ hasOwnProperty: undefined }], + [{ hasOwnProperty: void 0 }], [{}], - [{ hasOwnProperty: undefined }] + [{ hasOwnProperty: void 0 }] ], [ [{}], @@ -108,8 +106,8 @@ describe("deepMerge", () => { ], [ [{}], - [{ hasOwnProperty: undefined }], - [{ hasOwnProperty: undefined }] + [{ hasOwnProperty: void 0 }], + [{ hasOwnProperty: void 0 }] ], [ [{ @@ -134,5 +132,3 @@ describe("deepMerge", () => { }); } }); - -/* eslint-enable no-undefined -- `null` and `undefined` are different in options */