From dde19397a4b574ba94844d50e547887ec69f4431 Mon Sep 17 00:00:00 2001 From: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Date: Sun, 29 Oct 2023 16:31:57 +0200 Subject: [PATCH] schemadiff: remove table name from auto-generated FK constraint name (#14385) Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --- go/vt/schemadiff/names.go | 2 +- go/vt/schemadiff/names_test.go | 28 ++++++-- go/vt/vttablet/onlineddl/executor_test.go | 80 ++++++++++++++++------- 3 files changed, 81 insertions(+), 29 deletions(-) diff --git a/go/vt/schemadiff/names.go b/go/vt/schemadiff/names.go index 57d1d20e5c7..c0878d22eeb 100644 --- a/go/vt/schemadiff/names.go +++ b/go/vt/schemadiff/names.go @@ -39,7 +39,7 @@ func ExtractConstraintOriginalName(tableName string, constraintName string) stri if strings.HasPrefix(constraintName, fmt.Sprintf("%s_chk_", tableName)) { return constraintName[len(tableName)+1:] } - if strings.HasPrefix(constraintName, fmt.Sprintf("%s_fk_", tableName)) { + if strings.HasPrefix(constraintName, fmt.Sprintf("%s_ibfk_", tableName)) { return constraintName[len(tableName)+1:] } if submatch := constraintVitessNameRegexp.FindStringSubmatch(constraintName); len(submatch) > 0 { diff --git a/go/vt/schemadiff/names_test.go b/go/vt/schemadiff/names_test.go index f54f037ceb4..f6bb0f8c184 100644 --- a/go/vt/schemadiff/names_test.go +++ b/go/vt/schemadiff/names_test.go @@ -23,7 +23,7 @@ import ( ) func TestConstraintOriginalName(t *testing.T) { - { + t.Run("check", func(t *testing.T) { names := []string{ "check1", "check1_7no794p1x6zw6je1gfqmt7bca", @@ -36,8 +36,24 @@ func TestConstraintOriginalName(t *testing.T) { assert.Equal(t, "check1", originalName) }) } - } - { + }) + t.Run("ibfk", func(t *testing.T) { + names := []string{ + "ibfk_1", + "ibfk_1_7no794p1x6zw6je1gfqmt7bca", + "ibfk_1_etne0g9fvf3la2myjfsdgx9bx", + "mytable_ibfk_1", + } + for _, name := range names { + t.Run(name, func(t *testing.T) { + originalName := ExtractConstraintOriginalName("mytable", name) + assert.NotEmpty(t, originalName) + assert.Equal(t, "ibfk_1", originalName) + }) + } + }) + + t.Run("chk", func(t *testing.T) { names := []string{ "chk_1", "chk_1_7no794p1x6zw6je1gfqmt7bca", @@ -51,9 +67,9 @@ func TestConstraintOriginalName(t *testing.T) { assert.Equal(t, "chk_1", originalName) }) } - } + }) - { + t.Run("no change", func(t *testing.T) { names := []string{ "check1", "check_991ek3m5g69vcule23s9vnayd_check1", @@ -70,5 +86,5 @@ func TestConstraintOriginalName(t *testing.T) { assert.Equal(t, name, originalName) }) } - } + }) } diff --git a/go/vt/vttablet/onlineddl/executor_test.go b/go/vt/vttablet/onlineddl/executor_test.go index 691891c4af7..0da2b5b802e 100644 --- a/go/vt/vttablet/onlineddl/executor_test.go +++ b/go/vt/vttablet/onlineddl/executor_test.go @@ -45,11 +45,12 @@ func TestGetConstraintType(t *testing.T) { func TestValidateAndEditCreateTableStatement(t *testing.T) { e := Executor{} tt := []struct { - name string - query string - strategyOptions string - expectError string - countConstraints int + name string + query string + strategyOptions string + expectError string + countConstraints int + expectConstraintMap map[string]string }{ { name: "table with FK, not allowed", @@ -59,11 +60,10 @@ func TestValidateAndEditCreateTableStatement(t *testing.T) { i int not null, parent_id int not null, primary key(id), - constraint test_fk foreign key (parent_id) references onlineddl_test_parent (id) on delete no action + constraint test_ibfk foreign key (parent_id) references onlineddl_test_parent (id) on delete no action ) `, - countConstraints: 1, - expectError: schema.ErrForeignKeyFound.Error(), + expectError: schema.ErrForeignKeyFound.Error(), }, { name: "table with FK, allowed", @@ -73,11 +73,28 @@ func TestValidateAndEditCreateTableStatement(t *testing.T) { i int not null, parent_id int not null, primary key(id), - constraint test_fk foreign key (parent_id) references onlineddl_test_parent (id) on delete no action + constraint test_ibfk foreign key (parent_id) references onlineddl_test_parent (id) on delete no action ) `, + strategyOptions: "--unsafe-allow-foreign-keys", + countConstraints: 1, + expectConstraintMap: map[string]string{"test_ibfk": "test_ibfk_2wtivm6zk4lthpz14g9uoyaqk"}, + }, + { + name: "table with default FK name, strip table name", + query: ` + create table onlineddl_test ( + id int auto_increment, + i int not null, + parent_id int not null, + primary key(id), + constraint onlineddl_test_ibfk_1 foreign key (parent_id) references onlineddl_test_parent (id) on delete no action + ) + `, strategyOptions: "--unsafe-allow-foreign-keys", countConstraints: 1, + // we want 'onlineddl_test_' to be stripped out: + expectConstraintMap: map[string]string{"onlineddl_test_ibfk_1": "ibfk_1_2wtivm6zk4lthpz14g9uoyaqk"}, }, { name: "table with anonymous FK, allowed", @@ -90,8 +107,9 @@ func TestValidateAndEditCreateTableStatement(t *testing.T) { foreign key (parent_id) references onlineddl_test_parent (id) on delete no action ) `, - strategyOptions: "--unsafe-allow-foreign-keys", - countConstraints: 1, + strategyOptions: "--unsafe-allow-foreign-keys", + countConstraints: 1, + expectConstraintMap: map[string]string{"": "fk_2wtivm6zk4lthpz14g9uoyaqk"}, }, { name: "table with CHECK constraints", @@ -107,6 +125,12 @@ func TestValidateAndEditCreateTableStatement(t *testing.T) { ) `, countConstraints: 4, + expectConstraintMap: map[string]string{ + "check_1": "check_1_7dbssrkwdaxhdunwi5zj53q83", + "check_2": "check_2_ehg3rtk6ejvbxpucimeess30o", + "check_3": "check_3_0se0t8x98mf8v7lqmj2la8j9u", + "chk_1111033c1d2d5908bf1f956ba900b192_check_4": "chk_1111033c1d2d5908bf1f956ba900b192_c_0c2c3bxi9jp4evqrct44wg3xh", + }, }, { name: "table with both FOREIGN and CHECK constraints", @@ -116,12 +140,17 @@ func TestValidateAndEditCreateTableStatement(t *testing.T) { i int not null, primary key(id), constraint check_1 CHECK ((i >= 0)), - constraint test_fk foreign key (parent_id) references onlineddl_test_parent (id) on delete no action, + constraint test_ibfk foreign key (parent_id) references onlineddl_test_parent (id) on delete no action, constraint chk_1111033c1d2d5908bf1f956ba900b192_check_4 CHECK ((i >= 0)) ) `, strategyOptions: "--unsafe-allow-foreign-keys", countConstraints: 3, + expectConstraintMap: map[string]string{ + "check_1": "check_1_7dbssrkwdaxhdunwi5zj53q83", + "chk_1111033c1d2d5908bf1f956ba900b192_check_4": "chk_1111033c1d2d5908bf1f956ba900b192_c_0se0t8x98mf8v7lqmj2la8j9u", + "test_ibfk": "test_ibfk_2wtivm6zk4lthpz14g9uoyaqk", + }, }, } for _, tc := range tt { @@ -134,11 +163,12 @@ func TestValidateAndEditCreateTableStatement(t *testing.T) { onlineDDL := &schema.OnlineDDL{UUID: "a5a563da_dc1a_11ec_a416_0a43f95f28a3", Table: "onlineddl_test", Options: tc.strategyOptions} constraintMap, err := e.validateAndEditCreateTableStatement(context.Background(), onlineDDL, createTable) if tc.expectError != "" { - require.Error(t, err) assert.ErrorContains(t, err, tc.expectError) - } else { - assert.NoError(t, err) + return } + assert.NoError(t, err) + assert.Equal(t, tc.expectConstraintMap, constraintMap) + uniqueConstraintNames := map[string]bool{} err = sqlparser.Walk(func(node sqlparser.SQLNode) (kontinue bool, err error) { switch node := node.(type) { @@ -202,19 +232,25 @@ func TestValidateAndEditAlterTableStatement(t *testing.T) { expect: []string{"alter table t add constraint myfk_6fmhzdlya89128u5j3xapq34i foreign key (parent_id) references onlineddl_test_parent (id) on delete no action, algorithm = copy"}, }, { - alter: "alter table t add constraint t_fk_1 foreign key (parent_id) references onlineddl_test_parent (id) on delete no action", - expect: []string{"alter table t add constraint fk_1_6fmhzdlya89128u5j3xapq34i foreign key (parent_id) references onlineddl_test_parent (id) on delete no action, algorithm = copy"}, + // strip out table name + alter: "alter table t add constraint t_ibfk_1 foreign key (parent_id) references onlineddl_test_parent (id) on delete no action", + expect: []string{"alter table t add constraint ibfk_1_6fmhzdlya89128u5j3xapq34i foreign key (parent_id) references onlineddl_test_parent (id) on delete no action, algorithm = copy"}, + }, + { + // stript out table name + alter: "alter table t add constraint t_ibfk_1 foreign key (parent_id) references onlineddl_test_parent (id) on delete no action", + expect: []string{"alter table t add constraint ibfk_1_6fmhzdlya89128u5j3xapq34i foreign key (parent_id) references onlineddl_test_parent (id) on delete no action, algorithm = copy"}, }, { - alter: "alter table t add constraint t_fk_1 foreign key (parent_id) references onlineddl_test_parent (id) on delete no action, add constraint some_check check (id != 1)", - expect: []string{"alter table t add constraint fk_1_6fmhzdlya89128u5j3xapq34i foreign key (parent_id) references onlineddl_test_parent (id) on delete no action, add constraint some_check_aulpn7bjeortljhguy86phdn9 check (id != 1), algorithm = copy"}, + alter: "alter table t add constraint t_ibfk_1 foreign key (parent_id) references onlineddl_test_parent (id) on delete no action, add constraint some_check check (id != 1)", + expect: []string{"alter table t add constraint ibfk_1_6fmhzdlya89128u5j3xapq34i foreign key (parent_id) references onlineddl_test_parent (id) on delete no action, add constraint some_check_aulpn7bjeortljhguy86phdn9 check (id != 1), algorithm = copy"}, }, { - alter: "alter table t drop foreign key t_fk_1", + alter: "alter table t drop foreign key t_ibfk_1", m: map[string]string{ - "t_fk_1": "fk_1_aaaaaaaaaaaaaa", + "t_ibfk_1": "ibfk_1_aaaaaaaaaaaaaa", }, - expect: []string{"alter table t drop foreign key fk_1_aaaaaaaaaaaaaa, algorithm = copy"}, + expect: []string{"alter table t drop foreign key ibfk_1_aaaaaaaaaaaaaa, algorithm = copy"}, }, } for _, tc := range tt {