-
Notifications
You must be signed in to change notification settings - Fork 44
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: Add GraphQL subscriptions #934
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #934 +/- ##
===========================================
+ Coverage 60.05% 60.29% +0.23%
===========================================
Files 159 164 +5
Lines 17532 17700 +168
===========================================
+ Hits 10529 10672 +143
- Misses 6066 6081 +15
- Partials 937 947 +10
|
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.
More comments to come, looks good in general
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.
praise: Was a really really nice cleanup Fred of the pub/sub stuff.
PR looks very good overall, but I've added a few small/localized comments which would be good to get sorted pre-merge
} | ||
|
||
for _, def := range doc.Definitions { | ||
switch node := def.(type) { | ||
case *ast.OperationDefinition: | ||
if node.Operation == "query" { | ||
switch node.Operation { |
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.
praise: Thanks for making this a switch
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.
you're welcome :)
@@ -66,6 +67,17 @@ var ( | |||
mapStore bool | |||
) | |||
|
|||
// Represents a query assigned to a particular transaction. | |||
type SubscriptionQuery struct { |
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.
praise: I really like this test setup, thank you for finding a nice solution for this :)
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.
Nice work! Fairly clean review, I like the subscription test util modification as well.
Relevant issue(s) Resolves sourcenetwork#724 Description This PR adds the functionalities related to GQL subscriptions. Subscriptions can happen through the HTTP API or using db.ExecQuery directly. It piggy backs on the previously implemented event system to listen to DB updates and then using a new Publisher, broadcasts the updates to the listening clients. The new Publisher is designed to allow the listener to close the channel hence why you'll notice many thread safety mechanics in the related methods.
Relevant issue(s)
Resolves #724
Description
This PR adds the functionalities related to GQL subscriptions. Subscriptions can happen through the HTTP API or using
db.ExecQuery
directly.It piggy backs on the previously implemented event system to listen to DB updates and then using a new
Publisher
, broadcasts the updates to the listening clients. The newPublisher
is designed to allow the listener to close the channel hence why you'll notice many thread safety mechanics in the related methods.Tasks
How has this been tested?
unit and integration tests
Specify the platform(s) on which this was tested: