Repository: ariga/atlas
Pull request #239 (2021-11-17 20:02:57)

sql/internal/specutil: extract foreign-key ref table from spec


Patch:
diff --git a/sql/internal/specutil/convert.go b/sql/internal/specutil/convert.go
index 21c01442..116ad6fd 100644
--- a/sql/internal/specutil/convert.go
+++ b/sql/internal/specutil/convert.go
@@ -169,6 +169,18 @@ func linkForeignKeys(tbl *schema.Table, sch *schema.Schema, table *sqlspec.Table
 			}
 			fk.Columns = append(fk.Columns, col)
 		}
+		if len(spec.RefColumns) == 0 {
+			return fmt.Errorf("sqlspec: missing reference (parent) columns for foreign key: %q", spec.Symbol)
+		}
+		name, err := tableName(spec.RefColumns[0])
+		if err != nil {
+			return err
+		}
+		t, ok := sch.Table(name)
+		if !ok {
+			return fmt.Errorf("sqlspec: undefined table %q for foreign key: %q", name, spec.Symbol)
+		}
+		fk.RefTable = t
 		for _, ref := range spec.RefColumns {
 			col, err := resolveCol(ref, sch)
 			if err != nil {
diff --git a/sql/internal/sqlx/plan.go b/sql/internal/sqlx/plan.go
index cc2574aa..3d480cdc 100644
--- a/sql/internal/sqlx/plan.go
+++ b/sql/internal/sqlx/plan.go
@@ -5,6 +5,8 @@
 package sqlx
 
 import (
+	"errors"
+	"fmt"
 	"sort"
 
 	"ariga.io/atlas/sql/schema"
@@ -14,17 +16,20 @@ import (
 // references between changes if there is at least one circular
 // reference in the changeset. More explicitly, it postpones fks
 // creation, or deletes fks before deletes their tables.
-func DetachCycles(changes []schema.Change) []schema.Change {
-	sorted, hasCycle := sortMap(changes)
-	if hasCycle {
-		return detachReferences(changes)
+func DetachCycles(changes []schema.Change) ([]schema.Change, error) {
+	sorted, err := sortMap(changes)
+	if err == errCycle {
+		return detachReferences(changes), nil
+	}
+	if err != nil {
+		return nil, err
 	}
 	planned := make([]schema.Change, len(changes))
 	copy(planned, changes)
 	sort.Slice(planned, func(i, j int) bool {
 		return sorted[table(planned[i])] < sorted[table(planned[j])]
 	})
-	return planned
+	return planned, nil
 }
 
 // detachReferences detaches all table references.
@@ -83,16 +88,22 @@ func detachReferences(changes []schema.Change) []schema.Change {
 	return append(planned, deferred...)
 }
 
+// errCycle is an internal error to indicate a case of a cycle.
+var errCycle = errors.New("cycle detected")
+
 // sortMap returns an index-map indicates the position of table in a topological
 // sort in reversed order based on its references, and a boolean indicate if there
 // is a non-self loop.
-func sortMap(changes []schema.Change) (map[string]int, bool) {
+func sortMap(changes []schema.Change) (map[string]int, error) {
 	var (
-		visit    func(string) bool
-		deps     = dependencies(changes)
-		sorted   = make(map[string]int)
-		progress = make(map[string]bool)
+		visit     func(string) bool
+		sorted    = make(map[string]int)
+		progress  = make(map[string]bool)
+		deps, err = dependencies(changes)
 	)
+	if err != nil {
+		return nil, err
+	}
 	visit = func(name string) bool {
 		if _, done := sorted[name]; done {
 			return false
@@ -112,25 +123,31 @@ func sortMap(changes []schema.Change) (map[string]int, bool) {
 	}
 	for node := range deps {
 		if visit(node) {
-			return nil, true
+			return nil, errCycle
 		}
 	}
-	return sorted, false
+	return sorted, nil
 }
 
 // dependencies returned an adjacency list of all tables and the table they depend on
-func dependencies(changes []schema.Change) map[string][]*schema.Table {
+func dependencies(changes []schema.Change) (map[string][]*schema.Table, error) {
 	deps := make(map[string][]*schema.Table)
 	for _, change := range changes {
 		switch change := change.(type) {
 		case *schema.AddTable:
 			for _, fk := range change.T.ForeignKeys {
+				if err := checkFK(fk); err != nil {
+					return nil, err
+				}
 				if fk.RefTable != change.T {
 					deps[change.T.Name] = append(deps[change.T.Name], fk.RefTable)
 				}
 			}
 		case *schema.DropTable:
 			for _, fk := range change.T.ForeignKeys {
+				if err := checkFK(fk); err != nil {
+					return nil, err
+				}
 				if isDropped(changes, fk.RefTable) {
 					deps[fk.RefTable.Name] = append(deps[fk.RefTable.Name], fk.Table)
 				}
@@ -139,10 +156,16 @@ func dependencies(changes []schema.Change) map[string][]*schema.Table {
 			for _, c := range change.Changes {
 				switch c := c.(type) {
 				case *schema.AddForeignKey:
+					if err := checkFK(c.F); err != nil {
+						return nil, err
+					}
 					if c.F.RefTable != change.T {
 						deps[change.T.Name] = append(deps[change.T.Name], c.F.RefTable)
 					}
 				case *schema.ModifyForeignKey:
+					if err := checkFK(c.To); err != nil {
+						return nil, err
+					}
 					if c.To.RefTable != change.T {
 						deps[change.T.Name] = append(deps[change.T.Name], c.To.RefTable)
 					}
@@ -150,7 +173,27 @@ func dependencies(changes []schema.Change) map[string][]*schema.Table {
 			}
 		}
 	}
-	return deps
+	return deps, nil
+}
+
+func checkFK(fk *schema.ForeignKey) error {
+	var cause []string
+	if fk.Table == nil {
+		cause = append(cause, "child table")
+	}
+	if len(fk.Columns) == 0 {
+		cause = append(cause, "child columns")
+	}
+	if fk.RefTable == nil {
+		cause = append(cause, "parent table")
+	}
+	if len(fk.RefColumns) == 0 {
+		cause = append(cause, "parent columns")
+	}
+	if len(cause) != 0 {
+		return fmt.Errorf("missing %q for foreign key: %q", cause, fk.Symbol)
+	}
+	return nil
 }
 
 // table extracts a table from the given change.
diff --git a/sql/mysql/migrate.go b/sql/mysql/migrate.go
index f0fd90ee..a41b40e9 100644
--- a/sql/mysql/migrate.go
+++ b/sql/mysql/migrate.go
@@ -24,8 +24,11 @@ func (d *Driver) Migrate() schema.Execer {
 
 // Exec executes the changes on the database. An error is returned
 // if one of the operations fail, or a change is not supported.
-func (m *migrate) Exec(ctx context.Context, changes []schema.Change) (err error) {
-	planned := sqlx.DetachCycles(changes)
+func (m *migrate) Exec(ctx context.Context, changes []schema.Change) error {
+	planned, err := sqlx.DetachCycles(changes)
+	if err != nil {
+		return err
+	}
 	for _, c := range planned {
 		switch c := c.(type) {
 		case *schema.AddTable:
@@ -41,7 +44,7 @@ func (m *migrate) Exec(ctx context.Context, changes []schema.Change) (err error)
 			return err
 		}
 	}
-	return
+	return nil
 }
 
 // addTable builds and executes the query for creating a table in a schema.
diff --git a/sql/postgres/migrate.go b/sql/postgres/migrate.go
index a82fa081..0a601c32 100644
--- a/sql/postgres/migrate.go
+++ b/sql/postgres/migrate.go
@@ -23,8 +23,11 @@ func (d *Driver) Migrate() schema.Execer {
 
 // Exec executes the changes on the database. An error is returned
 // if one of the operations fail, or a change is not supported.
-func (m *migrate) Exec(ctx context.Context, changes []schema.Change) (err error) {
-	planned := sqlx.DetachCycles(changes)
+func (m *migrate) Exec(ctx context.Context, changes []schema.Change) error {
+	planned, err := sqlx.DetachCycles(changes)
+	if err != nil {
+		return err
+	}
 	for _, c := range planned {
 		switch c := c.(type) {
 		case *schema.AddTable:
@@ -40,7 +43,7 @@ func (m *migrate) Exec(ctx context.Context, changes []schema.Change) (err error)
 			return err
 		}
 	}
-	return
+	return nil
 }
 
 // addTable builds and executes the query for creating a table in a schema.


Test patch:
diff --git a/internal/integration/mysql_test.go b/internal/integration/mysql_test.go
index ccacec4c..78360b5e 100644
--- a/internal/integration/mysql_test.go
+++ b/internal/integration/mysql_test.go
@@ -458,18 +458,42 @@ schema "test" {
 }
 table "users" {
 	schema = "test"
-	column "email" {
-		type = "string"
+	column "id" {
+		type = "int"
+	}
+	primary_key {
+		columns = [table.users.column.id]
+	}
+}
+table "posts" {
+	schema = "test"
+	column "id" {
+		type = "int"
+	}
+	column "author_id" {
+		type = "int"
+	}
+	foreign_key "author" {
+		columns = [
+			table.posts.column.author_id,
+		]
+		ref_columns = [
+			table.users.column.id,
+		]
+	}
+	primary_key {
+		columns = [table.users.column.id]
 	}
 }
 `)
 		users := t.loadUsers()
-		t.dropTables(users.Name)
-		column, ok := users.Column("email")
-		require.True(t, ok, "expected name column")
+		posts := t.loadPosts()
+		t.dropTables(users.Name, posts.Name)
+		column, ok := users.Column("id")
+		require.True(t, ok, "expected id column")
 		require.Equal(t, "users", users.Name)
-		require.Equal(t, "email", column.Name)
-		require.Equal(t, column.Type.Raw, "varchar(255)")
+		column, ok = posts.Column("author_id")
+		require.Equal(t, "author_id", column.Name)
 		t.applyHcl(`
 schema "test" {
 }
diff --git a/sql/internal/sqlx/plan_test.go b/sql/internal/sqlx/plan_test.go
index e76b8858..5e11dd49 100644
--- a/sql/internal/sqlx/plan_test.go
+++ b/sql/internal/sqlx/plan_test.go
@@ -31,18 +31,21 @@ func TestDetachCycles(t *testing.T) {
 		{Symbol: "workplace", Table: users, Columns: users.Columns[1:], RefTable: workplaces, RefColumns: workplaces.Columns[:1]},
 	}
 	changes := []schema.Change{&schema.AddTable{T: workplaces}, &schema.AddTable{T: users}}
-	planned := DetachCycles(changes)
+	planned, err := DetachCycles(changes)
+	require.NoError(t, err)
 	require.Equal(t, changes, planned)
 
 	deletion := []schema.Change{&schema.DropTable{T: users}, &schema.DropTable{T: workplaces}}
-	planned = DetachCycles(deletion)
+	planned, err = DetachCycles(deletion)
+	require.NoError(t, err)
 	require.Equal(t, deletion, planned)
 
 	// Create a circular reference.
 	workplaces.ForeignKeys = []*schema.ForeignKey{
 		{Symbol: "owner", Table: workplaces, Columns: workplaces.Columns[1:], RefTable: users, RefColumns: users.Columns[:1]},
 	}
-	planned = DetachCycles(changes)
+	planned, err = DetachCycles(changes)
+	require.NoError(t, err)
 	require.Len(t, planned, 4)
 	require.Empty(t, planned[0].(*schema.AddTable).T.ForeignKeys)
 	require.Empty(t, planned[1].(*schema.AddTable).T.ForeignKeys)
@@ -63,7 +66,8 @@ func TestDetachCycles(t *testing.T) {
 		},
 	}, planned[3])
 
-	planned = DetachCycles(deletion)
+	planned, err = DetachCycles(deletion)
+	require.NoError(t, err)
 	require.Equal(t, &schema.ModifyTable{
 		T: users,
 		Changes: []schema.Change{
diff --git a/sql/mysql/sqlspec_test.go b/sql/mysql/sqlspec_test.go
index 2fe77dba..c4174116 100644
--- a/sql/mysql/sqlspec_test.go
+++ b/sql/mysql/sqlspec_test.go
@@ -137,6 +137,7 @@ table "accounts" {
 			Symbol:     "accounts",
 			Table:      exp.Tables[0],
 			Columns:    []*schema.Column{exp.Tables[0].Columns[2]},
+			RefTable:   exp.Tables[1],
 			RefColumns: []*schema.Column{exp.Tables[1].Columns[0]},
 			OnDelete:   schema.SetNull,
 		},
diff --git a/sql/postgres/sqlspec_test.go b/sql/postgres/sqlspec_test.go
index faeccea7..8f01012f 100644
--- a/sql/postgres/sqlspec_test.go
+++ b/sql/postgres/sqlspec_test.go
@@ -136,6 +136,7 @@ table "accounts" {
 			Symbol:     "accounts",
 			Table:      exp.Tables[0],
 			Columns:    []*schema.Column{exp.Tables[0].Columns[2]},
+			RefTable:   exp.Tables[1],
 			RefColumns: []*schema.Column{exp.Tables[1].Columns[0]},
 			OnDelete:   schema.SetNull,
 		},
diff --git a/sql/sqlite/sqlspec_test.go b/sql/sqlite/sqlspec_test.go
index a8a07ac2..85d7f9a2 100644
--- a/sql/sqlite/sqlspec_test.go
+++ b/sql/sqlite/sqlspec_test.go
@@ -136,6 +136,7 @@ table "accounts" {
 			Symbol:     "accounts",
 			Table:      exp.Tables[0],
 			Columns:    []*schema.Column{exp.Tables[0].Columns[2]},
+			RefTable:   exp.Tables[1],
 			RefColumns: []*schema.Column{exp.Tables[1].Columns[0]},
 			OnDelete:   schema.SetNull,
 		},