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 has filter on list of fields. #7363

Merged
merged 15 commits into from
Feb 5, 2021

Conversation

minhaj-shakeel
Copy link
Contributor

@minhaj-shakeel minhaj-shakeel commented Jan 25, 2021

Fixes GRAPHQL-814.


This change is Reviewable

@github-actions github-actions bot added the area/graphql Issues related to GraphQL support on Dgraph. label Jan 25, 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 58 files at r1.
Reviewable status: 4 of 58 files reviewed, 4 unresolved discussions (waiting on @minhaj-shakeel and @pawanrawal)


graphql/e2e/common/mutation.go, line 1077 at r1 (raw file):

0

if we are setting it to 1000 and expecting 0, how can this pass in the test?
seems something fishy with the test.


graphql/e2e/common/mutation.go, line 1151 at r1 (raw file):

 0

same here, 1000 expected as 0


graphql/e2e/common/query.go, line 697 at r1 (raw file):

	newPost1 := addPostWithNullText(t, newAuthor.ID, newCountry.ID, postExecutor)
	newPost2 := addPostWithNoTags(t, newAuthor.ID, newCountry.ID, postExecutor)

instead of repeating the same code in each of these special cases, can we just write one function

addPostWithVars(t, authorId, countryId string, postVar map[string]interface) *post {
   ...
   addPostParams := &common.GraphQLParams{
      ...
      Vars: map[string]interface{}{"post":postVar}
   }
   ...
}

and use that inside each of these special case functions?


graphql/e2e/common/query.go, line 713 at r1 (raw file):

Quoted 6 lines of code…
	var result struct {
		QueryPost []*post
	}
	err := json.Unmarshal([]byte(gqlResponse.Data), &result)
	require.NoError(t, err)
	require.Equal(t, 1, len(result.QueryPost))

instead of making sure that only one post is returned, we should make sure what is exactly returned in that post, because right now, that one post can also be the one that we are not expecting. So, we should do something like:

testutil.CompareJSON(t, `{"queryPost":[{"title": "..."}]}`)

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 55 of 58 files at r1, 3 of 3 files at r2.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @minhaj-shakeel)


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

				})
			case []interface{}:
				// has: [comments, text] -> has(comments) AND has(text)

Add 1 e2e test as well for which has takes a list of arguments.

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 2 of 2 files at r3.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @minhaj-shakeel)

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.

3 participants