Skip to content

Commit

Permalink
perf(GraphQL): Nested auth queries no longer search through all possi…
Browse files Browse the repository at this point in the history
…ble records (#5666)

Previously auth queries had type filter for all levels and hence it would fetch all nodes of a particular type. This PR only applies auth on the nodes that are part of the parent query. Hence, touching same/less number of nodes as it would have in the query without auth rules.
  • Loading branch information
Arijit Das committed Jul 10, 2020
1 parent e33850e commit 222f42a
Show file tree
Hide file tree
Showing 12 changed files with 707 additions and 307 deletions.
4 changes: 2 additions & 2 deletions graphql/admin/update_group.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ func (urw *updateGroupRewriter) Rewrite(
}
for _, ruleI := range rules {
rule := ruleI.(map[string]interface{})
variable := varGen.Next(ruleType, "", "")
variable := varGen.Next(ruleType, "", "", false)
predicate := rule["predicate"]
permission := rule["permission"]

Expand Down Expand Up @@ -96,7 +96,7 @@ func (urw *updateGroupRewriter) Rewrite(
continue
}

variable := varGen.Next(ruleType, "", "")
variable := varGen.Next(ruleType, "", "", false)
addAclRuleQuery(upsertQuery, predicate.(string), variable)

deleteJson := []byte(fmt.Sprintf(`[
Expand Down
10 changes: 5 additions & 5 deletions graphql/e2e/auth/add_mutation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -179,11 +179,11 @@ func TestAddDeepFilter(t *testing.T) {
mutation addColumn($column: AddColumnInput!) {
addColumn(input: [$column]) {
column {
name
inProject {
projID
name
}
name
inProject {
projID
name
}
}
}
}
Expand Down
105 changes: 94 additions & 11 deletions graphql/e2e/auth/auth_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,38 @@ type uidResult struct {
}
}

func (r *Region) add(t *testing.T, user, role string) {
getParams := &common.GraphQLParams{
Headers: getJWT(t, user, role),
Query: `
mutation addRegion($region: AddRegionInput!) {
addRegion(input: [$region]) {
numUids
}
}
`,
Variables: map[string]interface{}{"region": r},
}
gqlResponse := getParams.ExecuteAsPost(t, graphqlURL)
require.Nil(t, gqlResponse.Errors)
}

func (r *Region) delete(t *testing.T, user, role string) {
getParams := &common.GraphQLParams{
Headers: getJWT(t, user, role),
Query: `
mutation deleteRegion($name: String) {
deleteRegion(filter:{name: { eq: $name}}) {
msg
}
}
`,
Variables: map[string]interface{}{"name": r.Name},
}
gqlResponse := getParams.ExecuteAsPost(t, graphqlURL)
require.Nil(t, gqlResponse.Errors)
}

func getJWT(t *testing.T, user, role string) http.Header {
metaInfo.AuthVars = map[string]interface{}{}
if user != "" {
Expand All @@ -153,6 +185,57 @@ func getJWT(t *testing.T, user, role string) http.Header {
return h
}

func TestOptimizedNestedAuthQuery(t *testing.T) {
query := `
query {
queryMovie {
content
regionsAvailable {
name
global
}
}
}
`
user := "user1"
role := "ADMIN"

getUserParams := &common.GraphQLParams{
Headers: getJWT(t, user, role),
Query: query,
}

gqlResponse := getUserParams.ExecuteAsPost(t, graphqlURL)
require.Nil(t, gqlResponse.Errors)
beforeTouchUids := gqlResponse.Extensions["touched_uids"]
beforeResult := gqlResponse.Data

// Previously, Auth queries would have touched all the new `Regions`. But after the optimization
// we should only touch necessary `Regions` which are assigned to some `Movie`. Hence, adding
// these extra `Regions` would not increase the `touched_uids`.
var regions []Region
for i := 0; i < 100; i++ {
r := Region{
Name: fmt.Sprintf("Test_Region_%d", i),
Global: true,
}
r.add(t, user, role)
regions = append(regions, r)
}

gqlResponse = getUserParams.ExecuteAsPost(t, graphqlURL)
require.Nil(t, gqlResponse.Errors)

afterTouchUids := gqlResponse.Extensions["touched_uids"]
require.Equal(t, beforeTouchUids, afterTouchUids)
require.Equal(t, beforeResult, gqlResponse.Data)

// Clean up
for _, region := range regions {
region.delete(t, user, role)
}
}

func (s Student) deleteByEmail(t *testing.T) {
getParams := &common.GraphQLParams{
Query: `
Expand Down Expand Up @@ -625,14 +708,14 @@ func TestDeepRBACValue(t *testing.T) {
}

query := `
{
queryUser (filter:{username:{eq:"user1"}}) {
username
issues {
msg
}
}
}
query {
queryUser (filter:{username:{eq:"user1"}}) {
username
issues {
msg
}
}
}
`

for _, tcase := range testCases {
Expand All @@ -658,7 +741,7 @@ func TestRBACFilter(t *testing.T) {

query := `
query {
queryLog (order: {asc: logs}) {
queryLog (order: {asc: logs}) {
logs
}
}
Expand Down Expand Up @@ -803,8 +886,8 @@ func TestNestedFilter(t *testing.T) {
}}

query := `
query {
queryMovie (order: {asc: content}) {
query {
queryMovie (order: {asc: content}) {
content
regionsAvailable (order: {asc: name}) {
name
Expand Down
4 changes: 2 additions & 2 deletions graphql/e2e/auth/schema.graphql
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ type Region @auth(
"""}
){
id: ID!
name: String
name: String @search(by: [hash])
global: Boolean @search
users: [User]
}
Expand Down Expand Up @@ -192,7 +192,7 @@ type Movie @auth(
]}
) {
id: ID!
content: String
content: String @search(by: [hash])
hidden: Boolean @search
regionsAvailable: [Region]
reviews: [Review]
Expand Down
Loading

0 comments on commit 222f42a

Please sign in to comment.