Skip to content

Commit

Permalink
perf(GraphQL): JSON Part-5: Change query rewriting for scalar/object …
Browse files Browse the repository at this point in the history
…fields asked multiple times at same level (GRAPHQL-938) (#7283)

Previously, for a GraphQL query like this:

```
query {
    queryThing {
      i1: id
      i2: id
      name
      n: name
      n1: name
    }
}
```

GraphQL layer used to generate following DQL query:

```
query {
    queryThing(func: type(Thing)) {
      id : uid
      name : Thing.name
    }
}
```

This can’t work with the new way of JSON encoding for GraphQL because it expects that everything asked for in GraphQL query is present in DQL response. So, Now, the GraphQL layer generates the following DQL to make it work:

```
query {
    queryThing(func: type(Thing)) {
      Thing.i1 : uid
      Thing.i2: uid
      Thing.name : Thing.name
      Thing.n : Thing.name
      Thing.n1 : Thing.name
    }
}
```

Basically, the `DgraphAlias()` is now generated using the `TypeCondition.GraphqlAlias` format. That works even with fragments as they have their specific type conditions. Here are related thoughts:
```
query {
	queryHuman {
		name
		n: name
	}
	# DQL: use field.name as DgAlias()
	# queryHuman(func: type(Human)) {
	#   name: Human.name
	#   name: Human.name
	# }
	# => just using field.name doesn't work => use field.alias
	# queryHuman(func: type(Human)) {
	#   name: Human.name
	#   n: Human.name
	# }
	# but there are interfaces and fragments
	queryCharacter {
		... on Human {
			n: name
		}
		... on Droid {
			n: bio
		}
	}
	# DQL: use field.alias as DgAlias()
	# queryCharacter(func: type(Character)) {
	#   n: Character.name
	#   n: Character.bio
	# }
	# => just using field.alias doesn't work => use field.name + field.alias
	# queryCharacter(func: type(Character)) {
	#   name.n: Character.name
	#   bio.n: Character.bio
	# }
	# but the implementing types may have fields with same name not inherited from the interface
	queryThing {
		... on ThingOne {
			c: color
		}
		... on ThingTwo {
			c: color
		}
	}
	# DQL: use field.name + field.alias as DgAlias()
	# queryThing(func: type(Thing)) {
	#   color.c: ThingOne.color
	#   color.c: ThingTwo.color
	# }
	# => even that doesn't work => use Typename + field.name + field.alias
	# queryThing(func: type(Thing)) {
	#   ThingOne.color.c: ThingOne.color
	#   ThingTwo.color.c: ThingTwo.color
	# }
	# nice! but then different frags explicitly ask for an inherited field with same name & alias
	queryCharacter {
		... on Human {
			n: name
		}
		... on Droid {
			n: name
		}
	}
	# DQL: use Typename + field.name + field.alias as DgAlias()
	# queryCharacter(func: type(Character)) {
	#   Character.name.n: Character.name
	#   Character.name.n: Character.name
	# }
	# => doesn't work => use typeCond + Typename + field.name + field.alias
	# queryCharacter(func: type(Character)) {
	#   Human.Character.name.n: Character.name
	#   Droid.Character.name.n: Character.name
	# }
	# but wait, wouldn't just typeCond + field.alias work?
	# queryCharacter(func: type(Character)) {
	#   Human.n: Character.name
	#   Droid.n: Character.name
	# }
	# yeah! that works even for all the above cases.
	#
	# OR
	#
	# just appending the count when encountering duplicate alias at the same level also works.
	# But, we need to maintain the count map every time we need DgAlias().
	# Also, this requires query pre-processing and the generated aliases are non-deterministic.
	#
	# So, we are going with the typeCond + field.alias approach. That will work with @Custom(dql:...) too.
}
```
  • Loading branch information
abhimanyusinghgaur authored Jan 13, 2021
1 parent 01a617a commit c95c464
Show file tree
Hide file tree
Showing 21 changed files with 1,214 additions and 1,204 deletions.
16 changes: 12 additions & 4 deletions graphql/e2e/auth/auth_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1622,7 +1622,11 @@ func TestChildAggregateQueryWithDeepRBAC(t *testing.T) {
[
{
"username": "user1",
"issuesAggregate": null
"issuesAggregate": {
"count": null,
"msgMax": null,
"msgMin": null
}
}
]
}`},
Expand Down Expand Up @@ -1668,7 +1672,7 @@ func TestChildAggregateQueryWithDeepRBAC(t *testing.T) {
gqlResponse := getUserParams.ExecuteAsPost(t, common.GraphqlURL)
common.RequireNoGQLErrors(t, gqlResponse)

require.JSONEq(t, string(gqlResponse.Data), tcase.result)
require.JSONEq(t, tcase.result, string(gqlResponse.Data))
})
}
}
Expand All @@ -1684,7 +1688,11 @@ func TestChildAggregateQueryWithOtherFields(t *testing.T) {
{
"username": "user1",
"issues":[],
"issuesAggregate": null
"issuesAggregate": {
"count": null,
"msgMin": null,
"msgMax": null
}
}
]
}`},
Expand Down Expand Up @@ -1739,7 +1747,7 @@ func TestChildAggregateQueryWithOtherFields(t *testing.T) {
gqlResponse := getUserParams.ExecuteAsPost(t, common.GraphqlURL)
common.RequireNoGQLErrors(t, gqlResponse)

require.JSONEq(t, string(gqlResponse.Data), tcase.result)
require.JSONEq(t, tcase.result, string(gqlResponse.Data))
})
}
}
Expand Down
36 changes: 36 additions & 0 deletions graphql/e2e/common/common.go
Original file line number Diff line number Diff line change
Expand Up @@ -463,6 +463,42 @@ func assertUpdateGqlSchemaUsingAdminSchemaEndpt(t *testing.T, authority, schema
AssertSchemaUpdateCounterIncrement(t, authority, oldCounter)
}

// JSONEqGraphQL compares two JSON strings obtained from a /graphql response.
// To avoid issues, don't use space for indentation in expected input.
//
// The comparison requirements for JSON reported by /graphql are following:
// * The key order matters in object comparison, i.e.
// {"hello": "world", "foo": "bar"}
// is not same as:
// {"foo": "bar", "hello": "world"}
// * A key missing in an object is not same as that key present with value null, i.e.
// {"hello": "world"}
// is not same as:
// {"hello": "world", "foo": null}
// * Integers that are out of the [-(2^53)+1, (2^53)-1] precision range supported by JSON RFC,
// should still be encoded with full precision. i.e., the number 9007199254740993 ( = 2^53 + 1)
// should not get encoded as 9007199254740992 ( = 2^53). This happens in Go's standard JSON
// parser due to IEEE754 precision loss for floating point numbers.
//
// The above requirements are not satisfied by the standard require.JSONEq or testutil.CompareJSON
// methods.
// In order to satisfy all these requirements, this implementation just requires that the input
// strings be equal after removing `\r`, `\n`, `\t` whitespace characters from the inputs.
// TODO:
// Find a better way to do this such that order isn't mandated in list comparison.
// So that it is actually usable at places it is not used at present.
func JSONEqGraphQL(t *testing.T, expected, actual string) {
expected = strings.ReplaceAll(expected, "\r", "")
expected = strings.ReplaceAll(expected, "\n", "")
expected = strings.ReplaceAll(expected, "\t", "")

actual = strings.ReplaceAll(actual, "\r", "")
actual = strings.ReplaceAll(actual, "\n", "")
actual = strings.ReplaceAll(actual, "\t", "")

require.Equal(t, expected, actual)
}

func (twt *Tweets) DeleteByID(t *testing.T, user string, metaInfo *testutil.AuthMeta) {
getParams := &GraphQLParams{
Headers: GetJWT(t, user, "", metaInfo),
Expand Down
184 changes: 81 additions & 103 deletions graphql/e2e/common/fragment.go
Original file line number Diff line number Diff line change
Expand Up @@ -108,23 +108,15 @@ func fragmentInQuery(t *testing.T) {
queryStarshipExpected := fmt.Sprintf(`
{
"queryStarship":[{
"id": "%s",
"id":"%s",
"name":"Millennium Falcon",
"length":2
"length":2.000000
}]
}`, newStarship.ID)

var expected, result struct {
QueryStarship []*starship
}
err := json.Unmarshal([]byte(queryStarshipExpected), &expected)
require.NoError(t, err)
err = json.Unmarshal(gqlResponse.Data, &result)
require.NoError(t, err)
JSONEqGraphQL(t, queryStarshipExpected, string(gqlResponse.Data))

require.Equal(t, expected, result)

cleanupStarwars(t, result.QueryStarship[0].ID, "", "")
cleanupStarwars(t, newStarship.ID, "", "")
}

func fragmentInQueryOnInterface(t *testing.T) {
Expand Down Expand Up @@ -281,51 +273,51 @@ func fragmentInQueryOnInterface(t *testing.T) {
{
"queryCharacter":[
{
"__typename": "Human",
"id": "%s",
"name": "Han",
"appearsIn": ["EMPIRE"],
"starships": [{
"__typename": "Starship",
"id": "%s",
"name": "Millennium Falcon",
"length": 2
"__typename":"Human",
"id":"%s",
"name":"Han",
"appearsIn":["EMPIRE"],
"starships":[{
"__typename":"Starship",
"id":"%s",
"name":"Millennium Falcon",
"length":2.000000
}],
"totalCredits": 10,
"ename": "Han_employee"
"totalCredits":10.000000,
"ename":"Han_employee"
},
{
"__typename": "Droid",
"id": "%s",
"name": "R2-D2",
"appearsIn": ["EMPIRE"],
"primaryFunction": "Robot"
"__typename":"Droid",
"id":"%s",
"name":"R2-D2",
"appearsIn":["EMPIRE"],
"primaryFunction":"Robot"
}
],
"qc":[
{
"__typename": "Human",
"id": "%s",
"name": "Han"
"__typename":"Human",
"id":"%s",
"name":"Han"
},
{
"__typename": "Droid",
"appearsIn": ["EMPIRE"]
"__typename":"Droid",
"appearsIn":["EMPIRE"]
}
],
"qc1":[
{
"__typename": "Human",
"id": "%s"
"__typename":"Human",
"id":"%s"
},
{
"id": "%s"
"id":"%s"
}
],
"qc2":[
{
"name": "Han",
"n": "Han"
"name":"Han",
"n":"Han"
},
{
}
Expand All @@ -341,69 +333,63 @@ func fragmentInQueryOnInterface(t *testing.T) {
"primaryFunction":"Robot"
}
],
"qcRep1": [
{
"name": "Han",
"totalCredits": 10
},
{
"name": "R2-D2",
"primaryFunction": "Robot"
}
"qcRep1":[
{
"name":"Han",
"totalCredits":10.000000
},
{
"name":"R2-D2",
"primaryFunction":"Robot"
}
],
"qcRep2": [
{
"totalCredits": 10,
"name": "Han"
},
{
"name": "R2-D2",
"primaryFunction": "Robot"
}
"qcRep2":[
{
"totalCredits":10.000000,
"name":"Han"
},
{
"name":"R2-D2",
"primaryFunction":"Robot"
}
],
"qcRep3": [
{
"name": "Han"
},
{
"name": "R2-D2"
}
"qcRep3":[
{
"name":"Han"
},
{
"name":"R2-D2"
}
],
"queryThing":[
{
"__typename": "ThingOne",
"id": "%s",
"name": "Thing-1",
"color": "White",
"usedBy": "me"
"__typename":"ThingOne",
"id":"%s",
"name":"Thing-1",
"color":"White",
"usedBy":"me"
},
{
"__typename": "ThingTwo",
"id": "%s",
"name": "Thing-2",
"color": "Black",
"owner": "someone"
"__typename":"ThingTwo",
"id":"%s",
"name":"Thing-2",
"color":"Black",
"owner":"someone"
}
],
"qt":[
{
"__typename": "ThingOne",
"id": "%s"
"__typename":"ThingOne",
"id":"%s"
},
{
"__typename": "ThingTwo"
"__typename":"ThingTwo"
}
]
}`, humanID, newStarship.ID, droidID, humanID, humanID, droidID, thingOneId, thingTwoId,
thingOneId)

var expected, result map[string]interface{}
err := json.Unmarshal([]byte(queryCharacterExpected), &expected)
require.NoError(t, err)
err = json.Unmarshal(gqlResponse.Data, &result)
require.NoError(t, err)

require.Equal(t, expected, result)
JSONEqGraphQL(t, queryCharacterExpected, string(gqlResponse.Data))

cleanupStarwars(t, newStarship.ID, humanID, droidID)
deleteThingOne(t, thingOneId)
Expand Down Expand Up @@ -564,31 +550,23 @@ func fragmentInQueryOnObject(t *testing.T) {
{
"queryHuman":[
{
"__typename": "Human",
"id": "%s",
"name": "Han",
"appearsIn": ["EMPIRE"],
"starships": [{
"__typename": "Starship",
"id": "%s",
"name": "Millennium Falcon",
"length": 2
"__typename":"Human",
"id":"%s",
"name":"Han",
"appearsIn":["EMPIRE"],
"starships":[{
"__typename":"Starship",
"id":"%s",
"name":"Millennium Falcon",
"length":2.000000
}],
"totalCredits": 10,
"ename": "Han_employee"
"totalCredits":10.000000,
"ename":"Han_employee"
}
]
}`, humanID, newStarship.ID)

var expected, result struct {
QueryHuman []map[string]interface{}
}
err := json.Unmarshal([]byte(queryCharacterExpected), &expected)
require.NoError(t, err)
err = json.Unmarshal(gqlResponse.Data, &result)
require.NoError(t, err)

require.Equal(t, expected, result)
JSONEqGraphQL(t, queryCharacterExpected, string(gqlResponse.Data))

cleanupStarwars(t, newStarship.ID, humanID, "")
}
2 changes: 2 additions & 0 deletions graphql/e2e/common/lambda.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import (
)

func lambdaOnTypeField(t *testing.T) {
t.Skipf("enable after porting JSON for @custom")
query := `
query {
queryAuthor {
Expand Down Expand Up @@ -61,6 +62,7 @@ func lambdaOnTypeField(t *testing.T) {
}

func lambdaOnInterfaceField(t *testing.T) {
t.Skipf("enable after porting JSON for @custom")
starship := addStarship(t)
humanID := addHuman(t, starship.ID)
droidID := addDroid(t)
Expand Down
Loading

0 comments on commit c95c464

Please sign in to comment.