Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(eslint-plugin): fix schemas across several rules and add schema tests #6947

Merged
merged 11 commits into from Jul 10, 2023
1 change: 1 addition & 0 deletions packages/eslint-plugin/package.json
Expand Up @@ -68,6 +68,7 @@
"devDependencies": {
"@typescript-eslint/rule-schema-to-typescript-types": "5.59.5",
"@typescript-eslint/rule-tester": "5.59.5",
"ajv": "^6.12.6",
"cross-fetch": "*",
"jest-specific-snapshot": "*",
"json-schema": "*",
Expand Down
175 changes: 110 additions & 65 deletions packages/eslint-plugin/src/rules/no-restricted-imports.ts
Expand Up @@ -19,37 +19,95 @@ const baseRule = getESLintCoreRule('no-restricted-imports');
export type Options = InferOptionsTypeFromRule<typeof baseRule>;
export type MessageIds = InferMessageIdsTypeFromRule<typeof baseRule>;

// In some versions of eslint, the base rule has a completely incompatible schema
// This helper function is to safely try to get parts of the schema. If it's not
// possible, we'll fallback to less strict checks.
const tryAccess = <T>(getter: () => T, fallback: T): T => {
try {
return getter();
} catch {
return fallback;
}
};

const baseSchema = baseRule.meta.schema as {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: why did you switch back to basing off of the base rule's schema here?
Any reason or was it just to go back to de-duping things?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No strong reason: just to dedupe things / keep things in sync.

anyOf: [
unknown,
{
type: 'array';
items: [
{
type: 'object';
properties: {
paths: {
type: 'array';
items: {
anyOf: [
{ type: 'string' },
{
type: 'object';
properties: JSONSchema4['properties'];
required: string[];
},
];
};
};
patterns: {
anyOf: [
{ type: 'array'; items: { type: 'string' } },
{
type: 'array';
items: {
type: 'object';
properties: JSONSchema4['properties'];
required: string[];
};
},
];
};
};
},
];
},
];
};

const allowTypeImportsOptionSchema: JSONSchema4['properties'] = {
allowTypeImports: {
type: 'boolean',
description: 'Disallow value imports, but allow type-only imports.',
},
};

const arrayOfStringsOrObjects: JSONSchema4 = {
type: 'array',
items: {
anyOf: [
{ type: 'string' },
{
type: 'object',
additionalProperties: false,
properties: {
name: { type: 'string' },
message: {
type: 'string',
minLength: 1,
},
importNames: {
type: 'array',
items: {
type: 'string',
},
},
allowTypeImports: {
type: 'boolean',
description: 'Disallow value imports, but allow type-only imports.',
},
...tryAccess(
() =>
baseSchema.anyOf[1].items[0].properties.paths.items.anyOf[1]
.properties,
undefined,
),
...allowTypeImportsOptionSchema,
},
additionalProperties: false,
required: ['name'],
required: tryAccess(
() =>
baseSchema.anyOf[1].items[0].properties.paths.items.anyOf[1]
.required,
undefined,
),
},
],
},
uniqueItems: true,
};

const arrayOfStringsOrObjectPatterns: JSONSchema4 = {
anyOf: [
{
Expand All @@ -63,43 +121,48 @@ const arrayOfStringsOrObjectPatterns: JSONSchema4 = {
type: 'array',
items: {
type: 'object',
additionalProperties: false,
properties: {
importNames: {
type: 'array',
items: {
type: 'string',
},
minItems: 1,
uniqueItems: true,
},
group: {
type: 'array',
items: {
type: 'string',
},
minItems: 1,
uniqueItems: true,
},
message: {
type: 'string',
minLength: 1,
},
caseSensitive: {
type: 'boolean',
},
allowTypeImports: {
type: 'boolean',
description: 'Disallow value imports, but allow type-only imports.',
},
...tryAccess(
() =>
baseSchema.anyOf[1].items[0].properties.patterns.anyOf[1].items
.properties,
undefined,
),
...allowTypeImportsOptionSchema,
},
additionalProperties: false,
required: ['group'],
required: tryAccess(
() =>
baseSchema.anyOf[1].items[0].properties.patterns.anyOf[1].items
.required,
[],
),
},
uniqueItems: true,
},
],
};

const schema: JSONSchema4 = {
anyOf: [
arrayOfStringsOrObjects,
{
type: 'array',
items: [
{
type: 'object',
properties: {
paths: arrayOfStringsOrObjects,
patterns: arrayOfStringsOrObjectPatterns,
},
additionalProperties: false,
},
],
additionalItems: false,
},
],
};

function isObjectOfPaths(
obj: unknown,
): obj is { paths: ArrayOfStringOrObject } {
Expand Down Expand Up @@ -153,25 +216,7 @@ export default createRule<Options, MessageIds>({
},
messages: baseRule.meta.messages,
fixable: baseRule.meta.fixable,
schema: {
anyOf: [
arrayOfStringsOrObjects,
{
type: 'array',
items: [
{
type: 'object',
properties: {
paths: arrayOfStringsOrObjects,
patterns: arrayOfStringsOrObjectPatterns,
},
additionalProperties: false,
},
],
additionalItems: false,
},
],
},
schema,
},
defaultOptions: [],
create(context) {
Expand Down
1 change: 0 additions & 1 deletion packages/eslint-plugin/src/rules/parameter-properties.ts
Expand Up @@ -59,7 +59,6 @@ export default util.createRule<Options, MessageIds>({
items: {
$ref: '#/items/0/$defs/modifier',
},
minItems: 1,
bradzacher marked this conversation as resolved.
Show resolved Hide resolved
},
prefer: {
enum: ['class-property', 'parameter-property'],
Expand Down
32 changes: 32 additions & 0 deletions packages/eslint-plugin/tests/areOptionsValid.test.ts
@@ -0,0 +1,32 @@
import * as util from '../src/util';
import { areOptionsValid } from './areOptionsValid';

const exampleRule = util.createRule<['value-a' | 'value-b'], never>({
name: 'my-example-rule',
meta: {
type: 'layout',
docs: {
description: 'Detects something or other',
},
schema: [{ enum: ['value-a', 'value-b'] }],
messages: {},
},
defaultOptions: ['value-a'],
create() {
return {};
},
});

test('returns true for valid options', () => {
expect(areOptionsValid(exampleRule, ['value-a'])).toBe(true);
});

describe('returns false for invalid options', () => {
test('bad enum value', () => {
expect(areOptionsValid(exampleRule, ['value-c'])).toBe(false);
});

test('bad type', () => {
expect(areOptionsValid(exampleRule, [true])).toBe(false);
});
});
44 changes: 44 additions & 0 deletions packages/eslint-plugin/tests/areOptionsValid.ts
@@ -0,0 +1,44 @@
import { TSUtils } from '@typescript-eslint/utils';
import type { RuleModule } from '@typescript-eslint/utils/ts-eslint';
import Ajv from 'ajv';
import type { JSONSchema4 } from 'json-schema';

const ajv = new Ajv({ async: false });

export function areOptionsValid(
rule: RuleModule<string, readonly unknown[]>,
options: unknown,
): boolean {
const normalizedSchema = normalizeSchema(rule.meta.schema);

const valid = ajv.validate(normalizedSchema, options);
if (typeof valid !== 'boolean') {
// Schema could not validate options synchronously. This is not allowed for ESLint rules.
return false;
}

return valid;
}

function normalizeSchema(
schema: JSONSchema4 | readonly JSONSchema4[],
): JSONSchema4 {
if (!TSUtils.isArray(schema)) {
return schema;
}

if (schema.length === 0) {
return {
type: 'array',
minItems: 0,
maxItems: 0,
};
}

return {
type: 'array',
items: schema as JSONSchema4[],
minItems: 0,
maxItems: schema.length,
};
}
17 changes: 17 additions & 0 deletions packages/eslint-plugin/tests/rules/array-type.test.ts
Expand Up @@ -4,6 +4,7 @@ import { TSESLint } from '@typescript-eslint/utils';

import type { OptionString } from '../../src/rules/array-type';
import rule from '../../src/rules/array-type';
import { areOptionsValid } from '../areOptionsValid';

const ruleTester = new RuleTester({
parser: '@typescript-eslint/parser',
Expand Down Expand Up @@ -2156,3 +2157,19 @@ type BrokenArray = {
);
});
});

describe('schema validation', () => {
// https://github.com/typescript-eslint/typescript-eslint/issues/6852
test("array-type does not accept 'simple-array' option", () => {
if (areOptionsValid(rule, [{ default: 'simple-array' }])) {
throw new Error(`Options succeeded validation for bad options`);
}
});

// https://github.com/typescript-eslint/typescript-eslint/issues/6892
test('array-type does not accept non object option', () => {
if (areOptionsValid(rule, ['array'])) {
throw new Error(`Options succeeded validation for bad options`);
}
});
});
Comment on lines +2161 to +2175
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should do a rule-tester style API here to remove the boilerplate.
For example something like this:

ruleOptionsTester(rule, {
  valid: [ ... ],
  invalid: [
    [{ default: 'simple-array' }],
    ['array'],
  ],
});

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think something like this would be neat if we end up doing a lot of these kinds of tests. For now I think it's simple enough that we can leave this to be done in future if necessary?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that there is
eslint/rfcs#103 and the implementing PR eslint/eslint#16823

This would allow us to include these tests in the rule like:

tester.run('rule', rule, {
  valid: [ ... ],
  invalid: [ ... ],
  fatal: [
    {
      options: ['simple-array'],
      error: {
        name: 'SchemaValidationError',
        message: 'Whatever the json schema error is',
      },
    },
    {
      options: ['array'],
      error: {
        name: 'SchemaValidationError',
        message: 'Whatever the json schema error is',
      },
    },
  ],
}

so definitely we don't need to bother with building a bespoke test framework!

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

32 changes: 32 additions & 0 deletions packages/eslint-plugin/tests/schemas.test.ts
Expand Up @@ -7,6 +7,7 @@ import path from 'path';
import { format, resolveConfig } from 'prettier';

import rules from '../src/rules/index';
import { areOptionsValid } from './areOptionsValid';

const snapshotFolder = path.resolve(__dirname, 'schema-snapshots');
try {
Expand Down Expand Up @@ -153,3 +154,34 @@ describe('Rules should only define valid keys on schemas', () => {
});
}
});

describe('Rule schemas should validate options correctly', () => {
// Normally, we use the rule's default options as an example of valid options.
// However, the defaults might not actually be valid (especially in the case
// where the defaults have to cover multiple incompatible options).
// This override allows providing example valid options for rules which don't
// accept their defaults.
const overrideValidOptions: Record<string, unknown> = {
semi: ['never'],
'func-call-spacing': ['never'],
};

for (const [ruleName, rule] of Object.entries(rules)) {
test(`${ruleName} must accept valid options`, () => {
if (
!areOptionsValid(
rule,
overrideValidOptions[ruleName] ?? rule.defaultOptions,
)
) {
throw new Error(`Options failed validation against rule's schema`);
}
});

test(`${ruleName} rejects arbitrary options`, () => {
if (areOptionsValid(rule, [{ 'arbitrary-schemas.test.ts': true }])) {
throw new Error(`Options succeeded validation for arbitrary options`);
}
});
}
});