-
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
graphql: Apply type filter for get query at root level. #5497
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.
Fix those suggestions, otherwise, all good.
Reviewable status: 0 of 2 files reviewed, 3 unresolved discussions (waiting on @arijitAD, @MichaelJCompton, @pawanrawal, and @vardhanapoorv)
graphql/resolve/query_rewriter.go, line 238 at r1 (raw file):
} func getRootQuery(query *gql.GraphQuery, rootName string) (*gql.GraphQuery, bool) {
why does this return a bool - it looks un used.
graphql/resolve/query_rewriter.go, line 259 at r1 (raw file):
addTypeFilter(dgQuery, field.Type()) } else { addTypeFilter(dgQuery.Children[1], field.Type())
So this was the mistake, right? Sometimes the [1]
wasn't the right query cause the rewriting order doesn't enforce that?
In Apoorv's example, as it got deeper, the index [1]
was a different thing, so that's why it had strange results?
graphql/resolve/query_rewriter.go, line 271 at r1 (raw file):
addTypeFilter(dgQuery, field.Type()) } else { rootQuery, _ := getRootQuery(dgQuery, field.Name())
can you simplify and remove the if
altogether?
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: 0 of 3 files reviewed, 3 unresolved discussions (waiting on @MichaelJCompton, @pawanrawal, and @vardhanapoorv)
graphql/resolve/query_rewriter.go, line 238 at r1 (raw file):
Previously, MichaelJCompton (Michael Compton) wrote…
why does this return a bool - it looks un used.
This was there to signal the recursion that the root query is found at the child level.
childQuery, ok := getRootQuery(q, rootName); ok
Removed this recursion since the root query can be found in at max two levels.
graphql/resolve/query_rewriter.go, line 259 at r1 (raw file):
Previously, MichaelJCompton (Michael Compton) wrote…
So this was the mistake, right? Sometimes the
[1]
wasn't the right query cause the rewriting order doesn't enforce that?In Apoorv's example, as it got deeper, the index
[1]
was a different thing, so that's why it had strange results?
Yes. The query would be rewritten like the below example and this line caused the issue. Column1 as var(func: type(Column)) @filter(type(Project))
query {
getProject(func: uid(Project2)) @filter(uid(Project3)) {
projID : uid
columns : Project.columns @filter(uid(Column1)) {
name : Column.name
colID : uid
}
}
Project2 as var(func: uid(0x123))
Project3 as var(func: uid(Project2)) @cascade {
roles : Project.roles @filter(eq(Role.permission, "VIEW")) {
assignedTo : Role.assignedTo @filter(eq(User.username, "user1"))
dgraph.uid : uid
}
dgraph.uid : uid
}
Column1 as var(func: type(Column)) @filter(type(Project)) @cascade {
inProject : Column.inProject {
roles : Project.roles @filter(eq(Role.permission, "VIEW")) {
assignedTo : Role.assignedTo @filter(eq(User.username, "user1"))
dgraph.uid : uid
}
dgraph.uid : uid
}
dgraph.uid : uid
}
}
graphql/resolve/query_rewriter.go, line 271 at r1 (raw file):
Previously, MichaelJCompton (Michael Compton) wrote…
can you simplify and remove the
if
altogether?
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.
Reviewable status: 0 of 5 files reviewed, 3 unresolved discussions (waiting on @MichaelJCompton, @pawanrawal, and @vardhanapoorv)
…#5497) * Fix auth deep get query.
This change is