Add support for mapping all nullable types as pointers

[connec]

This is a bare minimum implementation of the approach suggested in #178 (comment), which I figured I'd open sooner than later for early feedback!

Per the new detail in the sample docs/genqlient.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:

# 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:

# operations.graphql
query userGetByID($id: ID!) {
  user(id: $id) {
    avatarUrl
    brandID
    brandVisible
    createdAt
    firstName
    id
    lastName
    name
    primaryEmail
  }
}

It will generate:

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:

• Written a clear PR title and description (above)
• Signed the Khan Academy CLA
• Added tests covering my changes, if applicable
• Included a link to the issue fixed, if applicable
• Included documentation, for new features
• 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.
Ooof, ignore me. I got confused with a similar change in gqlgen that is the opposite of this.

[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.

[connec]

Awesome! I did think about [String] and *[]*string. I could have gone either way but as you say []T can already distinguish between nil and empty. I think this matches what gqlgen does by default as well.