From a6c240de244b0e94ace4a518f2c67876a91f5882 Mon Sep 17 00:00:00 2001 From: Milos Djermanovic Date: Tue, 26 Dec 2023 22:35:15 +0100 Subject: [PATCH] feat!: Set default `schema: []`, drop support for function-style rules (#139) * drop support for function-style rules * implement schema changes * add unit tests for ConfigValidator#getRuleOptionsSchema * add regression tests for ConfigValidator#validateRuleOptions * add tests for error code * update fixture rules to use object-style --- lib/config-array/config-array.js | 19 +- lib/shared/config-validator.js | 81 ++++-- tests/fixtures/rules/custom-rule.cjs | 24 +- tests/fixtures/rules/dir1/no-strings.cjs | 17 +- tests/fixtures/rules/dir2/no-literals.cjs | 15 +- .../fixtures/rules/make-syntax-error-rule.cjs | 30 +- tests/fixtures/rules/wrong/custom-rule.cjs | 8 +- tests/lib/shared/config-validator.js | 256 ++++++++++++++++++ 8 files changed, 374 insertions(+), 76 deletions(-) create mode 100644 tests/lib/shared/config-validator.js diff --git a/lib/config-array/config-array.js b/lib/config-array/config-array.js index 133f5a24..5766fc46 100644 --- a/lib/config-array/config-array.js +++ b/lib/config-array/config-array.js @@ -319,31 +319,18 @@ function createConfig(instance, indices) { * @param {string} pluginId The plugin ID for prefix. * @param {Record} defs The definitions to collect. * @param {Map} map The map to output. - * @param {function(T): U} [normalize] The normalize function for each value. * @returns {void} */ -function collect(pluginId, defs, map, normalize) { +function collect(pluginId, defs, map) { if (defs) { const prefix = pluginId && `${pluginId}/`; for (const [key, value] of Object.entries(defs)) { - map.set( - `${prefix}${key}`, - normalize ? normalize(value) : value - ); + map.set(`${prefix}${key}`, value); } } } -/** - * Normalize a rule definition. - * @param {Function|Rule} rule The rule definition to normalize. - * @returns {Rule} The normalized rule definition. - */ -function normalizePluginRule(rule) { - return typeof rule === "function" ? { create: rule } : rule; -} - /** * Delete the mutation methods from a given map. * @param {Map} map The map object to delete. @@ -385,7 +372,7 @@ function initPluginMemberMaps(elements, slots) { collect(pluginId, plugin.environments, slots.envMap); collect(pluginId, plugin.processors, slots.processorMap); - collect(pluginId, plugin.rules, slots.ruleMap, normalizePluginRule); + collect(pluginId, plugin.rules, slots.ruleMap); } } diff --git a/lib/shared/config-validator.js b/lib/shared/config-validator.js index 32174a56..531ccbf7 100644 --- a/lib/shared/config-validator.js +++ b/lib/shared/config-validator.js @@ -5,6 +5,12 @@ /* eslint class-methods-use-this: "off" */ +//------------------------------------------------------------------------------ +// Typedefs +//------------------------------------------------------------------------------ + +/** @typedef {import("../shared/types").Rule} Rule */ + //------------------------------------------------------------------------------ // Requirements //------------------------------------------------------------------------------ @@ -33,6 +39,13 @@ const severityMap = { const validated = new WeakSet(); +// JSON schema that disallows passing any options +const noOptionsSchema = Object.freeze({ + type: "array", + minItems: 0, + maxItems: 0 +}); + //----------------------------------------------------------------------------- // Exports //----------------------------------------------------------------------------- @@ -44,17 +57,36 @@ export default class ConfigValidator { /** * Gets a complete options schema for a rule. - * @param {{create: Function, schema: (Array|null)}} rule A new-style rule object - * @returns {Object} JSON Schema for the rule's options. + * @param {Rule} rule A rule object + * @throws {TypeError} If `meta.schema` is specified but is not an array, object or `false`. + * @returns {Object|null} JSON Schema for the rule's options. + * `null` if rule wasn't passed or its `meta.schema` is `false`. */ getRuleOptionsSchema(rule) { if (!rule) { return null; } - const schema = rule.schema || rule.meta && rule.meta.schema; + if (!rule.meta) { + return { ...noOptionsSchema }; // default if `meta.schema` is not specified + } - // Given a tuple of schemas, insert warning level at the beginning + const schema = rule.meta.schema; + + if (typeof schema === "undefined") { + return { ...noOptionsSchema }; // default if `meta.schema` is not specified + } + + // `schema:false` is an allowed explicit opt-out of options validation for the rule + if (schema === false) { + return null; + } + + if (typeof schema !== "object" || schema === null) { + throw new TypeError("Rule's `meta.schema` must be an array or object"); + } + + // ESLint-specific array form needs to be converted into a valid JSON Schema definition if (Array.isArray(schema)) { if (schema.length) { return { @@ -64,16 +96,13 @@ export default class ConfigValidator { maxItems: schema.length }; } - return { - type: "array", - minItems: 0, - maxItems: 0 - }; + // `schema:[]` is an explicit way to specify that the rule does not accept any options + return { ...noOptionsSchema }; } - // Given a full schema, leave it alone - return schema || null; + // `schema:` is assumed to be a valid JSON Schema definition + return schema; } /** @@ -101,10 +130,18 @@ export default class ConfigValidator { */ validateRuleSchema(rule, localOptions) { if (!ruleValidators.has(rule)) { - const schema = this.getRuleOptionsSchema(rule); + try { + const schema = this.getRuleOptionsSchema(rule); + + if (schema) { + ruleValidators.set(rule, ajv.compile(schema)); + } + } catch (err) { + const errorWithCode = new Error(err.message, { cause: err }); - if (schema) { - ruleValidators.set(rule, ajv.compile(schema)); + errorWithCode.code = "ESLINT_INVALID_RULE_OPTIONS_SCHEMA"; + + throw errorWithCode; } } @@ -137,13 +174,21 @@ export default class ConfigValidator { this.validateRuleSchema(rule, Array.isArray(options) ? options.slice(1) : []); } } catch (err) { - const enhancedMessage = `Configuration for rule "${ruleId}" is invalid:\n${err.message}`; + let enhancedMessage = err.code === "ESLINT_INVALID_RULE_OPTIONS_SCHEMA" + ? `Error while processing options validation schema of rule '${ruleId}': ${err.message}` + : `Configuration for rule "${ruleId}" is invalid:\n${err.message}`; if (typeof source === "string") { - throw new Error(`${source}:\n\t${enhancedMessage}`); - } else { - throw new Error(enhancedMessage); + enhancedMessage = `${source}:\n\t${enhancedMessage}`; } + + const enhancedError = new Error(enhancedMessage, { cause: err }); + + if (err.code) { + enhancedError.code = err.code; + } + + throw enhancedError; } } diff --git a/tests/fixtures/rules/custom-rule.cjs b/tests/fixtures/rules/custom-rule.cjs index 6d8b662b..a7cc1340 100644 --- a/tests/fixtures/rules/custom-rule.cjs +++ b/tests/fixtures/rules/custom-rule.cjs @@ -1,15 +1,17 @@ -module.exports = function(context) { +"use strict"; - "use strict"; +module.exports = { + meta: { + schema: [] + }, - return { - "Identifier": function(node) { - if (node.name === "foo") { - context.report(node, "Identifier cannot be named 'foo'."); + create(context) { + return { + "Identifier": function(node) { + if (node.name === "foo") { + context.report(node, "Identifier cannot be named 'foo'."); + } } - } - }; - + }; + } }; - -module.exports.schema = []; diff --git a/tests/fixtures/rules/dir1/no-strings.cjs b/tests/fixtures/rules/dir1/no-strings.cjs index 1f566ac0..4997bbbe 100644 --- a/tests/fixtures/rules/dir1/no-strings.cjs +++ b/tests/fixtures/rules/dir1/no-strings.cjs @@ -1,14 +1,15 @@ "use strict"; -module.exports = function(context) { +module.exports = { + create(context) { + return { - return { + "Literal": function(node) { + if (typeof node.value === 'string') { + context.report(node, "String!"); + } - "Literal": function(node) { - if (typeof node.value === 'string') { - context.report(node, "String!"); } - - } - }; + }; + } }; diff --git a/tests/fixtures/rules/dir2/no-literals.cjs b/tests/fixtures/rules/dir2/no-literals.cjs index fdaa2d08..d872718a 100644 --- a/tests/fixtures/rules/dir2/no-literals.cjs +++ b/tests/fixtures/rules/dir2/no-literals.cjs @@ -1,11 +1,12 @@ "use strict"; -module.exports = function(context) { +module.exports = { + create (context) { + return { - return { - - "Literal": function(node) { - context.report(node, "Literal!"); - } - }; + "Literal": function(node) { + context.report(node, "Literal!"); + } + }; + } }; diff --git a/tests/fixtures/rules/make-syntax-error-rule.cjs b/tests/fixtures/rules/make-syntax-error-rule.cjs index 528b4b0f..3d34d9d1 100644 --- a/tests/fixtures/rules/make-syntax-error-rule.cjs +++ b/tests/fixtures/rules/make-syntax-error-rule.cjs @@ -1,14 +1,18 @@ -module.exports = function(context) { - return { - Program: function(node) { - context.report({ - node: node, - message: "ERROR", - fix: function(fixer) { - return fixer.insertTextAfter(node, "this is a syntax error."); - } - }); - } - }; +module.exports = { + meta: { + schema: [] + }, + create(context) { + return { + Program: function(node) { + context.report({ + node: node, + message: "ERROR", + fix: function(fixer) { + return fixer.insertTextAfter(node, "this is a syntax error."); + } + }); + } + }; + } }; -module.exports.schema = []; diff --git a/tests/fixtures/rules/wrong/custom-rule.cjs b/tests/fixtures/rules/wrong/custom-rule.cjs index 9a64230c..b3923e4d 100644 --- a/tests/fixtures/rules/wrong/custom-rule.cjs +++ b/tests/fixtures/rules/wrong/custom-rule.cjs @@ -1,5 +1,7 @@ -module.exports = function() { +"use strict"; - "use strict"; - return (null).something; +module.exports = { + create() { + return (null).something; + } }; diff --git a/tests/lib/shared/config-validator.js b/tests/lib/shared/config-validator.js new file mode 100644 index 00000000..abed9832 --- /dev/null +++ b/tests/lib/shared/config-validator.js @@ -0,0 +1,256 @@ +/** + * @fileoverview Tests for ConfigValidator class + */ + +//------------------------------------------------------------------------------ +// Requirements +//------------------------------------------------------------------------------ + +import { assert } from "chai"; +import nodeAssert from "assert"; + +import ConfigValidator from "../../../lib/shared/config-validator.js"; + +//------------------------------------------------------------------------------ +// Helpers +//------------------------------------------------------------------------------ + +const mockRule = { + meta: { + schema: [{ + enum: ["first", "second"] + }] + }, + create(context) { + return { + Program(node) { + context.report(node, "Expected a validation error."); + } + }; + } +}; + +const mockObjectRule = { + meta: { + schema: { + enum: ["first", "second"] + } + }, + create(context) { + return { + Program(node) { + context.report(node, "Expected a validation error."); + } + }; + } +}; + +const mockInvalidSchemaTypeRule = { + meta: { + schema: true + }, + create(context) { + return { + Program(node) { + context.report(node, "Expected a validation error."); + } + }; + } +}; + +const mockInvalidJSONSchemaRule = { + meta: { + schema: { + minItems: [] + } + }, + create(context) { + return { + Program(node) { + context.report(node, "Expected a validation error."); + } + }; + } +}; + +//------------------------------------------------------------------------------ +// Tests +//------------------------------------------------------------------------------ + +describe("ConfigValidator", () => { + let validator; + + beforeEach(() => { + validator = new ConfigValidator(); + }); + + describe("getRuleOptionsSchema", () => { + + const noOptionsSchema = { + type: "array", + minItems: 0, + maxItems: 0 + }; + + it("should return null for a missing rule", () => { + assert.strictEqual(validator.getRuleOptionsSchema(void 0), null); + assert.strictEqual(validator.getRuleOptionsSchema(null), null); + }); + + it("should not modify object schema", () => { + assert.deepStrictEqual(validator.getRuleOptionsSchema(mockObjectRule), { + enum: ["first", "second"] + }); + }); + + it("should return schema that doesn't accept options if rule doesn't have `meta`", () => { + const rule = {}; + const result = validator.getRuleOptionsSchema(rule); + + assert.deepStrictEqual(result, noOptionsSchema); + }); + + it("should return schema that doesn't accept options if rule doesn't have `meta.schema`", () => { + const rule = { meta: {} }; + const result = validator.getRuleOptionsSchema(rule); + + assert.deepStrictEqual(result, noOptionsSchema); + }); + + it("should return schema that doesn't accept options if `meta.schema` is `undefined`", () => { + const rule = { meta: { schema: void 0 } }; + const result = validator.getRuleOptionsSchema(rule); + + assert.deepStrictEqual(result, noOptionsSchema); + }); + + it("should return schema that doesn't accept options if `meta.schema` is `[]`", () => { + const rule = { meta: { schema: [] } }; + const result = validator.getRuleOptionsSchema(rule); + + assert.deepStrictEqual(result, noOptionsSchema); + }); + + it("should return JSON Schema definition object if `meta.schema` is in the array form", () => { + const firstOption = { enum: ["always", "never"] }; + const rule = { meta: { schema: [firstOption] } }; + const result = validator.getRuleOptionsSchema(rule); + + assert.deepStrictEqual( + result, + { + type: "array", + items: [firstOption], + minItems: 0, + maxItems: 1 + } + ); + }); + + it("should return `meta.schema` as is if `meta.schema` is an object", () => { + const schema = { + type: "array", + items: [{ + enum: ["always", "never"] + }] + }; + const rule = { meta: { schema } }; + const result = validator.getRuleOptionsSchema(rule); + + assert.deepStrictEqual(result, schema); + }); + + it("should return `null` if `meta.schema` is `false`", () => { + const rule = { meta: { schema: false } }; + const result = validator.getRuleOptionsSchema(rule); + + assert.strictEqual(result, null); + }); + + [null, true, 0, 1, "", "always", () => {}].forEach(schema => { + it(`should throw an error if \`meta.schema\` is ${typeof schema} ${schema}`, () => { + const rule = { meta: { schema } }; + + assert.throws(() => { + validator.getRuleOptionsSchema(rule); + }, "Rule's `meta.schema` must be an array or object"); + }); + }); + + it("should ignore top-level `schema` property", () => { + const rule = { schema: { enum: ["always", "never"] } }; + const result = validator.getRuleOptionsSchema(rule); + + assert.deepStrictEqual(result, noOptionsSchema); + }); + + }); + + describe("validateRuleOptions", () => { + + it("should throw for incorrect warning level number", () => { + const fn = validator.validateRuleOptions.bind(validator, mockRule, "mock-rule", 3, "tests"); + + assert.throws(fn, "tests:\n\tConfiguration for rule \"mock-rule\" is invalid:\n\tSeverity should be one of the following: 0 = off, 1 = warn, 2 = error (you passed '3').\n"); + }); + + it("should throw for incorrect warning level string", () => { + const fn = validator.validateRuleOptions.bind(validator, mockRule, "mock-rule", "booya", "tests"); + + assert.throws(fn, "tests:\n\tConfiguration for rule \"mock-rule\" is invalid:\n\tSeverity should be one of the following: 0 = off, 1 = warn, 2 = error (you passed '\"booya\"').\n"); + }); + + it("should throw for invalid-type warning level", () => { + const fn = validator.validateRuleOptions.bind(validator, mockRule, "mock-rule", [["error"]], "tests"); + + assert.throws(fn, "tests:\n\tConfiguration for rule \"mock-rule\" is invalid:\n\tSeverity should be one of the following: 0 = off, 1 = warn, 2 = error (you passed '[ \"error\" ]').\n"); + }); + + it("should only check warning level for nonexistent rules", () => { + const fn1 = validator.validateRuleOptions.bind(validator, void 0, "non-existent-rule", [3, "foobar"], "tests"); + + assert.throws(fn1, "tests:\n\tConfiguration for rule \"non-existent-rule\" is invalid:\n\tSeverity should be one of the following: 0 = off, 1 = warn, 2 = error (you passed '3').\n"); + + const fn2 = validator.validateRuleOptions.bind(validator, null, "non-existent-rule", [3, "foobar"], "tests"); + + assert.throws(fn2, "tests:\n\tConfiguration for rule \"non-existent-rule\" is invalid:\n\tSeverity should be one of the following: 0 = off, 1 = warn, 2 = error (you passed '3').\n"); + }); + + it("should throw for incorrect configuration values", () => { + const fn = validator.validateRuleOptions.bind(validator, mockRule, "mock-rule", [2, "frist"], "tests"); + + assert.throws(fn, "tests:\n\tConfiguration for rule \"mock-rule\" is invalid:\n\tValue \"frist\" should be equal to one of the allowed values.\n"); + }); + + it("should throw for too many configuration values", () => { + const fn = validator.validateRuleOptions.bind(validator, mockRule, "mock-rule", [2, "first", "second"], "tests"); + + assert.throws(fn, "tests:\n\tConfiguration for rule \"mock-rule\" is invalid:\n\tValue [\"first\",\"second\"] should NOT have more than 1 items.\n"); + }); + + it("should throw with error code ESLINT_INVALID_RULE_OPTIONS_SCHEMA for rule with an invalid schema type", () => { + const fn = validator.validateRuleOptions.bind(validator, mockInvalidSchemaTypeRule, "invalid-schema-rule", [2], "tests"); + + nodeAssert.throws( + fn, + { + code: "ESLINT_INVALID_RULE_OPTIONS_SCHEMA", + message: "tests:\n\tError while processing options validation schema of rule 'invalid-schema-rule': Rule's `meta.schema` must be an array or object" + } + ); + }); + + it("should throw with error code ESLINT_INVALID_RULE_OPTIONS_SCHEMA for rule with an invalid JSON schema", () => { + const fn = validator.validateRuleOptions.bind(validator, mockInvalidJSONSchemaRule, "invalid-schema-rule", [2], "tests"); + + nodeAssert.throws( + fn, + { + code: "ESLINT_INVALID_RULE_OPTIONS_SCHEMA", + message: "tests:\n\tError while processing options validation schema of rule 'invalid-schema-rule': minItems must be number" + } + ); + }); + + }); +});