Skip to content

Commit

Permalink
schemadiff: remove table name from auto-generated FK constraint name (#…
Browse files Browse the repository at this point in the history 
…14385)

Signed-off-by: Shlomi Noach <[email protected]>
  • Loading branch information
@shlomi-noach
shlomi-noach authored Oct 29, 2023
1 parent 54f2daf commit dde1939
Show file tree
Hide file tree
Showing 3 changed files with 81 additions and 29 deletions.
2 changes: 1 addition & 1 deletion go/vt/schemadiff/names.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
28 changes: 22 additions & 6 deletions go/vt/schemadiff/names_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import (
)

func TestConstraintOriginalName(t *testing.T) {
{
t.Run("check", func(t *testing.T) {
names := []string{
"check1",
"check1_7no794p1x6zw6je1gfqmt7bca",
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -70,5 +86,5 @@ func TestConstraintOriginalName(t *testing.T) {
assert.Equal(t, name, originalName)
})
}
}
})
}
80 changes: 58 additions & 22 deletions go/vt/vttablet/onlineddl/executor_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -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 {
Expand All @@ -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) {
Expand Down Expand Up @@ -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 {
Expand Down

0 comments on commit dde1939

Please sign in to comment.