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): Support using custom DQL with @groupby #7476

Merged
merged 12 commits into from
Feb 25, 2021

Conversation

minhaj-shakeel
Copy link
Contributor

@minhaj-shakeel minhaj-shakeel commented Feb 24, 2021

Fixes GRAPHQL-754.
This PR adds support for the @remoteResponse directive in the GraphQL schema. Users can define this directive on the fields of a @Remote type in order to map the response JSON key of a custom query to a GraphQL field.
For example, for the given GraphQL schema:-

type User {
	screen_name: String! @id
	followers: Int @search
	tweets: [Tweets] @hasInverse(field: user)
}
type UserTweetCount @remote {
	screen_name: String
	tweetCount: Int
}
type UserMap @remote {
	followers: Int
	count: Int
}
type GroupUserMapQ @remote {
	groupby: [UserMap] @remoteResponse(name: "@groupby")
}

the below @Custom DQL query is made possible:-

queryUserKeyMap: [GroupUserMapQ] @custom(dql: """
{
	queryUserKeyMap(func: type(User)) @groupby(followers: User.followers){
		count(uid)
	}
}
""")

This change is Reviewable

@github-actions github-actions bot added the area/graphql Issues related to GraphQL support on Dgraph. label Feb 24, 2021
Copy link
Contributor

@abhimanyusinghgaur abhimanyusinghgaur left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 4 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @minhaj-shakeel and @pawanrawal)


graphql/schema/rules.go, line 2122 at r1 (raw file):

"Type %s; Argument %s inside @remoteResponse directive must be defined."

also mention the field in the error message


graphql/schema/wrappers.go, line 923 at r1 (raw file):

	fd := f.field.ObjectDefinition.Fields.ForName(f.Name())
	remoteResponseName := remoteResponseDirectiveArgument(fd)

The ForName() lookups here may turn out to be expensive.
Let's store the remoteResponse names in our schema wrapper like we store dgraphPredicate, customDirectives, lambdaDirectives mapping there and then remove the directive from the field definition once it has been added to the mapping.

Copy link
Contributor

@abhimanyusinghgaur abhimanyusinghgaur left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 59 of 59 files at r2, 2 of 2 files at r3.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @minhaj-shakeel and @pawanrawal)

@minhaj-shakeel minhaj-shakeel merged commit 7405eeb into master Feb 25, 2021
@minhaj-shakeel minhaj-shakeel deleted the minhaj/groupby branch February 25, 2021 17:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/graphql Issues related to GraphQL support on Dgraph.
Development

Successfully merging this pull request may close these issues.

None yet

2 participants