Repository: ariga/atlas
Pull request #716 (2022-04-17 14:03:42)

sql/mysql: apply changes to generated columns


Patch:
diff --git a/sql/mysql/diff.go b/sql/mysql/diff.go
index 37bba5fa..6071b501 100644
--- a/sql/mysql/diff.go
+++ b/sql/mysql/diff.go
@@ -380,18 +380,7 @@ func (*diff) generatedChanged(from, to *schema.Column) bool {
 	case fromHas != toHas:
 		return true
 	case fromHas && toHas:
-		t := func(s string) string {
-			switch s = strings.ToUpper(s); s {
-			// The default is VIRTUAL if no type is specified.
-			case "":
-				return virtual
-			// In MariaDB, PERSISTENT is synonyms for STORED.
-			case persistent:
-				return stored
-			}
-			return s
-		}
-		return t(fromX.Type) != t(toX.Type) || fromX.Expr != toX.Expr
+		return storedOrVirtual(fromX.Type) != storedOrVirtual(toX.Type) || fromX.Expr != toX.Expr
 	default: // !fromHas && !toHas.
 		return false
 	}
diff --git a/sql/mysql/migrate.go b/sql/mysql/migrate.go
index 073cacdf..b567924e 100644
--- a/sql/mysql/migrate.go
+++ b/sql/mysql/migrate.go
@@ -335,6 +335,9 @@ func (s *state) alterTable(t *schema.Table, changes []schema.Change) error {
 				}
 				reverse = append(reverse, &schema.DropColumn{C: change.C})
 			case *schema.ModifyColumn:
+				if err := checkChangeGenerated(change); err != nil {
+					return err
+				}
 				b.P("MODIFY COLUMN")
 				if err := s.column(b, t, change.To); err != nil {
 					return err
@@ -409,7 +412,7 @@ func (s *state) alterTable(t *schema.Table, changes []schema.Change) error {
 			return nil
 		})
 		if err != nil {
-			return "", nil
+			return "", err
 		}
 		return b.String(), nil
 	}
@@ -717,6 +720,24 @@ func supportsCharset(t schema.Type) bool {
 	}
 }
 
+// checkChangeGenerated checks if the change of a generated column is valid.
+func checkChangeGenerated(c *schema.ModifyColumn) error {
+	if !c.Change.Is(schema.ChangeGenerated) {
+		return nil
+	}
+	var fromX, toX schema.GeneratedExpr
+	switch fromHas, toHas := sqlx.Has(c.From.Attrs, &fromX), sqlx.Has(c.To.Attrs, &toX); {
+	case !fromHas && toHas && storedOrVirtual(toX.Type) == virtual:
+		return fmt.Errorf("changing column %q to VIRTUAL generated column is not supported (drop and add is required)", c.From.Name)
+	case fromHas && !toHas && storedOrVirtual(fromX.Type) == virtual:
+		return fmt.Errorf("changing VIRTUAL generated column %q to non-generated column is not supported (drop and add is required)", c.From.Name)
+	case fromHas && toHas && storedOrVirtual(fromX.Type) != storedOrVirtual(toX.Type):
+		return fmt.Errorf("changing the store type of generated column %q from %q to %q is not supported", c.From.Name, storedOrVirtual(fromX.Type), storedOrVirtual(toX.Type))
+	default:
+		return nil
+	}
+}
+
 func quote(s string) string {
 	if sqlx.IsQuoted(s, '"', '\'') {
 		return s
diff --git a/sql/mysql/sqlspec.go b/sql/mysql/sqlspec.go
index 8f69ed24..da0e96d2 100644
--- a/sql/mysql/sqlspec.go
+++ b/sql/mysql/sqlspec.go
@@ -268,15 +268,11 @@ func columnSpec(c *schema.Column, t *schema.Table) (*sqlspec.Column, error) {
 
 // fromGenExpr returns the spec for a generated expression.
 func fromGenExpr(x schema.GeneratedExpr) *schemaspec.Resource {
-	t := strings.ToUpper(x.Type)
-	if t == "" {
-		t = virtual
-	}
 	return &schemaspec.Resource{
 		Type: "as",
 		Attrs: []*schemaspec.Attr{
 			specutil.StrAttr("expr", x.Expr),
-			specutil.VarAttr("type", t),
+			specutil.VarAttr("type", storedOrVirtual(x.Type)),
 		},
 	}
 }
@@ -304,17 +300,28 @@ func convertGenExpr(r *schemaspec.Resource, c *schema.Column) error {
 		if err := asR.As(&spec); err != nil {
 			return err
 		}
-		if spec.Type == "" {
-			spec.Type = virtual
-		}
 		c.Attrs = append(c.Attrs, &schema.GeneratedExpr{
-			Type: spec.Type,
 			Expr: spec.Expr,
+			Type: storedOrVirtual(spec.Type),
 		})
 	}
 	return nil
 }
 
+// storedOrVirtual returns a STORED or VIRTUAL
+// generated type option based on the given string.
+func storedOrVirtual(s string) string {
+	switch s = strings.ToUpper(s); s {
+	// The default is VIRTUAL if no type is specified.
+	case "":
+		return virtual
+	// In MariaDB, PERSISTENT is synonyms for STORED.
+	case persistent:
+		return stored
+	}
+	return s
+}
+
 // checkSpec converts from a concrete MySQL schema.Check into a sqlspec.Check.
 func checkSpec(s *schema.Check) *sqlspec.Check {
 	c := specutil.FromCheck(s)


Test patch:
diff --git a/sql/mysql/migrate_test.go b/sql/mysql/migrate_test.go
index aed9b689..3af3ce6c 100644
--- a/sql/mysql/migrate_test.go
+++ b/sql/mysql/migrate_test.go
@@ -6,6 +6,7 @@ package mysql
 
 import (
 	"context"
+	"strconv"
 	"testing"
 
 	"ariga.io/atlas/sql/internal/sqltest"
@@ -623,24 +624,122 @@ func TestPlanChanges(t *testing.T) {
 			},
 			wantErr: true,
 		},
+		// Changing a regular column to a VIRTUAL generated column is not allowed.
+		{
+			input: []schema.Change{
+				&schema.ModifyTable{
+					T: schema.NewTable("users"),
+					Changes: []schema.Change{
+						&schema.ModifyColumn{
+							Change: schema.ChangeGenerated,
+							From:   schema.NewColumn("c"),
+							To:     schema.NewColumn("c").SetGeneratedExpr(&schema.GeneratedExpr{Expr: "1"}),
+						},
+					},
+				},
+			},
+			wantErr: true,
+		},
+		// Changing a VIRTUAL generated column to a regular column is not allowed.
+		{
+			input: []schema.Change{
+				&schema.ModifyTable{
+					T: schema.NewTable("users"),
+					Changes: []schema.Change{
+						&schema.ModifyColumn{
+							Change: schema.ChangeGenerated,
+							From:   schema.NewColumn("c").SetGeneratedExpr(&schema.GeneratedExpr{Expr: "1", Type: "VIRTUAL"}),
+							To:     schema.NewColumn("c"),
+						},
+					},
+				},
+			},
+			wantErr: true,
+		},
+		// Changing the storage type of generated column is not allowed.
+		{
+			input: []schema.Change{
+				&schema.ModifyTable{
+					T: schema.NewTable("users"),
+					Changes: []schema.Change{
+						&schema.ModifyColumn{
+							Change: schema.ChangeGenerated,
+							From:   schema.NewColumn("c").SetGeneratedExpr(&schema.GeneratedExpr{Expr: "1", Type: "VIRTUAL"}),
+							To:     schema.NewColumn("c").SetGeneratedExpr(&schema.GeneratedExpr{Expr: "1", Type: "STORED"}),
+						},
+					},
+				},
+			},
+			wantErr: true,
+		},
+		// Changing a STORED generated column to a regular column.
+		{
+			input: []schema.Change{
+				&schema.ModifyTable{
+					T: schema.NewTable("users"),
+					Changes: []schema.Change{
+						&schema.ModifyColumn{
+							Change: schema.ChangeGenerated,
+							From:   schema.NewIntColumn("c", "int").SetGeneratedExpr(&schema.GeneratedExpr{Expr: "1", Type: "STORED"}),
+							To:     schema.NewIntColumn("c", "int"),
+						},
+					},
+				},
+			},
+			wantPlan: &migrate.Plan{
+				Reversible: true,
+				Changes: []*migrate.Change{
+					{
+						Cmd:     "ALTER TABLE `users` MODIFY COLUMN `c` int NOT NULL",
+						Reverse: "ALTER TABLE `users` MODIFY COLUMN `c` int AS (1) STORED NOT NULL",
+					},
+				},
+			},
+		},
+		// Changing a regular column to a STORED generated column.
+		{
+			input: []schema.Change{
+				&schema.ModifyTable{
+					T: schema.NewTable("users"),
+					Changes: []schema.Change{
+						&schema.ModifyColumn{
+							Change: schema.ChangeGenerated,
+							From:   schema.NewIntColumn("c", "int"),
+							To:     schema.NewIntColumn("c", "int").SetGeneratedExpr(&schema.GeneratedExpr{Expr: "1", Type: "STORED"}),
+						},
+					},
+				},
+			},
+			wantPlan: &migrate.Plan{
+				Reversible: true,
+				Changes: []*migrate.Change{
+					{
+						Cmd:     "ALTER TABLE `users` MODIFY COLUMN `c` int AS (1) STORED NOT NULL",
+						Reverse: "ALTER TABLE `users` MODIFY COLUMN `c` int NOT NULL",
+					},
+				},
+			},
+		},
 	}
-	for _, tt := range tests {
-		db, _, err := newMigrate("8.0.16")
-		require.NoError(t, err)
-		plan, err := db.PlanChanges(context.Background(), "wantPlan", tt.input)
-		if tt.wantErr {
-			require.Error(t, err, "expect plan to fail")
-			return
-		}
-		require.NoError(t, err)
-		require.NotNil(t, plan)
-		require.Equal(t, tt.wantPlan.Reversible, plan.Reversible)
-		require.Equal(t, tt.wantPlan.Transactional, plan.Transactional)
-		require.Equal(t, len(tt.wantPlan.Changes), len(plan.Changes))
-		for i, c := range plan.Changes {
-			require.Equal(t, tt.wantPlan.Changes[i].Cmd, c.Cmd)
-			require.Equal(t, tt.wantPlan.Changes[i].Reverse, c.Reverse)
-		}
+	for i, tt := range tests {
+		t.Run(strconv.Itoa(i), func(t *testing.T) {
+			db, _, err := newMigrate("8.0.16")
+			require.NoError(t, err)
+			plan, err := db.PlanChanges(context.Background(), "wantPlan", tt.input)
+			if tt.wantErr {
+				require.Error(t, err, "expect plan to fail")
+				return
+			}
+			require.NoError(t, err)
+			require.NotNil(t, plan)
+			require.Equal(t, tt.wantPlan.Reversible, plan.Reversible)
+			require.Equal(t, tt.wantPlan.Transactional, plan.Transactional)
+			require.Equal(t, len(tt.wantPlan.Changes), len(plan.Changes))
+			for i, c := range plan.Changes {
+				require.Equal(t, tt.wantPlan.Changes[i].Cmd, c.Cmd)
+				require.Equal(t, tt.wantPlan.Changes[i].Reverse, c.Reverse)
+			}
+		})
 	}
 }