-
-
Notifications
You must be signed in to change notification settings - Fork 901
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
[GraphQL] Mercure Subscription Support #3321
[GraphQL] Mercure Subscription Support #3321
Conversation
e590ae1
to
6ca6c2c
Compare
@lukasluecke @mahmoodbazdar @CvekCoding if you could have a look at this PR? |
@alanpoulain Thanks for your effort! I will check it out in the next days 🙂 |
@alanpoulain wow, great. I'll check this on NY vacation (at least I'll try:)). Great thanks for this update - sounds great. As far as I know it's a first graphql subscription implementation in php world. |
@alanpoulain Looks good so far - I'll see if I can find the time this week to try it out (i.e. build a working client-side integration utilizing this). |
foreach ($subscriptions as [$subscriptionId, $subscriptionFields, $subscriptionResult]) { | ||
$resolverContext = ['fields' => $subscriptionFields, 'is_collection' => false, 'is_mutation' => false, 'is_subscription' => true]; | ||
|
||
$data = ($this->serializeStage)($object, $resourceClass, 'update', $resolverContext); |
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.
@alanpoulain This means that the subscription data will get serialized using the update
context, right? Should there be a way to override it for the subscription payload specifically?
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 it would be a bad idea to have a subscription
context. Subscriptions are here to send the updated data like you would receive it if you made a mutation and read the received data.
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 might be right 😉 what about the case where there is no update
mutation on the resource? (It can still be changed from other parts of the API, right?)
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.
Would you see the item_query
context instead then?
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.
@alanpoulain Sorry for the late reply - I think trying update
first and falling back to item_query
would be good, wdyt?
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.
Actually, if you don't have an update
operation, the subscription fields are not generated, so it makes sense to me to keep it like this.
foreach ($payloads as [$subscriptionId, $data]) { | ||
$updates[] = new Update( | ||
$this->graphQlMercureSubscriptionUriGenerator->generateMercureTopicUrl($subscriptionId), | ||
$this->serializer->serialize($data, key($this->formats)), |
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.
@alanpoulain Wouldn't this serialize the messages using the same format (defaults to JSON-LD I believe?) as the normal mercure updates? I think it would make sense to use GraphQL here, no?
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 normalized data is sent as JSON like the GraphQL endpoint would do. Maybe I should use json
instead of the key of formats, WDYT?
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've changed it to mimic how the endpoint is doing it:
return new JsonResponse($executionResult->toArray($this->debug)); |
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.
LGTM 👍
9197b5c
to
11bfab4
Compare
|
||
/** | ||
* @param array<string, string[]|string> $formats | ||
*/ | ||
public function __construct(ResourceClassResolverInterface $resourceClassResolver, IriConverterInterface $iriConverter, ResourceMetadataFactoryInterface $resourceMetadataFactory, SerializerInterface $serializer, array $formats, MessageBusInterface $messageBus = null, callable $publisher = null, ExpressionLanguage $expressionLanguage = null) | ||
public function __construct(ResourceClassResolverInterface $resourceClassResolver, IriConverterInterface $iriConverter, ResourceMetadataFactoryInterface $resourceMetadataFactory, SerializerInterface $serializer, array $formats, MessageBusInterface $messageBus = null, callable $publisher = null, ?GraphQlSubscriptionManagerInterface $graphQlSubscriptionManager = null, ?GraphQlMercureSubscriptionUriGeneratorInterface $graphQlMercureSubscriptionUriGenerator = null, ExpressionLanguage $expressionLanguage = null) |
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 position of $expressionLanguage
should be kept to prevent a BC break.
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 expression language is not defined in the configuration so it would mean doing dirty tricks.
Since it's experimental, I thought it would be better to not prevent the BC.
* | ||
* @return Update[] | ||
*/ | ||
private function getGraphQlSubscriptionUpdates($object, array $targets, string $type): array |
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.
Couldn't we move the GraphQL-specific code in a decorator?
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.
It would mean introducing methods to get the created/updated/deleted items and to duplicate the code in postFlush
and a little from publishUpdate
. Is it worth it?
$resourceMetadata = $this->resourceMetadataFactory->create($resourceClass); | ||
|
||
if ($subscriptionId && $resourceMetadata->getAttribute('mercure', false)) { | ||
if (!$this->mercureSubscriptionUriGenerator) { |
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.
Shouldn't this exception be thrown in the constructor?
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've thought the subscription system to be independent of Mercure. Here, we need to see if the resource has a mercure
attribute and, if it's the case, throw the exception if the bundle is not required. We cannot do this in the constructor.
private $requestContext; | ||
private $hub; | ||
|
||
public function __construct(RequestContext $requestContext, string $hub) |
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.
Maybe could we inject the values of the router.request_context.*
parameters instead (https://symfonycasts.com/screencast/mailer/route-context#setting-router-request-context). It will avoid a coupling with Symfony.
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.
It would be nice but these parameters are not dynamically updated whereas it's the case for the service.
public function generateMercureTopicUrl(string $subscriptionId): string | ||
{ | ||
if (('' === $scheme = $this->requestContext->getScheme()) || ('' === $host = $this->requestContext->getHost())) { | ||
throw new \RuntimeException('Cannot generate topic URL from request 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.
I suggest to use a default value if the context isn't available, such as https://api-platform.com/subsriptions/
. IRIs don't have to exist.
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.
OK, it's an edge case anyway.
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.
Actually, maybe should we always use this default, and let the user pass another namespace if required, instead of relying on the router.
* | ||
* @author Alan Poulain <contact@alanpoulain.eu> | ||
*/ | ||
interface MercureSubscriptionUriGeneratorInterface |
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.
MercureSubscriptionIriGeneratorInterface
for consistency (we use IRI everywhere).
|
||
public function generateMercureTopicUrl(string $subscriptionId): string; | ||
|
||
public function generateMercureUrl(string $subscriptionId): string; |
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 suggest to simplify this interface and to provide only a single method getTopic(string $subscriptionId)
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 can remove the generateMercureTopicIri
but I need two methods: one for getting the topic IRI and one for getting the Mercure URL (with the topic in the query).
* | ||
* @author Alan Poulain <contact@alanpoulain.eu> | ||
*/ | ||
final class SubscriptionIdentifierGenerator implements SubscriptionIdentifierGeneratorInterface |
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.
Are this class and the related really necessary? Cannot we merge it with the topic generator, and use the topic as ID everywhere?
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.
This class is necessary because I use a random string for the subscriptions. I can remove it if I use a hash instead. Maybe using a SHA-256 hash would be a better idea like we discussed since Apollo is doing it for its persisted queries.
Also I cannot add it to the generator since it's not related to Mercure.
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.
Using a hash sounds like a good idea!
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.
Done. I think keeping the service is a good idea since it allows to change how the identifier is generated if needed.
11bfab4
to
b49b30c
Compare
cc9da4f
to
51a0581
Compare
@alanpoulain I have now implemented this in my project with a matching frontend client, and it works great! Thank you very much for your work! 🏆 I do have some questions / improvement requests for the future: |
Thank you for your feedback @lukasluecke! |
@alanpoulain Great! After some more testing I think it would probably be good to get #3206 (or something comparable) in together with this, otherwise things might get confusing 🙈 Could you maybe take a look at this? |
Can't it be done later? |
Of course, it's not strictly required / related to the changes directly 🙂 I'll try to look into it in the next days 👍 |
51a0581
to
095ea9b
Compare
Hey guys, is this now widely available? Doesn't seem to work on 2.5.4 (I get 'Internal Error: Subscription type is not defined the Schema' with relay). Is this my fault or it's not yet implemented in this version? Thanks. |
Not yet. In 2.6. |
@alanpoulain Quick question - is there an easy solution to make Mercure use the GraphQL serializer & format for the updates it sends? Would make it a lot easier to properly integrate clientside, instead of dealing with the JSONLD and the differences. |
It shouldn't use JSON-LD by default but the GraphQL format. See this test: core/features/graphql/subscription.feature Lines 196 to 207 in 2c87089
|
@alanpoulain Sorry, I was a little confused when posting. It's not JSONLD, but it's also not what usually is available in a GraphQL response. I'm using a normalized cache in the frontend, and in order to match the subscription results I need the I fixed it temporarily in $rootObject = array_keys($data)[0];
$data[$rootObject]['__typename'] = $this->resourceMetadataFactory->create($resourceClass)->getShortName();
$data[$rootObject]['id'] = $iri; |
I understand for the For the ID and not the IRI, I don't remember why it's like this (and I even wrote it in the tests), but it seems an issue to me indeed. Maybe the context is not given correctly to the stage. |
hey @alanpoulain thanks for your great work to integrate graphql subscription Schema is not configured for subscriptions. I want to get notification whenever a new one is created so I have a ngOnInit(){
this.apollo.subscribe({
query: gql`
subscription{
newNotification{
title
content
}
}`,
fetchPolicy: "no-cache"
})
.subscribe(res => console.log(res));
} why it doesn't know the subscription schema ? |
You need to use a custom link that knows how to handle the mercure subscription. I can give you an example of what I'm using tomorrow, but it's for urql, not Apollo. |
@lukasluecke ok thanks for your response |
Hi, @lukasluecke @SlimenTN, i'm trying using apollo client with a splitLink to connect to api-platform subscriptions, but i didn't succeeded and didn't find any good exemple code. Have some of you found a "good" way to implement this ? Thanks |
Hi @xleliberty actually still waiting for @lukasluecke's example, because as I mentioned there isn't a lot mentioned about the subscription in apiplatform's documentation. |
Ok Thanks Slimen, You're right, i feel too like we are missing a bit of informations about subscription consumption on the JS side. Once we have more info bits, i'll try to add a little doc PR for this. @alanpoulain , maybe you have some hints about js consumption ? Thanks |
@SlimenTN @xleliberty Sorry, totally forgot about this! You can see my example at https://gist.github.com/lukasluecke/0c8e2c12a955198926779a8691874c03. |
Hi @lukasluecke , thanks for your input ! |
@SlimenTN, @xleliberty with Apollo, you can use it like a normal Query, something like this:
|
Anyone tried implementing this with react-relay on the frontend? |
GraphQL Subscriptions in API Platform will use Mercure as the underlying protocol (for the first version) with this PR.
Only the update subscription is available. Maybe the delete subscription would be useful too but it could be done later.
A subscription can be done like this:
In the JSON response (in
mercureUrl
field), you will receive an URL like this:https://demo.mercure.rocks/hub?topic=http://example.com/subscriptions/fc806a60cca4e93544d555912ce8b549
This URL is used by the Mercure protocol to subscribe to updates by using Server-sent events:
To enable subscriptions for a resource, follow the related Mercure documentation. It mainly consists of adding this annotation:
You will only receive the update if the payload you have given when subscribing has changed.
To do so, a Symfony cache is used to store the subscriptions and their payloads:
api_platform.graphql.cache.subscription
.The best way to store the subscriptions is to use an adapter like Redis.
An Apollo transport like this one: https://github.com/CodeCommission/subscriptions-transport-sse could be written later in order to make it work seamlessly with Apollo.