-
Notifications
You must be signed in to change notification settings - Fork 205
feat(router): provide field arguments in operation context #2385
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
base: main
Are you sure you want to change the base?
Changes from all commits
f6d76f5
ec45e9e
c9cc1e6
9bf3562
8477e1a
c5d995f
4800a8c
130bd22
9f0c25f
08ca36e
b28d9a7
a63ff06
d6fcf63
cdca088
b9cbf97
a14b7c4
33d8dfc
1c1490e
0766e76
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,58 @@ | ||
| package core | ||
|
|
||
| import "github.com/wundergraph/astjson" | ||
|
|
||
| // fieldArgs is a collection of field arguments with their names | ||
| // as keys and their corresponding values. | ||
| type fieldArgs map[string]*astjson.Value | ||
|
|
||
| // Arguments allow access to GraphQL field arguments used by clients. | ||
| type Arguments struct { | ||
| // data holds a map which contains all field arguments | ||
| // for any given field of an operation. | ||
| data map[string]fieldArgs | ||
| } | ||
|
|
||
| // Get will return the value of argument a from field f. | ||
| // | ||
| // To access an argument of a root level field, you need to pass the | ||
| // response key of the field as the first argument to Get and the name of the argument | ||
| // as the second argument, e.g. Get("rootfield_name", "argument_name") . | ||
| // | ||
| // The response key is the alias if present, otherwise the field name. | ||
| // For aliased fields like "myAlias: user(id: 1)", use the alias "myAlias" in the path. | ||
| // | ||
| // The field path uses dot notation for nested fields. | ||
| // For example you can access arg1 on field2 on the operation | ||
| // | ||
| // subscription { | ||
| // mySub(arg1: "val1", arg2: "val2") { | ||
| // field1 | ||
| // field2(arg1: "val3", arg2: "val4") | ||
| // } | ||
| // } | ||
| // | ||
| // You need to call Get("mySub.field2", "arg1") . | ||
| // | ||
| // For aliased fields: | ||
| // | ||
| // query { | ||
| // a: user(id: "1") { name } | ||
| // b: user(id: "2") { name } | ||
| // } | ||
| // | ||
| // You need to call Get("a", "id") or Get("b", "id") respectively. | ||
| // | ||
| // If fa is nil, or f or a cannot be found, nil is returned. | ||
| func (fa *Arguments) Get(f string, a string) *astjson.Value { | ||
| if fa == nil || fa.data == nil { | ||
| return nil | ||
| } | ||
|
|
||
| args, found := fa.data[f] | ||
| if !found { | ||
| return nil | ||
| } | ||
|
|
||
| return args[a] | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -482,16 +482,16 @@ type OperationContext interface { | |
| Hash() uint64 | ||
| // Content is the content of the operation | ||
| Content() string | ||
| // Variables is the variables of the operation | ||
| // Arguments allow access to GraphQL operation field arguments. | ||
| Arguments() *Arguments | ||
| // Variables allow access to GraphQL operation variables. | ||
| Variables() *astjson.Value | ||
| // ClientInfo returns information about the client that initiated this operation | ||
| ClientInfo() ClientInfo | ||
|
|
||
| // Sha256Hash returns the SHA256 hash of the original operation | ||
| // It is important to note that this hash is not calculated just because this method has been called | ||
| // and is only calculated based on other existing logic (such as if sha256Hash is used in expressions) | ||
| Sha256Hash() string | ||
|
|
||
| // QueryPlanStats returns some statistics about the query plan for the operation | ||
| // if called too early in request chain, it may be inaccurate for modules, using | ||
| // in Middleware is recommended | ||
|
|
@@ -524,7 +524,11 @@ type operationContext struct { | |
| // RawContent is the raw content of the operation | ||
| rawContent string | ||
| // Content is the normalized content of the operation | ||
| content string | ||
| content string | ||
| // fieldArguments are the arguments of the operation. | ||
| // These are not mapped by default, only when certain custom modules require them. | ||
| fieldArguments Arguments | ||
| // variables are the variables of the operation | ||
| variables *astjson.Value | ||
| files []*httpclient.FileUpload | ||
| clientInfo *ClientInfo | ||
|
|
@@ -560,6 +564,10 @@ func (o *operationContext) Variables() *astjson.Value { | |
| return o.variables | ||
| } | ||
|
|
||
| func (o *operationContext) Arguments() *Arguments { | ||
| return &o.fieldArguments | ||
| } | ||
|
Comment on lines
+567
to
+569
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure if this is a good idea. An operation can be huge and even if you only register EnterArgument the visitor will still visit every node in the document. It might be better to have some kind of lazy walking here to only request the arguments when you really need them and then store them in the field arguments instead of walking on every request. Would that make sense?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The problem I have with lazy walking the document (i.e. first time a hook accesses field arguments) is that I have to remember the AST, make the walk thread safe (prevent two hooks from walking simultaneously) and remember the result globally for all hooks of that subscription query. It introduces more complex mechanisms compared to the current implementation. The upside of that is that the ast walking is only performed when really needed, and it's not done during query normalization. But that's only an advantage if walking field arguments is actually expensive. I'll get some big queries and see how it affects performance, so we can be sure lazy walking is a good tradeoff.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I crunched some numbers. There is a benchmark in sec/op was between 0.4 and 1.3ms slower with field argument mappings (1.2 to 2% slower). The operations I tested did not contain many field arguments but lots of fields. This made the walker walk a lot of fields but it did not often enter my visitor to create the map for field arguments. I suppose on heavy operations with lots of field arguments we could see an increase in allocations and processing time. But in order to feel that it would involve lots of field arguments and the question is how realistic that is. I assume field arguments are only used for certain fields here and there. So all in all the performance penalty is not too heavy I think. Given the fact that the field arguments walker only runs when Cosmo Streams Custom Modules are registered I think it's okay to let stay as is? To top it off I will also introduce an option to disable field argument mapping explicitely via config, even if these Custom Modules are registered. What do you think @Noroth ? |
||
|
|
||
| func (o *operationContext) Files() []*httpclient.FileUpload { | ||
| return o.files | ||
| } | ||
|
|
||
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.
Add some unit tests for this type. Check if you can provoke panics here.