-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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 parameterised cascade in graphql. #6251
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please run go fmt
on all files
Reviewed 43 of 43 files at r1.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @jatindevdg and @MichaelJCompton)
graphql/dgraph/graphquery.go, line 77 at r1 (raw file):
} else { x.Check2(b.WriteString(" @cascade(")) x.Check2(b.WriteString(strings.Join(query.Cascade,",")))
Can you add a space after the comma so that the rewritten queries look better.
x.Check2(b.WriteString(strings.Join(query.Cascade,", ")))
graphql/e2e/common/query.go, line 1845 at r1 (raw file):
variables: map[string]interface{}{"ids": authorIds}, respData: `{ "queryAuthor": [{
this isn't well indented
graphql/e2e/common/query.go, line 1927 at r1 (raw file):
}] }, { "dob": null,
extra spaces, remove please. Also format the JSON properly.
graphql/resolve/query_test.yaml, line 750 at r1 (raw file):
gqlquery: | query { queryAuthor @cascade(fields:["dob"]) {
Add these test cases.
- Add a case where you have cascade with one of the fields being of
ID
type. - Add a case where one of the fields within cascade belongs to an inteface that the type implements
- Add a case where you query for an interface and then use cascade
graphql/schema/gqlschema.go, line 135 at r1 (raw file):
directive @custom(http: CustomHTTP, dql: String) on FIELD_DEFINITION directive @remote on OBJECT | INTERFACE directive @cascade(fields :[String]) on FIELD
fields: [String]
space after :
not before
graphql/schema/wrappers.go, line 682 at r1 (raw file):
return []string{"__all__"} } var fields []string
Pre allocate slice of correct length to avoid multiple allocations
fields := make([]string, 0, len(arg.Value.Children)
graphql/schema/wrappers.go, line 683 at r1 (raw file):
} var fields []string typ:=f.Type()
This code isn't formatted properly. Please run gofmt
on it and your editor should be setup to run it automatically on save.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @jatindevdg and @MichaelJCompton)
graphql/resolve/query_test.yaml, line 747 at r1 (raw file):
- name: "Parameterized Cascade directive on filter query"
we should also add similar tests in mutation_query_test.yml
, so we are sure that mutation queries are also rewritten properly.
graphql/schema/validation_rules.go, line 70 at r1 (raw file):
"cascade"
we have a const in schema package called cascadeDirective
which can be reused instead of creating this string here.
graphql/schema/validation_rules.go, line 71 at r1 (raw file):
"fields"
the fields
argument should also be declared as a constant like cascadeDirective
and then reused everywhere it is needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 2 of 44 files reviewed, 10 unresolved discussions (waiting on @abhimanyusinghgaur, @manishrjain, @MichaelJCompton, @pawanrawal, and @vvbalaji-dgraph)
graphql/dgraph/graphquery.go, line 77 at r1 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
Can you add a space after the comma so that the rewritten queries look better.
x.Check2(b.WriteString(strings.Join(query.Cascade,", ")))
done.
graphql/e2e/common/query.go, line 1845 at r1 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
this isn't well indented
done.
graphql/e2e/common/query.go, line 1927 at r1 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
extra spaces, remove please. Also format the JSON properly.
done.
graphql/resolve/query_test.yaml, line 747 at r1 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
we should also add similar tests in
mutation_query_test.yml
, so we are sure that mutation queries are also rewritten properly.
done.
graphql/resolve/query_test.yaml, line 750 at r1 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
Add these test cases.
- Add a case where you have cascade with one of the fields being of
ID
type.- Add a case where one of the fields within cascade belongs to an inteface that the type implements
- Add a case where you query for an interface and then use cascade
done.
graphql/schema/gqlschema.go, line 135 at r1 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
fields: [String]
space after:
not before
done
graphql/schema/validation_rules.go, line 70 at r1 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
"cascade"
we have a const in schema package called
cascadeDirective
which can be reused instead of creating this string here.
changed.
graphql/schema/validation_rules.go, line 71 at r1 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
"fields"
the
fields
argument should also be declared as a constant likecascadeDirective
and then reused everywhere it is needed.
changed.
graphql/schema/wrappers.go, line 682 at r1 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
Pre allocate slice of correct length to avoid multiple allocations
fields := make([]string, 0, len(arg.Value.Children)
done.
graphql/schema/wrappers.go, line 683 at r1 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
This code isn't formatted properly. Please run
gofmt
on it and your editor should be setup to run it automatically on save.
done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 42 of 43 files at r2, 2 of 2 files at r3.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @abhimanyusinghgaur, @jatindevdg, @manishrjain, @MichaelJCompton, and @vvbalaji-dgraph)
graphql/e2e/common/error_test.yaml, line 119 at r3 (raw file):
- name: "@cascade only accepts numUids or given type name as arguments for add or update payload "
Ok, so it also accepts numUids? Is there a test for that?
graphql/e2e/common/error_test.yaml, line 125 at r3 (raw file):
author { name }
fix the formatting here
graphql/e2e/common/query.go, line 1845 at r1 (raw file):
Previously, JatinDevDG (Jatin Dev) wrote…
done.
still not very well intended, do you do it manually?
graphql/e2e/common/query.go, line 1911 at r3 (raw file):
"name":"Kramer", "reputation": null, "country": {
sti
graphql/e2e/common/query.go, line 2003 at r3 (raw file):
"dob": null }, { "dob": null,
JSON is badly formatted here
graphql/e2e/common/query.go, line 2013 at r3 (raw file):
query: `query { queryHuman() @cascade(fields:["name"]) { name
bad formatting, name and total credits are not indented
graphql/e2e/common/query.go, line 2031 at r3 (raw file):
name appearsIn }
again badly formatted
graphql/resolve/query_test.yaml, line 750 at r1 (raw file):
Previously, JatinDevDG (Jatin Dev) wrote…
done.
I don't see the interface queries here
graphql/resolve/query_test.yaml, line 964 at r3 (raw file):
dob reputation posts @cascade(fields:["text","title"]) {
What if you add id
here, does it work then?
graphql/schema/wrappers.go, line 760 at r3 (raw file):
typ := f.Type() var idField string if f.field.ObjectDefinition.Name == "Query" {
Is this if blocked needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 43 files at r2.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @abhimanyusinghgaur, @jatindevdg, @manishrjain, @MichaelJCompton, and @vvbalaji-dgraph)
graphql/e2e/common/query.go, line 1972 at r3 (raw file):
Quoted 7 lines of code…
"reputation": 4.5, "name":"George", "dob": null, "posts": [{ "title": "A show about nothing", "text": "Got ya!" }]
still unaligned it seems
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 43 of 45 files reviewed, 13 unresolved discussions (waiting on @abhimanyusinghgaur, @jatindevdg, @manishrjain, @MichaelJCompton, @pawanrawal, and @vvbalaji-dgraph)
graphql/e2e/common/error_test.yaml, line 119 at r3 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
Ok, so it also accepts numUids? Is there a test for that?
yeah, if you give typename or numUids, cascade without parameters is forwarded to query field of mutaton. There is unit test for it.
graphql/e2e/common/error_test.yaml, line 125 at r3 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
fix the formatting here
fixed the formatting of all the e2e tests.
graphql/e2e/common/query.go, line 1845 at r1 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
still not very well intended, do you do it manually?
yeah, i got confused because in IDE it was properly formated.
graphql/resolve/query_test.yaml, line 750 at r1 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
I don't see the interface queries here
ohh, i added them in e2e. i will add those here also.
graphql/resolve/query_test.yaml, line 964 at r3 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
What if you add
id
here, does it work then?
no, post doesn't have id field, so it will give error. PostID will work.
graphql/schema/wrappers.go, line 760 at r3 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
Is this if blocked needed?
yes it is needed, because for add or update payload ,we don't have id field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 42 of 45 files reviewed, 13 unresolved discussions (waiting on @abhimanyusinghgaur, @jatindevdg, @manishrjain, @MichaelJCompton, @pawanrawal, and @vvbalaji-dgraph)
graphql/resolve/query_test.yaml, line 750 at r1 (raw file):
Previously, JatinDevDG (Jatin Dev) wrote…
ohh, i added them in e2e. i will add those here also.
added unit tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 40 of 45 files reviewed, 13 unresolved discussions (waiting on @abhimanyusinghgaur, @jatindevdg, @manishrjain, @MichaelJCompton, @pawanrawal, and @vvbalaji-dgraph)
graphql/schema/wrappers.go, line 760 at r3 (raw file):
Previously, JatinDevDG (Jatin Dev) wrote…
yes it is needed, because for add or update payload ,we don't have id field.
yeah, you were right, we don't need this if block. if id is not present idField will be empty.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 40 of 45 files reviewed, 8 unresolved discussions (waiting on @abhimanyusinghgaur, @jatindevdg, @manishrjain, @MichaelJCompton, @pawanrawal, and @vvbalaji-dgraph)
graphql/schema/wrappers.go, line 760 at r3 (raw file):
Previously, JatinDevDG (Jatin Dev) wrote…
yeah, you were right, we don't need this if block. if id is not present idField will be empty.
do you have a test for a case where you have cascade and id field is not present on the type?
Not for id, but we have test which checks that field in cascade argument should be present in given type.This check is done at validation time. But yeah if we don't give id as argument and only give cascade then there will be error, fixing it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 40 of 47 files reviewed, 8 unresolved discussions (waiting on @abhimanyusinghgaur, @manishrjain, @MichaelJCompton, @pawanrawal, and @vvbalaji-dgraph)
graphql/e2e/common/query.go, line 1911 at r3 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
sti
done.
graphql/e2e/common/query.go, line 1972 at r3 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
"reputation": 4.5, "name":"George", "dob": null, "posts": [{ "title": "A show about nothing", "text": "Got ya!" }]
still unaligned it seems
done.
graphql/e2e/common/query.go, line 2003 at r3 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
JSON is badly formatted here
done.
graphql/e2e/common/query.go, line 2013 at r3 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
bad formatting, name and total credits are not indented
done.
graphql/e2e/common/query.go, line 2031 at r3 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
again badly formatted
done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 5 files at r4, 4 of 4 files at r5.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @abhimanyusinghgaur, @jatindevdg, @manishrjain, @MichaelJCompton, and @vvbalaji-dgraph)
graphql/resolve/mutation_query_test.yaml, line 302 at r5 (raw file):
gqlquery: | mutation { ADD_UPDATE_MUTATION {
What happens if you apply @cascade here with id
field, would that be an error?
graphql/schema/validation_rules.go, line 78 at r5 (raw file):
return } dir := directive.ParentDefinition.Fields
fields := directive.
dir is not the right name
There was a problem hiding this 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 47 files reviewed, 6 unresolved discussions (waiting on @abhimanyusinghgaur, @manishrjain, @MichaelJCompton, @pawanrawal, and @vvbalaji-dgraph)
graphql/resolve/mutation_query_test.yaml, line 302 at r5 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
What happens if you apply @cascade here with
id
field, would that be an error?
yes.Anything apart from "numUids" and "post" in this case will return error.
graphql/schema/validation_rules.go, line 78 at r5 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
fields := directive.
dir is not the right name
changed to typFields.
This PR adds parameterised cascade in graphql. We can now specify individual fields in @cascade.
Fixes GRAPHQL-605
This change is
Docs Preview: