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): add support for between filter in GraphQL #6651

Merged
merged 19 commits into from
Oct 19, 2020

Conversation

minhaj-shakeel
Copy link
Contributor

@minhaj-shakeel minhaj-shakeel commented Oct 5, 2020

Fixes GRAPHQL-667.

This PR extend support for between filter in GraphQL. This filter can be used for all scalars when indexed in a type (exact index for String). For the given type

type Student{
   age: Int @search
   name: String @search(by: [exact])
}

between filter can be used both on the age and name field. for eg:

queryStudent(filter: {age: {between: {min: 10, max: 20}}}){
    age
    name
}

and

queryStudent(filter: {name: {between: {min: "abc", max: "def"}}}){
    age
    name
}

are both valid queries.


This change is Reviewable

Docs Preview: Dgraph Preview

@github-actions github-actions bot added the area/graphql Issues related to GraphQL support on Dgraph. label Oct 5, 2020
@pawanrawal
Copy link
Contributor

Currently between only works at the root and not inside filter, so it won't work well with GraphQL. We are keeping this on hold until between works properly at the root and in filters and then we'll do this.

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.

It's looking great but some CI tests are failing. Can you fix those?

Reviewed 49 of 49 files at r2.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @MichaelJCompton and @minhaj-shakeel)


graphql/e2e/common/query.go, line 449 at r2 (raw file):

	err := json.Unmarshal([]byte(gqlResponse.Data), &result)
	require.NoError(t, err)
	require.Equal(t, 3, len(result.QueryPost))

Can we compare the result as well?


graphql/resolve/query_rewriter.go, line 1097 at r2 (raw file):

					// 	between(numLikes,10,20). Order of arguments (min,max) is neccessary or
					// it will return empty
					vals := val.(map[string]interface{})

Is this safe to do? We should check the second argument and return an error if this conversion is not possible.


graphql/resolve/query_rewriter.go, line 1098 at r2 (raw file):

					// it will return empty
					vals := val.(map[string]interface{})
					args = append(args, gql.Arg{Value: maybeQuoteArg(fn, vals["min"])})

You can do this in one statement.

args = append(args, arg1 , arg2)

Copy link
Contributor Author

@minhaj-shakeel minhaj-shakeel 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: 46 of 49 files reviewed, 3 unresolved discussions (waiting on @MichaelJCompton and @pawanrawal)


graphql/e2e/common/query.go, line 449 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Can we compare the result as well?

Done.


graphql/resolve/query_rewriter.go, line 1097 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Is this safe to do? We should check the second argument and return an error if this conversion is not possible.

Yeah, it is safe as it will be get caught up in the validation stage.


graphql/resolve/query_rewriter.go, line 1098 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

You can do this in one statement.

args = append(args, arg1 , arg2)

Done.

@minhaj-shakeel minhaj-shakeel merged commit 542cfff into master Oct 19, 2020
@minhaj-shakeel minhaj-shakeel deleted the minhaj/between-filter branch October 19, 2020 11:24
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.

2 participants