schemadiff: formalize SchemaDiff as the only schema diffing mechanism

go/vt/schemadiff/diff.go

@@ -160,7 +160,11 @@ func DiffSchemasSQL(sql1 string, sql2 string, hints *DiffHints) ([]EntityDiff, e
 	if err != nil {
 		return nil, err
 	}
-	return schema1.Diff(schema2, hints)
+	diff, err := schema1.SchemaDiff(schema2, hints)
+	if err != nil {
+		return nil, err
+	}
+	return diff.OrderedDiffs()
 }
 
 // DiffSchemasSQL compares two schemas and returns the list of diffs that turn
@@ -172,5 +176,9 @@ func DiffSchemas(schema1 *Schema, schema2 *Schema, hints *DiffHints) ([]EntityDi
 	if schema2 == nil {
 		schema2 = newEmptySchema()
 	}
-	return schema1.Diff(schema2, hints)
+	diff, err := schema1.SchemaDiff(schema2, hints)
+	if err != nil {
+		return nil, err
+	}
+	return diff.OrderedDiffs()
 }

go/vt/schemadiff/diff_test.go

@@ -752,16 +752,16 @@ func TestDiffSchemas(t *testing.T) {
 				"drop table t1",
 				"alter table t2 modify column id bigint",
 				"alter view v2 as select id from t2",
-				"create table t4 (\n\tid int,\n\tprimary key (id)\n)",
 				"create view v0 as select * from v2, t2",
+				"create table t4 (\n\tid int,\n\tprimary key (id)\n)",
 			},
 			cdiffs: []string{
 				"DROP VIEW `v1`",
 				"DROP TABLE `t1`",
 				"ALTER TABLE `t2` MODIFY COLUMN `id` bigint",
 				"ALTER VIEW `v2` AS SELECT `id` FROM `t2`",
-				"CREATE TABLE `t4` (\n\t`id` int,\n\tPRIMARY KEY (`id`)\n)",
 				"CREATE VIEW `v0` AS SELECT * FROM `v2`, `t2`",
+				"CREATE TABLE `t4` (\n\t`id` int,\n\tPRIMARY KEY (`id`)\n)",
 			},
 		},
 	}
@@ -816,9 +816,9 @@ func TestDiffSchemas(t *testing.T) {
 					// validate schema1 unaffected by Apply
 					assert.Equal(t, schema1SQL, schema1.ToSQL())
 
-					appliedDiff, err := schema2.Diff(applied, hints)
+					appliedDiff, err := schema2.SchemaDiff(applied, hints)
 					require.NoError(t, err)
-					assert.Empty(t, appliedDiff)
+					assert.True(t, appliedDiff.Empty())
 					assert.Equal(t, schema2.ToQueries(), applied.ToQueries())
 				}
 			}
@@ -867,7 +867,9 @@ func TestSchemaApplyError(t *testing.T) {
 			assert.NoError(t, err)
 
 			{
-				diffs, err := schema1.Diff(schema2, hints)
+				diff, err := schema1.SchemaDiff(schema2, hints)
+				require.NoError(t, err)
+				diffs, err := diff.OrderedDiffs()
 				assert.NoError(t, err)
 				assert.NotEmpty(t, diffs)
 				_, err = schema1.Apply(diffs)
@@ -876,7 +878,9 @@ func TestSchemaApplyError(t *testing.T) {
 				require.Error(t, err, "expected error applying to schema2. diffs: %v", diffs)
 			}
 			{
-				diffs, err := schema2.Diff(schema1, hints)
+				diff, err := schema2.SchemaDiff(schema1, hints)
+				require.NoError(t, err)
+				diffs, err := diff.OrderedDiffs()
 				assert.NoError(t, err)
 				assert.NotEmpty(t, diffs, "schema1: %v, schema2: %v", schema1.ToSQL(), schema2.ToSQL())
 				_, err = schema2.Apply(diffs)

go/vt/schemadiff/schema.go

@@ -445,7 +445,7 @@ func (s *Schema) ViewNames() []string {
 
 // Diff compares this schema with another schema, and sees what it takes to make this schema look
 // like the other. It returns a list of diffs.
-func (s *Schema) Diff(other *Schema, hints *DiffHints) (diffs []EntityDiff, err error) {
+func (s *Schema) diff(other *Schema, hints *DiffHints) (diffs []EntityDiff, err error) {
 	// dropped entities
 	var dropDiffs []EntityDiff
 	for _, e := range s.Entities() {
@@ -767,11 +767,13 @@ func (s *Schema) Apply(diffs []EntityDiff) (*Schema, error) {
 	return dup, nil
 }
 
-// SchemaDiff calulates a rich diff between this schema and the given schema. It is stronger than Diff().
-// On top of returning the list of diffs that can take this schema into the given schema, this function also
-// evaluates the dependencies between those diffs, if any.
+// SchemaDiff calulates a rich diff between this schema and the given schema. It builds on top of diff():
+// on top of the list of diffs that can take this schema into the given schema, this function also
+// evaluates the dependencies between those diffs, if any, and the resulting SchemaDiff object offers OrderedDiffs(),
+// the safe ordering of diffs that, when appleid sequentially, does not produce any conflicts and keeps schema valid
+// at each step.
 func (s *Schema) SchemaDiff(other *Schema, hints *DiffHints) (*SchemaDiff, error) {
-	diffs, err := s.Diff(other, hints)
+	diffs, err := s.diff(other, hints)
 	if err != nil {
 		return nil, err
 	}
@@ -793,7 +795,7 @@ func (s *Schema) SchemaDiff(other *Schema, hints *DiffHints) (*SchemaDiff, error
 		return dependentDiffs, relationsMade
 	}
 
-	for _, diff := range schemaDiff.allDiffs() {
+	for _, diff := range schemaDiff.UnorderedDiffs() {
 		switch diff := diff.(type) {
 		case *CreateViewEntityDiff:
 			checkDependencies(diff, getViewDependentTableNames(diff.createView))

go/vt/schemadiff/schema_diff.go

@@ -166,9 +166,16 @@ func (d *SchemaDiff) diffsByEntityName(name string) (diffs []EntityDiff) {
 	return diffs
 }
 
-// allDiffs returns all the diffs. These are not sorted by dependencies. These are basically
-// the original diffs, "flatteninf" any subsequent diffs they may have.
-func (d *SchemaDiff) allDiffs() []EntityDiff {
+// Empty returns 'true' when there are no diff entries
+func (d *SchemaDiff) Empty() bool {
+	return len(d.diffs) == 0
+}
+
+// UnorderedDiffs returns all the diffs. These are not sorted by dependencies. These are basically
+// the original diffs, but "flattening" any subsequent diffs they may have. as result:
+// - Diffs in the returned slice have no subsequent diffs
+// - The returned slice may be longer than the number of diffs supplied by loadDiffs()
+func (d *SchemaDiff) UnorderedDiffs() []EntityDiff {
 	return d.diffs
 }
 

go/vt/schemadiff/schema_diff_test.go

@@ -99,7 +99,7 @@ func TestPermutations(t *testing.T) {
 			schemaDiff, err := fromSchema.SchemaDiff(toSchema, hints)
 			require.NoError(t, err)
 
-			allDiffs := schemaDiff.allDiffs()[:]
+			allDiffs := schemaDiff.UnorderedDiffs()
 			require.Equal(t, tc.expectDiffs, len(allDiffs))
 
 			toSingleString := func(diffs []EntityDiff) string {
@@ -112,7 +112,7 @@ func TestPermutations(t *testing.T) {
 			t.Run("no early break", func(t *testing.T) {
 				iteration := 0
 				allPerms := map[string]bool{}
-				allDiffs := schemaDiff.allDiffs()
+				allDiffs := schemaDiff.UnorderedDiffs()
 				originalSingleString := toSingleString(allDiffs)
 				earlyBreak := permutateDiffs(allDiffs, func(pdiffs []EntityDiff) (earlyBreak bool) {
 					// cover all permutations
@@ -132,7 +132,7 @@ func TestPermutations(t *testing.T) {
 			})
 			t.Run("early break", func(t *testing.T) {
 				allPerms := map[string]bool{}
-				allDiffs := schemaDiff.allDiffs()[:]
+				allDiffs := schemaDiff.UnorderedDiffs()
 				originalSingleString := toSingleString(allDiffs)
 				earlyBreak := permutateDiffs(allDiffs, func(pdiffs []EntityDiff) (earlyBreak bool) {
 					// Single visit
@@ -667,7 +667,7 @@ func TestSchemaDiff(t *testing.T) {
 			schemaDiff, err := fromSchema.SchemaDiff(toSchema, hints)
 			require.NoError(t, err)
 
-			allDiffs := schemaDiff.allDiffs()
+			allDiffs := schemaDiff.UnorderedDiffs()
 			allDiffsStatements := []string{}
 			for _, diff := range allDiffs {
 				allDiffsStatements = append(allDiffsStatements, diff.CanonicalStatementString())