From 7e3f5844adb79c776a30a6e01fcb2b373049fb8e Mon Sep 17 00:00:00 2001 From: Craig Silverstein Date: Fri, 27 Mar 2020 12:07:02 -0700 Subject: [PATCH 1/5] Do not require a resolver for "empty" extended types. Summary: If our schema has a field with a type defined in another service, then we need to define an "empty extend" of that type in this service, so this service knows what the type is like. But the graphql-server will never ask us to actually resolve this "empty extend", so we don't require a resolver function for it. Example: ``` type MyType { myvar: TypeDefinedInOtherService } // Federation needs this type, but it doesn't need a resolver for // it! graphql-server will never ask *us* to resolve a // TypeDefinedInOtherService; it will ask the other service. extend TypeDefinedInOtherService @key(fields: "id") { id: ID @extends } ``` Test Plan: I manually tested this on a service (`assignments`) that we have that fell afoul of this problem. But I had a hard time adding tests inside gqlgen because the error happens at validation-time, and the federation tests are not set up to go that far down the processing path. Reviewers: benkraft, lizfaubell, dhruv Subscribers: #graphql Differential Revision: https://phabricator.khanacademy.org/D61883 --- plugin/federation/federation.go | 41 +++++++++++++++++++++++++----- plugin/federation/federation.gotpl | 40 +++++++++++++++-------------- 2 files changed, 56 insertions(+), 25 deletions(-) diff --git a/plugin/federation/federation.go b/plugin/federation/federation.go index 40e812a7bb0..c51e1fef211 100644 --- a/plugin/federation/federation.go +++ b/plugin/federation/federation.go @@ -95,11 +95,13 @@ func (f *federation) InjectSourceLate(schema *ast.Schema) *ast.Source { } entities += e.Name - resolverArgs := "" - for _, field := range e.KeyFields { - resolverArgs += fmt.Sprintf("%s: %s,", field.Field.Name, field.Field.Type.String()) + if e.ResolverName != "" { + resolverArgs := "" + for _, field := range e.KeyFields { + resolverArgs += fmt.Sprintf("%s: %s,", field.Field.Name, field.Field.Type.String()) + } + resolvers += fmt.Sprintf("\t%s(%s): %s!\n", e.ResolverName, resolverArgs, e.Def.Name) } - resolvers += fmt.Sprintf("\t%s(%s): %s!\n", e.ResolverName, resolverArgs, e.Def.Name) } @@ -162,6 +164,10 @@ type RequireField struct { TypeReference *config.TypeReference // The Go representation of that field type } +func (e *Entity) allFieldsAreKeyFields() bool { + return len(e.Def.Fields) == len(e.KeyFields) +} + func (f *federation) GenerateCode(data *codegen.Data) error { if len(f.Entities) > 0 { data.Objects.ByName("Entity").Root = true @@ -251,13 +257,36 @@ func (f *federation) setEntities(schema *ast.Schema) { } - f.Entities = append(f.Entities, &Entity{ + e := &Entity{ Name: schemaType.Name, KeyFields: keyFields, Def: schemaType, ResolverName: resolverName, Requires: requires, - }) + } + // If our schema has a field with a type defined in + // another service, then we need to define an "empty + // extend" of that type in this service, so this service + // knows what the type is like. But the graphql-server + // will never ask us to actually resolve this "empty + // extend", so we don't require a resolver function for + // it. (Well, it will never ask in practice; it's + // unclear whether the spec guarantees this. See + // https://github.com/apollographql/apollo-server/issues/3852 + // ). Example: + // type MyType { + // myvar: TypeDefinedInOtherService + // } + // // Federation needs this type, but + // // it doesn't need a resolver for it! + // extend TypeDefinedInOtherService @key(fields: "id") { + // id: ID @extends + // } + if (e.allFieldsAreKeyFields()) { + e.ResolverName = "" + } + + f.Entities = append(f.Entities, e) } } } diff --git a/plugin/federation/federation.gotpl b/plugin/federation/federation.gotpl index 2661495a388..96c25e85723 100644 --- a/plugin/federation/federation.gotpl +++ b/plugin/federation/federation.gotpl @@ -34,29 +34,31 @@ func (ec *executionContext) __resolve_entities(ctx context.Context, representati } switch typeName { {{ range .Entities }} - case "{{.Def.Name}}": - {{ range $i, $keyField := .KeyFields -}} - id{{$i}}, err := ec.{{.TypeReference.UnmarshalFunc}}(ctx, rep["{{$keyField.Field.Name}}"]) - if err != nil { - return nil, errors.New(fmt.Sprintf("Field %s undefined in schema.", "{{$keyField.Field.Name}}")) - } - {{end}} - - entity, err := ec.resolvers.Entity().{{.ResolverName | go}}(ctx, - {{ range $i, $_ := .KeyFields -}} id{{$i}}, {{end}}) - if err != nil { - return nil, err - } - - {{ range .Requires }} - {{ range .Fields}} - entity.{{.NameGo}}, err = ec.{{.TypeReference.UnmarshalFunc}}(ctx, rep["{{.Name}}"]) + {{ if .ResolverName }} + case "{{.Def.Name}}": + {{ range $i, $keyField := .KeyFields -}} + id{{$i}}, err := ec.{{.TypeReference.UnmarshalFunc}}(ctx, rep["{{$keyField.Field.Name}}"]) if err != nil { - return nil, err + return nil, errors.New(fmt.Sprintf("Field %s undefined in schema.", "{{$keyField.Field.Name}}")) } + {{end}} + + entity, err := ec.resolvers.Entity().{{.ResolverName | go}}(ctx, + {{ range $i, $_ := .KeyFields -}} id{{$i}}, {{end}}) + if err != nil { + return nil, err + } + + {{ range .Requires }} + {{ range .Fields}} + entity.{{.NameGo}}, err = ec.{{.TypeReference.UnmarshalFunc}}(ctx, rep["{{.Name}}"]) + if err != nil { + return nil, err + } + {{ end }} {{ end }} + list = append(list, entity) {{ end }} - list = append(list, entity) {{ end }} default: return nil, errors.New("unknown type: "+typeName) From 36b5ed834d3444affb6df88b65f4f3ec8a42d488 Mon Sep 17 00:00:00 2001 From: Craig Silverstein Date: Fri, 27 Mar 2020 17:56:53 -0700 Subject: [PATCH 2/5] Actually, we need to check all-external, not all-key. We might well be defining our own type that has only key-fields, but if they're not external then we're the primary provider of the type and thus we need to expose it. I should be loking at `@external`, not `@key`. Test plan: go test ./plugin/federation/ --- plugin/federation/federation.go | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/plugin/federation/federation.go b/plugin/federation/federation.go index c51e1fef211..e9f23cb795f 100644 --- a/plugin/federation/federation.go +++ b/plugin/federation/federation.go @@ -164,8 +164,13 @@ type RequireField struct { TypeReference *config.TypeReference // The Go representation of that field type } -func (e *Entity) allFieldsAreKeyFields() bool { - return len(e.Def.Fields) == len(e.KeyFields) +func (e *Entity) allFieldsAreExternal() bool { + for _, field := range e.Def.Fields { + if field.Directives.ForName("external") == nil { + return false + } + } + return true } func (f *federation) GenerateCode(data *codegen.Data) error { @@ -280,9 +285,9 @@ func (f *federation) setEntities(schema *ast.Schema) { // // Federation needs this type, but // // it doesn't need a resolver for it! // extend TypeDefinedInOtherService @key(fields: "id") { - // id: ID @extends + // id: ID @external // } - if (e.allFieldsAreKeyFields()) { + if (e.allFieldsAreExternal()) { e.ResolverName = "" } From 0e2666fb2bfca1134661dc835b80b1061550225c Mon Sep 17 00:00:00 2001 From: Craig Silverstein Date: Fri, 27 Mar 2020 18:54:40 -0700 Subject: [PATCH 3/5] Run `go fmt` --- plugin/federation/federation.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugin/federation/federation.go b/plugin/federation/federation.go index e9f23cb795f..cfa7caf970c 100644 --- a/plugin/federation/federation.go +++ b/plugin/federation/federation.go @@ -287,7 +287,7 @@ func (f *federation) setEntities(schema *ast.Schema) { // extend TypeDefinedInOtherService @key(fields: "id") { // id: ID @external // } - if (e.allFieldsAreExternal()) { + if e.allFieldsAreExternal() { e.ResolverName = "" } From 1ecd0749dd3ba3ba6ba387d0391e69d410b313de Mon Sep 17 00:00:00 2001 From: Craig Silverstein Date: Fri, 27 Mar 2020 19:16:06 -0700 Subject: [PATCH 4/5] Handle the case that all entities are "empty extend". In that case, there are no resolvers to write, so we shouldn't emit any. --- plugin/federation/federation.go | 18 ++++++++++++------ .../testdata/followschema/out/resolver.go | 3 ++- .../testdata/singlefile/out/resolver.go | 9 +++++++-- 3 files changed, 21 insertions(+), 9 deletions(-) diff --git a/plugin/federation/federation.go b/plugin/federation/federation.go index cfa7caf970c..6d7d9c7c4cb 100644 --- a/plugin/federation/federation.go +++ b/plugin/federation/federation.go @@ -111,18 +111,24 @@ func (f *federation) InjectSourceLate(schema *ast.Schema) *ast.Source { return nil } + // resolvers can be empty if a service defines only "empty + // extend" types. This should be rare. + if resolvers != "" { + resolvers = ` +# fake type to build resolver interfaces for users to implement +type Entity { + ` + resolvers + ` +} +` + } + return &ast.Source{ Name: "federation/entity.graphql", BuiltIn: true, Input: ` # a union of all types that use the @key directive union _Entity = ` + entities + ` - -# fake type to build resolver interfaces for users to implement -type Entity { - ` + resolvers + ` -} - +` + resolvers + ` type _Service { sdl: String } diff --git a/plugin/resolvergen/testdata/followschema/out/resolver.go b/plugin/resolvergen/testdata/followschema/out/resolver.go index 61296288110..fbe00ecff89 100644 --- a/plugin/resolvergen/testdata/followschema/out/resolver.go +++ b/plugin/resolvergen/testdata/followschema/out/resolver.go @@ -1,6 +1,7 @@ +package customresolver + // This file will not be regenerated automatically. // // It serves as dependency injection for your app, add any dependencies you require here. -package customresolver type CustomResolverType struct{} diff --git a/plugin/resolvergen/testdata/singlefile/out/resolver.go b/plugin/resolvergen/testdata/singlefile/out/resolver.go index 99a46892ae4..54d778636ae 100644 --- a/plugin/resolvergen/testdata/singlefile/out/resolver.go +++ b/plugin/resolvergen/testdata/singlefile/out/resolver.go @@ -1,6 +1,7 @@ -// THIS CODE IS A STARTING POINT ONLY. IT WILL NOT BE UPDATED WITH SCHEMA CHANGES. package customresolver +// THIS CODE IS A STARTING POINT ONLY. IT WILL NOT BE UPDATED WITH SCHEMA CHANGES. + import ( "context" ) @@ -10,11 +11,15 @@ type CustomResolverType struct{} func (r *queryCustomResolverType) Resolver(ctx context.Context) (*Resolver, error) { panic("not implemented") } + func (r *resolverCustomResolverType) Name(ctx context.Context, obj *Resolver) (string, error) { panic("not implemented") } -func (r *CustomResolverType) Query() QueryResolver { return &queryCustomResolverType{r} } +// Query returns QueryResolver implementation. +func (r *CustomResolverType) Query() QueryResolver { return &queryCustomResolverType{r} } + +// Resolver returns ResolverResolver implementation. func (r *CustomResolverType) Resolver() ResolverResolver { return &resolverCustomResolverType{r} } type queryCustomResolverType struct{ *CustomResolverType } From 55e0f0db84dfc709c837074d57cd26f9a579e9c5 Mon Sep 17 00:00:00 2001 From: Craig Silverstein Date: Fri, 27 Mar 2020 19:22:44 -0700 Subject: [PATCH 5/5] Check in a place where `Entity` might be nil now. --- plugin/federation/federation.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/plugin/federation/federation.go b/plugin/federation/federation.go index 6d7d9c7c4cb..7d9abc9774c 100644 --- a/plugin/federation/federation.go +++ b/plugin/federation/federation.go @@ -181,7 +181,9 @@ func (e *Entity) allFieldsAreExternal() bool { func (f *federation) GenerateCode(data *codegen.Data) error { if len(f.Entities) > 0 { - data.Objects.ByName("Entity").Root = true + if data.Objects.ByName("Entity") != nil { + data.Objects.ByName("Entity").Root = true + } for _, e := range f.Entities { obj := data.Objects.ByName(e.Def.Name) for _, field := range obj.Fields {