Skip to content

Commit

Permalink
Fix: don't generate invalid options in config-rule (#8326)
Browse files Browse the repository at this point in the history
Previously, the `config-rule` logic for generating rule options had a bug where it could generate invalid options. For example, it could generate these options for the `id-match` rule:

```json
[2, { "properties": true }]
```

This is invalid because the first option in `id-match` must be a string, and the second option must be an object. The config generator doesn't handle strings, but it was incorrectly skipping the string option and trying to fill in the config with an object after realizing that it couldn't generate a string. As a result, it placed the object where the string was supposed to go, so an invalid set of options was created. A similar issue can appear when generating configs for the `max-statements` rule.

Luckily, the `id-match` and `max-statements` rule are still able to run when passed an invalid config, and the invalid config never results in fewer errors being reported than the default, so the invalid options wouldn't ever get output to a config file. However, this is only the case due to implementation details in the two rules, and a new rule could be introduced in the future that crashes when given an invalid schema, so this is still worth fixing.

This commit fixes the issue by stopping early when the option generator reaches an option that it doesn't know how to fill in.
  • Loading branch information
not-an-aardvark authored and gyandeeps committed Mar 27, 2017
1 parent 6eda3b5 commit d52173f
Show file tree
Hide file tree
Showing 3 changed files with 55 additions and 11 deletions.
24 changes: 14 additions & 10 deletions lib/config/config-rule.js
Expand Up @@ -223,7 +223,7 @@ class RuleConfigSet {
/**
* Add rule configurations from a schema object
* @param {Object} obj Schema item with type === "object"
* @returns {void}
* @returns {boolean} true if at least one schema for the object could be generated, false otherwise
*/
addObject(obj) {
const objectConfigSet = {
Expand Down Expand Up @@ -258,7 +258,10 @@ class RuleConfigSet {

if (objectConfigSet.objectConfigs.length > 0) {
this.ruleConfigs = this.ruleConfigs.concat(combineArrays(this.ruleConfigs, objectConfigSet.objectConfigs));
return true;
}

return false;
}
}

Expand All @@ -271,20 +274,21 @@ function generateConfigsFromSchema(schema) {
const configSet = new RuleConfigSet();

if (Array.isArray(schema)) {
schema.forEach(opt => {
for (const opt of schema) {
if (opt.enum) {
configSet.addEnums(opt.enum);
}

if (opt.type && opt.type === "object") {
configSet.addObject(opt);
}
} else if (opt.type && opt.type === "object") {
if (!configSet.addObject(opt)) {
break;
}

if (opt.oneOf) {
// TODO (IanVS): support oneOf
} else {

// TODO (IanVS): not yet implemented
// If we don't know how to fill in this option, don't fill in any of the following options.
break;
}
});
}
}
configSet.addErrorSeverity();
return configSet.ruleConfigs;
Expand Down
16 changes: 16 additions & 0 deletions tests/fixtures/config-rule/schemas.js
Expand Up @@ -69,6 +69,22 @@ module.exports = {
"additionalProperties": false
}],

mixedObjectWithNothingEnum: [{
"type": "object",
"properties": {},
"additionalProperties": false
},
{
"enum": ["always", "never"]
}],

mixedStringEnum: [{
"type": "string"
},
{
"enum": ["always", "never"]
}],

oneOf: [{
"oneOf": [
{
Expand Down
26 changes: 25 additions & 1 deletion tests/lib/config/config-rule.js
Expand Up @@ -230,7 +230,7 @@ describe("ConfigRule", () => {
});
});

describe("for a schema with an enum and an object with no usable properties", () => {
describe("for a schema with an enum followed by an object with no usable properties", () => {
before(() => {
actualConfigs = ConfigRule.generateConfigsFromSchema(schema.mixedEnumObjectWithNothing);
});
Expand All @@ -242,6 +242,30 @@ describe("ConfigRule", () => {
});
});

describe("for a schema with an enum preceded by an object with no usable properties", () => {
before(() => {
actualConfigs = ConfigRule.generateConfigsFromSchema(schema.mixedObjectWithNothingEnum);
});

it("should not create a config for the enum", () => {
const expectedConfigs = [2];

assert.sameDeepMembers(actualConfigs, expectedConfigs);
});
});

describe("for a schema with an enum preceded by a string", () => {
before(() => {
actualConfigs = ConfigRule.generateConfigsFromSchema(schema.mixedStringEnum);
});

it("should not create a config for the enum", () => {
const expectedConfigs = [2];

assert.sameDeepMembers(actualConfigs, expectedConfigs);
});
});

describe("for a schema with oneOf", () => {

before(() => {
Expand Down

0 comments on commit d52173f

Please sign in to comment.