Repository: Khan/genqlient
Pull request #198 (2022-05-18 18:36:33)

wip: Add support for mapping all nullable types as pointers
This is a bare minimum implementation of the approach suggested in https://github.com/Khan/genqlient/issues/178#issuecomment-1077559343, which I figured I'd open sooner than later for early feedback!

Per the new detail in the sample `docs/genqlient.yaml`:

```yaml
# Customize how optional fields are handled.
optional:
  # Customize how models are generated for optional fields. This can currently
  # be set to one of the following values:
  # - value (default): optional fields are generated as values, the same as
  #   non-optional fields. E.g. fields with GraphQL types `String` or `String!`
  #   will both map to the Go type `string`. When values are absent in
  #   responses the zero value will be used.
  # - pointer: optional fields are generated as pointers. E.g. fields with
  #   GraphQL type `String` will map to the Go type `*string`. When values are
  #   absent in responses `nil` will be used.
  output: value
```

The documentation is slightly misleading atm as there's actually no validation of the values, and only `pointer` will have any effect (anything else has the default behaviour). It also affects arguments, no just fields, which is desirable imo. There are also no tests, as I'm not quite sure where best to put them 😅 Any pointers (pun intended?) would be great.

I've tested this out with a small local setup, which isn't fully integrated yet, so I'm not at all confident that it fully works. It does generate what I'd expect for a simple schema, e.g. given:

```graphql
# schema.graphql
scalar DateTime

type Query {
  user(id: ID!): User
}

type User {
  avatarUrl: String
  brandID: ID
  brandVisible: Boolean
  createdAt: DateTime
  firstName: String
  id: ID!
  lastName: String
  name: String
  primaryEmail: String
}
```

And:

```graphql
# operations.graphql
query userGetByID($id: ID!) {
  user(id: $id) {
    avatarUrl
    brandID
    brandVisible
    createdAt
    firstName
    id
    lastName
    name
    primaryEmail
  }
}
```

It will generate:

```go
func userGetByID(
	ctx context.Context,
	client graphql.Client,
	id string,
) (*userGetByIDResponse, error) {
	/* ... */
}

type userGetByIDResponse struct {
	User *userGetByIDUser `json:"user"`
}

type userGetByIDUser struct {
	AvatarUrl *string `json:"avatarUrl"`
	BrandID *string `json:"brandID"`
	BrandVisible *bool `json:"brandVisible"`
	CreatedAt *time.Time `json:"createdAt"`
	FirstName *string `json:"firstName"`
	Id string `json:"id"`
	LastName *string `json:"lastName"`
	Name *string `json:"name"`
	PrimaryEmail *string `json:"primaryEmail"`
}

// ...
```

---

I have:
- [x] Written a clear PR title and description (above)
- [x] Signed the [Khan Academy CLA](https://www.khanacademy.org/r/cla)
- [x] Added tests covering my changes, if applicable
- [x] Included a link to the issue fixed, if applicable
- [x] Included documentation, for new features
- [x] Added an entry to the changelog


StevenACoffman: Hi! The majority of gqlgen users are using versions where resolvers return pointers, so I think that should be the default now and people can opt-in to the value behavior.

Otherwise, it's kind of a jarring change.
benjaminjkraft: This seems like a great start, thank you!  Fingers crossed that the tests don't find any place it needs to be more complicated.

> It also affects arguments, no just fields, which is desirable imo.

If that's simpler, and seems to work for you, I think it's fine!  Only thing is to rename the option to match.  Maybe just `optional: pointer` and later if we add more knobs that can be a shorthand for something else.

> There are also no tests, as I'm not quite sure where best to put them

Hmm, slightly awkward actually.  In general this sort of thing goes in `TestGenerateWithConfig`, but you'll definitely need to test it on a more complex query than it currently uses to get any interesting behavior.  (Currently it uses `generate/testdata/queries/SimpleQuery.graphql`, the schema it uses is in `generate/testdata/queries/schema.graphql`.  This writes out to various snapshots in `generate/testdata/snapshots`, as well as checking the snapshots compile.)  Probably the easiest thing is just to add the query to use as a parameter to the table-driven test (currently it's just a constant `sourceFilename`), so you can add a few tests using more complicated queries (or use some of the existing ones in the same directory, which are probably plenty complex).

Then the other thing is whether we need an integration test.  If we do, it will be a bit annoying, since this is a global config option and those all share one config.  But if we are able to stick to doing exactly the same thing as the `pointer: true` option, that may not be necessary.
connec: Thanks for the fast feedback! I'll reconsider the naming and investigate and adding a test, though it will likely be a couple of days before I get to it.
connec: I've added a couple of tests, one that has a few nullable outputs (including lists) and one with some nullable inputs. The result is what I expect.
benjaminjkraft: Neat, thanks for putting this together!

Only thing I notice is that a value of type `[String]` becomes `[]*string`, not `*[]*string`, which in theory might be what one would expect.  But I think that's probably fine, since slices already have distinct nil vs. empty slices, and it's more consistent with how the `pointer` option already works.  Which is for the best because it might be quite annoying to implement (would require redoing the handling of such types in marshalers, which is necessarily very tricky code to work with).  I'm gonna add a few words to the documentation to mention that, and then merge.

Patch:
diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md
index 922ea66..f0d2f5d 100644
--- a/docs/CHANGELOG.md
+++ b/docs/CHANGELOG.md
@@ -31,6 +31,7 @@ When releasing a new version:
 - You can now enable `use_extensions` in the configuration file, to receive extensions returned by the GraphQL API server. Generated functions will return extensions as `map[string]interface{}`, if enabled.
 - You can now use `graphql.NewClientUsingGet` to create a client that uses query parameters to pass the query to the GraphQL API server.
 - In config files, `schema`, `operations`, and `generated` can now be absolute paths.
+- You can now configure how nullable types are mapped to Go types in the configuration file. Specifically, you can set `optional: pointer` to have all nullable GraphQL arguments, input fields, and output fields map to pointers.
 
 ### Bug fixes:
 
diff --git a/docs/genqlient.yaml b/docs/genqlient.yaml
index d30235b..37bb8e9 100644
--- a/docs/genqlient.yaml
+++ b/docs/genqlient.yaml
@@ -98,6 +98,23 @@ use_struct_references: boolean
 use_extensions: boolean
 
 
+# Customize how optional fields are handled.
+optional:
+  # Customize how models are generated for optional fields. This can currently
+  # be set to one of the following values:
+  # - value (default): optional fields are generated as values, the same as
+  #   non-optional fields. E.g. fields with GraphQL types `String` or `String!`
+  #   will both map to the Go type `string`. When values are absent in
+  #   responses the zero value will be used.
+  # - pointer: optional fields are generated as pointers. E.g. fields with
+  #   GraphQL type `String` will map to the Go type `*string`. When values are
+  #   absent in responses `nil` will be used. Optional list fields do not use
+  #   pointers-to-slices, so the GraphQL type `[String]` will map to the Go
+  #   type `[]*string`, not `*[]*string`; GraphQL null and empty list simply
+  #   map to Go nil- and empty-slice.
+  output: value
+
+
 # A map from GraphQL type name to Go fully-qualified type name to override
 # the Go type genqlient will use for this GraphQL type.
 #
diff --git a/generate/config.go b/generate/config.go
index 0654a33..d6f057d 100644
--- a/generate/config.go
+++ b/generate/config.go
@@ -26,6 +26,7 @@ type Config struct {
 	ContextType      string                  `yaml:"context_type"`
 	ClientGetter     string                  `yaml:"client_getter"`
 	Bindings         map[string]*TypeBinding `yaml:"bindings"`
+	Optional         string                  `yaml:"optional"`
 	StructReferences bool                    `yaml:"use_struct_references"`
 	Extensions       bool                    `yaml:"use_extensions"`
 
diff --git a/generate/convert.go b/generate/convert.go
index 38035f5..adfbf0c 100644
--- a/generate/convert.go
+++ b/generate/convert.go
@@ -256,7 +256,7 @@ func (g *generator) convertType(
 			oe := true
 			options.Omitempty = &oe
 		}
-	} else if options.GetPointer() {
+	} else if options.GetPointer() || (!typ.NonNull && g.Config.Optional == "pointer") {
 		// Whatever we get, wrap it in a pointer.  (Because of the way the
 		// options work, recursing here isn't as connvenient.)
 		// Note this does []*T or [][]*T, not e.g. *[][]T.  See #16.


Test patch:
diff --git a/generate/generate_test.go b/generate/generate_test.go
index 64690a2..21320fe 100644
--- a/generate/generate_test.go
+++ b/generate/generate_test.go
@@ -147,55 +147,64 @@ func getDefaultConfig(t *testing.T) *Config {
 // configurations.  It uses snapshots, just like TestGenerate.
 func TestGenerateWithConfig(t *testing.T) {
 	tests := []struct {
-		name    string
-		baseDir string  // relative to dataDir
-		config  *Config // omits Schema and Operations, set below.
+		name       string
+		baseDir    string   // relative to dataDir
+		operations []string // overrides the default set below
+		config     *Config  // omits Schema and Operations, set below.
 	}{
-		{"DefaultConfig", "", getDefaultConfig(t)},
-		{"Subpackage", "", &Config{
+		{"DefaultConfig", "", nil, getDefaultConfig(t)},
+		{"Subpackage", "", nil, &Config{
 			Generated: "mypkg/myfile.go",
 		}},
-		{"SubpackageConfig", "mypkg", &Config{
+		{"SubpackageConfig", "mypkg", nil, &Config{
 			Generated: "myfile.go", // (relative to genqlient.yaml)
 		}},
-		{"PackageName", "", &Config{
+		{"PackageName", "", nil, &Config{
 			Generated: "myfile.go",
 			Package:   "mypkg",
 		}},
-		{"ExportOperations", "", &Config{
+		{"ExportOperations", "", nil, &Config{
 			Generated:        "generated.go",
 			ExportOperations: "operations.json",
 		}},
-		{"CustomContext", "", &Config{
+		{"CustomContext", "", nil, &Config{
 			Generated:   "generated.go",
 			ContextType: "github.com/Khan/genqlient/internal/testutil.MyContext",
 		}},
-		{"StructReferences", "", &Config{
+		{"StructReferences", "", nil, &Config{
 			StructReferences: true,
 			Generated:        "generated-structrefs.go",
 		}},
-		{"NoContext", "", &Config{
+		{"NoContext", "", nil, &Config{
 			Generated:   "generated.go",
 			ContextType: "-",
 		}},
-		{"ClientGetter", "", &Config{
+		{"ClientGetter", "", nil, &Config{
 			Generated:    "generated.go",
 			ClientGetter: "github.com/Khan/genqlient/internal/testutil.GetClientFromContext",
 		}},
-		{"ClientGetterCustomContext", "", &Config{
+		{"ClientGetterCustomContext", "", nil, &Config{
 			Generated:    "generated.go",
 			ClientGetter: "github.com/Khan/genqlient/internal/testutil.GetClientFromMyContext",
 			ContextType:  "github.com/Khan/genqlient/internal/testutil.MyContext",
 		}},
-		{"ClientGetterNoContext", "", &Config{
+		{"ClientGetterNoContext", "", nil, &Config{
 			Generated:    "generated.go",
 			ClientGetter: "github.com/Khan/genqlient/internal/testutil.GetClientFromNowhere",
 			ContextType:  "-",
 		}},
-		{"Extensions", "", &Config{
+		{"Extensions", "", nil, &Config{
 			Generated:  "generated.go",
 			Extensions: true,
 		}},
+		{"OptionalValue", "", []string{"ListInput.graphql", "QueryWithSlices.graphql"}, &Config{
+			Generated: "generated.go",
+			Optional:  "value",
+		}},
+		{"OptionalPointer", "", []string{"ListInput.graphql", "QueryWithSlices.graphql"}, &Config{
+			Generated: "generated.go",
+			Optional:  "pointer",
+		}},
 	}
 
 	sourceFilename := "SimpleQuery.graphql"
@@ -206,7 +215,14 @@ func TestGenerateWithConfig(t *testing.T) {
 		t.Run(test.name, func(t *testing.T) {
 			err := config.ValidateAndFillDefaults(baseDir)
 			config.Schema = []string{filepath.Join(dataDir, "schema.graphql")}
-			config.Operations = []string{filepath.Join(dataDir, sourceFilename)}
+			if test.operations == nil {
+				config.Operations = []string{filepath.Join(dataDir, sourceFilename)}
+			} else {
+				config.Operations = make([]string, len(test.operations))
+				for i := range test.operations {
+					config.Operations[i] = filepath.Join(dataDir, test.operations[i])
+				}
+			}
 			if err != nil {
 				t.Fatal(err)
 			}
diff --git a/generate/testdata/snapshots/TestGenerateWithConfig-OptionalPointer-testdata-queries-generated.go b/generate/testdata/snapshots/TestGenerateWithConfig-OptionalPointer-testdata-queries-generated.go
new file mode 100644
index 0000000..72a5a55
--- /dev/null
+++ b/generate/testdata/snapshots/TestGenerateWithConfig-OptionalPointer-testdata-queries-generated.go
@@ -0,0 +1,142 @@
+// Code generated by github.com/Khan/genqlient, DO NOT EDIT.
+
+package queries
+
+import (
+	"context"
+
+	"github.com/Khan/genqlient/graphql"
+)
+
+// ListInputQueryResponse is returned by ListInputQuery on success.
+type ListInputQueryResponse struct {
+	// user looks up a user by some stuff.
+	//
+	// See UserQueryInput for what stuff is supported.
+	// If query is null, returns the current user.
+	User *ListInputQueryUser `json:"user"`
+}
+
+// GetUser returns ListInputQueryResponse.User, and is useful for accessing the field via an interface.
+func (v *ListInputQueryResponse) GetUser() *ListInputQueryUser { return v.User }
+
+// ListInputQueryUser includes the requested fields of the GraphQL type User.
+// The GraphQL type's documentation follows.
+//
+// A User is a user!
+type ListInputQueryUser struct {
+	// id is the user's ID.
+	//
+	// It is stable, unique, and opaque, like all good IDs.
+	Id string `json:"id"`
+}
+
+// GetId returns ListInputQueryUser.Id, and is useful for accessing the field via an interface.
+func (v *ListInputQueryUser) GetId() string { return v.Id }
+
+// QueryWithSlicesResponse is returned by QueryWithSlices on success.
+type QueryWithSlicesResponse struct {
+	// user looks up a user by some stuff.
+	//
+	// See UserQueryInput for what stuff is supported.
+	// If query is null, returns the current user.
+	User *QueryWithSlicesUser `json:"user"`
+}
+
+// GetUser returns QueryWithSlicesResponse.User, and is useful for accessing the field via an interface.
+func (v *QueryWithSlicesResponse) GetUser() *QueryWithSlicesUser { return v.User }
+
+// QueryWithSlicesUser includes the requested fields of the GraphQL type User.
+// The GraphQL type's documentation follows.
+//
+// A User is a user!
+type QueryWithSlicesUser struct {
+	Emails                []string  `json:"emails"`
+	EmailsOrNull          []string  `json:"emailsOrNull"`
+	EmailsWithNulls       []*string `json:"emailsWithNulls"`
+	EmailsWithNullsOrNull []*string `json:"emailsWithNullsOrNull"`
+}
+
+// GetEmails returns QueryWithSlicesUser.Emails, and is useful for accessing the field via an interface.
+func (v *QueryWithSlicesUser) GetEmails() []string { return v.Emails }
+
+// GetEmailsOrNull returns QueryWithSlicesUser.EmailsOrNull, and is useful for accessing the field via an interface.
+func (v *QueryWithSlicesUser) GetEmailsOrNull() []string { return v.EmailsOrNull }
+
+// GetEmailsWithNulls returns QueryWithSlicesUser.EmailsWithNulls, and is useful for accessing the field via an interface.
+func (v *QueryWithSlicesUser) GetEmailsWithNulls() []*string { return v.EmailsWithNulls }
+
+// GetEmailsWithNullsOrNull returns QueryWithSlicesUser.EmailsWithNullsOrNull, and is useful for accessing the field via an interface.
+func (v *QueryWithSlicesUser) GetEmailsWithNullsOrNull() []*string { return v.EmailsWithNullsOrNull }
+
+// __ListInputQueryInput is used internally by genqlient
+type __ListInputQueryInput struct {
+	Names []*string `json:"names"`
+}
+
+// GetNames returns __ListInputQueryInput.Names, and is useful for accessing the field via an interface.
+func (v *__ListInputQueryInput) GetNames() []*string { return v.Names }
+
+func ListInputQuery(
+	ctx context.Context,
+	client graphql.Client,
+	names []*string,
+) (*ListInputQueryResponse, error) {
+	req := &graphql.Request{
+		OpName: "ListInputQuery",
+		Query: `
+query ListInputQuery ($names: [String]) {
+	user(query: {names:$names}) {
+		id
+	}
+}
+`,
+		Variables: &__ListInputQueryInput{
+			Names: names,
+		},
+	}
+	var err error
+
+	var data ListInputQueryResponse
+	resp := &graphql.Response{Data: &data}
+
+	err = client.MakeRequest(
+		ctx,
+		req,
+		resp,
+	)
+
+	return &data, err
+}
+
+func QueryWithSlices(
+	ctx context.Context,
+	client graphql.Client,
+) (*QueryWithSlicesResponse, error) {
+	req := &graphql.Request{
+		OpName: "QueryWithSlices",
+		Query: `
+query QueryWithSlices {
+	user {
+		emails
+		emailsOrNull
+		emailsWithNulls
+		emailsWithNullsOrNull
+	}
+}
+`,
+	}
+	var err error
+
+	var data QueryWithSlicesResponse
+	resp := &graphql.Response{Data: &data}
+
+	err = client.MakeRequest(
+		ctx,
+		req,
+		resp,
+	)
+
+	return &data, err
+}
+
diff --git a/generate/testdata/snapshots/TestGenerateWithConfig-OptionalValue-testdata-queries-generated.go b/generate/testdata/snapshots/TestGenerateWithConfig-OptionalValue-testdata-queries-generated.go
new file mode 100644
index 0000000..7d6fafe
--- /dev/null
+++ b/generate/testdata/snapshots/TestGenerateWithConfig-OptionalValue-testdata-queries-generated.go
@@ -0,0 +1,142 @@
+// Code generated by github.com/Khan/genqlient, DO NOT EDIT.
+
+package queries
+
+import (
+	"context"
+
+	"github.com/Khan/genqlient/graphql"
+)
+
+// ListInputQueryResponse is returned by ListInputQuery on success.
+type ListInputQueryResponse struct {
+	// user looks up a user by some stuff.
+	//
+	// See UserQueryInput for what stuff is supported.
+	// If query is null, returns the current user.
+	User ListInputQueryUser `json:"user"`
+}
+
+// GetUser returns ListInputQueryResponse.User, and is useful for accessing the field via an interface.
+func (v *ListInputQueryResponse) GetUser() ListInputQueryUser { return v.User }
+
+// ListInputQueryUser includes the requested fields of the GraphQL type User.
+// The GraphQL type's documentation follows.
+//
+// A User is a user!
+type ListInputQueryUser struct {
+	// id is the user's ID.
+	//
+	// It is stable, unique, and opaque, like all good IDs.
+	Id string `json:"id"`
+}
+
+// GetId returns ListInputQueryUser.Id, and is useful for accessing the field via an interface.
+func (v *ListInputQueryUser) GetId() string { return v.Id }
+
+// QueryWithSlicesResponse is returned by QueryWithSlices on success.
+type QueryWithSlicesResponse struct {
+	// user looks up a user by some stuff.
+	//
+	// See UserQueryInput for what stuff is supported.
+	// If query is null, returns the current user.
+	User QueryWithSlicesUser `json:"user"`
+}
+
+// GetUser returns QueryWithSlicesResponse.User, and is useful for accessing the field via an interface.
+func (v *QueryWithSlicesResponse) GetUser() QueryWithSlicesUser { return v.User }
+
+// QueryWithSlicesUser includes the requested fields of the GraphQL type User.
+// The GraphQL type's documentation follows.
+//
+// A User is a user!
+type QueryWithSlicesUser struct {
+	Emails                []string `json:"emails"`
+	EmailsOrNull          []string `json:"emailsOrNull"`
+	EmailsWithNulls       []string `json:"emailsWithNulls"`
+	EmailsWithNullsOrNull []string `json:"emailsWithNullsOrNull"`
+}
+
+// GetEmails returns QueryWithSlicesUser.Emails, and is useful for accessing the field via an interface.
+func (v *QueryWithSlicesUser) GetEmails() []string { return v.Emails }
+
+// GetEmailsOrNull returns QueryWithSlicesUser.EmailsOrNull, and is useful for accessing the field via an interface.
+func (v *QueryWithSlicesUser) GetEmailsOrNull() []string { return v.EmailsOrNull }
+
+// GetEmailsWithNulls returns QueryWithSlicesUser.EmailsWithNulls, and is useful for accessing the field via an interface.
+func (v *QueryWithSlicesUser) GetEmailsWithNulls() []string { return v.EmailsWithNulls }
+
+// GetEmailsWithNullsOrNull returns QueryWithSlicesUser.EmailsWithNullsOrNull, and is useful for accessing the field via an interface.
+func (v *QueryWithSlicesUser) GetEmailsWithNullsOrNull() []string { return v.EmailsWithNullsOrNull }
+
+// __ListInputQueryInput is used internally by genqlient
+type __ListInputQueryInput struct {
+	Names []string `json:"names"`
+}
+
+// GetNames returns __ListInputQueryInput.Names, and is useful for accessing the field via an interface.
+func (v *__ListInputQueryInput) GetNames() []string { return v.Names }
+
+func ListInputQuery(
+	ctx context.Context,
+	client graphql.Client,
+	names []string,
+) (*ListInputQueryResponse, error) {
+	req := &graphql.Request{
+		OpName: "ListInputQuery",
+		Query: `
+query ListInputQuery ($names: [String]) {
+	user(query: {names:$names}) {
+		id
+	}
+}
+`,
+		Variables: &__ListInputQueryInput{
+			Names: names,
+		},
+	}
+	var err error
+
+	var data ListInputQueryResponse
+	resp := &graphql.Response{Data: &data}
+
+	err = client.MakeRequest(
+		ctx,
+		req,
+		resp,
+	)
+
+	return &data, err
+}
+
+func QueryWithSlices(
+	ctx context.Context,
+	client graphql.Client,
+) (*QueryWithSlicesResponse, error) {
+	req := &graphql.Request{
+		OpName: "QueryWithSlices",
+		Query: `
+query QueryWithSlices {
+	user {
+		emails
+		emailsOrNull
+		emailsWithNulls
+		emailsWithNullsOrNull
+	}
+}
+`,
+	}
+	var err error
+
+	var data QueryWithSlicesResponse
+	resp := &graphql.Response{Data: &data}
+
+	err = client.MakeRequest(
+		ctx,
+		req,
+		resp,
+	)
+
+	return &data, err
+}
+