schemadiff: remove ForeignKeyLoopError and loop detection logic

Signed-off-by: Shlomi Noach <[email protected]>
vitessio · Mar 18, 2024 · 5c93fff · 5c93fff
1 parent 51debbd
commit 5c93fff
Show file tree

Hide file tree

Showing 3 changed files with 1 addition and 118 deletions.
diff --git 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
@@ -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
@@ -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{