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.

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

schemadiff: validate() table structure at the end of apply() #10189

Merged
22 changes: 22 additions & 0 deletions go/vt/schemadiff/table.go
Original file line number Diff line number Diff line change
Expand Up @@ -1021,6 +1021,9 @@ func (c *CreateTableEntity) apply(diff *AlterTableEntityDiff) error {
return err
}
}
if err := c.validate(); err != nil {
return err
}
return nil
}

Expand Down Expand Up @@ -1058,3 +1061,22 @@ func (c *CreateTableEntity) Apply(diff EntityDiff) (Entity, error) {
}
return dup, nil
}

// validate checks that the table structure is valid:
// - all columns referenced by keys exist
func (c *CreateTableEntity) validate() error {
// validate all columns referenced by indexes do in fact exist
columnExists := map[string]bool{}
for _, col := range c.CreateTable.TableSpec.Columns {
columnExists[col.Name.String()] = true
}
for _, key := range c.CreateTable.TableSpec.Indexes {
for _, col := range key.Columns {
colName := col.Column.String()
if !columnExists[colName] {
return errors.Wrapf(ErrInvalidColumnInKey, "key: %v, column: %v", key.Info.Name.String(), colName)
}
}
}
return nil
}
102 changes: 102 additions & 0 deletions go/vt/schemadiff/table_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ limitations under the License.
package schemadiff

import (
"errors"
"testing"

"github.com/stretchr/testify/assert"
Expand Down Expand Up @@ -705,3 +706,104 @@ func TestCreateTableDiff(t *testing.T) {
})
}
}

func TestValidate(t *testing.T) {
tt := []struct {
name string
from string
alter string
expectErr error
}{
{
name: "add column",
from: "create table t (id int primary key)",
alter: "alter table t add column i int",
},
{
name: "add key",
from: "create table t (id int primary key, i int)",
alter: "alter table t add key i_idx(i)",
},
{
name: "add column and key",
from: "create table t (id int primary key)",
alter: "alter table t add column i int, add key i_idx(i)",
},
{
name: "add key, missing column",
from: "create table t (id int primary key, i int)",
alter: "alter table t add key j_idx(j)",
expectErr: ErrInvalidColumnInKey,
},
{
name: "add key, missing column 2",
from: "create table t (id int primary key, i int)",
alter: "alter table t add key j_idx(j, i)",
expectErr: ErrInvalidColumnInKey,
},
{
name: "drop column, ok",
from: "create table t (id int primary key, i int, i2 int, key i_idx(i))",
alter: "alter table t drop column i2",
},
{
name: "drop column, break key",
from: "create table t (id int primary key, i int, key i_idx(i))",
alter: "alter table t drop column i",
expectErr: ErrInvalidColumnInKey,
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Right now this makes sense as a safeguard until we improve the drop column logic to also drop references from the index to mimic MySQL behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we choose to go that path then I'll update this PR; no need to postpone that to a future PR.

{
name: "drop column, break key 2",
from: "create table t (id int primary key, i int, i2 int, key i_idx(i, i2))",
alter: "alter table t drop column i",
expectErr: ErrInvalidColumnInKey,
},
{
name: "drop column, break key 3",
from: "create table t (id int primary key, i int, i2 int, key i_idx(i, i2))",
alter: "alter table t drop column i2",
expectErr: ErrInvalidColumnInKey,
},
{
name: "drop column, break key 4",
from: "create table t (id int primary key, i int, i2 int, key some_key(id, i), key i_idx(i, i2))",
alter: "alter table t drop column i2",
expectErr: ErrInvalidColumnInKey,
},
{
name: "add multiple keys, multi columns, ok",
from: "create table t (id int primary key, i1 int, i2 int, i3 int)",
alter: "alter table t add key i12_idx(i1, i2), add key i32_idx(i3, i2), add key i21_idx(i2, i1)",
},
{
name: "add multiple keys, multi columns, missing column",
from: "create table t (id int primary key, i1 int, i2 int, i4 int)",
alter: "alter table t add key i12_idx(i1, i2), add key i32_idx(i3, i2), add key i21_idx(i2, i1)",
expectErr: ErrInvalidColumnInKey,
},
}
for _, ts := range tt {
t.Run(ts.name, func(t *testing.T) {
stmt, err := sqlparser.Parse(ts.from)
require.NoError(t, err)
fromCreateTable, ok := stmt.(*sqlparser.CreateTable)
require.True(t, ok)

stmt, err = sqlparser.Parse(ts.alter)
require.NoError(t, err)
alterTable, ok := stmt.(*sqlparser.AlterTable)
require.True(t, ok)

c := NewCreateTableEntity(fromCreateTable)
a := &AlterTableEntityDiff{from: c, alterTable: alterTable}
applied, err := c.Apply(a)
if ts.expectErr != nil {
assert.Error(t, err)
assert.True(t, errors.Is(err, ts.expectErr))
} else {
assert.NoError(t, err)
assert.NotNil(t, applied)
}
})
}
}
2 changes: 2 additions & 0 deletions go/vt/schemadiff/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@ var (
ErrApplyDuplicateKey = errors.New("duplicate key")
ErrApplyDuplicateColumn = errors.New("duplicate column")
ErrApplyDuplicateConstraint = errors.New("duplicate constraint")

ErrInvalidColumnInKey = errors.New("invalid column referenced by key")
)

// Entity stands for a database object we can diff:
Expand Down