Clean up, test, and document ContextType and ClientGetter options (#77)

## Summary:
ContextType is in use at Khan as a part of our ka-context system; it
basically just lets you configure the type to pass as the `ctx` argument
to genqlient helpers (or say to omit such an argument).  ClientGetter I
wrote thinking we might use it; then we didn't (because we have a few
different clients we may use) but it's not much code and may be helpful
to others.  In this commit I clean up, document, and add tests for both
options.

The cleanup is mainly for ClientGetter, which was kind of broken before
because it was a Go snippet but couldn't specify imports.  I was
thinking maybe you want to be able to write `ctx.Something()`, but I
just don't see how to make it work, so I made it a function of context,
which is probably the better idea anyway.

Additionally, I improved the documentation for both, and added tests for
those and several other config options that weren't completely tested.

Fixes #5.

Issue: #5

## Test plan:
make check


Author: benjaminjkraft

Reviewers: dnerdy, aberkan, MiguelCastillo

Required Reviewers: 

Approved By: dnerdy

Checks: ✅ Test (1.17), ✅ Test (1.16), ✅ Test (1.15), ✅ Test (1.14), ✅ Test (1.13), ✅ Lint, ✅ Test (1.17), ✅ Test (1.16), ✅ Test (1.15), ✅ Test (1.14), ✅ Test (1.13), ✅ Lint

Pull Request URL: #77

generate/config.go

@@ -53,16 +53,29 @@ type Config struct {
 	// Generated
 	Package string `yaml:"package"`
 
-	// Set to the fully-qualified name of a type which generated helpers should
-	// accept and use as the context.Context for HTTP requests.  Defaults to
-	// context.Context; set to the empty string to omit context entirely.
+	// Set to the fully-qualified name of a Go type which generated helpers
+	// should accept and use as the context.Context for HTTP requests.
+	//
+	// Defaults to context.Context; set to the empty string to omit context
+	// entirely (i.e. use context.Background()).  Must be a type which
+	// implements context.Context.
 	ContextType string `yaml:"context_type"`
 
-	// If set, a snippet of Go code to get a *graphql.Client from the context
-	// (which will be named ctx).  For example, this might do
-	// ctx.Value(myKey).(*graphql.Client).  If omitted, client must be
-	// passed to each method explicitly.
-	// TODO(#5): This is a bit broken, fix it.
+	// If set, a function to get a graphql.Client, perhaps from the context.
+	// By default, the client must be passed explicitly to each genqlient
+	// generated query-helper.
+	//
+	// This is useful if you have a shared client, either a global, or
+	// available from context, and don't want to pass it explicitly.  In this
+	// case the signature of the genqlient-generated helpers will omit the
+	// `graphql.Context` and they will call this function instead.
+	//
+	// Must be the fully-qualified name of a function which accepts a context
+	// (of the type configured as ContextType (above), which defaults to
+	// `context.Context`, or a function of no arguments if ContextType is set
+	// to the empty string) and returns (graphql.Client, error).  If the
+	// client-getter returns an error, the helper will return the error
+	// without making a query.
 	ClientGetter string `yaml:"client_getter"`
 
 	// A map from GraphQL type name to Go fully-qualified type name to override

generate/generate.go

@@ -7,6 +7,7 @@ package generate
 import (
 	"bytes"
 	"encoding/json"
+	"fmt"
 	"go/format"
 	"sort"
 	"strings"
@@ -66,7 +67,7 @@ type argument struct {
 	Options     *GenqlientDirective
 }
 
-func newGenerator(config *Config, schema *ast.Schema) *generator {
+func newGenerator(config *Config, schema *ast.Schema) (*generator, error) {
 	g := generator{
 		Config:        config,
 		typeMap:       map[string]goType{},
@@ -76,20 +77,26 @@ func newGenerator(config *Config, schema *ast.Schema) *generator {
 		schema:        schema,
 	}
 
-	if g.Config.ClientGetter == "" {
-		_, err := g.addRef("github.com/Khan/genqlient/graphql.Client")
+	_, err := g.addRef("github.com/Khan/genqlient/graphql.Client")
+	if err != nil {
+		return nil, err
+	}
+
+	if g.Config.ClientGetter != "" {
+		_, err := g.addRef(g.Config.ClientGetter)
 		if err != nil {
-			panic(err)
+			return nil, fmt.Errorf("invalid client_getter: %w", err)
 		}
 	}
+
 	if g.Config.ContextType != "" {
 		_, err := g.addRef(g.Config.ContextType)
 		if err != nil {
-			panic(err)
+			return nil, fmt.Errorf("invalid context_type: %w", err)
 		}
 	}
 
-	return &g
+	return &g, nil
 }
 
 func (g *generator) Types() (string, error) {
@@ -305,7 +312,11 @@ func Generate(config *Config) (map[string][]byte, error) {
 	// Step 2: For each operation, convert it into data structures representing
 	// Go types (defined in types.go).  The bulk of this logic is in
 	// convert.go.
-	g := newGenerator(config, schema)
+	g, err := newGenerator(config, schema)
+	if err != nil {
+		return nil, err
+	}
+
 	for _, op := range document.Operations {
 		if err = g.addOperation(op); err != nil {
 			return nil, err

generate/generate_test.go

@@ -18,6 +18,36 @@ const (
 	errorsDir = "testdata/errors"
 )
 
+// buildGoFile returns an error if the given Go code is not valid.
+//
+// namePrefix is used for the temp-file, and is just for debugging.
+func buildGoFile(namePrefix string, content []byte) error {
+	// We need to put this within the current module, rather than in
+	// /tmp, so that it can access internal/testutil.
+	f, err := ioutil.TempFile("./testdata/tmp", namePrefix+"_*.go")
+	if err != nil {
+		return err
+	}
+	defer func() {
+		f.Close()
+		os.Remove(f.Name())
+	}()
+
+	_, err = f.Write(content)
+	if err != nil {
+		return err
+	}
+
+	cmd := exec.Command("go", "build", f.Name())
+	cmd.Stdout = os.Stdout
+	cmd.Stderr = os.Stderr
+	err = cmd.Run()
+	if err != nil {
+		return fmt.Errorf("generated code does not compile: %w", err)
+	}
+	return nil
+}
+
 // TestGenerate is a snapshot-based test of code-generation.
 //
 // This file just has the test runner; the actual data is all in
@@ -91,29 +121,102 @@ func TestGenerate(t *testing.T) {
 						"https://github.com/Khan/genqlient/issues/43")
 				}
 
-				goContent := generated[goFilename]
-				// We need to put this within the current module, rather than in
-				// /tmp, so that it can access internal/testutil.
-				f, err := ioutil.TempFile("./testdata/tmp", sourceFilename+"_*.go")
+				err := buildGoFile(sourceFilename, generated[goFilename])
 				if err != nil {
-					t.Fatal(err)
+					t.Error(err)
 				}
-				defer func() {
-					f.Close()
-					os.Remove(f.Name())
-				}()
+			})
+		})
+	}
+}
 
-				_, err = f.Write(goContent)
-				if err != nil {
-					t.Fatal(err)
+// TestGenerateWithConfig tests several configuration options that affect
+// generated code but don't require particular query structures to test.
+//
+// It runs a simple query from TestGenerate with several different genqlient
+// configurations.  It uses snapshots, just like TestGenerate.
+func TestGenerateWithConfig(t *testing.T) {
+	tests := []struct {
+		name               string
+		fakeConfigFilename string
+		config             *Config // omits Schema and Operations, set below.
+	}{
+		{"DefaultConfig", "genqlient.yaml", defaultConfig},
+		{"Subpackage", "genqlient.yaml", &Config{
+			Generated:   "mypkg/myfile.go",
+			ContextType: "context.Context", // (from defaultConfig)
+		}},
+		{"SubpackageConfig", "mypkg/genqlient.yaml", &Config{
+			Generated:   "myfile.go", // (relative to genqlient.yaml)
+			ContextType: "context.Context",
+		}},
+		{"PackageName", "genqlient.yaml", &Config{
+			Generated:   "myfile.go",
+			Package:     "mypkg",
+			ContextType: "context.Context",
+		}},
+		{"ExportOperations", "genqlient.yaml", &Config{
+			Generated:        "generated.go",
+			ExportOperations: "operations.json",
+			ContextType:      "context.Context",
+		}},
+		{"CustomContext", "genqlient.yaml", &Config{
+			Generated:   "generated.go",
+			ContextType: "github.com/Khan/genqlient/internal/testutil.MyContext",
+		}},
+		{"NoContext", "genqlient.yaml", &Config{
+			Generated:   "generated.go",
+			ContextType: "",
+		}},
+		{"ClientGetter", "genqlient.yaml", &Config{
+			Generated:    "generated.go",
+			ClientGetter: "github.com/Khan/genqlient/internal/testutil.GetClientFromContext",
+			ContextType:  "context.Context",
+		}},
+		{"ClientGetterCustomContext", "genqlient.yaml", &Config{
+			Generated:    "generated.go",
+			ClientGetter: "github.com/Khan/genqlient/internal/testutil.GetClientFromMyContext",
+			ContextType:  "github.com/Khan/genqlient/internal/testutil.MyContext",
+		}},
+		{"ClientGetterNoContext", "genqlient.yaml", &Config{
+			Generated:    "generated.go",
+			ClientGetter: "github.com/Khan/genqlient/internal/testutil.GetClientFromNowhere",
+			ContextType:  "",
+		}},
+	}
+
+	sourceFilename := "SimpleQuery.graphql"
+
+	for _, test := range tests {
+		config := test.config
+		t.Run(test.name, func(t *testing.T) {
+			err := config.ValidateAndFillDefaults(
+				filepath.Join(dataDir, test.fakeConfigFilename))
+			config.Schema = filepath.Join(dataDir, "schema.graphql")
+			config.Operations = []string{filepath.Join(dataDir, sourceFilename)}
+			if err != nil {
+				t.Fatal(err)
+			}
+			generated, err := Generate(config)
+			if err != nil {
+				t.Fatal(err)
+			}
+
+			for filename, content := range generated {
+				t.Run(filename, func(t *testing.T) {
+					testutil.Cupaloy.SnapshotT(t, string(content))
+				})
+			}
+
+			t.Run("Build", func(t *testing.T) {
+				if testing.Short() {
+					t.Skip("skipping build due to -short")
 				}
 
-				cmd := exec.Command("go", "build", f.Name())
-				cmd.Stdout = os.Stdout
-				cmd.Stderr = os.Stderr
-				err = cmd.Run()
+				err := buildGoFile(sourceFilename,
+					generated[config.Generated])
 				if err != nil {
-					t.Fatal(fmt.Errorf("generated code does not compile: %w", err))
+					t.Error(err)
 				}
 			})
 		})

generate/operation.go.tmpl

@@ -43,9 +43,18 @@ func {{.Name}}(
     }
     {{end}}
     {{end}}
+    {{end -}}
+
+    var err error
+    {{if $.Config.ClientGetter -}}
+    client, err := {{ref $.Config.ClientGetter}}({{if $.Config.ContextType}}ctx{{else}}{{end}})
+    if err != nil {
+        return nil, err
+    }
     {{end}}
+
 	var retval {{.ResponseName}}
-    err := {{if $.Config.ClientGetter}}{{$.Config.ClientGetter}}{{else}}client{{end}}.MakeRequest(
+    err = client.MakeRequest(
         {{if $.Config.ContextType}}ctx{{else}}nil{{end}},
         "{{.Name}}",
         `{{.Body}}`,