From d5c678acb0813ce51c6d3eca9c5cef911218e050 Mon Sep 17 00:00:00 2001 From: Ben Chelli Date: Fri, 27 Aug 2021 23:33:53 +0200 Subject: [PATCH 1/8] Add support for ALTER TABLE with multiple statements --- src/ast-mapper.ts | 88 ++++++++++++++++----------- src/syntax/alter-table.ne | 9 ++- src/syntax/alter-table.spec.ts | 107 ++++++++++++++++++++------------- src/syntax/ast.ts | 2 +- src/syntax/spec-utils.ts | 2 +- 5 files changed, 127 insertions(+), 81 deletions(-) diff --git a/src/ast-mapper.ts b/src/ast-mapper.ts index 4dfe975..d65c545 100644 --- a/src/ast-mapper.ts +++ b/src/ast-mapper.ts @@ -642,51 +642,67 @@ export class AstDefaultMapper implements IAstMapper { if (!table) { return null; // no table } - let change: a.TableAlteration | nil; - switch (st.change.type) { - case 'add column': { - change = this.addColumn(st.change, st.table); - break; - } - case 'add constraint': { - change = this.addConstraint(st.change, st.table); - break; - } - case 'alter column': { - change = this.alterColumn(st.change, st.table); - break; - } - case 'rename': { - change = this.renameTable(st.change, st.table); - break; - } - case 'rename column': { - change = this.renameColumn(st.change, st.table); - break; - } - case 'rename constraint': { - change = this.renameConstraint(st.change, st.table); - break; - } - case 'drop column': { - change = this.dropColumn(st.change, st.table); - break; + let changes: a.TableAlteration[] = []; + let hasChanged: boolean = false; + for (let i = 0; i < (st.changes?.length || 0); i++) { + const currentChange: a.TableAlteration = st.changes[i]; + + let change: a.TableAlteration | nil; + switch (currentChange.type) { + case 'add column': { + change = this.addColumn(currentChange, st.table); + break; + } + case 'add constraint': { + change = this.addConstraint(currentChange, st.table); + break; + } + case 'alter column': { + change = this.alterColumn(currentChange, st.table); + break; + } + case 'rename': { + change = this.renameTable(currentChange, st.table); + break; + } + case 'rename column': { + change = this.renameColumn(currentChange, st.table); + break; + } + case 'rename constraint': { + change = this.renameConstraint(currentChange, st.table); + break; + } + case 'drop column': { + change = this.dropColumn(currentChange, st.table); + break; + } + case 'owner': { + change = this.setTableOwner(currentChange, st.table); + break; + } + default: + throw NotSupported.never(currentChange); } - case 'owner': { - change = this.setTableOwner(st.change, st.table); - break; + + hasChanged = hasChanged || (change != currentChange); + + if (!!change) { + changes.push(change); } - default: - throw NotSupported.never(st.change); } - if (!change) { + if (!changes.length) { return null; // no change left } + if (!hasChanged) { + return st; + } + return assignChanged(st, { table, - change, + changes, }); } diff --git a/src/syntax/alter-table.ne b/src/syntax/alter-table.ne index 2234bbc..328ed8a 100644 --- a/src/syntax/alter-table.ne +++ b/src/syntax/alter-table.ne @@ -5,15 +5,19 @@ # https://www.postgresql.org/docs/12/sql-altertable.html altertable_statement -> kw_alter %kw_table kw_ifexists:? %kw_only:? table_ref - altertable_action {% x => track(x, { + altertable_actions {% x => track(x, { type: 'alter table', ... x[2] ? {ifExists: true} : {}, ... x[3] ? {only: true} : {}, table: unwrap(x[4]), - change: unwrap(x[5]), + changes: unbox(x[5]).map(unwrap), }) %} +altertable_actions -> altertable_action (comma altertable_action {% last %}):* {% ([head, tail]) => { + return [head, ...(tail || [])]; +} %} + altertable_action -> altertable_rename_table | altertable_rename_column @@ -104,3 +108,4 @@ altercol_generated_seq setSeqOpts(ret, x[1]); return track(x, ret); }%} + diff --git a/src/syntax/alter-table.spec.ts b/src/syntax/alter-table.spec.ts index 144e25d..c3aa17f 100644 --- a/src/syntax/alter-table.spec.ts +++ b/src/syntax/alter-table.spec.ts @@ -12,14 +12,14 @@ describe('Alter table', () => { _location: { start: 12, end: 16 }, name: 'test' }, - change: { + changes: [{ _location: { start: 17, end: 34 }, type: 'rename', to: { _location: { start: 27, end: 34 }, name: 'newname' }, - } + }] }); checkAlterTable(['alter table if exists only test rename to newname'], { @@ -27,46 +27,46 @@ describe('Alter table', () => { table: { name: 'test' }, ifExists: true, only: true, - change: { + changes: [{ type: 'rename', to: { name: 'newname' }, - } + }] }); checkAlterTable(['alter table ONLY test rename to newname'], { type: 'alter table', table: { name: 'test' }, only: true, - change: { + changes: [{ type: 'rename', to: { name: 'newname' }, - } + }] }); checkAlterTable(['alter table test rename column a to b', 'alter table test rename a to b',], { type: 'alter table', table: { name: 'test' }, - change: { + changes: [{ type: 'rename column', column: { name: 'a' }, to: { name: 'b', }, - } + }] }); checkAlterTable(['alter table test rename constraint a to b',], { type: 'alter table', table: { name: 'test' }, - change: { + changes: [{ type: 'rename constraint', constraint: { name: 'a' }, to: { name: 'b' }, - } + }] }); checkAlterTable(['alter table test add column a jsonb not null', 'alter table test add a jsonb not null'], { type: 'alter table', table: { name: 'test' }, - change: { + changes: [{ type: 'add column', column: { kind: 'column', @@ -74,13 +74,13 @@ describe('Alter table', () => { dataType: { name: 'jsonb' }, constraints: [{ type: 'not null' }], }, - } + }] }); checkAlterTable(['alter table test add column if not exists a jsonb not null', 'alter table test add if not exists a jsonb not null'], { type: 'alter table', table: { name: 'test' }, - change: { + changes: [{ type: 'add column', ifNotExists: true, column: { @@ -89,73 +89,73 @@ describe('Alter table', () => { dataType: { name: 'jsonb' }, constraints: [{ type: 'not null' }], }, - } + }] }); checkAlterTable(['alter table test drop column if exists a', 'alter table test drop if exists a'], { type: 'alter table', table: { name: 'test' }, - change: { + changes: [{ type: 'drop column', column: { name: 'a' }, ifExists: true, - } + }] }); checkAlterTable(['alter table test drop column a', 'alter table test drop a'], { type: 'alter table', table: { name: 'test' }, - change: { + changes: [{ type: 'drop column', column: { name: 'a' }, - } + }] }); checkAlterTable(['alter table test alter column a set data type jsonb', 'alter table test alter a type jsonb'], { type: 'alter table', table: { name: 'test' }, - change: { + changes: [{ type: 'alter column', column: { name: 'a' }, alter: { type: 'set type', dataType: { name: 'jsonb' }, } - } + }] }); checkAlterTable(['alter table test alter a set default 42'], { type: 'alter table', table: { name: 'test' }, - change: { + changes: [{ type: 'alter column', column: { name: 'a' }, alter: { type: 'set default', default: { type: 'integer', value: 42 } } - } + }] }); checkAlterTable(['alter table test alter a drop default'], { type: 'alter table', table: { name: 'test' }, - change: { + changes: [{ type: 'alter column', column: { name: 'a' }, alter: { type: 'drop default', } - } + }] }); checkAlterTable(['alter table test alter a drop not null'], { type: 'alter table', table: { name: 'test' }, - change: { + changes: [{ type: 'alter column', column: { name: 'a' }, alter: { type: 'drop not null', } - } + }] }); @@ -169,7 +169,7 @@ describe('Alter table', () => { _location: { start: 12, end: 15 }, name: 'tbl' }, - change: { + changes: [{ type: 'add constraint', _location: { start: 16, end: 51 }, constraint: { @@ -193,13 +193,13 @@ describe('Alter table', () => { }, } }, - } + }] }); checkAlterTable(`ALTER TABLE tbl ADD check (a > 0)`, { type: 'alter table', table: { name: 'tbl' }, - change: { + changes: [{ type: 'add constraint', constraint: { type: 'check', @@ -210,7 +210,7 @@ describe('Alter table', () => { right: { type: 'integer', value: 0 }, } }, - } + }] }); @@ -221,7 +221,7 @@ describe('Alter table', () => { ON DELETE NO ACTION ON UPDATE NO ACTION;`, { type: 'alter table', table: { name: 'photo' }, - change: { + changes: [{ type: 'add constraint', constraint: { type: 'foreign key', @@ -232,7 +232,7 @@ describe('Alter table', () => { onUpdate: 'no action', onDelete: 'no action', } - } + }] }); @@ -240,24 +240,24 @@ describe('Alter table', () => { PRIMARY KEY ("a", "b")`, { type: 'alter table', table: { name: 'test' }, - change: { + changes: [{ type: 'add constraint', constraint: { type: 'primary key', constraintName: { name: 'cname' }, columns: [{ name: 'a' }, { name: 'b' }], } - } + }] }) checkAlterTable(`ALTER TABLE public.tbl OWNER to postgres;`, { type: 'alter table', table: { name: 'tbl', schema: 'public' }, - change: { + changes: [{ type: 'owner', to: { name: 'postgres' }, - } + }] }) // https://github.com/oguimbal/pg-mem/issues/9 @@ -266,7 +266,7 @@ describe('Alter table', () => { type: 'alter table', table: { name: 'location', schema: 'public' }, only: true, - change: { + changes: [{ type: 'add constraint', constraint: { type: 'foreign key', @@ -276,7 +276,7 @@ describe('Alter table', () => { foreignTable: { name: 'city', schema: 'public' }, match: 'full', } - } + }] }); @@ -290,7 +290,7 @@ describe('Alter table', () => { );`, { type: 'alter table', table: { name: 'city', schema: 'public' }, - change: { + changes: [{ type: 'alter column', column: { name: 'city_id' }, alter: { @@ -305,6 +305,31 @@ describe('Alter table', () => { cache: 1, } } - } + }] }) -}); \ No newline at end of file + + // https://github.com/oguimbal/pgsql-ast-parser/issues/57 + checkAlterTable(` + ALTER TABLE public.tbl + OWNER to postgres, + ADD check (a > 0); + `, { + type: 'alter table', + table: { name: 'tbl', schema: 'public' }, + changes: [{ + type: 'owner', + to: { name: 'postgres' }, + }, { + type: 'add constraint', + constraint: { + type: 'check', + expr: { + type: 'binary', + left: { type: 'ref', name: 'a' }, + op: '>', + right: { type: 'integer', value: 0 }, + } + }, + }] + }) +}); diff --git a/src/syntax/ast.ts b/src/syntax/ast.ts index 8202628..331329f 100644 --- a/src/syntax/ast.ts +++ b/src/syntax/ast.ts @@ -231,7 +231,7 @@ export interface AlterTableStatement extends PGNode { table: QNameAliased; only?: boolean; ifExists?: boolean; - change: TableAlteration; + changes: TableAlteration[]; } export interface TableAlterationRename extends PGNode { diff --git a/src/syntax/spec-utils.ts b/src/syntax/spec-utils.ts index 5379541..49bab8e 100644 --- a/src/syntax/spec-utils.ts +++ b/src/syntax/spec-utils.ts @@ -310,4 +310,4 @@ export function tbl(nm: string): FromTable { type: 'table', name: name(nm), }; -} \ No newline at end of file +} From fb476c775494d5bd89b986f58a108b79fe5253c6 Mon Sep 17 00:00:00 2001 From: Ben Chelli Date: Sat, 28 Aug 2021 00:04:14 +0200 Subject: [PATCH 2/8] fix to SQL testing --- src/ast-mapper.ts | 63 +++++++++++++++++++---------------------------- 1 file changed, 25 insertions(+), 38 deletions(-) diff --git a/src/ast-mapper.ts b/src/ast-mapper.ts index d65c545..4b66ce6 100644 --- a/src/ast-mapper.ts +++ b/src/ast-mapper.ts @@ -29,6 +29,7 @@ export interface IAstPartialMapper { transaction?: (val: a.CommitStatement | a.RollbackStatement | a.StartTransactionStatement) => a.Statement | nil createIndex?: (val: a.CreateIndexStatement) => a.Statement | nil alterTable?: (st: a.AlterTableStatement) => a.Statement | nil + tableAlteration?: (change: a.TableAlteration, table: a.QNameAliased) => a.TableAlteration | nil dropColumn?: (change: a.TableAlterationDropColumn, table: a.QNameAliased) => a.TableAlteration | nil renameConstraint?: (change: a.TableAlterationRenameConstraint, table: a.QNameAliased) => a.TableAlteration | nil setTableOwner?: (change: a.TableAlterationOwner, table: a.QNameAliased) => a.TableAlteration | nil @@ -646,44 +647,7 @@ export class AstDefaultMapper implements IAstMapper { let hasChanged: boolean = false; for (let i = 0; i < (st.changes?.length || 0); i++) { const currentChange: a.TableAlteration = st.changes[i]; - - let change: a.TableAlteration | nil; - switch (currentChange.type) { - case 'add column': { - change = this.addColumn(currentChange, st.table); - break; - } - case 'add constraint': { - change = this.addConstraint(currentChange, st.table); - break; - } - case 'alter column': { - change = this.alterColumn(currentChange, st.table); - break; - } - case 'rename': { - change = this.renameTable(currentChange, st.table); - break; - } - case 'rename column': { - change = this.renameColumn(currentChange, st.table); - break; - } - case 'rename constraint': { - change = this.renameConstraint(currentChange, st.table); - break; - } - case 'drop column': { - change = this.dropColumn(currentChange, st.table); - break; - } - case 'owner': { - change = this.setTableOwner(currentChange, st.table); - break; - } - default: - throw NotSupported.never(currentChange); - } + const change: a.TableAlteration | nil = this.tableAlteration(currentChange, st.table); hasChanged = hasChanged || (change != currentChange); @@ -707,6 +671,29 @@ export class AstDefaultMapper implements IAstMapper { } + tableAlteration(change: a.TableAlteration, table: a.QNameAliased): a.TableAlteration | nil { + switch (change.type) { + case 'add column': + return this.addColumn(change, table); + case 'add constraint': + return this.addConstraint(change, table); + case 'alter column': + return this.alterColumn(change, table); + case 'rename': + return this.renameTable(change, table); + case 'rename column': + return this.renameColumn(change, table); + case 'rename constraint': + return this.renameConstraint(change, table); + case 'drop column': + return this.dropColumn(change, table); + case 'owner': + return this.setTableOwner(change, table); + default: + throw NotSupported.never(change); + } + } + dropColumn(change: a.TableAlterationDropColumn, table: a.QNameAliased): a.TableAlteration | nil { return change; } From cfc01066dc387959750ff781518f1b58c7a7bc75 Mon Sep 17 00:00:00 2001 From: Ben Chelli Date: Sat, 28 Aug 2021 00:17:34 +0200 Subject: [PATCH 3/8] Add table alteration to the to-sql.js --- src/to-sql.ts | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/src/to-sql.ts b/src/to-sql.ts index 57a0f82..ae6b2a2 100644 --- a/src/to-sql.ts +++ b/src/to-sql.ts @@ -328,6 +328,29 @@ const visitor = astVisitor(m => ({ m.super().alterTable(t); }, + tableAlteration: (change, table) => { + switch (change.type) { + case 'add column': + return m.addColumn(change, table); + case 'add constraint': + return m.addConstraint(change, table); + case 'alter column': + return m.alterColumn(change, table); + case 'rename': + return m.renameTable(change, table); + case 'rename column': + return m.renameColumn(change, table); + case 'rename constraint': + return m.renameConstraint(change, table); + case 'drop column': + return m.dropColumn(change, table); + case 'owner': + return m.setTableOwner(change, table); + default: + throw NotSupported.never(change); + } + }, + array: v => { ret.push(v.type === 'array' ? 'ARRAY[' : '('); list(v.expressions, e => m.expr(e), false); From 3baacde127f403ac64362c5512908a5454e31459 Mon Sep 17 00:00:00 2001 From: Ben Chelli Date: Sat, 28 Aug 2021 10:52:07 +0200 Subject: [PATCH 4/8] ingore vim swap files --- .gitignore | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/.gitignore b/.gitignore index 20b4130..afdedc0 100644 --- a/.gitignore +++ b/.gitignore @@ -5,3 +5,12 @@ package-lock.json coverage dist lib + +# Vim swap files +# Swap +[._]*.s[a-v][a-z] +[._]*.sw[a-p] +[._]s[a-rt-v][a-z] +[._]ss[a-gi-z] +[._]sw[a-p] + From f62cdf640e31af8fd8e83528d3dad7d832637b2f Mon Sep 17 00:00:00 2001 From: Ben Chelli Date: Sat, 28 Aug 2021 10:52:42 +0200 Subject: [PATCH 5/8] Process properly reformatting multi-alter statements --- src/to-sql.ts | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/to-sql.ts b/src/to-sql.ts index ae6b2a2..8427675 100644 --- a/src/to-sql.ts +++ b/src/to-sql.ts @@ -100,6 +100,13 @@ function visitQualifiedName(cs: QName) { ret.push(ident(cs.name), ' '); } +function visitQualifiedNameAliased(cs: QNameAliased) { + visitQualifiedName(cs); + if (cs.alias) { + ret.push(' AS ', name(cs.alias), ' '); + } +} + function visitOrderBy(m: IAstVisitor, orderBy: OrderByStatement[]) { ret.push(' ORDER BY '); list(orderBy, e => { @@ -325,7 +332,8 @@ const visitor = astVisitor(m => ({ if (t.only) { ret.push(' ONLY '); } - m.super().alterTable(t); + visitQualifiedNameAliased(t.table); + list(t.changes, change => m.tableAlteration(change, t.table), false); }, tableAlteration: (change, table) => { From 4f306cfc168c3eda2adb99fb3d5f1976232912c4 Mon Sep 17 00:00:00 2001 From: Ben Chelli Date: Sat, 28 Aug 2021 10:53:02 +0200 Subject: [PATCH 6/8] add more test --- src/syntax/alter-table.spec.ts | 47 ++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/src/syntax/alter-table.spec.ts b/src/syntax/alter-table.spec.ts index c3aa17f..71982ae 100644 --- a/src/syntax/alter-table.spec.ts +++ b/src/syntax/alter-table.spec.ts @@ -332,4 +332,51 @@ describe('Alter table', () => { }, }] }) + + checkAlterTable(` + ALTER TABLE public.tbl + OWNER to postgres, + ALTER COLUMN city_id ADD GENERATED ALWAYS AS IDENTITY ( + SEQUENCE NAME public.city_city_id_seq + START WITH 0 + INCREMENT BY 1 + MINVALUE 0 + NO MAXVALUE + CACHE 1 + ), + ADD check (a > 0); + `, { + type: 'alter table', + table: { name: 'tbl', schema: 'public' }, + changes: [{ + type: 'owner', + to: { name: 'postgres' }, + }, { + type: 'alter column', + column: { name: 'city_id' }, + alter: { + type: 'add generated', + always: 'always', + sequence: { + name: { name: 'city_city_id_seq', schema: 'public' }, + startWith: 0, + incrementBy: 1, + minValue: 0, + maxValue: 'no maxvalue', + cache: 1, + } + } + }, { + type: 'add constraint', + constraint: { + type: 'check', + expr: { + type: 'binary', + left: { type: 'ref', name: 'a' }, + op: '>', + right: { type: 'integer', value: 0 }, + } + }, + }] + }) }); From b8cdae1df9c62e8da5e91b6f6870c4bba216c112 Mon Sep 17 00:00:00 2001 From: Ben Chelli Date: Sat, 28 Aug 2021 11:21:03 +0200 Subject: [PATCH 7/8] Fix type issues --- src/to-sql.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/to-sql.ts b/src/to-sql.ts index 8427675..77a2d70 100644 --- a/src/to-sql.ts +++ b/src/to-sql.ts @@ -1,7 +1,7 @@ import { IAstPartialMapper, AstDefaultMapper } from './ast-mapper'; import { astVisitor, IAstVisitor, IAstFullVisitor } from './ast-visitor'; import { NotSupported, nil, ReplaceReturnType, NoExtraProperties } from './utils'; -import { TableConstraint, JoinClause, ColumnConstraint, AlterSequenceStatement, CreateSequenceStatement, AlterSequenceSetOptions, CreateSequenceOptions, QName, SetGlobalValue, AlterColumnAddGenerated, QColumn, Name, OrderByStatement } from './syntax/ast'; +import { TableConstraint, JoinClause, ColumnConstraint, AlterSequenceStatement, CreateSequenceStatement, AlterSequenceSetOptions, CreateSequenceOptions, QName, SetGlobalValue, AlterColumnAddGenerated, QColumn, Name, OrderByStatement, QNameAliased } from './syntax/ast'; import { literal } from './pg-escape'; import { sqlKeywords } from './keywords'; @@ -103,7 +103,7 @@ function visitQualifiedName(cs: QName) { function visitQualifiedNameAliased(cs: QNameAliased) { visitQualifiedName(cs); if (cs.alias) { - ret.push(' AS ', name(cs.alias), ' '); + ret.push(' AS ', ident(cs.alias), ' '); } } From d5b321c7ffe9537ac5852fff12a45cbbfbdebaaf Mon Sep 17 00:00:00 2001 From: Ben Chelli Date: Sat, 28 Aug 2021 17:31:28 +0200 Subject: [PATCH 8/8] Remove unnecessary comment --- .gitignore | 1 - 1 file changed, 1 deletion(-) diff --git a/.gitignore b/.gitignore index afdedc0..657fde2 100644 --- a/.gitignore +++ b/.gitignore @@ -7,7 +7,6 @@ dist lib # Vim swap files -# Swap [._]*.s[a-v][a-z] [._]*.sw[a-p] [._]s[a-rt-v][a-z]