feat(GraphQL): Add support for Geo point type in Graphql. #6481

arijitAD · 2020-09-17T13:27:47Z

Fixes GRAPHQL-682

This change is

Docs Preview:

…type

abhimanyusinghgaur

We should add a rule to mark Point as a reserved type name, I guess adding Pointto isReservedKeyWord function in rules.go and also to forbiddenInputTypeNames inside inputTypeNameValidation function should suffice. And test for the same that one can't add a type named Point in their schema.

Reviewed 20 of 63 files at r1.
Reviewable status: 20 of 63 files reviewed, 7 unresolved discussions (waiting on @arijitAD, @MichaelJCompton, and @pawanrawal)

graphql/resolve/add_mutation_test.yaml, line 21 at r1 (raw file):

A uid and type should get injected and all data transformed to
    underlying Dgraph edge names

stale message

graphql/resolve/resolver.go, line 1252 at r1 (raw file):

Quoted 5 lines of code…

if field.ResponseName() == "longitude" {
			x.Check2(buf.WriteString(longitude))
		} else if field.ResponseName() == "latitude" {
			x.Check2(buf.WriteString(latitude))
		}

we sohuld convert this part to a switch case.

graphql/resolve/resolver.go, line 1255 at r1 (raw file):

Quoted 7 lines of code…

else {
			return nil, x.GqlErrorList{&x.GqlError{
				Message:   "Invalid field for Geo type",
				Locations: []x.Location{field.Location()},
				Path:      copyPath(path),
			}}
		}

we will never get this else case, because we didn't define any other fields in the schema for Point type. If someone tried that, it would fail at parser level validation itself.

graphql/schema/dgraph_schemagen_test.yml, line 616 at r1 (raw file):

        id: ID!
        name: String!
        location: Point @search

can we add another Point type field without @search, so the generated dgraph schema should not have @index for that.

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

// If there's no arg, then it can be an enum or has to be a scalar that's
		// not ID. The schema generation will add the default search
		// for that type.

comment can be updated now

graphql/schema/wrappers_test.go, line 135 at r1 (raw file):

Quoted 4 lines of code…

"Point": map[string]string{
			"latitude":  "Point.latitude",
			"longitude": "Point.longitude",
		},

We shouldn't need this test, because dgraphPredicate for Point doesn't make sense, i.e., we don't use these names to rewrite the point type. So, we shouldn't generate dgraphPredicate for Point type.

graphql/schema/wrappers_test.go, line 262 at r1 (raw file):

Quoted 4 lines of code…

"Point": map[string]string{
			"latitude":  "Point.latitude",
			"longitude": "Point.longitude",
		},

same for this test, it shouldn't be required.

arijitAD

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

graphql/resolve/add_mutation_test.yaml, line 21 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

A uid and type should get injected and all data transformed to
    underlying Dgraph edge names

stale message

Done.

graphql/resolve/resolver.go, line 1252 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

if field.ResponseName() == "longitude" {
			x.Check2(buf.WriteString(longitude))
		} else if field.ResponseName() == "latitude" {
			x.Check2(buf.WriteString(latitude))
		}

we sohuld convert this part to a switch case.

Done.

graphql/resolve/resolver.go, line 1255 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

else {
			return nil, x.GqlErrorList{&x.GqlError{
				Message:   "Invalid field for Geo type",
				Locations: []x.Location{field.Location()},
				Path:      copyPath(path),
			}}
		}
we will never get this else case, because we didn't define any other fields in the schema for Point type. If someone tried that, it would fail at parser level validation itself.

Done.

graphql/schema/dgraph_schemagen_test.yml, line 616 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

can we add another Point type field without @search, so the generated dgraph schema should not have @index for that.

Done.

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

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

// If there's no arg, then it can be an enum or has to be a scalar that's
		// not ID. The schema generation will add the default search
		// for that type.

comment can be updated now

Done.

graphql/schema/wrappers_test.go, line 135 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

"Point": map[string]string{
			"latitude":  "Point.latitude",
			"longitude": "Point.longitude",
		},
We shouldn't need this test, because dgraphPredicate for Point doesn't make sense, i.e., we don't use these names to rewrite the point type. So, we shouldn't generate dgraphPredicate for Point type.

Done.

graphql/schema/wrappers_test.go, line 262 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

"Point": map[string]string{
			"latitude":  "Point.latitude",
			"longitude": "Point.longitude",
		},
same for this test, it shouldn't be required.

Done.

…type

pawanrawal

Reviewed 52 of 63 files at r1, 14 of 14 files at r2.
Reviewable status: all files reviewed, 17 unresolved discussions (waiting on @abhimanyusinghgaur, @arijitAD, and @MichaelJCompton)

graphql/e2e/common/mutation.go, line 3752 at r2 (raw file):

func mutationGeoType(t *testing.T) {
	addHotelParams := &GraphQLParams{
		Query: `

extra spaces

graphql/e2e/directives/schema.graphql, line 7 at r2 (raw file):

    id: ID!
    name: String! @search(by: [exact])
    location: Point @search

Are we also doing a query to search based on a point? I don't see any e2e test or unit test for a query.

graphql/e2e/directives/schema_response.json, line 4 at r2 (raw file):

    "schema": [
        {
            "predicate": "dgraph.graphql.schema_history",

This file has a bit too much changed to see what the actual diff is. How do you get this? I usually print and copy the generated schema and then format it using VSCode editor.

graphql/e2e/schema/generatedSchema.graphql, line 81 at r2 (raw file):

}

input PointRef {

No different between Point and PointRef. Is there a reason we need both?

graphql/resolve/add_mutation_test.yaml, line 18 at r2 (raw file):

    { "hotel":
      { "name": "Taj Hotel",
        "location": { "latitude": 11.11 , "longitude" : 22.22}

If only one of long/lat is given, I hope that is an error. Is there a test for the error message returned in that case?

graphql/resolve/mutation_rewriter.go, line 1214 at r2 (raw file):

				// { "title": "...", "author": { "username": "new user", "dob": "...", ... }
				//          like here ^^
				if fieldDef.Type().IsGeo() {

Should this be IsGeo or IsPoint? Later we'll have other things with IsGeo as well.

graphql/resolve/mutation_rewriter.go, line 1223 at r2 (raw file):

							newFragment(
								map[string]interface{}{
									"type":        "Point",

I suppose this is required for Dgraph to be able to do the filtering right? Please add a comment if that is the case here.

graphql/resolve/query_test.go, line 59 at r2 (raw file):

	for _, tcase := range tests {
		t.Run(tcase.Name, func(t *testing.T) {

revert please

graphql/resolve/resolver.go, line 1236 at r2 (raw file):

	x.Check2(buf.WriteRune('{'))

	coordinate, _ := val["coordinates"].([]interface{})

What if the value returned is null? Should we just return null then? Imagine user running this over an existing Dgraph database in which some values are null for some coordinates?

graphql/schema/dgraph_schemagen_test.yml, line 616 at r2 (raw file):

        id: ID!
        name: String!
        location: Point @search

What happens if I have @search(by: [int]) on a Point type? I hope that fails by the correct error message?

…type

arijitAD

Reviewable status: 52 of 62 files reviewed, 17 unresolved discussions (waiting on @abhimanyusinghgaur, @MichaelJCompton, and @pawanrawal)

graphql/e2e/common/mutation.go, line 3752 at r2 (raw file):