From 5d73822a49deee48059c2b1bf32f411b6e619911 Mon Sep 17 00:00:00 2001 From: orangain Date: Mon, 19 Feb 2024 02:29:01 +0900 Subject: [PATCH] Add rules index-referencing-column and reference-actions (#343) * Add rule index-referencing-column * Describe index-referencing-column in rules/README * Refactor TableReference to have more properties * Add rule reference-actions * Use type assertion instead of non-null assertion * Use more descriptive names * Use Object.values instead of Object.entries * Add columnNames to TableReference * Add suggestedMigration to reference-actions * Describe reference-actions in rules/README * Add schema to suggestedMigration --- src/rules/README.md | 29 +++ src/rules/index.ts | 1 + src/rules/references.test.ts | 422 +++++++++++++++++++++++++++++++++++ src/rules/references.ts | 113 ++++++++++ 4 files changed, 565 insertions(+) create mode 100644 src/rules/references.test.ts create mode 100644 src/rules/references.ts diff --git a/src/rules/README.md b/src/rules/README.md index 538afca..39d2ca6 100644 --- a/src/rules/README.md +++ b/src/rules/README.md @@ -98,3 +98,32 @@ Identity tables that do not have a primary key defined. Tables can be ignored by }], }, ``` + +## index-referencing-column + +PostgreSQL does not automatically create an index on the referencing column (not the referenced column) of a foreign key constraint. This rule can enforce that you create an index on the referencing column. + +As the official PostgreSQL documentation states, it is not always needed to index the referencing columns, but it is often a good idea to do so. + +> Since a DELETE of a row from the referenced table or an UPDATE of a referenced column will require a scan of the referencing table for rows matching the old value, it is often a good idea to index the referencing columns too. Because this is not always needed, and there are many choices available on how to index, the declaration of a foreign key constraint does not automatically create an index on the referencing columns. + +You can learn more here: [Foreign Keys](https://www.postgresql.org/docs/current/ddl-constraints.html#DDL-CONSTRAINTS-FK) + +```js +rules: { + 'index-referencing-column': ['error'], +} +``` + +## reference-actions + +This rule enforces that foreign key constraints have specific `ON UPDATE` and `ON DELETE` actions. Available actions are: `NO ACTION`, `RESTRICT`, `CASCADE`, `SET NULL`, `SET DEFAULT`. When `onUpdate` or `onDelete` is not specified, the rule allows any action for the unspecified action. + +```js +rules: { + 'reference-actions': ['error', { + onUpdate: 'NO ACTION', + onDelete: 'CASCADE', + }], +} +``` diff --git a/src/rules/index.ts b/src/rules/index.ts index a5eac32..733c46b 100644 --- a/src/rules/index.ts +++ b/src/rules/index.ts @@ -1,4 +1,5 @@ export * from "./nameCasing"; export * from "./nameInflection"; +export * from "./references"; export * from "./requirePrimaryKey"; export * from "./types"; diff --git a/src/rules/references.test.ts b/src/rules/references.test.ts new file mode 100644 index 0000000..e55a622 --- /dev/null +++ b/src/rules/references.test.ts @@ -0,0 +1,422 @@ +import { Schema } from "extract-pg-schema"; +import { describe, expect, it, test, vi } from "vitest"; + +import DeepPartial from "../tests/DeepPartial"; +import { indexReferencingColumn, referenceActions } from "./references"; + +describe("indexReferencingColumn", () => { + describe("no tables", () => { + it("should pass when no tables exist", () => { + const mockReporter = vi.fn(); + const schemaObject: DeepPartial = { + tables: [], + views: [], + }; + + indexReferencingColumn.process({ + options: [], + schemaObject: schemaObject as Schema, + report: mockReporter, + }); + + expect(mockReporter).toBeCalledTimes(0); + }); + }); + + describe("single column reference", () => { + it("should pass when index found on referencing column", () => { + const mockReporter = vi.fn(); + const schemaObject: DeepPartial = { + name: "schema", + tables: [ + { + name: "test", + columns: [ + { + name: "id", + references: [{ name: "test_id_fkey" }], + }, + ], + indices: [{ name: "test_id_idx", columns: [{ name: "id" }] }], + }, + ], + }; + + indexReferencingColumn.process({ + options: [], + schemaObject: schemaObject as Schema, + report: mockReporter, + }); + + expect(mockReporter).toBeCalledTimes(0); + }); + + it("should report when no index found on referencing column", () => { + const mockReporter = vi.fn(); + const schemaObject: DeepPartial = { + name: "schema", + tables: [ + { + name: "test", + columns: [ + { + name: "id", + references: [{ name: "test_id_fkey" }], + }, + ], + indices: [], + }, + ], + }; + + indexReferencingColumn.process({ + options: [], + schemaObject: schemaObject as Schema, + report: mockReporter, + }); + + expect(mockReporter).toBeCalledTimes(1); + expect(mockReporter).toBeCalledWith( + expect.objectContaining({ + rule: "index-referencing-column", + identifier: `schema.test.test_id_fkey`, + message: `No index found on referencing column(s) id`, + suggestedMigration: `CREATE INDEX ON "schema"."test" ("id");`, + }), + ); + }); + + it("should report when referencing column is indexed as secondary column", () => { + const mockReporter = vi.fn(); + const schemaObject: DeepPartial = { + name: "schema", + tables: [ + { + name: "test", + columns: [ + { + name: "id", + references: [{ name: "test_id_fkey" }], + }, + { + name: "sub_id", + references: [], + }, + ], + indices: [ + { + name: "test_id_idx", + columns: [{ name: "sub_id" }, { name: "id" }], + }, + ], + }, + ], + }; + + indexReferencingColumn.process({ + options: [], + schemaObject: schemaObject as Schema, + report: mockReporter, + }); + + expect(mockReporter).toBeCalledTimes(1); + expect(mockReporter).toBeCalledWith( + expect.objectContaining({ + rule: "index-referencing-column", + identifier: `schema.test.test_id_fkey`, + message: `No index found on referencing column(s) id`, + suggestedMigration: `CREATE INDEX ON "schema"."test" ("id");`, + }), + ); + }); + }); + + describe("multiple column reference", () => { + it("should pass when index found on at least one referencing column", () => { + const mockReporter = vi.fn(); + const schemaObject: DeepPartial = { + name: "schema", + tables: [ + { + name: "test", + columns: [ + { + name: "id", + references: [{ name: "test_id_sub_id_fkey" }], + }, + { + name: "sub_id", + references: [{ name: "test_id_sub_id_fkey" }], + }, + ], + indices: [ + { name: "test_sub_id_idx", columns: [{ name: "sub_id" }] }, + ], + }, + ], + }; + + indexReferencingColumn.process({ + options: [], + schemaObject: schemaObject as Schema, + report: mockReporter, + }); + + expect(mockReporter).toBeCalledTimes(0); + }); + + it("should report when no index found on referencing columns", () => { + const mockReporter = vi.fn(); + const schemaObject: DeepPartial = { + name: "schema", + tables: [ + { + name: "test", + columns: [ + { + name: "id", + references: [{ name: "test_id_sub_id_fkey" }], + }, + { + name: "sub_id", + references: [{ name: "test_id_sub_id_fkey" }], + }, + ], + indices: [], + }, + ], + }; + + indexReferencingColumn.process({ + options: [], + schemaObject: schemaObject as Schema, + report: mockReporter, + }); + + expect(mockReporter).toBeCalledTimes(1); + expect(mockReporter).toBeCalledWith( + expect.objectContaining({ + rule: "index-referencing-column", + identifier: `schema.test.test_id_sub_id_fkey`, + message: `No index found on referencing column(s) id, sub_id`, + suggestedMigration: `CREATE INDEX ON "schema"."test" ("id", "sub_id");`, + }), + ); + }); + }); +}); + +describe("referenceActions", () => { + describe("no tables", () => { + it("should pass when no tables exist", () => { + const mockReporter = vi.fn(); + const schemaObject: DeepPartial = { + tables: [], + views: [], + }; + + referenceActions.process({ + options: [{}], + schemaObject: schemaObject as Schema, + report: mockReporter, + }); + + expect(mockReporter).toBeCalledTimes(0); + }); + }); + + describe("single column reference", () => { + test.each([ + { option: {} }, // [onUpdate, onDelete] = [skipped, skipped] + { option: { onUpdate: "NO ACTION" } }, // [correct, skipped] + { option: { onDelete: "CASCADE" } }, // [skipped, correct] + { option: { onUpdate: "NO ACTION", onDelete: "CASCADE" } }, // [correct, correct] + ])( + "should pass when specified value is undefined or same as actual: $option", + ({ option }) => { + const mockReporter = vi.fn(); + const schemaObject: DeepPartial = { + name: "schema", + tables: [ + { + name: "test", + columns: [ + { + name: "id", + references: [ + { + name: "test_id_fkey", + onUpdate: "NO ACTION", + onDelete: "CASCADE", + }, + ], + }, + ], + }, + ], + }; + + referenceActions.process({ + options: [option], + schemaObject: schemaObject as Schema, + report: mockReporter, + }); + + expect(mockReporter).toBeCalledTimes(0); + }, + ); + + test.each([ + { + option: { onUpdate: "CASCADE" }, // [onUpdate, onDelete] = [wrong, skipped] + expectedIssues: [ + { + message: `Reference action ON UPDATE expected to be "CASCADE" but got "NO ACTION"`, + suggestedMigration: `ALTER TABLE "schema"."test" DROP CONSTRAINT "test_id_fkey", ADD CONSTRAINT "test_id_fkey" FOREIGN KEY ("id") REFERENCES "other_table"("test_id") ON UPDATE CASCADE ON DELETE CASCADE;`, + }, + ], + }, + { + option: { onUpdate: "CASCADE", onDelete: "CASCADE" }, // [wrong, correct] + expectedIssues: [ + { + message: `Reference action ON UPDATE expected to be "CASCADE" but got "NO ACTION"`, + suggestedMigration: `ALTER TABLE "schema"."test" DROP CONSTRAINT "test_id_fkey", ADD CONSTRAINT "test_id_fkey" FOREIGN KEY ("id") REFERENCES "other_table"("test_id") ON UPDATE CASCADE ON DELETE CASCADE;`, + }, + ], + }, + { + option: { onDelete: "NO ACTION" }, // [skipped, wrong] + expectedIssues: [ + { + message: `Reference action ON DELETE expected to be "NO ACTION" but got "CASCADE"`, + suggestedMigration: `ALTER TABLE "schema"."test" DROP CONSTRAINT "test_id_fkey", ADD CONSTRAINT "test_id_fkey" FOREIGN KEY ("id") REFERENCES "other_table"("test_id") ON UPDATE NO ACTION ON DELETE NO ACTION;`, + }, + ], + }, + { + option: { onUpdate: "NO ACTION", onDelete: "NO ACTION" }, // [correct, wrong] + expectedIssues: [ + { + message: `Reference action ON DELETE expected to be "NO ACTION" but got "CASCADE"`, + suggestedMigration: `ALTER TABLE "schema"."test" DROP CONSTRAINT "test_id_fkey", ADD CONSTRAINT "test_id_fkey" FOREIGN KEY ("id") REFERENCES "other_table"("test_id") ON UPDATE NO ACTION ON DELETE NO ACTION;`, + }, + ], + }, + { + option: { onUpdate: "CASCADE", onDelete: "NO ACTION" }, // [wrong, wrong] + expectedIssues: [ + { + message: `Reference action ON UPDATE expected to be "CASCADE" but got "NO ACTION"`, + suggestedMigration: `ALTER TABLE "schema"."test" DROP CONSTRAINT "test_id_fkey", ADD CONSTRAINT "test_id_fkey" FOREIGN KEY ("id") REFERENCES "other_table"("test_id") ON UPDATE CASCADE ON DELETE NO ACTION;`, + }, + { + message: `Reference action ON DELETE expected to be "NO ACTION" but got "CASCADE"`, + suggestedMigration: `ALTER TABLE "schema"."test" DROP CONSTRAINT "test_id_fkey", ADD CONSTRAINT "test_id_fkey" FOREIGN KEY ("id") REFERENCES "other_table"("test_id") ON UPDATE CASCADE ON DELETE NO ACTION;`, + }, + ], + }, + ])( + "should report when specified value is not undefined and differs: $option", + ({ option, expectedIssues }) => { + const mockReporter = vi.fn(); + const schemaObject: DeepPartial = { + name: "schema", + tables: [ + { + name: "test", + columns: [ + { + name: "id", + references: [ + { + name: "test_id_fkey", + tableName: "other_table", + columnName: "test_id", + onUpdate: "NO ACTION", + onDelete: "CASCADE", + }, + ], + }, + ], + }, + ], + }; + + referenceActions.process({ + options: [option], + schemaObject: schemaObject as Schema, + report: mockReporter, + }); + + expect(mockReporter).toBeCalledTimes(expectedIssues.length); + expectedIssues.forEach((expectedIssue) => { + expect(mockReporter).toBeCalledWith( + expect.objectContaining({ + rule: "reference-actions", + identifier: `schema.test.test_id_fkey`, + message: expectedIssue.message, + suggestedMigration: expectedIssue.suggestedMigration, + }), + ); + }); + }, + ); + }); + + describe("multiple column reference", () => { + it("should report only once when multiple column reference has incorrect action", () => { + const mockReporter = vi.fn(); + const schemaObject: DeepPartial = { + name: "schema", + tables: [ + { + name: "test", + columns: [ + { + name: "id", + references: [ + { + name: "test_id_sub_id_fkey", + tableName: "other_table", + columnName: "test_id", + onUpdate: "NO ACTION", + onDelete: "CASCADE", + }, + ], + }, + { + name: "sub_id", + references: [ + { + name: "test_id_sub_id_fkey", + tableName: "other_table", + columnName: "test_sub_id", + onUpdate: "NO ACTION", + onDelete: "CASCADE", + }, + ], + }, + ], + }, + ], + }; + + referenceActions.process({ + options: [{ onUpdate: "CASCADE" }], + schemaObject: schemaObject as Schema, + report: mockReporter, + }); + + expect(mockReporter).toBeCalledTimes(1); + expect(mockReporter).toBeCalledWith( + expect.objectContaining({ + rule: "reference-actions", + identifier: `schema.test.test_id_sub_id_fkey`, + message: `Reference action ON UPDATE expected to be "CASCADE" but got "NO ACTION"`, + suggestedMigration: `ALTER TABLE "schema"."test" DROP CONSTRAINT "test_id_sub_id_fkey", ADD CONSTRAINT "test_id_sub_id_fkey" FOREIGN KEY ("id", "sub_id") REFERENCES "other_table"("test_id", "test_sub_id") ON UPDATE CASCADE ON DELETE CASCADE;`, + }), + ); + }); + }); +}); diff --git a/src/rules/references.ts b/src/rules/references.ts new file mode 100644 index 0000000..68e1b5b --- /dev/null +++ b/src/rules/references.ts @@ -0,0 +1,113 @@ +import { + ColumnReference, + TableColumn, + TableDetails, + TableIndex, +} from "extract-pg-schema"; +import * as R from "ramda"; + +import Rule from "../Rule"; + +export const indexReferencingColumn: Rule = { + name: "index-referencing-column", + docs: { + description: "Require index on referencing column of foreign keys", + }, + process({ schemaObject, report }) { + const validator = ({ columns, name: tableName, indices }: TableDetails) => { + const tableReferences = buildTableReferences(columns); + tableReferences.forEach((tableReference) => { + if ( + !columnsCoveredByIndex(tableReference.referencingColumnNames, indices) + ) { + report({ + rule: this.name, + identifier: `${schemaObject.name}.${tableName}.${tableReference.name}`, + message: `No index found on referencing column(s) ${tableReference.referencingColumnNames.join(", ")}`, + suggestedMigration: `CREATE INDEX ON ${quote(schemaObject.name)}.${quote(tableName)} (${tableReference.referencingColumnNames.map(quote).join(", ")});`, + }); + } + }); + }; + schemaObject.tables.forEach(validator); + }, +}; + +export const referenceActions: Rule = { + name: "reference-actions", + docs: { + description: + "Require references to have specific ON DELETE and ON UPDATE actions", + }, + process({ options: [{ onUpdate, onDelete }], schemaObject, report }) { + const validator = ({ columns, name: tableName }: TableDetails) => { + const tableReferences = buildTableReferences(columns); + function suggestedMigration(tableReference: TableReference): string { + return ( + `ALTER TABLE ${quote(schemaObject.name)}.${quote(tableName)} DROP CONSTRAINT ${quote(tableReference.name)}, ` + + `ADD CONSTRAINT ${quote(tableReference.name)} FOREIGN KEY (${tableReference.referencingColumnNames.map(quote).join(", ")}) ` + + `REFERENCES ${quote(tableReference.tableName)}(${tableReference.columnNames.map(quote).join(", ")}) ` + + `ON UPDATE ${onUpdate ?? tableReference.onUpdate} ON DELETE ${onDelete ?? tableReference.onDelete};` + ); + } + tableReferences.forEach((tableReference) => { + if (onUpdate !== undefined && tableReference.onUpdate !== onUpdate) { + report({ + rule: this.name, + identifier: `${schemaObject.name}.${tableName}.${tableReference.name}`, + message: `Reference action ON UPDATE expected to be "${onUpdate}" but got "${tableReference.onUpdate}"`, + suggestedMigration: suggestedMigration(tableReference), + }); + } + if (onDelete !== undefined && tableReference.onDelete !== onDelete) { + report({ + rule: this.name, + identifier: `${schemaObject.name}.${tableName}.${tableReference.name}`, + message: `Reference action ON DELETE expected to be "${onDelete}" but got "${tableReference.onDelete}"`, + suggestedMigration: suggestedMigration(tableReference), + }); + } + }); + }; + schemaObject.tables.forEach(validator); + }, +}; + +type TableReference = Omit & { + columnNames: string[]; + referencingColumnNames: string[]; +}; + +function buildTableReferences(columns: TableColumn[]): TableReference[] { + type ColumnReferencePair = { + referncingColumnName: string; + reference: ColumnReference; + }; + const columnReferencePairs: ColumnReferencePair[] = columns + .map((c) => + c.references.map((r) => ({ referncingColumnName: c.name, reference: r })), + ) + .flat(); + const columnReferencePairsByName = R.groupBy( + (p) => p.reference.name, + columnReferencePairs, + ) as Record; + return Object.values(columnReferencePairsByName).map((pairs) => { + const columnNames = pairs.map((p) => p.reference.columnName); + const referencingColumnNames = pairs.map((p) => p.referncingColumnName); + const { columnName: _, ...rest } = pairs[0].reference; + return { columnNames, referencingColumnNames, ...rest }; + }); +} + +function columnsCoveredByIndex( + columns: string[], + indices: TableIndex[], +): boolean { + // At least one column must be covered by an index + return columns.some((c) => indices.some((i) => i.columns[0].name === c)); +} + +function quote(s: string): string { + return `"${s}"`; +}