[Proposal] Always use pointers in resolver return types

[vektah]

Currently the return value for a resolver is based on the schema Non-Nullability.

eg
for query { user: User!} we generate func (r *Query) User(ctx context.Context) (User, error)
for query { user: User} we generate func (r *Query) User(ctx context.Context) (*User, error)

Initially, I thought this would be useful as the compiler can validate the non-nullability, however it:

• Forces you to return an empty objects with errors, eg return User{}, fmt.Errorf("not found")
• Isn't very idiomatic. Most go apis will return (*Thing, error), and return nil when error is set
• Fragments function signatures; if you have a database function looking up a user it probably already returns a (*User, error), being able to return it directly would be nice.

Proposal

Regardless of nullability always use a pointer. Gqlgen already knows how to check for nulls in fields (used when binding directly to model fields that have pointers).

This would need to go behind a config flag for now. We can opt in new users in init, and output a deprecation message for some release in the future to drop. Or maybe we just keep it. Its probably a pretty small change either way.

What are peoples thoughts on this?

related prisma/prisma#3288

[edsrzf]

I think defaulting to a pointer makes sense. The current behavior has definitely been an annoyance at times.

What about types that are normally passed by value (except when nullable), like int? Maybe exclusions for certain built-in types? (Possibly even standard library types that are normally passed by value like time.Time.) It might also be nice to default to pointers, but allow overriding to values via configuration/directive.

[alexbyk]

And ([]*Todo, error) instead of ([]Todo, error)

[mathewbyrne]

Having worked with gqlgen with a small to medium sized schema for about 6 months now, I can definitely say that I would prefer this change, largely for the benefits listed above.

[azavorotnii]

@vektah real drawback of always-pointers approach - they can be nil for non-null schema. This makes unnecessary null-check and makes it less clear for developer if there can return nil value or can not. I think what there is right now is correct and clear for developer what expected as a result of resolver method.

[mathewbyrne]

@azavorotnii we're talking about the return value from resolvers though. I think I would still expect gqlgen to generate resolver signatures that take non-pointer arguments for non-nullable GraphQL arguments. That way only gqlgen needs to do the null checking.

@vektah was that what you were imagining with this change?

[azavorotnii]

@mathewbyrne I understand that this is only about return values. But assuming schema:

type Query {
  user (id: ID!): User!
}

We will get resolver with nullable return value which can be defined like this by developer:

type Resolver {
  users map[string]*User
}

func (r *Resolver) User(ctx context.Context, id string) (*User, error) {
  return r.users[id], nil
}

Which will end up as nil, nil if user not in map. This makes gqlgen responsible to double check non-null condition which could be avoided by method signature. Leaving return types as is will force developer to handle nil-cases themselves which is good:

func (r *Resolver) User(ctx context.Context, id string) (User, error) {
  if u := r.users[id]; r != nil {
    return *u, nil
  }
  return User{}, errors.New("user not found")
}

[mathewbyrne]

This makes gqlgen responsible to double check non-null condition which could be avoided by method signature.

To me this seems fine, and exactly within the domain gqlgen is responsible for. We're generating the code so it's no problem to add a check like this into the template. I believe it's also easier for the resolver developer to not have to worry about this, as well as having the additional benefits outlined earlier in this thread.

[vektah]

Which will end up as nil, nil if user not in map. This makes gqlgen responsible to double check non-null condition which could be avoided by method signature.

It already is responsible, Any NonNull field that has an error can already be returned as null, and the spec is pretty clear on how it should be handled.

Leaving return types as is will force developer to handle nil-cases themselves which is good:

Yeah, this was my initial thinking too, but in practice I've grown to hate it. It gets in the way more than it helps.

type Resolver {
  users map[string]*User
}

func (r *Resolver) User(ctx context.Context, id string) (*User, error) {
  return r.users[id], nil
}

This is actually totally valid, and would already produce an error like this if it were marked as NonNull and the user wasn't found.

  "errors": [
    {
      "message": "must not be null",
      "path": [
        "user"
      ]
    }
  ],

However, be very careful about null bubbling behaviour, this is very rarely what you want for find methods like this! By forcing user to be NotNull, not finding the user becomes an error, and errors on non null fields mean the top level query object is invalid, and therefore data will be set to null to ensure type sanity in typescript/flow. This means any other top level queries used by other components also won't be returned.

[PatrLind]

It seems to me that this is still not fully implemented. When there is a list of objects then the object is still not a pointer (0.8.3).

type Query {
    languages(languageId: ID): [Language!]!
}

gives:

type QueryResolver interface {
	Languages(ctx context.Context, languageID *string) ([]Language, error)
}

I expect it to be:

type QueryResolver interface {
	Languages(ctx context.Context, languageID *string) ([]*Language, error)
}

[ghost]

Sorry to reopen here... but I fundamentally disagree with the original statement. Most database tools I've used (sqlx for example) don't return pointers. Making everything a pointer means you have to do nil checking (mentioned above), but it also means that you can't compare objects without de-referencing them.

I am of the belief that pointers should only be used where you actually need to mutate the original state, in order to share it between different actors (not to be confused with threads/routines).

I would really appreciate some feedback here, this is something that should be changed. Dave Cheney mentions some points in the following articles that kind of help back this opinion.

• https://dave.cheney.net/2014/03/17/pointers-in-go
• https://dave.cheney.net/2016/03/19/should-methods-be-declared-on-t-or-t
• https://dave.cheney.net/2013/01/19/what-is-the-zero-value-and-why-is-it-useful

Are we trying to return pointers or values?

Also, @PatrLind those slices are technically pointers already, and thus can be nil: https://play.golang.org/p/IpyzWkUVRqJ

[PatrLind]

@rucuriousyet, I have no preference over either pointers or values (copies), but the reason I commented was because the interface was inconsistent. Some things were returned as pointers and some as values regardless of nullable or not.
With regards to how GraphQL data types can have null values, there is a (subtle) difference between an empty array and a null array in GraphQL (but not in go, unless you use a pointer to the array).
I am just a simple library user, and I have nothing to do with the development of this, but I think that this proposal should have been discussed a bit more before being implemented. It also broke a lot of my code when the package was updated.

[ghost]

Agreed! I think versioning the project might be a good idea if changes are going to be implemented like this.

[steebchen]

From the proposal in the first comment, I can understand why the change was introduced for single things, e.g. *User instead of User (and I think this is a good change). However, I don't quite understand why it was introduced for slices as well, as it was just added in the PR but I could not find any information or reasoning for this specific change.

This makes gqlgen incompatible with Prisma, because while Prisma returns pointers for single values it returns a slice of structs for multiple values.

Was the slice-of-pointers change just introduced for consistency, so [User], [User!] and [User!]! will generate the same code? Would it really make sense for a tool like Prisma to return a slice of pointers? If yes, why? It would be great if someone could clarify this.

[steebchen]

@vektah, @mathewbyrne or @alexbyk; it would be great if one of you could clarify my questions above some time :)

[robert-zaremba]

Same thing here:
#782

[robojones]

@vektah, @mathewbyrne and @alexbyk; Why is none of you answering to @steebchen?

[steebchen]

I have now managed to update my code in a rather bigger project for some other features in the latest release and I can only say that this update makes everything hideous, especially with Prisma. There is literally no single use case where it makes sense to return a slice of pointers, and it makes every resolver which returns an array extremely cumbersome to work with. Often, you end up doing this:

users, err := r.Prisma.Users(...)

nodes := []*prisma.User{}

for _, item := range users {
	clone := item // a clone is needed, otherwise the pointer to item is used
	nodes = append(nodes, &clone)
}

return nodes

Also, if you work with helper methods which return an array, you either have to implement the method to return a slice of pointers, which is also very error-prone, or (again) convert it always manually by using the method above. Of course, you can also create conversion methods for all types you are converting from, but please...

I still don't understand why this change was made in the first place, and I don't understand why no one can answer me this. I assume because there is no answer, but silence doesn't help anyone. Why did you make this change? Why are you not considering reverting this change or making this at least configurable?

This change is not documented, not discussed at all (except a single comment which suggests the change), it seems to confuse everyone, it's a huge refactoring burden, it makes working with slices of structs cumbersome to work with, and basically kills compatibility with Prisma.

cc @vektah @mathewbyrne

[frederikhors]

@steebchen Only today my knowledge of Golang allows me to understand what you complained about a year ago in this issue.

May I know how you solved? Are you still using gqlgen? Do you use anything else?

Thanks in advance.

[vektah]

The config option omit_slice_element_pointers:true should generate slices as []Thing instead of []*Thing.

Introduced in #874