[RFC] Option in config to have return values based on schema nullability

[frederikhors]

What happened?

In #375 we discussed and changed the resolvers return from this:

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)

to always this:

query { user: User } ---> func (r *Query) User(ctx context.Context) (*User, error)

What did you expect?

To have in config something like: omit_slice_element_pointers: false (for slices) to have not pointer return values for resolvers.

Does it make sense or am I wrong?

[frederikhors]

This is an example of the "issue".

I'm using @vektah/dataloaden with:

//go:generate go run github.com/vektah/dataloaden PlayerLoader int demo_app/Player

and in my resolver I have to use this code:

func (r *teamResolver) Category(ctx context.Context, obj *Team) (*Category, error) {
    if obj == nil {
        return nil, nil
    }
    res, err := dataloaders.For(ctx).PlayerById.Load(obj.CategoryID)
    if err != nil {
        return nil, err
    }
    return &res, err
}

instead of:

func (r *teamResolver) Category(ctx context.Context, obj *Team) (Category, error) {
    if obj == nil {
        return Category{}, nil
    }
    return dataloaders.For(ctx).PlayerById.Load(obj.ExamID)
}

[ianling]

Can this be revisited @frederikhors @StevenACoffman ? The original issue (#375) that caused this change is flat-out wrong in its assumptions and resulted in a half-baked "solution" that I can't opt out of. It lists the following things as reasons to always return pointers instead of values:

1. Forces you to return an empty objects with errors, eg return User{}, fmt.Errorf("not found")
2. Isn't very idiomatic. Most go apis will return (*Thing, error), and return nil when error is set
3. 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.

Issue 1 is not actually a problem and is perfectly normal in Go code.

Issue 2 is wrong and doesn't cite any examples to support their claim.

Issue 3 also isn't true.

This comment that was left a few months after the change was implemented said about as much, and has a bunch of people agreeing with it, but it was ignored.

Ultimately, I'm now left in the position of having to design my API in a confusing way that uses and returns pointers to things unnecessarily.

[StevenACoffman]

Referenced comment:

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

The second article cited ended with:

In short, I think that you should prefer declaring methods on *T unless you have a strong reason to do otherwise.

[ianling]

I think the second article linked is not relevant to this issue, this has nothing to do with whether the receiver of a method is a pointer or not. The person who linked it probably should have left it out as it's just a distraction.

[StevenACoffman]

I often end up with *Users instead of Users becuase my structs are generally non-trivial and so it is inefficient to copy heavy structs around by value. I do get the argument for preferring []Users over []*Users, since returning a slice of even a heavy User struct is efficient due to the reference type nature of slices.

In general, I prefer to let people do whatever they want as long as it is easy to do the commonly desired thing, so I'm not opposed to having a config option, assuming it has test coverage and doesn't break backwards compatibility.

No one has yet cared enough to write code to make this happen though.

[StevenACoffman]

Also, I generally don't have the time to keep up with discussions (or even issues), but make time to keep up with PRs, as those demonstrate a level of commitment that I try to honor when I volunteer my time to this project. It was entirely unmaintained for an extended period before I and others stepped up.

@frederikhors has been pretty responsive for more recent issues/discussions and has been tirelessly going through the huge number of accumulated old issues trying to suss out which are still relevant. It is by nature slow work, and often thankless, and he's also a volunteer.

I think characterizing that as ignoring a popular comment during a period when the project was not as actively maintained is maybe missing a little bit of that nuance, so please try to keep it in mind.

[ianling]

I apologize, I didn't intend to imply that any of the maintainers are lazy. There has been little discussion of this since the change was originally implemented, so it was clearly also not an issue that many users of gqlgen are bothered by, and it's totally understandable that time was spent on other more pressing things. It was just frustrating to me personally because it is a strange design decision that, at least for my use case, had only cons and no pros, and was implemented in a way that I cannot opt out of.

I agree that the ideal approach would have been to make it user-configurable so that people can do whatever they want. There are definitely times when it makes more sense to return pointers instead of values, but I think that doing so for performance reasons without benchmarking your specific case is generally going to fall under premature optimization and lead to more complicated APIs for no tangible real-world performance improvement.

I'll see if I can figure out how to add a config option for this, as I would really prefer not to have to alter every single database call in my application to return pointers.

[deitrix]

I agree with this. It also stops me from being able to use my API clients / repositories types directly as resolvers, because of this extra mapping step. I end up with a bunch of unnecessary resolver functions like:

func (r *queryResolver) ResourceByID(ctx context.Context, id int) (*company.Resource, error) {
    b, err := r.ResourceClient.ResourceByID(ctx, id)
    return &b, err
}

If I had the option to control whether the returned type was a pointer or not, maybe even at a directive level, I could just be using company.ResourceClient as my resolver.

type Resolver struct {
    company.ResourceClient
}