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

graphql: Apply type filter for get query at root level. #5497

Merged
merged 3 commits into from
Jun 1, 2020

Conversation

arijitAD
Copy link

@arijitAD arijitAD commented May 21, 2020

This change is Reviewable

@github-actions github-actions bot added the area/graphql Issues related to GraphQL support on Dgraph. label May 21, 2020
@arijitAD arijitAD changed the title Fix auth deep get query. graphql: Fix auth deep get query. May 22, 2020
@arijitAD arijitAD changed the title graphql: Fix auth deep get query. graphql: Apply type filter for get query at root level. May 26, 2020
Copy link
Contributor

@MichaelJCompton MichaelJCompton left a comment

Choose a reason for hiding this comment

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

:lgtm:

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?

Copy link
Author

@arijitAD arijitAD 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: 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.

Copy link
Contributor

@MichaelJCompton MichaelJCompton left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 0 of 5 files reviewed, 3 unresolved discussions (waiting on @MichaelJCompton, @pawanrawal, and @vardhanapoorv)

@arijitAD arijitAD merged commit 1a3b417 into master Jun 1, 2020
dna2github pushed a commit to dna2fork/dgraph that referenced this pull request Jul 18, 2020
@joshua-goldstein joshua-goldstein deleted the arijitAD/Fix-Deep-Auth-Get-Query branch August 11, 2022 20:45
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