Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add lint-friendly small interfaces option for resolvers #134

Merged
merged 12 commits into from
Jun 26, 2018

Conversation

mastercactapus
Copy link
Contributor

@mastercactapus mastercactapus commented Jun 15, 2018

This PR adds a ShortResolvers interface to generated.go allowing lint-friendly resolver methods. There is also a utility method that can take a ShortResolvers and return a Resolvers interface.

The new interface is a possible solution to the linting issues raised in #106. It provides a lint-friendly path to implementing resolvers via smaller type-specific interfaces for each object type.

The implementation defines methods that return resolvers for each object type from the base. Each type (if it has resolver fields) additionally has a TypeNameResolver interface where TypeName would be the object type (e.g. Query).

No changes were made to the existing interface/implementation. To use the new one, a FromShort method will map a ShortResolvers to the existing Resolvers interface. That way the existing Resolvers interface remains the only necessary input type. This also makes it opt-in without breaking backwards compatibility, or adding additional maintenance to the existing code.

The "smaller interfaces" example from #106 (comment) could then be used as follows:

// graph is the package name where `generated.go` lives
graph.MakeExecutableSchema(graph.FromShort(&App{}))

FromShort takes in the App (which fulfills the small interfaces needed by ShortResolvers) and returns the present-day Resolvers interface.


Generated Code Example

Existing interface (unchanged):

type Resolvers interface {
	Query_user(ctx context.Context, id string) (*user.User, error)

	User_avatarURL(ctx context.Context, obj *user.User) (*string, error)
	User_contactMethods(ctx context.Context, obj *user.User) ([]contactmethod.ContactMethod, error)
}

Net new code (example) in generated.go

type ShortResolvers interface {
	Query() QueryResolver
	User() UserResolver
}
type QueryResolver interface {
	User(ctx context.Context, id string) (*user.User, error)
}
type UserResolver interface {
	AvatarURL(ctx context.Context, obj *user.User) (*string, error)
	ContactMethods(ctx context.Context, obj *user.User) ([]contactmethod.ContactMethod, error)
}

func FromShort(r ShortResolvers) Resolvers { return shortMapper{r: r} }

type shortMapper struct {
	r ShortResolvers
}

func (s shortMapper) Query_user(ctx context.Context, id string) (*user.User, error) {
	return s.r.Query().User(ctx, id)
}

func (s shortMapper) User_avatarURL(ctx context.Context, obj *user.User) (*string, error) {
	return s.r.User().AvatarURL(ctx, obj)
}

func (s shortMapper) User_contactMethods(ctx context.Context, obj *user.User) ([]contactmethod.ContactMethod, error) {
	return s.r.User().ContactMethods(ctx, obj)
}

I'm not particularly attached to the name(s), just tried to keep it distinct and descriptive and picked something I could come up with in a reasonable amount of time. Suggestions welcome :)

EDIT: Switched from ShortResolver to ShortResolvers to prevent possible collision with an object named Short

@mastercactapus
Copy link
Contributor Author

@vektah What do you think of this approach overall, worth pursuing?

Also, I kept the implementation simple (just upper-cased the first letter of the field resolver) but I didn't see the ToCamel helper when I did -- that might be a better choice for mapping the names.

@vektah
Copy link
Collaborator

vektah commented Jun 21, 2018

I don't think I want to maintaining code between two different ways of doing this, there should be one good way to do this.

I am open to changing the core implementation though, and this does look pretty nice. I'm more interested in the changes to user code that this would cause, and some examples of that would be great. I might try converting one of our codebases over to see what it looks like.

@mastercactapus
Copy link
Contributor Author

Fair point. If it is introduced as a breaking change we should make sure it plays well with dep and such so things don't break for people updating though. I think we should be careful/thoughtful of the migration path, so as to not hurt adoption and set precedence for future changes as they may happen.

And as far as that goes (and thus the changes to user code) I'm thinking:

  • Offer a go fix equivalent that will refactor existing code (would require some research, but maybe the most painless for users)
  • Deprecate MakeExecutableSchema, make a new method, and offer a way to generate a "shim" (inverse of this PR, old interface in - new interface out), projects can upgrade on their time
  • Release as a new major version, and if no tooling, offer suggestions on how to restructure/refactor existing code

Maybe we can try looking at usage around github. If people have a single struct with all methods, for example, something not too unlike what's in this PR would work. If there are projects that break the resolver into pieces, using embedded structs to put them together, it's a little more complex, but still possible.

It could be a fun problem to solve (not sarcastic) :)

@eric-am
Copy link

eric-am commented Jun 22, 2018 via email

@vektah
Copy link
Collaborator

vektah commented Jun 23, 2018

I think breaks are fine while gqlgen is still 0.x provided that:

  • they fail at compile time, eg dont swap the meaning of args with the same types.
  • there is a good description of the change + migration notes in the release notes

Deprecate MakeExecutableSchema, make a new method, and offer a way to generate a "shim" (inverse of this PR, old interface in - new interface out), projects can upgrade on their time

This sounds good, lets land this so people can start opting into the new behaviour and work on the rest as a separate piece.

Offer a go fix equivalent that will refactor existing code (would require some research, but maybe the most painless for users)

First thought is this is probably overkill, but it does feed really well into #9.

There are a few codemods that would make working with gqlgen nicer:

  • new project = generate all resolver stubs
  • modified schema = add stubs / remove resolvers
  • breaking changes = convert resolvers between formats

@mastercactapus
Copy link
Contributor Author

mastercactapus commented Jun 24, 2018

This sounds good, lets land this so people can start opting into the new behaviour and work on the rest as a separate piece.

@vektah Just to make sure I understand, for this PR:

  • add a new function (How about NewExecutableSchema(ShortResolvers) gqlgen.ExecutableSchema ?)
  • from there, for now, call MakeExecutableSchema with a private wrapper (make WithShort private) .

Sound right?

Also:

  • Any names we should change? (e.g. SchemaResolvers instead of ShortResolvers or something?)
  • Should I add a deprecation comment to MakeExecutableSchema, or wait?

@vektah
Copy link
Collaborator

vektah commented Jun 25, 2018

add a new function (How about NewExecutableSchema(ShortResolvers) gqlgen.ExecutableSchema ?)
from there, for now, call MakeExecutableSchema with a private wrapper (make WithShort private) .

👌

Any names we should change? (e.g. SchemaResolvers instead of ShortResolvers or something?)

SchemaResolvers sounds like something that returns schema. What about ResolverRoot? It's not a resolver itself, its an interface returning each of the resolvers?

Should I add a deprecation comment to MakeExecutableSchema, or wait?

Yeah, lets mark it as deprecated and maybe a link pointing back here or to the issue.

There are some doc updates here too, I wonder if they should land as part of this PR or in a follow up?

@mastercactapus
Copy link
Contributor Author

Updated, and went with ResolverRoot. I'll play with it a bit through the day today and make sure nothing else crops up. I'll post findings by EOD.

For the doc updates, a case could be made either way. One reason for follow up work would be to give some time to determine what the "recommended" structure/conventions should be with the new interface.

@mastercactapus
Copy link
Contributor Author

On second thought, maybe updating the examples is warranted:

example/chat/server/server.go:23:40:warning: chat.MakeExecutableSchema is deprecated: Use NewExecutableSchema instead.

:)

@vektah
Copy link
Collaborator

vektah commented Jun 25, 2018

One reason for follow up work would be to give some time to determine what the "recommended" structure/conventions should be with the new interface.

Yeah, I think that makes sense. Lets leave it a bit and give people a chance to try it.

It probably makes sense to do the deprecation/docs/examples all together. Lets pull the deprecation out for now so this can land cleanly on its own.

- t.Parallel() doesn't guarantee parallel execution
- moved goroutine so the test can execute independently
@mastercactapus
Copy link
Contributor Author

Did a bit of testing on our test branch today, seems to be good.

postErrCh := make(chan error)
go func() {
var resp interface{}
// can't call t.Fatal from separate goroutine, so we return to the err chan for later
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can't call fatal (because it relies on panic handlers...) but you can call t.Error and friends (assert.Equal, instead of require.Equal).

I think this can go back to the way it was, except using go func, and assert instead of t.Run and require.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I think that comment might be a little misleading (see: oops forgot to update).

The issue we saw locally (after moving out of t.Run) was that sub.Next would hang indefinitely (or until timeout) if the mutation call failed (we did a negative test by making the mutation invalid to see) because it never gets any message.

The real reason it's there (and what that comment probably should say) is so we can use select to wait for either sub.Next or the Post call to fail and then exit to avoid the deadlock.

@vektah vektah merged commit 9fa3f0f into 99designs:master Jun 26, 2018
@mastercactapus mastercactapus deleted the small-interfaces branch June 26, 2018 13:50
@jayp
Copy link

jayp commented Jun 27, 2018

Nice one @mastercactapus

cgxxv pushed a commit to cgxxv/gqlgen that referenced this pull request Mar 25, 2022
add lint-friendly small interfaces option for resolvers
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants