Skip to content
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 subscriptions support to ktor server #1774

Merged
merged 14 commits into from
May 19, 2023

Conversation

thevietto
Copy link
Contributor

📝 Description

Out of the box subscriptions support to the ktor-server

I added an implementation for both legacy subscriptions-transport-ws and graphql-ws protocols. Latter is the default.

basic configuration example:

    install(GraphQL) {
        schema {
            subscriptions = listOf(
                ExampleSubscriptionService()
            )
        }
    }
    install(Routing) {
        graphQLSubscriptionsRoute()
    }
  • Docs updated
  • Example app updated

🔗 Related Issues

#1587
#1756

Sorry, something went wrong.

/**
* Default implementation of Apollo Subscription Lifecycle Events (No-op).
*/
open class DefaultKtorGraphQLLegacySubscriptionHooks : KtorGraphQLSubscriptionHooks

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably, it doesn't make sense to make the default implementation open

Copy link
Contributor Author

@thevietto thevietto May 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hate library classes that closes themselves from being extended (e.g if just need to add 1 line, not copying all of the code). Even though the default impl is empty, it may be convenient to declare MyImpl extends DefaultImpl

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since those hooks are generic and applicable to both graphql-ws and subscriptions-transport-ws lets drop the Legacy from the name. We can probably update KDoc to be more generic as well - yes you can use similar hooks in apollo-server but again those are pretty generic lifecycle events.

Suggested change
open class DefaultKtorGraphQLLegacySubscriptionHooks : KtorGraphQLSubscriptionHooks
open class DefaultKtorGraphQLSubscriptionHooks : KtorGraphQLSubscriptionHooks

private val pongMessage = objectMapper.writeValueAsString(SubscriptionOperationMessage(MessageTypes.GQL_PONG))

override suspend fun handle(session: WebSocketServerSession) {
logger.debug("New client connected")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

log message without context (and there are more below in the class)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unfortunately Ktor doesn't provide any ID or something contextual for the WebSocketSession. So if there is a new client connected, it doesn't seem obvious what to log (except you generate an artificial ID and log it, but I decided not to do that)

what you will get in the log though, is the coroutine trace, which is good enough I think

@thevietto thevietto requested a review from allebdev May 11, 2023 09:55
Copy link
Collaborator

@dariuszkuc dariuszkuc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice work! lets drop the Apollo subscriptions-transport-ws protocol as it is unmaintained since 2018 so there is no point in adding support for it

@@ -160,6 +166,15 @@ class GraphQL(config: GraphQLConfiguration) {
)
)

val subscriptionsHandler: KtorGraphQLSubscriptionHandler = KtorGraphQLWebSocketProtocolHandler(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we make it lazily created? not everyone needs subscriptions so this object may never be used

Suggested change
val subscriptionsHandler: KtorGraphQLSubscriptionHandler = KtorGraphQLWebSocketProtocolHandler(
val subscriptionsHandler: KtorGraphQLSubscriptionHandler by lazy {
KtorGraphQLWebSocketProtocolHandler(...)
}

import io.ktor.server.routing.application
import io.ktor.server.routing.get
import io.ktor.server.routing.post
import io.ktor.http.*
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please don't use wildcard imports, use single import per line -> this comment applies to all other files as well

@@ -18,6 +18,7 @@ package com.expediagroup.graphql.server.ktor

import com.expediagroup.graphql.server.execution.GraphQLRequestHandler
import com.expediagroup.graphql.server.execution.GraphQLServer
import com.expediagroup.graphql.server.ktor.subscriptions.KtorGraphQLSubscriptionHandler
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

guessing this will fail linting as it is not used in the file

Suggested change
import com.expediagroup.graphql.server.ktor.subscriptions.KtorGraphQLSubscriptionHandler

/**
* Default implementation of Apollo Subscription Lifecycle Events (No-op).
*/
open class DefaultKtorGraphQLLegacySubscriptionHooks : KtorGraphQLSubscriptionHooks
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since those hooks are generic and applicable to both graphql-ws and subscriptions-transport-ws lets drop the Legacy from the name. We can probably update KDoc to be more generic as well - yes you can use similar hooks in apollo-server but again those are pretty generic lifecycle events.

Suggested change
open class DefaultKtorGraphQLLegacySubscriptionHooks : KtorGraphQLSubscriptionHooks
open class DefaultKtorGraphQLSubscriptionHooks : KtorGraphQLSubscriptionHooks

@thevietto
Copy link
Contributor Author

thanks @dariuszkuc
all fixed, could you re-review?

@thevietto thevietto requested a review from dariuszkuc May 18, 2023 12:57

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
}
```

### `subscriptions-transport-ws` (legacy)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was just dropped so lets update the docs to reflect it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, updated 👍

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@thevietto thevietto requested a review from dariuszkuc May 18, 2023 18:41
@dariuszkuc dariuszkuc requested review from tapaderster and samuelAndalon and removed request for allebdev May 18, 2023 19:02
Copy link
Collaborator

@dariuszkuc dariuszkuc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look fine to me. @samuelAndalon any thoughts on this PR?

thevietto and others added 2 commits May 19, 2023 19:28

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Co-authored-by: Dariusz Kuc <9501705+dariuszkuc@users.noreply.github.com>

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Co-authored-by: Dariusz Kuc <9501705+dariuszkuc@users.noreply.github.com>
@samuelAndalon samuelAndalon merged commit 98e900e into ExpediaGroup:master May 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants