diff --git a/cmd/pgtidy/lint.go b/cmd/pgtidy/lint.go new file mode 100644 index 0000000..b9a78ef --- /dev/null +++ b/cmd/pgtidy/lint.go @@ -0,0 +1,146 @@ +package main + +import ( + "fmt" + "io" + "os" + "strings" + + "github.com/hein/pgtidy/pkg/diagnostics" + "github.com/hein/pgtidy/pkg/lint" +) + +// cmdLint implements `pgtidy lint`. Reads one or more SQL files (or stdin) and +// reports lint findings to stdout. Exits non-zero when any findings are found. +// +// Output format (one line per finding): +// +// file:line:col: [RULEID] severity: message +func cmdLint(args []string, stdin io.Reader, stdout, stderr io.Writer) int { + var ( + only []string // --only=ID,ID rule filter + files []string + ) + for _, a := range args { + switch { + case a == "-h" || a == "--help": + lintUsage(stdout) + return 0 + case strings.HasPrefix(a, "--only="): + ids := strings.Split(strings.TrimPrefix(a, "--only="), ",") + for _, id := range ids { + if id = strings.TrimSpace(id); id != "" { + only = append(only, strings.ToUpper(id)) + } + } + case len(a) > 1 && a[0] == '-': + fmt.Fprintf(stderr, "pgtidy lint: unknown flag %q\n", a) + return 2 + default: + files = append(files, a) + } + } + + eng := lint.New() + + check := func(sql, file string) ([]diagnostics.Diagnostic, error) { + diags, err := eng.Check(sql, file) + if err != nil { + return nil, err + } + if len(only) > 0 { + filtered := diags[:0] + for _, d := range diags { + for _, id := range only { + if d.RuleID == id { + filtered = append(filtered, d) + break + } + } + } + diags = filtered + } + return diags, nil + } + + printDiags := func(diags []diagnostics.Diagnostic) { + for _, d := range diags { + loc := d.File + if d.Line > 0 { + loc = fmt.Sprintf("%s:%d:%d", d.File, d.Line, d.Col) + } + if loc == "" { + loc = "stdin" + } + fmt.Fprintf(stdout, "%s: [%s] %s: %s\n", loc, d.RuleID, d.Severity, d.Message) + } + } + + found := false + + if len(files) == 0 { + src, err := io.ReadAll(stdin) + if err != nil { + fmt.Fprintf(stderr, "pgtidy: reading stdin: %v\n", err) + return 2 + } + diags, err := check(string(src), "") + if err != nil { + fmt.Fprintf(stderr, "pgtidy: %v\n", err) + return 2 + } + printDiags(diags) + if len(diags) > 0 { + found = true + } + } else { + for _, path := range files { + src, err := os.ReadFile(path) + if err != nil { + fmt.Fprintf(stderr, "pgtidy: %v\n", err) + return 2 + } + diags, err := check(string(src), path) + if err != nil { + fmt.Fprintf(stderr, "pgtidy: %v\n", err) + return 2 + } + printDiags(diags) + if len(diags) > 0 { + found = true + } + } + } + + if found { + return 1 + } + return 0 +} + +func lintUsage(w io.Writer) { + fmt.Fprint(w, `Usage: pgtidy lint [flags] [file ...] + +Read SQL from files (or stdin) and report lint findings. + +Flags: + --only=ID,... comma-separated rule IDs to enable (default: all rules) + -h, --help show this help + +Rules: + MIG001 CREATE INDEX without CONCURRENTLY blocks writes + MIG002 ALTER TABLE ADD COLUMN NOT NULL with no DEFAULT may rewrite table + MIG003 ALTER TABLE ADD CONSTRAINT without NOT VALID validates all rows + COR001 SELECT * is fragile; enumerate columns explicitly + COR002 UPDATE without WHERE modifies every row + COR003 DELETE without WHERE removes every row + NAM001 Table name is not snake_case + NAM002 Column name is not snake_case + NAM003 Function/procedure name is not snake_case + +Exit codes: + 0 no findings + 1 one or more findings + 2 error (file not found, parse error, bad flag) +`) +} diff --git a/cmd/pgtidy/main.go b/cmd/pgtidy/main.go index a4f997c..f29ad1d 100644 --- a/cmd/pgtidy/main.go +++ b/cmd/pgtidy/main.go @@ -1,5 +1,4 @@ -// Command pgtidy is the PgTidy CLI: a PostgreSQL formatter (and, later, linter) -// and LSP server. Today it provides the `fmt` subcommand. +// Command pgtidy is the PgTidy CLI: a PostgreSQL formatter and linter. package main import ( @@ -23,6 +22,8 @@ func run(args []string, stdin io.Reader, stdout, stderr io.Writer) int { switch args[0] { case "fmt", "format": return cmdFmt(args[1:], stdin, stdout, stderr) + case "lint": + return cmdLint(args[1:], stdin, stdout, stderr) case "version", "--version", "-v": fmt.Fprintf(stdout, "pgtidy %s\n", version) return 0 @@ -40,13 +41,20 @@ func usage(w io.Writer) { fmt.Fprint(w, `pgtidy โ€” PostgreSQL formatter and linter Usage: - pgtidy fmt [flags] [files...] Format SQL/PL-pgSQL (stdin if no files) - pgtidy version Print version - pgtidy help Show this help + pgtidy fmt [flags] [files...] Format SQL/PL-pgSQL (stdin if no files) + pgtidy lint [flags] [files...] Lint SQL (stdin if no files) + pgtidy version Print version + pgtidy help Show this help fmt flags: -w, --write Rewrite files in place -l, --list List files whose formatting differs (no writes) + -d, --diff Print unified diff of changes --check Exit non-zero if any input is not already formatted (CI) + +lint flags: + --only=ID,... Enable only the specified rule IDs (comma-separated) + +Run 'pgtidy lint --help' for the full rule list. `) } diff --git a/docs/todo.md b/docs/todo.md index 15eadf5..eb67906 100644 --- a/docs/todo.md +++ b/docs/todo.md @@ -96,12 +96,19 @@ Legend: โœ… done ยท ๐Ÿšง in progress ยท โฌœ not started --- -## V2 โ€” Linter (later) -- `pkg/pgast`: `go-pgquery` (WASM, no cgo) wrapper โ†’ real PG AST. -- `pkg/lint`: rule engine + packs โ€” style/consistency, **migration safety** (locks, unsafe - ALTER/ADD COLUMN, non-CONCURRENTLY index, blocking constraints), naming, correctness. -- `pkg/diagnostics`: shared diagnostic type (CLI + LSP). -- `pgtidy lint` subcommand; `--fix` for autofixable rules. +## โœ… V2 โ€” Linter +- `pkg/pgast`: `go-pgquery` (WASM, no cgo) wrapper โ†’ real PG AST. `FirstTokenOffset` + skips leading whitespace/comments for accurate line numbers. +- `pkg/diagnostics`: `Diagnostic{RuleID, Severity, Message, File, Line, Col}`. +- `pkg/lint`: `Engine`, `Rule` interface, `New()` with all built-ins: + - MIG001 CREATE INDEX without CONCURRENT + - MIG002 ALTER TABLE ADD COLUMN NOT NULL without DEFAULT + - MIG003 ALTER TABLE ADD CONSTRAINT FK/CHECK without NOT VALID + - COR001 SELECT * | COR002 UPDATE without WHERE | COR003 DELETE without WHERE + - NAM001/2/3 table/column/function names not snake_case (quoted identifiers only) +- `pgtidy lint [--only=ID,...] [files...]`; exits 1 on findings, 2 on error. +- Fixture SQL in `testdata/lint/`; 6 tests covering violations + clean fixtures. +- _`--fix` for autofixable rules: future._ ## V3 โ€” LSP + VSCode (later) - `pkg/lsp`: formatting + range formatting, publishDiagnostics, codeAction quick-fixes. diff --git a/go.mod b/go.mod index f586a63..736454d 100644 --- a/go.mod +++ b/go.mod @@ -2,4 +2,12 @@ module github.com/hein/pgtidy go 1.26 -require gopkg.in/yaml.v3 v3.0.1 // indirect +require ( + github.com/pganalyze/pg_query_go/v6 v6.2.2 // indirect + github.com/tetratelabs/wazero v1.12.0 // indirect + github.com/wasilibs/go-pgquery v0.0.0-20260623022807-b68b28131ed3 // indirect + github.com/wasilibs/wazero-helpers v0.0.0-20250123031827-cd30c44769bb // indirect + golang.org/x/sys v0.44.0 // indirect + google.golang.org/protobuf v1.36.11 // indirect + gopkg.in/yaml.v3 v3.0.1 // indirect +) diff --git a/go.sum b/go.sum index 4bc0337..863bd75 100644 --- a/go.sum +++ b/go.sum @@ -1,3 +1,15 @@ +github.com/pganalyze/pg_query_go/v6 v6.2.2 h1:O0L6zMC226R82RF3X5n0Ki6HjytDsoAzuzp4ATVAHNo= +github.com/pganalyze/pg_query_go/v6 v6.2.2/go.mod h1:Cn6+j4870kJz3iYNsb0VsNG04vpSWgEvBwc590J4qD0= +github.com/tetratelabs/wazero v1.12.0 h1:DuWcpNu/FzgEXgGBDp8J1Spc+CWOvvtvVyjKlaZopYU= +github.com/tetratelabs/wazero v1.12.0/go.mod h1:LvKtzl2RqO4gyF27BiXU+nKAjcV8f38U+kP/q2vgxh0= +github.com/wasilibs/go-pgquery v0.0.0-20260623022807-b68b28131ed3 h1:XRtDsFHBMM6PNAh4EW7ChRjuZ2C9Vo19/1Gdp6G1NR0= +github.com/wasilibs/go-pgquery v0.0.0-20260623022807-b68b28131ed3/go.mod h1:ZSyYLCRbk2xPqu7lgfrDSSHm+g/7Rxk6JK4KE2cxJ3s= +github.com/wasilibs/wazero-helpers v0.0.0-20250123031827-cd30c44769bb h1:gQ+ZV4wJke/EBKYciZ2MshEouEHFuinB85dY3f5s1q8= +github.com/wasilibs/wazero-helpers v0.0.0-20250123031827-cd30c44769bb/go.mod h1:jMeV4Vpbi8osrE/pKUxRZkVaA0EX7NZN0A9/oRzgpgY= +golang.org/x/sys v0.44.0 h1:ildZl3J4uzeKP07r2F++Op7E9B29JRUy+a27EibtBTQ= +golang.org/x/sys v0.44.0/go.mod h1:4GL1E5IUh+htKOUEOaiffhrAeqysfVGipDYzABqnCmw= +google.golang.org/protobuf v1.36.11 h1:fV6ZwhNocDyBLK0dj+fg8ektcVegBBuEolpbTQyBNVE= +google.golang.org/protobuf v1.36.11/go.mod h1:HTf+CrKn2C3g5S8VImy6tdcUvCska2kB7j23XfzDpco= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= diff --git a/pkg/diagnostics/diagnostics.go b/pkg/diagnostics/diagnostics.go new file mode 100644 index 0000000..ed35b00 --- /dev/null +++ b/pkg/diagnostics/diagnostics.go @@ -0,0 +1,30 @@ +// Package diagnostics defines the shared Diagnostic type emitted by the linter +// (and, later, the LSP). Both the CLI and the language server import this +// package to avoid a circular dependency between pkg/lint and pkg/lsp. +package diagnostics + +// Severity is the urgency of a diagnostic. +type Severity string + +const ( + SeverityError Severity = "error" + SeverityWarning Severity = "warning" + SeverityHint Severity = "hint" +) + +// Diagnostic is a single lint finding. +type Diagnostic struct { + // RuleID is the stable identifier for the rule that produced this finding + // (e.g. "MIG001"). + RuleID string + // Severity is the urgency level. + Severity Severity + // Message is a human-readable description of the finding. + Message string + // File is the path to the source file, or "" for stdin. + File string + // Line is the 1-based line number of the finding. + Line int + // Col is the 1-based column number of the finding. + Col int +} diff --git a/pkg/lint/lint.go b/pkg/lint/lint.go new file mode 100644 index 0000000..d844f8e --- /dev/null +++ b/pkg/lint/lint.go @@ -0,0 +1,119 @@ +// Package lint provides the rule engine and built-in rule packs for PgTidy. +// +// Usage: +// +// eng := lint.New() +// diags, err := eng.Check(sql, filename) +package lint + +import ( + "sort" + + pg_query "github.com/pganalyze/pg_query_go/v6" + + "github.com/hein/pgtidy/pkg/diagnostics" + "github.com/hein/pgtidy/pkg/pgast" +) + +// Rule is implemented by each lint rule. +type Rule interface { + // ID returns the stable rule identifier (e.g. "MIG001"). + ID() string + // Severity returns the default severity for findings from this rule. + Severity() diagnostics.Severity + // Check inspects the parsed statement list and returns any findings. + // src is the original SQL string, used for location lookups. + Check(stmts []*pg_query.RawStmt, src string) []diagnostics.Diagnostic +} + +// Engine runs a set of rules against SQL input. +type Engine struct { + rules []Rule +} + +// New returns an Engine loaded with all built-in rules. +func New() *Engine { + e := &Engine{} + e.registerAll() + return e +} + +// Custom returns an empty Engine; use Register to add rules. +func Custom() *Engine { + return &Engine{} +} + +func (e *Engine) registerAll() { + e.Register(ruleMIG001{}) + e.Register(ruleMIG002{}) + e.Register(ruleMIG003{}) + e.Register(ruleCOR001{}) + e.Register(ruleCOR002{}) + e.Register(ruleCOR003{}) + e.Register(ruleNAM001{}) + e.Register(ruleNAM002{}) + e.Register(ruleNAM003{}) +} + +// Register adds a rule to the engine. +func (e *Engine) Register(r Rule) { + e.rules = append(e.rules, r) +} + +// Rules returns a copy of the registered rule slice (for testing/inspection). +func (e *Engine) Rules() []Rule { + out := make([]Rule, len(e.rules)) + copy(out, e.rules) + return out +} + +// Check parses sql and runs all registered rules. Diagnostics are sorted by +// (line, col, ruleID). A parse error is returned as a single diagnostic with +// ruleID "PARSE". +func (e *Engine) Check(sql, file string) ([]diagnostics.Diagnostic, error) { + result, err := pgast.Parse(sql) + if err != nil { + line, col := parseErrLocation(err, sql) + return []diagnostics.Diagnostic{{ + RuleID: "PARSE", + Severity: diagnostics.SeverityError, + Message: err.Error(), + File: file, + Line: line, + Col: col, + }}, nil + } + + var all []diagnostics.Diagnostic + for _, r := range e.rules { + found := r.Check(result.Stmts, sql) + for i := range found { + found[i].File = file + } + all = append(all, found...) + } + + sort.Slice(all, func(i, j int) bool { + if all[i].Line != all[j].Line { + return all[i].Line < all[j].Line + } + if all[i].Col != all[j].Col { + return all[i].Col < all[j].Col + } + return all[i].RuleID < all[j].RuleID + }) + return all, nil +} + +// parseErrLocation extracts a (line, col) from a pg_query parse error. +// The error message format is "syntax error at or near ... (location N)" where +// N is a 1-based character offset. +func parseErrLocation(err error, sql string) (line, col int) { + // pg_query errors expose Location via a type assertion. + type locErr interface{ GetCursorPos() int } + if le, ok := err.(locErr); ok { + pos := le.GetCursorPos() - 1 // convert to 0-based + return pgast.LocationToLineCol(sql, pos) + } + return 1, 1 +} diff --git a/pkg/lint/lint_test.go b/pkg/lint/lint_test.go new file mode 100644 index 0000000..c68fad0 --- /dev/null +++ b/pkg/lint/lint_test.go @@ -0,0 +1,125 @@ +package lint_test + +import ( + "os" + "path/filepath" + "testing" + + "github.com/hein/pgtidy/pkg/diagnostics" + "github.com/hein/pgtidy/pkg/lint" +) + +func fixtureDir() string { + return filepath.Join("..", "..", "testdata", "lint") +} + +func checkFile(t *testing.T, name string) []diagnostics.Diagnostic { + t.Helper() + src, err := os.ReadFile(filepath.Join(fixtureDir(), name)) + if err != nil { + t.Fatalf("read %s: %v", name, err) + } + eng := lint.New() + diags, err := eng.Check(string(src), name) + if err != nil { + t.Fatalf("Check(%s): %v", name, err) + } + return diags +} + +func ruleIDs(diags []diagnostics.Diagnostic) map[string]int { + m := make(map[string]int) + for _, d := range diags { + m[d.RuleID]++ + } + return m +} + +func TestMigrationViolations(t *testing.T) { + diags := checkFile(t, "migration_violations.sql") + ids := ruleIDs(diags) + + cases := []struct { + id string + count int + }{ + {"MIG001", 1}, // one non-concurrent index + {"MIG002", 1}, // one NOT NULL + no DEFAULT + {"MIG003", 2}, // one FK + one CHECK without NOT VALID + } + for _, c := range cases { + if got := ids[c.id]; got != c.count { + t.Errorf("rule %s: want %d findings, got %d", c.id, c.count, got) + } + } +} + +func TestMigrationClean(t *testing.T) { + diags := checkFile(t, "migration_clean.sql") + ids := ruleIDs(diags) + for _, id := range []string{"MIG001", "MIG002", "MIG003"} { + if n := ids[id]; n != 0 { + t.Errorf("rule %s: want 0 findings on clean fixture, got %d", id, n) + } + } +} + +func TestCorrectnessViolations(t *testing.T) { + diags := checkFile(t, "correctness_violations.sql") + ids := ruleIDs(diags) + + cases := []struct { + id string + count int + }{ + {"COR001", 1}, + {"COR002", 1}, + {"COR003", 1}, + } + for _, c := range cases { + if got := ids[c.id]; got != c.count { + t.Errorf("rule %s: want %d findings, got %d", c.id, c.count, got) + } + } +} + +func TestNamingViolations(t *testing.T) { + diags := checkFile(t, "naming_violations.sql") + ids := ruleIDs(diags) + + cases := []struct { + id string + count int + }{ + {"NAM001", 1}, // UserAccounts + {"NAM002", 2}, // userId, emailAddress + {"NAM003", 1}, // GetUserById + } + for _, c := range cases { + if got := ids[c.id]; got != c.count { + t.Errorf("rule %s: want %d findings, got %d", c.id, c.count, got) + } + } +} + +func TestParseError(t *testing.T) { + eng := lint.New() + diags, err := eng.Check("SELECT FROM WHERE", "test.sql") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(diags) == 0 || diags[0].RuleID != "PARSE" { + t.Errorf("expected a PARSE diagnostic, got %v", diags) + } +} + +func TestCustomEngine(t *testing.T) { + eng := lint.Custom() + eng.Register(lint.New().Rules()[0]) // register first rule only + // Just confirm it doesn't panic and returns results. + diags, err := eng.Check("SELECT 1", "stdin") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + _ = diags +} diff --git a/pkg/lint/rules_correctness.go b/pkg/lint/rules_correctness.go new file mode 100644 index 0000000..bc00d64 --- /dev/null +++ b/pkg/lint/rules_correctness.go @@ -0,0 +1,127 @@ +package lint + +import ( + "fmt" + + pg_query "github.com/pganalyze/pg_query_go/v6" + + "github.com/hein/pgtidy/pkg/diagnostics" + "github.com/hein/pgtidy/pkg/pgast" +) + +// COR001 โ€” SELECT *. +// Selecting all columns by wildcard is fragile: column additions/removals +// break callers silently. Enumerate the columns you need explicitly. +type ruleCOR001 struct{} + +func (ruleCOR001) ID() string { return "COR001" } +func (ruleCOR001) Severity() diagnostics.Severity { return diagnostics.SeverityHint } + +func (ruleCOR001) Check(stmts []*pg_query.RawStmt, src string) []diagnostics.Diagnostic { + var out []diagnostics.Diagnostic + for _, raw := range stmts { + walkSelectStar(raw.Stmt, src, &out) + } + return out +} + +func walkSelectStar(node *pg_query.Node, src string, out *[]diagnostics.Diagnostic) { + if node == nil { + return + } + sel, ok := node.GetNode().(*pg_query.Node_SelectStmt) + if !ok { + return + } + checkSelectStmt(sel.SelectStmt, src, out) +} + +func checkSelectStmt(s *pg_query.SelectStmt, src string, out *[]diagnostics.Diagnostic) { + if s == nil { + return + } + for _, t := range s.TargetList { + rt, ok := t.GetNode().(*pg_query.Node_ResTarget) + if !ok { + continue + } + cr, ok := rt.ResTarget.Val.GetNode().(*pg_query.Node_ColumnRef) + if !ok { + continue + } + for _, f := range cr.ColumnRef.Fields { + if _, isStar := f.GetNode().(*pg_query.Node_AStar); isStar { + line, col := pgast.LocationToLineCol(src, int(cr.ColumnRef.Location)) + *out = append(*out, diagnostics.Diagnostic{ + RuleID: "COR001", + Severity: diagnostics.SeverityHint, + Message: "SELECT * is fragile; enumerate the columns explicitly", + Line: line, + Col: col, + }) + } + } + } + // Recurse into set-operation branches (UNION, INTERSECT, EXCEPT). + checkSelectStmt(s.Larg, src, out) + checkSelectStmt(s.Rarg, src, out) +} + +// COR002 โ€” UPDATE without WHERE. +// An UPDATE with no WHERE clause modifies every row in the table. +type ruleCOR002 struct{} + +func (ruleCOR002) ID() string { return "COR002" } +func (ruleCOR002) Severity() diagnostics.Severity { return diagnostics.SeverityWarning } + +func (ruleCOR002) Check(stmts []*pg_query.RawStmt, src string) []diagnostics.Diagnostic { + var out []diagnostics.Diagnostic + for _, raw := range stmts { + u, ok := raw.Stmt.GetNode().(*pg_query.Node_UpdateStmt) + if !ok { + continue + } + if u.UpdateStmt.WhereClause != nil { + continue + } + line, col := pgast.LocationToLineCol(src, pgast.FirstTokenOffset(src, int(raw.StmtLocation))) + out = append(out, diagnostics.Diagnostic{ + RuleID: "COR002", + Severity: diagnostics.SeverityWarning, + Message: fmt.Sprintf("UPDATE %q has no WHERE clause; this modifies every row", relName(u.UpdateStmt.Relation)), + Line: line, + Col: col, + }) + } + return out +} + +// COR003 โ€” DELETE without WHERE. +// A DELETE with no WHERE clause removes every row from the table. +// Use TRUNCATE if you intend a full wipe; it is faster and explicit. +type ruleCOR003 struct{} + +func (ruleCOR003) ID() string { return "COR003" } +func (ruleCOR003) Severity() diagnostics.Severity { return diagnostics.SeverityWarning } + +func (ruleCOR003) Check(stmts []*pg_query.RawStmt, src string) []diagnostics.Diagnostic { + var out []diagnostics.Diagnostic + for _, raw := range stmts { + d, ok := raw.Stmt.GetNode().(*pg_query.Node_DeleteStmt) + if !ok { + continue + } + if d.DeleteStmt.WhereClause != nil { + continue + } + line, col := pgast.LocationToLineCol(src, pgast.FirstTokenOffset(src, int(raw.StmtLocation))) + out = append(out, diagnostics.Diagnostic{ + RuleID: "COR003", + Severity: diagnostics.SeverityWarning, + Message: fmt.Sprintf("DELETE FROM %q has no WHERE clause; this removes every row (use TRUNCATE if intentional)", relName(d.DeleteStmt.Relation)), + Line: line, + Col: col, + }) + } + return out +} diff --git a/pkg/lint/rules_migration.go b/pkg/lint/rules_migration.go new file mode 100644 index 0000000..2b57d0a --- /dev/null +++ b/pkg/lint/rules_migration.go @@ -0,0 +1,171 @@ +package lint + +import ( + "fmt" + + pg_query "github.com/pganalyze/pg_query_go/v6" + + "github.com/hein/pgtidy/pkg/diagnostics" + "github.com/hein/pgtidy/pkg/pgast" +) + +// MIG001 โ€” CREATE INDEX without CONCURRENT. +// A non-concurrent index build holds ShareLock for its entire duration, +// blocking all writes on the table. In production migrations use +// CREATE INDEX CONCURRENTLY instead. +type ruleMIG001 struct{} + +func (ruleMIG001) ID() string { return "MIG001" } +func (ruleMIG001) Severity() diagnostics.Severity { return diagnostics.SeverityWarning } + +func (ruleMIG001) Check(stmts []*pg_query.RawStmt, src string) []diagnostics.Diagnostic { + var out []diagnostics.Diagnostic + for _, raw := range stmts { + idx, ok := raw.Stmt.GetNode().(*pg_query.Node_IndexStmt) + if !ok { + continue + } + s := idx.IndexStmt + if s.Concurrent || s.Isconstraint { + continue + } + line, col := pgast.LocationToLineCol(src, pgast.FirstTokenOffset(src, int(raw.StmtLocation))) + out = append(out, diagnostics.Diagnostic{ + RuleID: "MIG001", + Severity: diagnostics.SeverityWarning, + Message: fmt.Sprintf("CREATE INDEX on %q without CONCURRENT blocks writes; use CREATE INDEX CONCURRENTLY", relName(s.Relation)), + Line: line, + Col: col, + }) + } + return out +} + +// MIG002 โ€” ALTER TABLE ADD COLUMN with NOT NULL and no DEFAULT. +// Pre-PG11 this rewrites the whole table. Even on PG11+ the column may require +// a costly table scan to validate the NOT NULL constraint when no DEFAULT is +// supplied. Use a nullable column first, backfill, then add the constraint. +type ruleMIG002 struct{} + +func (ruleMIG002) ID() string { return "MIG002" } +func (ruleMIG002) Severity() diagnostics.Severity { return diagnostics.SeverityWarning } + +func (ruleMIG002) Check(stmts []*pg_query.RawStmt, src string) []diagnostics.Diagnostic { + var out []diagnostics.Diagnostic + for _, raw := range stmts { + alt, ok := raw.Stmt.GetNode().(*pg_query.Node_AlterTableStmt) + if !ok { + continue + } + tbl := alt.AlterTableStmt + for _, cmdNode := range tbl.Cmds { + cmd, ok := cmdNode.GetNode().(*pg_query.Node_AlterTableCmd) + if !ok { + continue + } + ac := cmd.AlterTableCmd + if ac.Subtype != pg_query.AlterTableType_AT_AddColumn { + continue + } + col, ok := ac.Def.GetNode().(*pg_query.Node_ColumnDef) + if !ok { + continue + } + if hasNotNullNoDefault(col.ColumnDef.Constraints) { + line, col2 := pgast.LocationToLineCol(src, pgast.FirstTokenOffset(src, int(raw.StmtLocation))) + out = append(out, diagnostics.Diagnostic{ + RuleID: "MIG002", + Severity: diagnostics.SeverityWarning, + Message: fmt.Sprintf("ALTER TABLE %q ADD COLUMN with NOT NULL and no DEFAULT may rewrite the table; add nullable, backfill, then constrain", relName(tbl.Relation)), + Line: line, + Col: col2, + }) + } + } + } + return out +} + +// MIG003 โ€” ALTER TABLE ADD CONSTRAINT (FK or CHECK) without NOT VALID. +// Without NOT VALID, PostgreSQL validates all existing rows immediately, +// holding locks that block concurrent writes. Use NOT VALID + a separate +// VALIDATE CONSTRAINT to spread the lock window. +type ruleMIG003 struct{} + +func (ruleMIG003) ID() string { return "MIG003" } +func (ruleMIG003) Severity() diagnostics.Severity { return diagnostics.SeverityWarning } + +func (ruleMIG003) Check(stmts []*pg_query.RawStmt, src string) []diagnostics.Diagnostic { + var out []diagnostics.Diagnostic + for _, raw := range stmts { + alt, ok := raw.Stmt.GetNode().(*pg_query.Node_AlterTableStmt) + if !ok { + continue + } + tbl := alt.AlterTableStmt + for _, cmdNode := range tbl.Cmds { + cmd, ok := cmdNode.GetNode().(*pg_query.Node_AlterTableCmd) + if !ok { + continue + } + if cmd.AlterTableCmd.Subtype != pg_query.AlterTableType_AT_AddConstraint { + continue + } + con, ok := cmd.AlterTableCmd.Def.GetNode().(*pg_query.Node_Constraint) + if !ok { + continue + } + c := con.Constraint + if c.Contype != pg_query.ConstrType_CONSTR_FOREIGN && c.Contype != pg_query.ConstrType_CONSTR_CHECK { + continue + } + if c.SkipValidation { + continue // NOT VALID is present + } + kind := "FOREIGN KEY" + if c.Contype == pg_query.ConstrType_CONSTR_CHECK { + kind = "CHECK" + } + line, col := pgast.LocationToLineCol(src, pgast.FirstTokenOffset(src, int(raw.StmtLocation))) + out = append(out, diagnostics.Diagnostic{ + RuleID: "MIG003", + Severity: diagnostics.SeverityWarning, + Message: fmt.Sprintf("ALTER TABLE %q ADD %s CONSTRAINT without NOT VALID validates all rows immediately; use NOT VALID + VALIDATE CONSTRAINT", relName(tbl.Relation), kind), + Line: line, + Col: col, + }) + } + } + return out +} + +// hasNotNullNoDefault returns true when the constraint list has CONSTR_NOTNULL +// but no CONSTR_DEFAULT. +func hasNotNullNoDefault(constraints []*pg_query.Node) bool { + hasNN := false + hasDef := false + for _, cn := range constraints { + c, ok := cn.GetNode().(*pg_query.Node_Constraint) + if !ok { + continue + } + switch c.Constraint.Contype { + case pg_query.ConstrType_CONSTR_NOTNULL: + hasNN = true + case pg_query.ConstrType_CONSTR_DEFAULT: + hasDef = true + } + } + return hasNN && !hasDef +} + +// relName returns "schema.rel" or "rel" from a RangeVar. +func relName(rv *pg_query.RangeVar) string { + if rv == nil { + return "?" + } + if rv.Schemaname != "" { + return rv.Schemaname + "." + rv.Relname + } + return rv.Relname +} diff --git a/pkg/lint/rules_naming.go b/pkg/lint/rules_naming.go new file mode 100644 index 0000000..e528d79 --- /dev/null +++ b/pkg/lint/rules_naming.go @@ -0,0 +1,135 @@ +package lint + +import ( + "fmt" + "regexp" + "strings" + + pg_query "github.com/pganalyze/pg_query_go/v6" + + "github.com/hein/pgtidy/pkg/diagnostics" + "github.com/hein/pgtidy/pkg/pgast" +) + +// reSnakeCase matches valid snake_case identifiers: lowercase letters, digits, +// and underscores, starting with a letter or underscore. +var reSnakeCase = regexp.MustCompile(`^[a-z_][a-z0-9_]*$`) + +func isSnakeCase(s string) bool { + return reSnakeCase.MatchString(s) +} + +// NAM001 โ€” table names must be snake_case. +type ruleNAM001 struct{} + +func (ruleNAM001) ID() string { return "NAM001" } +func (ruleNAM001) Severity() diagnostics.Severity { return diagnostics.SeverityHint } + +func (ruleNAM001) Check(stmts []*pg_query.RawStmt, src string) []diagnostics.Diagnostic { + var out []diagnostics.Diagnostic + for _, raw := range stmts { + cs, ok := raw.Stmt.GetNode().(*pg_query.Node_CreateStmt) + if !ok { + continue + } + name := cs.CreateStmt.Relation.Relname + if name == "" || isSnakeCase(name) { + continue + } + line, col := pgast.LocationToLineCol(src, pgast.FirstTokenOffset(src, int(raw.StmtLocation))) + out = append(out, diagnostics.Diagnostic{ + RuleID: "NAM001", + Severity: diagnostics.SeverityHint, + Message: fmt.Sprintf("table name %q is not snake_case (expected %q)", name, toSnakeCase(name)), + Line: line, + Col: col, + }) + } + return out +} + +// NAM002 โ€” column names in CREATE TABLE must be snake_case. +type ruleNAM002 struct{} + +func (ruleNAM002) ID() string { return "NAM002" } +func (ruleNAM002) Severity() diagnostics.Severity { return diagnostics.SeverityHint } + +func (ruleNAM002) Check(stmts []*pg_query.RawStmt, src string) []diagnostics.Diagnostic { + var out []diagnostics.Diagnostic + for _, raw := range stmts { + cs, ok := raw.Stmt.GetNode().(*pg_query.Node_CreateStmt) + if !ok { + continue + } + for _, elt := range cs.CreateStmt.TableElts { + cd, ok := elt.GetNode().(*pg_query.Node_ColumnDef) + if !ok { + continue + } + name := cd.ColumnDef.Colname + if name == "" || isSnakeCase(name) { + continue + } + line, col := pgast.LocationToLineCol(src, int(cd.ColumnDef.Location)) + out = append(out, diagnostics.Diagnostic{ + RuleID: "NAM002", + Severity: diagnostics.SeverityHint, + Message: fmt.Sprintf("column name %q is not snake_case (expected %q)", name, toSnakeCase(name)), + Line: line, + Col: col, + }) + } + } + return out +} + +// NAM003 โ€” function/procedure names must be snake_case. +type ruleNAM003 struct{} + +func (ruleNAM003) ID() string { return "NAM003" } +func (ruleNAM003) Severity() diagnostics.Severity { return diagnostics.SeverityHint } + +func (ruleNAM003) Check(stmts []*pg_query.RawStmt, src string) []diagnostics.Diagnostic { + var out []diagnostics.Diagnostic + for _, raw := range stmts { + cf, ok := raw.Stmt.GetNode().(*pg_query.Node_CreateFunctionStmt) + if !ok { + continue + } + // Funcname is a list of String nodes: [schema, funcname] or [funcname]. + name := lastStringNode(cf.CreateFunctionStmt.Funcname) + if name == "" || isSnakeCase(name) { + continue + } + line, col := pgast.LocationToLineCol(src, pgast.FirstTokenOffset(src, int(raw.StmtLocation))) + out = append(out, diagnostics.Diagnostic{ + RuleID: "NAM003", + Severity: diagnostics.SeverityHint, + Message: fmt.Sprintf("function name %q is not snake_case (expected %q)", name, toSnakeCase(name)), + Line: line, + Col: col, + }) + } + return out +} + +// lastStringNode returns the string value of the last node in a name-list +// (schema-qualified names like [schema, name] โ†’ name). +func lastStringNode(nodes []*pg_query.Node) string { + for i := len(nodes) - 1; i >= 0; i-- { + sv, ok := nodes[i].GetNode().(*pg_query.Node_String_) + if ok { + return sv.String_.Sval + } + } + return "" +} + +// toSnakeCase converts a CamelCase or mixed-case identifier to snake_case as a +// suggested fix shown in the diagnostic message. +var reWordBoundary = regexp.MustCompile(`([a-z0-9])([A-Z])`) + +func toSnakeCase(s string) string { + s = reWordBoundary.ReplaceAllString(s, "${1}_${2}") + return strings.ToLower(s) +} diff --git a/pkg/pgast/pgast.go b/pkg/pgast/pgast.go new file mode 100644 index 0000000..a5c5a52 --- /dev/null +++ b/pkg/pgast/pgast.go @@ -0,0 +1,75 @@ +// Package pgast wraps go-pgquery (libpg_query compiled to WASM via wazero, no +// cgo) to produce a PostgreSQL parse tree from a SQL string. +// +// The concrete node types live in github.com/pganalyze/pg_query_go/v6 and are +// generated from the PostgreSQL source; callers import that package directly +// when walking the tree. +package pgast + +import ( + "strings" + + pg_query "github.com/pganalyze/pg_query_go/v6" + waspg "github.com/wasilibs/go-pgquery" +) + +// ParseResult is re-exported so callers don't need to import both packages. +type ParseResult = pg_query.ParseResult + +// Parse parses sql and returns the PostgreSQL parse tree. The returned error +// is a pg_query parse error containing a message and location; it is safe to +// display to the user. +func Parse(sql string) (*ParseResult, error) { + return waspg.Parse(sql) +} + +// FirstTokenOffset advances past whitespace and SQL comments (-- line and +// /* block */) from start, returning the byte offset of the first real token. +// This is needed because RawStmt.StmtLocation points to the start of the +// statement "block" which includes any preceding comments, not to the first +// keyword token. +func FirstTokenOffset(sql string, start int) int { + i := start + n := len(sql) + for i < n { + switch { + case sql[i] == ' ' || sql[i] == '\t' || sql[i] == '\r' || sql[i] == '\n': + i++ + case i+1 < n && sql[i] == '-' && sql[i+1] == '-': + for i < n && sql[i] != '\n' { + i++ + } + case i+1 < n && sql[i] == '/' && sql[i+1] == '*': + i += 2 + for i+1 < n && !(sql[i] == '*' && sql[i+1] == '/') { + i++ + } + if i+1 < n { + i += 2 + } + default: + return i + } + } + return start +} + +// LocationToLineCol converts a 0-based byte offset into a 1-based (line, col) +// pair. Returns (1, 1) for a negative offset. +func LocationToLineCol(sql string, offset int) (line, col int) { + if offset < 0 { + return 1, 1 + } + if offset > len(sql) { + offset = len(sql) + } + before := sql[:offset] + line = strings.Count(before, "\n") + 1 + lastNL := strings.LastIndex(before, "\n") + if lastNL < 0 { + col = offset + 1 + } else { + col = offset - lastNL + } + return line, col +} diff --git a/testdata/lint/correctness_violations.sql b/testdata/lint/correctness_violations.sql new file mode 100644 index 0000000..1f52c1a --- /dev/null +++ b/testdata/lint/correctness_violations.sql @@ -0,0 +1,10 @@ +-- Fixtures for correctness rules (COR001-COR003). + +-- COR001: SELECT * +SELECT * FROM orders; + +-- COR002: UPDATE without WHERE +UPDATE orders SET status = 'archived'; + +-- COR003: DELETE without WHERE +DELETE FROM orders; diff --git a/testdata/lint/migration_clean.sql b/testdata/lint/migration_clean.sql new file mode 100644 index 0000000..93fd373 --- /dev/null +++ b/testdata/lint/migration_clean.sql @@ -0,0 +1,17 @@ +-- Fixtures for migration safety rules โ€” all statements are safe. + +-- MIG001 safe: CONCURRENTLY +CREATE INDEX CONCURRENTLY idx_orders_user ON orders(user_id); + +-- MIG002 safe: nullable column (no NOT NULL) +ALTER TABLE orders ADD COLUMN note text; + +-- MIG002 safe: NOT NULL with a DEFAULT +ALTER TABLE orders ADD COLUMN status text NOT NULL DEFAULT 'pending'; + +-- MIG003 safe: FK with NOT VALID +ALTER TABLE orders + ADD CONSTRAINT fk_orders_user FOREIGN KEY (user_id) REFERENCES users(id) NOT VALID; + +-- MIG003 safe: CHECK with NOT VALID +ALTER TABLE orders ADD CONSTRAINT chk_positive CHECK (amount > 0) NOT VALID; diff --git a/testdata/lint/migration_violations.sql b/testdata/lint/migration_violations.sql new file mode 100644 index 0000000..ed16d30 --- /dev/null +++ b/testdata/lint/migration_violations.sql @@ -0,0 +1,15 @@ +-- Fixtures for migration safety rules (MIG001-MIG003). +-- Every statement here should trigger at least one finding. + +-- MIG001: non-concurrent index +CREATE INDEX idx_orders_user ON orders(user_id); + +-- MIG002: ADD COLUMN NOT NULL without DEFAULT +ALTER TABLE orders ADD COLUMN status text NOT NULL; + +-- MIG003: ADD FOREIGN KEY without NOT VALID +ALTER TABLE orders + ADD CONSTRAINT fk_orders_user FOREIGN KEY (user_id) REFERENCES users(id); + +-- MIG003: ADD CHECK without NOT VALID +ALTER TABLE orders ADD CONSTRAINT chk_positive CHECK (amount > 0); diff --git a/testdata/lint/naming_violations.sql b/testdata/lint/naming_violations.sql new file mode 100644 index 0000000..332c780 --- /dev/null +++ b/testdata/lint/naming_violations.sql @@ -0,0 +1,15 @@ +-- Fixtures for naming rules (NAM001-NAM003). +-- Quoted identifiers preserve case in the PG parse tree; unquoted identifiers +-- are always folded to lowercase by the parser and cannot violate snake_case. + +-- NAM001: quoted CamelCase table name +CREATE TABLE "UserAccounts" ( + -- NAM002: quoted camelCase column names + "userId" integer PRIMARY KEY, + "emailAddress" text NOT NULL +); + +-- NAM003: quoted CamelCase function name +CREATE FUNCTION "GetUserById"(p_id integer) RETURNS text LANGUAGE sql AS $$ + SELECT email FROM users WHERE id = p_id +$$;