From db67518f5132269108acbd597671e9994a31efdc Mon Sep 17 00:00:00 2001 From: Andrew Benton Date: Tue, 18 Jul 2023 09:41:48 -0400 Subject: [PATCH 1/3] feat(vet): Introduce a query annotation to opt out of sqlc vet rules --- docs/howto/vet.md | 53 +++++++++++++++++++++++++++++--- examples/authors/mysql/query.sql | 1 + internal/cmd/vet.go | 6 +++- internal/compiler/parse.go | 2 +- internal/metadata/meta.go | 7 +++-- internal/metadata/meta_test.go | 2 +- 6 files changed, 62 insertions(+), 9 deletions(-) diff --git a/docs/howto/vet.md b/docs/howto/vet.md index e51c82c747..087bde83e9 100644 --- a/docs/howto/vet.md +++ b/docs/howto/vet.md @@ -7,10 +7,12 @@ Rules are defined in the `sqlc` [configuration](../reference/config) file. They consist of a name, message, and a [Common Expression Language (CEL)](https://github.com/google/cel-spec) expression. Expressions are evaluated using [cel-go](https://github.com/google/cel-go). -If an expression evaluates to `true`, an error is reported using the given message. +If an expression evaluates to `true`, `sqlc vet` will report an error using the given message. -Each expression has access to variables from your sqlc configuration and queries, -defined in the following struct: +## Defining lint rules + +Each lint rule's CEL expression has access to variables from your sqlc configuration and queries, +defined in the following struct. ```proto message Config @@ -109,4 +111,47 @@ example](https://github.com/kyleconroy/sqlc/blob/main/examples/authors/sqlc.yaml Please note that `sqlc` does not manage or migrate your database. Use your migration tool of choice to create the necessary database tables and objects -before running `sqlc vet`. +before running `sqlc vet` with the `sqlc/db-prepare` rule. + +## Running lint rules + +When you add the name of a defined rule to the rules list +for a [sql package](https://docs.sqlc.dev/en/stable/reference/config.html#sql), +`sqlc vet` will evaluate that rule against every query in the package. + +In the example below, two rules are defined but only one is enabled. + +```yaml +version: 2 +sql: + - schema: "query.sql" + queries: "query.sql" + engine: "postgresql" + gen: + go: + package: "authors" + out: "db" + rules: + - no-delete +rules: + - name: no-pg + message: "invalid engine: postgresql" + rule: | + config.engine == "postgresql" + - name: no-delete + message: "don't use delete statements" + rule: | + query.sql.contains("DELETE") +``` + +### 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. + +```sql +/* name: GetAuthor :one */ +/* @sqlc-vet-disable */ +SELECT * FROM authors +WHERE id = ? LIMIT 1; +``` \ No newline at end of file diff --git a/examples/authors/mysql/query.sql b/examples/authors/mysql/query.sql index c3b5866149..0611beb707 100644 --- a/examples/authors/mysql/query.sql +++ b/examples/authors/mysql/query.sql @@ -1,4 +1,5 @@ /* name: GetAuthor :one */ +/* @sqlc-vet-disable */ SELECT * FROM authors WHERE id = ? LIMIT 1; diff --git a/internal/cmd/vet.go b/internal/cmd/vet.go index 33d19fec05..6df74837a4 100644 --- a/internal/cmd/vet.go +++ b/internal/cmd/vet.go @@ -31,6 +31,7 @@ import ( var ErrFailedChecks = errors.New("failed checks") const RuleDbPrepare = "sqlc/db-prepare" +const QueryFlagSqlcVetDisable = "@sqlc-vet-disable" func NewCmdVet() *cobra.Command { return &cobra.Command{ @@ -249,7 +250,6 @@ func (c *checker) checkSQL(ctx context.Context, s config.SQL) error { return ErrFailedChecks } - // TODO: Add MySQL support var prep preparer if s.Database != nil { if c.NoDatabase { @@ -299,6 +299,10 @@ 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].Flags[QueryFlagSqlcVetDisable] { + fmt.Printf("Skipping vet rules for query %s", query.Name) + continue + } q := vetQuery(query) for _, name := range s.Rules { // Built-in rule diff --git a/internal/compiler/parse.go b/internal/compiler/parse.go index 932df32d32..19c105be40 100644 --- a/internal/compiler/parse.go +++ b/internal/compiler/parse.go @@ -126,7 +126,7 @@ func (c *Compiler) parseQuery(stmt ast.Node, src string, o opts.Parser) (*Query, return nil, err } - flags, err := metadata.ParseQueryFlags(comments) + flags, comments, err := metadata.ParseQueryFlags(comments) if err != nil { return nil, err } diff --git a/internal/metadata/meta.go b/internal/metadata/meta.go index 3c34eff58a..da03ba2a2a 100644 --- a/internal/metadata/meta.go +++ b/internal/metadata/meta.go @@ -104,8 +104,9 @@ func ParseQueryNameAndType(t string, commentStyle CommentSyntax) (string, string return "", "", nil } -func ParseQueryFlags(comments []string) (map[string]bool, error) { +func ParseQueryFlags(comments []string) (map[string]bool, []string, error) { flags := make(map[string]bool) + remainingComments := make([]string, 0, len(comments)) for _, line := range comments { cleanLine := strings.TrimPrefix(line, "--") cleanLine = strings.TrimPrefix(cleanLine, "/*") @@ -115,7 +116,9 @@ func ParseQueryFlags(comments []string) (map[string]bool, error) { if strings.HasPrefix(cleanLine, "@") { flagName := strings.SplitN(cleanLine, " ", 2)[0] flags[flagName] = true + continue } + remainingComments = append(remainingComments, line) } - return flags, nil + return flags, remainingComments, nil } diff --git a/internal/metadata/meta_test.go b/internal/metadata/meta_test.go index cbfcb6fba6..ce5d656bd4 100644 --- a/internal/metadata/meta_test.go +++ b/internal/metadata/meta_test.go @@ -53,7 +53,7 @@ func TestParseQueryFlags(t *testing.T) { "-- @flag-foo", }, } { - flags, err := ParseQueryFlags(comments) + flags, _, err := ParseQueryFlags(comments) if err != nil { t.Errorf("expected query flags to parse, got error: %s", err) } From 19225415bb3694e8a8d86ff58f2bd0caed78c4fa Mon Sep 17 00:00:00 2001 From: Andrew Benton Date: Tue, 18 Jul 2023 15:14:57 -0400 Subject: [PATCH 2/3] Add proper logging, and a test --- examples/authors/mysql/query.sql | 1 - internal/cmd/vet.go | 5 ++++- internal/endtoend/testdata/vet_disable/exec.json | 3 +++ internal/endtoend/testdata/vet_disable/query.sql | 6 ++++++ internal/endtoend/testdata/vet_disable/sqlc.yaml | 15 +++++++++++++++ internal/endtoend/testdata/vet_disable/stderr.txt | 1 + internal/engine/dolphin/convert.go | 3 +-- 7 files changed, 30 insertions(+), 4 deletions(-) create mode 100644 internal/endtoend/testdata/vet_disable/exec.json create mode 100644 internal/endtoend/testdata/vet_disable/query.sql create mode 100644 internal/endtoend/testdata/vet_disable/sqlc.yaml create mode 100644 internal/endtoend/testdata/vet_disable/stderr.txt diff --git a/examples/authors/mysql/query.sql b/examples/authors/mysql/query.sql index 0611beb707..c3b5866149 100644 --- a/examples/authors/mysql/query.sql +++ b/examples/authors/mysql/query.sql @@ -1,5 +1,4 @@ /* name: GetAuthor :one */ -/* @sqlc-vet-disable */ SELECT * FROM authors WHERE id = ? LIMIT 1; diff --git a/internal/cmd/vet.go b/internal/cmd/vet.go index 6df74837a4..1233614340 100644 --- a/internal/cmd/vet.go +++ b/internal/cmd/vet.go @@ -6,6 +6,7 @@ import ( "errors" "fmt" "io" + "log" "os" "path/filepath" "runtime/trace" @@ -300,7 +301,9 @@ func (c *checker) checkSQL(ctx context.Context, s config.SQL) error { cfg := vetConfig(req) for i, query := range req.Queries { if result.Queries[i].Flags[QueryFlagSqlcVetDisable] { - fmt.Printf("Skipping vet rules for query %s", query.Name) + if debug.Active { + log.Printf("Skipping vet rules for query: %s\n", query.Name) + } continue } q := vetQuery(query) diff --git a/internal/endtoend/testdata/vet_disable/exec.json b/internal/endtoend/testdata/vet_disable/exec.json new file mode 100644 index 0000000000..07753636ee --- /dev/null +++ b/internal/endtoend/testdata/vet_disable/exec.json @@ -0,0 +1,3 @@ +{ + "command": "vet" +} diff --git a/internal/endtoend/testdata/vet_disable/query.sql b/internal/endtoend/testdata/vet_disable/query.sql new file mode 100644 index 0000000000..c1eb8d2219 --- /dev/null +++ b/internal/endtoend/testdata/vet_disable/query.sql @@ -0,0 +1,6 @@ +-- name: SkipVet :exec +-- @sqlc-vet-disable +SELECT true; + +-- name: RunVet :exec +SELECT true; \ No newline at end of file diff --git a/internal/endtoend/testdata/vet_disable/sqlc.yaml b/internal/endtoend/testdata/vet_disable/sqlc.yaml new file mode 100644 index 0000000000..ff7c1b5200 --- /dev/null +++ b/internal/endtoend/testdata/vet_disable/sqlc.yaml @@ -0,0 +1,15 @@ +version: 2 +sql: + - schema: "query.sql" + queries: "query.sql" + engine: "postgresql" + gen: + go: + package: "db" + out: "db" + rules: + - always-fail +rules: + - name: always-fail + message: "Fail" + rule: "true" diff --git a/internal/endtoend/testdata/vet_disable/stderr.txt b/internal/endtoend/testdata/vet_disable/stderr.txt new file mode 100644 index 0000000000..9acbb8b34b --- /dev/null +++ b/internal/endtoend/testdata/vet_disable/stderr.txt @@ -0,0 +1 @@ +query.sql: RunVet: always-fail: Fail diff --git a/internal/engine/dolphin/convert.go b/internal/engine/dolphin/convert.go index bd642c55ed..132be0b3f4 100644 --- a/internal/engine/dolphin/convert.go +++ b/internal/engine/dolphin/convert.go @@ -1,7 +1,6 @@ package dolphin import ( - "fmt" "log" "strings" @@ -144,7 +143,7 @@ func (c *cc) convertAlterTableStmt(n *pcast.AlterTableStmt) ast.Node { default: if debug.Active { - fmt.Printf("dolphin.convert: Unknown alter table cmd %v\n", spec.Tp) + log.Printf("dolphin.convert: Unknown alter table cmd %v\n", spec.Tp) } continue } From c587d3e27a0bd6a991d08e4a054e638719390b0c Mon Sep 17 00:00:00 2001 From: Andrew Benton Date: Tue, 18 Jul 2023 15:27:33 -0400 Subject: [PATCH 3/3] Don't remove comments when parsing query annotation metadata --- internal/compiler/parse.go | 2 +- internal/metadata/meta.go | 6 ++---- internal/metadata/meta_test.go | 2 +- 3 files changed, 4 insertions(+), 6 deletions(-) diff --git a/internal/compiler/parse.go b/internal/compiler/parse.go index 19c105be40..932df32d32 100644 --- a/internal/compiler/parse.go +++ b/internal/compiler/parse.go @@ -126,7 +126,7 @@ func (c *Compiler) parseQuery(stmt ast.Node, src string, o opts.Parser) (*Query, return nil, err } - flags, comments, err := metadata.ParseQueryFlags(comments) + flags, err := metadata.ParseQueryFlags(comments) if err != nil { return nil, err } diff --git a/internal/metadata/meta.go b/internal/metadata/meta.go index da03ba2a2a..4176da1e2b 100644 --- a/internal/metadata/meta.go +++ b/internal/metadata/meta.go @@ -104,9 +104,8 @@ func ParseQueryNameAndType(t string, commentStyle CommentSyntax) (string, string return "", "", nil } -func ParseQueryFlags(comments []string) (map[string]bool, []string, error) { +func ParseQueryFlags(comments []string) (map[string]bool, error) { flags := make(map[string]bool) - remainingComments := make([]string, 0, len(comments)) for _, line := range comments { cleanLine := strings.TrimPrefix(line, "--") cleanLine = strings.TrimPrefix(cleanLine, "/*") @@ -118,7 +117,6 @@ func ParseQueryFlags(comments []string) (map[string]bool, []string, error) { flags[flagName] = true continue } - remainingComments = append(remainingComments, line) } - return flags, remainingComments, nil + return flags, nil } diff --git a/internal/metadata/meta_test.go b/internal/metadata/meta_test.go index ce5d656bd4..cbfcb6fba6 100644 --- a/internal/metadata/meta_test.go +++ b/internal/metadata/meta_test.go @@ -53,7 +53,7 @@ func TestParseQueryFlags(t *testing.T) { "-- @flag-foo", }, } { - flags, _, err := ParseQueryFlags(comments) + flags, err := ParseQueryFlags(comments) if err != nil { t.Errorf("expected query flags to parse, got error: %s", err) }