fix(graphql): adding support for @id with type other than strings

[aman-bansal]

This PR fixes GRAPHQL-762. Earlier we only can define String! datatypes with @id directive. This enables user to define @id directive with Int, Int64, Float datatypes too.

---

This change is 

[netlify]

Deploy preview for dgraph-docs ready!

Built with commit 107fb6c

https://deploy-preview-7019--dgraph-docs.netlify.app

[abhimanyusinghgaur]

In addition to e2e, we should also add mutation rewriting test cases for these.
Some common practices in tests:

• Data added in a test should be cleaned up at the end of the test (see DeleteGqlType in common/mutaiton.go)
• using RequireNoGQLErrors(t, response) instead of require.Nil(t, response.Errors)
• using testutil.CompareJSON() for comparing the expected output of query

Reviewed 16 of 16 files at r1.
Reviewable status: all files reviewed, 14 unresolved discussions (waiting on @aman-bansal, @MichaelJCompton, and @pawanrawal)

---

graphql/e2e/common/mutation.go, line 4598 at r1 (raw file):

require.Nil(t, response.Errors)

RequireNoGQLErrors(t, response)

---

graphql/e2e/common/mutation.go, line 4607 at r1 (raw file):

Quoted 6 lines of code…

	b, _ := response.Data.MarshalJSON()
	expBuffer := new(bytes.Buffer)
	require.NoError(t, json.Compact(expBuffer, []byte(expected)))
	actBuffer := new(bytes.Buffer)
	require.NoError(t, json.Compact(actBuffer, b))
	require.Equal(t, expBuffer.String(), actBuffer.String())

testutil.CompareJSON(t, expected, string(response.Data))

---

graphql/e2e/common/mutation.go, line 4614 at r1 (raw file):

	response = query.ExecuteAsPost(t, GraphqlURL)
	require.Contains(t, response.Errors.Error(), "already exists")
}

cleanup the added data at the end of each test, there is a generic function deleteGqlType for helping with that.

---

graphql/e2e/common/mutation.go, line 4630 at r1 (raw file):

Quoted 4 lines of code…

	if response.Errors != nil {
		log.Print("error from servce is", response.Errors.Error())
	}
	require.Nil(t, response.Errors)

RequireNoGQLErrors(t, response)

---

graphql/e2e/common/mutation.go, line 4643 at r1 (raw file):

Quoted 6 lines of code…

	b, _ := response.Data.MarshalJSON()
	expBuffer := new(bytes.Buffer)
	require.NoError(t, json.Compact(expBuffer, []byte(expected)))
	actBuffer := new(bytes.Buffer)
	require.NoError(t, json.Compact(actBuffer, b))
	require.Equal(t, expBuffer.String(), actBuffer.String())

testutil.CompareJSON

---

graphql/e2e/common/mutation.go, line 4670 at r1 (raw file):

require.Nil(t, response.Errors)

RequireNoGQLErrors(t, response)

---

graphql/e2e/common/mutation.go, line 4678 at r1 (raw file):

Quoted 6 lines of code…

	b, _ := response.Data.MarshalJSON()
	expBuffer := new(bytes.Buffer)
	require.NoError(t, json.Compact(expBuffer, []byte(expected)))
	actBuffer := new(bytes.Buffer)
	require.NoError(t, json.Compact(actBuffer, b))
	require.Equal(t, expBuffer.String(), actBuffer.String())

testutil.CompareJSON

---

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

queryBook

can we instead use getBook type of queries for all these query tests, because an @id field should be allowed from get queries.

---

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

require.Nil(t, response.Errors)

RequireNoGQLErrors(t, response)

---

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

Quoted 6 lines of code…

	b, _ := response.Data.MarshalJSON()
	expBuffer := new(bytes.Buffer)
	require.NoError(t, json.Compact(expBuffer, []byte(expected)))
	actBuffer := new(bytes.Buffer)
	require.NoError(t, json.Compact(actBuffer, b))
	require.Equal(t, expBuffer.String(), actBuffer.String())

testutil.CompareJSON

---

graphql/resolve/mutation_rewriter.go, line 998 at r1 (raw file):

"Int!"

.Name() will only return Int
.String() would give Int!
So, we can skip mentioning Int!, Float!, ... cases here.

---

graphql/resolve/mutation_rewriter.go, line 999 at r1 (raw file):

xidVal.(int64)

should we also do an ok check for these conversions to int or float like we are doing for String?

---

graphql/resolve/mutation_rewriter.go, line 1003 at r1 (raw file):

			case "Int64!", "Int64":
				fallthrough

we can skip mentioning this, it would anyways fall to default case

---

graphql/schema/rules.go, line 2001 at r1 (raw file):

"Type %s; Field %s: with @id directive must be of type String!, not %s"

... of type String!, Int!, Int64 or Float!

[aman-bansal]

Reviewable status: 12 of 18 files reviewed, 14 unresolved discussions (waiting on @abhimanyusinghgaur, @MichaelJCompton, and @pawanrawal)

---

graphql/e2e/common/mutation.go, line 4598 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

require.Nil(t, response.Errors)

RequireNoGQLErrors(t, response)

Done.

---

graphql/e2e/common/mutation.go, line 4607 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

	b, _ := response.Data.MarshalJSON()
	expBuffer := new(bytes.Buffer)
	require.NoError(t, json.Compact(expBuffer, []byte(expected)))
	actBuffer := new(bytes.Buffer)
	require.NoError(t, json.Compact(actBuffer, b))
	require.Equal(t, expBuffer.String(), actBuffer.String())

testutil.CompareJSON(t, expected, string(response.Data))

I guess there is no need. require.JSONEq does map based matching. Any special case I am missing here?

---

graphql/e2e/common/mutation.go, line 4614 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

cleanup the added data at the end of each test, there is a generic function deleteGqlType for helping with that.

Done.

---

graphql/e2e/common/mutation.go, line 4630 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

	if response.Errors != nil {
		log.Print("error from servce is", response.Errors.Error())
	}
	require.Nil(t, response.Errors)

RequireNoGQLErrors(t, response)

Done.

---

graphql/e2e/common/mutation.go, line 4643 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

	b, _ := response.Data.MarshalJSON()
	expBuffer := new(bytes.Buffer)
	require.NoError(t, json.Compact(expBuffer, []byte(expected)))
	actBuffer := new(bytes.Buffer)
	require.NoError(t, json.Compact(actBuffer, b))
	require.Equal(t, expBuffer.String(), actBuffer.String())

testutil.CompareJSON

Done.

---

graphql/e2e/common/mutation.go, line 4670 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

require.Nil(t, response.Errors)

RequireNoGQLErrors(t, response)

Done.

---

graphql/e2e/common/mutation.go, line 4678 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

	b, _ := response.Data.MarshalJSON()
	expBuffer := new(bytes.Buffer)
	require.NoError(t, json.Compact(expBuffer, []byte(expected)))
	actBuffer := new(bytes.Buffer)
	require.NoError(t, json.Compact(actBuffer, b))
	require.Equal(t, expBuffer.String(), actBuffer.String())

testutil.CompareJSON

Done.

---

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

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

queryBook

can we instead use getBook type of queries for all these query tests, because an @id field should be allowed from get queries.

Good catch. It's Done.

---

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

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

require.Nil(t, response.Errors)

RequireNoGQLErrors(t, response)

Done.

---

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

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

	b, _ := response.Data.MarshalJSON()
	expBuffer := new(bytes.Buffer)
	require.NoError(t, json.Compact(expBuffer, []byte(expected)))
	actBuffer := new(bytes.Buffer)
	require.NoError(t, json.Compact(actBuffer, b))
	require.Equal(t, expBuffer.String(), actBuffer.String())

testutil.CompareJSON

Done.

---

graphql/resolve/mutation_rewriter.go, line 998 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

"Int!"

.Name() will only return Int
.String() would give Int!
So, we can skip mentioning Int!, Float!, ... cases here.

Done.

---

graphql/resolve/mutation_rewriter.go, line 999 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

xidVal.(int64)

should we also do an ok check for these conversions to int or float like we are doing for String?

Done.

---

graphql/resolve/mutation_rewriter.go, line 1003 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

			case "Int64!", "Int64":
				fallthrough

we can skip mentioning this, it would anyways fall to default case

Its a clean way in my opinion. We are handling Int64 as special case. This makes it known that Int64 is handled in different way.

---

graphql/schema/rules.go, line 2001 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

"Type %s; Field %s: with @id directive must be of type String!, not %s"

... of type String!, Int!, Int64 or Float!

Done.

[pawanrawal]

Reviewable status: 12 of 18 files reviewed, 15 unresolved discussions (waiting on @abhimanyusinghgaur, @aman-bansal, @MichaelJCompton, and @pawanrawal)

---

graphql/resolve/query_test.yaml, line 871 at r2 (raw file):

  dgquery: |-
    query {
      queryAuthor(func: type(Author)) @filter((le(Author.dob, "2001-01-01") AND eq(Author.name, "A. N. Author") AND gt(Author.reputation, "2.5"))) {

Can you add a unit test for the float issue that you are talking about?

[aman-bansal]

Reviewable status: 11 of 18 files reviewed, 15 unresolved discussions (waiting on @abhimanyusinghgaur, @MichaelJCompton, and @pawanrawal)

---

graphql/resolve/query_test.yaml, line 871 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Can you add a unit test for the float issue that you are talking about?

done

[pawanrawal]

We should also add some query and mutation rewriting unit tests for the new types that now support ID.

Reviewed 2 of 6 files at r2.
Reviewable status: 13 of 18 files reviewed, 16 unresolved discussions (waiting on @abhimanyusinghgaur, @aman-bansal, @MichaelJCompton, and @pawanrawal)

---

graphql/e2e/common/mutation.go, line 4597 at r3 (raw file):

	response := query.ExecuteAsPost(t, GraphqlURL)
	RequireNoGQLErrors(t, response)
	var expected = `{

A shorthand notation is more common unless you want to explicitly define the type.

expected := `
...
`

---

graphql/schema/wrappers.go, line 1064 at r3 (raw file):

		var ok bool
		var xidArgVal string
		switch f.ArgValue(xidArgName).(type) {

You can also get the typecasted value v here and then you can use that directly for int64 and float64 case.

[aman-bansal]

Reviewable status: 5 of 18 files reviewed, 16 unresolved discussions (waiting on @abhimanyusinghgaur, @MichaelJCompton, and @pawanrawal)

---

graphql/e2e/common/mutation.go, line 4597 at r3 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

A shorthand notation is more common unless you want to explicitly define the type.

expected := `
...
`

oh i started with different approach. forgot to change that. thanks

---

graphql/schema/wrappers.go, line 1064 at r3 (raw file):

way in my opinion. We are handling Int64 as special case. This makes it known that Int64 is handled in different way.
oh yes right! Done

[pawanrawal]

Reviewed 5 of 16 files at r1, 13 of 13 files at r4.
Reviewable status: all files reviewed, 14 unresolved discussions (waiting on @abhimanyusinghgaur and @MichaelJCompton)