-
Notifications
You must be signed in to change notification settings - Fork 172
feat: add SubscriptionOnStartHandler #2059
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: topic/streams-v1
Are you sure you want to change the base?
feat: add SubscriptionOnStartHandler #2059
Conversation
…fields only where needed
…subscription and not going to other client that subscribed the same subscription
Co-authored-by: Ludwig Bedacht <[email protected]>
… change behaviour based on error type
router/core/subscriptions_modules.go
Outdated
|
||
type SubscriptionOnStartHookContext interface { | ||
// the request context | ||
RequestContext() RequestContext |
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.
Whatever we expose here needs to be tested. E.g., the ResponseWriter wouldn't work in subscriptions. Ideally, in our first iteration, we are purely use case driven and expose only functionality and data that is necessary to enable these.
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.
Mmm I see, so you would prefer an ad-hoc version (I'm thinking of a SubscriptionRequestContext
) with only things that make sense?
router/core/subscriptions_modules.go
Outdated
// Check if the error is a StreamHookError and should close the connection | ||
var streamHookErr *StreamHookError | ||
close := false | ||
if errors.As(err, &streamHookErr) { |
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 think any error should close the subscription. Why do we want to do it selectively?
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.
The idea was that this would allow the customer to customize the client's behaviour: if an error with closeConnection set to false was returned, the client could have just write something to the UI (like "no initial data found, waiting for the first real data") and then, when a message with the data arrived, the client could just read it.
In the real world, the client will simply close the subscription on the first error it receives, so this behaviour makes no sense :(
I'll remove it to avoid complexity and confusion.
@@ -88,31 +87,44 @@ func (p *ProviderAdapter) topicPoller(ctx context.Context, client *kgo.Client, u | |||
r := iter.Next() | |||
|
|||
p.logger.Debug("subscription update", zap.String("topic", r.Topic), zap.ByteString("data", r.Value)) | |||
updater.Update(r.Value) | |||
headers := make(map[string][]byte) |
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.
Do we already have the requirement for this? If not, I'd reduce the implementation to the absolute minimum while allowing for further extensions in the future.
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.
Yes, one of the requirements is to read/write kafka headers.
func (c *EngineDataSourceFactory) ResolveDataSourceSubscription() (datasource.SubscriptionDataSource, error) { | ||
return datasource.NewPubSubSubscriptionDataSource[*SubscriptionEventConfiguration]( | ||
c.RedisAdapter, | ||
func(ctx *resolve.Context, input []byte, xxh *xxhash.Digest) 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.
Why did we move this outside of the provider specific 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.
Because the hook is managed inside the PubSubSubscriptionDataSource. Having it in the datasource allows the implemented providers (and the future ones!) to just return the PubSubSubscriptionDataSource to implements the hooks.
An alternative solution that I thought was to embed this struct inside another one that was implementing the hooks, but looked like an overly complicated solution.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores
Depends on engine PR: wundergraph/graphql-go-tools#1243
How to use the new hook?
The hook implemented in this PR is the
SubscriptionOnStartHandler
as defined here:cosmo/router/core/subscriptions_modules.go
Line 107 in ceac255
Example
Query
On the demo graph, we will create a module that add some logic to the following query
Module code that implement the new hook
As an example, this module will emit a message if the subscription is started with employeeID == 1.
Checklist