fix(codegen): Never generate return structs for :exec (#2976)

kyleconroy · web-flow · commit 721eba09793f · 2023-11-15T09:44:52.000-08:00
* fix(codegen): Never generate return structs for :exec

* fix(validate): Ensure batch commands have return values

* Fix test output
diff --git a/internal/codegen/golang/result.go b/internal/codegen/golang/result.go
@@ -312,16 +312,16 @@ func buildQueries(req *plugin.GenerateRequest, options *opts.Options, structs []
 	return qs, nil
 }
 
+var cmdReturnsData = map[string]struct{}{
+	metadata.CmdBatchMany: {},
+	metadata.CmdBatchOne:  {},
+	metadata.CmdMany:      {},
+	metadata.CmdOne:       {},
+}
+
 func putOutColumns(query *plugin.Query) bool {
-	if len(query.Columns) > 0 {
-		return true
-	}
-	for _, allowed := range []string{metadata.CmdMany, metadata.CmdOne, metadata.CmdBatchMany} {
-		if query.Cmd == allowed {
-			return true
-		}
-	}
-	return false
+	_, found := cmdReturnsData[query.Cmd]
+	return found
 }
 
 // It's possible that this method will generate duplicate JSON tag values
diff --git a/internal/codegen/golang/result_test.go b/internal/codegen/golang/result_test.go
@@ -50,7 +50,7 @@ func TestPutOutColumns_ForZeroColumns(t *testing.T) {
 		},
 		{
 			cmd:  metadata.CmdBatchOne,
-			want: false,
+			want: true,
 		},
 	}
 	for _, tc := range tests {
diff --git a/internal/endtoend/testdata/cte_left_join/postgresql/pgx/go/query.sql.go b/internal/endtoend/testdata/cte_left_join/postgresql/pgx/go/query.sql.go
diff --git a/internal/endtoend/testdata/exec_no_return_struct/issue.md b/internal/endtoend/testdata/exec_no_return_struct/issue.md
@@ -0,0 +1 @@
+https://github.com/sqlc-dev/sqlc/issues/2970
diff --git a/internal/endtoend/testdata/exec_no_return_struct/postgresql/pgx/go/db.go b/internal/endtoend/testdata/exec_no_return_struct/postgresql/pgx/go/db.go
diff --git a/internal/endtoend/testdata/exec_no_return_struct/postgresql/pgx/go/models.go b/internal/endtoend/testdata/exec_no_return_struct/postgresql/pgx/go/models.go
diff --git a/internal/endtoend/testdata/exec_no_return_struct/postgresql/pgx/go/query.sql.go b/internal/endtoend/testdata/exec_no_return_struct/postgresql/pgx/go/query.sql.go
diff --git a/internal/endtoend/testdata/exec_no_return_struct/postgresql/pgx/query.sql b/internal/endtoend/testdata/exec_no_return_struct/postgresql/pgx/query.sql
@@ -0,0 +1,13 @@
+-- name: ResetTaskRun :exec
+UPDATE job
+SET last_run = NOW()
+FROM (
+        SELECT last_run,
+            task_name
+        FROM job AS o
+        WHERE o.task_name = $1 
+  		FOR UPDATE
+    ) AS old_values
+WHERE job.task_name = $1
+RETURNING job.last_run AS this_run,
+    old_values.last_run AS last_run;
diff --git a/internal/endtoend/testdata/exec_no_return_struct/postgresql/pgx/schema.sql b/internal/endtoend/testdata/exec_no_return_struct/postgresql/pgx/schema.sql
@@ -0,0 +1,5 @@
+CREATE TABLE IF NOT EXISTS job
+(
+    task_name text NOT NULL,
+    last_run timestamp with time zone DEFAULT now() NOT NULL
+);
diff --git a/internal/endtoend/testdata/exec_no_return_struct/postgresql/pgx/sqlc.yaml b/internal/endtoend/testdata/exec_no_return_struct/postgresql/pgx/sqlc.yaml
@@ -0,0 +1,10 @@
+version: "2"
+sql:
+  - engine: "postgresql"
+    schema: "schema.sql"
+    queries: "query.sql"
+    gen:
+      go:
+        package: "querytest"
+        out: "go"
+        sql_package: "pgx/v5"
diff --git a/internal/endtoend/testdata/invalid_queries_foo/pgx/v4/query.sql b/internal/endtoend/testdata/invalid_queries_foo/pgx/v4/query.sql
@@ -15,3 +15,6 @@ UPDATE foo SET id = $2 WHERE id = $1;
 
 -- name: InsertFoo :one
 INSERT INTO foo (id) VALUES ($1);
+
+-- name: InsertFoo :batchone
+INSERT INTO foo (id) VALUES ($1);
diff --git a/internal/endtoend/testdata/invalid_queries_foo/pgx/v4/stderr.txt b/internal/endtoend/testdata/invalid_queries_foo/pgx/v4/stderr.txt
@@ -4,4 +4,5 @@ query.sql:5:1: invalid query comment: -- name: ListFoos :one :many
 query.sql:8:1: invalid query type: :two
 query.sql:11:1: query "DeleteFoo" specifies parameter ":one" without containing a RETURNING clause
 query.sql:14:1: query "UpdateFoo" specifies parameter ":one" without containing a RETURNING clause
-query.sql:17:1: query "InsertFoo" specifies parameter ":one" without containing a RETURNING clause
+query.sql:17:1: query "InsertFoo" specifies parameter ":one" without containing a RETURNING clause
+query.sql:20:1: query "InsertFoo" specifies parameter ":batchone" without containing a RETURNING clause
diff --git a/internal/endtoend/testdata/invalid_queries_foo/pgx/v5/query.sql b/internal/endtoend/testdata/invalid_queries_foo/pgx/v5/query.sql
@@ -15,3 +15,6 @@ UPDATE foo SET id = $2 WHERE id = $1;
 
 -- name: InsertFoo :one
 INSERT INTO foo (id) VALUES ($1);
+
+-- name: InsertFoo :batchone
+INSERT INTO foo (id) VALUES ($1);
diff --git a/internal/endtoend/testdata/invalid_queries_foo/pgx/v5/stderr.txt b/internal/endtoend/testdata/invalid_queries_foo/pgx/v5/stderr.txt
@@ -4,4 +4,5 @@ query.sql:5:1: invalid query comment: -- name: ListFoos :one :many
 query.sql:8:1: invalid query type: :two
 query.sql:11:1: query "DeleteFoo" specifies parameter ":one" without containing a RETURNING clause
 query.sql:14:1: query "UpdateFoo" specifies parameter ":one" without containing a RETURNING clause
-query.sql:17:1: query "InsertFoo" specifies parameter ":one" without containing a RETURNING clause
+query.sql:17:1: query "InsertFoo" specifies parameter ":one" without containing a RETURNING clause
+query.sql:20:1: query "InsertFoo" specifies parameter ":batchone" without containing a RETURNING clause
diff --git a/internal/sql/validate/cmd.go b/internal/sql/validate/cmd.go
@@ -67,9 +67,11 @@ func Cmd(n ast.Node, name, cmd string) error {
 		return validateCopyfrom(n)
 	}
 	if (cmd == metadata.CmdBatchExec || cmd == metadata.CmdBatchMany) || cmd == metadata.CmdBatchOne {
-		return validateBatch(n)
+		if err := validateBatch(n); err != nil {
+			return err
+		}
 	}
-	if !(cmd == metadata.CmdMany || cmd == metadata.CmdOne) {
+	if !(cmd == metadata.CmdMany || cmd == metadata.CmdOne || cmd == metadata.CmdBatchMany || cmd == metadata.CmdBatchOne) {
 		return nil
 	}
 	var list *ast.List

Original file line number	Diff line number	Diff line change
`@@ -50,7 +50,7 @@ func TestPutOutColumns_ForZeroColumns(t *testing.T) {`
`50`	`50`	`},`
`51`	`51`	`{`
`52`	`52`	`cmd: metadata.CmdBatchOne,`
`53`		`- want: false,`
	`53`	`+ want: true,`
`54`	`54`	`},`
`55`	`55`	`}`
`56`	`56`	`for _, tc := range tests {`
Original file line number	Diff line number	Diff line change
`@@ -0,0 +1 @@`
	`1`	`+https://github.com/sqlc-dev/sqlc/issues/2970`
Original file line number	Diff line number	Diff line change
`@@ -67,9 +67,11 @@ func Cmd(n ast.Node, name, cmd string) error {`
`67`	`67`	`return validateCopyfrom(n)`
`68`	`68`	`}`
`69`	`69`	`if (cmd == metadata.CmdBatchExec \|\| cmd == metadata.CmdBatchMany) \|\| cmd == metadata.CmdBatchOne {`
`70`		`- return validateBatch(n)`
	`70`	`+ if err := validateBatch(n); err != nil {`
	`71`	`+ return err`
	`72`	`+ }`
`71`	`73`	`}`
`72`		`- if !(cmd == metadata.CmdMany \|\| cmd == metadata.CmdOne) {`
	`74`	`+ if !(cmd == metadata.CmdMany \|\| cmd == metadata.CmdOne \|\| cmd == metadata.CmdBatchMany \|\| cmd == metadata.CmdBatchOne) {`
`73`	`75`	`return nil`
`74`	`76`	`}`
`75`	`77`	`var list *ast.List`