diff --git a/README.md b/README.md index 9bbe33c..9c3bc12 100644 --- a/README.md +++ b/README.md @@ -67,6 +67,53 @@ A frequent operation that falls into this category is creating an index concurre `pgsafemigrate` assumes all statements are wrapped in a transaction, unless the `sql-migrate` `notransaction` command is defined. +## Rules + +### High Availability + +#### high-availability-alter-column-not-null-exclusive-lock + +Setting a column as NOT NULL acquires an exclusive lock on the table until the constraint is validated on all table rows. +#### high-availability-avoid-non-concurrent-index-creation + +Non-concurrent index creation will not allow writes while the index is being built. + +#### high-availability-avoid-non-concurrent-index-drop + +Non-concurrent index drop will not allow writes while the index is being built. + +#### high-availability-avoid-required-column + +Newly added columns must either define a default value or be nullable. + +#### high-availability-avoid-table-rename + +Renaming a table can cause errors in previous application versions. + +### Maintainability + +#### maintainability-describe-new-column-with-comment + +Newly added columns should also include a COMMENT for documentation purposes. + +#### maintainability-indexes-name-is-required + +Indexes should be explicitly named. + +### Transactions + +#### transactions-concurrent-index-operation-cannot-be-executed-in-transaction + +Concurrent index operations cannot be executed inside a transaction. + +#### transactions-index-if-not-exists-missing + +Creating/removing an index outside of a transaction without an IF (NOT) EXISTS option can cause a migration to not be idempotent. + +#### transactions-no-nested-transactions + +Nested transactions are not supported in PostgreSQL. + ## Contributing ### Adding New Rules diff --git a/loader/loader.go b/loader/loader.go index 83ef078..2f16f44 100644 --- a/loader/loader.go +++ b/loader/loader.go @@ -118,7 +118,7 @@ func ScanCommentsFromString(sql string) ([]Comment, error) { } c := Comment{ Content: sql[token.GetStart()+3 : token.GetEnd()], - TokenIndex: i, + TokenIndex: i + 1, } c.SQLMigrateAnnotation = strings.HasPrefix(c.Content, "+migrate") if strings.HasPrefix(c.Content, "+migrate Up") { diff --git a/loader/loader_test.go b/loader/loader_test.go index bd97128..ddf7ded 100644 --- a/loader/loader_test.go +++ b/loader/loader_test.go @@ -1,6 +1,7 @@ package loader import ( + "fmt" migrate "github.com/rubenv/sql-migrate" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -128,3 +129,102 @@ SELECT 3; UPDATE "movies" SET updated_at = CURRENT_TIMESTAMP WHERE id>10;`, }) } } + +func TestScanCommentsFromString(t *testing.T) { + tests := []struct { + name string + sql string + want []Comment + wantErr assert.ErrorAssertionFunc + }{ + { + name: "sql-migrate commands", + sql: `-- +migrate Up notransaction +SELECT 1; + +-- +migrate Down +-- noop`, + want: []Comment{ + { + TokenIndex: 1, + Content: `+migrate Up notransaction`, + SQLMigrateDirection: migrate.Up, + SQLMigrateAnnotation: true, + }, + { + TokenIndex: 5, + Content: `+migrate Down`, + SQLMigrateDirection: migrate.Down, + SQLMigrateAnnotation: true, + }, + { + TokenIndex: 6, + Content: `noop`, + SQLMigrateDirection: migrate.Down, + SQLMigrateAnnotation: false, + }, + }, + wantErr: assert.NoError, + }, + { + name: "sql-migrate commands: valid nolint annotation", + sql: `-- +migrate Up notransaction +-- pgsafemigrate:nolint +SELECT 1; + +-- +migrate Down +-- noop`, + want: []Comment{ + { + TokenIndex: 1, + Content: `+migrate Up notransaction`, + SQLMigrateDirection: migrate.Up, + SQLMigrateAnnotation: true, + }, + { + TokenIndex: 2, + Content: `pgsafemigrate:nolint`, + SQLMigrateDirection: migrate.Up, + SQLMigrateAnnotation: false, + NoLintAnnotation: annotations.NoLint{ + Valid: true, + }, + }, + { + TokenIndex: 6, + Content: `+migrate Down`, + SQLMigrateDirection: migrate.Down, + SQLMigrateAnnotation: true, + }, + { + TokenIndex: 7, + Content: `noop`, + SQLMigrateDirection: migrate.Down, + SQLMigrateAnnotation: false, + }, + }, + wantErr: assert.NoError, + }, + { + name: "sql-migrate commands: no-lint annotations must be adjacent", + sql: `-- +migrate Up notransaction +SELECT 1; +-- pgsafemigrate:nolint + +-- +migrate Down +-- noop`, + wantErr: assert.Error, + }, + } + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + got, err := ScanCommentsFromString(tt.sql) + if !tt.wantErr(t, err, fmt.Sprintf("ScanCommentsFromString(%v)", tt.sql)) { + return + } + assert.Equalf(t, tt.want, got, "ScanCommentsFromString(%v)", tt.sql) + }) + } +} diff --git a/rules/runner_test.go b/rules/runner_test.go index 5b06830..4855c80 100644 --- a/rules/runner_test.go +++ b/rules/runner_test.go @@ -102,6 +102,72 @@ ALTER TABLE movies DROP COLUMN released_at; }, wantErr: assert.NoError, }, + { + name: "statements with violations", + args: args{ + migrationFile: loader.MigrationFile{ + Path: "test1.sql", + Contents: ` +CREATE INDEX test_idx ON movies(title); +`, + }, + }, + want: []StatementResult{ + { + Passed: false, + Direction: migrate.Up, + Errors: []ReportedError{ + Violation{ + rule: All()["high-availability-avoid-non-concurrent-index-creation"], + statement: "CREATE INDEX test_idx ON movies(title);", + }, + }, + }, + }, + wantErr: assert.NoError, + }, + { + name: "statements with violations with matching no-lint annotation", + args: args{ + migrationFile: loader.MigrationFile{ + Path: "test1.sql", + Contents: `-- pgsafemigrate:nolint:high-availability-avoid-non-concurrent-index-creation +CREATE INDEX test_idx ON movies(title); +`, + }, + }, + want: []StatementResult{ + { + Passed: true, + Direction: migrate.Up, + }, + }, + wantErr: assert.NoError, + }, + { + name: "statements with violations with not matching no-lint annotation", + args: args{ + migrationFile: loader.MigrationFile{ + Path: "test1.sql", + Contents: `-- pgsafemigrate:nolint:high-availability-another-rule +CREATE INDEX test_idx ON movies(title); +`, + }, + }, + want: []StatementResult{ + { + Passed: false, + Direction: migrate.Up, + Errors: []ReportedError{ + Violation{ + rule: All()["high-availability-avoid-non-concurrent-index-creation"], + statement: "CREATE INDEX test_idx ON movies(title);", + }, + }, + }, + }, + wantErr: assert.NoError, + }, } for _, tt := range tests { tt := tt diff --git a/rules/transaction_test.go b/rules/transaction_test.go new file mode 100644 index 0000000..84c08f1 --- /dev/null +++ b/rules/transaction_test.go @@ -0,0 +1,51 @@ +package rules + +import ( + pg_query "github.com/pganalyze/pg_query_go/v4" + "github.com/stretchr/testify/assert" + "strings" + "testing" +) + +func TestNestedTransaction_Process(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + sql string + inTransaction bool + want bool + }{ + { + name: "transaction BEGIN statement found", + sql: `BEGIN; ALTER TABLE movies ADD COLUMN released_at TIMESTAMP;`, + inTransaction: true, + want: true, + }, + { + name: "transaction BEGIN statement found but no active transaction context", + sql: `BEGIN; ALTER TABLE movies ADD COLUMN released_at TIMESTAMP;`, + inTransaction: false, + want: true, + }, + { + name: "no transaction management statements found", + sql: `ALTER TABLE movies ADD COLUMN released_at TIMESTAMP;`, + inTransaction: true, + want: false, + }, + } + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + r := NestedTransaction{} + var allNodes []*pg_query.Node + for _, statement := range strings.Split(tt.sql, "\n") { + allNodes = append(allNodes, parseStatement(t, statement)) + } + assert.Equal(t, tt.want, r.Process(allNodes[0], allNodes, true)) + }) + } +}