From 5cd54d83e8cb3f71cf829d097c7a86fbf9bb6eb1 Mon Sep 17 00:00:00 2001 From: Ariel Mashraki <7413593+a8m@users.noreply.github.com> Date: Thu, 19 May 2022 19:04:44 +0300 Subject: [PATCH] sql: apply ALTER reverse changes from down to top (#811) --- go.sum | 3 ++ sql/internal/sqlx/sqlx.go | 7 ++++ sql/internal/sqlx/sqlx_test.go | 58 ++++++++++++++++++++++++++++++++++ sql/mysql/migrate.go | 3 ++ sql/mysql/migrate_test.go | 41 ++++++++++++++++++++++-- sql/postgres/migrate.go | 3 ++ sql/postgres/migrate_test.go | 10 +++--- 7 files changed, 118 insertions(+), 7 deletions(-) diff --git a/go.sum b/go.sum index 095ef9adb69..e365cb3c3cb 100644 --- a/go.sum +++ b/go.sum @@ -14,6 +14,7 @@ github.com/chzyer/readline v0.0.0-20180603132655-2972be24d48e h1:fY5BOSpyZCqRo5O github.com/chzyer/readline v0.0.0-20180603132655-2972be24d48e/go.mod h1:nSuG5e5PlCu98SY8svDHJxuZscDgtXS6KTTbou5AhLI= github.com/chzyer/test v0.0.0-20180213035817-a1ea475d72b1 h1:q763qf9huN11kDQavWsoZXJNW3xEE4JJyHa5Q25/sd8= github.com/chzyer/test v0.0.0-20180213035817-a1ea475d72b1/go.mod h1:Q3SI9o4m/ZMnBNeIyt5eFwwo7qiLfzFZmjNmxjkiQlU= +github.com/cpuguy83/go-md2man/v2 v2.0.1 h1:r/myEWzV9lfsM1tFLgDyu0atFtJ1fXn261LKYj/3DxU= github.com/cpuguy83/go-md2man/v2 v2.0.1/go.mod h1:tgQtvFlXSQOSOSIRvRPT7W67SCa46tRHOmNcaadrF8o= github.com/creack/pty v1.1.9/go.mod h1:oKZEueFk5CKHvIhNR5MUki03XCEU+Q6VDXinZuGJ33E= github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= @@ -57,6 +58,7 @@ github.com/mitchellh/go-wordwrap v0.0.0-20150314170334-ad45545899c7 h1:DpOJ2HYzC github.com/mitchellh/go-wordwrap v0.0.0-20150314170334-ad45545899c7/go.mod h1:ZXFpozHsX6DPmq2I0TCekCxypsnAUbP2oI0UX1GXzOo= github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= +github.com/russross/blackfriday/v2 v2.1.0 h1:JIOH55/0cWyOuilr9/qlrm0BSXldqnqwMsf35Ld67mk= github.com/russross/blackfriday/v2 v2.1.0/go.mod h1:+Rmxgy9KzJVeS9/2gXHxylqXiyQDYRxCVz55jmeOWTM= github.com/sergi/go-diff v1.0.0 h1:Kpca3qRNrduNnOQeazBd0ysaKrUJiIuISHxogkT9RPQ= github.com/sergi/go-diff v1.0.0/go.mod h1:0CfEIISq7TuYL3j771MWULgwwjU+GofnZX9QAmXWZgo= @@ -107,6 +109,7 @@ google.golang.org/appengine v1.6.5/go.mod h1:8WjMMxjGQR8xUklV/ARdw2HLXBOI7O7uCID gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= gopkg.in/check.v1 v1.0.0-20180628173108-788fd7840127 h1:qIbj1fsPNlZgppZ+VLlY7N33q108Sa+fhmuc+sWQYwY= gopkg.in/check.v1 v1.0.0-20180628173108-788fd7840127/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= +gopkg.in/yaml.v2 v2.4.0 h1:D8xgwECY7CYvx+Y2n4sBz93Jn9JRvxdiyyo8CTfuKaY= gopkg.in/yaml.v2 v2.4.0/go.mod h1:RDklbk79AGWmwhnvt/jBztapEOGDOx6ZbXqjP6csGnQ= gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= gopkg.in/yaml.v3 v3.0.0-20210107192922-496545a6307b h1:h8qDotaEPuJATrMmW04NCwg7v22aHH28wwpauUhK9Oo= diff --git a/sql/internal/sqlx/sqlx.go b/sql/internal/sqlx/sqlx.go index a11e906b947..804b11298ec 100644 --- a/sql/internal/sqlx/sqlx.go +++ b/sql/internal/sqlx/sqlx.go @@ -430,3 +430,10 @@ func ExprLastIndex(expr string) int { } return -1 } + +// ReverseChanges reverses the order of the changes. +func ReverseChanges(c []schema.Change) { + for i, n := 0, len(c); i < n/2; i++ { + c[i], c[n-i-1] = c[n-i-1], c[i] + } +} diff --git a/sql/internal/sqlx/sqlx_test.go b/sql/internal/sqlx/sqlx_test.go index fe27f04fda1..8612e94b097 100644 --- a/sql/internal/sqlx/sqlx_test.go +++ b/sql/internal/sqlx/sqlx_test.go @@ -122,3 +122,61 @@ func TestExprLastIndex(t *testing.T) { }) } } + +func TestReverseChanges(t *testing.T) { + tests := []struct { + input []schema.Change + expect []schema.Change + }{ + { + input: []schema.Change{ + (*schema.AddColumn)(nil), + }, + expect: []schema.Change{ + (*schema.AddColumn)(nil), + }, + }, + { + input: []schema.Change{ + (*schema.AddColumn)(nil), + (*schema.DropColumn)(nil), + }, + expect: []schema.Change{ + (*schema.DropColumn)(nil), + (*schema.AddColumn)(nil), + }, + }, + { + input: []schema.Change{ + (*schema.AddColumn)(nil), + (*schema.ModifyColumn)(nil), + (*schema.DropColumn)(nil), + }, + expect: []schema.Change{ + (*schema.DropColumn)(nil), + (*schema.ModifyColumn)(nil), + (*schema.AddColumn)(nil), + }, + }, + { + input: []schema.Change{ + (*schema.AddColumn)(nil), + (*schema.ModifyColumn)(nil), + (*schema.DropColumn)(nil), + (*schema.ModifyColumn)(nil), + }, + expect: []schema.Change{ + (*schema.ModifyColumn)(nil), + (*schema.DropColumn)(nil), + (*schema.ModifyColumn)(nil), + (*schema.AddColumn)(nil), + }, + }, + } + for i, tt := range tests { + t.Run(strconv.Itoa(i), func(t *testing.T) { + ReverseChanges(tt.input) + require.Equal(t, tt.expect, tt.input) + }) + } +} diff --git a/sql/mysql/migrate.go b/sql/mysql/migrate.go index cb4157a164e..a249c17b5fa 100644 --- a/sql/mysql/migrate.go +++ b/sql/mysql/migrate.go @@ -444,6 +444,9 @@ func (s *state) alterTable(t *schema.Table, changes []schema.Change) error { Comment: fmt.Sprintf("modify %q table", t.Name), } if reversible { + // Changes should be reverted in + // a reversed order they were created. + sqlx.ReverseChanges(reverse) if change.Reverse, err = build(reverse); err != nil { return fmt.Errorf("reversd alter table %q: %v", t.Name, err) } diff --git a/sql/mysql/migrate_test.go b/sql/mysql/migrate_test.go index eca702ea698..a032f112d24 100644 --- a/sql/mysql/migrate_test.go +++ b/sql/mysql/migrate_test.go @@ -485,7 +485,42 @@ func TestPlanChanges(t *testing.T) { Changes: []*migrate.Change{ { Cmd: "ALTER TABLE `users` ADD COLUMN `name` varchar(255) NOT NULL, ADD INDEX `id_key` USING HASH (`id`) COMMENT \"comment\", ADD CONSTRAINT `id_nonzero` CHECK (id > 0) ENFORCED, AUTO_INCREMENT 1000", - Reverse: "ALTER TABLE `users` DROP COLUMN `name`, DROP INDEX `id_key`, DROP CONSTRAINT `id_nonzero`, AUTO_INCREMENT 1", + Reverse: "ALTER TABLE `users` AUTO_INCREMENT 1, DROP CONSTRAINT `id_nonzero`, DROP INDEX `id_key`, DROP COLUMN `name`", + }, + }, + }, + }, + { + changes: []schema.Change{ + func() schema.Change { + users := schema.NewTable("users"). + AddColumns(schema.NewIntColumn("id", "int")) + posts := schema.NewTable("posts"). + AddColumns( + schema.NewIntColumn("id", "int"), + schema.NewIntColumn("author_id", "int"), + ) + posts.AddForeignKeys( + schema.NewForeignKey("author"). + AddColumns(posts.Columns[1]). + SetRefTable(users). + AddRefColumns(users.Columns[0]), + ) + return &schema.ModifyTable{ + T: posts, + Changes: []schema.Change{ + &schema.AddColumn{C: posts.Columns[1]}, + &schema.AddForeignKey{F: posts.ForeignKeys[0]}, + }, + } + }(), + }, + wantPlan: &migrate.Plan{ + Reversible: true, + Changes: []*migrate.Change{ + { + Cmd: "ALTER TABLE `posts` ADD COLUMN `author_id` int NOT NULL, ADD CONSTRAINT `author` FOREIGN KEY (`author_id`) REFERENCES `users` (`id`)", + Reverse: "ALTER TABLE `posts` DROP FOREIGN KEY `author`, DROP COLUMN `author_id`", }, }, }, @@ -513,7 +548,7 @@ func TestPlanChanges(t *testing.T) { Changes: []*migrate.Change{ { Cmd: "ALTER TABLE `users` ADD COLUMN `c2` int AS (c1*2) NOT NULL, ADD COLUMN `c3` int AS (c1*c2) STORED NOT NULL", - Reverse: "ALTER TABLE `users` DROP COLUMN `c2`, DROP COLUMN `c3`", + Reverse: "ALTER TABLE `users` DROP COLUMN `c3`, DROP COLUMN `c2`", }, }, }, @@ -604,7 +639,7 @@ func TestPlanChanges(t *testing.T) { Changes: []*migrate.Change{ { Cmd: "ALTER TABLE `users` ALTER CHECK `check1` ENFORCED, ALTER CHECK `check2` NOT ENFORCED, DROP CHECK `check3`, ADD CONSTRAINT `check3` CHECK (id >= 0)", - Reverse: "ALTER TABLE `users` ALTER CHECK `check1` NOT ENFORCED, ALTER CHECK `check2` ENFORCED, DROP CHECK `check3`, ADD CONSTRAINT `check3` CHECK (id > 0)", + Reverse: "ALTER TABLE `users` DROP CHECK `check3`, ADD CONSTRAINT `check3` CHECK (id > 0), ALTER CHECK `check2` ENFORCED, ALTER CHECK `check1` NOT ENFORCED", }, }, }, diff --git a/sql/postgres/migrate.go b/sql/postgres/migrate.go index 58110911df0..c9fe7f5b44f 100644 --- a/sql/postgres/migrate.go +++ b/sql/postgres/migrate.go @@ -396,6 +396,9 @@ func (s *state) alterTable(t *schema.Table, changes []schema.Change) error { Comment: fmt.Sprintf("modify %q table", t.Name), } if reversible { + // Changes should be reverted in + // a reversed order they were created. + sqlx.ReverseChanges(reverse) if change.Reverse, err = build(reverse); err != nil { return fmt.Errorf("reversd alter table %q: %v", t.Name, err) } diff --git a/sql/postgres/migrate_test.go b/sql/postgres/migrate_test.go index a042c605c4c..45ed4adb3d4 100644 --- a/sql/postgres/migrate_test.go +++ b/sql/postgres/migrate_test.go @@ -354,7 +354,7 @@ func TestPlanChanges(t *testing.T) { Changes: []*migrate.Change{ { Cmd: `ALTER TABLE "users" ADD COLUMN "name" character varying(255) NOT NULL DEFAULT 'logged_in', ADD COLUMN "last" character varying(255) NOT NULL DEFAULT 'logged_in', ADD CONSTRAINT "name_not_empty" CHECK ("name" <> ''), DROP CONSTRAINT "id_nonzero", DROP CONSTRAINT "id_iseven", ADD CONSTRAINT "id_iseven" CHECK (("id") % 2 = 0)`, - Reverse: `ALTER TABLE "users" DROP COLUMN "name", DROP COLUMN "last", DROP CONSTRAINT "name_not_empty", ADD CONSTRAINT "id_nonzero" CHECK ("id" <> 0), DROP CONSTRAINT "id_iseven", ADD CONSTRAINT "id_iseven" CHECK ("id" % 2 = 0)`, + Reverse: `ALTER TABLE "users" DROP CONSTRAINT "id_iseven", ADD CONSTRAINT "id_iseven" CHECK ("id" % 2 = 0), ADD CONSTRAINT "id_nonzero" CHECK ("id" <> 0), DROP CONSTRAINT "name_not_empty", DROP COLUMN "last", DROP COLUMN "name"`, }, { Cmd: `CREATE INDEX "id_key" ON "users" ("id" DESC) WHERE success`, @@ -409,7 +409,7 @@ func TestPlanChanges(t *testing.T) { Changes: []*migrate.Change{ { Cmd: `ALTER TABLE "users" DROP COLUMN "name", ALTER COLUMN "id" SET GENERATED BY DEFAULT SET START WITH 1024 SET INCREMENT BY 1 RESTART`, - Reverse: `ALTER TABLE "users" ADD COLUMN "name" character varying NOT NULL, ALTER COLUMN "id" SET GENERATED BY DEFAULT SET START WITH 1 SET INCREMENT BY 1 RESTART`, + Reverse: `ALTER TABLE "users" ALTER COLUMN "id" SET GENERATED BY DEFAULT SET START WITH 1 SET INCREMENT BY 1 RESTART, ADD COLUMN "name" character varying NOT NULL`, }, }, }, @@ -680,8 +680,10 @@ func TestPlanChanges(t *testing.T) { Reversible: true, Transactional: true, Changes: []*migrate.Change{ - {Cmd: `ALTER TABLE "users" ALTER COLUMN "one" DROP DEFAULT, ALTER COLUMN "two" DROP DEFAULT, ALTER COLUMN "id" SET GENERATED BY DEFAULT SET START WITH 1024 SET INCREMENT BY 1 RESTART`, - Reverse: `ALTER TABLE "users" ALTER COLUMN "one" SET DEFAULT 'one', ALTER COLUMN "two" SET DEFAULT 'two', ALTER COLUMN "id" SET GENERATED BY DEFAULT SET START WITH 1 SET INCREMENT BY 1 RESTART`}, + { + Cmd: `ALTER TABLE "users" ALTER COLUMN "one" DROP DEFAULT, ALTER COLUMN "two" DROP DEFAULT, ALTER COLUMN "id" SET GENERATED BY DEFAULT SET START WITH 1024 SET INCREMENT BY 1 RESTART`, + Reverse: `ALTER TABLE "users" ALTER COLUMN "id" SET GENERATED BY DEFAULT SET START WITH 1 SET INCREMENT BY 1 RESTART, ALTER COLUMN "two" SET DEFAULT 'two', ALTER COLUMN "one" SET DEFAULT 'one'`, + }, }, }, },