-
Notifications
You must be signed in to change notification settings - Fork 530
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 support for brokered subscriptions #776
base: main
Are you sure you want to change the base?
Conversation
Move storage of subscriptions and documents behind an abstraction layer. Pass Absinthe context to `publish_subscription`.
Hey @jscheid I'm really excited to see this PR, I've wanted to do this exact thing for a while. Are you able to link to what an alternative store would look like? I think that will help me evaluate the context related changes better. We have a similar a sort of hacked together internal setup for doing subscriptions over SQS, stored in postgres, and so on my end I'll throw together a postgres version of the store so that we can make sure it all works in several scenarios. |
Hi @benwilson512, there's a Redix implementation in this PR, is that what you mean? |
@@ -0,0 +1,67 @@ | |||
defmodule Absinthe.Subscription.RedixStore do |
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.
As a minor note, while I definitely appreciate this as an example, I definitely don't want to have Absinthe ship with this built in. It couples Absinthe too closely to a specific redis library. I'd be more than happy however to promote an external package that contained this code however.
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.
That's fine, as I said, it's only an illustration.
@jscheid I'm not seeing how that PR uses the new context argument passed to publish_subscription. |
@benwilson512 this PR doesn't use the new context argument. It's meant for use by alternate pubsub implementations that want to scope the topics by some user information. No such alternate implementations are included -- the purpose of the PR is to make those possible, not add them. Does that make sense, and can you see the need for providing |
Not yet I guess. The topic when generated by the subscription setup function has access to the context, and can embed any data it wants at that point in time. Without a concrete example to work off I'm hesitant to change this public behaviour function. |
Ok, maybe I'm missing something. Essentially the problem I'm trying to solve is this: If you take SQS as an example, the client wouldn't subscribe to one queue per subscription, but instead use a single queue for all subscriptions (because that is both more efficient in terms of HTTP traffic and cheaper in terms of AWS pricing.) With the context information, You make it sound like there may be a different way to do this but I can't see it. What exactly are you referring to by "subscription setup function"? |
Oh, I think I see. You mean But wouldn't that mean that topics couldn't be shared by subscriptions anymore? For instance, if you have some global, public state, would you want to use a single topic for it rather than generating a different topic per subscriber? |
Hey @benwilson512 , no rush on this one but just wanted to check in to see if you were waiting for anything from me? As I said, I'm happy to make changes but not entirely sure yet what you had in mind. |
Hey @jscheid I am not waiting on you. I completely understand now why you're passing in the context. The delay is most around my wish to re-evaluate slightly how document execution and publication happens so as to make it easier to set some of those values. Your solution may well be the best option, but now that I'm aware of the problem I'm thinking through some other available options for solving it. |
Thanks for the update @benwilson512. Sounds good, take your time! |
Am I right in understanding that this is an attempt to enable a subscription to "switch" transports? ie: it starts as an HTTP request and then is delivered via message queue? I have a few questions:
|
That's correct, the point of this PR is to enable updates that are delivered through a different transport mechanism than other GraphQL requests and responses.
|
@binaryseed there's also some deliberation on the subscription ID delivery in the sibling PR description. |
Wondering the state of this PR? Those of us looking to use Apollo and Absinthe in production would love to use it (and an example if someone has the time!) |
Hi @derekbrown to be clear, you can use apollo and Absinthe in production today via https://github.com/absinthe-graphql/absinthe-socket/tree/master/packages/socket-apollo-link This PR is about enable subscriptions over something like SQS |
We're using Apollo on iOS. |
What would be the transport mechanism for subscriptions? If you can do websocket / HTTP you also don't need these changes. If you're looking for APNS then this might be more relevant. |
We're using websockets, but (unless I'm missing something which may very well be the case!) the Apollo operation message shape requires a transform. Something akin to this acts as a bandaid, but far from ideal. |
It goes beyond the operation shape. Apollo websocket clients use a websocket frame protocol that is specific to Apollo server. This PR isn't going to help with either the operation shape NOR the websocket frame protocol. Absinthe as a library is already 100% transport agnostic. I'd consider making a forum post about connecting to Absinthe via apollo. The brokered subscription concept has more to do with storing documents that aren't connected to specific processes. If you're using websockets at all you can use the existing process oriented document storage. Absinthe itself isn't the bottleneck here, and this PR will not help you. I'm not an iOS developer so I'm not 100% sure what options exist out there, but you either need to use the library you linked to make Phoenix talk Apollo's frame format, or you need to find a plugin for the apollo ios library that enables it to talk the phoenix channel frame format. |
This is work in progress, for now I'm putting it up here only to start a conversation.
The aim of this change is to enable use of message brokers for delivering subscription updates. I'm planning to use it with AWT IoT, but it should pave the way for using any broker, including other MQTT/AMQP/etc brokers and commercial offerings such as Pusher or Ably.
This requires two changes outlined below.
Subscription Documents Storage Abstraction
The Registry works well when the lifetime of a subscription is tied to an underlying WebSocket, but doesn't for the case where subscriptions are "out of band" in the sense that there is no direct connection between client and server.
Specifically, when a node goes down it takes all the documents in its Registry with it, which isn't a problem for WebSockets as the client will be forced to reconnect anyway. It is a problem when a broker is used since there's no easy way to make the client aware of the need to set up the subscription anew.
There's also a wrinkle in that the Registry implementation is tied to the HTTP request pid, which works well only when both the subscription setup and update delivery are handled in the same request.
All of this is why the major change here is to move storage behind an abstraction so that it can be swapped out. I've included an alternate storage layer based on Redix for illustrative purposes; it wouldn't necessarily have to stay for the final PR, although I'd be happy to donate it. (It's incomplete though, I'm planning to add expiry/keep-alive in order to preempt misbehaved clients. This shouldn't affect the interface though.)
Context During Publishing
The other change is passing the Absinthe context to
publish_subscription
. This is useful becausepublish_subscription
can use it to construct an (external) topic that includes some kind of user indentification, which in turn enables access control. Without being able to scope topics by user, authorizing access is much less convenient and efficient.absinthe_plug changes
There's also a change necessary to
absinthe_plug
, which currently assumes that subscriptions are updated through a WebSocket. I'll open a separate PR for that.What do you think about these changes? If you agree with them in general I'm happy to spend some time on polish and adding a few tests. As far as naming goes I'm not attached to the names I've picked, feel free to suggest alternatives.
One open question is how to handle backward compatibility, as you can see I've put in a transparent fallback for the first change but not for the second. Happy to tweak that for consistency, one way or the other.