-
Notifications
You must be signed in to change notification settings - Fork 62
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
Add Context Support for GraphQL Resolvers #417
Conversation
da80ff8
to
ef5fb9b
Compare
Codecov Report
@@ Coverage Diff @@
## master #417 +/- ##
============================================
- Coverage 94.84% 94.63% -0.22%
- Complexity 278 285 +7
============================================
Files 49 50 +1
Lines 2912 2983 +71
Branches 1347 1390 +43
============================================
+ Hits 2762 2823 +61
- Misses 132 138 +6
- Partials 18 22 +4
Continue to review full report at Codecov.
|
ballerina/annotations.bal
Outdated
public type GraphqlServiceConfig record {| | ||
int maxQueryDepth?; | ||
ListenerAuthConfig[] auth?; | ||
ContextInit contextInit?; |
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.
What if we assign a default value for this ? Something like
isolated function initDefaultContext(http:Request request, http:RequestContext requestContext) returns graphql:Context|error {
return graphql:Context();
}
That way it is explicit that we create a default one if nothing is given.
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 already have a function for this:
isolated function initContext(http:Request request, http:RequestContext requestContext) returns Context|error { |
I'll rename this as initDefaultContext
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.
@ThisaruGuruge @shafreenAnfar What do you think about the field name contextInit
? I prefer this to be as Context context
, and passing a function pointer which creates a Context
for me. But I understand that it is not possible with Ballerina way of handling types. So, what do you think of having this as ContextInit context
?
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.
At first, I went through the same thought process. But then went with contextInit
because this isn't actually a context
, but a function that initializes a context.
IMHO, this makes it easy for the developers to understand what it is. They have to provide a ContextInit
(which is a function pointer), not a Context
(which is an object).
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. Agree with the clarification.
Purpose
Add GraphQL context support that can be used to share metadata between the resolvers.
Fixes: #1906
Fixes: #1953
Examples
Developers can provide a function to initialize the context which must be an isolated function, and must have two input parameters:
http:Request
andhttp:RequestContext
. This function must have the return type ofgraphql:Context|error
.Inside this function, the
graphql:Context
object can be created and the developer can add attributes to the context as key-value pairs. The key is always a string, the value can be avalue:Clonable|isolated object {}
. Simply put, this can be a primitive value, anyreadonly
value, or an isolated function pointer, or an isolated object. The context object has three functions,add
,get
, andremove
.Then this function can be provided as a service config field. Using this function, a context will be created per request. When needed, the context can be defined as the first parameter of a resolver (resource/remote) function. Following is an example:
Checklist