Repository: ariga/atlas
Pull request #1084 (2022-08-22 11:18:21)

sql/sqlspec/destructive: enable error by default


Patch:
diff --git a/cmd/atlas/internal/lint/run.go b/cmd/atlas/internal/lint/run.go
index 64316e86..a93bf250 100644
--- a/cmd/atlas/internal/lint/run.go
+++ b/cmd/atlas/internal/lint/run.go
@@ -45,7 +45,17 @@ type Runner struct {
 func (r *Runner) Run(ctx context.Context) error {
 	switch err := r.summary(ctx); err.(type) {
 	case nil:
-		return r.ReportWriter.WriteReport(r.sum)
+		if err := r.ReportWriter.WriteReport(r.sum); err != nil {
+			return err
+		}
+		// If any of the analyzers returns
+		// an error, fail silently.
+		for _, f := range r.sum.Files {
+			if f.Error != "" {
+				return SilentError{}
+			}
+		}
+		return nil
 	case *FileError:
 		if err := r.ReportWriter.WriteReport(r.sum); err != nil {
 			return err
@@ -129,7 +139,8 @@ var (
 			Funcs(TemplateFuncs).
 			Parse(`
 {{- range $f := .Files }}
-	{{- if $f.Error }}
+	{{- /* If there is an error but not diagnostics, print it. */}}
+	{{- if and $f.Error (not $f.Reports) }}
 		{{- printf "%s: %s\n" $f.Name $f.Error }}
 	{{- else }}
 		{{- range $r := $f.Reports }}
diff --git a/schemahcl/spec.go b/schemahcl/spec.go
index dd0129ce..1652ffa2 100644
--- a/schemahcl/spec.go
+++ b/schemahcl/spec.go
@@ -196,6 +196,9 @@ func (a *Attr) Bools() ([]bool, error) {
 
 // Resource returns the first child Resource by its type and reports whether it was found.
 func (r *Resource) Resource(t string) (*Resource, bool) {
+	if r == nil {
+		return nil, false
+	}
 	for i := range r.Children {
 		if r.Children[i].Type == t {
 			return r.Children[i], true
diff --git a/sql/mysql/mysqlcheck/mysqlcheck.go b/sql/mysql/mysqlcheck/mysqlcheck.go
index 2bc249ac..9cd7f5a9 100644
--- a/sql/mysql/mysqlcheck/mysqlcheck.go
+++ b/sql/mysql/mysqlcheck/mysqlcheck.go
@@ -133,9 +133,10 @@ func addNotNull(p *datadepend.ColumnPass) (diags []sqlcheck.Diagnostic, err erro
 
 func init() {
 	sqlcheck.Register(mysql.DriverName, func(r *schemahcl.Resource) (sqlcheck.Analyzer, error) {
-		return sqlcheck.Analyzers{
-			destructive.New(destructive.Options{}),
-			NewDataDepend(nil),
-		}, nil
+		d, err := destructive.New(r)
+		if err != nil {
+			return nil, err
+		}
+		return sqlcheck.Analyzers{d, NewDataDepend(nil)}, nil
 	})
 }
diff --git a/sql/postgres/postgrescheck/postgrescheck.go b/sql/postgres/postgrescheck/postgrescheck.go
index 461a93dd..ad1496f8 100644
--- a/sql/postgres/postgrescheck/postgrescheck.go
+++ b/sql/postgres/postgrescheck/postgrescheck.go
@@ -36,10 +36,11 @@ func NewDataDepend(*schemahcl.Resource) *datadepend.Analyzer {
 }
 
 func init() {
-	sqlcheck.Register(postgres.DriverName, func(*schemahcl.Resource) (sqlcheck.Analyzer, error) {
-		return sqlcheck.Analyzers{
-			destructive.New(destructive.Options{}),
-			NewDataDepend(nil),
-		}, nil
+	sqlcheck.Register(postgres.DriverName, func(r *schemahcl.Resource) (sqlcheck.Analyzer, error) {
+		d, err := destructive.New(r)
+		if err != nil {
+			return nil, err
+		}
+		return sqlcheck.Analyzers{d, NewDataDepend(nil)}, nil
 	})
 }
diff --git a/sql/sqlcheck/destructive/destructive.go b/sql/sqlcheck/destructive/destructive.go
index 65df5bcc..a6e1479e 100644
--- a/sql/sqlcheck/destructive/destructive.go
+++ b/sql/sqlcheck/destructive/destructive.go
@@ -6,6 +6,7 @@ package destructive
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"strings"
 
@@ -15,36 +16,21 @@ import (
 	"ariga.io/atlas/sql/sqlcheck"
 )
 
-type (
-	// Options defines the configuration options
-	// for the destructive changes checker.
-	Options struct {
-		// DropSchema indicates if the analyzer should check for schema dropping.
-		DropSchema *bool `spec:"drop_schema,omitempty"`
-
-		// DropTable indicates if the analyzer should check for table dropping.
-		DropTable *bool `spec:"drop_schema,omitempty"`
-
-		// DropColumn indicates if the analyzer should check for column dropping.
-		DropColumn *bool `spec:"drop_column,omitempty"`
-
-		// Allow drivers to extend the configuration.
-		schemahcl.DefaultExtension
-	}
-
-	// Analyzer checks for destructive changes.
-	Analyzer struct {
-		Options
-	}
-)
+// Analyzer checks for destructive changes.
+type Analyzer struct {
+	sqlcheck.Options
+}
 
 // New creates a new destructive changes Analyzer with the given options.
-func New(opts Options) *Analyzer {
+func New(r *schemahcl.Resource) (*Analyzer, error) {
 	d := &Analyzer{}
-	d.DropSchema = abool(opts.DropSchema, true)
-	d.DropTable = abool(opts.DropTable, true)
-	d.DropColumn = abool(opts.DropColumn, true)
-	return d
+	d.Error = true
+	if r, ok := r.Resource("destructive"); ok {
+		if err := r.As(&d.Options); err != nil {
+			return nil, fmt.Errorf("sql/sqlcheck: parsing destructive check options: %w", err)
+		}
+	}
+	return d, nil
 }
 
 // Analyze implements sqlcheck.Analyzer.
@@ -54,7 +40,7 @@ func (a *Analyzer) Analyze(_ context.Context, p *sqlcheck.Pass) error {
 		for _, c := range sc.Changes {
 			switch c := c.(type) {
 			case *schema.DropSchema:
-				if *a.DropSchema && p.File.SchemaSpan(c.S) != sqlcheck.SpanTemporary {
+				if p.File.SchemaSpan(c.S) != sqlcheck.SpanTemporary {
 					var text string
 					switch n := len(c.S.Tables); {
 					case n == 0:
@@ -67,16 +53,13 @@ func (a *Analyzer) Analyze(_ context.Context, p *sqlcheck.Pass) error {
 					diags = append(diags, sqlcheck.Diagnostic{Pos: sc.Pos, Text: text})
 				}
 			case *schema.DropTable:
-				if *a.DropTable && p.File.SchemaSpan(c.T.Schema) != sqlcheck.SpanDropped && p.File.TableSpan(c.T) != sqlcheck.SpanTemporary {
+				if p.File.SchemaSpan(c.T.Schema) != sqlcheck.SpanDropped && p.File.TableSpan(c.T) != sqlcheck.SpanTemporary {
 					diags = append(diags, sqlcheck.Diagnostic{
 						Pos:  sc.Pos,
 						Text: fmt.Sprintf("Dropping table %q", c.T.Name),
 					})
 				}
 			case *schema.ModifyTable:
-				if !*a.DropColumn {
-					continue
-				}
 				for i := range c.Changes {
 					d, ok := c.Changes[i].(*schema.DropColumn)
 					if !ok || p.File.ColumnSpan(c.T, d.C) == sqlcheck.SpanTemporary {
@@ -93,17 +76,11 @@ func (a *Analyzer) Analyze(_ context.Context, p *sqlcheck.Pass) error {
 		}
 	}
 	if len(diags) > 0 {
-		p.Reporter.WriteReport(sqlcheck.Report{
-			Text:        fmt.Sprintf("Destructive changes detected in file %s", p.File.Name()),
-			Diagnostics: diags,
-		})
+		text := fmt.Sprintf("Destructive changes detected in file %s", p.File.Name())
+		p.Reporter.WriteReport(sqlcheck.Report{Text: text, Diagnostics: diags})
+		if a.Error {
+			return errors.New(text)
+		}
 	}
 	return nil
 }
-
-func abool(p *bool, v bool) *bool {
-	if p != nil {
-		return p
-	}
-	return &v
-}
diff --git a/sql/sqlcheck/sqlcheck.go b/sql/sqlcheck/sqlcheck.go
index 08a8fe71..5cf08bb1 100644
--- a/sql/sqlcheck/sqlcheck.go
+++ b/sql/sqlcheck/sqlcheck.go
@@ -79,6 +79,16 @@ type (
 	ReportWriter interface {
 		WriteReport(Report)
 	}
+
+	// Options defines a generic configuration options for analyzers.
+	Options struct {
+		// Error indicates if an analyzer should
+		// error in case a Diagnostic was found.
+		Error bool `spec:"error"`
+
+		// Allow drivers to extend the configuration.
+		schemahcl.DefaultExtension
+	}
 )
 
 // Analyzers implements Analyzer.
diff --git a/sql/sqlite/sqlitecheck/sqlitecheck.go b/sql/sqlite/sqlitecheck/sqlitecheck.go
index b0e024d4..00d4cd71 100644
--- a/sql/sqlite/sqlitecheck/sqlitecheck.go
+++ b/sql/sqlite/sqlitecheck/sqlitecheck.go
@@ -39,7 +39,11 @@ func NewDataDepend(*schemahcl.Resource) *datadepend.Analyzer {
 }
 
 func init() {
-	sqlcheck.Register(sqlite.DriverName, func(*schemahcl.Resource) (sqlcheck.Analyzer, error) {
+	sqlcheck.Register(sqlite.DriverName, func(r *schemahcl.Resource) (sqlcheck.Analyzer, error) {
+		dest, err := destructive.New(r)
+		if err != nil {
+			return nil, err
+		}
 		return sqlcheck.Analyzers{
 			sqlcheck.AnalyzerFunc(func(ctx context.Context, p *sqlcheck.Pass) error {
 				var changes []*sqlcheck.Change
@@ -76,7 +80,7 @@ func init() {
 				p.File.Changes = changes
 				return nil
 			}),
-			destructive.New(destructive.Options{}),
+			dest,
 			NewDataDepend(nil),
 		}, nil
 	})


Test patch:
diff --git a/cmd/atlas/internal/cmdapi/migrate_test.go b/cmd/atlas/internal/cmdapi/migrate_test.go
index b669ca31..b13f48fb 100644
--- a/cmd/atlas/internal/cmdapi/migrate_test.go
+++ b/cmd/atlas/internal/cmdapi/migrate_test.go
@@ -489,7 +489,7 @@ func TestMigrate_Lint(t *testing.T) {
 		"--dev-url", openSQLite(t, ""),
 		"--latest", "1",
 	)
-	require.NoError(t, err)
+	require.Error(t, err)
 	require.Equal(t, "Destructive changes detected in file 2.sql:\n\n\tL1: Dropping table \"t\"\n\n", s)
 	s, err = runCmd(
 		Root, "migrate", "lint",
@@ -498,7 +498,7 @@ func TestMigrate_Lint(t *testing.T) {
 		"--latest", "1",
 		"--log", "{{ range .Files }}{{ .Name }}{{ end }}",
 	)
-	require.NoError(t, err)
+	require.Error(t, err)
 	require.Equal(t, "2.sql", s)
 
 	// Change files to golang-migrate format.
diff --git a/internal/integration/script_test.go b/internal/integration/script_test.go
index d9b97395..36e4244d 100644
--- a/internal/integration/script_test.go
+++ b/internal/integration/script_test.go
@@ -400,7 +400,7 @@ func cmdCLI(ts *testscript.TestScript, neg bool, args []string, dbURL, cliPath s
 		stderr := &bytes.Buffer{}
 		cmd.Stderr = stderr
 		cmd.Dir = workDir
-		if err := cmd.Run(); err != nil {
+		if err := cmd.Run(); err != nil && !neg {
 			ts.Fatalf("\n[stderr]\n%s", stderr)
 		}
 	default:
diff --git a/internal/integration/testdata/mysql/cli-migrate-lint-destructive.txt b/internal/integration/testdata/mysql/cli-migrate-lint-destructive.txt
index a75fa599..1a73b753 100644
--- a/internal/integration/testdata/mysql/cli-migrate-lint-destructive.txt
+++ b/internal/integration/testdata/mysql/cli-migrate-lint-destructive.txt
@@ -6,7 +6,8 @@ cmp got.txt empty.txt
 atlas migrate lint --dir file://migrations2 --dev-url URL --latest=1 > got.txt
 cmp got.txt empty.txt
 
-atlas migrate lint --dir file://migrations3 --dev-url URL --latest=1 > got.txt
+# Expect the command to fail; exit code 1.
+! atlas migrate lint --dir file://migrations3 --dev-url URL --latest=1 > got.txt
 cmp got.txt expected3.txt
 
 -- empty.txt --
diff --git a/internal/integration/testdata/sqlite/cli-migrate-lint-destructive.txt b/internal/integration/testdata/sqlite/cli-migrate-lint-destructive.txt
index 0df6df76..bcc59a29 100644
--- a/internal/integration/testdata/sqlite/cli-migrate-lint-destructive.txt
+++ b/internal/integration/testdata/sqlite/cli-migrate-lint-destructive.txt
@@ -1,7 +1,9 @@
-atlas migrate lint --dir file://migrations --dev-url URL --latest=1 > got1.txt
+# Expect the command to fail; exit code 1.
+! atlas migrate lint --dir file://migrations --dev-url URL --latest=1 > got1.txt
 cmp got1.txt expected1.txt
 
-atlas migrate lint --dir file://migrations --dev-url URL --latest=2 > got2.txt
+# Expect the command to fail; exit code 1.
+! atlas migrate lint --dir file://migrations --dev-url URL --latest=2 > got2.txt
 cmp got2.txt expected2.txt
 
 -- migrations/1.sql --
diff --git a/internal/integration/testdata/sqlite/cli-migrate-lint-minimal-env.txt b/internal/integration/testdata/sqlite/cli-migrate-lint-minimal-env.txt
index 95aa6113..02d59c3f 100644
--- a/internal/integration/testdata/sqlite/cli-migrate-lint-minimal-env.txt
+++ b/internal/integration/testdata/sqlite/cli-migrate-lint-minimal-env.txt
@@ -1,5 +1,7 @@
-atlas migrate lint --env local --latest=2 > got1.txt
+# Expect the command to fail; exit code 1.
+! atlas migrate lint --env local --latest=2 > got1.txt
 cmp got1.txt expected1.txt
+
 -- atlas.hcl --
 env "local" {
     dev = "URL"
diff --git a/sql/sqlcheck/destructive/destructive_test.go b/sql/sqlcheck/destructive/destructive_test.go
index f6522b47..730eefa2 100644
--- a/sql/sqlcheck/destructive/destructive_test.go
+++ b/sql/sqlcheck/destructive/destructive_test.go
@@ -8,6 +8,8 @@ import (
 	"context"
 	"testing"
 
+	"ariga.io/atlas/schemahcl"
+	"ariga.io/atlas/sql/internal/specutil"
 	"ariga.io/atlas/sql/migrate"
 	"ariga.io/atlas/sql/schema"
 	"ariga.io/atlas/sql/sqlcheck"
@@ -59,9 +61,10 @@ func TestAnalyzer_DropTable(t *testing.T) {
 			}),
 		}
 	)
-	az := destructive.New(destructive.Options{})
-	err := az.Analyze(context.Background(), pass)
+	az, err := destructive.New(nil)
 	require.NoError(t, err)
+	err = az.Analyze(context.Background(), pass)
+	require.Error(t, err)
 	require.Equal(t, `Destructive changes detected in file 1.sql`, report.Text)
 	require.Len(t, report.Diagnostics, 2)
 	require.Equal(t, `Dropping table "users"`, report.Diagnostics[0].Text)
@@ -96,8 +99,9 @@ func TestAnalyzer_SkipTemporaryTable(t *testing.T) {
 			}),
 		}
 	)
-	az := destructive.New(destructive.Options{})
-	err := az.Analyze(context.Background(), pass)
+	az, err := destructive.New(nil)
+	require.NoError(t, err)
+	err = az.Analyze(context.Background(), pass)
 	require.NoError(t, err)
 	require.Nil(t, report, "no report")
 }
@@ -145,8 +149,18 @@ func TestAnalyzer_DropSchema(t *testing.T) {
 			}),
 		}
 	)
-	az := destructive.New(destructive.Options{})
-	err := az.Analyze(context.Background(), pass)
+	az, err := destructive.New(&schemahcl.Resource{
+		Children: []*schemahcl.Resource{
+			{
+				Type: "destructive",
+				Attrs: []*schemahcl.Attr{
+					specutil.BoolAttr("error", false),
+				},
+			},
+		},
+	})
+	require.NoError(t, err)
+	err = az.Analyze(context.Background(), pass)
 	require.NoError(t, err)
 	require.Equal(t, `Destructive changes detected in file 1.sql`, report.Text)
 	require.Len(t, report.Diagnostics, 2)
@@ -198,66 +212,21 @@ func TestAnalyzer_DropColumn(t *testing.T) {
 			}),
 		}
 	)
-	az := destructive.New(destructive.Options{})
-	err := az.Analyze(context.Background(), pass)
-	require.NoError(t, err)
-	require.Len(t, report.Diagnostics, 1)
-	require.Equal(t, `Dropping non-virtual column "c"`, report.Diagnostics[0].Text)
-}
-
-func TestAnalyzer_Options(t *testing.T) {
-	var (
-		off  bool
-		pass = &sqlcheck.Pass{
-			Dev: &sqlclient.Client{Name: "mysql"},
-			File: &sqlcheck.File{
-				File: testFile{name: "1.sql"},
-				Changes: []*sqlcheck.Change{
-					{
-						Stmt: "DROP DATABASE `test`",
-						Changes: schema.Changes{
-							&schema.DropSchema{
-								S: schema.New("test"),
-							},
-						},
-					},
-					{
-						Stmt: "DROP TABLE `users`",
-						Changes: schema.Changes{
-							&schema.DropTable{
-								T: schema.NewTable("users").
-									SetSchema(schema.New("test")),
-							},
-						},
-					},
-					{
-						Stmt: "ALTER TABLE `pets`",
-						Changes: []schema.Change{
-							&schema.ModifyTable{
-								T: schema.NewTable("pets").
-									SetSchema(schema.New("test")),
-								Changes: schema.Changes{
-									&schema.DropColumn{
-										C: schema.NewColumn("c"),
-									},
-								},
-							},
-						},
-					},
+	az, err := destructive.New(&schemahcl.Resource{
+		Children: []*schemahcl.Resource{
+			{
+				Type: "destructive",
+				Attrs: []*schemahcl.Attr{
+					specutil.BoolAttr("error", false),
 				},
 			},
-			Reporter: sqlcheck.ReportWriterFunc(func(r sqlcheck.Report) {
-				t.Fatal("unexpected report")
-			}),
-		}
-	)
-	az := destructive.New(destructive.Options{
-		DropTable:  &off,
-		DropSchema: &off,
-		DropColumn: &off,
+		},
 	})
-	err := az.Analyze(context.Background(), pass)
 	require.NoError(t, err)
+	err = az.Analyze(context.Background(), pass)
+	require.NoError(t, err)
+	require.Len(t, report.Diagnostics, 1)
+	require.Equal(t, `Dropping non-virtual column "c"`, report.Diagnostics[0].Text)
 }
 
 type testFile struct {
diff --git a/sql/sqlite/sqlitecheck/sqlitecheck_test.go b/sql/sqlite/sqlitecheck/sqlitecheck_test.go
index 16b6e0b3..f05734a9 100644
--- a/sql/sqlite/sqlitecheck/sqlitecheck_test.go
+++ b/sql/sqlite/sqlitecheck/sqlitecheck_test.go
@@ -110,7 +110,8 @@ func TestDetectModifyTable(t *testing.T) {
 	)
 	az, err := sqlcheck.AnalyzerFor(sqlite.DriverName, nil)
 	require.NoError(t, err)
-	require.NoError(t, az.Analyze(context.Background(), pass))
+	err = az.Analyze(context.Background(), pass)
+	require.EqualError(t, err, "Destructive changes detected in file 1.sql")
 
 	require.Equal(t, report.Text, "Destructive changes detected in file 1.sql")
 	require.Len(t, report.Diagnostics, 3)