Refactor field resolution closer to schema types (#573)

dackroyd · pavelnikolov · web-flow · commit 8d0aad85f6b2 · 2023-02-18T13:17:34.000+02:00
* Refactor field resolution closer to schema types

Much of the work of executing resolvers relates to the resolvable
portion of fields, rather than the selection made for the request.
With the introduction of directive visitors, these were being recreated
for each request, instead of being built once, and re-used.

By pushing the resolution of fields down into the `resolvable.Field`,
directives applied to the field definition can be setup just once when
the schema is created, and re-used across all requests.
Small exceptions to this exist for field types that embed this to
provide request specific values (resolver instance and arguments),
which get passed along with the call.

The chain of directive visitor functions still needs to be recreated
for each resolve call, because the inner resolve function needs to
accept the request specific field resolver instance, but other
per-request overheads should be removed.

* Fix feedback

---------

Co-authored-by: Pavel Nikolov &lt;me@pavelnikolov.net&gt;
diff --git a/internal/exec/exec.go b/internal/exec/exec.go
@@ -64,12 +64,6 @@ func (r *Request) Execute(ctx context.Context, s *resolvable.Schema, op *types.O
 	return out.Bytes(), r.Errs
 }
 
-type resolverFunc func(ctx context.Context, args interface{}) (output interface{}, err error)
-
-func (f resolverFunc) Resolve(ctx context.Context, args interface{}) (output interface{}, err error) {
-	return f(ctx, args)
-}
-
 type fieldToExec struct {
 	field    *selected.SchemaField
 	sels     []selected.Selection
@@ -78,58 +72,7 @@ type fieldToExec struct {
 }
 
 func (f *fieldToExec) resolve(ctx context.Context) (output interface{}, err error) {
-	var args interface{}
-
-	if f.field.ArgsPacker != nil {
-		args = f.field.PackedArgs.Interface()
-	}
-
-	currResolver := f.resolveField
-
-	for _, pd := range f.field.PackedDirectives {
-		pd := pd // Needed to avoid passing only the last directive, since we're closing over this loop var pointer
-		innerResolver := currResolver
-
-		currResolver = func(ctx context.Context, args interface{}) (output interface{}, err error) {
-			return pd.Resolve(ctx, args, resolverFunc(innerResolver))
-		}
-	}
-
-	return currResolver(ctx, args)
-}
-
-func (f *fieldToExec) resolveField(ctx context.Context, args interface{}) (output interface{}, err error) {
-	if !f.field.UseMethodResolver() {
-		res := f.resolver
-
-		// TODO extract out unwrapping ptr logic to a common place
-		if res.Kind() == reflect.Ptr {
-			res = res.Elem()
-		}
-
-		return res.FieldByIndex(f.field.FieldIndex).Interface(), nil
-	}
-
-	var in []reflect.Value
-	var callOut []reflect.Value
-
-	if f.field.HasContext {
-		in = append(in, reflect.ValueOf(ctx))
-	}
-
-	if f.field.ArgsPacker != nil {
-		in = append(in, reflect.ValueOf(args))
-	}
-
-	callOut = f.resolver.Method(f.field.MethodIndex).Call(in)
-	result := callOut[0]
-
-	if f.field.HasError && !callOut[1].IsNil() {
-		resolverErr := callOut[1].Interface().(error)
-		return result.Interface(), resolverErr
-	}
-
-	return result.Interface(), nil
+	return f.field.Resolve(ctx, f.resolver)
 }
 
 func resolvedToNull(b *bytes.Buffer) bool {
diff --git a/internal/exec/resolvable/resolvable.go b/internal/exec/resolvable/resolvable.go
@@ -47,7 +47,7 @@ type Field struct {
 	HasContext        bool
 	HasError          bool
 	ArgsPacker        *packer.StructPacker
-	DirectivesPackers map[string]*packer.StructPacker
+	DirectiveVisitors []directives.ResolverInterceptor
 	ValueExec         Resolvable
 	TraceLabel        string
 }
@@ -56,6 +56,68 @@ func (f *Field) UseMethodResolver() bool {
 	return len(f.FieldIndex) == 0
 }
 
+func (f *Field) Resolve(ctx context.Context, resolver reflect.Value, args interface{}) (output interface{}, err error) {
+	// Short circuit case to avoid wrapping functions
+	if len(f.DirectiveVisitors) == 0 {
+		return f.resolve(ctx, resolver, args)
+	}
+
+	wrapResolver := func(ctx context.Context, args interface{}) (output interface{}, err error) {
+		return f.resolve(ctx, resolver, args)
+	}
+
+	for _, d := range f.DirectiveVisitors {
+		d := d // Needed to avoid passing only the last directive, since we're closing over this loop var pointer
+		innerResolver := wrapResolver
+
+		wrapResolver = func(ctx context.Context, args interface{}) (output interface{}, err error) {
+			return d.Resolve(ctx, args, resolverFunc(innerResolver))
+		}
+	}
+
+	return wrapResolver(ctx, args)
+}
+
+func (f *Field) resolve(ctx context.Context, resolver reflect.Value, args interface{}) (output interface{}, err error) {
+	if !f.UseMethodResolver() {
+		res := resolver
+
+		// TODO extract out unwrapping ptr logic to a common place
+		if res.Kind() == reflect.Ptr {
+			res = res.Elem()
+		}
+
+		return res.FieldByIndex(f.FieldIndex).Interface(), nil
+	}
+
+	var in []reflect.Value
+	var callOut []reflect.Value
+
+	if f.HasContext {
+		in = append(in, reflect.ValueOf(ctx))
+	}
+
+	if f.ArgsPacker != nil {
+		in = append(in, reflect.ValueOf(args))
+	}
+
+	callOut = resolver.Method(f.MethodIndex).Call(in)
+	result := callOut[0]
+
+	if f.HasError && !callOut[1].IsNil() {
+		resolverErr := callOut[1].Interface().(error)
+		return result.Interface(), resolverErr
+	}
+
+	return result.Interface(), nil
+}
+
+type resolverFunc func(ctx context.Context, args interface{}) (output interface{}, err error)
+
+func (f resolverFunc) Resolve(ctx context.Context, args interface{}) (output interface{}, err error) {
+	return f(ctx, args)
+}
+
 type TypeAssertion struct {
 	MethodIndex int
 	TypeExec    Resolvable
@@ -71,17 +133,22 @@ func (*Object) isResolvable() {}
 func (*List) isResolvable()   {}
 func (*Scalar) isResolvable() {}
 
-func ApplyResolver(s *types.Schema, resolver interface{}, dirVisitors []directives.Directive, useFieldResolvers bool) (*Schema, error) {
+func ApplyResolver(s *types.Schema, resolver interface{}, dirs []directives.Directive, useFieldResolvers bool) (*Schema, error) {
 	if resolver == nil {
 		return &Schema{Meta: newMeta(s), Schema: *s}, nil
 	}
 
-	ds, err := applyDirectives(s, dirVisitors)
+	ds, err := applyDirectives(s, dirs)
+	if err != nil {
+		return nil, err
+	}
+
+	directivePackers, err := buildDirectivePackers(s, ds)
 	if err != nil {
 		return nil, err
 	}
 
-	b := newBuilder(s, ds, useFieldResolvers)
+	b := newBuilder(s, directivePackers, useFieldResolvers)
 
 	var query, mutation, subscription Resolvable
 
@@ -158,6 +225,44 @@ func ApplyResolver(s *types.Schema, resolver interface{}, dirVisitors []directiv
 	}, nil
 }
 
+func buildDirectivePackers(s *types.Schema, visitors map[string]directives.Directive) (map[string]*packer.StructPacker, error) {
+	// Directive packers need to use a dedicated builder which is ready ('finish()' called) while
+	// schema fields (and their argument packers) are still being built
+	builder := packer.NewBuilder()
+
+	packers := map[string]*packer.StructPacker{}
+	for _, d := range s.Directives {
+		n := d.Name
+
+		v, ok := visitors[n]
+		if !ok {
+			// Directives which need visitors have already been checked
+			// Anything without a visitor now is a built-in directive without a packer.
+			continue
+		}
+
+		if _, ok = v.(directives.ResolverInterceptor); !ok {
+			// Directive doesn't apply at field resolution time, skip it
+			continue
+		}
+
+		r := reflect.TypeOf(v)
+
+		p, err := builder.MakeStructPacker(d.Arguments, r)
+		if err != nil {
+			return nil, err
+		}
+
+		packers[n] = p
+	}
+
+	if err := builder.Finish(); err != nil {
+		return nil, err
+	}
+
+	return packers, nil
+}
+
 func applyDirectives(s *types.Schema, visitors []directives.Directive) (map[string]directives.Directive, error) {
 	byName := make(map[string]directives.Directive, len(s.Directives))
 
@@ -209,7 +314,7 @@ func applyDirectives(s *types.Schema, visitors []directives.Directive) (map[stri
 type execBuilder struct {
 	schema            *types.Schema
 	resMap            map[typePair]*resMapEntry
-	directives        map[string]directives.Directive
+	directivePackers  map[string]*packer.StructPacker
 	packerBuilder     *packer.Builder
 	useFieldResolvers bool
 }
@@ -224,11 +329,11 @@ type resMapEntry struct {
 	targets []*Resolvable
 }
 
-func newBuilder(s *types.Schema, directives map[string]directives.Directive, useFieldResolvers bool) *execBuilder {
+func newBuilder(s *types.Schema, directives map[string]*packer.StructPacker, useFieldResolvers bool) *execBuilder {
 	return &execBuilder{
 		schema:            s,
 		resMap:            make(map[typePair]*resMapEntry),
-		directives:        directives,
+		directivePackers:  directives,
 		packerBuilder:     packer.NewBuilder(),
 		useFieldResolvers: useFieldResolvers,
 	}
@@ -471,39 +576,9 @@ func (b *execBuilder) makeFieldExec(typeName string, f *types.FieldDefinition, m
 		}
 	}
 
-	directivesPackers := map[string]*packer.StructPacker{}
-	for _, d := range f.Directives {
-		n := d.Name.Name
-
-		// skip special directives without packers
-		if n == "deprecated" {
-			continue
-		}
-
-		v, ok := b.directives[n]
-		if !ok {
-			return nil, fmt.Errorf("directive %q on field %q does not have a visitor registered with the schema", n, f.Name)
-		}
-
-		if _, ok = v.(directives.ResolverInterceptor); !ok {
-			// Directive doesn't apply at field resolution time, skip it
-			continue
-		}
-
-		r := reflect.TypeOf(v)
-
-		// The directive definition is needed here in order to get the arguments definition list.
-		// d.Arguments wouldn't work in this case because it does not contain args type information.
-		dd, ok := b.schema.Directives[n]
-		if !ok {
-			return nil, fmt.Errorf("directive definition %q is not defined in the schema", n)
-		}
-		p, err := b.packerBuilder.MakeStructPacker(dd.Arguments, r)
-		if err != nil {
-			return nil, err
-		}
-
-		directivesPackers[n] = p
+	pd, err := packDirectives(f.Directives, b.directivePackers)
+	if err != nil {
+		return nil, err
 	}
 
 	fe := &Field{
@@ -513,7 +588,7 @@ func (b *execBuilder) makeFieldExec(typeName string, f *types.FieldDefinition, m
 		FieldIndex:        fieldIndex,
 		HasContext:        hasContext,
 		ArgsPacker:        argsPacker,
-		DirectivesPackers: directivesPackers,
+		DirectiveVisitors: pd,
 		HasError:          hasError,
 		TraceLabel:        fmt.Sprintf("GraphQL field: %s.%s", typeName, f.Name),
 	}
@@ -535,6 +610,32 @@ func (b *execBuilder) makeFieldExec(typeName string, f *types.FieldDefinition, m
 	return fe, nil
 }
 
+func packDirectives(ds types.DirectiveList, packers map[string]*packer.StructPacker) ([]directives.ResolverInterceptor, error) {
+	packed := make([]directives.ResolverInterceptor, 0, len(ds))
+	for _, d := range ds {
+		dp, ok := packers[d.Name.Name]
+		if !ok {
+			continue // skip directives without packers
+		}
+
+		args := make(map[string]interface{})
+		for _, arg := range d.Arguments {
+			args[arg.Name.Name] = arg.Value.Deserialize(nil)
+		}
+
+		p, err := dp.Pack(args)
+		if err != nil {
+			return nil, err
+		}
+
+		v := p.Interface().(directives.ResolverInterceptor)
+
+		packed = append(packed, v)
+	}
+
+	return packed, nil
+}
+
 func findMethod(t reflect.Type, name string) int {
 	for i := 0; i < t.NumMethod(); i++ {
 		if strings.EqualFold(stripUnderscore(name), stripUnderscore(t.Method(i).Name)) {
diff --git a/internal/exec/selected/selected.go b/internal/exec/selected/selected.go
