Skip to content

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

Merged
arijitAD merged 3 commits intomasterfrom
arijitAD/Fix-Deep-Auth-Get-Query
Jun 1, 2020
Merged

graphql: Apply type filter for get query at root level.#5497
arijitAD merged 3 commits intomasterfrom
arijitAD/Fix-Deep-Auth-Get-Query

Conversation

@arijitAD
Copy link
Copy Markdown

@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 requested a review from vardhanapoorv May 21, 2020 18:09
@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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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