Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

schemadiff: remove table name from auto-generated FK constraint name #14385

Merged
merged 4 commits into from
Oct 29, 2023
Merged

schemadiff: remove table name from auto-generated FK constraint name #14385

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4fcf5ed
schemadiff: remove table name from FK constraint name
shlomi-noach Oct 28, 2023
6369661
remove earlier '_fk' indicator; it was wrong to use it, as MySQL uses…
shlomi-noach Oct 29, 2023
f8d5754
adapt tests
shlomi-noach Oct 29, 2023
800232f
adapt tests
shlomi-noach Oct 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions go/vt/schemadiff/names.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,9 @@ func ExtractConstraintOriginalName(tableName string, constraintName string) stri
if strings.HasPrefix(constraintName, fmt.Sprintf("%s_fk_", tableName)) {
return constraintName[len(tableName)+1:]
}
if strings.HasPrefix(constraintName, fmt.Sprintf("%s_ibfk_", tableName)) {
return constraintName[len(tableName)+1:]
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need the case above this one still? Since that’s not what the automatic names look like?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed.

if submatch := constraintVitessNameRegexp.FindStringSubmatch(constraintName); len(submatch) > 0 {
return submatch[1]
}
Expand Down
28 changes: 22 additions & 6 deletions go/vt/schemadiff/names_test.go
View file
Open in desktop
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)
})
}
}
})
}
60 changes: 48 additions & 12 deletions go/vt/vttablet/onlineddl/executor_test.go
View file
Open in desktop
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 @@ -62,8 +63,7 @@ func TestValidateAndEditCreateTableStatement(t *testing.T) {
constraint test_fk 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 @@ -76,8 +76,25 @@ func TestValidateAndEditCreateTableStatement(t *testing.T) {
constraint test_fk 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_fk": "test_fk_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 @@ -122,6 +146,11 @@ func TestValidateAndEditCreateTableStatement(t *testing.T) {
`,
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_fk": "test_fk_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,6 +232,12 @@ 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"},
},
{
// 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_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"},
},
Expand Down
Loading