fix(GraphQL): Requesting only __typename now returns results

[JatinDev543]

This PR fixes Type name issue for single level and nested GraphQL queries. #5020

---

This change is 

Docs Preview:

[MichaelJCompton]

Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @jatindevdg, @MichaelJCompton, and @pawanrawal)

---

graphql/e2e/common/query.go, line 1151 at r1 (raw file):

                "__typename": "Country"
          }
        ]

I think this query should have 1 result for every country

if I had asked for { id __typename } ... then I would get an array with n countries in it, so why does { __typename } always give me just one element?

---

graphql/resolve/query.go, line 104 at r1 (raw file):

	}

        if string(resp.GetJson()) == fmt.Sprintf(`{"%s":[]}`, query.Name()) {

I think why this already works at deep levels is because we always write { uid } into every level of the query.

I'm wondering if we just make sure that happens at the top level will we get the right array answer ???

so the rewriter will end up with a query like queryCountry(func ...) { uid }

[abhimanyusinghgaur]

Reviewed 2 of 3 files at r2.
Reviewable status: 2 of 3 files reviewed, 6 unresolved discussions (waiting on @jatindevdg and @pawanrawal)

---

graphql/e2e/common/query.go, line 1154 at r2 (raw file):

Quoted 11 lines of code…

	"queryCountry": [
          {
                "__typename": "Country"
          },
          {
                "__typename": "Country"
          },
          {
                "__typename": "Country"
          }
        ]

We are getting 3 countries because we are initially inserting them during BootstrapServer. But, that may change in future. So, can we instead first do a query like:

queryCountry {
    uid
}

and then count how many countries are returned in that response. Then we should form the response for __typename query dynamically to match that exact number of countries.

Also, we should write another test case for get queries too.

---

graphql/e2e/common/query.go, line 1163 at r2 (raw file):

}

func queryNestedOnlyTypename(t *testing.T) {

Nice! we didn't had this test case earlier.

---

graphql/e2e/common/query.go, line 1193 at r2 (raw file):

	testutil.CompareJSON(t, expected, string(gqlResponse.Data))
}
func queryNestedTypename(t *testing.T) {

A new line between two functions.

---

graphql/resolve/query_rewriter.go, line 176 at r2 (raw file):

	if len(dgQuery.Children) == 0 {
		return
	}

I guess just this check could have been removed instead. But, doing it this way results in changes in a lot of test cases related to auth. I am not sure if that should happen.
Instead if we do it for each type of query separately, then the testcases for auth won't need to be changed.
Let's talk to @MichaelJCompton about this.

[MichaelJCompton]

reviewed.

Reviewable status: 2 of 3 files reviewed, 6 unresolved discussions (waiting on @abhimanyusinghgaur, @jatindevdg, and @pawanrawal)

---

graphql/e2e/common/query.go, line 1154 at r2 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

	"queryCountry": [
          {
                "__typename": "Country"
          },
          {
                "__typename": "Country"
          },
          {
                "__typename": "Country"
          }
        ]

We are getting 3 countries because we are initially inserting them during BootstrapServer. But, that may change in future. So, can we instead first do a query like:

queryCountry {
    uid
}

and then count how many countries are returned in that response. Then we should form the response for __typename query dynamically to match that exact number of countries.

Also, we should write another test case for get queries too.

👍

---

graphql/e2e/common/query.go, line 1163 at r2 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

Nice! we didn't had this test case earlier.

same as above, check how many are there first and then test for that many.

---

graphql/resolve/query_rewriter.go, line 176 at r2 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

	if len(dgQuery.Children) == 0 {
		return
	}

I guess just this check could have been removed instead. But, doing it this way results in changes in a lot of test cases related to auth. I am not sure if that should happen.
Instead if we do it for each type of query separately, then the testcases for auth won't need to be changed.
Let's talk to @MichaelJCompton about this.

The less tests that change the better.

[JatinDev543]

Reviewed 2 of 3 files at r2.
Reviewable status: 2 of 3 files reviewed, 6 unresolved discussions (waiting on @jatindevdg and @pawanrawal)

graphql/e2e/common/query.go, line 1154 at r2 (raw file):

Quoted 11 lines of code…
We are getting 3 countries because we are initially inserting them during BootstrapServer. But, that may change in future. So, can we instead first do a query like:

queryCountry {
    uid
}

and then count how many countries are returned in that response. Then we should form the response for __typename query dynamically to match that exact number of countries.

Also, we should write another test case for get queries too.

graphql/e2e/common/query.go, line 1163 at r2 (raw file):

}

func queryNestedOnlyTypename(t *testing.T) {

Nice! we didn't had this test case earlier.

Tests were there but have other fields with __typename. I just copy and modified them.

graphql/e2e/common/query.go, line 1193 at r2 (raw file):

	testutil.CompareJSON(t, expected, string(gqlResponse.Data))
}
func queryNestedTypename(t *testing.T) {

A new line between two functions.

graphql/resolve/query_rewriter.go, line 176 at r2 (raw file):

	if len(dgQuery.Children) == 0 {
		return
	}

I guess just this check could have been removed instead. But, doing it this way results in changes in a lot of test cases related to auth. I am not sure if that should happen.
Instead if we do it for each type of query separately, then the testcases for auth won't need to be changed.

Yeah if we remove this check then we have uid child for nodes also which already have other fields.This is require because function is recursive and that's why i created other function , to deal with empty query results seperately. Will check how to do it seperately for individual queries.

Let's talk to @MichaelJCompton about this.

[MichaelJCompton]

let's get this gong through CI

Reviewable status: 1 of 3 files reviewed, 10 unresolved discussions (waiting on @abhimanyusinghgaur, @jatindevdg, and @pawanrawal)

---

graphql/resolve/query.go, line 108 at r4 (raw file):

	//  	query.Type().Name()))
	//  }

delete all this

---

graphql/resolve/query_rewriter.go, line 233 at r4 (raw file):

Quoted 5 lines of code…

	if authRw.writingAuth() {
		addUID(dgQuery, true)
	} else {
		addUID(dgQuery, false)
	}

simpler ?

addUID(dgQuery, authRw.writingAuth() )

---

graphql/resolve/query_rewriter.go, line 333 at r4 (raw file):

	}
	selectionAuth := addSelectionSetFrom(dgQuery, field, auth)
	if auth.writingAuth() {

same here - simpler version

---

graphql/resolve/query_rewriter.go, line 386 at r4 (raw file):

	selectionAuth := addSelectionSetFrom(dgQuery, field, authRw)

	if authRw.writingAuth() {

again

[pawanrawal]

Reviewed 2 of 4 files at r5, 2 of 2 files at r6.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @abhimanyusinghgaur and @jatindevdg)

---

graphql/e2e/common/query.go, line 1233 at r6 (raw file):

	RequireNoGQLErrors(t, gqlResponse)
	require.NoError(t, json.Unmarshal([]byte(gqlResponse.Data), &authorCountResp))
	require.True(t, len(authorCountResp.QueryAuthor) > 0)

This could have been simpler had we just done first:1 here. Then we didn't need to count number of authors and construct the response dynamically.

__
_
graphql/resolve/query_rewriter.go, line 615 at r6 (raw file):

			Attr: "dgraph.type",
		})
	}

Why don't we add the dgraph.uid edge for these cases.

if len(selSet) == 1 && !auth.... && selSet[0].Name() == schema.Typename {
  q.Children = append(q.Children, `dgraph.uid`)
}

Reason being that fetching uid is more performant than fetching dgraph.type and dgraph.type can be inferred in completeObject like we do already?

[JatinDev543]

Reviewed 2 of 4 files at r5, 2 of 2 files at r6.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @abhimanyusinghgaur and @jatindevdg)

graphql/e2e/common/query.go, line 1233 at r6 (raw file):

	RequireNoGQLErrors(t, gqlResponse)
	require.NoError(t, json.Unmarshal([]byte(gqlResponse.Data), &authorCountResp))
	require.True(t, len(authorCountResp.QueryAuthor) > 0)

This could have been simpler had we just done first:1 here. Then we didn't need to count number of authors and construct the response dynamically.

i didn't get the point here, we have to construct response to check if there are equal __typename generated as there are nodes of given type that we count using first query.

graphql/resolve/query_rewriter.go, line 615 at r6 (raw file):

			Attr: "dgraph.type",
		})
	}

Why don't we add the dgraph.uid edge for these cases.

if len(selSet) == 1 && !auth.... && selSet[0].Name() == schema.Typename {
  q.Children = append(q.Children, `dgraph.uid`)
}

Reason being that fetching uid is more performant than fetching dgraph.type and dgraph.type can be inferred in completeObject like we do already?

ok, will change dgraph.typename to dgraph.uid , there is no harm in it. Previously we are adding extra uid's in adduid functions and had issues in tests for nested queries, so we decided to add extra field in addSelectionSetFrom function and modified a code that add dgraph.type already for interface for minimum changes. Will add dgraph.uid here.

[pawanrawal]

Reviewed 2 of 2 files at r7.
Reviewable status: all files reviewed, 16 unresolved discussions (waiting on @abhimanyusinghgaur, @jatindevdg, and @pawanrawal)

---

graphql/e2e/common/query.go, line 1163 at r2 (raw file):

Previously, JatinDevDG (Jatin Dev) wrote…

Tests were there but have other fields with __typename. I just copy and modified them.

why is this commented?

It was just for testing. Uncommented and changed test.

---

graphql/e2e/common/query.go, line 1233 at r6 (raw file):

Previously, JatinDevDG (Jatin Dev) wrote…

i didn't get the point here, we have to construct response to check if there are equal __typename generated as there are nodes of given type that we count using first query.

I was saying that all that we want to check is that the correct __typename is returned. We could just do queryAuthor(first: 1) and that way we would know that we expect only one object with a typename. We don't have to first get the number of author and then construct a response for them like we do below.

yeah, i modified the tests now,. We now add the dummy data and know in advance how many nodes ar there in result. So we can add expected results in advance.

---

graphql/e2e/common/query.go, line 1253 at r7 (raw file):

//
//func onlytypenameForInterface(t *testing.T) {

Why is this commented out?

changed test.

---

graphql/resolve/query_rewriter.go, line 380 at r7 (raw file):

func (authRw *authRewriter) writingAuth() bool {
	if authRw == nil || !authRw.isWritingAuth {

This can just be

return authRw != nil && authRw.isWritingAuth

Done.
__
graphql/resolve/query_rewriter.go, line 613 at r7 (raw file):

			})

		} else if !auth.writingAuth() &&

Can you add a comment about why we don't need this for auth queries?

---

graphql/resolve/query_rewriter.go, line 624 at r7 (raw file):

	}

	selSet = nil

You don't need to set this explicitly.

deleted.

[MichaelJCompton]

Reviewable status: 0 of 3 files reviewed, 15 unresolved discussions (waiting on @abhimanyusinghgaur, @jatindevdg, and @pawanrawal)

[JatinDev543]

Reviewed 2 of 2 files at r7.
Reviewable status: all files reviewed, 16 unresolved discussions (waiting on @abhimanyusinghgaur, @jatindevdg, and @pawanrawal)

graphql/e2e/common/query.go, line 1163 at r2 (raw file):

Previously, JatinDevDG (Jatin Dev) wrote…
why is this commented?

It was just for testing. Uncommented and changed test.

graphql/e2e/common/query.go, line 1233 at r6 (raw file):

Previously, JatinDevDG (Jatin Dev) wrote…
I was saying that all that we want to check is that the correct __typename is returned. We could just do queryAuthor(first: 1) and that way we would know that we expect only one object with a typename. We don't have to first get the number of author and then construct a response for them like we do below.

yeah, i modified the tests now,. We now add the dummy data and know in advance how many nodes are there in result. So we can add expected results in advance.

graphql/e2e/common/query.go, line 1253 at r7 (raw file):

//
//func onlytypenameForInterface(t *testing.T) {

Why is this commented out?

changed it.

graphql/resolve/query_rewriter.go, line 380 at r7 (raw file):

func (authRw *authRewriter) writingAuth() bool {
	if authRw == nil || !authRw.isWritingAuth {

This can just be

return authRw != nil && authRw.isWritingAuth

Done.

__
graphql/resolve/query_rewriter.go, line 613 at r7 (raw file):

			})

		} else if !auth.writingAuth() &&

Can you add a comment about why we don't need this for auth queries?

we don't need this for auth queries because it is us who are forming the auth queries in first place for internal purposes, it is not a user query. Querying it for them would just add an overhead which we can avoid.
Added comment in code also.

graphql/resolve/query_rewriter.go, line 624 at r7 (raw file):

	}

	selSet = nil

You don't need to set this explicitly.

deleted.