Repository: ariga/atlas
Pull request #1516 (2023-03-08 11:47:23)

sql/sqlite/sqlitecheck: add backwards-incompatible checks


Patch:
diff --git a/cmd/atlas/internal/sqlparse/myparse/myparse.go b/cmd/atlas/internal/sqlparse/myparse/myparse.go
index 89daa830..e0b12c6c 100644
--- a/cmd/atlas/internal/sqlparse/myparse/myparse.go
+++ b/cmd/atlas/internal/sqlparse/myparse/myparse.go
@@ -155,7 +155,7 @@ func (p *Parser) ColumnHasReferences(stmt *migrate.Stmt, c1 *schema.Column) (boo
 	return false, nil
 }
 
-// CreateViewAfter checks if the view was created after the position with the given name to a table.
+// CreateViewAfter checks if a view was created after the position with the given name to a table.
 func (p *Parser) CreateViewAfter(f migrate.File, old, new string, pos int) (bool, error) {
 	pr := parser.New()
 	return parseutil.MatchStmtAfter(f, pos, func(s *migrate.Stmt) (bool, error) {
diff --git a/cmd/atlas/internal/sqlparse/parseutil/parseutil.go b/cmd/atlas/internal/sqlparse/parseutil/parseutil.go
index 4985a1ee..bf7995b9 100644
--- a/cmd/atlas/internal/sqlparse/parseutil/parseutil.go
+++ b/cmd/atlas/internal/sqlparse/parseutil/parseutil.go
@@ -64,8 +64,8 @@ func RenameIndex(modify *schema.ModifyTable, r *Rename) {
 
 // RenameTable patches DROP/ADD table commands to RENAME.
 func RenameTable(changes schema.Changes, r *Rename) schema.Changes {
-	i := changes.IndexDropTable(r.From)
-	j := changes.IndexAddTable(r.To)
+	i := changes.LastIndexDropTable(r.From)
+	j := changes.LastIndexAddTable(r.To)
 	if i != -1 && j != -1 {
 		changes[max(i, j)] = &schema.RenameTable{
 			From: changes[i].(*schema.DropTable).T,
diff --git a/cmd/atlas/internal/sqlparse/pgparse/pgparse.go b/cmd/atlas/internal/sqlparse/pgparse/pgparse.go
index 40cd6795..615e899a 100644
--- a/cmd/atlas/internal/sqlparse/pgparse/pgparse.go
+++ b/cmd/atlas/internal/sqlparse/pgparse/pgparse.go
@@ -50,7 +50,7 @@ func (p *Parser) ColumnFilledBefore(f migrate.File, t *schema.Table, c *schema.C
 	})
 }
 
-// CreateViewAfter checks if the view was created after the position with the given name to a table.
+// CreateViewAfter checks if a view was created after the position with the given name to a table.
 func (p *Parser) CreateViewAfter(f migrate.File, old, new string, pos int) (bool, error) {
 	return parseutil.MatchStmtAfter(f, pos, func(s *migrate.Stmt) (bool, error) {
 		stmt, err := parser.ParseOne(s.Text)
diff --git a/cmd/atlas/internal/sqlparse/sqliteparse/sqliteparse.go b/cmd/atlas/internal/sqlparse/sqliteparse/sqliteparse.go
index b053c7d4..9daa4e21 100644
--- a/cmd/atlas/internal/sqlparse/sqliteparse/sqliteparse.go
+++ b/cmd/atlas/internal/sqlparse/sqliteparse/sqliteparse.go
@@ -7,6 +7,7 @@ package sqliteparse
 import (
 	"errors"
 	"fmt"
+	"strconv"
 	"strings"
 
 	"ariga.io/atlas/cmd/atlas/internal/sqlparse/parseutil"
@@ -92,8 +93,8 @@ func (s *Stmt) RenameColumn() (*parseutil.Rename, bool) {
 		return nil, false
 	}
 	return &parseutil.Rename{
-		From: alter.old_column_name.GetText(),
-		To:   alter.new_column_name.GetText(),
+		From: unquote(alter.old_column_name.GetText()),
+		To:   unquote(alter.new_column_name.GetText()),
 	}, true
 }
 
@@ -107,8 +108,8 @@ func (s *Stmt) RenameTable() (*parseutil.Rename, bool) {
 		return nil, false
 	}
 	return &parseutil.Rename{
-		From: alter.Table_name(0).GetText(),
-		To:   alter.new_table_name.GetText(),
+		From: unquote(alter.Table_name(0).GetText()),
+		To:   unquote(alter.new_table_name.GetText()),
 	}, true
 }
 
@@ -128,6 +129,18 @@ func (s *Stmt) TableUpdate(t *schema.Table) (*Update_stmtContext, bool) {
 	return u, true
 }
 
+// CreateView reports if the statement is a CREATE VIEW command with the given name.
+func (s *Stmt) CreateView(name string) (*Create_view_stmtContext, bool) {
+	if s.stmt.GetChildCount() != 1 {
+		return nil, false
+	}
+	v, ok := s.stmt.GetChild(0).(*Create_view_stmtContext)
+	if !ok || unquote(v.View_name().GetText()) != name {
+		return nil, false
+	}
+	return v, true
+}
+
 // FileParser implements the sqlparse.Parser
 type FileParser struct{}
 
@@ -175,6 +188,29 @@ func (p *FileParser) ColumnFilledBefore(f migrate.File, t *schema.Table, c *sche
 	})
 }
 
+// CreateViewAfter checks if a view was created after the position with the given name to a table.
+func (p *FileParser) CreateViewAfter(f migrate.File, old, new string, pos int) (bool, error) {
+	return parseutil.MatchStmtAfter(f, pos, func(s *migrate.Stmt) (bool, error) {
+		stmt, err := ParseStmt(s.Text)
+		if err != nil {
+			return false, err
+		}
+		v, ok := stmt.CreateView(old)
+		if !ok {
+			return false, nil
+		}
+		sc, ok := v.Select_stmt().(*Select_stmtContext)
+		if !ok {
+			return false, nil
+		}
+		idx := slices.IndexFunc(sc.Select_core(0).GetChildren(), func(t antlr.Tree) bool {
+			ts, ok := t.(*Table_or_subqueryContext)
+			return ok && unquote(ts.GetText()) == new
+		})
+		return idx != -1, nil
+	})
+}
+
 // FixChange fixes the changes according to the given statement.
 func (p *FileParser) FixChange(_ migrate.Driver, s string, changes schema.Changes) (schema.Changes, error) {
 	stmt, err := ParseStmt(s)
@@ -214,8 +250,12 @@ func isnull(t antlr.Tree) bool {
 }
 
 func unquote(s string) string {
-	if len(s) < 2 || s[0] != '`' || s[len(s)-1] != '`' {
-		return s
+	switch {
+	case len(s) < 2:
+	case s[0] == '`' && s[len(s)-1] == '`', s[0] == '"' && s[len(s)-1] == '"':
+		if u, err := strconv.Unquote(s); err == nil {
+			return u
+		}
 	}
-	return s[1 : len(s)-1]
+	return s
 }
diff --git a/sql/schema/migrate.go b/sql/schema/migrate.go
index a9a1bd1a..ef172c0c 100644
--- a/sql/schema/migrate.go
+++ b/sql/schema/migrate.go
@@ -322,6 +322,24 @@ func (c Changes) IndexDropTable(name string) int {
 	})
 }
 
+// LastIndexAddTable returns the index of the last AddTable in the changes
+// with the given name, or -1 if there is no such change in the Changes.
+func (c Changes) LastIndexAddTable(name string) int {
+	return c.rsearch(func(c Change) bool {
+		a, ok := c.(*AddTable)
+		return ok && a.T.Name == name
+	})
+}
+
+// LastIndexDropTable returns the index of the last DropTable in the changes
+// with the given name, or -1 if there is no such change in the Changes.
+func (c Changes) LastIndexDropTable(name string) int {
+	return c.rsearch(func(c Change) bool {
+		a, ok := c.(*DropTable)
+		return ok && a.T.Name == name
+	})
+}
+
 // IndexAddColumn returns the index of the first AddColumn in the changes
 // with the given name, or -1 if there is no such change in the Changes.
 func (c Changes) IndexAddColumn(name string) int {
@@ -392,6 +410,17 @@ func (c Changes) search(f func(Change) bool) int {
 	return -1
 }
 
+// rsearch is the reversed version of search. It returns the
+// index of the last call to f that returns true, or -1.
+func (c Changes) rsearch(f func(Change) bool) int {
+	for i := len(c) - 1; i >= 0; i-- {
+		if f(c[i]) {
+			return i
+		}
+	}
+	return -1
+}
+
 // changes.
 func (*AddAttr) change()          {}
 func (*DropAttr) change()         {}
diff --git a/sql/sqlite/sqlitecheck/sqlitecheck.go b/sql/sqlite/sqlitecheck/sqlitecheck.go
index f945d028..c9266604 100644
--- a/sql/sqlite/sqlitecheck/sqlitecheck.go
+++ b/sql/sqlite/sqlitecheck/sqlitecheck.go
@@ -16,6 +16,7 @@ import (
 	"ariga.io/atlas/sql/sqlcheck/condrop"
 	"ariga.io/atlas/sql/sqlcheck/datadepend"
 	"ariga.io/atlas/sql/sqlcheck/destructive"
+	"ariga.io/atlas/sql/sqlcheck/incompatible"
 	"ariga.io/atlas/sql/sqlite"
 )
 
@@ -68,17 +69,25 @@ func init() {
 		if err != nil {
 			return nil, err
 		}
+		bc, err := incompatible.New(r)
+		if err != nil {
+			return nil, err
+		}
 		return []sqlcheck.Analyzer{
 			sqlcheck.AnalyzerFunc(func(ctx context.Context, p *sqlcheck.Pass) error {
 				var changes []*sqlcheck.Change
 				// Detect sequence of changes using temporary table and transform them to one ModifyTable change.
 				// See: https://www.sqlite.org/lang_altertable.html#making_other_kinds_of_table_schema_changes.
 				for i := 0; i < len(p.File.Changes); i++ {
-					if i+3 >= len(p.File.Changes) || !modifyUsingTemp(p.File.Changes[i], p.File.Changes[i+2], p.File.Changes[i+3]) {
+					if i+3 >= len(p.File.Changes) {
+						changes = append(changes, p.File.Changes[i])
+						continue
+					}
+					prevT, currT, ok := modifyUsingTemp(p.File.Changes[i], p.File.Changes[i+2], p.File.Changes[i+3])
+					if !ok {
 						changes = append(changes, p.File.Changes[i])
 						continue
 					}
-					prevT, currT := p.File.Changes[i+2].Changes[0].(*schema.DropTable).T, p.File.Changes[i+3].Changes[1].(*schema.AddTable).T
 					diff, err := p.Dev.Driver.TableDiff(prevT, currT)
 					if err != nil {
 						return nil
@@ -106,21 +115,35 @@ func init() {
 				p.File.Changes = changes
 				return nil
 			}),
-			ds, dd, cd,
+			ds, dd, cd, bc,
 		}, nil
 	})
 }
 
 // modifyUsingTemp indicates if the 3 changes represents a table modification using
 // the pattern mentioned in the link below: "CREATE", "INSERT", "DROP" and "RENAME".
-func modifyUsingTemp(c1, c2, c3 *sqlcheck.Change) bool {
-	if len(c1.Changes) != 1 || !isAddT(c1.Changes[0], "new_") || len(c2.Changes) != 1 || len(c3.Changes) != 2 {
-		return false
+func modifyUsingTemp(c1, c2, c3 *sqlcheck.Change) (from, to *schema.Table, _ bool) {
+	if len(c1.Changes) != 1 || !isAddT(c1.Changes[0], "new_") || len(c2.Changes) != 1 || len(c3.Changes) == 0 {
+		return nil, nil, false
 	}
+	// New table layout.
 	add := c1.Changes[0].(*schema.AddTable)
-	name := strings.TrimPrefix(add.T.Name, "new_")
-	// "DROP T" and "RENAME new_T to T".
-	return isDropT(c2.Changes[0], name) && isDropT(c3.Changes[0], add.T.Name) && isAddT(c3.Changes[1], name)
+	prefixed, name := add.T.Name, strings.TrimPrefix(add.T.Name, "new_")
+	add.T.Name = name
+	// Right after "INSERT", the "DROP T" is expected.
+	if !isDropT(c2.Changes[0], name) {
+		return nil, nil, false
+	}
+	drop := c2.Changes[0].(*schema.DropTable)
+	// "RENAME T" is expected after "DROP T".
+	if len(c3.Changes) == 1 && isRenameT(c3.Changes[0], prefixed, name) {
+		return drop.T, add.T, true
+	}
+	// In case no parser is attached, "RENAME T" will be presented as "DROP T" and "ADD T".
+	if len(c3.Changes) == 2 && isDropT(c3.Changes[0], prefixed) && isAddT(c3.Changes[1], name) {
+		return drop.T, add.T, true
+	}
+	return nil, nil, false
 }
 
 func isAddT(c schema.Change, prefix string) bool {
@@ -128,7 +151,12 @@ func isAddT(c schema.Change, prefix string) bool {
 	return ok && strings.HasPrefix(a.T.Name, prefix)
 }
 
-func isDropT(c schema.Change, prefix string) bool {
+func isDropT(c schema.Change, name string) bool {
 	d, ok := c.(*schema.DropTable)
-	return ok && strings.HasPrefix(d.T.Name, prefix)
+	return ok && d.T.Name == name
+}
+
+func isRenameT(c schema.Change, from, to string) bool {
+	r, ok := c.(*schema.RenameTable)
+	return ok && r.From.Name == from && r.To.Name == to
 }


Test patch:
diff --git a/cmd/atlas/internal/sqlparse/sqliteparse/sqliteparse_test.go b/cmd/atlas/internal/sqlparse/sqliteparse/sqliteparse_test.go
index d3d7727c..c077dbfa 100644
--- a/cmd/atlas/internal/sqlparse/sqliteparse/sqliteparse_test.go
+++ b/cmd/atlas/internal/sqlparse/sqliteparse/sqliteparse_test.go
@@ -143,3 +143,59 @@ UPDATE t1 SET c = 2 WHERE c IS NULL;
 		})
 	}
 }
+
+func TestCreateViewAfter(t *testing.T) {
+	for i, tt := range []struct {
+		file        string
+		pos         int
+		wantCreated bool
+		wantErr     bool
+	}{
+		{
+			file: `
+ALTER TABLE old RENAME TO new;
+CREATE VIEW old AS SELECT * FROM new;
+`,
+			pos:         1,
+			wantCreated: true,
+		},
+		{
+			file: `
+ALTER TABLE old RENAME TO new;
+CREATE VIEW old AS SELECT * FROM users;
+`,
+			pos: 1,
+		},
+		{
+			file: `
+ALTER TABLE old RENAME TO new;
+CREATE VIEW old AS SELECT * FROM new JOIN new;
+`,
+			pos: 1,
+		},
+		{
+			file: `
+ALTER TABLE old RENAME TO new;
+CREATE VIEW old AS SELECT * FROM new;
+`,
+			pos: 100,
+		},
+		{
+			file: `
+ALTER TABLE old RENAME TO new;
+CREATE VIEW old AS SELECT a, b, c FROM new;
+`,
+			wantCreated: true,
+		},
+	} {
+		t.Run(strconv.Itoa(i), func(t *testing.T) {
+			var (
+				p sqliteparse.FileParser
+				f = migrate.NewLocalFile("file", []byte(tt.file))
+			)
+			created, err := p.CreateViewAfter(f, "old", "new", tt.pos)
+			require.Equal(t, err != nil, tt.wantErr, err)
+			require.Equal(t, created, tt.wantCreated)
+		})
+	}
+}
diff --git a/internal/integration/testdata/sqlite/cli-migrate-lint-incompatible.txt b/internal/integration/testdata/sqlite/cli-migrate-lint-incompatible.txt
new file mode 100644
index 00000000..44c8bd94
--- /dev/null
+++ b/internal/integration/testdata/sqlite/cli-migrate-lint-incompatible.txt
@@ -0,0 +1,49 @@
+atlas migrate lint --dir file://migrations1 --dev-url URL --latest=1 > got.txt
+cmp got.txt expected1.txt
+
+atlas migrate lint --dir file://migrations2 --dev-url URL --latest=1 > got.txt
+cmp got.txt expected2.txt
+
+atlas migrate lint --dir file://migrations3 --dev-url URL --latest=1 > got.txt
+cmp got.txt expected3.txt
+
+-- migrations1/1.sql --
+CREATE TABLE users (id int);
+
+-- migrations1/2.sql --
+ALTER TABLE users RENAME TO atlas_users;
+
+-- expected1.txt --
+2.sql: backwards-incompatible changes detected:
+
+	L1: Renaming table "users" to "atlas_users"
+
+-- migrations2/1.sql --
+CREATE TABLE users (id int);
+
+-- migrations2/2.sql --
+ALTER TABLE users RENAME COLUMN id TO uid;
+
+-- expected2.txt --
+2.sql: backwards-incompatible changes detected:
+
+	L1: Renaming column "id" to "uid"
+
+-- migrations3/1.sql --
+CREATE TABLE users (id int);
+CREATE TABLE posts (id int);
+CREATE TABLE cards (id int);
+
+-- migrations3/2.sql --
+ALTER TABLE `users` RENAME TO `atlas_users`;
+CREATE VIEW "users" AS SELECT * FROM "atlas_users";
+ALTER TABLE posts RENAME COLUMN id TO uid;
+ALTER TABLE posts ADD COLUMN id int AS (uid);
+ALTER TABLE cards RENAME COLUMN id TO cid;
+ALTER TABLE cards ADD COLUMN name text;
+
+-- expected3.txt --
+2.sql: backwards-incompatible changes detected:
+
+	L5: Renaming column "id" to "cid"
+
diff --git a/sql/sqlite/sqlitecheck/sqlitecheck_test.go b/sql/sqlite/sqlitecheck/sqlitecheck_test.go
index e2591e5e..5899577c 100644
--- a/sql/sqlite/sqlitecheck/sqlitecheck_test.go
+++ b/sql/sqlite/sqlitecheck/sqlitecheck_test.go
@@ -125,7 +125,7 @@ func TestDetectModifyTable(t *testing.T) {
 	)
 	azs, err := sqlcheck.AnalyzerFor(sqlite.DriverName, nil)
 	require.NoError(t, err)
-	require.Len(t, azs, 4)
+	require.Len(t, azs, 5)
 	require.NoError(t, azs[0].Analyze(context.Background(), pass))
 	err = azs[1].Analyze(context.Background(), pass)
 	require.EqualError(t, err, "destructive changes detected")