Repository: ariga/atlas
Pull request #407 (2022-01-13 17:21:06)

sql/mysql: reject CHARSET/COLLATE attributes for non-char columns on migrate
- Reject CHARSET/COLLATE attributes for non-char columns on migration
- "Normalize" inspected JSON columns on MariaDB
- Add UnsetCollate and UnsetCharset


Patch:
diff --git a/sql/mysql/inspect.go b/sql/mysql/inspect.go
index 78ceea1f..4db18e0a 100644
--- a/sql/mysql/inspect.go
+++ b/sql/mysql/inspect.go
@@ -391,15 +391,17 @@ func (i *inspect) checks(ctx context.Context, t *schema.Table) error {
 			check.Attrs = append(check.Attrs, &Enforced{})
 		}
 		t.Attrs = append(t.Attrs, check)
-		// In MariaDB, JSON is an alias to LONGTEXT, and the JSON_VALID
-		// CHECK constraint is automatically created for the column for
-		// versions >= 10.4.3. However, we expect tools like Atlas and
-		// Ent to manually add this CHECK for older versions of MariaDB.
+		// In MariaDB, JSON is an alias to LONGTEXT. For versions >= 10.4.3, the CHARSET and COLLATE set to utf8mb4
+		// and a CHECK constraint is automatically created for the column as well (i.e. JSON_VALID(`<C>`)). However,
+		// we expect tools like Atlas and Ent to manually add this CHECK for older versions of MariaDB.
 		if i.mariadb() {
 			c, ok := t.Column(check.Name)
 			if ok && c.Type.Raw == TypeLongText && check.Expr == fmt.Sprintf("json_valid(`%s`)", c.Name) {
 				c.Type.Raw = TypeJSON
 				c.Type.Type = &schema.JSONType{T: TypeJSON}
+				// Unset the inspected CHARSET/COLLATE attributes
+				// as they are valid only for character types.
+				c.UnsetCharset().UnsetCollation()
 			}
 		}
 
diff --git a/sql/mysql/migrate.go b/sql/mysql/migrate.go
index 007f4b13..111a2818 100644
--- a/sql/mysql/migrate.go
+++ b/sql/mysql/migrate.go
@@ -381,12 +381,18 @@ func (s *state) column(b *sqlx.Builder, t *schema.Table, c *schema.Column) error
 	for _, a := range c.Attrs {
 		switch a := a.(type) {
 		case *schema.Charset:
+			if !supportsCharset(c.Type.Type) {
+				return fmt.Errorf("column %q of type %T does not support the CHARSE attribute", c.Name, c.Type.Type)
+			}
 			// Define the charset explicitly
 			// in case it is not the default.
 			if s.character(t) != a.V {
 				b.P("CHARSET", a.V)
 			}
 		case *schema.Collation:
+			if !supportsCharset(c.Type.Type) {
+				return fmt.Errorf("column %q of type %T does not support the COLLATE attribute", c.Name, c.Type.Type)
+			}
 			// Define the collation explicitly
 			// in case it is not the default.
 			if s.collation(t) != a.V {
@@ -590,6 +596,17 @@ func (s *state) check(b *sqlx.Builder, c *schema.Check) {
 	}
 }
 
+// supportsCharset reports if the given type supports the CHARSET and COLLATE
+// clauses. See: https://dev.mysql.com/doc/refman/8.0/en/charset-column.html
+func supportsCharset(t schema.Type) bool {
+	switch t.(type) {
+	case *schema.StringType, *schema.EnumType, *SetType:
+		return true
+	default:
+		return false
+	}
+}
+
 func quote(s string) string {
 	if sqlx.IsQuoted(s, '"', '\'') {
 		return s
diff --git a/sql/schema/dsl.go b/sql/schema/dsl.go
index 737b6b50..03b04b18 100644
--- a/sql/schema/dsl.go
+++ b/sql/schema/dsl.go
@@ -23,6 +23,12 @@ func (s *Schema) SetCharset(v string) *Schema {
 	return s
 }
 
+// UnsetCharset unsets the Charset attribute.
+func (s *Schema) UnsetCharset() *Schema {
+	del(&s.Attrs, &Charset{})
+	return s
+}
+
 // SetCollation sets or appends the Collation attribute
 // to the schema with the given value.
 func (s *Schema) SetCollation(v string) *Schema {
@@ -30,6 +36,12 @@ func (s *Schema) SetCollation(v string) *Schema {
 	return s
 }
 
+// UnsetCollation the Collation attribute.
+func (s *Schema) UnsetCollation() *Schema {
+	del(&s.Attrs, &Collation{})
+	return s
+}
+
 // SetComment sets or appends the Comment attribute
 // to the schema with the given value.
 func (s *Schema) SetComment(v string) *Schema {
@@ -70,6 +82,12 @@ func (t *Table) SetCharset(v string) *Table {
 	return t
 }
 
+// UnsetCharset unsets the Charset attribute.
+func (t *Table) UnsetCharset() *Table {
+	del(&t.Attrs, &Charset{})
+	return t
+}
+
 // SetCollation sets or appends the Collation attribute
 // to the table with the given value.
 func (t *Table) SetCollation(v string) *Table {
@@ -77,6 +95,12 @@ func (t *Table) SetCollation(v string) *Table {
 	return t
 }
 
+// UnsetCollation the Collation attribute.
+func (t *Table) UnsetCollation() *Table {
+	del(&t.Attrs, &Collation{})
+	return t
+}
+
 // SetComment sets or appends the Comment attribute
 // to the table with the given value.
 func (t *Table) SetComment(v string) *Table {
@@ -398,6 +422,12 @@ func (c *Column) SetCharset(v string) *Column {
 	return c
 }
 
+// UnsetCharset unsets the Charset attribute.
+func (c *Column) UnsetCharset() *Column {
+	del(&c.Attrs, &Charset{})
+	return c
+}
+
 // SetCollation sets or appends the Collation attribute
 // to the column with the given value.
 func (c *Column) SetCollation(v string) *Column {
@@ -405,6 +435,12 @@ func (c *Column) SetCollation(v string) *Column {
 	return c
 }
 
+// UnsetCollation the Collation attribute.
+func (c *Column) UnsetCollation() *Column {
+	del(&c.Attrs, &Collation{})
+	return c
+}
+
 // SetComment sets or appends the Comment attribute
 // to the column with the given value.
 func (c *Column) SetComment(v string) *Column {
@@ -595,3 +631,15 @@ func replaceOrAppend(attrs *[]Attr, v Attr) {
 	}
 	*attrs = append(*attrs, v)
 }
+
+// del searches an attribute of the same type as v in
+// the list and delete it.
+func del(attrs *[]Attr, v Attr) {
+	t := reflect.TypeOf(v)
+	for i := range *attrs {
+		if reflect.TypeOf((*attrs)[i]) == t {
+			*attrs = append((*attrs)[:i], (*attrs)[i+1:]...)
+			return
+		}
+	}
+}


Test patch:
diff --git a/internal/integration/mysql_test.go b/internal/integration/mysql_test.go
index 869f7fa2..9c1e605a 100644
--- a/internal/integration/mysql_test.go
+++ b/internal/integration/mysql_test.go
@@ -930,27 +930,21 @@ create table atlas_types_sanity
 				}(),
 				Schema: realm.Schemas[0],
 				Columns: []*schema.Column{
-					{
-						Name: "tJSON",
-						Type: func() *schema.ColumnType {
-							if t.version == "Maria102" || t.version == "Maria103" {
-								return &schema.ColumnType{Type: &schema.StringType{T: "longtext"},
-									Raw: "longtext", Null: true}
-							}
-							return &schema.ColumnType{Type: &schema.JSONType{T: "json"},
-								Raw: "json", Null: true}
-
-						}(),
-						Attrs: func() []schema.Attr {
-							if t.mariadb() {
-								return []schema.Attr{
-									&schema.Charset{V: "utf8mb4"},
-									&schema.Collation{V: "utf8mb4_bin"},
-								}
+					func() *schema.Column {
+						c := &schema.Column{Name: "tJSON", Type: &schema.ColumnType{Type: &schema.JSONType{T: "json"}, Raw: "json", Null: true}}
+						switch t.version {
+						case "Maria107":
+							c.Attrs = []schema.Attr{}
+						case "Maria102", "Maria103":
+							c.Type.Raw = "longtext"
+							c.Type.Type = &schema.StringType{T: "longtext"}
+							c.Attrs = []schema.Attr{
+								&schema.Charset{V: "utf8mb4"},
+								&schema.Collation{V: "utf8mb4_bin"},
 							}
-							return nil
-						}(),
-					},
+						}
+						return c
+					}(),
 				},
 			}
 			require.EqualValues(t, &expected, ts)
diff --git a/sql/mysql/migrate_test.go b/sql/mysql/migrate_test.go
index 32b1838f..0a089d3e 100644
--- a/sql/mysql/migrate_test.go
+++ b/sql/mysql/migrate_test.go
@@ -172,28 +172,29 @@ func TestMigrate_DetachCycles(t *testing.T) {
 
 func TestPlanChanges(t *testing.T) {
 	tests := []struct {
-		changes []schema.Change
-		plan    *migrate.Plan
+		input    []schema.Change
+		wantPlan *migrate.Plan
+		wantErr  bool
 	}{
 		{
-			changes: []schema.Change{
+			input: []schema.Change{
 				&schema.AddSchema{S: &schema.Schema{Name: "test", Attrs: []schema.Attr{&schema.Charset{V: "latin"}}}},
 			},
-			plan: &migrate.Plan{
+			wantPlan: &migrate.Plan{
 				Reversible: true,
 				Changes:    []*migrate.Change{{Cmd: "CREATE DATABASE `test` CHARSET latin", Reverse: "DROP DATABASE `test`"}},
 			},
 		},
 		{
-			changes: []schema.Change{
+			input: []schema.Change{
 				&schema.DropSchema{S: &schema.Schema{Name: "atlas", Attrs: []schema.Attr{&schema.Charset{V: "latin"}}}},
 			},
-			plan: &migrate.Plan{
+			wantPlan: &migrate.Plan{
 				Changes: []*migrate.Change{{Cmd: "DROP DATABASE `atlas`"}},
 			},
 		},
 		{
-			changes: []schema.Change{
+			input: []schema.Change{
 				func() *schema.AddTable {
 					t := &schema.Table{
 						Name: "posts",
@@ -207,13 +208,13 @@ func TestPlanChanges(t *testing.T) {
 					return &schema.AddTable{T: t}
 				}(),
 			},
-			plan: &migrate.Plan{
+			wantPlan: &migrate.Plan{
 				Reversible: true,
 				Changes:    []*migrate.Change{{Cmd: "CREATE TABLE `posts` (`id` bigint NOT NULL AUTO_INCREMENT, `text` text NULL, `uuid` char(36) NULL CHARSET utf8mb4 COLLATE utf8mb4_bin, PRIMARY KEY (`id`))", Reverse: "DROP TABLE `posts`"}},
 			},
 		},
 		{
-			changes: []schema.Change{
+			input: []schema.Change{
 				func() *schema.AddTable {
 					t := &schema.Table{
 						Name: "posts",
@@ -243,13 +244,13 @@ func TestPlanChanges(t *testing.T) {
 					return &schema.AddTable{T: t}
 				}(),
 			},
-			plan: &migrate.Plan{
+			wantPlan: &migrate.Plan{
 				Reversible: true,
 				Changes:    []*migrate.Change{{Cmd: "CREATE TABLE `posts` (`id` bigint NOT NULL AUTO_INCREMENT, `text` text NULL, `ch` char NOT NULL, PRIMARY KEY (`id`), INDEX `text_prefix` (`text` (100) DESC), CONSTRAINT `id_nonzero` CHECK (`id` > 0)) CHARSET utf8mb4 COLLATE utf8mb4_bin COMMENT \"posts comment\" COMPRESSION=\"ZLIB\" AUTO_INCREMENT 100", Reverse: "DROP TABLE `posts`"}},
 			},
 		},
 		{
-			changes: []schema.Change{
+			input: []schema.Change{
 				func() *schema.AddTable {
 					t := &schema.Table{
 						Name: "posts",
@@ -263,21 +264,21 @@ func TestPlanChanges(t *testing.T) {
 					return &schema.AddTable{T: t}
 				}(),
 			},
-			plan: &migrate.Plan{
+			wantPlan: &migrate.Plan{
 				Reversible: true,
 				Changes:    []*migrate.Change{{Cmd: "CREATE TABLE `posts` (`id` bigint NOT NULL AUTO_INCREMENT, `text` text NULL, PRIMARY KEY (`id`)) AUTO_INCREMENT 10", Reverse: "DROP TABLE `posts`"}},
 			},
 		},
 		{
-			changes: []schema.Change{
+			input: []schema.Change{
 				&schema.DropTable{T: &schema.Table{Name: "posts"}},
 			},
-			plan: &migrate.Plan{
+			wantPlan: &migrate.Plan{
 				Changes: []*migrate.Change{{Cmd: "DROP TABLE `posts`"}},
 			},
 		},
 		{
-			changes: []schema.Change{
+			input: []schema.Change{
 				func() schema.Change {
 					users := &schema.Table{
 						Name: "users",
@@ -317,7 +318,7 @@ func TestPlanChanges(t *testing.T) {
 					}
 				}(),
 			},
-			plan: &migrate.Plan{
+			wantPlan: &migrate.Plan{
 				Reversible: true,
 				Changes: []*migrate.Change{
 					{
@@ -328,7 +329,7 @@ func TestPlanChanges(t *testing.T) {
 			},
 		},
 		{
-			changes: []schema.Change{
+			input: []schema.Change{
 				func() schema.Change {
 					users := &schema.Table{
 						Name: "users",
@@ -350,7 +351,7 @@ func TestPlanChanges(t *testing.T) {
 					}
 				}(),
 			},
-			plan: &migrate.Plan{
+			wantPlan: &migrate.Plan{
 				Reversible: true,
 				Changes: []*migrate.Change{
 					{
@@ -361,7 +362,7 @@ func TestPlanChanges(t *testing.T) {
 			},
 		},
 		{
-			changes: []schema.Change{
+			input: []schema.Change{
 				func() schema.Change {
 					users := &schema.Table{
 						Name: "users",
@@ -408,7 +409,7 @@ func TestPlanChanges(t *testing.T) {
 					}
 				}(),
 			},
-			plan: &migrate.Plan{
+			wantPlan: &migrate.Plan{
 				Reversible: true,
 				Changes: []*migrate.Change{
 					{
@@ -418,17 +419,38 @@ func TestPlanChanges(t *testing.T) {
 				},
 			},
 		},
+		{
+			input: []schema.Change{
+				&schema.AddTable{
+					T: schema.NewTable("users").AddColumns(schema.NewIntColumn("id", "bigint").SetCharset("utf8mb4")),
+				},
+			},
+			wantErr: true,
+		},
+		{
+			input: []schema.Change{
+				&schema.AddTable{
+					T: schema.NewTable("users").AddColumns(schema.NewIntColumn("id", "bigint").SetCollation("utf8mb4_general_ci")),
+				},
+			},
+			wantErr: true,
+		},
 	}
 	for _, tt := range tests {
 		db, _, err := newMigrate("8.0.16")
 		require.NoError(t, err)
-		plan, err := db.PlanChanges(context.Background(), "plan", tt.changes)
+		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.Equal(t, tt.plan.Reversible, plan.Reversible)
-		require.Equal(t, tt.plan.Transactional, plan.Transactional)
+		require.NotNil(t, plan)
+		require.Equal(t, tt.wantPlan.Reversible, plan.Reversible)
+		require.Equal(t, tt.wantPlan.Transactional, plan.Transactional)
 		for i, c := range plan.Changes {
-			require.Equal(t, tt.plan.Changes[i].Cmd, c.Cmd)
-			require.Equal(t, tt.plan.Changes[i].Reverse, c.Reverse)
+			require.Equal(t, tt.wantPlan.Changes[i].Cmd, c.Cmd)
+			require.Equal(t, tt.wantPlan.Changes[i].Reverse, c.Reverse)
 		}
 	}
 }
diff --git a/sql/schema/dsl_test.go b/sql/schema/dsl_test.go
index 93eeff75..b6074c86 100644
--- a/sql/schema/dsl_test.go
+++ b/sql/schema/dsl_test.go
@@ -139,6 +139,8 @@ func TestSchema_SetCharset(t *testing.T) {
 	s.SetCharset("latin1")
 	require.Len(t, s.Attrs, 1)
 	require.Equal(t, &schema.Charset{V: "latin1"}, s.Attrs[0])
+	s.UnsetCharset()
+	require.Empty(t, s.Attrs)
 }
 
 func TestSchema_SetCollation(t *testing.T) {
@@ -150,6 +152,8 @@ func TestSchema_SetCollation(t *testing.T) {
 	s.SetCollation("latin1_swedish_ci")
 	require.Len(t, s.Attrs, 1)
 	require.Equal(t, &schema.Collation{V: "latin1_swedish_ci"}, s.Attrs[0])
+	s.UnsetCollation()
+	require.Empty(t, s.Attrs)
 }
 
 func TestSchema_SetComment(t *testing.T) {