From 2ddc0e882033ac8fb48a8255a6f0f95ac2ea1d2c Mon Sep 17 00:00:00 2001 From: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Date: Mon, 18 Mar 2024 10:55:01 +0200 Subject: [PATCH] `schemadiff`: remove `ForeignKeyLoopError` and loop detection logic (#15507) Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --- go/vt/schemadiff/errors.go | 38 -------------------- go/vt/schemadiff/schema.go | 61 --------------------------------- go/vt/schemadiff/schema_test.go | 20 +---------- 3 files changed, 1 insertion(+), 118 deletions(-) diff --git a/go/vt/schemadiff/errors.go b/go/vt/schemadiff/errors.go index dc73acdb9a0..dac1e6ca31f 100644 --- a/go/vt/schemadiff/errors.go +++ b/go/vt/schemadiff/errors.go @@ -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 diff --git a/go/vt/schemadiff/schema.go b/go/vt/schemadiff/schema.go index 8081c6eaeea..efad76d9a33 100644 --- a/go/vt/schemadiff/schema.go +++ b/go/vt/schemadiff/schema.go @@ -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" ) @@ -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": diff --git a/go/vt/schemadiff/schema_test.go b/go/vt/schemadiff/schema_test.go index 19a1b95e186..4e3440830f4 100644 --- a/go/vt/schemadiff/schema_test.go +++ b/go/vt/schemadiff/schema_test.go @@ -17,7 +17,6 @@ limitations under the License. package schemadiff import ( - "errors" "fmt" "math/rand" "sort" @@ -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 @@ -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 @@ -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)", @@ -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{