Make generated code pass golint

[flicaflow]

golint is not very happy with the generated code. Main offenders are missing function and type commends and method names with underscores. The first problem is contained in generated code and might be ok since users are not supposed look at it (it stil clutters the output of a go lint run).However the second is bad since users have to implement the resolver interface. I care about this because my editor screams at me and I cannot do anything about it.

I think it would be good style to follow the established code conventions. Adding code comments should be an easy fix. Changing the naming scheme of the resolver methods is not so simple since it would break resolver implementations. However I would say better now then later.

Btw. Thanks for the wonderful work!

[vektah]

The reason the underscores are there is to split the type from the method in a way that isn't likely to be used normally. Foo.barBaz -> Foo_barBaz. Without a separator Foo.barBaz and FooBar.baz are ambiguous.

The go lint rule is to stop people using snake case, which I appreciate, but this is camel case with underscores as a separator. I don't think the limiting rule should apply. Gomatalinter lets you add comments that opt out of rules, but go lint by itself doesn't.

It was a considered decision, How do others feel about this?

[flicaflow]

I get your point and I don't have a better solution to clarify the ambiguity right now. Still I think it would be a big plus if the generated code would be 100% standard tooling compliant. There might be projects out there which enforce a build/test/lint pre-commit hook. And the nice thing about go is that there is just one default linter (looking at you ESLint).

However I can certainly life with this..

Cheers

[rodcorsi]

I think a good start point for this topic is golint pass at least in client resolvers.
Change the resolver to:

type Mutation interface {
	CreateTodo(ctx context.Context, text string) (Todo, error)
}

type Query interface {
	Todos(ctx context.Context) ([]Todo, error)
}

type Resolvers interface {
	Mutation() Mutation
	Query() Query
}

[mastercactapus]

@rodcorsi I think that's probably the most "idiomatic" solution -- with smaller interfaces for testing and separation. However it does come with overhead (e.g. many separate types) compared to the single flat interface.

I'd be curious to know how common name conflicts are in the wild. Maybe the cost of possible ambiguity is justifiable?

Anyway, I'm not sure the best solution either yet, however I do feel it's worth reconsidering the options. The method implementations can live/be used elsewhere (e.g. tests, separate files) so it does add overhead for every new contributor to a project if the defaults don't work (i.e. golint).

I'm more visual for this type of thing, so here's some pseudo code illustrating the different strategies suggested.

Given the schema:

type Query {
  user(id: ID!) User
}
type Mutation {
  setEmail(userID: ID!, email: String!) User
}
type User {
  id: ID!
  name: String!
  email: String!
}

Let's say the our resolver starts as a struct:

// imported from user pkg
// type User struct {
//   ID string
//   Name string
// }
type App struct {
  UserStore user.Store
}

Today's state:

• Our resolver code (non generated code) wont pass linting
• It does follow the conventions laid out for examples and tests for methods
• Simple and flat
• Large interface (potentially)

// imported from graph pkg
// type Resolver interface {
//   Query_user(ctx context.Context, id string) (*user.User, error)
//   User_email(ctx context.Context, u *user.User) (string, error)
//   Mutation_setEmail(ctx context.Context, id, email string) (*user.User, error)
// }
func (a *App) Query_user(ctx context.Context, id string) (*user.User, error) {
  return a.UserStore.FindOne(ctx, id)
}
func (a *App) User_email(ctx context.Context, u *user.User) (string, error) {
  return a.UserStore.Email(ctx, u.ID)
}
func (a *App) Mutation_setEmail(ctx context.Context, id, email string) (*user.User, error) {
  err := a.UserStore.SetEmail(ctx, id, email)
  if err != nil {
    return nil, err
  }
  return a.UserStore.FindOne(ctx, id)
}

Dropping the underscores:

• Non generated code passes linting
• Simple and flat
• Large interface (potentially)
• Higher likelihood of naming conflict, with no good workaround

// imported from graph pkg
// type Resolver interface {
//   QueryUser(ctx context.Context, id string) (*user.User, error)
//   UserEmail(ctx context.Context, u *user.User) (string, error)
//   MutationSetEmail(ctx context.Context, id, email string) (*user.User, error)
// }
func (a *App) QueryUser(ctx context.Context, id string) (*user.User, error) {
  return a.UserStore.FindOne(ctx, id)
}
func (a *App) UserEmail(ctx context.Context, u *user.User) (string, error) {
  return a.UserStore.Email(ctx, u.ID)
}
func (a *App) MutationSetEmail(ctx context.Context, id, email string) (*user.User, error) {
  err := a.UserStore.SetEmail(ctx, id, email)
  if err != nil {
    return nil, err
  }
  return a.UserStore.FindOne(ctx, id)
}

Smaller interfaces:

• A bit more complicated (need new aliased types, and methods for each schema type)
• No naming conflicts
• Passes linting
• Small interfaces

// imported from graph pkg
// type Resolver interface {
//   Query() QueryResolver
//   Mutation() MutationResolver
//   User() UserResolver
// }
// type QueryResolver interface {
//   User(ctx context.Context, id string) (*user.User, error)
// }
// type MutationResolver interface {
//   SetEmail(ctx context.Context, id, email string) (*user.User, error)
// }
// type UserResolver interface {
//   Email(ctx context.Context, u *user.User) (string, error)
// }
func (a *App) Query() graph.QueryResolver { return (*Query)(a) }
func (a *App) Mutation() graph.MutationResolver { return (*Mutation)(a) }
func (a *App) User() graph.UserResolver { return (*User)(a) }

type Query App
func (a *Query) User(ctx context.Context, id string) (*user.User, error) {
  return a.UserStore.FindOne(ctx, id)
}

type User App
func (a *User) Email(ctx context.Context, u *user.User) (string, error) {
  return a.UserStore.Email(ctx, u.ID)
}

type Mutation App
func (a *Mutation) SetEmail(ctx context.Context, id, email string) (*user.User, error) {
  err := a.UserStore.SetEmail(ctx, id, email)
  if err != nil {
    return nil, err
  }
  return a.UserStore.FindOne(ctx, id)
}

---

Looking at them now, I think my preference is the 3rd option. My use case is a larger project with a 1100-line schema (so far). So breaking it apart would have definite benefits for organization in my case, as well as friendliness with golint.

Having said that, it should also be easy to map the small interfaces to the flat one too:

type App struct {
  SmallInterfaceResolver
}

func (a *App) Query_user(ctx context.Context, id string) (*user.User, error) {
  return a.Query().User(ctx, id)
}

@vektah thoughts?

[ghost]

What about adding a "gen" or "gql" prefix like:

func (a *App) GenQueryUser(ctx context.Context, id string) (*user.User, error) {
  ...
}
func (a *App) GenUserEmail(ctx context.Context, u *user.User) (string, error) {
  ...
}
func (a *App) GenMutationSetEmail(ctx context.Context, id, email string) (*user.User, error) {
  ...
}

or

func (a *App) GqlQueryUser(ctx context.Context, id string) (*user.User, error) {
  ...
}
func (a *App) GqlUserEmail(ctx context.Context, u *user.User) (string, error) {
  ...
}
func (a *App) GqlMutationSetEmail(ctx context.Context, id, email string) (*user.User, error) {
  ...
}

[mastercactapus]

@everdev That could work, but the ambiguity is between the type and field, rather than with the prefix. So it would be a little different.

Given this schema:

type Service {
  UserName: String
}
type ServiceUser {
  Name: String
}

Today, we would have:

• Service_UserName
• ServiceUser_Name

So instead of a "prefix", it would have to be something in between the two (Field in this example):

• ServiceFieldUserName
• ServiceUserFieldName

Or maybe a letter:

• ServiceTUserName
• ServiceUserTName

Basically we would replace the underscore with a letter or word. It's a potential 4th option that would pass linting, and could be a flag (e.g. override the type-field separator, with default _).

[jayp]

Generated code doesnt need to pass linter. Most dont: gomock, stringer, etc. One should disable linter for such files. However, I agree that we should try to get linter to pass for the interface methods one needs to implement. Liking the smaller interfaces approach proposed by @mastercactapus. The gqlgen tool take this one vs. multiple interfaces as an option?

[microo8]

Generated code doesn't need to pass linter, but we need to write the resolvers.

And I need write:

func (a *App) Query_user(ctx context.Context, id string) (*user.User, error) {
  return a.UserStore.FindOne(ctx, id)
}

then the linter gives a warning because of the Query_user name.

[jonlundy]

golint will pass the generated code if it has the generated code header.

^// Code generated .* DO NOT EDIT\.$

https://golang.org/s/generatedcode

[mastercactapus]

@jonlundy Good to know, I've updated my PR to fix the header.

As an added bonus it looks like vscode (maybe others) also recognize that header:

[jonlundy]

Yeah Intelj based editors and some golint tools will also respect the official header format.

[jonlundy]

I do like the smaller resolver option. It feels very natural and could be useful for large codebases

[vektah]

Thanks for the considered writeup @mastercactapus, that sums up the situation pretty well.

Your option 3 keeps the "god" object App passing all the deps in. This means tests still need to build the whole graph and all of its dependencies. This is totally fine, as it still not any worse that what it currently is.

I wonder if gqlgen could generate a struct instead of an interface for the base level:

type struct Resolvers {
    Query QueryResolver
    User UserResolver
}

interface User {
    Email() (string, error)
}

and its up to the user to build it, and wire in their needed deps:

graph := Resolvers {
    User: MyUserResolver{userService, otherDeps...}
    Query: MyQueryResolver{userService, ...}
}

The advantage here is you get type validation without having to explicitly depend on the interface in the generated code (can make it hard to generate, if the code implementing the resolvers is in the same package as the generated code...). But it does mean its easy to forget a resolver.

---

Another random thought, is that most of our code is organised by the return type of the resolver, not the object its on. Eg there are many ways to fetch a user, that all do the same thing:

• Query_getUser
• Todo_getUser
• Post_getUser
• Post_getAdmin

With the current system, all of those things can easily be grouped together, with smaller resolvers it would look weirder having 3 methods from 3 different objects defined in the same file.

I wonder if the whole resolver interface is backwards? Could it be:

type UserResolver interface {
    UserOnQuery(ctx context.Context, id int) (User, error)
    UserOnTodo(ctx context.Context, todo *Todo) (User, error)
    UserOnPost(ctx context.Context, post *Post) (User, error)
    AdminOnPost(ctx context.Context, post *Post) (User, error) // There might be multiple edges returning user
    SetEmailOnMutation(ctx context.Context, id, email) (*user.User, error)
}

I don't really like the naming, but I think there might be something to it. 🔥

[vvakame]

-some_super_cool_option_name _ → Query_Todos
-some_super_cool_option_name On → QueryOnTodos
-some_super_cool_option_name 🐱 → Query🐱Todos

if we using On and we made the TodoOn+Bar and Todo+OnBar, it's collide.
We can switch to 🐱, TodoOn🐱Bar and Todo🐱OnBar, It's not collide.
If we select On is default. User can specified _. they can moving new concatenate character at a favorite timing.

[vektah]

Yeah, it has the same issues, but its much less likely, you would need a User.onUser and a OnUser.user. As I said, I'm not really a fan of using that separator, it feels kinda gross.

I just wanted to highlight how we structure our code doesn't really line up with how the small resolvers are structured, or where dependencies end up getting used. (Everything has a user, so everything needs the UserService, even though they are all just calling the same method, getUserById).

[mastercactapus]

@vektah
The struct is interesting, as you said though, it does make it easy to forget. Maybe we could assert all fields on the initial call so we at least get a failure on startup rather than during operation. Or we could "gracefully" handle it, and return nil (and maybe an error) for those calls. It would make it easier to separate concerns/code though.

The way I structured App was just one way of approaching it. Rather than making alias types for App like I did, you could make a struct, as in your example, that implements methods like QueryResolver that just returns the .Query method.

I think either way the generated code asks for the resolvers is acceptable, it's possible to structure the project code as separate structs or a single one with aliased types and simple "wrap" it to whatever we land on for MakeExecutableSchema.

---

As for the grouping, (grouped by parent type, or return type) I think it might vary project to project. In my case, we have interfaces like UserStore ServiceStore etc.. that live in App. Most resolvers end up being, effectively, one liners that just call to methods on the "store".

Most maintenance for us ends up to be adding or updating fields per parent type, and so it's been beneficial to group things that way, since things like fetching a user are already "grouped". When it comes to debugging, it's been easy to find a field based on the query in question (e.g. User.service -> find user.go, and the service resolver). We've also had to add "versioned" fields (e.g. status deprecated, and status2 added) here and there and it could be hard to follow the context if those two fields were separated.

Now, having said that, it could also be preference, or just "what we're used to" (we're looking at migrating from a different library where that was the only option). But our schema is getting pretty big (>1100 lines IIRC, though I have no good frame of reference) and I can say organizing it by parent type has worked really well for finding things day-to-day.

[jonlundy]

For the most part when i have a Object_foo its typically written to just call the Query_foo function.

func (r Resolver) HasAFoo_foo(ctx context.Context, obj *model.HasAFoo) (model.Foo, error) {
	if obj == nil {
		return model.Client{}, fmt.Errorf("Nil HasAFoo") // I assume this will never happen?
	}

	return r.Query_foo(ctx, obj.FooID)
}

[jonlundy]

@mastercactapus @vektah Is there any command line tools that I can give an object and an interface and it lists all the functions that are missing to make it fulfill the interface? That would be very helpful when updating schemas.

[vektah]

@jonlundy Intellij is pretty good at this stuff:

#9 might be able to do something similar when generating, the tricky part is working out which interface you intended to implement. Maybe it should just be a flag to gqlgen,

gqlgen -update github.com/me/graph.App

[vektah]

I've had time to try the short resolver syntax, and I think I'm sold.

[vektah]

With #134 landing you should be able to define resolvers using the new linter-friendly syntax.