Khan / genqlient

a truly type-safe Go GraphQL client
MIT License
1.07k stars 107 forks source link

Nullable list in optional pointer mode generates non-pointer slice #275

Closed KennethWussmann closed 8 months ago

KennethWussmann commented 1 year ago

Describe the bug When the GraphQL schema includes a nullable list like [String!] and the optional mode in the YAML config is set to pointer the generated Go code results in []string instead of *[]string

To Reproduce I'm taking your example with the GitHub schema from the repos example directory and made the following adjustments:

  1. Set the optional mode to pointer in the genqlient.yaml
    # Added this line:
    optional: pointer
  2. Add a new query to genqlient.graphql that returns a nullable list of non-nullable strings
    # Added this query
    query getMeta {
    meta {
    gitIpAddresses
    }
    }
  3. Regenerate the code using go generate ./...
  4. Check the generated.go for the chosen type of gitIpAddresses
    type getMetaMetaGitHubMetadata struct {
    // IP addresses that users connect to for git operations
    GitIpAddresses []string `json:"gitIpAddresses"`
    }

Expected behavior The above steps should have generated the following Go code:

type getMetaMetaGitHubMetadata struct {
    // IP addresses that users connect to for git operations
    GitIpAddresses *[]string `json:"gitIpAddresses"`
}

So that GitIpAddresses is a pointer *[]string like it was configured in the YAML.

Because looking at the GraphQL schema shows that gitIpAddresses is a nullable array:

type GitHubMetadata {
  # ...
  """
  IP addresses that users connect to for git operations
  """
  gitIpAddresses: [String!]
  # ...
}

genqlient version: v0.6.0

benjaminjkraft commented 1 year ago

This behavior is intended -- since slices can be empty or nil we never need a pointer to represent null, so it's just extra indirection that makes things harder to work with. Are you running into a case where you need a pointer here, and if so can you say more about why it's useful? If there's a reason to do so we could add another option (it's oo big a behavior change to do without putting it behind an option unless there's a very very good reason).

(Edit: we should probably also document this better! Also, this is a dupe of #16 but let's keep discussion here since there's more detail.)

KennethWussmann commented 1 year ago

Hey Ben, ah okay, fair enough. I just stumbled upon that myself and thought it was an unwanted inconsistency. But you are right, nil on slices also works fine for me. I'd consider it case closed if this works as intended 👍🏻