Repository: ariga/atlas
Pull request #222 (2021-11-14 15:06:06)

sql/internal/specutil: fix missing schema name for table


Patch:
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
index d4fcaaf0..85f4322d 100644
--- a/.github/workflows/ci.yml
+++ b/.github/workflows/ci.yml
@@ -110,7 +110,7 @@ jobs:
           POSTGRES_DB: test
           POSTGRES_PASSWORD: pass
         ports:
-          - 5433:5432
+          - 5432:5432
         options: >-
           --health-cmd pg_isready
           --health-interval 10s
@@ -118,6 +118,18 @@ jobs:
           --health-retries 5
       postgres13:
         image: postgres:13.1
+        env:
+          POSTGRES_DB: test
+          POSTGRES_PASSWORD: pass
+        ports:
+          - 5433:5432
+        options: >-
+          --health-cmd pg_isready
+          --health-interval 10s
+          --health-timeout 5s
+          --health-retries 5
+      postgres14:
+        image: postgres:14
         env:
           POSTGRES_DB: test
           POSTGRES_PASSWORD: pass
diff --git a/internal/integration/docker-compose.yaml b/internal/integration/docker-compose.yaml
index b4229e34..2c49a632 100644
--- a/internal/integration/docker-compose.yaml
+++ b/internal/integration/docker-compose.yaml
@@ -65,7 +65,7 @@ services:
     healthcheck:
       test: pg_isready -U postgres
     ports:
-      - 5433:5432
+      - 5432:5432
 
   postgres13:
     platform: linux/amd64
@@ -76,4 +76,15 @@ services:
     healthcheck:
       test: pg_isready -U postgres
     ports:
-      - 5434:5432
\ No newline at end of file
+      - 5433:5432
+
+  postgres14:
+    platform: linux/amd64
+    image: postgres:13
+    environment:
+      POSTGRES_DB: test
+      POSTGRES_PASSWORD: pass
+    healthcheck:
+      test: pg_isready -U postgres
+    ports:
+      - 5434:5432
diff --git a/sql/internal/specutil/convert.go b/sql/internal/specutil/convert.go
index 2332131b..5f9bb1c2 100644
--- a/sql/internal/specutil/convert.go
+++ b/sql/internal/specutil/convert.go
@@ -212,6 +212,7 @@ func FromSchema(s *schema.Schema, fn TableSpecFunc) (*sqlspec.Schema, []*sqlspec
 		if err != nil {
 			return nil, nil, err
 		}
+		table.SchemaName = s.Name
 		tables = append(tables, table)
 	}
 	return spec, tables, nil
diff --git a/sql/internal/sqlx/diff.go b/sql/internal/sqlx/diff.go
index 3a1d414e..f0d3e4f4 100644
--- a/sql/internal/sqlx/diff.go
+++ b/sql/internal/sqlx/diff.go
@@ -49,15 +49,14 @@ type (
 		ReferenceChanged(from, to schema.ReferenceOption) bool
 	}
 
-	// A Normalizer wraps the Normalize method for normalizing table
-	// elements that were inspected from the database, or were defined
-	// by the users to a standard form.
+	// A Normalizer wraps the Normalize method for normalizing the from and to tables before
+	// running diffing. The "from" usually represents the inspected database state (current),
+	// and the second represents the desired state.
 	//
-	// If the DiffDriver implements the Normalizer interface, TableDiff
-	// normalizes its table inputs before starting the diff process.
+	// If the DiffDriver implements the Normalizer interface, TableDiff normalizes its table
+	// inputs before starting the diff process.
 	Normalizer interface {
-		// Normalize normalizes a list of tables.
-		Normalize(...*schema.Table)
+		Normalize(from, to *schema.Table)
 	}
 )
 
diff --git a/sql/mysql/diff.go b/sql/mysql/diff.go
index b492cc1e..27c81986 100644
--- a/sql/mysql/diff.go
+++ b/sql/mysql/diff.go
@@ -124,6 +124,24 @@ func (*diff) ReferenceChanged(from, to schema.ReferenceOption) bool {
 	return from != to
 }
 
+// Normalize implements the sqlx.Normalizer interface.
+func (d *diff) Normalize(from, to *schema.Table) {
+	for i, idx := range from.Indexes {
+		if _, ok := to.Index(idx.Name); ok {
+			continue
+		}
+		// MySQL requires that foreign key columns be indexed; Therefore, if the child
+		// table is defined on non-indexed columns, an index is automatically created
+		// to satisfy the constraint.
+		// Therefore, if no such key was defined on the desired state, the diff will
+		// recommend to drop it on migration. Therefore, we fix it by dropping it from
+		// the current state manually.
+		if keySupportsFK(from, idx) {
+			from.Indexes = append(from.Indexes[:i], from.Indexes[i+1:]...)
+		}
+	}
+}
+
 // collationChange returns the schema change for migrating the collation if
 // it was changed and its not the default attribute inherited from its parent.
 func (d *diff) collationChange(from, top, to []schema.Attr) schema.Change {
@@ -360,3 +378,24 @@ func displayWidth(attr []schema.Attr) *DisplayWidth {
 	}
 	return d
 }
+
+// keySupportsFK reports if the index key was created automatically by MySQL
+// to support the constraint. See sql/sql_table.cc#find_fk_supporting_key.
+func keySupportsFK(t *schema.Table, idx *schema.Index) bool {
+	if _, ok := t.ForeignKey(idx.Name); ok {
+		return true
+	}
+search:
+	for _, fk := range t.ForeignKeys {
+		if len(fk.Columns) != len(idx.Parts) {
+			continue
+		}
+		for i, c := range fk.Columns {
+			if idx.Parts[i].C == nil || idx.Parts[i].C.Name != c.Name {
+				continue search
+			}
+		}
+		return true
+	}
+	return false
+}
diff --git a/sql/postgres/diff.go b/sql/postgres/diff.go
index 1f4c7b2b..1c1cd2a8 100644
--- a/sql/postgres/diff.go
+++ b/sql/postgres/diff.go
@@ -226,10 +226,9 @@ func (d *diff) typeChanged(from, to *schema.Column) (bool, error) {
 }
 
 // Normalize implements the sqlx.Normalizer interface.
-func (d *diff) Normalize(tables ...*schema.Table) {
-	for _, t := range tables {
-		d.normalize(t)
-	}
+func (d *diff) Normalize(from, to *schema.Table) {
+	d.normalize(from)
+	d.normalize(to)
 }
 
 func (d *diff) normalize(table *schema.Table) {
diff --git a/sql/postgres/migrate.go b/sql/postgres/migrate.go
index 102168b2..781b3b04 100644
--- a/sql/postgres/migrate.go
+++ b/sql/postgres/migrate.go
@@ -86,7 +86,7 @@ func (m *migrate) modifyTable(ctx context.Context, modify *schema.ModifyTable) e
 		changes     []schema.Change
 		addI, dropI []*schema.Index
 	)
-	for _, change := range modify.Changes {
+	for _, change := range skipAutoDropped(modify.Changes) {
 		switch change := change.(type) {
 		case *schema.DropAttr:
 			return fmt.Errorf("unsupported change type: %T", change)
@@ -160,7 +160,7 @@ func (m *migrate) alterTable(ctx context.Context, t *schema.Table, changes []sch
 			b.P("ADD")
 			m.fks(b, change.F)
 		case *schema.DropForeignKey:
-			b.P("DROP FOREIGN KEY").Ident(change.F.Symbol)
+			b.P("DROP CONSTRAINT").Ident(change.F.Symbol)
 		}
 	})
 	if _, err := m.ExecContext(ctx, b.String()); err != nil {
@@ -421,6 +421,32 @@ func (m *migrate) fks(b *sqlx.Builder, fks ...*schema.ForeignKey) {
 	})
 }
 
+// skipAutoDropped filters unnecessary changes that are automatically
+// dropped by the database before running ALTER TABLE.
+func skipAutoDropped(changes []schema.Change) []schema.Change {
+	dropC := make(map[string]bool)
+	for _, c := range changes {
+		if c, ok := c.(*schema.DropColumn); ok {
+			dropC[c.C.Name] = true
+		}
+	}
+	for i, c := range changes {
+		switch c := c.(type) {
+		case *schema.DropIndex:
+			// Simple case for skipping key dropping, if its only column is also dropped.
+			// See https://www.postgresql.org/docs/current/sql-altertable.html
+			if len(c.I.Parts) == 1 && c.I.Parts[0].C != nil && dropC[c.I.Parts[0].C.Name] {
+				changes = append(changes[:i], changes[i+1:]...)
+			}
+		case *schema.DropForeignKey:
+			if len(c.F.Columns) == 1 && dropC[c.F.Columns[0].Name] {
+				changes = append(changes[:i], changes[i+1:]...)
+			}
+		}
+	}
+	return changes
+}
+
 // Build instantiates a new builder and writes the given phrase to it.
 func Build(phrase string) *sqlx.Builder {
 	b := &sqlx.Builder{QuoteChar: '"'}


Test patch:
diff --git a/internal/integration/mysql_test.go b/internal/integration/mysql_test.go
index 70593f59..9fc7a151 100644
--- a/internal/integration/mysql_test.go
+++ b/internal/integration/mysql_test.go
@@ -424,6 +424,55 @@ func TestMySQL_ForeignKey(t *testing.T) {
 			t.ensureNoChange(postsT, usersT)
 		})
 	})
+
+	t.Run("AddDrop", func(t *testing.T) {
+		myRun(t, func(t *myTest) {
+			usersT := t.users()
+			t.dropTables(usersT.Name)
+			err := t.drv.Migrate().Exec(ctx, []schema.Change{
+				&schema.AddTable{T: usersT},
+			})
+			require.NoError(t, err)
+			t.ensureNoChange(usersT)
+
+			// Add foreign key.
+			usersT.Columns = append(usersT.Columns, &schema.Column{
+				Name: "spouse_id",
+				Type: &schema.ColumnType{Raw: "bigint", Type: &schema.IntegerType{T: "bigint"}, Null: true},
+			})
+			usersT.ForeignKeys = append(usersT.ForeignKeys, &schema.ForeignKey{
+				Symbol:     "spouse_id",
+				Table:      usersT,
+				Columns:    usersT.Columns[len(usersT.Columns)-1:],
+				RefTable:   usersT,
+				RefColumns: usersT.Columns[:1],
+				OnDelete:   schema.NoAction,
+			})
+
+			changes, err := t.drv.Diff().TableDiff(t.loadUsers(), usersT)
+			require.NoError(t, err)
+			require.Len(t, changes, 2)
+			addC, ok := changes[0].(*schema.AddColumn)
+			require.True(t, ok)
+			require.Equal(t, "spouse_id", addC.C.Name)
+			addF, ok := changes[1].(*schema.AddForeignKey)
+			require.True(t, ok)
+			require.Equal(t, "spouse_id", addF.F.Symbol)
+			err = t.drv.Migrate().Exec(ctx, []schema.Change{&schema.ModifyTable{T: usersT, Changes: changes}})
+			require.NoError(t, err)
+			t.ensureNoChange(usersT)
+
+			// Drop foreign keys.
+			usersT.Columns = usersT.Columns[:len(usersT.Columns)-1]
+			usersT.ForeignKeys = usersT.ForeignKeys[:len(usersT.ForeignKeys)-1]
+			changes, err = t.drv.Diff().TableDiff(t.loadUsers(), usersT)
+			require.NoError(t, err)
+			require.Len(t, changes, 2)
+			err = t.drv.Migrate().Exec(ctx, []schema.Change{&schema.ModifyTable{T: usersT, Changes: changes}})
+			require.NoError(t, err)
+			t.ensureNoChange(usersT)
+		})
+	})
 }
 
 func TestMySQL_HCL(t *testing.T) {
@@ -578,9 +627,9 @@ func (t *myTest) loadUsers() *schema.Table {
 func (t *myTest) loadPosts() *schema.Table {
 	realm := t.loadRealm()
 	require.Len(t, realm.Schemas, 1)
-	users, ok := realm.Schemas[0].Table("posts")
+	posts, ok := realm.Schemas[0].Table("posts")
 	require.True(t, ok)
-	return users
+	return posts
 }
 
 // defaultConfig returns the default charset and
diff --git a/internal/integration/postgres_test.go b/internal/integration/postgres_test.go
index 858973f2..75597258 100644
--- a/internal/integration/postgres_test.go
+++ b/internal/integration/postgres_test.go
@@ -34,7 +34,7 @@ var pgTests struct {
 func pgRun(t *testing.T, fn func(*pgTest)) {
 	pgTests.Do(func() {
 		pgTests.drivers = make(map[string]*pgTest)
-		for version, port := range map[string]int{"10": 5430, "11": 5431, "12": 5433, "13": 5434} {
+		for version, port := range map[string]int{"10": 5430, "11": 5431, "12": 5432, "13": 5433, "14": 5434} {
 			db, err := sql.Open("postgres", fmt.Sprintf("host=localhost port=%d user=postgres dbname=test password=pass sslmode=disable", port))
 			require.NoError(t, err)
 			drv, err := postgres.Open(db)
@@ -283,6 +283,126 @@ func TestPostgres_Enums(t *testing.T) {
 	})
 }
 
+func TestPostgres_ForeignKey(t *testing.T) {
+	ctx := context.Background()
+	t.Run("ChangeAction", func(t *testing.T) {
+		pgRun(t, func(t *pgTest) {
+			usersT, postsT := t.users(), t.posts()
+			t.dropTables(postsT.Name, usersT.Name)
+			err := t.drv.Migrate().Exec(ctx, []schema.Change{
+				&schema.AddTable{T: usersT},
+				&schema.AddTable{T: postsT},
+			})
+			require.NoError(t, err)
+			t.ensureNoChange(postsT, usersT)
+
+			postsT = t.loadPosts()
+			fk, ok := postsT.ForeignKey("author_id")
+			require.True(t, ok)
+			fk.OnUpdate = schema.SetNull
+			fk.OnDelete = schema.Cascade
+			changes, err := t.drv.Diff().TableDiff(t.loadPosts(), postsT)
+			require.NoError(t, err)
+			require.Len(t, changes, 1)
+			modifyF, ok := changes[0].(*schema.ModifyForeignKey)
+			require.True(t, ok)
+			require.True(t, modifyF.Change == schema.ChangeUpdateAction|schema.ChangeDeleteAction)
+
+			err = t.drv.Migrate().Exec(ctx, []schema.Change{&schema.ModifyTable{T: postsT, Changes: changes}})
+			require.NoError(t, err)
+			t.ensureNoChange(postsT, usersT)
+		})
+	})
+
+	t.Run("UnsetNull", func(t *testing.T) {
+		pgRun(t, func(t *pgTest) {
+			usersT, postsT := t.users(), t.posts()
+			t.dropTables(postsT.Name, usersT.Name)
+			fk, ok := postsT.ForeignKey("author_id")
+			require.True(t, ok)
+			fk.OnDelete = schema.SetNull
+			fk.OnUpdate = schema.SetNull
+			err := t.drv.Migrate().Exec(ctx, []schema.Change{
+				&schema.AddTable{T: usersT},
+				&schema.AddTable{T: postsT},
+			})
+			require.NoError(t, err)
+			t.ensureNoChange(postsT, usersT)
+
+			postsT = t.loadPosts()
+			c, ok := postsT.Column("author_id")
+			require.True(t, ok)
+			c.Type.Null = false
+			fk, ok = postsT.ForeignKey("author_id")
+			require.True(t, ok)
+			fk.OnUpdate = schema.NoAction
+			fk.OnDelete = schema.NoAction
+			changes, err := t.drv.Diff().TableDiff(t.loadPosts(), postsT)
+			require.NoError(t, err)
+			require.Len(t, changes, 2)
+			modifyC, ok := changes[0].(*schema.ModifyColumn)
+			require.True(t, ok)
+			require.True(t, modifyC.Change == schema.ChangeNull)
+			modifyF, ok := changes[1].(*schema.ModifyForeignKey)
+			require.True(t, ok)
+			require.True(t, modifyF.Change == schema.ChangeUpdateAction|schema.ChangeDeleteAction)
+
+			err = t.drv.Migrate().Exec(ctx, []schema.Change{&schema.ModifyTable{T: postsT, Changes: changes}})
+			require.NoError(t, err)
+			t.ensureNoChange(postsT, usersT)
+		})
+	})
+
+	t.Run("AddDrop", func(t *testing.T) {
+		pgRun(t, func(t *pgTest) {
+			usersT := t.users()
+			t.dropTables(usersT.Name)
+			err := t.drv.Migrate().Exec(ctx, []schema.Change{
+				&schema.AddTable{T: usersT},
+			})
+			require.NoError(t, err)
+			t.ensureNoChange(usersT)
+
+			// Add foreign key.
+			usersT.Columns = append(usersT.Columns, &schema.Column{
+				Name: "spouse_id",
+				Type: &schema.ColumnType{Raw: "bigint", Type: &schema.IntegerType{T: "bigint"}, Null: true},
+			})
+			usersT.ForeignKeys = append(usersT.ForeignKeys, &schema.ForeignKey{
+				Symbol:     "spouse_id",
+				Table:      usersT,
+				Columns:    usersT.Columns[len(usersT.Columns)-1:],
+				RefTable:   usersT,
+				RefColumns: usersT.Columns[:1],
+				OnDelete:   schema.NoAction,
+			})
+
+			changes, err := t.drv.Diff().TableDiff(t.loadUsers(), usersT)
+			require.NoError(t, err)
+			require.Len(t, changes, 2)
+			addC, ok := changes[0].(*schema.AddColumn)
+			require.True(t, ok)
+			require.Equal(t, "spouse_id", addC.C.Name)
+			addF, ok := changes[1].(*schema.AddForeignKey)
+			require.True(t, ok)
+			require.Equal(t, "spouse_id", addF.F.Symbol)
+			err = t.drv.Migrate().Exec(ctx, []schema.Change{&schema.ModifyTable{T: usersT, Changes: changes}})
+			require.NoError(t, err)
+			t.ensureNoChange(usersT)
+
+			// Drop foreign keys.
+			usersT.Columns = usersT.Columns[:len(usersT.Columns)-1]
+			usersT.ForeignKeys = usersT.ForeignKeys[:len(usersT.ForeignKeys)-1]
+			changes, err = t.drv.Diff().TableDiff(t.loadUsers(), usersT)
+			require.NoError(t, err)
+			require.Len(t, changes, 2)
+			err = t.drv.Migrate().Exec(ctx, []schema.Change{&schema.ModifyTable{T: usersT, Changes: changes}})
+			require.NoError(t, err)
+			t.ensureNoChange(usersT)
+		})
+	})
+}
+
 func (t *pgTest) loadRealm() *schema.Realm {
 	r, err := t.drv.InspectRealm(context.Background(), &schema.InspectRealmOption{
 		Schemas: []string{"public"},
@@ -299,6 +419,14 @@ func (t *pgTest) loadUsers() *schema.Table {
 	return users
 }
 
+func (t *pgTest) loadPosts() *schema.Table {
+	realm := t.loadRealm()
+	require.Len(t, realm.Schemas, 1)
+	posts, ok := realm.Schemas[0].Table("posts")
+	require.True(t, ok)
+	return posts
+}
+
 func (t *pgTest) users() *schema.Table {
 	usersT := &schema.Table{
 		Name:   "users",
diff --git a/sql/internal/specutil/convert_test.go b/sql/internal/specutil/convert_test.go
index de4ca6ee..0f50e6be 100644
--- a/sql/internal/specutil/convert_test.go
+++ b/sql/internal/specutil/convert_test.go
@@ -4,6 +4,8 @@ import (
 	"testing"
 
 	"ariga.io/atlas/schema/schemaspec"
+	"ariga.io/atlas/sql/schema"
+	"ariga.io/atlas/sql/sqlspec"
 	"github.com/stretchr/testify/require"
 )
 
@@ -20,3 +22,19 @@ func TestRef_TableName(t *testing.T) {
 	require.NoError(t, err)
 	require.Equal(t, "accounts", c)
 }
+
+func TestFromSpec_SchemaName(t *testing.T) {
+	sc := &schema.Schema{
+		Name: "schema",
+		Tables: []*schema.Table{
+			{},
+		},
+	}
+	sc.Tables[0].Schema = sc
+	s, ta, err := FromSchema(sc, func(table *schema.Table) (*sqlspec.Table, error) {
+		return &sqlspec.Table{}, nil
+	})
+	require.NoError(t, err)
+	require.Equal(t, sc.Name, s.Name)
+	require.Equal(t, sc.Name, ta[0].SchemaName)
+}