-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
feat: create @requires integration that functions like a normal resolver #3292
feat: create @requires integration that functions like a normal resolver #3292
Conversation
The "explicit_requires" flag current generates the @requires resolver inside the execution context. It also doesn't use the normal resolver resolution process and adds a custom implementation that is always called, even if the field isn't requested. The new "computed_requires" flag looks for @requires directives and mutates the schema to add dynamic field arguments and directives that are used internally by gqlgen. These additional directives (@entityReference and @populateFromRepresentations) are runtime directives that "computed_requires" adds implementations for that enable us to fetch the correct data from the representations array and populated it into the dynamic argument for the @requires field.
@@ -66,3 +67,5 @@ models: | |||
- github.com/99designs/gqlgen/graphql.Int | |||
- github.com/99designs/gqlgen/graphql.Int64 | |||
- github.com/99designs/gqlgen/graphql.Int32 | |||
|
|||
omit_complexity: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simply reduces generated code for unused features. Not a material change.
@@ -11,23 +11,62 @@ import ( | |||
"github.com/99designs/gqlgen/_examples/federation/reviews/graph/model" | |||
) | |||
|
|||
// ManufacturerID is the resolver for the manufacturerID field. | |||
func (r *productResolver) ManufacturerID(ctx context.Context, obj *model.Product, federationRequires map[string]interface{}) (*string, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New resolver function automatically generated and includes the @requires
fields in the federationRequires
map.
@@ -97,7 +121,7 @@ func BuildData(cfg *config.Config, plugins ...any) (*Data, error) { | |||
|
|||
dataDirectives := make(map[string]*Directive) | |||
for name, d := range b.Directives { | |||
if !d.Builtin { | |||
if !d.SkipRuntime { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed this — BuiltIn
didn't accurately describe the behavior.
Name: "excludeDirective", | ||
Args: nil, | ||
DirectiveConfig: config.DirectiveConfig{ | ||
SkipRuntime: false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Used composition here in the underlying struct so I needed to update the instantiation.
Name string | ||
Args []*FieldArgument | ||
|
||
config.DirectiveConfig |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There can be multiple config properties, I thought it was easier to just pass this through. I can change this to individual parameters and map them onto the object if you'd prefer.
|
||
func (d *Directive) CallPath() string { | ||
if d.IsBuiltIn() { | ||
return "builtInDirective" + d.CallName() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We now have two types of directives: user provided and "built in" where the implementation is statically provided by the plug in.
For builtIn
directives we call a static function vs an instance function.
a01332f
to
2dcfdb3
Compare
// - 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 | ||
} | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is causing one test to fail
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reverted it but this change gets made automatically when I run go test ./...
. I think we need to revert #3243.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, crud. huh. Either that, or rethink how we do testing for that behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah potentially. Either way, I would revert for now, we can fix it later. I don't want master
to be in a broken state. We should also update CI to ensure the git state isn't dirty after running the tests.
// - 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 | ||
} | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is causing one test to fail
2dcfdb3
to
0508cef
Compare
The
explicit_requires
flag currently generates the@requires
resolver inside the execution context. It also doesn't use the normal resolver resolution process and adds a custom implementation that is always called, even if the field isn't requested.The new
computed_requires
flag looks for@requires
directives and mutates the schema to add dynamic field arguments and directives that are used internally bygqlgen
.These additional directives (
@entityReference
and@populateFromRepresentations
) are runtime directives that "computed_requires" adds implementations for that enable us to fetch the correct data from the representations array and populated it into the dynamic argument for the@requires
field.The new implementation still isn't typesafe and generates the required fields into a
map[string]any
. This is the same as the existingexplicit_requires
implementation.We will follow up with type safety in a future PR. To support type safety we need to generate types and marshaling logic for GraphQL operations (vs just schemas like we do today) which is a large lift.
I have: