From db1e3b81e71adcbad5143cf4b91fb402bb7ceba6 Mon Sep 17 00:00:00 2001 From: Joseph Lyons Date: Tue, 6 Dec 2022 00:19:31 +1100 Subject: [PATCH] Implicit external check (#2449) * Prevent entity resolver generation for stub types. In Federation 2 key fields are implicitly external * Add more comments to "isResolvable" * Check that no resolvers are set for stub "Hello" * Run generate with go 1.16 * Simplify implicit external check * Add stricter federation version check. Update comment on expected behavior of the resolvable argument. Add comment to documentation about external directive. * Preallocate keyFields slice * Add non stub type to federation v2 test * Do not append to preallocated slice * Add test coverage for multiple fields in key * Fix typo in comment --- docs/content/recipes/federation.md | 4 +- plugin/federation/entity.go | 109 ++++++++++++++++++ plugin/federation/federation.go | 40 +------ plugin/federation/federation_test.go | 6 + .../testdata/federation2/federation2.graphql | 10 ++ .../federation2/generated/federation.go | 24 ++-- .../modelgen/out_struct_pointers/generated.go | 11 ++ .../filetemplate/out/schema.custom.go | 8 +- .../followschema/out/schema.resolvers.go | 8 +- 9 files changed, 159 insertions(+), 61 deletions(-) create mode 100644 plugin/federation/entity.go diff --git a/docs/content/recipes/federation.md b/docs/content/recipes/federation.md index 46fc94b53b..65a0a922f9 100644 --- a/docs/content/recipes/federation.md +++ b/docs/content/recipes/federation.md @@ -47,12 +47,12 @@ type Review { } extend type User @key(fields: "id") { - id: ID! @external + id: ID! @external # External directive not required for key fields in federation v2 reviews: [Review] } extend type Product @key(fields: "upc") { - upc: String! @external + upc: String! @external # External directive not required for key fields in federation v2 reviews: [Review] } ``` diff --git a/plugin/federation/entity.go b/plugin/federation/entity.go new file mode 100644 index 0000000000..0d7fbed6f4 --- /dev/null +++ b/plugin/federation/entity.go @@ -0,0 +1,109 @@ +package federation + +import ( + "github.com/99designs/gqlgen/codegen/config" + "github.com/99designs/gqlgen/plugin/federation/fieldset" + "github.com/vektah/gqlparser/v2/ast" +) + +// Entity represents a federated type +// that was declared in the GQL schema. +type Entity struct { + Name string // The same name as the type declaration + Def *ast.Definition + Resolvers []*EntityResolver + Requires []*Requires + Multi bool +} + +type EntityResolver struct { + ResolverName string // The resolver name, such as FindUserByID + KeyFields []*KeyField // The fields declared in @key. + InputType string // The Go generated input type for multi entity resolvers +} + +type KeyField struct { + Definition *ast.FieldDefinition + Field fieldset.Field // len > 1 for nested fields + Type *config.TypeReference // The Go representation of that field type +} + +// Requires represents an @requires clause +type Requires struct { + Name string // the name of the field + Field fieldset.Field // source Field, len > 1 for nested fields + Type *config.TypeReference // The Go representation of that field type +} + +func (e *Entity) allFieldsAreExternal(federationVersion int) bool { + for _, field := range e.Def.Fields { + if !e.isFieldImplicitlyExternal(field, federationVersion) && field.Directives.ForName("external") == nil { + return false + } + } + return true +} + +// In federation v2, key fields are implicitly external. +func (e *Entity) isFieldImplicitlyExternal(field *ast.FieldDefinition, federationVersion int) bool { + // Key fields are only implicitly external in Federation 2 + if federationVersion != 2 { + return false + } + // TODO: From the spec, it seems like if an entity is not resolvable then it should not only not have a resolver, but should not appear in the _Entitiy union. + // The current implementation is a less drastic departure from the previous behavior, but should probably be reviewed. + // See https://www.apollographql.com/docs/federation/subgraph-spec/ + if e.isResolvable() { + return false + } + // If the field is a key field, it is implicitly external + if e.isKeyField(field) { + return true + } + + return false +} + +// Determine if the entity is resolvable. +func (e *Entity) isResolvable() bool { + key := e.Def.Directives.ForName("key") + if key == nil { + // If there is no key directive, the entity is resolvable. + return true + } + resolvable := key.Arguments.ForName("resolvable") + if resolvable == nil { + // If there is no resolvable argument, the entity is resolvable. + return true + } + // only if resolvable: false has been set on the @key directive do we consider the entity non-resolvable. + return resolvable.Value.Raw != "false" +} + +// Determine if a field is part of the entities key. +func (e *Entity) isKeyField(field *ast.FieldDefinition) bool { + for _, keyField := range e.keyFields() { + if keyField == field.Name { + return true + } + } + return false +} + +// Get the key fields for this entity. +func (e *Entity) keyFields() []string { + key := e.Def.Directives.ForName("key") + if key == nil { + return []string{} + } + fields := key.Arguments.ForName("fields") + if fields == nil { + return []string{} + } + fieldSet := fieldset.New(fields.Value.Raw, nil) + keyFields := make([]string, len(fieldSet)) + for i, field := range fieldSet { + keyFields[i] = field[0] + } + return keyFields +} diff --git a/plugin/federation/federation.go b/plugin/federation/federation.go index cd60acee25..d0d3cac8a2 100644 --- a/plugin/federation/federation.go +++ b/plugin/federation/federation.go @@ -198,44 +198,6 @@ type Entity { } } -// Entity represents a federated type -// that was declared in the GQL schema. -type Entity struct { - Name string // The same name as the type declaration - Def *ast.Definition - Resolvers []*EntityResolver - Requires []*Requires - Multi bool -} - -type EntityResolver struct { - ResolverName string // The resolver name, such as FindUserByID - KeyFields []*KeyField // The fields declared in @key. - InputType string // The Go generated input type for multi entity resolvers -} - -type KeyField struct { - Definition *ast.FieldDefinition - Field fieldset.Field // len > 1 for nested fields - Type *config.TypeReference // The Go representation of that field type -} - -// Requires represents an @requires clause -type Requires struct { - Name string // the name of the field - Field fieldset.Field // source Field, len > 1 for nested fields - Type *config.TypeReference // The Go representation of that field type -} - -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 { if len(f.Entities) > 0 { if data.Objects.ByName("Entity") != nil { @@ -323,7 +285,7 @@ func (f *federation) setEntities(schema *ast.Schema) { // extend TypeDefinedInOtherService @key(fields: "id") { // id: ID @external // } - if !e.allFieldsAreExternal() { + if !e.allFieldsAreExternal(f.Version) { for _, dir := range keys { if len(dir.Arguments) > 2 { panic("More than two arguments provided for @key declaration.") diff --git a/plugin/federation/federation_test.go b/plugin/federation/federation_test.go index 75fb462b8c..a0579f437d 100644 --- a/plugin/federation/federation_test.go +++ b/plugin/federation/federation_test.go @@ -121,6 +121,12 @@ func TestCodeGenerationFederation2(t *testing.T) { err := f.MutateConfig(cfg) require.NoError(t, err) + require.Equal(t, "ExternalExtension", f.Entities[0].Name) + require.Len(t, f.Entities[0].Resolvers, 1) + require.Equal(t, "Hello", f.Entities[1].Name) + require.Empty(t, f.Entities[1].Resolvers) + require.Equal(t, "World", f.Entities[2].Name) + require.Empty(t, f.Entities[2].Resolvers) data, err := codegen.BuildData(cfg) if err != nil { diff --git a/plugin/federation/testdata/federation2/federation2.graphql b/plugin/federation/testdata/federation2/federation2.graphql index 875a9b6289..8de6542da0 100644 --- a/plugin/federation/testdata/federation2/federation2.graphql +++ b/plugin/federation/testdata/federation2/federation2.graphql @@ -10,6 +10,16 @@ type Hello @key(fields:"name", resolvable: false) { name: String! } +type World @key(fields: "foo bar", resolvable: false) { + foo: String! + bar: Int! +} + +extend type ExternalExtension @key(fields: " upc ") { + upc: String! + reviews: [Hello] +} + type CustomQuery { hello: Hello! } diff --git a/plugin/federation/testdata/federation2/generated/federation.go b/plugin/federation/testdata/federation2/generated/federation.go index 38e956b7c8..813fc0df1c 100644 --- a/plugin/federation/testdata/federation2/generated/federation.go +++ b/plugin/federation/testdata/federation2/generated/federation.go @@ -80,21 +80,21 @@ func (ec *executionContext) __resolve_entities(ctx context.Context, representati }() switch typeName { - case "Hello": - resolverName, err := entityResolverNameForHello(ctx, rep) + case "ExternalExtension": + resolverName, err := entityResolverNameForExternalExtension(ctx, rep) if err != nil { - return fmt.Errorf(`finding resolver for Entity "Hello": %w`, err) + return fmt.Errorf(`finding resolver for Entity "ExternalExtension": %w`, err) } switch resolverName { - case "findHelloByName": - id0, err := ec.unmarshalNString2string(ctx, rep["name"]) + case "findExternalExtensionByUpc": + id0, err := ec.unmarshalNString2string(ctx, rep["upc"]) if err != nil { - return fmt.Errorf(`unmarshalling param 0 for findHelloByName(): %w`, err) + return fmt.Errorf(`unmarshalling param 0 for findExternalExtensionByUpc(): %w`, err) } - entity, err := ec.resolvers.Entity().FindHelloByName(ctx, id0) + entity, err := ec.resolvers.Entity().FindExternalExtensionByUpc(ctx, id0) if err != nil { - return fmt.Errorf(`resolving Entity "Hello": %w`, err) + return fmt.Errorf(`resolving Entity "ExternalExtension": %w`, err) } list[idx[i]] = entity @@ -169,7 +169,7 @@ func (ec *executionContext) __resolve_entities(ctx context.Context, representati } } -func entityResolverNameForHello(ctx context.Context, rep map[string]interface{}) (string, error) { +func entityResolverNameForExternalExtension(ctx context.Context, rep map[string]interface{}) (string, error) { for { var ( m map[string]interface{} @@ -178,10 +178,10 @@ func entityResolverNameForHello(ctx context.Context, rep map[string]interface{}) ) _ = val m = rep - if _, ok = m["name"]; !ok { + if _, ok = m["upc"]; !ok { break } - return "findHelloByName", nil + return "findExternalExtensionByUpc", nil } - return "", fmt.Errorf("%w for Hello", ErrTypeNotFound) + return "", fmt.Errorf("%w for ExternalExtension", ErrTypeNotFound) } diff --git a/plugin/modelgen/out_struct_pointers/generated.go b/plugin/modelgen/out_struct_pointers/generated.go index 51f1034c1d..7b414f6dd5 100644 --- a/plugin/modelgen/out_struct_pointers/generated.go +++ b/plugin/modelgen/out_struct_pointers/generated.go @@ -53,6 +53,10 @@ type UnionWithDescription interface { IsUnionWithDescription() } +type X interface { + IsX() +} + type CDImplemented struct { A string `json:"a" database:"CDImplementeda"` B int `json:"b" database:"CDImplementedb"` @@ -163,6 +167,13 @@ type TypeWithDescription struct { func (TypeWithDescription) IsUnionWithDescription() {} +type Xer struct { + Id string `json:"Id" database:"XerId"` + Name string `json:"Name" database:"XerName"` +} + +func (Xer) IsX() {} + type FooBarr struct { Name string `json:"name" database:"_Foo_Barrname"` } diff --git a/plugin/resolvergen/testdata/filetemplate/out/schema.custom.go b/plugin/resolvergen/testdata/filetemplate/out/schema.custom.go index d7917ee998..7b08f087fa 100644 --- a/plugin/resolvergen/testdata/filetemplate/out/schema.custom.go +++ b/plugin/resolvergen/testdata/filetemplate/out/schema.custom.go @@ -2,7 +2,7 @@ package customresolver // This file will be automatically regenerated based on the schema, any resolver implementations // will be copied through when generating and any unknown code will be moved to the end. -// Code generated by github.com/99designs/gqlgen version v0.17.20-dev DO NOT EDIT. +// Code generated by github.com/99designs/gqlgen version v0.17.21-dev DO NOT EDIT. import ( "context" @@ -36,9 +36,9 @@ type resolverCustomResolverType struct{ *CustomResolverType } // !!! WARNING !!! // The code below was going to be deleted when updating resolvers. It has been copied here so you have // one last chance to move it out of harms way if you want. There are two reasons this happens: -// - When renaming or deleting a resolver the old code will be put in here. You can safely delete -// it when you're done. -// - You have helper methods in this file. Move them out to keep these resolver files clean. +// - When renaming or deleting a resolver the old code will be put in here. You can safely delete +// it when you're done. +// - You have helper methods in this file. Move them out to keep these resolver files clean. func AUserHelperFunction() { // AUserHelperFunction implementation } diff --git a/plugin/resolvergen/testdata/followschema/out/schema.resolvers.go b/plugin/resolvergen/testdata/followschema/out/schema.resolvers.go index d7917ee998..7b08f087fa 100644 --- a/plugin/resolvergen/testdata/followschema/out/schema.resolvers.go +++ b/plugin/resolvergen/testdata/followschema/out/schema.resolvers.go @@ -2,7 +2,7 @@ package customresolver // This file will be automatically regenerated based on the schema, any resolver implementations // will be copied through when generating and any unknown code will be moved to the end. -// Code generated by github.com/99designs/gqlgen version v0.17.20-dev DO NOT EDIT. +// Code generated by github.com/99designs/gqlgen version v0.17.21-dev DO NOT EDIT. import ( "context" @@ -36,9 +36,9 @@ type resolverCustomResolverType struct{ *CustomResolverType } // !!! WARNING !!! // The code below was going to be deleted when updating resolvers. It has been copied here so you have // one last chance to move it out of harms way if you want. There are two reasons this happens: -// - When renaming or deleting a resolver the old code will be put in here. You can safely delete -// it when you're done. -// - You have helper methods in this file. Move them out to keep these resolver files clean. +// - When renaming or deleting a resolver the old code will be put in here. You can safely delete +// it when you're done. +// - You have helper methods in this file. Move them out to keep these resolver files clean. func AUserHelperFunction() { // AUserHelperFunction implementation }