Repository: sqlc-dev/sqlc
Pull request #2709 (2023-09-06 23:34:58)

fix(codegen/golang): Refactor imports code to match templates
Fixes https://github.com/sqlc-dev/sqlc/issues/2707

I took a bigger approach than https://github.com/sqlc-dev/sqlc/pull/2708 to fix this issue. The problem was that the `Fields` method didn't use the same logic as the `Pair` method. When a template only called `Pair` but the import code called `Fields`, we'd get a mismatch.

To fix this, I created a new method called `Pairs` that uses the same logic as the Pair method. I updated the import code to call this. It also checks if the Arg is a struct and checks the fields.

This code is really brittle and until we do a large refactoring, we will sadly still run into issues.


Patch:
diff --git a/Dockerfile b/Dockerfile
index e6c258e69..88a5995af 100644
--- a/Dockerfile
+++ b/Dockerfile
@@ -1,5 +1,5 @@
 # STEP 1: Build sqlc
-FROM golang:1.21.0 AS builder
+FROM golang:1.21.1 AS builder
 
 COPY . /workspace
 WORKDIR /workspace
diff --git a/internal/codegen/golang/gen.go b/internal/codegen/golang/gen.go
index 7cd0a8dcc..5407c2e48 100644
--- a/internal/codegen/golang/gen.go
+++ b/internal/codegen/golang/gen.go
@@ -303,7 +303,7 @@ func usesBatch(queries []Query) bool {
 
 func checkNoTimesForMySQLCopyFrom(queries []Query) error {
 	for _, q := range queries {
-		for _, f := range q.Arg.Fields() {
+		for _, f := range q.Arg.CopyFromMySQLFields() {
 			if f.Type == "time.Time" {
 				return fmt.Errorf("values with a timezone are not yet supported")
 			}
diff --git a/internal/codegen/golang/imports.go b/internal/codegen/golang/imports.go
index 71146ba64..ab5de6fa2 100644
--- a/internal/codegen/golang/imports.go
+++ b/internal/codegen/golang/imports.go
@@ -242,11 +242,9 @@ func (i *importer) interfaceImports() fileImports {
 					return true
 				}
 			}
-			if !q.Arg.isEmpty() {
-				for _, f := range q.Arg.Fields() {
-					if hasPrefixIgnoringSliceAndPointerPrefix(f.Type, name) {
-						return true
-					}
+			for _, f := range q.Arg.Pairs() {
+				if hasPrefixIgnoringSliceAndPointerPrefix(f.Type, name) {
+					return true
 				}
 			}
 		}
@@ -312,13 +310,20 @@ func (i *importer) queryImports(filename string) fileImports {
 					return true
 				}
 			}
-			if !q.Arg.isEmpty() {
-				for _, f := range q.Arg.Fields() {
+			// Check the fields of the argument struct if it's emitted
+			if q.Arg.EmitStruct() {
+				for _, f := range q.Arg.Struct.Fields {
 					if hasPrefixIgnoringSliceAndPointerPrefix(f.Type, name) {
 						return true
 					}
 				}
 			}
+			// Check the argument pairs inside the method definition
+			for _, f := range q.Arg.Pairs() {
+				if hasPrefixIgnoringSliceAndPointerPrefix(f.Type, name) {
+					return true
+				}
+			}
 		}
 		return false
 	})
@@ -441,15 +446,15 @@ func (i *importer) batchImports() fileImports {
 					return true
 				}
 			}
-			if !q.Arg.isEmpty() {
-				if q.Arg.EmitStruct() {
-					for _, f := range q.Arg.Struct.Fields {
-						if hasPrefixIgnoringSliceAndPointerPrefix(f.Type, name) {
-							return true
-						}
+			if q.Arg.EmitStruct() {
+				for _, f := range q.Arg.Struct.Fields {
+					if hasPrefixIgnoringSliceAndPointerPrefix(f.Type, name) {
+						return true
 					}
 				}
-				if hasPrefixIgnoringSliceAndPointerPrefix(q.Arg.Type(), name) {
+			}
+			for _, f := range q.Arg.Pairs() {
+				if hasPrefixIgnoringSliceAndPointerPrefix(f.Type, name) {
 					return true
 				}
 			}
diff --git a/internal/codegen/golang/query.go b/internal/codegen/golang/query.go
index 4fbecaffb..434a90946 100644
--- a/internal/codegen/golang/query.go
+++ b/internal/codegen/golang/query.go
@@ -38,21 +38,41 @@ func (v QueryValue) isEmpty() bool {
 	return v.Typ == "" && v.Name == "" && v.Struct == nil
 }
 
+type Argument struct {
+	Name string
+	Type string
+}
+
 func (v QueryValue) Pair() string {
-	if v.isEmpty() {
-		return ""
+	var out []string
+	for _, arg := range v.Pairs() {
+		out = append(out, arg.Name+" "+arg.Type)
 	}
+	return strings.Join(out, ",")
+}
 
-	var out []string
+// Return the argument name and type for query methods. Should only be used in
+// the context of method arguments.
+func (v QueryValue) Pairs() []Argument {
+	if v.isEmpty() {
+		return nil
+	}
 	if !v.EmitStruct() && v.IsStruct() {
+		var out []Argument
 		for _, f := range v.Struct.Fields {
-			out = append(out, toLowerCase(f.Name)+" "+f.Type)
+			out = append(out, Argument{
+				Name: toLowerCase(f.Name),
+				Type: f.Type,
+			})
 		}
-
-		return strings.Join(out, ",")
+		return out
+	}
+	return []Argument{
+		{
+			Name: v.Name,
+			Type: v.DefineType(),
+		},
 	}
-
-	return v.Name + " " + v.DefineType()
 }
 
 func (v QueryValue) SlicePair() string {
@@ -202,7 +222,11 @@ func (v QueryValue) Scan() string {
 	return "\n" + strings.Join(out, ",\n")
 }
 
-func (v QueryValue) Fields() []Field {
+// Deprecated: This method does not respect the Emit field set on the
+// QueryValue. It's used by the go-sql-driver-mysql/copyfromCopy.tmpl and should
+// not be used other places.
+func (v QueryValue) CopyFromMySQLFields() []Field {
+	// fmt.Printf("%#v\n", v)
 	if v.Struct != nil {
 		return v.Struct.Fields
 	}
diff --git a/internal/codegen/golang/templates/go-sql-driver-mysql/copyfromCopy.tmpl b/internal/codegen/golang/templates/go-sql-driver-mysql/copyfromCopy.tmpl
index b3e01f2bb..3bdc5d572 100644
--- a/internal/codegen/golang/templates/go-sql-driver-mysql/copyfromCopy.tmpl
+++ b/internal/codegen/golang/templates/go-sql-driver-mysql/copyfromCopy.tmpl
@@ -4,16 +4,16 @@
 var readerHandlerSequenceFor{{.MethodName}} uint32 = 1
 
 func convertRowsFor{{.MethodName}}(w *io.PipeWriter, {{.Arg.SlicePair}}) {
-	e := mysqltsv.NewEncoder(w, {{ len .Arg.Fields }}, nil)
+	e := mysqltsv.NewEncoder(w, {{ len .Arg.CopyFromMySQLFields }}, nil)
 	for _, row := range {{.Arg.Name}} {
 {{- with $arg := .Arg }}
-{{- range $arg.Fields}}
+{{- range $arg.CopyFromMySQLFields}}
 {{- if eq .Type "string"}}
-	e.AppendString({{if eq (len $arg.Fields) 1}}row{{else}}row.{{.Name}}{{end}})
+	e.AppendString({{if eq (len $arg.CopyFromMySQLFields) 1}}row{{else}}row.{{.Name}}{{end}})
 {{- else if eq .Type "[]byte"}}
-	e.AppendBytes({{if eq (len $arg.Fields) 1}}row{{else}}row.{{.Name}}{{end}})
+	e.AppendBytes({{if eq (len $arg.CopyFromMySQLFields) 1}}row{{else}}row.{{.Name}}{{end}})
 {{- else}}
-	e.AppendValue({{if eq (len $arg.Fields) 1}}row{{else}}row.{{.Name}}{{end}})
+	e.AppendValue({{if eq (len $arg.CopyFromMySQLFields) 1}}row{{else}}row.{{.Name}}{{end}})
 {{- end}}
 {{- end}}
 {{- end}}


Test patch:
diff --git a/internal/endtoend/testdata/query_parameter_limit_to_zero/postgresql/go/querier.go b/internal/endtoend/testdata/query_parameter_limit_to_zero/postgresql/go/querier.go
new file mode 100644
index 000000000..75a40f397
--- /dev/null
+++ b/internal/endtoend/testdata/query_parameter_limit_to_zero/postgresql/go/querier.go
@@ -0,0 +1,18 @@
+// Code generated by sqlc. DO NOT EDIT.
+// versions:
+//   sqlc v1.21.0
+
+package querytest
+
+import (
+	"context"
+)
+
+type Querier interface {
+	CreateAuthor(ctx context.Context, arg CreateAuthorParams) (Author, error)
+	DeleteAuthor(ctx context.Context, arg DeleteAuthorParams) error
+	GetAuthor(ctx context.Context, arg GetAuthorParams) (Author, error)
+	ListAuthors(ctx context.Context) ([]Author, error)
+}
+
+var _ Querier = (*Queries)(nil)
diff --git a/internal/endtoend/testdata/query_parameter_limit_to_zero/postgresql/sqlc.json b/internal/endtoend/testdata/query_parameter_limit_to_zero/postgresql/sqlc.json
index dd45084b0..94207244d 100644
--- a/internal/endtoend/testdata/query_parameter_limit_to_zero/postgresql/sqlc.json
+++ b/internal/endtoend/testdata/query_parameter_limit_to_zero/postgresql/sqlc.json
@@ -7,8 +7,9 @@
         "name": "querytest",
         "schema": "query.sql",
         "queries": "query.sql",
-        "query_parameter_limit": 0
+        "query_parameter_limit": 0,
+        "emit_interface": true
       }
     ]
   }
-  
\ No newline at end of file
+