Document rules and expand test coverage for loader and rules

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

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") {

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)
+		})
+	}
+}

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

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))
+		})
+	}
+}