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
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@
"@types/prettier": "*",
"@typescript-eslint/rule-schema-to-typescript-types": "6.0.0",
"@typescript-eslint/rule-tester": "6.0.0",
"ajv": "^6.12.6",
"chalk": "^5.0.1",
"cross-fetch": "*",
"jest-specific-snapshot": "*",
Expand Down
185 changes: 117 additions & 68 deletions packages/eslint-plugin/src/rules/no-restricted-imports.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
import type { TSESTree } from '@typescript-eslint/utils';
import type { JSONSchema4 } from '@typescript-eslint/utils/json-schema';
import type {
JSONSchema4AnyOfSchema,
JSONSchema4ArraySchema,
JSONSchema4ObjectSchema,
} from '@typescript-eslint/utils/json-schema';
import type {
ArrayOfStringOrObject,
ArrayOfStringOrObjectPatterns,
Expand All @@ -19,38 +23,96 @@
export type Options = InferOptionsTypeFromRule<typeof baseRule>;
export type MessageIds = InferMessageIdsTypeFromRule<typeof baseRule>;

const arrayOfStringsOrObjects: JSONSchema4 = {
// 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;

Check warning on line 33 in packages/eslint-plugin/src/rules/no-restricted-imports.ts

View check run for this annotation

Codecov / codecov/patch

packages/eslint-plugin/src/rules/no-restricted-imports.ts#L33

Added line #L33 was not covered by tests
}
};

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: JSONSchema4ObjectSchema['properties'];
required: string[];
},
];
};
};
patterns: {
anyOf: [
{ type: 'array'; items: { type: 'string' } },
{
type: 'array';
items: {
type: 'object';
properties: JSONSchema4ObjectSchema['properties'];
required: string[];
};
},
];
};
};
},
];
},
];
};

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

const arrayOfStringsOrObjects: JSONSchema4ArraySchema = {
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 = {

const arrayOfStringsOrObjectPatterns: JSONSchema4AnyOfSchema = {
anyOf: [
{
type: 'array',
Expand All @@ -63,43 +125,48 @@
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: JSONSchema4AnyOfSchema = {
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 +220,7 @@
},
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
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,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: {
type: 'string',
Expand Down
32 changes: 32 additions & 0 deletions packages/eslint-plugin/tests/areOptionsValid.test.ts
Original file line number Diff line number Diff line change
@@ -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: [{ type: 'string', 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
Original file line number Diff line number Diff line change
@@ -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
Original file line number Diff line number Diff line change
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.