Skip to content

Allow selective disabling of rules per query #3620

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 18 additions & 1 deletion docs/howto/vet.md
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,24 @@ rules:
### Opting-out of lint rules

For any query, you can tell `sqlc vet` not to evaluate lint rules using the
`@sqlc-vet-disable` query annotation.
`@sqlc-vet-disable` query annotation. The annotation accepts a list of rules to ignore.

```sql
/* name: GetAuthor :one */
/* @sqlc-vet-disable sqlc/db-prepare no-pg */
SELECT * FROM authors
WHERE id = ? LIMIT 1;
```
The rules can also be split across lines.
```sql
/* name: GetAuthor :one */
/* @sqlc-vet-disable sqlc/db-prepare */
/* @sqlc-vet-disable no-pg */
SELECT * FROM authors
WHERE id = ? LIMIT 1;
```

To skip all rules for a query, you can provide the `@sqlc-vet-disable` annotation without any parameters.

```sql
/* name: GetAuthor :one */
Expand Down
144 changes: 81 additions & 63 deletions internal/cmd/vet.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,13 @@ import (
"encoding/json"
"errors"
"fmt"
"github.com/sqlc-dev/sqlc/internal/constants"
"io"
"log"
"os"
"path/filepath"
"runtime/trace"
"slices"
"strings"
"time"

Expand All @@ -37,9 +39,6 @@ var ErrFailedChecks = errors.New("failed checks")

var pjson = protojson.UnmarshalOptions{AllowPartial: true, DiscardUnknown: true}

const RuleDbPrepare = "sqlc/db-prepare"
const QueryFlagSqlcVetDisable = "@sqlc-vet-disable"

func NewCmdVet() *cobra.Command {
return &cobra.Command{
Use: "vet",
Expand Down Expand Up @@ -109,7 +108,7 @@ func Vet(ctx context.Context, dir, filename string, opts *Options) error {
}

rules := map[string]rule{
RuleDbPrepare: {NeedsPrepare: true},
constants.QueryRuleDbPrepare: {NeedsPrepare: true},
}

for _, c := range conf.Rules {
Expand Down Expand Up @@ -538,11 +537,23 @@ func (c *checker) checkSQL(ctx context.Context, s config.SQL) error {
req := codeGenRequest(result, combo)
cfg := vetConfig(req)
for i, query := range req.Queries {
if result.Queries[i].Metadata.Flags[QueryFlagSqlcVetDisable] {
if debug.Active {
log.Printf("Skipping vet rules for query: %s\n", query.Name)
md := result.Queries[i].Metadata
if md.Flags[constants.QueryFlagSqlcVetDisable] {
// If the vet disable flag is specified without any rules listed, all rules are ignored.
if len(md.RuleSkiplist) == 0 {
if debug.Active {
log.Printf("Skipping all vet rules for query: %s\n", query.Name)
}
continue
}

// Rules which are listed to be disabled but not declared in the config file are rejected.
for r := range md.RuleSkiplist {
if !slices.Contains(s.Rules, r) {
fmt.Fprintf(c.Stderr, "%s: %s: rule-check error: rule %q does not exist in the config file\n", query.Filename, query.Name, r)
errored = true
}
}
continue
}

evalMap := map[string]any{
Expand All @@ -551,74 +562,81 @@ func (c *checker) checkSQL(ctx context.Context, s config.SQL) error {
}

for _, name := range s.Rules {
rule, ok := c.Rules[name]
if !ok {
return fmt.Errorf("type-check error: a rule with the name '%s' does not exist", name)
}

if rule.NeedsPrepare {
if prep == nil {
fmt.Fprintf(c.Stderr, "%s: %s: %s: error preparing query: database connection required\n", query.Filename, query.Name, name)
errored = true
continue
if _, skip := md.RuleSkiplist[name]; skip {
if debug.Active {
log.Printf("Skipping vet rule %q for query: %s\n", name, query.Name)
}
prepName := fmt.Sprintf("sqlc_vet_%d_%d", time.Now().Unix(), i)
if err := prep.Prepare(ctx, prepName, query.Text); err != nil {
fmt.Fprintf(c.Stderr, "%s: %s: %s: error preparing query: %s\n", query.Filename, query.Name, name, err)
errored = true
continue
} else {
rule, ok := c.Rules[name]
if !ok {
return fmt.Errorf("type-check error: a rule with the name '%s' does not exist", name)
}
}

// short-circuit for "sqlc/db-prepare" rule which doesn't have a CEL program
if rule.Program == nil {
continue
}

// Get explain output for this query if we need it
_, pgsqlOK := evalMap["postgresql"]
_, mysqlOK := evalMap["mysql"]
if rule.NeedsExplain && !(pgsqlOK || mysqlOK) {
if expl == nil {
fmt.Fprintf(c.Stderr, "%s: %s: %s: error explaining query: database connection required\n", query.Filename, query.Name, name)
errored = true
continue
if rule.NeedsPrepare {
if prep == nil {
fmt.Fprintf(c.Stderr, "%s: %s: %s: error preparing query: database connection required\n", query.Filename, query.Name, name)
errored = true
continue
}
prepName := fmt.Sprintf("sqlc_vet_%d_%d", time.Now().Unix(), i)
if err := prep.Prepare(ctx, prepName, query.Text); err != nil {
fmt.Fprintf(c.Stderr, "%s: %s: %s: error preparing query: %s\n", query.Filename, query.Name, name, err)
errored = true
continue
}
}
engineOutput, err := expl.Explain(ctx, query.Text, query.Params...)
if err != nil {
fmt.Fprintf(c.Stderr, "%s: %s: %s: error explaining query: %s\n", query.Filename, query.Name, name, err)
errored = true

// short-circuit for "sqlc/db-prepare" rule which doesn't have a CEL program
if rule.Program == nil {
continue
}

evalMap["postgresql"] = engineOutput.PostgreSQL
evalMap["mysql"] = engineOutput.MySQL
}
// Get explain output for this query if we need it
_, pgsqlOK := evalMap["postgresql"]
_, mysqlOK := evalMap["mysql"]
if rule.NeedsExplain && !(pgsqlOK || mysqlOK) {
if expl == nil {
fmt.Fprintf(c.Stderr, "%s: %s: %s: error explaining query: database connection required\n", query.Filename, query.Name, name)
errored = true
continue
}
engineOutput, err := expl.Explain(ctx, query.Text, query.Params...)
if err != nil {
fmt.Fprintf(c.Stderr, "%s: %s: %s: error explaining query: %s\n", query.Filename, query.Name, name, err)
errored = true
continue
}

evalMap["postgresql"] = engineOutput.PostgreSQL
evalMap["mysql"] = engineOutput.MySQL
}

if debug.Debug.DumpVetEnv {
fmt.Printf("vars for rule '%s' evaluating against query '%s':\n", name, query.Name)
debug.DumpAsJSON(evalMap)
}
if debug.Debug.DumpVetEnv {
fmt.Printf("vars for rule '%s' evaluating against query '%s':\n", name, query.Name)
debug.DumpAsJSON(evalMap)
}

out, _, err := (*rule.Program).Eval(evalMap)
if err != nil {
return err
}
tripped, ok := out.Value().(bool)
if !ok {
return fmt.Errorf("expression returned non-bool value: %v", out.Value())
}
if tripped {
// TODO: Get line numbers in the output
if rule.Message == "" {
fmt.Fprintf(c.Stderr, "%s: %s: %s\n", query.Filename, query.Name, name)
} else {
fmt.Fprintf(c.Stderr, "%s: %s: %s: %s\n", query.Filename, query.Name, name, rule.Message)
out, _, err := (*rule.Program).Eval(evalMap)
if err != nil {
return err
}
tripped, ok := out.Value().(bool)
if !ok {
return fmt.Errorf("expression returned non-bool value: %v", out.Value())
}
if tripped {
// TODO: Get line numbers in the output
if rule.Message == "" {
fmt.Fprintf(c.Stderr, "%s: %s: %s\n", query.Filename, query.Name, name)
} else {
fmt.Fprintf(c.Stderr, "%s: %s: %s: %s\n", query.Filename, query.Name, name, rule.Message)
}
errored = true
}
errored = true
}
}
}

if errored {
return ErrFailedChecks
}
Expand Down
2 changes: 1 addition & 1 deletion internal/compiler/parse.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ func (c *Compiler) parseQuery(stmt ast.Node, src string, o opts.Parser) (*Query,
return nil, err
}

md.Params, md.Flags, err = metadata.ParseParamsAndFlags(cleanedComments)
md.Params, md.Flags, md.RuleSkiplist, err = metadata.ParseCommentFlags(cleanedComments)
if err != nil {
return nil, err
}
Expand Down
12 changes: 12 additions & 0 deletions internal/constants/query.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
package constants

// Flags
const (
QueryFlagParam = "@param"
QueryFlagSqlcVetDisable = "@sqlc-vet-disable"
)

// Rules
const (
QueryRuleDbPrepare = "sqlc/db-prepare"
)
27 changes: 25 additions & 2 deletions internal/endtoend/testdata/vet_disable/query.sql
Original file line number Diff line number Diff line change
@@ -1,6 +1,29 @@
-- name: SkipVet :exec
-- name: RunVetAll :exec
SELECT true;

-- name: SkipVetAll :exec
-- @sqlc-vet-disable
SELECT true;

-- name: RunVet :exec
-- name: SkipVetSingleLine :exec
-- @sqlc-vet-disable always-fail no-exec
SELECT true;

-- name: SkipVetMultiLine :exec
-- @sqlc-vet-disable always-fail
-- @sqlc-vet-disable no-exec
SELECT true;

-- name: SkipVet_always_fail :exec
-- @sqlc-vet-disable always-fail
SELECT true;

-- name: SkipVet_no_exec :exec
-- @sqlc-vet-disable no-exec
SELECT true;

-- name: SkipVetInvalidRule :exec
-- @sqlc-vet-disable always-fail
-- @sqlc-vet-disable block-delete
-- @sqlc-vet-disable no-exec
SELECT true;
6 changes: 6 additions & 0 deletions internal/endtoend/testdata/vet_disable/sqlc.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,13 @@ sql:
out: "db"
rules:
- always-fail
- no-exec
rules:
- name: always-fail
message: "Fail"
rule: "true"

- name: no-exec
message: "don't use exec"
rule: |
query.cmd == "exec"
6 changes: 5 additions & 1 deletion internal/endtoend/testdata/vet_disable/stderr.txt
Original file line number Diff line number Diff line change
@@ -1 +1,5 @@
query.sql: RunVet: always-fail: Fail
query.sql: RunVetAll: always-fail: Fail
query.sql: RunVetAll: no-exec: don't use exec
query.sql: SkipVet_always_fail: no-exec: don't use exec
query.sql: SkipVet_no_exec: always-fail: Fail
query.sql: SkipVetInvalidRule: rule-check error: rule "block-delete" does not exist in the config file
29 changes: 25 additions & 4 deletions internal/metadata/meta.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package metadata
import (
"bufio"
"fmt"
"github.com/sqlc-dev/sqlc/internal/constants"
"strings"
"unicode"

Expand All @@ -18,6 +19,10 @@ type Metadata struct {
Params map[string]string
Flags map[string]bool

// RuleSkiplist contains the names of rules to disable vetting for.
// If the map is empty, but the disable vet flag is specified, then all rules are ignored.
RuleSkiplist map[string]struct{}

Filename string
}

Expand Down Expand Up @@ -113,9 +118,12 @@ func ParseQueryNameAndType(t string, commentStyle CommentSyntax) (string, string
return "", "", nil
}

func ParseParamsAndFlags(comments []string) (map[string]string, map[string]bool, error) {
// ParseCommentFlags processes the comments provided with queries to determine the metadata params, flags and rules to skip.
// All flags in query comments are prefixed with `@`, e.g. @param, @@sqlc-vet-disable.
func ParseCommentFlags(comments []string) (map[string]string, map[string]bool, map[string]struct{}, error) {
params := make(map[string]string)
flags := make(map[string]bool)
ruleSkiplist := make(map[string]struct{})

for _, line := range comments {
s := bufio.NewScanner(strings.NewReader(line))
Expand All @@ -129,7 +137,7 @@ func ParseParamsAndFlags(comments []string) (map[string]string, map[string]bool,
}

switch token {
case "@param":
case constants.QueryFlagParam:
s.Scan()
name := s.Text()
var rest []string
Expand All @@ -138,14 +146,27 @@ func ParseParamsAndFlags(comments []string) (map[string]string, map[string]bool,
rest = append(rest, paramToken)
}
params[name] = strings.Join(rest, " ")

case constants.QueryFlagSqlcVetDisable:
flags[token] = true

// Vet rules can all be disabled in the same line or split across lines .i.e.
// /* @sqlc-vet-disable sqlc/db-prepare delete-without-where */
// is equivalent to:
// /* @sqlc-vet-disable sqlc/db-prepare */
// /* @sqlc-vet-disable delete-without-where */
for s.Scan() {
ruleSkiplist[s.Text()] = struct{}{}
}

default:
flags[token] = true
}

if s.Err() != nil {
return params, flags, s.Err()
return params, flags, ruleSkiplist, s.Err()
}
}

return params, flags, nil
return params, flags, ruleSkiplist, nil
}
Loading
Loading