From baf228d8c13052e0a1a038527bccd4fe06db3550 Mon Sep 17 00:00:00 2001 From: Andreas Kutschera Date: Thu, 27 Oct 2022 17:45:11 +0200 Subject: [PATCH 1/6] fix: check column references in ORDER BY (#1411) --- internal/compiler/output_columns.go | 37 ++++++- internal/compiler/parse_test.go | 104 ++++++++++++++++++ .../postgresql/query.sql | 8 ++ .../postgresql/sqlc.yaml | 7 ++ .../postgresql/stderr.txt | 2 + 5 files changed, 156 insertions(+), 2 deletions(-) create mode 100644 internal/compiler/parse_test.go create mode 100644 internal/endtoend/testdata/order_by_non_existing_column/postgresql/query.sql create mode 100644 internal/endtoend/testdata/order_by_non_existing_column/postgresql/sqlc.yaml create mode 100644 internal/endtoend/testdata/order_by_non_existing_column/postgresql/stderr.txt diff --git a/internal/compiler/output_columns.go b/internal/compiler/output_columns.go index b34715b35d..dd88efc10c 100644 --- a/internal/compiler/output_columns.go +++ b/internal/compiler/output_columns.go @@ -67,16 +67,41 @@ func outputColumns(qc *QueryCatalog, node ast.Node) ([]*Column, error) { if n.GroupClause != nil { for _, item := range n.GroupClause.Items { - ref, ok := item.(*ast.ColumnRef) + if err := findColumnForNode(item, tables, n); err != nil { + return nil, err + } + } + } + if n.SortClause != nil { + for _, item := range n.SortClause.Items { + sb, ok := item.(*ast.SortBy) if !ok { continue } - if err := findColumnForRef(ref, tables, n); err != nil { + if err := findColumnForNode(sb.Node, tables, n); err != nil { return nil, err } } } + if n.WindowClause != nil { + for _, item := range n.WindowClause.Items { + sb, ok := item.(*ast.List) + if !ok { + continue + } + for _, single := range sb.Items { + caseExpr, ok := single.(*ast.CaseExpr) + if !ok { + continue + } + + if err := findColumnForNode(caseExpr.Xpr, tables, n); err != nil { + return nil, err + } + } + } + } // For UNION queries, targets is empty and we need to look for the // columns in Largs. @@ -516,6 +541,14 @@ func outputColumnRefs(res *ast.ResTarget, tables []*Table, node *ast.ColumnRef) return cols, nil } +func findColumnForNode(item ast.Node, tables []*Table, n *ast.SelectStmt) error { + ref, ok := item.(*ast.ColumnRef) + if !ok { + return nil + } + return findColumnForRef(ref, tables, n) +} + func findColumnForRef(ref *ast.ColumnRef, tables []*Table, selectStatement *ast.SelectStmt) error { parts := stringSlice(ref.Fields) var alias, name string diff --git a/internal/compiler/parse_test.go b/internal/compiler/parse_test.go new file mode 100644 index 0000000000..853b6425d0 --- /dev/null +++ b/internal/compiler/parse_test.go @@ -0,0 +1,104 @@ +package compiler + +import ( + "fmt" + "strings" + "testing" + + "github.com/kyleconroy/sqlc/internal/config" + "github.com/kyleconroy/sqlc/internal/engine/dolphin" + "github.com/kyleconroy/sqlc/internal/engine/postgresql" + "github.com/kyleconroy/sqlc/internal/engine/sqlite" + "github.com/kyleconroy/sqlc/internal/opts" +) + +func Test_ParseQueryErrors(t *testing.T) { + for _, tc := range []struct { + name string + engine config.Engine + createStmt string + selectStmt string + parser Parser + wantErr error + }{ + { + name: "unreferenced order column postgresql", + engine: config.EnginePostgreSQL, + createStmt: `CREATE TABLE authors (id INT);`, + selectStmt: `SELECT id FROM authors ORDER BY foo;`, + parser: postgresql.NewParser(), + wantErr: fmt.Errorf(`column reference "foo" not found`), + }, + { + name: "unreferenced order column mysql", + engine: config.EngineMySQL, + createStmt: `CREATE TABLE authors (id INT);`, + selectStmt: `SELECT id FROM authors ORDER BY foo;`, + parser: dolphin.NewParser(), + wantErr: fmt.Errorf(`column reference "foo" not found`), + }, + { + name: "unreferenced order column sqlite", + engine: config.EngineSQLite, + createStmt: `CREATE TABLE authors (id INT);`, + selectStmt: `SELECT id FROM authors ORDER BY foo;`, + parser: sqlite.NewParser(), + wantErr: fmt.Errorf(`column reference "foo" not found`), + }, + { + name: "unreferenced group column postgresql", + engine: config.EnginePostgreSQL, + createStmt: `CREATE TABLE authors ( id INT );`, + selectStmt: `SELECT id FROM authors GROUP BY foo;`, + parser: postgresql.NewParser(), + wantErr: fmt.Errorf(`column reference "foo" not found`), + }, + { + name: "unreferenced group column mysql", + engine: config.EngineMySQL, + createStmt: `CREATE TABLE authors (id INT);`, + selectStmt: `SELECT id FROM authors GROUP BY foo;`, + parser: dolphin.NewParser(), + wantErr: fmt.Errorf(`column reference "foo" not found`), + }, + { + name: "unreferenced group column sqlite", + engine: config.EngineSQLite, + createStmt: `CREATE TABLE authors (id INT);`, + selectStmt: `SELECT id FROM authors GROUP BY foo;`, + parser: sqlite.NewParser(), + wantErr: fmt.Errorf(`column reference "foo" not found`), + }, + } { + t.Run(tc.name, func(t *testing.T) { + conf := config.SQL{ + Engine: tc.engine, + } + combo := config.CombinedSettings{} + comp := NewCompiler(conf, combo) + stmts, err := tc.parser.Parse(strings.NewReader(tc.createStmt)) + if err != nil { + t.Fatalf("cannot parse test catalog: %v", err) + } + err = comp.catalog.Update(stmts[0], comp) + if err != nil { + t.Fatalf("cannot update test catalog: %v", err) + } + stmts, err = tc.parser.Parse(strings.NewReader(tc.selectStmt)) + if err != nil { + t.Errorf("Parse failed: %v", err) + } + if len(stmts) != 1 { + t.Errorf("expected one statement, got %d", len(stmts)) + } + + _, err = comp.parseQuery(stmts[0].Raw, tc.selectStmt, opts.Parser{}) + if err == nil { + t.Fatalf("expected parseQuery to return an error, got nil") + } + if err.Error() != tc.wantErr.Error() { + t.Errorf("error message: want %s, got %s", tc.wantErr, err.Error()) + } + }) + } +} diff --git a/internal/endtoend/testdata/order_by_non_existing_column/postgresql/query.sql b/internal/endtoend/testdata/order_by_non_existing_column/postgresql/query.sql new file mode 100644 index 0000000000..b1a4b4f638 --- /dev/null +++ b/internal/endtoend/testdata/order_by_non_existing_column/postgresql/query.sql @@ -0,0 +1,8 @@ +-- Example queries for sqlc +CREATE TABLE authors ( + id INT +); + +-- name: ListAuthors :many +SELECT id FROM authors +ORDER BY adfadsf; \ No newline at end of file diff --git a/internal/endtoend/testdata/order_by_non_existing_column/postgresql/sqlc.yaml b/internal/endtoend/testdata/order_by_non_existing_column/postgresql/sqlc.yaml new file mode 100644 index 0000000000..c4b3831631 --- /dev/null +++ b/internal/endtoend/testdata/order_by_non_existing_column/postgresql/sqlc.yaml @@ -0,0 +1,7 @@ +version: 1 +packages: + - path: "go" + name: "querytest" + engine: "postgresql" + schema: "query.sql" + queries: "query.sql" \ No newline at end of file diff --git a/internal/endtoend/testdata/order_by_non_existing_column/postgresql/stderr.txt b/internal/endtoend/testdata/order_by_non_existing_column/postgresql/stderr.txt new file mode 100644 index 0000000000..a783df1baa --- /dev/null +++ b/internal/endtoend/testdata/order_by_non_existing_column/postgresql/stderr.txt @@ -0,0 +1,2 @@ +# package querytest +query.sql:8:10: column reference "adfadsf" not found From a49bddd4076e207137d77e56cf0e4904cb16bd48 Mon Sep 17 00:00:00 2001 From: Andreas Kutschera Date: Thu, 10 Nov 2022 19:35:21 +0100 Subject: [PATCH 2/6] test: move test cases to endtoend tests --- internal/compiler/parse_test.go | 104 ------------------ .../mysql/query.sql | 8 ++ .../mysql/sqlc.yaml | 7 ++ .../mysql/stderr.txt | 2 + .../sqlite/query.sql | 8 ++ .../sqlite/sqlc.yaml | 7 ++ .../sqlite/stderr.txt | 2 + 7 files changed, 34 insertions(+), 104 deletions(-) delete mode 100644 internal/compiler/parse_test.go create mode 100644 internal/endtoend/testdata/order_by_non_existing_column/mysql/query.sql create mode 100644 internal/endtoend/testdata/order_by_non_existing_column/mysql/sqlc.yaml create mode 100644 internal/endtoend/testdata/order_by_non_existing_column/mysql/stderr.txt create mode 100644 internal/endtoend/testdata/order_by_non_existing_column/sqlite/query.sql create mode 100644 internal/endtoend/testdata/order_by_non_existing_column/sqlite/sqlc.yaml create mode 100644 internal/endtoend/testdata/order_by_non_existing_column/sqlite/stderr.txt diff --git a/internal/compiler/parse_test.go b/internal/compiler/parse_test.go deleted file mode 100644 index 853b6425d0..0000000000 --- a/internal/compiler/parse_test.go +++ /dev/null @@ -1,104 +0,0 @@ -package compiler - -import ( - "fmt" - "strings" - "testing" - - "github.com/kyleconroy/sqlc/internal/config" - "github.com/kyleconroy/sqlc/internal/engine/dolphin" - "github.com/kyleconroy/sqlc/internal/engine/postgresql" - "github.com/kyleconroy/sqlc/internal/engine/sqlite" - "github.com/kyleconroy/sqlc/internal/opts" -) - -func Test_ParseQueryErrors(t *testing.T) { - for _, tc := range []struct { - name string - engine config.Engine - createStmt string - selectStmt string - parser Parser - wantErr error - }{ - { - name: "unreferenced order column postgresql", - engine: config.EnginePostgreSQL, - createStmt: `CREATE TABLE authors (id INT);`, - selectStmt: `SELECT id FROM authors ORDER BY foo;`, - parser: postgresql.NewParser(), - wantErr: fmt.Errorf(`column reference "foo" not found`), - }, - { - name: "unreferenced order column mysql", - engine: config.EngineMySQL, - createStmt: `CREATE TABLE authors (id INT);`, - selectStmt: `SELECT id FROM authors ORDER BY foo;`, - parser: dolphin.NewParser(), - wantErr: fmt.Errorf(`column reference "foo" not found`), - }, - { - name: "unreferenced order column sqlite", - engine: config.EngineSQLite, - createStmt: `CREATE TABLE authors (id INT);`, - selectStmt: `SELECT id FROM authors ORDER BY foo;`, - parser: sqlite.NewParser(), - wantErr: fmt.Errorf(`column reference "foo" not found`), - }, - { - name: "unreferenced group column postgresql", - engine: config.EnginePostgreSQL, - createStmt: `CREATE TABLE authors ( id INT );`, - selectStmt: `SELECT id FROM authors GROUP BY foo;`, - parser: postgresql.NewParser(), - wantErr: fmt.Errorf(`column reference "foo" not found`), - }, - { - name: "unreferenced group column mysql", - engine: config.EngineMySQL, - createStmt: `CREATE TABLE authors (id INT);`, - selectStmt: `SELECT id FROM authors GROUP BY foo;`, - parser: dolphin.NewParser(), - wantErr: fmt.Errorf(`column reference "foo" not found`), - }, - { - name: "unreferenced group column sqlite", - engine: config.EngineSQLite, - createStmt: `CREATE TABLE authors (id INT);`, - selectStmt: `SELECT id FROM authors GROUP BY foo;`, - parser: sqlite.NewParser(), - wantErr: fmt.Errorf(`column reference "foo" not found`), - }, - } { - t.Run(tc.name, func(t *testing.T) { - conf := config.SQL{ - Engine: tc.engine, - } - combo := config.CombinedSettings{} - comp := NewCompiler(conf, combo) - stmts, err := tc.parser.Parse(strings.NewReader(tc.createStmt)) - if err != nil { - t.Fatalf("cannot parse test catalog: %v", err) - } - err = comp.catalog.Update(stmts[0], comp) - if err != nil { - t.Fatalf("cannot update test catalog: %v", err) - } - stmts, err = tc.parser.Parse(strings.NewReader(tc.selectStmt)) - if err != nil { - t.Errorf("Parse failed: %v", err) - } - if len(stmts) != 1 { - t.Errorf("expected one statement, got %d", len(stmts)) - } - - _, err = comp.parseQuery(stmts[0].Raw, tc.selectStmt, opts.Parser{}) - if err == nil { - t.Fatalf("expected parseQuery to return an error, got nil") - } - if err.Error() != tc.wantErr.Error() { - t.Errorf("error message: want %s, got %s", tc.wantErr, err.Error()) - } - }) - } -} diff --git a/internal/endtoend/testdata/order_by_non_existing_column/mysql/query.sql b/internal/endtoend/testdata/order_by_non_existing_column/mysql/query.sql new file mode 100644 index 0000000000..b1a4b4f638 --- /dev/null +++ b/internal/endtoend/testdata/order_by_non_existing_column/mysql/query.sql @@ -0,0 +1,8 @@ +-- Example queries for sqlc +CREATE TABLE authors ( + id INT +); + +-- name: ListAuthors :many +SELECT id FROM authors +ORDER BY adfadsf; \ No newline at end of file diff --git a/internal/endtoend/testdata/order_by_non_existing_column/mysql/sqlc.yaml b/internal/endtoend/testdata/order_by_non_existing_column/mysql/sqlc.yaml new file mode 100644 index 0000000000..c4b3831631 --- /dev/null +++ b/internal/endtoend/testdata/order_by_non_existing_column/mysql/sqlc.yaml @@ -0,0 +1,7 @@ +version: 1 +packages: + - path: "go" + name: "querytest" + engine: "postgresql" + schema: "query.sql" + queries: "query.sql" \ No newline at end of file diff --git a/internal/endtoend/testdata/order_by_non_existing_column/mysql/stderr.txt b/internal/endtoend/testdata/order_by_non_existing_column/mysql/stderr.txt new file mode 100644 index 0000000000..a783df1baa --- /dev/null +++ b/internal/endtoend/testdata/order_by_non_existing_column/mysql/stderr.txt @@ -0,0 +1,2 @@ +# package querytest +query.sql:8:10: column reference "adfadsf" not found diff --git a/internal/endtoend/testdata/order_by_non_existing_column/sqlite/query.sql b/internal/endtoend/testdata/order_by_non_existing_column/sqlite/query.sql new file mode 100644 index 0000000000..b1a4b4f638 --- /dev/null +++ b/internal/endtoend/testdata/order_by_non_existing_column/sqlite/query.sql @@ -0,0 +1,8 @@ +-- Example queries for sqlc +CREATE TABLE authors ( + id INT +); + +-- name: ListAuthors :many +SELECT id FROM authors +ORDER BY adfadsf; \ No newline at end of file diff --git a/internal/endtoend/testdata/order_by_non_existing_column/sqlite/sqlc.yaml b/internal/endtoend/testdata/order_by_non_existing_column/sqlite/sqlc.yaml new file mode 100644 index 0000000000..c4b3831631 --- /dev/null +++ b/internal/endtoend/testdata/order_by_non_existing_column/sqlite/sqlc.yaml @@ -0,0 +1,7 @@ +version: 1 +packages: + - path: "go" + name: "querytest" + engine: "postgresql" + schema: "query.sql" + queries: "query.sql" \ No newline at end of file diff --git a/internal/endtoend/testdata/order_by_non_existing_column/sqlite/stderr.txt b/internal/endtoend/testdata/order_by_non_existing_column/sqlite/stderr.txt new file mode 100644 index 0000000000..a783df1baa --- /dev/null +++ b/internal/endtoend/testdata/order_by_non_existing_column/sqlite/stderr.txt @@ -0,0 +1,2 @@ +# package querytest +query.sql:8:10: column reference "adfadsf" not found From 0638f3716f3e34594e6ecc3f3c1d6e7ae7d83efe Mon Sep 17 00:00:00 2001 From: Andreas Kutschera Date: Sat, 12 Nov 2022 12:54:38 +0100 Subject: [PATCH 3/6] feat: add validate_order_by config option #1411 --- internal/compiler/expand.go | 2 +- internal/compiler/output_columns.go | 58 +++++++++++++++-------------- internal/compiler/parse.go | 4 +- internal/compiler/query_catalog.go | 4 +- internal/config/config.go | 3 +- internal/config/v_one.go | 6 +++ internal/config/v_two.go | 4 ++ 7 files changed, 47 insertions(+), 34 deletions(-) diff --git a/internal/compiler/expand.go b/internal/compiler/expand.go index 5c2e9c481a..f213f1e8ca 100644 --- a/internal/compiler/expand.go +++ b/internal/compiler/expand.go @@ -55,7 +55,7 @@ func (c *Compiler) quoteIdent(ident string) string { } func (c *Compiler) expandStmt(qc *QueryCatalog, raw *ast.RawStmt, node ast.Node) ([]source.Edit, error) { - tables, err := sourceTables(qc, node) + tables, err := sourceTables(qc, node, *c.conf.ValidateOrderBy) if err != nil { return nil, err } diff --git a/internal/compiler/output_columns.go b/internal/compiler/output_columns.go index dd88efc10c..d9c81c8bf7 100644 --- a/internal/compiler/output_columns.go +++ b/internal/compiler/output_columns.go @@ -14,11 +14,11 @@ import ( // OutputColumns determines which columns a statement will output func (c *Compiler) OutputColumns(stmt ast.Node) ([]*catalog.Column, error) { - qc, err := buildQueryCatalog(c.catalog, stmt) + qc, err := buildQueryCatalog(c.catalog, stmt, *c.conf.ValidateOrderBy) if err != nil { return nil, err } - cols, err := outputColumns(qc, stmt) + cols, err := outputColumns(qc, stmt, *c.conf.ValidateOrderBy) if err != nil { return nil, err } @@ -50,8 +50,8 @@ func hasStarRef(cf *ast.ColumnRef) bool { // // Return an error if column references are ambiguous // Return an error if column references don't exist -func outputColumns(qc *QueryCatalog, node ast.Node) ([]*Column, error) { - tables, err := sourceTables(qc, node) +func outputColumns(qc *QueryCatalog, node ast.Node, validateOrderBy bool) ([]*Column, error) { + tables, err := sourceTables(qc, node, validateOrderBy) if err != nil { return nil, err } @@ -72,32 +72,34 @@ func outputColumns(qc *QueryCatalog, node ast.Node) ([]*Column, error) { } } } - if n.SortClause != nil { - for _, item := range n.SortClause.Items { - sb, ok := item.(*ast.SortBy) - if !ok { - continue - } + if validateOrderBy { + if n.SortClause != nil { + for _, item := range n.SortClause.Items { + sb, ok := item.(*ast.SortBy) + if !ok { + continue + } - if err := findColumnForNode(sb.Node, tables, n); err != nil { - return nil, err + if err := findColumnForNode(sb.Node, tables, n); err != nil { + return nil, err + } } } - } - if n.WindowClause != nil { - for _, item := range n.WindowClause.Items { - sb, ok := item.(*ast.List) - if !ok { - continue - } - for _, single := range sb.Items { - caseExpr, ok := single.(*ast.CaseExpr) + if n.WindowClause != nil { + for _, item := range n.WindowClause.Items { + sb, ok := item.(*ast.List) if !ok { continue } + for _, single := range sb.Items { + caseExpr, ok := single.(*ast.CaseExpr) + if !ok { + continue + } - if err := findColumnForNode(caseExpr.Xpr, tables, n); err != nil { - return nil, err + if err := findColumnForNode(caseExpr.Xpr, tables, n); err != nil { + return nil, err + } } } } @@ -106,7 +108,7 @@ func outputColumns(qc *QueryCatalog, node ast.Node) ([]*Column, error) { // For UNION queries, targets is empty and we need to look for the // columns in Largs. if len(targets.Items) == 0 && n.Larg != nil { - return outputColumns(qc, n.Larg) + return outputColumns(qc, n.Larg, validateOrderBy) } case *ast.CallStmt: targets = &ast.List{} @@ -267,7 +269,7 @@ func outputColumns(qc *QueryCatalog, node ast.Node) ([]*Column, error) { case ast.EXISTS_SUBLINK: cols = append(cols, &Column{Name: name, DataType: "bool", NotNull: true}) case ast.EXPR_SUBLINK: - subcols, err := outputColumns(qc, n.Subselect) + subcols, err := outputColumns(qc, n.Subselect, validateOrderBy) if err != nil { return nil, err } @@ -303,7 +305,7 @@ func outputColumns(qc *QueryCatalog, node ast.Node) ([]*Column, error) { cols = append(cols, col) case *ast.SelectStmt: - subcols, err := outputColumns(qc, n) + subcols, err := outputColumns(qc, n, validateOrderBy) if err != nil { return nil, err } @@ -390,7 +392,7 @@ func isTableRequired(n ast.Node, col *Column, prior int) int { // Return an error if column references don't exist // Return an error if a table is referenced twice // Return an error if an unknown column is referenced -func sourceTables(qc *QueryCatalog, node ast.Node) ([]*Table, error) { +func sourceTables(qc *QueryCatalog, node ast.Node, validateOrderBy bool) ([]*Table, error) { var list *ast.List switch n := node.(type) { case *ast.DeleteStmt: @@ -447,7 +449,7 @@ func sourceTables(qc *QueryCatalog, node ast.Node) ([]*Table, error) { tables = append(tables, table) case *ast.RangeSubselect: - cols, err := outputColumns(qc, n.Subquery) + cols, err := outputColumns(qc, n.Subquery, validateOrderBy) if err != nil { return nil, err } diff --git a/internal/compiler/parse.go b/internal/compiler/parse.go index 0cc76b0728..2648ce7e8c 100644 --- a/internal/compiler/parse.go +++ b/internal/compiler/parse.go @@ -83,7 +83,7 @@ func (c *Compiler) parseQuery(stmt ast.Node, src string, o opts.Parser) (*Query, } else { sort.Slice(refs, func(i, j int) bool { return refs[i].ref.Number < refs[j].ref.Number }) } - qc, err := buildQueryCatalog(c.catalog, raw.Stmt) + qc, err := buildQueryCatalog(c.catalog, raw.Stmt, *c.conf.ValidateOrderBy) if err != nil { return nil, err } @@ -92,7 +92,7 @@ func (c *Compiler) parseQuery(stmt ast.Node, src string, o opts.Parser) (*Query, if err != nil { return nil, err } - cols, err := outputColumns(qc, raw.Stmt) + cols, err := outputColumns(qc, raw.Stmt, *c.conf.ValidateOrderBy) if err != nil { return nil, err } diff --git a/internal/compiler/query_catalog.go b/internal/compiler/query_catalog.go index 8dc0a0ac2c..c9e45ba575 100644 --- a/internal/compiler/query_catalog.go +++ b/internal/compiler/query_catalog.go @@ -12,7 +12,7 @@ type QueryCatalog struct { ctes map[string]*Table } -func buildQueryCatalog(c *catalog.Catalog, node ast.Node) (*QueryCatalog, error) { +func buildQueryCatalog(c *catalog.Catalog, node ast.Node, validateOrderBy bool) (*QueryCatalog, error) { var with *ast.WithClause switch n := node.(type) { case *ast.DeleteStmt: @@ -30,7 +30,7 @@ func buildQueryCatalog(c *catalog.Catalog, node ast.Node) (*QueryCatalog, error) if with != nil { for _, item := range with.Ctes.Items { if cte, ok := item.(*ast.CommonTableExpr); ok { - cols, err := outputColumns(qc, cte.Ctequery) + cols, err := outputColumns(qc, cte.Ctequery, validateOrderBy) if err != nil { return nil, err } diff --git a/internal/config/config.go b/internal/config/config.go index 5e8f14292d..eecbf5fa44 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -92,6 +92,7 @@ type SQL struct { Schema Paths `json:"schema" yaml:"schema"` Queries Paths `json:"queries" yaml:"queries"` StrictFunctionChecks bool `json:"strict_function_checks" yaml:"strict_function_checks"` + ValidateOrderBy *bool `json:"validate_order_by" yaml:"validate_order_by"` Gen SQLGen `json:"gen" yaml:"gen"` Codegen []Codegen `json:"codegen" yaml:"codegen"` } @@ -119,7 +120,7 @@ type SQLGo struct { EmitResultStructPointers bool `json:"emit_result_struct_pointers" yaml:"emit_result_struct_pointers"` EmitParamsStructPointers bool `json:"emit_params_struct_pointers" yaml:"emit_params_struct_pointers"` EmitMethodsWithDBArgument bool `json:"emit_methods_with_db_argument,omitempty" yaml:"emit_methods_with_db_argument"` - EmitPointersForNullTypes bool `json:"emit_pointers_for_null_types" yaml:"emit_pointers_for_null_types"` + EmitPointersForNullTypes bool `json:"emit_pointers_for_null_types" yaml:"emit_pointers_for_null_types"` EmitEnumValidMethod bool `json:"emit_enum_valid_method,omitempty" yaml:"emit_enum_valid_method"` EmitAllEnumValues bool `json:"emit_all_enum_values,omitempty" yaml:"emit_all_enum_values"` JSONTagsCaseStyle string `json:"json_tags_case_style,omitempty" yaml:"json_tags_case_style"` diff --git a/internal/config/v_one.go b/internal/config/v_one.go index b44ff659e4..24ea035907 100644 --- a/internal/config/v_one.go +++ b/internal/config/v_one.go @@ -43,6 +43,7 @@ type v1PackageSettings struct { OutputQuerierFileName string `json:"output_querier_file_name,omitempty" yaml:"output_querier_file_name"` OutputFilesSuffix string `json:"output_files_suffix,omitempty" yaml:"output_files_suffix"` StrictFunctionChecks bool `json:"strict_function_checks" yaml:"strict_function_checks"` + ValidateOrderBy *bool `json:"validate_order_by" yaml:"validate_order_by"` } func v1ParseConfig(rd io.Reader) (Config, error) { @@ -115,6 +116,10 @@ func (c *V1GenerateSettings) Translate() Config { } for _, pkg := range c.Packages { + if pkg.ValidateOrderBy == nil { + defaultValue := true + pkg.ValidateOrderBy = &defaultValue + } conf.SQL = append(conf.SQL, SQL{ Engine: pkg.Engine, Schema: pkg.Schema, @@ -146,6 +151,7 @@ func (c *V1GenerateSettings) Translate() Config { }, }, StrictFunctionChecks: pkg.StrictFunctionChecks, + ValidateOrderBy: pkg.ValidateOrderBy, }) } diff --git a/internal/config/v_two.go b/internal/config/v_two.go index f0bad47e78..6b0280ca91 100644 --- a/internal/config/v_two.go +++ b/internal/config/v_two.go @@ -97,6 +97,10 @@ func v2ParseConfig(rd io.Reader) (Config, error) { return conf, ErrPluginNotFound } } + if conf.SQL[j].ValidateOrderBy == nil { + defaultValidate := true + conf.SQL[j].ValidateOrderBy = &defaultValidate + } } return conf, nil } From 3d270fe75120be1f54940c5fa3199f41079bf96a Mon Sep 17 00:00:00 2001 From: Andreas Kutschera Date: Sat, 12 Nov 2022 14:08:10 +0100 Subject: [PATCH 4/6] feat: expand error message #1411 Tell the uses how to switch off validation here. --- internal/compiler/output_columns.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/compiler/output_columns.go b/internal/compiler/output_columns.go index d9c81c8bf7..d2b3c0fbf2 100644 --- a/internal/compiler/output_columns.go +++ b/internal/compiler/output_columns.go @@ -81,7 +81,7 @@ func outputColumns(qc *QueryCatalog, node ast.Node, validateOrderBy bool) ([]*Co } if err := findColumnForNode(sb.Node, tables, n); err != nil { - return nil, err + return nil, fmt.Errorf("%v: if you want to skip this validation, set 'validate_order_by' to false", err) } } } @@ -98,7 +98,7 @@ func outputColumns(qc *QueryCatalog, node ast.Node, validateOrderBy bool) ([]*Co } if err := findColumnForNode(caseExpr.Xpr, tables, n); err != nil { - return nil, err + return nil, fmt.Errorf("%v: if you want to skip this validation, set 'validate_order_by' to false", err) } } } From 930f7dafcc29f0071d719ac1ccdc9cc8f1f554b2 Mon Sep 17 00:00:00 2001 From: Andreas Kutschera Date: Sat, 12 Nov 2022 14:11:10 +0100 Subject: [PATCH 5/6] feat: add expanded error message to test #1411 --- .../testdata/order_by_non_existing_column/mysql/stderr.txt | 2 +- .../testdata/order_by_non_existing_column/postgresql/stderr.txt | 2 +- .../testdata/order_by_non_existing_column/sqlite/stderr.txt | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/endtoend/testdata/order_by_non_existing_column/mysql/stderr.txt b/internal/endtoend/testdata/order_by_non_existing_column/mysql/stderr.txt index a783df1baa..535be3d070 100644 --- a/internal/endtoend/testdata/order_by_non_existing_column/mysql/stderr.txt +++ b/internal/endtoend/testdata/order_by_non_existing_column/mysql/stderr.txt @@ -1,2 +1,2 @@ # package querytest -query.sql:8:10: column reference "adfadsf" not found +query.sql:7:1: column reference "adfadsf" not found: if you want to skip this validation, set 'validate_order_by' to false diff --git a/internal/endtoend/testdata/order_by_non_existing_column/postgresql/stderr.txt b/internal/endtoend/testdata/order_by_non_existing_column/postgresql/stderr.txt index a783df1baa..535be3d070 100644 --- a/internal/endtoend/testdata/order_by_non_existing_column/postgresql/stderr.txt +++ b/internal/endtoend/testdata/order_by_non_existing_column/postgresql/stderr.txt @@ -1,2 +1,2 @@ # package querytest -query.sql:8:10: column reference "adfadsf" not found +query.sql:7:1: column reference "adfadsf" not found: if you want to skip this validation, set 'validate_order_by' to false diff --git a/internal/endtoend/testdata/order_by_non_existing_column/sqlite/stderr.txt b/internal/endtoend/testdata/order_by_non_existing_column/sqlite/stderr.txt index a783df1baa..535be3d070 100644 --- a/internal/endtoend/testdata/order_by_non_existing_column/sqlite/stderr.txt +++ b/internal/endtoend/testdata/order_by_non_existing_column/sqlite/stderr.txt @@ -1,2 +1,2 @@ # package querytest -query.sql:8:10: column reference "adfadsf" not found +query.sql:7:1: column reference "adfadsf" not found: if you want to skip this validation, set 'validate_order_by' to false From 6312b24200cb2e78a33f4cd284745ebdb0774e2c Mon Sep 17 00:00:00 2001 From: Kyle Conroy Date: Thu, 8 Jun 2023 09:28:07 -0700 Subject: [PATCH 6/6] compiler: Add functions to the compiler struct Don't pass configuration around as a parameter --- internal/compiler/expand.go | 2 +- internal/compiler/output_columns.go | 28 ++++++++++--------- internal/compiler/parse.go | 4 +-- internal/compiler/query_catalog.go | 4 +-- internal/config/config.go | 2 +- internal/config/v_one.go | 8 +++--- internal/config/v_two.go | 4 +-- .../mysql/stderr.txt | 2 +- .../postgresql/stderr.txt | 2 +- .../sqlite/stderr.txt | 2 +- 10 files changed, 30 insertions(+), 28 deletions(-) diff --git a/internal/compiler/expand.go b/internal/compiler/expand.go index f213f1e8ca..f9e8a19a05 100644 --- a/internal/compiler/expand.go +++ b/internal/compiler/expand.go @@ -55,7 +55,7 @@ func (c *Compiler) quoteIdent(ident string) string { } func (c *Compiler) expandStmt(qc *QueryCatalog, raw *ast.RawStmt, node ast.Node) ([]source.Edit, error) { - tables, err := sourceTables(qc, node, *c.conf.ValidateOrderBy) + tables, err := c.sourceTables(qc, node) if err != nil { return nil, err } diff --git a/internal/compiler/output_columns.go b/internal/compiler/output_columns.go index d2b3c0fbf2..4ffb593274 100644 --- a/internal/compiler/output_columns.go +++ b/internal/compiler/output_columns.go @@ -14,11 +14,11 @@ import ( // OutputColumns determines which columns a statement will output func (c *Compiler) OutputColumns(stmt ast.Node) ([]*catalog.Column, error) { - qc, err := buildQueryCatalog(c.catalog, stmt, *c.conf.ValidateOrderBy) + qc, err := c.buildQueryCatalog(c.catalog, stmt) if err != nil { return nil, err } - cols, err := outputColumns(qc, stmt, *c.conf.ValidateOrderBy) + cols, err := c.outputColumns(qc, stmt) if err != nil { return nil, err } @@ -50,8 +50,8 @@ func hasStarRef(cf *ast.ColumnRef) bool { // // Return an error if column references are ambiguous // Return an error if column references don't exist -func outputColumns(qc *QueryCatalog, node ast.Node, validateOrderBy bool) ([]*Column, error) { - tables, err := sourceTables(qc, node, validateOrderBy) +func (c *Compiler) outputColumns(qc *QueryCatalog, node ast.Node) ([]*Column, error) { + tables, err := c.sourceTables(qc, node) if err != nil { return nil, err } @@ -72,6 +72,10 @@ func outputColumns(qc *QueryCatalog, node ast.Node, validateOrderBy bool) ([]*Co } } } + validateOrderBy := true + if c.conf.StrictOrderBy != nil { + validateOrderBy = *c.conf.StrictOrderBy + } if validateOrderBy { if n.SortClause != nil { for _, item := range n.SortClause.Items { @@ -79,9 +83,8 @@ func outputColumns(qc *QueryCatalog, node ast.Node, validateOrderBy bool) ([]*Co if !ok { continue } - if err := findColumnForNode(sb.Node, tables, n); err != nil { - return nil, fmt.Errorf("%v: if you want to skip this validation, set 'validate_order_by' to false", err) + return nil, fmt.Errorf("%v: if you want to skip this validation, set 'strict_order_by' to false", err) } } } @@ -96,9 +99,8 @@ func outputColumns(qc *QueryCatalog, node ast.Node, validateOrderBy bool) ([]*Co if !ok { continue } - if err := findColumnForNode(caseExpr.Xpr, tables, n); err != nil { - return nil, fmt.Errorf("%v: if you want to skip this validation, set 'validate_order_by' to false", err) + return nil, fmt.Errorf("%v: if you want to skip this validation, set 'strict_order_by' to false", err) } } } @@ -108,7 +110,7 @@ func outputColumns(qc *QueryCatalog, node ast.Node, validateOrderBy bool) ([]*Co // For UNION queries, targets is empty and we need to look for the // columns in Largs. if len(targets.Items) == 0 && n.Larg != nil { - return outputColumns(qc, n.Larg, validateOrderBy) + return c.outputColumns(qc, n.Larg) } case *ast.CallStmt: targets = &ast.List{} @@ -269,7 +271,7 @@ func outputColumns(qc *QueryCatalog, node ast.Node, validateOrderBy bool) ([]*Co case ast.EXISTS_SUBLINK: cols = append(cols, &Column{Name: name, DataType: "bool", NotNull: true}) case ast.EXPR_SUBLINK: - subcols, err := outputColumns(qc, n.Subselect, validateOrderBy) + subcols, err := c.outputColumns(qc, n.Subselect) if err != nil { return nil, err } @@ -305,7 +307,7 @@ func outputColumns(qc *QueryCatalog, node ast.Node, validateOrderBy bool) ([]*Co cols = append(cols, col) case *ast.SelectStmt: - subcols, err := outputColumns(qc, n, validateOrderBy) + subcols, err := c.outputColumns(qc, n) if err != nil { return nil, err } @@ -392,7 +394,7 @@ func isTableRequired(n ast.Node, col *Column, prior int) int { // Return an error if column references don't exist // Return an error if a table is referenced twice // Return an error if an unknown column is referenced -func sourceTables(qc *QueryCatalog, node ast.Node, validateOrderBy bool) ([]*Table, error) { +func (c *Compiler) sourceTables(qc *QueryCatalog, node ast.Node) ([]*Table, error) { var list *ast.List switch n := node.(type) { case *ast.DeleteStmt: @@ -449,7 +451,7 @@ func sourceTables(qc *QueryCatalog, node ast.Node, validateOrderBy bool) ([]*Tab tables = append(tables, table) case *ast.RangeSubselect: - cols, err := outputColumns(qc, n.Subquery, validateOrderBy) + cols, err := c.outputColumns(qc, n.Subquery) if err != nil { return nil, err } diff --git a/internal/compiler/parse.go b/internal/compiler/parse.go index 2648ce7e8c..998a23d0ba 100644 --- a/internal/compiler/parse.go +++ b/internal/compiler/parse.go @@ -83,7 +83,7 @@ func (c *Compiler) parseQuery(stmt ast.Node, src string, o opts.Parser) (*Query, } else { sort.Slice(refs, func(i, j int) bool { return refs[i].ref.Number < refs[j].ref.Number }) } - qc, err := buildQueryCatalog(c.catalog, raw.Stmt, *c.conf.ValidateOrderBy) + qc, err := c.buildQueryCatalog(c.catalog, raw.Stmt) if err != nil { return nil, err } @@ -92,7 +92,7 @@ func (c *Compiler) parseQuery(stmt ast.Node, src string, o opts.Parser) (*Query, if err != nil { return nil, err } - cols, err := outputColumns(qc, raw.Stmt, *c.conf.ValidateOrderBy) + cols, err := c.outputColumns(qc, raw.Stmt) if err != nil { return nil, err } diff --git a/internal/compiler/query_catalog.go b/internal/compiler/query_catalog.go index c9e45ba575..eecaf72de4 100644 --- a/internal/compiler/query_catalog.go +++ b/internal/compiler/query_catalog.go @@ -12,7 +12,7 @@ type QueryCatalog struct { ctes map[string]*Table } -func buildQueryCatalog(c *catalog.Catalog, node ast.Node, validateOrderBy bool) (*QueryCatalog, error) { +func (comp *Compiler) buildQueryCatalog(c *catalog.Catalog, node ast.Node) (*QueryCatalog, error) { var with *ast.WithClause switch n := node.(type) { case *ast.DeleteStmt: @@ -30,7 +30,7 @@ func buildQueryCatalog(c *catalog.Catalog, node ast.Node, validateOrderBy bool) if with != nil { for _, item := range with.Ctes.Items { if cte, ok := item.(*ast.CommonTableExpr); ok { - cols, err := outputColumns(qc, cte.Ctequery, validateOrderBy) + cols, err := comp.outputColumns(qc, cte.Ctequery) if err != nil { return nil, err } diff --git a/internal/config/config.go b/internal/config/config.go index eecbf5fa44..db5a6f4cc4 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -92,7 +92,7 @@ type SQL struct { Schema Paths `json:"schema" yaml:"schema"` Queries Paths `json:"queries" yaml:"queries"` StrictFunctionChecks bool `json:"strict_function_checks" yaml:"strict_function_checks"` - ValidateOrderBy *bool `json:"validate_order_by" yaml:"validate_order_by"` + StrictOrderBy *bool `json:"strict_order_by" yaml:"strict_order_by"` Gen SQLGen `json:"gen" yaml:"gen"` Codegen []Codegen `json:"codegen" yaml:"codegen"` } diff --git a/internal/config/v_one.go b/internal/config/v_one.go index 24ea035907..45320f00f5 100644 --- a/internal/config/v_one.go +++ b/internal/config/v_one.go @@ -43,7 +43,7 @@ type v1PackageSettings struct { OutputQuerierFileName string `json:"output_querier_file_name,omitempty" yaml:"output_querier_file_name"` OutputFilesSuffix string `json:"output_files_suffix,omitempty" yaml:"output_files_suffix"` StrictFunctionChecks bool `json:"strict_function_checks" yaml:"strict_function_checks"` - ValidateOrderBy *bool `json:"validate_order_by" yaml:"validate_order_by"` + StrictOrderBy *bool `json:"strict_order_by" yaml:"strict_order_by"` } func v1ParseConfig(rd io.Reader) (Config, error) { @@ -116,9 +116,9 @@ func (c *V1GenerateSettings) Translate() Config { } for _, pkg := range c.Packages { - if pkg.ValidateOrderBy == nil { + if pkg.StrictOrderBy == nil { defaultValue := true - pkg.ValidateOrderBy = &defaultValue + pkg.StrictOrderBy = &defaultValue } conf.SQL = append(conf.SQL, SQL{ Engine: pkg.Engine, @@ -151,7 +151,7 @@ func (c *V1GenerateSettings) Translate() Config { }, }, StrictFunctionChecks: pkg.StrictFunctionChecks, - ValidateOrderBy: pkg.ValidateOrderBy, + StrictOrderBy: pkg.StrictOrderBy, }) } diff --git a/internal/config/v_two.go b/internal/config/v_two.go index 6b0280ca91..f6b37effe0 100644 --- a/internal/config/v_two.go +++ b/internal/config/v_two.go @@ -97,9 +97,9 @@ func v2ParseConfig(rd io.Reader) (Config, error) { return conf, ErrPluginNotFound } } - if conf.SQL[j].ValidateOrderBy == nil { + if conf.SQL[j].StrictOrderBy == nil { defaultValidate := true - conf.SQL[j].ValidateOrderBy = &defaultValidate + conf.SQL[j].StrictOrderBy = &defaultValidate } } return conf, nil diff --git a/internal/endtoend/testdata/order_by_non_existing_column/mysql/stderr.txt b/internal/endtoend/testdata/order_by_non_existing_column/mysql/stderr.txt index 535be3d070..166178156e 100644 --- a/internal/endtoend/testdata/order_by_non_existing_column/mysql/stderr.txt +++ b/internal/endtoend/testdata/order_by_non_existing_column/mysql/stderr.txt @@ -1,2 +1,2 @@ # package querytest -query.sql:7:1: column reference "adfadsf" not found: if you want to skip this validation, set 'validate_order_by' to false +query.sql:7:1: column reference "adfadsf" not found: if you want to skip this validation, set 'strict_order_by' to false diff --git a/internal/endtoend/testdata/order_by_non_existing_column/postgresql/stderr.txt b/internal/endtoend/testdata/order_by_non_existing_column/postgresql/stderr.txt index 535be3d070..166178156e 100644 --- a/internal/endtoend/testdata/order_by_non_existing_column/postgresql/stderr.txt +++ b/internal/endtoend/testdata/order_by_non_existing_column/postgresql/stderr.txt @@ -1,2 +1,2 @@ # package querytest -query.sql:7:1: column reference "adfadsf" not found: if you want to skip this validation, set 'validate_order_by' to false +query.sql:7:1: column reference "adfadsf" not found: if you want to skip this validation, set 'strict_order_by' to false diff --git a/internal/endtoend/testdata/order_by_non_existing_column/sqlite/stderr.txt b/internal/endtoend/testdata/order_by_non_existing_column/sqlite/stderr.txt index 535be3d070..166178156e 100644 --- a/internal/endtoend/testdata/order_by_non_existing_column/sqlite/stderr.txt +++ b/internal/endtoend/testdata/order_by_non_existing_column/sqlite/stderr.txt @@ -1,2 +1,2 @@ # package querytest -query.sql:7:1: column reference "adfadsf" not found: if you want to skip this validation, set 'validate_order_by' to false +query.sql:7:1: column reference "adfadsf" not found: if you want to skip this validation, set 'strict_order_by' to false