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

schemadiff: remove ForeignKeyLoopError and loop detection logic #15507

Merged
merged 1 commit into from
Mar 18, 2024
Merged

schemadiff: remove ForeignKeyLoopError and loop detection logic #15507

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
38 changes: 0 additions & 38 deletions go/vt/schemadiff/errors.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,44 +286,6 @@ func (e *ForeignKeyDependencyUnresolvedError) Error() string {
sqlescape.EscapeID(e.Table))
}

type ForeignKeyLoopError struct {
Table string
Loop []*ForeignKeyTableColumns
}

func (e *ForeignKeyLoopError) Error() string {
tableIsInsideLoop := false

loop := e.Loop
// The tables in the loop could be e.g.:
// t1->t2->a->b->c->a
// In such case, the loop is a->b->c->a. The last item is always the head & tail of the loop.
// We want to distinguish between the case where the table is inside the loop and the case where it's outside,
// so we remove the prefix of the loop that doesn't participate in the actual cycle.
if len(loop) > 0 {
last := loop[len(loop)-1]
for i := range loop {
if loop[i].Table == last.Table {
loop = loop[i:]
break
}
}
}
escaped := make([]string, len(loop))
for i, fk := range loop {
escaped[i] = fk.Escaped()
if fk.Table == e.Table {
tableIsInsideLoop = true
}
}
if tableIsInsideLoop {
return fmt.Sprintf("table %s participates in a circular foreign key reference: %s",
sqlescape.EscapeID(e.Table), strings.Join(escaped, ", "))
}
return fmt.Sprintf("table %s references a circular foreign key reference: %s",
sqlescape.EscapeID(e.Table), strings.Join(escaped, ", "))
}

type ForeignKeyNonexistentReferencedTableError struct {
Table string
ReferencedTable string
Expand Down
61 changes: 0 additions & 61 deletions go/vt/schemadiff/schema.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,10 @@ package schemadiff

import (
"errors"
"slices"
"sort"
"strings"

"golang.org/x/exp/maps"

"vitess.io/vitess/go/mysql/capabilities"
"vitess.io/vitess/go/vt/graph"
"vitess.io/vitess/go/vt/sqlparser"
"vitess.io/vitess/go/vt/vtgate/semantics"
)
Expand Down Expand Up @@ -289,63 +285,6 @@ func (s *Schema) normalize(hints *DiffHints) error {
s.sorted = append(s.sorted, t)
dependencyLevels[t.Name()] = iterationLevel // all in same level
}

// Now, let's see if the loop is valid or invalid. For example:
// users.avatar_id -> avatars.id
// avatars.creator_id -> users.id
// is a valid loop, because even though the two tables reference each other, the loop ends in different columns.
type tableCol struct {
tableName sqlparser.TableName
colNames sqlparser.Columns
}
var tableColHash = func(tc tableCol) string {
res := sqlparser.String(tc.tableName)
for _, colName := range tc.colNames {
res += "|" + sqlparser.String(colName)
}
return res
}
var decodeTableColHash = func(hash string) *ForeignKeyTableColumns {
tokens := strings.Split(hash, "|")
return &ForeignKeyTableColumns{tokens[0], tokens[1:]}
}
g := graph.NewGraph[string]()
for _, table := range s.tables {
for _, cfk := range table.TableSpec.Constraints {
check, ok := cfk.Details.(*sqlparser.ForeignKeyDefinition)
if !ok {
// Not a foreign key
continue
}

parentVertex := tableCol{
tableName: check.ReferenceDefinition.ReferencedTable,
colNames: check.ReferenceDefinition.ReferencedColumns,
}
childVertex := tableCol{
tableName: table.Table,
colNames: check.Source,
}
g.AddEdge(tableColHash(parentVertex), tableColHash(childVertex))
}
}
cycles := g.GetCycles() // map of table name to cycle
// golang maps have undefined iteration order. For consistent output, we sort the keys.
vertices := maps.Keys(cycles)
slices.Sort(vertices)
for _, vertex := range vertices {
cycle := cycles[vertex]
if len(cycle) == 0 {
continue
}
cycleTables := make([]*ForeignKeyTableColumns, len(cycle))
for i := range cycle {
// Reduce tablename|colname(s) to just tablename
cycleTables[i] = decodeTableColHash(cycle[i])
}
tableName := cycleTables[0].Table
errs = errors.Join(errs, addEntityFkError(s.named[tableName], &ForeignKeyLoopError{Table: tableName, Loop: cycleTables}))
}
}

// We now iterate all views. We iterate "dependency levels":
Expand Down
20 changes: 1 addition & 19 deletions go/vt/schemadiff/schema_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ limitations under the License.
package schemadiff

import (
"errors"
"fmt"
"math/rand"
"sort"
Expand Down Expand Up @@ -356,10 +355,6 @@ func TestInvalidSchema(t *testing.T) {
create table t11 (id int primary key, i int, constraint f1101 foreign key (i) references t12 (i) on delete restrict);
create table t12 (id int primary key, i int, constraint f1201 foreign key (i) references t11 (i) on delete set null)
`,
expectErr: errors.Join(
&ForeignKeyLoopError{Table: "t11", Loop: []*ForeignKeyTableColumns{{"t11", []string{"i"}}, {"t12", []string{"i"}}, {"t11", []string{"i"}}}},
&ForeignKeyLoopError{Table: "t12", Loop: []*ForeignKeyTableColumns{{"t12", []string{"i"}}, {"t11", []string{"i"}}, {"t12", []string{"i"}}}},
),
},
{
// t10, t12<->t11
Expand Down Expand Up @@ -388,10 +383,6 @@ func TestInvalidSchema(t *testing.T) {
create table t12 (id int primary key, i int, constraint f1205 foreign key (id) references t11 (i) on delete restrict);
create table t13 (id int primary key, i int, constraint f1305 foreign key (i) references t11 (id) on delete restrict)
`,
expectErr: errors.Join(
&ForeignKeyLoopError{Table: "t11", Loop: []*ForeignKeyTableColumns{{"t11", []string{"i"}}, {"t12", []string{"id"}}, {"t11", []string{"i"}}}},
&ForeignKeyLoopError{Table: "t12", Loop: []*ForeignKeyTableColumns{{"t12", []string{"id"}}, {"t11", []string{"i"}}, {"t12", []string{"id"}}}},
),
},
{
// t10, t12<->t11<-t13<-t14
Expand All @@ -411,11 +402,6 @@ func TestInvalidSchema(t *testing.T) {
create table t12 (id int primary key, i int, key i_idx (i), constraint f1207 foreign key (id) references t13 (i));
create table t13 (id int primary key, i int, key i_idx (i), constraint f1307 foreign key (i) references t11 (i));
`,
expectErr: errors.Join(
&ForeignKeyLoopError{Table: "t11", Loop: []*ForeignKeyTableColumns{{"t11", []string{"i"}}, {"t13", []string{"i"}}, {"t12", []string{"id"}}, {"t11", []string{"i"}}}},
&ForeignKeyLoopError{Table: "t12", Loop: []*ForeignKeyTableColumns{{"t12", []string{"id"}}, {"t11", []string{"i"}}, {"t13", []string{"i"}}, {"t12", []string{"id"}}}},
&ForeignKeyLoopError{Table: "t13", Loop: []*ForeignKeyTableColumns{{"t13", []string{"i"}}, {"t12", []string{"id"}}, {"t11", []string{"i"}}, {"t13", []string{"i"}}}},
),
},
{
schema: "create table t11 (id int primary key, i int, key ix(i), constraint f11 foreign key (i) references t11(id2) on delete restrict)",
Expand Down Expand Up @@ -535,11 +521,7 @@ func TestInvalidTableForeignKeyReference(t *testing.T) {
"create table t12 (id int primary key, i int, constraint f13 foreign key (i) references t13(i) on delete restrict)",
}
_, err := NewSchemaFromQueries(NewTestEnv(), fkQueries)
assert.Error(t, err)

assert.ErrorContains(t, err, (&ForeignKeyLoopError{Table: "t11", Loop: []*ForeignKeyTableColumns{{"t11", []string{"i"}}, {"t13", []string{"i"}}, {"t12", []string{"i"}}, {"t11", []string{"i"}}}}).Error())
assert.ErrorContains(t, err, (&ForeignKeyLoopError{Table: "t12", Loop: []*ForeignKeyTableColumns{{"t12", []string{"i"}}, {"t11", []string{"i"}}, {"t13", []string{"i"}}, {"t12", []string{"i"}}}}).Error())
assert.ErrorContains(t, err, (&ForeignKeyLoopError{Table: "t13", Loop: []*ForeignKeyTableColumns{{"t13", []string{"i"}}, {"t12", []string{"i"}}, {"t11", []string{"i"}}, {"t13", []string{"i"}}}}).Error())
assert.NoError(t, err)
}
{
fkQueries := []string{
Expand Down
Loading