Repository: ariga/atlas
Pull request #938 (2022-07-13 09:30:38)

sql/sqlcheck: add datadepend checks for postgres and sqlite


Patch:
diff --git a/cmd/atlas/internal/cmdapi/schema.go b/cmd/atlas/internal/cmdapi/schema.go
index 44b06157..7a8e6c7f 100644
--- a/cmd/atlas/internal/cmdapi/schema.go
+++ b/cmd/atlas/internal/cmdapi/schema.go
@@ -93,7 +93,7 @@ migration.`,
 	// SchemaInspect represents the 'atlas schema inspect' subcommand.
 	SchemaInspect = &cobra.Command{
 		Use:   "inspect",
-		Short: "Inspect a database's and print its schema in Atlas DDL syntax.",
+		Short: "Inspect a database and print its schema in Atlas DDL syntax.",
 		Long: `'atlas schema inspect' connects to the given database and inspects its schema.
 It then prints to the screen the schema of that database in Atlas DDL syntax. This output can be
 saved to a file, commonly by redirecting the output to a file named with a ".hcl" suffix:
diff --git a/internal/integration/testdata/sqlite/cli-migrate-lint-sanity.txt b/internal/integration/testdata/sqlite/cli-migrate-lint-destructive.txt
similarity index 100%
rename from internal/integration/testdata/sqlite/cli-migrate-lint-sanity.txt
rename to internal/integration/testdata/sqlite/cli-migrate-lint-destructive.txt
diff --git a/sql/mysql/mysqlcheck/mysqlcheck.go b/sql/mysql/mysqlcheck/mysqlcheck.go
index 79752b05..2bc249ac 100644
--- a/sql/mysql/mysqlcheck/mysqlcheck.go
+++ b/sql/mysql/mysqlcheck/mysqlcheck.go
@@ -5,7 +5,6 @@
 package mysqlcheck
 
 import (
-	"context"
 	"fmt"
 	"strconv"
 	"strings"
@@ -18,82 +17,31 @@ import (
 	"ariga.io/atlas/sql/sqlcheck/destructive"
 )
 
-type (
-	// DataDependOptions defines the additional configuration
-	// options for the data-dependent changes checker.
-	DataDependOptions struct {
-		// NotNull indicates if the analyzer should check for modification
-		// or addition of NOT NULL constraints to columns.
-		NotNull *bool `spec:"not_null,omitempty"`
-
-		// Allow drivers to extend the configuration.
-		schemahcl.DefaultExtension
-	}
-
-	// DataDepend checks data-dependent changes for MySQL.
-	DataDepend struct {
-		DataDependOptions
-		*datadepend.Analyzer
-	}
-)
-
 // NewDataDepend creates new data-depend analyzer.
-func NewDataDepend(*schemahcl.Resource) (*DataDepend, error) {
-	notnull := true
-	return &DataDepend{
-		Analyzer:          datadepend.New(datadepend.Options{}),
-		DataDependOptions: DataDependOptions{NotNull: &notnull},
-	}, nil
+func NewDataDepend(*schemahcl.Resource) *datadepend.Analyzer {
+	var opts datadepend.Options
+	opts.Handler.AddNotNull = addNotNull
+	return datadepend.New(opts)
 }
 
-// Analyze runs data-depend analysis on MySQL changes.
-func (d *DataDepend) Analyze(ctx context.Context, p *sqlcheck.Pass) error {
-	diags := d.Diagnostics(ctx, p)
-	for _, sc := range p.File.Changes {
-		for _, c := range sc.Changes {
-			m, ok := c.(*schema.ModifyTable)
-			if !ok {
-				continue
-			}
-			for _, c := range m.Changes {
-				switch c := c.(type) {
-				case *schema.AddColumn:
-					ds, err := d.addColumn(p, sc, m.T, c.C)
-					if err != nil {
-						return err
-					}
-					diags = append(diags, ds...)
-				}
-			}
-		}
-	}
-	d.Report(p, diags)
-	return nil
-}
-
-func (d *DataDepend) addColumn(p *sqlcheck.Pass, change *sqlcheck.Change, t *schema.Table, c *schema.Column) (diags []sqlcheck.Diagnostic, err error) {
-	// Skip this check in case it is disabled, the column is nullable
-	// or with default value, or the table was added in this file.
-	if !*d.NotNull || c.Type.Null || c.Default != nil || p.File.TableSpan(t)&sqlcheck.SpanAdded == 1 {
-		return
-	}
+func addNotNull(p *datadepend.ColumnPass) (diags []sqlcheck.Diagnostic, err error) {
 	// Two types of reporting, implicit rows update and
 	// changes that may cause the migration to fail.
 	mightFail := func(tt string) {
 		diags = append(diags, sqlcheck.Diagnostic{
-			Pos: change.Pos,
+			Pos: p.Change.Pos,
 			Text: fmt.Sprintf(
 				"Adding a non-nullable %q column %q will fail in case table %q is not empty",
-				tt, c.Name, t.Name,
+				tt, p.Column.Name, p.Table.Name,
 			),
 		})
 	}
 	implicitUpdate := func(tt, v string) {
 		diags = append(diags, sqlcheck.Diagnostic{
-			Pos: change.Pos,
+			Pos: p.Change.Pos,
 			Text: fmt.Sprintf(
 				"Adding a non-nullable %q column %q on table %q without a default value implicitly sets existing rows with %s",
-				tt, c.Name, t.Name, v,
+				tt, p.Column.Name, p.Table.Name, v,
 			),
 		})
 	}
@@ -101,9 +49,9 @@ func (d *DataDepend) addColumn(p *sqlcheck.Pass, change *sqlcheck.Change, t *sch
 	if !ok {
 		return nil, fmt.Errorf("unexpected migrate driver %T", p.Dev.Driver)
 	}
-	switch ct := c.Type.Type.(type) {
+	switch ct := p.Column.Type.Type.(type) {
 	case *mysql.BitType, *schema.BoolType, *schema.IntegerType, *schema.DecimalType, *schema.FloatType, *schema.BinaryType:
-		tt, err := mysql.FormatType(c.Type.Type)
+		tt, err := mysql.FormatType(p.Column.Type.Type)
 		if err != nil {
 			return nil, err
 		}
@@ -122,7 +70,7 @@ func (d *DataDepend) addColumn(p *sqlcheck.Pass, change *sqlcheck.Change, t *sch
 		}
 	case *schema.EnumType:
 		if len(ct.Values) == 0 {
-			return nil, fmt.Errorf("unexpected empty values for enum column %q.%q", t.Name, c.Name)
+			return nil, fmt.Errorf("unexpected empty values for enum column %q.%q", p.Table.Name, p.Column.Name)
 		}
 		implicitUpdate("enum", strconv.Quote(ct.Values[0]))
 	case *mysql.SetType:
@@ -154,8 +102,8 @@ func (d *DataDepend) addColumn(p *sqlcheck.Pass, change *sqlcheck.Change, t *sch
 			case drv.Maria():
 				// Maria has a special behavior for the first TIMESTAMP column.
 				// See: https://mariadb.com/kb/en/timestamp/#automatic-values
-				for i := 0; i < len(t.Columns) && t.Columns[i].Name != c.Name; i++ {
-					tt, err := mysql.FormatType(t.Columns[i].Type.Type)
+				for i := 0; i < len(p.Table.Columns) && p.Table.Columns[i].Name != p.Column.Name; i++ {
+					tt, err := mysql.FormatType(p.Table.Columns[i].Type.Type)
 					if err != nil {
 						return nil, err
 					}
@@ -185,13 +133,9 @@ func (d *DataDepend) addColumn(p *sqlcheck.Pass, change *sqlcheck.Change, t *sch
 
 func init() {
 	sqlcheck.Register(mysql.DriverName, func(r *schemahcl.Resource) (sqlcheck.Analyzer, error) {
-		az, err := NewDataDepend(r)
-		if err != nil {
-			return nil, err
-		}
 		return sqlcheck.Analyzers{
 			destructive.New(destructive.Options{}),
-			az,
+			NewDataDepend(nil),
 		}, nil
 	})
 }
diff --git a/sql/postgres/postgrescheck/postgrescheck.go b/sql/postgres/postgrescheck/postgrescheck.go
index 12f94441..461a93dd 100644
--- a/sql/postgres/postgrescheck/postgrescheck.go
+++ b/sql/postgres/postgrescheck/postgrescheck.go
@@ -5,18 +5,41 @@
 package sqlitecheck
 
 import (
+	"fmt"
+
 	"ariga.io/atlas/schemahcl"
+	"ariga.io/atlas/sql/postgres"
 	"ariga.io/atlas/sql/sqlcheck"
 	"ariga.io/atlas/sql/sqlcheck/datadepend"
 	"ariga.io/atlas/sql/sqlcheck/destructive"
-	"ariga.io/atlas/sql/sqlite"
 )
 
+// NewDataDepend creates new data-depend analyzer.
+func NewDataDepend(*schemahcl.Resource) *datadepend.Analyzer {
+	var opts datadepend.Options
+	opts.Handler.AddNotNull = func(p *datadepend.ColumnPass) ([]sqlcheck.Diagnostic, error) {
+		tt, err := postgres.FormatType(p.Column.Type.Type)
+		if err != nil {
+			return nil, err
+		}
+		return []sqlcheck.Diagnostic{
+			{
+				Pos: p.Change.Pos,
+				Text: fmt.Sprintf(
+					"Adding a non-nullable %q column %q will fail in case table %q is not empty",
+					tt, p.Column.Name, p.Table.Name,
+				),
+			},
+		}, nil
+	}
+	return datadepend.New(opts)
+}
+
 func init() {
-	sqlcheck.Register(sqlite.DriverName, func(*schemahcl.Resource) (sqlcheck.Analyzer, error) {
+	sqlcheck.Register(postgres.DriverName, func(*schemahcl.Resource) (sqlcheck.Analyzer, error) {
 		return sqlcheck.Analyzers{
 			destructive.New(destructive.Options{}),
-			datadepend.New(datadepend.Options{}),
+			NewDataDepend(nil),
 		}, nil
 	})
 }
diff --git a/sql/sqlcheck/datadepend/datadepend.go b/sql/sqlcheck/datadepend/datadepend.go
index b53693fe..6aeca413 100644
--- a/sql/sqlcheck/datadepend/datadepend.go
+++ b/sql/sqlcheck/datadepend/datadepend.go
@@ -8,7 +8,6 @@ import (
 	"context"
 	"fmt"
 
-	"ariga.io/atlas/schemahcl"
 	"ariga.io/atlas/sql/schema"
 	"ariga.io/atlas/sql/sqlcheck"
 )
@@ -21,26 +20,49 @@ type (
 		// addition of unique indexes to tables that can cause migration to fail.
 		UniqueIndex *bool `spec:"drop_schema,omitempty"`
 
-		// Allow drivers to extend the configuration.
-		schemahcl.DefaultExtension
+		// NotNull indicates if the analyzer should check for modification
+		// or addition of NOT NULL constraints to columns.
+		NotNull *bool `spec:"not_null,omitempty"`
+
+		// Underlying driver handlers.
+		Handler struct {
+			// AddNotNull is applied when a new non-nullable column was
+			// added to an existing table.
+			AddNotNull ColumnHandler
+		}
 	}
 
 	// Analyzer checks data-dependent changes.
 	Analyzer struct {
 		Options
 	}
+
+	// ColumnPass wraps the information needed
+	// by the handler below to diagnose columns.
+	ColumnPass struct {
+		*sqlcheck.Pass
+		Change *sqlcheck.Change // Change context (statement).
+		Table  *schema.Table    // The table this column belongs to.
+		Column *schema.Column   // The diagnosed column.
+	}
+
+	// ColumnHandler allows provide custom diagnostic for specific column rules.
+	ColumnHandler func(*ColumnPass) ([]sqlcheck.Diagnostic, error)
 )
 
 // New creates a new data-dependant analyzer with the given options.
 func New(opts Options) *Analyzer {
-	unique := true
+	notnull, unique := true, true
+	if opts.NotNull != nil {
+		notnull = *opts.NotNull
+	}
 	if opts.UniqueIndex != nil {
 		unique = *opts.UniqueIndex
 	}
-	return &Analyzer{Options: Options{UniqueIndex: &unique}}
+	return &Analyzer{Options: Options{UniqueIndex: &unique, NotNull: &notnull, Handler: opts.Handler}}
 }
 
-// Analyze implements sqlcheck.Analyzer.
+// Analyze runs data-depend analysis on MySQL changes.
 func (a *Analyzer) Analyze(ctx context.Context, p *sqlcheck.Pass) error {
 	a.Report(p, a.Diagnostics(ctx, p))
 	return nil
@@ -48,10 +70,6 @@ func (a *Analyzer) Analyze(ctx context.Context, p *sqlcheck.Pass) error {
 
 // Diagnostics runs the common analysis on the file and returns its diagnostics.
 func (a *Analyzer) Diagnostics(_ context.Context, p *sqlcheck.Pass) (diags []sqlcheck.Diagnostic) {
-	// Skip running the analysis in case the check is disabled.
-	if !*a.UniqueIndex {
-		return
-	}
 	for _, sc := range p.File.Changes {
 		for _, c := range sc.Changes {
 			m, ok := c.(*schema.ModifyTable)
@@ -72,19 +90,29 @@ func (a *Analyzer) Diagnostics(_ context.Context, p *sqlcheck.Pass) (diags []sql
 						return nil
 					}()
 					// A unique index was added on an existing column.
-					if c.I.Unique && column != nil {
+					if *a.UniqueIndex && c.I.Unique && column != nil {
 						diags = append(diags, sqlcheck.Diagnostic{
 							Pos:  sc.Pos,
 							Text: fmt.Sprintf("Adding a unique index %q on table %q might fail in case column %q contains duplicate entries", c.I.Name, m.T.Name, column.Name),
 						})
 					}
 				case *schema.ModifyIndex:
-					if c.Change.Is(schema.ChangeUnique) && c.To.Unique && p.File.IndexSpan(m.T, c.To)&sqlcheck.SpanAdded == 0 {
+					if *a.UniqueIndex && c.Change.Is(schema.ChangeUnique) && c.To.Unique && p.File.IndexSpan(m.T, c.To)&sqlcheck.SpanAdded == 0 {
 						diags = append(diags, sqlcheck.Diagnostic{
 							Pos:  sc.Pos,
 							Text: fmt.Sprintf("Modifying an index %q on table %q might fail in case of duplicate entries", c.To.Name, m.T.Name),
 						})
 					}
+				case *schema.AddColumn:
+					// In case the column is nullable without default
+					// value and the table was not added in this file.
+					if *a.NotNull && a.Handler.AddNotNull != nil && !c.C.Type.Null && c.C.Default == nil && p.File.TableSpan(m.T)&sqlcheck.SpanAdded != 1 {
+						d, err := a.Handler.AddNotNull(&ColumnPass{Pass: p, Change: sc, Table: m.T, Column: c.C})
+						if err != nil {
+							return
+						}
+						diags = append(diags, d...)
+					}
 				}
 			}
 		}
diff --git a/sql/sqlite/sqlitecheck/sqlitecheck.go b/sql/sqlite/sqlitecheck/sqlitecheck.go
index 9075c5e5..b0e024d4 100644
--- a/sql/sqlite/sqlitecheck/sqlitecheck.go
+++ b/sql/sqlite/sqlitecheck/sqlitecheck.go
@@ -6,6 +6,7 @@ package sqlitecheck
 
 import (
 	"context"
+	"fmt"
 	"strings"
 
 	"ariga.io/atlas/schemahcl"
@@ -16,6 +17,27 @@ import (
 	"ariga.io/atlas/sql/sqlite"
 )
 
+// NewDataDepend creates new data-depend analyzer.
+func NewDataDepend(*schemahcl.Resource) *datadepend.Analyzer {
+	var opts datadepend.Options
+	opts.Handler.AddNotNull = func(p *datadepend.ColumnPass) ([]sqlcheck.Diagnostic, error) {
+		tt, err := sqlite.FormatType(p.Column.Type.Type)
+		if err != nil {
+			return nil, err
+		}
+		return []sqlcheck.Diagnostic{
+			{
+				Pos: p.Change.Pos,
+				Text: fmt.Sprintf(
+					"Adding a non-nullable %q column %q will fail in case table %q is not empty",
+					tt, p.Column.Name, p.Table.Name,
+				),
+			},
+		}, nil
+	}
+	return datadepend.New(opts)
+}
+
 func init() {
 	sqlcheck.Register(sqlite.DriverName, func(*schemahcl.Resource) (sqlcheck.Analyzer, error) {
 		return sqlcheck.Analyzers{
@@ -55,7 +77,7 @@ func init() {
 				return nil
 			}),
 			destructive.New(destructive.Options{}),
-			datadepend.New(datadepend.Options{}),
+			NewDataDepend(nil),
 		}, nil
 	})
 }


Test patch:
diff --git a/internal/integration/testdata/sqlite/cli-migrate-lint-add-notnull.txt b/internal/integration/testdata/sqlite/cli-migrate-lint-add-notnull.txt
new file mode 100644
index 00000000..e3ad8681
--- /dev/null
+++ b/internal/integration/testdata/sqlite/cli-migrate-lint-add-notnull.txt
@@ -0,0 +1,19 @@
+atlas migrate lint --dir file://migrations --dev-url URL --latest=2 > got1.txt
+cmp got1.txt expected1.txt
+
+-- migrations/1.sql --
+CREATE TABLE users (id int);
+
+/* Adding a not-null column without default to a table created in this file should not report. */
+ALTER TABLE users ADD COLUMN c1 int NOT NULL;
+
+-- migrations/2.sql --
+ALTER TABLE users ADD COLUMN c2 int NOT NULL;
+
+ALTER TABLE users ADD COLUMN c3 int NOT NULL DEFAULT 1;
+
+-- expected1.txt --
+Data dependent changes detected in file 2.sql:
+
+	L1: Adding a non-nullable "int" column "c2" will fail in case table "users" is not empty
+
diff --git a/sql/mysql/mysqlcheck/mysqlcheck_test.go b/sql/mysql/mysqlcheck/mysqlcheck_test.go
index f8671513..3fcac4d7 100644
--- a/sql/mysql/mysqlcheck/mysqlcheck_test.go
+++ b/sql/mysql/mysqlcheck/mysqlcheck_test.go
@@ -60,8 +60,7 @@ func TestDataDepend_MySQL_ImplicitUpdate(t *testing.T) {
 			}),
 		}
 	)
-	az, err := mysqlcheck.NewDataDepend(nil)
-	require.NoError(t, err)
+	az := mysqlcheck.NewDataDepend(nil)
 	require.NoError(t, az.Analyze(context.Background(), pass))
 	require.Equal(t, report.Diagnostics[0].Text, `Adding a non-nullable "int" column "b" on table "users" without a default value implicitly sets existing rows with 0`)
 	require.Equal(t, report.Diagnostics[1].Text, `Adding a non-nullable "float" column "c" on table "users" without a default value implicitly sets existing rows with 0`)
@@ -106,8 +105,7 @@ func TestDataDepend_MySQL8_ImplicitUpdate(t *testing.T) {
 			}),
 		}
 	)
-	az, err := mysqlcheck.NewDataDepend(nil)
-	require.NoError(t, err)
+	az := mysqlcheck.NewDataDepend(nil)
 	require.NoError(t, az.Analyze(context.Background(), pass))
 	require.Equal(t,
 		report.Diagnostics[0].Text,
@@ -154,8 +152,7 @@ func TestDataDepend_MySQL_MightFail(t *testing.T) {
 			}),
 		}
 	)
-	az, err := mysqlcheck.NewDataDepend(nil)
-	require.NoError(t, err)
+	az := mysqlcheck.NewDataDepend(nil)
 	require.NoError(t, az.Analyze(context.Background(), pass))
 	require.Equal(t, report.Diagnostics[0].Text, `Adding a non-nullable "date" column "b" will fail in case table "users" is not empty`)
 	require.Equal(t, report.Diagnostics[1].Text, `Adding a non-nullable "datetime" column "c" will fail in case table "users" is not empty`)
@@ -210,8 +207,7 @@ func TestDataDepend_Maria_ImplicitUpdate(t *testing.T) {
 			}),
 		}
 	)
-	az, err := mysqlcheck.NewDataDepend(nil)
-	require.NoError(t, err)
+	az := mysqlcheck.NewDataDepend(nil)
 	require.NoError(t, az.Analyze(context.Background(), pass))
 	require.Equal(t, report.Diagnostics[0].Text, `Adding a non-nullable "text" column "b" on table "users" without a default value implicitly sets existing rows with ""`)
 	require.Equal(t, report.Diagnostics[1].Text, `Adding a non-nullable "json" column "c" on table "users" without a default value implicitly sets existing rows with ""`)