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

Separate resolvers for queries and mutations #182

Open
wants to merge 7 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@sanae10001
Copy link
Contributor

sanae10001 commented Mar 15, 2018

See #145

@sanae10001 sanae10001 force-pushed the sanae10001:separateresolvers branch from 3ac05e2 to df9ba5a Mar 15, 2018

@sanae10001

This comment has been minimized.

Copy link
Contributor Author

sanae10001 commented Mar 15, 2018

github.com/graph-gophers/graphql-go (download)
github.com/opentracing/opentracing-go (download)
Fetching https://golang.org/x/net/context?go-get=1
Parsing meta tags from https://golang.org/x/net/context?go-get=1 (status code 200)
get "golang.org/x/net/context": found meta tag main.metaImport{Prefix:"golang.org/x/net", VCS:"git", RepoRoot:"https://go.googlesource.com/net"} at https://golang.org/x/net/context?go-get=1
get "golang.org/x/net/context": verifying non-authoritative meta tag
Fetching https://golang.org/x/net?go-get=1
Parsing meta tags from https://golang.org/x/net?go-get=1 (status code 200)
golang.org/x/net (download)
graphql.go:10:2: use of internal package not allowed
graphql.go:11:2: use of internal package not allowed
graphql.go:12:2: use of internal package not allowed
graphql.go:13:2: use of internal package not allowed
graphql.go:14:2: use of internal package not allowed
graphql.go:15:2: use of internal package not allowed
graphql.go:16:2: use of internal package not allowed
internal/exec/exec.go:11:2: use of internal package not allowed
internal/exec/exec.go:12:2: use of internal package not allowed
internal/exec/exec.go:13:2: use of internal package not allowed
internal/exec/exec.go:14:2: use of internal package not allowed
internal/exec/exec.go:15:2: use of internal package not allowed
internal/exec/packer/packer.go:10:2: use of internal package not allowed
internal/exec/packer/packer.go:11:2: use of internal package not allowed
internal/exec/resolvable/meta.go:7:2: use of internal package not allowed
internal/exec/resolvable/resolvable.go:12:2: use of internal package not allowed
internal/exec/resolvable/meta.go:8:2: use of internal package not allowed
internal/exec/selected/selected.go:9:2: use of internal package not allowed
internal/exec/selected/selected.go:10:2: use of internal package not allowed
internal/exec/selected/selected.go:11:2: use of internal package not allowed
internal/exec/selected/selected.go:12:2: use of internal package not allowed
internal/exec/selected/selected.go:13:2: use of internal package not allowed
internal/query/query.go:9:2: use of internal package not allowed
internal/schema/schema.go:9:2: use of internal package not allowed
internal/validation/validation.go:12:2: use of internal package not allowed
internal/validation/validation.go:13:2: use of internal package not allowed
internal/validation/validation.go:14:2: use of internal package not allowed
introspection/introspection.go:6:2: use of internal package not allowed
introspection/introspection.go:7:2: use of internal package not allowed

What happened?

@ghostiam

This comment has been minimized.

Copy link

ghostiam commented Mar 15, 2018

This change will break compatibility and many people will have to rewrite their code.
I don't want a repeat of the story > satori/go.uuid#66

@tonyghita

This comment has been minimized.

Copy link
Member

tonyghita commented Mar 15, 2018

@sanae10001 are you still working within $GOPATH/src/github.com/neelance/graphql-go? You probably want to update your working directory.

@GhostRussia there may be some way that we can provide the new functionality without breaking clients (i.e. via some option or some temporary cleverness in the resolver lookup code).

Either way, we'll want to ensure we communicate the changes and impact of the changes before merging this in.

@ghostiam

This comment has been minimized.

Copy link

ghostiam commented Mar 15, 2018

I think can use the interface to determine the type of resolver.

type Queryer interface {
	Query() interface{}
}

type Mutationer interface {
	Mutation() interface{}
}

And when using interfaces, we do not have to do extra reflection.

rFunc := root.MethodByName("Query")
if !rFunc.IsValid() {
	return nil, errors.New("not found query resolver")
}
 
queryResolver = rFunc.Call(nil)[0]

use:

type RootResolver struct {
}

func (r *Resolver) Query() interface{} {
	return &QueryResolver{}
}

func (r *Resolver) Mutation() interface{} {
	return &MutationResolver{}
}

graphql.MustParseSchema(schema, &RootResolver),

I checked here, this code allows working with two versions of resolvers. Original tests pass, but other tests are needed.

full resolvers patch:
https://gist.github.com/GhostRussia/faa89c113cfca55366ee3867193d4d09#file-0001-separate-resolver-patch

Adapted tests patch:
https://gist.github.com/GhostRussia/faa89c113cfca55366ee3867193d4d09#file-0002-separate-resolver-test-patch

and pass test https://github.com/graph-gophers/graphql-go/pull/182/files#diff-3a3269459d813f994deb0997a653aac2
after change

- func (r *RootResolver) Query() *QueryResolver {
+ func (r *RootResolver) Query() interface{} {
	return &QueryResolver{}
}

- func (r *RootResolver) Mutation() *MutationResolver {
+ func (r *RootResolver) Mutation() interface{} {
	return &MutationResolver{}
}
@ghostiam

This comment has been minimized.

Copy link

ghostiam commented Mar 16, 2018

#182 (comment)
@tonyghita SemaphoreCI still working within $GOPATH/src/github.com/neelance/graphql-go for PR

@tonyghita

This comment has been minimized.

Copy link
Member

tonyghita commented Mar 16, 2018

Looks like we had two webhooks to the SemaphoreCI integration that were racing each other. I removed the old one that pointed to the previous repository, so we shouldn't see these issues.

@mytototo

This comment has been minimized.

Copy link

mytototo commented Mar 23, 2018

Hi there,

Is there any ETA regarding this PR?
How do you want to handle backward compatibilities?

Thanks a lot for your contribution.

@sanae10001 sanae10001 force-pushed the sanae10001:separateresolvers branch from affd1aa to ffadb9b Mar 26, 2018

@sanae10001

This comment has been minimized.

Copy link
Contributor Author

sanae10001 commented Apr 10, 2018

@mytototo Now, it don't break compatibility. All above codes can correctly run. Maybe more tests are needed.

@mytototo

This comment has been minimized.

Copy link

mytototo commented Apr 18, 2018

@sanae10001 Ok, thanks a lot.

@cad

This comment has been minimized.

Copy link

cad commented Sep 24, 2018

Any plan on merging this?

@mrg0lden

This comment has been minimized.

Copy link

mrg0lden commented Nov 5, 2018

Do you need any help with this PR to get it merged? @pavelnikolov

@sanae10001 sanae10001 force-pushed the sanae10001:separateresolvers branch from ffadb9b to 8996c33 Nov 7, 2018

@pavelnikolov

This comment has been minimized.

Copy link
Member

pavelnikolov commented Nov 8, 2018

I'm trying to respond promptly to all new issues and PRs. I'm sorry for the delay on existing PRs. I'll review shortly.

@pavelnikolov

This comment has been minimized.

Copy link
Member

pavelnikolov commented Nov 16, 2018

I wonder if this can be done in a way that doesn't introduce a breaking change 🤔

@sanae10001

This comment has been minimized.

Copy link
Contributor Author

sanae10001 commented Nov 16, 2018

I wonder if this can be done in a way that doesn't introduce a breaking change 🤔

It don't break compatibility. All above codes can correctly run.

@mrg0lden

This comment has been minimized.

Copy link

mrg0lden commented Nov 16, 2018

@pavelnikolov

This comment has been minimized.

Copy link
Member

pavelnikolov commented Nov 18, 2018

I wonder if this can be done in a way that doesn't introduce a breaking change 🤔

It don't break compatibility. All above codes can correctly run.

This is not completely true. You had to change the Star Wars example, right? This means that all users of the library will have to do the same when they switch to the latest version. It's not optional. It forces the users to upgrade.

@pavelnikolov

This comment has been minimized.

Copy link
Member

pavelnikolov commented Nov 18, 2018

BTW, what about implementing Go modules? This way we can introduce breaking changes with time when a gain is found.

Maybe it's a good idea. This in turn, would also increase complexity. I don't want to introduce something without a clear problem that it solves. This is a very dangerous path that can lead to a lot of unnecessary complexity with very little ROI.

@sanae10001 sanae10001 force-pushed the sanae10001:separateresolvers branch from a34aa17 to e98d8f1 Nov 19, 2018

@sanae10001 sanae10001 force-pushed the sanae10001:separateresolvers branch from e98d8f1 to 1e4bb14 Nov 19, 2018

@sanae10001

This comment has been minimized.

Copy link
Contributor Author

sanae10001 commented Nov 19, 2018

@pavelnikolov
Sorry, my mistake.
The changes of Star Wars example only are legacy codes on commit 80ff5e1.
Already restore.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.