Repository: ariga/atlas
Pull request #714 (2022-04-17 12:17:55)

sql/postgres: fixed invalid postgres DSN
DSN for Postgres with underscore (_) in the username causes an error: first path segment in URL cannot contain colon. 

Reason: the provider (postgres://) got stripped from the DSN which is then parsed as an URL. If the DSN contains username:password url.Parse either misinterpret the url (if the username contains only letters) or otherwise produces an error "first path segment in URL cannot contain colon".


Patch:
diff --git a/cmd/action/mux.go b/cmd/action/mux.go
index e93cbd4e..f203e985 100644
--- a/cmd/action/mux.go
+++ b/cmd/action/mux.go
@@ -109,7 +109,7 @@ func SchemaNameFromURL(ctx context.Context, url string) (string, error) {
 		}
 		return cfg.DBName, err
 	case "postgres":
-		return postgresSchema(dsn)
+		return postgresSchema(url)
 	case "sqlite":
 		return schemaName(ctx, dsn)
 	default:
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/cmd/action/mux_test.go b/cmd/action/mux_test.go
index ae15f8fc..1ed100c5 100644
--- a/cmd/action/mux_test.go
+++ b/cmd/action/mux_test.go
@@ -150,6 +150,10 @@ func Test_PostgresSchemaDSN(t *testing.T) {
 			url:      "postgres://localhost:5432/dbname?search_path=",
 			expected: "",
 		},
+		{
+			url:      "postgres://user_name:password@localhost:5432/dbname?sslmode=disable",
+			expected: "",
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.url, func(t *testing.T) {
diff --git a/internal/integration/script_test.go b/internal/integration/script_test.go
index d22c56db..91f431ea 100644
--- a/internal/integration/script_test.go
+++ b/internal/integration/script_test.go
@@ -488,7 +488,7 @@ func cmdApply(ts *testscript.TestScript, neg bool, args []string, apply func(con
 }
 
 func matchErr(ts *testscript.TestScript, err error, p string) {
-	re, rerr := regexp.Compile(`(?m)` + p)
+	re, rerr := regexp.Compile(`(?m)` + regexp.QuoteMeta(p))
 	ts.Check(rerr)
 	if !re.MatchString(err.Error()) {
 		ts.Fatalf("mismatched errors: %v != %s", err, p)
diff --git a/internal/integration/testdata/mysql/column-generated.txt b/internal/integration/testdata/mysql/column-generated.txt
index e6a90878..9cdceda8 100644
--- a/internal/integration/testdata/mysql/column-generated.txt
+++ b/internal/integration/testdata/mysql/column-generated.txt
@@ -4,6 +4,13 @@
 apply 1.hcl
 cmpshow users 1.sql
 
+! apply 2.fail1.hcl 'alter table "users": changing VIRTUAL generated column "b" to non-generated column is not supported (drop and add is required)'
+! apply 2.fail2.hcl 'alter table "users": changing column "a" to VIRTUAL generated column is not supported (drop and add is required)'
+! apply 2.fail3.hcl 'alter table "users": changing the store type of generated column "c" from "STORED" to "VIRTUAL" is not supported'
+
+apply 3.hcl
+cmpshow users 3.sql
+
 -- 1.hcl --
 schema "$db" {
     charset = "$charset"
@@ -48,3 +55,125 @@ CREATE TABLE `users` (
   `b` int GENERATED ALWAYS AS ((`a` * 2)) VIRTUAL NOT NULL,
   `c` int GENERATED ALWAYS AS ((`a` * `b`)) STORED NOT NULL
 )
+
+-- 2.fail1.hcl --
+schema "$db" {
+    charset = "$charset"
+    collate = "$collate"
+}
+
+table "users" {
+    schema = schema.$db
+    column "a" {
+        type = int
+    }
+    column "b" {
+        type = int
+    }
+    column "c" {
+        type = int
+        as {
+            expr = "a * b"
+            type = STORED
+        }
+    }
+}
+
+-- 2.fail2.hcl --
+schema "$db" {
+    charset = "$charset"
+    collate = "$collate"
+}
+
+table "users" {
+    schema = schema.$db
+    column "a" {
+        type = int
+        as = "1"
+    }
+    column "b" {
+        type = int
+        as = "a * 2"
+    }
+    column "c" {
+        type = int
+        as {
+            expr = "a * b"
+            type = VIRTUAL
+        }
+    }
+}
+
+-- 2.fail3.hcl --
+schema "$db" {
+    charset = "$charset"
+    collate = "$collate"
+}
+
+table "users" {
+    schema = schema.$db
+    column "a" {
+        type = int
+    }
+    column "b" {
+        type = int
+        as = "a * 2"
+    }
+    column "c" {
+        type = int
+        as {
+            expr = "a * b"
+            type = VIRTUAL
+        }
+    }
+}
+
+
+-- 3.hcl --
+schema "$db" {
+    charset = "$charset"
+    collate = "$collate"
+}
+
+table "users" {
+    schema = schema.$db
+    column "a" {
+        type = int
+        as {
+            expr = "1"
+            type = STORED
+        }
+    }
+    column "b" {
+        type = int
+        as = "a * 3"
+    }
+    column "c" {
+        type = int
+        as {
+            expr = "a * b"
+            type = STORED
+        }
+    }
+}
+
+-- 3.sql --
+CREATE TABLE `users` (
+  `a` int(11) GENERATED ALWAYS AS (1) STORED,
+  `b` int(11) GENERATED ALWAYS AS (`a` * 3) VIRTUAL,
+  `c` int(11) GENERATED ALWAYS AS (`a` * `b`) STORED
+)
+
+-- mysql57/3.sql --
+CREATE TABLE `users` (
+  `a` int(11) GENERATED ALWAYS AS (1) STORED NOT NULL,
+  `b` int(11) GENERATED ALWAYS AS ((`a` * 3)) VIRTUAL NOT NULL,
+  `c` int(11) GENERATED ALWAYS AS ((`a` * `b`)) STORED NOT NULL
+)
+
+-- mysql8/3.sql --
+CREATE TABLE `users` (
+  `a` int GENERATED ALWAYS AS (1) STORED NOT NULL,
+  `b` int GENERATED ALWAYS AS ((`a` * 3)) VIRTUAL NOT NULL,
+  `c` int GENERATED ALWAYS AS ((`a` * `b`)) STORED NOT NULL
+)
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)
+			}
+		})
 	}
 }