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(GraphQL):This PR adds subscriptions to custom DQL. #7385

Merged
merged 16 commits into from
Feb 18, 2021

Conversation

JatinDev543
Copy link
Contributor

@JatinDev543 JatinDev543 commented Feb 2, 2021

Fixes GRAPHQL-993
As of now, we only allow subscriptions on GraphQL queries. As we can use DQL through @custom in graphql, many users requested to use subscriptions on custom DQL.

We have the @withSubscription directive, that we use on GraphQL types to generate subscriptions on queries for that type.
We are now extending this directive to DQL queries in custom queries. Users can specify @withSubscription directive on individual DQL queries in type Query and those queries will be added in type subscription.

For example, we have two custom DQL queries below queryTweetsSortedByAuthorFollowers and queryUserTweetCounts . queryUserTweetCounts have @withSubscription directive, and it will be added to subscription type and users will be able to subscribe to this query.

type Query {
  queryTweetsSortedByAuthorFollowers(search: String!): [Tweets] @custom(dql: """
	query q($search: string) {
		var(func: type(Tweets)) @filter(anyoftext(Tweets.text, $search)) {
			Tweets.author {
				followers as User.followers
			}
			authorFollowerCount as sum(val(followers))
		}
		queryTweetsSortedByAuthorFollowers(func: uid(authorFollowerCount), orderdesc: val(authorFollowerCount)) {
			id: uid
			text: Tweets.text
			author: Tweets.author {
			    screen_name: User.screen_name
			    followers: User.followers
			}
			timestamp: Tweets.timestamp
		}
	}
	""")

  queryUserTweetCounts: [UserTweetCount]  `@withSubscription` @custom(dql: """
	query {
		queryUserTweetCounts(func: type(User)) {
			screen_name: User.screen_name
			tweetCount: count(User.tweets)
		}
	}
	""")
}


This change is Reviewable

@github-actions github-actions bot added area/documentation Documentation related issues. area/graphql Issues related to GraphQL support on Dgraph. labels Feb 2, 2021
Copy link
Contributor

@pawanrawal pawanrawal left a comment

Choose a reason for hiding this comment

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

Reviewed 7 of 54 files at r1.
Reviewable status: 7 of 54 files reviewed, 5 unresolved discussions (waiting on @danielmai and @jatindevdg)


graphql/e2e/schema/generatedSchema.graphql, line 183 at r1 (raw file):

directive @dgraph(type: String, pred: String) on OBJECT | INTERFACE | FIELD_DEFINITION
directive @id on FIELD_DEFINITION
directive @withSubscription on OBJECT | INTERFACE | FIELD_DEFINITION

We should have a validation that this is only allowed on fields within Query and not fields within types.


graphql/e2e/subscription/subscription_test.go, line 75 at r1 (raw file):

# Dgraph.Authorization {"VerificationKey":"secret","Header":"Authorization","Namespace":"https://dgraph.io","Algo":"HS256"}
`
	schCustomDQL = `type Tweets {

have type tweets on the next line


graphql/schema/gqlschema.go, line 850 at r1 (raw file):

			for _, q := range defn.Fields {
				customDir := q.Directives.ForName(customDirective)
				subsDir := q.Directives.ForName(subscriptionDirective)

what happens if there is a withSubscription and its not a DQL query, is that an error?


graphql/schema/gqlschema.go, line 852 at r1 (raw file):

				subsDir := q.Directives.ForName(subscriptionDirective)
				if customDir != nil {
					if !(customDir.Arguments.ForName("dql") == nil || subsDir == nil) {

(if subsDir != nil && dql != nil) is more readable


graphql/schema/testdata/schemagen/input/custom-dql-query-with-subscription.graphql, line 18 at r1 (raw file):

type Query {
    queryTweetsSortedByAuthorFollowers(search: String!): [Tweets] @custom(dql: """

not useful for this test, please remove as this is probably already tested in another file

Copy link
Contributor Author

@JatinDev543 JatinDev543 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 3 of 56 files reviewed, 5 unresolved discussions (waiting on @danielmai and @pawanrawal)


graphql/e2e/schema/generatedSchema.graphql, line 183 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

We should have a validation that this is only allowed on fields within Query and not fields within types.

done.


graphql/e2e/subscription/subscription_test.go, line 75 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

have type tweets on the next line

done.


graphql/schema/gqlschema.go, line 850 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

what happens if there is a withSubscription and its not a DQL query, is that an error?

added validation error for that case.


graphql/schema/gqlschema.go, line 852 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

(if subsDir != nil && dql != nil) is more readable

changed.


graphql/schema/testdata/schemagen/input/custom-dql-query-with-subscription.graphql, line 18 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

not useful for this test, please remove as this is probably already tested in another file

removed.

Copy link
Contributor

@pawanrawal pawanrawal left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 11 files at r2.
Reviewable status: 4 of 57 files reviewed, all discussions resolved (waiting on @danielmai and @pawanrawal)

@JatinDev543 JatinDev543 merged commit f35fbf8 into master Feb 18, 2021
@JatinDev543 JatinDev543 deleted the jatin/GRAPHQL-993 branch February 18, 2021 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/documentation Documentation related issues. area/graphql Issues related to GraphQL support on Dgraph.
Development

Successfully merging this pull request may close these issues.

2 participants