From 3027ec8fe5e1f2972adfa0bcbdfcaddc1e0f0eaf Mon Sep 17 00:00:00 2001 From: Ryan Slade Date: Tue, 10 Dec 2024 15:41:21 +0100 Subject: [PATCH 1/6] Convert DROP INDEX SQL to pgroll operation --- pkg/sql2pgroll/convert.go | 2 + pkg/sql2pgroll/drop.go | 45 ++++++++++++++++++++ pkg/sql2pgroll/drop_test.go | 64 +++++++++++++++++++++++++++++ pkg/sql2pgroll/expect/drop_index.go | 11 +++++ 4 files changed, 122 insertions(+) create mode 100644 pkg/sql2pgroll/drop.go create mode 100644 pkg/sql2pgroll/drop_test.go create mode 100644 pkg/sql2pgroll/expect/drop_index.go diff --git a/pkg/sql2pgroll/convert.go b/pkg/sql2pgroll/convert.go index 40ca02641..69d812a57 100644 --- a/pkg/sql2pgroll/convert.go +++ b/pkg/sql2pgroll/convert.go @@ -45,6 +45,8 @@ func convert(sql string) (migrations.Operations, error) { return convertAlterTableStmt(node.AlterTableStmt) case *pgq.Node_RenameStmt: return convertRenameStmt(node.RenameStmt) + case *pgq.Node_DropStmt: + return convertDropStatement(node.DropStmt) default: return makeRawSQLOperation(sql), nil } diff --git a/pkg/sql2pgroll/drop.go b/pkg/sql2pgroll/drop.go new file mode 100644 index 000000000..7def22f27 --- /dev/null +++ b/pkg/sql2pgroll/drop.go @@ -0,0 +1,45 @@ +// SPDX-License-Identifier: Apache-2.0 + +package sql2pgroll + +import ( + pgq "github.com/pganalyze/pg_query_go/v6" + + "github.com/xataio/pgroll/pkg/migrations" +) + +// convertDropStatement converts supported drop statements to pgroll operations +func convertDropStatement(stmt *pgq.DropStmt) (migrations.Operations, error) { + switch stmt.RemoveType { + case pgq.ObjectType_OBJECT_INDEX: + return convertDropIndexStatement(stmt) + } + return nil, nil +} + +// convertDropIndexStatement converts simple DROP INDEX statements to pgroll operations +func convertDropIndexStatement(stmt *pgq.DropStmt) (migrations.Operations, error) { + if !canConvertDropIndex(stmt) { + return nil, nil + } + s := stmt.Objects[0].GetList().Items[0].GetString_().GetSval() + return migrations.Operations{ + &migrations.OpDropIndex{ + Name: s, + }, + }, nil +} + +// canConvertDropIndex checks whether we can convert the statement without losing any information. +func canConvertDropIndex(stmt *pgq.DropStmt) bool { + if len(stmt.Objects) > 1 { + return false + } + if stmt.MissingOk || stmt.Concurrent { + return false + } + if stmt.Behavior == pgq.DropBehavior_DROP_CASCADE { + return false + } + return true +} diff --git a/pkg/sql2pgroll/drop_test.go b/pkg/sql2pgroll/drop_test.go new file mode 100644 index 000000000..b4c2f744d --- /dev/null +++ b/pkg/sql2pgroll/drop_test.go @@ -0,0 +1,64 @@ +// SPDX-License-Identifier: Apache-2.0 + +package sql2pgroll_test + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/xataio/pgroll/pkg/migrations" + "github.com/xataio/pgroll/pkg/sql2pgroll" + "github.com/xataio/pgroll/pkg/sql2pgroll/expect" +) + +func TestDropIndexStatements(t *testing.T) { + t.Parallel() + + tests := []struct { + sql string + expectedOp migrations.Operation + }{ + { + sql: "DROP INDEX foo", + expectedOp: expect.DropIndexOp1, + }, + { + sql: "DROP INDEX foo RESTRICT", + expectedOp: expect.DropIndexOp1, + }, + } + + for _, tc := range tests { + t.Run(tc.sql, func(t *testing.T) { + ops, err := sql2pgroll.Convert(tc.sql) + require.NoError(t, err) + + require.Len(t, ops, 1) + + assert.Equal(t, tc.expectedOp, ops[0]) + }) + } +} + +func TestUnconvertableDropIndexStatements(t *testing.T) { + t.Parallel() + + tests := []string{ + "DROP INDEX CONCURRENTLY foo", + "DROP INDEX IF EXISTS foo", + "DROP INDEX foo CASCADE", + } + + for _, sql := range tests { + t.Run(sql, func(t *testing.T) { + ops, err := sql2pgroll.Convert(sql) + require.NoError(t, err) + + require.Len(t, ops, 1) + + assert.Equal(t, expect.RawSQLOp(sql), ops[0]) + }) + } +} diff --git a/pkg/sql2pgroll/expect/drop_index.go b/pkg/sql2pgroll/expect/drop_index.go new file mode 100644 index 000000000..e621712cd --- /dev/null +++ b/pkg/sql2pgroll/expect/drop_index.go @@ -0,0 +1,11 @@ +// SPDX-License-Identifier: Apache-2.0 + +package expect + +import ( + "github.com/xataio/pgroll/pkg/migrations" +) + +var DropIndexOp1 = &migrations.OpDropIndex{ + Name: "foo", +} From c681207298e2438b81ecca26e78ec3aa29a8e603 Mon Sep 17 00:00:00 2001 From: Ryan Slade Date: Wed, 11 Dec 2024 10:25:22 +0100 Subject: [PATCH 2/6] Make the linter happy --- pkg/sql2pgroll/drop.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/pkg/sql2pgroll/drop.go b/pkg/sql2pgroll/drop.go index 7def22f27..7bd66e8c9 100644 --- a/pkg/sql2pgroll/drop.go +++ b/pkg/sql2pgroll/drop.go @@ -10,8 +10,7 @@ import ( // convertDropStatement converts supported drop statements to pgroll operations func convertDropStatement(stmt *pgq.DropStmt) (migrations.Operations, error) { - switch stmt.RemoveType { - case pgq.ObjectType_OBJECT_INDEX: + if stmt.RemoveType == pgq.ObjectType_OBJECT_INDEX { return convertDropIndexStatement(stmt) } return nil, nil From 8c823e9d584e736795d70e64c0ede3b8546c32b7 Mon Sep 17 00:00:00 2001 From: Ryan Slade Date: Wed, 11 Dec 2024 10:33:33 +0100 Subject: [PATCH 3/6] Consistently use getters --- pkg/sql2pgroll/drop.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/sql2pgroll/drop.go b/pkg/sql2pgroll/drop.go index 7bd66e8c9..c30ef6139 100644 --- a/pkg/sql2pgroll/drop.go +++ b/pkg/sql2pgroll/drop.go @@ -21,7 +21,7 @@ func convertDropIndexStatement(stmt *pgq.DropStmt) (migrations.Operations, error if !canConvertDropIndex(stmt) { return nil, nil } - s := stmt.Objects[0].GetList().Items[0].GetString_().GetSval() + s := stmt.GetObjects()[0].GetList().GetItems()[0].GetString_().GetSval() return migrations.Operations{ &migrations.OpDropIndex{ Name: s, From 5021168ff3895d9c4a71b4a2ee807c4a056ae401 Mon Sep 17 00:00:00 2001 From: Ryan Slade Date: Wed, 11 Dec 2024 10:37:12 +0100 Subject: [PATCH 4/6] Support IF EXISTS and CONCURRENTLY --- pkg/sql2pgroll/drop.go | 3 --- pkg/sql2pgroll/drop_test.go | 9 +++++++-- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/pkg/sql2pgroll/drop.go b/pkg/sql2pgroll/drop.go index c30ef6139..5ff2e9e86 100644 --- a/pkg/sql2pgroll/drop.go +++ b/pkg/sql2pgroll/drop.go @@ -34,9 +34,6 @@ func canConvertDropIndex(stmt *pgq.DropStmt) bool { if len(stmt.Objects) > 1 { return false } - if stmt.MissingOk || stmt.Concurrent { - return false - } if stmt.Behavior == pgq.DropBehavior_DROP_CASCADE { return false } diff --git a/pkg/sql2pgroll/drop_test.go b/pkg/sql2pgroll/drop_test.go index b4c2f744d..581507bea 100644 --- a/pkg/sql2pgroll/drop_test.go +++ b/pkg/sql2pgroll/drop_test.go @@ -28,6 +28,13 @@ func TestDropIndexStatements(t *testing.T) { sql: "DROP INDEX foo RESTRICT", expectedOp: expect.DropIndexOp1, }, + { + sql: "DROP INDEX IF EXISTS foo", + expectedOp: expect.DropIndexOp1, + }, { + sql: "DROP INDEX CONCURRENTLY foo", + expectedOp: expect.DropIndexOp1, + }, } for _, tc := range tests { @@ -46,8 +53,6 @@ func TestUnconvertableDropIndexStatements(t *testing.T) { t.Parallel() tests := []string{ - "DROP INDEX CONCURRENTLY foo", - "DROP INDEX IF EXISTS foo", "DROP INDEX foo CASCADE", } From d10e72f808832c8da67acebb147408a8d8ee9993 Mon Sep 17 00:00:00 2001 From: Ryan Slade Date: Wed, 11 Dec 2024 10:38:54 +0100 Subject: [PATCH 5/6] Add failing test case --- pkg/sql2pgroll/drop_test.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/pkg/sql2pgroll/drop_test.go b/pkg/sql2pgroll/drop_test.go index 581507bea..d842c8a47 100644 --- a/pkg/sql2pgroll/drop_test.go +++ b/pkg/sql2pgroll/drop_test.go @@ -24,6 +24,10 @@ func TestDropIndexStatements(t *testing.T) { sql: "DROP INDEX foo", expectedOp: expect.DropIndexOp1, }, + { + sql: "DROP INDEX myschema.foo", + expectedOp: expect.DropIndexOp1, + }, { sql: "DROP INDEX foo RESTRICT", expectedOp: expect.DropIndexOp1, @@ -31,7 +35,8 @@ func TestDropIndexStatements(t *testing.T) { { sql: "DROP INDEX IF EXISTS foo", expectedOp: expect.DropIndexOp1, - }, { + }, + { sql: "DROP INDEX CONCURRENTLY foo", expectedOp: expect.DropIndexOp1, }, From 3ceb3bd4f6259bedbfbda45008832d1111c49ff8 Mon Sep 17 00:00:00 2001 From: Ryan Slade Date: Wed, 11 Dec 2024 11:10:04 +0100 Subject: [PATCH 6/6] Handle index names with schemas --- pkg/sql2pgroll/drop.go | 11 +++++++++-- pkg/sql2pgroll/drop_test.go | 2 +- pkg/sql2pgroll/expect/drop_index.go | 4 ++++ 3 files changed, 14 insertions(+), 3 deletions(-) diff --git a/pkg/sql2pgroll/drop.go b/pkg/sql2pgroll/drop.go index 5ff2e9e86..ed6596b21 100644 --- a/pkg/sql2pgroll/drop.go +++ b/pkg/sql2pgroll/drop.go @@ -3,6 +3,8 @@ package sql2pgroll import ( + "strings" + pgq "github.com/pganalyze/pg_query_go/v6" "github.com/xataio/pgroll/pkg/migrations" @@ -21,10 +23,15 @@ func convertDropIndexStatement(stmt *pgq.DropStmt) (migrations.Operations, error if !canConvertDropIndex(stmt) { return nil, nil } - s := stmt.GetObjects()[0].GetList().GetItems()[0].GetString_().GetSval() + items := stmt.GetObjects()[0].GetList().GetItems() + parts := make([]string, len(items)) + for i, item := range items { + parts[i] = item.GetString_().GetSval() + } + return migrations.Operations{ &migrations.OpDropIndex{ - Name: s, + Name: strings.Join(parts, "."), }, }, nil } diff --git a/pkg/sql2pgroll/drop_test.go b/pkg/sql2pgroll/drop_test.go index d842c8a47..52c415eb1 100644 --- a/pkg/sql2pgroll/drop_test.go +++ b/pkg/sql2pgroll/drop_test.go @@ -26,7 +26,7 @@ func TestDropIndexStatements(t *testing.T) { }, { sql: "DROP INDEX myschema.foo", - expectedOp: expect.DropIndexOp1, + expectedOp: expect.DropIndexOp2, }, { sql: "DROP INDEX foo RESTRICT", diff --git a/pkg/sql2pgroll/expect/drop_index.go b/pkg/sql2pgroll/expect/drop_index.go index e621712cd..81d60e7b8 100644 --- a/pkg/sql2pgroll/expect/drop_index.go +++ b/pkg/sql2pgroll/expect/drop_index.go @@ -9,3 +9,7 @@ import ( var DropIndexOp1 = &migrations.OpDropIndex{ Name: "foo", } + +var DropIndexOp2 = &migrations.OpDropIndex{ + Name: "myschema.foo", +}