Separate resolvers for queries and mutations

[nicksnyder]

It appears that the schemaResolver passed in to ParseSchema must contain all root resolvers for both queries and mutations. Since these are separate namespaces in the graphql interface, it would be great to have them be separate namespaces (resolver objects) in the Go implementation too.

Consider the following schema:

schema {
  query: Query
  mutation: Mutation
}

type Query {
    something: Something
}

type Mutation {
    updateSomething(value: String): Something
}

type Something {
    value: String
}

This currently requires an implementation like this:

func init() {
    graphql.MustParseSchema(schemaFromSnippetAbove, &schemaResolver{})
}

type schemaResolver struct{}

func (r *schemaResolver) Something() *somethingResolver {
	return &somethingResolver{}
}

func (r *schemaResolver) UpdateSomething(ctx context.Context, args *struct {
	Value string
}) *somethingResolver {
	return &somethingResolver{value: args.Value}
}

type somethingResolver struct {
	value string
}

func (r *somethingResolver) Value() string {
	return r.value
}

But I would prefer it to require something like this:

func init() {
    graphql.MustParseSchema(schemaFromSnippetAbove, &schemaResolver{})
}

type schemaResolver struct{}

func (r *schemaResolver) Query() *queryResolver {
	return &queryResolver{}
}

func (r *schemaResolver) Mutation() *mutationResolver {
	return &mutationResolver{}
}

type queryResolver struct{}

func (r *queryResolver) Something() *somethingResolver {
	return &somethingResolver{}
}

type mutationResolver struct{}

func (r *mutationResolver) UpdateSomething(ctx context.Context, args *struct {
	Value string
}) *somethingResolver {
	return &somethingResolver{value: args.Value}
}

type somethingResolver struct {
	value string
}

func (r *somethingResolver) Value() string {
	return r.value
}

This would better model what is happening at the graphql layer, which would make the server code more understandable, and also avoid unnecessary limitations (e.g. GraphQL allows you to have a mutation and a query with the same name, but graphql-go does not).

[bsr203]

similar request.
#142
#128

[nicksnyder]

#142 is not the right solution in my option. I think there should still be a single top level resolver and that resolver should explicitly model the query resolver and the mutation resolver, as described above.

[tonyghita]

And later, we'll need to add a subscription resolver as well

[nicksnyder]

Exactly

[sanae10001]

Good idea.

[pavelnikolov]

If the query and mutation resolvers are split, it'd be easy to allow only query requests over HTTP GET requests - see issue #108

[tonyghita]

@neelance would you mind if we started work on a pull-request to resolve this issue? What are your thoughts on the approach we should take?

[tonyghita]

The SDL RFC was merged into the specification recently, and I think approaches queries vs. mutations vs. subscriptions in this way: https://github.com/facebook/graphql/pull/90/files#diff-fe406b08746616e2f5f00909488cce66R167

[sanae10001]

Any working PR about this?

[tonyghita]

We don't yet have a PR open for this specific change. I'd be happy to work through it with someone if they were to open a PR.

I'd like to get this change in but I'm not quite there yet (still working through the schema and query parsing code to gain understanding).

[sanae10001]

@tonyghita We can together discuss this PR.

[vasiliy-t]

Why not just embed specific resolvers in root resolver?

type RootResolver struct {
    mutationResolver
    queryResolver
}

type queryResolver struct{}
type mutationResolver struct{}

[nicksnyder]

See issue description:

GraphQL allows you to have a mutation and a query with the same name, but graphql-go does not).

If you have a mutation and query with the same name, your suggestion wouldn’t compile.

[vasiliy-t]

@nicksnyder Now I see the point. Thank you.

[ghostiam]

Changes should be compatible with the existing code, if we do not want to break everything, like satori/go.uuid#66

[sanae10001]

@GhostRussia Yes, you are right. Updated.

[tendolukwago]

Can anyone comment on the status of this? As of now, is there a recommended way of implementing this? I'm having trouble implementing mutations and some guidance would be much appreciated. @nicksnyder

[CreatCodeBuild]

Any progress on this? I really need this feature to implement my API.

[abourget]

Another option would be to add a SchemaOpt that prefixes all function mappings with the root type (Query, Mutation, Subscription).

Would be opt-in, would allow splitting of resolvers without ambiguity.

Otherwise, any progress?

[abourget]

here's a sample patch:

diff --git a/graphql.go b/graphql.go
index be0e1c4..e317c93 100644
--- a/graphql.go
+++ b/graphql.go
@@ -38,7 +38,7 @@ func ParseSchema(schemaString string, resolver interface{}, opts ...SchemaOpt) (
 	}
 
 	if resolver != nil {
-		r, err := resolvable.ApplyResolver(s.schema, resolver)
+		r, err := resolvable.ApplyResolver(s.schema, resolver, s.prefixRootFunctions)
 		if err != nil {
 			return nil, err
 		}
@@ -69,6 +69,7 @@ type Schema struct {
 	logger                log.Logger
 	useStringDescriptions bool
 	disableIntrospection  bool
+	prefixRootFunctions   bool
 }
 
 // SchemaOpt is an option to pass to ParseSchema or MustParseSchema.
@@ -98,6 +99,13 @@ func MaxDepth(n int) SchemaOpt {
 	}
 }
 
+// Add the Query, Subscription and Mutation prefixes to the root resolver function when doing reflection from schema to Go code.
+func PrefixRootFunctions() SchemaOpt {
+	return func(s *Schema) {
+		s.prefixRootFunctions = true
+	}
+}
+
 // MaxParallelism specifies the maximum number of resolvers per request allowed to run in parallel. The default is 10.
 func MaxParallelism(n int) SchemaOpt {
 	return func(s *Schema) {
diff --git a/internal/exec/resolvable/resolvable.go b/internal/exec/resolvable/resolvable.go
index 2780923..d9791d0 100644
--- a/internal/exec/resolvable/resolvable.go
+++ b/internal/exec/resolvable/resolvable.go
@@ -60,25 +60,25 @@ func (*Object) isResolvable() {}
 func (*List) isResolvable()   {}
 func (*Scalar) isResolvable() {}
 
-func ApplyResolver(s *schema.Schema, resolver interface{}) (*Schema, error) {
+func ApplyResolver(s *schema.Schema, resolver interface{}, prefixRootFuncs bool) (*Schema, error) {
 	b := newBuilder(s)
 
 	var query, mutation, subscription Resolvable
 
 	if t, ok := s.EntryPoints["query"]; ok {
-		if err := b.assignExec(&query, t, reflect.TypeOf(resolver)); err != nil {
+		if err := b.assignExec(&query, t, reflect.TypeOf(resolver), prefixRootFuncs); err != nil {
 			return nil, err
 		}
 	}
 
 	if t, ok := s.EntryPoints["mutation"]; ok {
-		if err := b.assignExec(&mutation, t, reflect.TypeOf(resolver)); err != nil {
+		if err := b.assignExec(&mutation, t, reflect.TypeOf(resolver), prefixRootFuncs); err != nil {
 			return nil, err
 		}
 	}
 
 	if t, ok := s.EntryPoints["subscription"]; ok {
-		if err := b.assignExec(&subscription, t, reflect.TypeOf(resolver)); err != nil {
+		if err := b.assignExec(&subscription, t, reflect.TypeOf(resolver), prefixRootFuncs); err != nil {
 			return nil, err
 		}
 	}
@@ -130,14 +130,14 @@ func (b *execBuilder) finish() error {
 	return b.packerBuilder.Finish()
 }
 
-func (b *execBuilder) assignExec(target *Resolvable, t common.Type, resolverType reflect.Type) error {
+func (b *execBuilder) assignExec(target *Resolvable, t common.Type, resolverType reflect.Type, prefixFuncs bool) error {
 	k := typePair{t, resolverType}
 	ref, ok := b.resMap[k]
 	if !ok {
 		ref = &resMapEntry{}
 		b.resMap[k] = ref
 		var err error
-		ref.exec, err = b.makeExec(t, resolverType)
+		ref.exec, err = b.makeExec(t, resolverType, prefixFuncs)
 		if err != nil {
 			return err
 		}
@@ -146,13 +146,13 @@ func (b *execBuilder) assignExec(target *Resolvable, t common.Type, resolverType
 	return nil
 }
 
-func (b *execBuilder) makeExec(t common.Type, resolverType reflect.Type) (Resolvable, error) {
+func (b *execBuilder) makeExec(t common.Type, resolverType reflect.Type, prefixFuncs bool) (Resolvable, error) {
 	var nonNull bool
 	t, nonNull = unwrapNonNull(t)
 
 	switch t := t.(type) {
 	case *schema.Object:
-		return b.makeObjectExec(t.Name, t.Fields, nil, nonNull, resolverType)
+		return b.makeObjectExecWithPrefix(t.Name, t.Fields, nil, nonNull, resolverType, prefixFuncs)
 
 	case *schema.Interface:
 		return b.makeObjectExec(t.Name, t.Fields, t.PossibleTypes, nonNull, resolverType)
@@ -180,7 +180,7 @@ func (b *execBuilder) makeExec(t common.Type, resolverType reflect.Type) (Resolv
 			return nil, fmt.Errorf("%s is not a slice", resolverType)
 		}
 		e := &List{}
-		if err := b.assignExec(&e.Elem, t.OfType, resolverType.Elem()); err != nil {
+		if err := b.assignExec(&e.Elem, t.OfType, resolverType.Elem(), false); err != nil {
 			return nil, err
 		}
 		return e, nil
@@ -212,6 +212,9 @@ func makeScalarExec(t *schema.Scalar, resolverType reflect.Type) (Resolvable, er
 
 func (b *execBuilder) makeObjectExec(typeName string, fields schema.FieldList, possibleTypes []*schema.Object,
 	nonNull bool, resolverType reflect.Type) (*Object, error) {
+	return b.makeObjectExecWithPrefix(typeName, fields, possibleTypes, nonNull, resolverType, false)
+}
+func (b *execBuilder) makeObjectExecWithPrefix(typeName string, fields schema.FieldList, possibleTypes []*schema.Object, nonNull bool, resolverType reflect.Type, prefixFuncs bool) (*Object, error) {
 	if !nonNull {
 		if resolverType.Kind() != reflect.Ptr && resolverType.Kind() != reflect.Interface {
 			return nil, fmt.Errorf("%s is not a pointer or interface", resolverType)
@@ -223,17 +226,23 @@ func (b *execBuilder) makeObjectExec(typeName string, fields schema.FieldList, p
 	Fields := make(map[string]*Field)
 	rt := unwrapPtr(resolverType)
 	for _, f := range fields {
+
+		methodName := f.Name
+		if prefixFuncs {
+			methodName = typeName + f.Name
+		}
+
 		fieldIndex := -1
-		methodIndex := findMethod(resolverType, f.Name)
+		methodIndex := findMethod(resolverType, methodName)
 		if b.schema.UseFieldResolvers && methodIndex == -1 {
 			fieldIndex = findField(rt, f.Name)
 		}
 		if methodIndex == -1 && fieldIndex == -1 {
 			hint := ""
-			if findMethod(reflect.PtrTo(resolverType), f.Name) != -1 {
+			if findMethod(reflect.PtrTo(resolverType), methodName) != -1 {
 				hint = " (hint: the method exists on the pointer type)"
 			}
-			return nil, fmt.Errorf("%s does not resolve %q: missing method for field %q%s", resolverType, typeName, f.Name, hint)
+			return nil, fmt.Errorf("%s does not resolve %q: missing method for field %q%s", resolverType, typeName, methodName, hint)
 		}
 
 		var m reflect.Method
@@ -266,7 +275,7 @@ func (b *execBuilder) makeObjectExec(typeName string, fields schema.FieldList, p
 			a := &TypeAssertion{
 				MethodIndex: methodIndex,
 			}
-			if err := b.assignExec(&a.TypeExec, impl, resolverType.Method(methodIndex).Type.Out(0)); err != nil {
+			if err := b.assignExec(&a.TypeExec, impl, resolverType.Method(methodIndex).Type.Out(0), false); err != nil {
 				return nil, err
 			}
 			typeAssertions[impl.Name] = a
@@ -358,7 +367,7 @@ func (b *execBuilder) makeFieldExec(typeName string, f *schema.Field, m reflect.
 	} else {
 		out = sf.Type
 	}
-	if err := b.assignExec(&fe.ValueExec, f.Type, out); err != nil {
+	if err := b.assignExec(&fe.ValueExec, f.Type, out, false); err != nil {
 		return nil, err
 	}

What about that?

[abourget]

We have the above in our fork, I'd gladly push it upstream if the method is acceptable. (https://github.com/dfuse-io/graphql-go)

[pavelnikolov]

@abourget, does this mean that, for example, if you want to use both a query and a mutation named Products, then in your Go code in the schema resolver you have methods named QueryProducts and MutationProducts respectively? If that is the case, I am not a big fan of this. I do like that it is opt-in, though.
I still want this to be opt-in (e.i. no braking changes), however, when enabled I would prefer one struct with (up to) three methods, as suggested by the original post above:

type schemaResolver struct{}

func (r *schemaResolver) Query() *queryResolver {
	return &queryResolver{}
}

func (r *schemaResolver) Mutation() *mutationResolver {
	return &mutationResolver{}
}

func (r *schemaResolver) Subscription() *subscriptionResolver {
	return &subscriptionResolver{}
}
...

The way you suggest it doesn't add breaking changes but adds semantical meaning to the name of the resolvers. This in turn increases complexity...

[abourget]

Interesting, I can see that.

In my own code, it did keep things simpler when I wanted to opt into this, because my root resolver, which already had all the objects and database connections as fields, didn't need to be split in 3. What you propose would require three objects, each with their fields for databases, or a reference to the actual root resolver.

I prefer pushing the complexity into the lib than into the hands of users like me, but perhaps I haven't looked at how simple your proposition would be in the lib, so I can't measure the complexity delta. Do you think there's a short path to handle those three fields, keeping them opt-in?

[pavelnikolov]

TBH, although it might sound like a cliche, there is no short path to where we want to go. Having semantic prefixes feels like magic. And requires internal knowledge of the library or reading documentation in order to use it. In other words, it is not intuitive. I would prefer to wait until I'm happy with the solution. With that being said, what is the problem with reusing the DB code the Query, Mutation and Subscription resolvers respectively? Could you give me a very simple example of how you use it? I want to understand your point of view.

[smithaitufe]

@pavelnikolov Please do we have any update on this?

[pavelnikolov]

Not that I am aware of.

[phifty]

Still no progress here?

Since the lib is still pre version 1.0, would expect a breaking change not such a big thing. Currently, a clean interface should be a higher goal.

[pavelnikolov]

@phifty breaking changes are not acceptable at this stage. Versions and tags will be added shortly.

[maoueh]

For info, we use PR #421 in production and we like the way it feels. Personally, I add the option in every project we do, the resolve reads better in my opinion even when there is no conflicts.

func QuerySomething(...) { ... }

func SubscriptionTransactions(...) { ... }

func MutationUser(...) { ... }