feat(GraphQL): Add support for Polygon and Multi-Polygon in GraphQL

[arijitAD]

Fixes GRAPHQL-662
Fixes GRAPHQL-695

This PR adds support for Polygon and MultiPolygon geo types in GraphQL. See RFC for more details.

---

This change is 

Docs Preview:

[abhimanyusinghgaur]

should we also add a test to validate that PointList can't be specified in input schema as a type for some field?

Reviewed 21 of 65 files at r1.
Reviewable status: 21 of 65 files reviewed, 14 unresolved discussions (waiting on @arijitAD, @MichaelJCompton, and @pawanrawal)

---

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

	t.Run("Check cascade with mutation without ID field", checkCascadeWithMutationWithoutIDField)
	t.Run("Geo - Point type", mutationPointType)
	t.Run("Geo - Polygon type", mutationPolygonType)

e2e for MultiPolygon

---

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

			  name
			  location {
				latitude

we should also put __typename check for Point type now that it works

---

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

Quoted 22 lines of code…

  name: "Add mutation with geo without longitude"
  gqlmutation: |
    mutation addHotel($hotel: AddHotelInput!) {
      addHotel(input: [$hotel]) {
        hotel {
          name
          location {
            latitude
            longitude
          }
        }
      }
    }
  gqlvariables: |
    { "hotel":
      { "name": "Taj Hotel",
        "location": { "latitude": 11.11 }
      }
    }
  explanation: "Add mutation for Point type mutation should have both latitude and longitude"
  validationerror:
    { "message": "input: variable.hotel.location.longitude must be defined" }

seems we removed this test, should we keep it?
If it is from gqlparser, then we can skip this test, because this check is enforced through our schema.

---

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

makePolygon

rewritePolygon

---

graphql/resolve/query_rewriter.go, line 1127 at r1 (raw file):

Quoted 5 lines of code…

					distance := geoParams["distance"]
					coordinate, _ := geoParams["coordinate"].(map[string]interface{})
					if coordinate == nil {
						return nil
					}

I was thinking that we can skip this nil check for coordinate like we have done for distance, because according to our schema it is non-null. So, if someone gives null or any other value that doesn't conform to our schema, then it should be caught at parser level itself. So, here it should always be a map.

---

graphql/resolve/query_rewriter.go, line 1140 at r1 (raw file):

Quoted 4 lines of code…

					polygon, _ := geoParams["polygon"].(map[string]interface{})
					if polygon == nil {
						return nil
					}

can skip this check because of schema validations by parser.

---

graphql/resolve/query_rewriter.go, line 1162 at r1 (raw file):

					} else if point, ok := geoParams["point"].(map[string]interface{}); ok {
						res = writePoint(point)
					}

the other case for this where both point and polygon are null in this would be taken care of after my PR for unions is merged with @oneOf directive. Which will ensure that always one of the fields should be there.

---

graphql/resolve/query_rewriter.go, line 1169 at r1 (raw file):

contains

intersects

---

graphql/resolve/query_rewriter.go, line 1170 at r1 (raw file):

coordinates:

polygons:

---

graphql/resolve/query_test.yaml, line 296 at r1 (raw file):

      }
    }

rewriting tests for MultiPolygon are missing

---

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

// It returns a bracketed json object like { "longitude" : 12.32 , "latitude" : 123.32 }.
func completeGeoObject(field schema.Field, val map[string]interface{}, path []interface{}) ([]byte, x.GqlErrorList) {

comment can be updated to reflect the new behaviour

---

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

	typ, _ := val["type"].(string)
	var buf bytes.Buffer
	var err x.GqlErrorList

we are not using this, can be removed

---

graphql/resolve/update_mutation_test.yaml, line 135 at r1 (raw file):

set

delete

---

graphql/resolve/update_mutation_test.yaml, line 140 at r1 (raw file):

Quoted 8 lines of code…

    - deletejson: |
        { "uid" : "uid(x)",
          "Hotel.area": {
            "type": "Polygon",
            "coordinates": [[[22.22,11.11],[16.16,15.15],[21.21,20.2]],[[22.28,11.18],[16.18,15.18],[21.28,20.28]]]
          }
        }
      cond: "@if(gt(len(x), 0))"

should we add an e2e test to check if this rewriting works for remove case?

---

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

Quoted 17 lines of code…

  -
    name: "custom field shouldn't be part of dgraph schema"
    input: |
      type User {
        id: ID!
        name: String!
        bio: String! @lambda
        friends: [User] @custom(http: {
          url: "http://mock:8888/users",
          method: "GET"
        })
      }
    output: |
      type User {
        User.name
      }
      User.name: string .

seems this test got removed in merge

[abhimanyusinghgaur]

Reviewable status: 2 of 66 files reviewed, all discussions resolved (waiting on @abhimanyusinghgaur, @MichaelJCompton, and @pawanrawal)

---

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

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

e2e for MultiPolygon

Done

---

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

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

we should also put __typename check for Point type now that it works

Done

---

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

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

  name: "Add mutation with geo without longitude"
  gqlmutation: |
    mutation addHotel($hotel: AddHotelInput!) {
      addHotel(input: [$hotel]) {
        hotel {
          name
          location {
            latitude
            longitude
          }
        }
      }
    }
  gqlvariables: |
    { "hotel":
      { "name": "Taj Hotel",
        "location": { "latitude": 11.11 }
      }
    }
  explanation: "Add mutation for Point type mutation should have both latitude and longitude"
  validationerror:
    { "message": "input: variable.hotel.location.longitude must be defined" }

seems we removed this test, should we keep it?
If it is from gqlparser, then we can skip this test, because this check is enforced through our schema.

It is from gqlparser, can be skipped.

---

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

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

makePolygon

rewritePolygon

Done

---

graphql/resolve/query_rewriter.go, line 1127 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

					distance := geoParams["distance"]
					coordinate, _ := geoParams["coordinate"].(map[string]interface{})
					if coordinate == nil {
						return nil
					}

I was thinking that we can skip this nil check for coordinate like we have done for distance, because according to our schema it is non-null. So, if someone gives null or any other value that doesn't conform to our schema, then it should be caught at parser level itself. So, here it should always be a map.

Done

---

graphql/resolve/query_rewriter.go, line 1140 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

					polygon, _ := geoParams["polygon"].(map[string]interface{})
					if polygon == nil {
						return nil
					}

can skip this check because of schema validations by parser.

Done

---

graphql/resolve/query_rewriter.go, line 1169 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

contains

intersects

Done

---

graphql/resolve/query_rewriter.go, line 1170 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

coordinates:

polygons:

Done

---

graphql/resolve/query_test.yaml, line 296 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

rewriting tests for MultiPolygon are missing

Done

---

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

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

// It returns a bracketed json object like { "longitude" : 12.32 , "latitude" : 123.32 }.
func completeGeoObject(field schema.Field, val map[string]interface{}, path []interface{}) ([]byte, x.GqlErrorList) {

comment can be updated to reflect the new behaviour

Done

---

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

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

we are not using this, can be removed

Done

---

graphql/resolve/update_mutation_test.yaml, line 135 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

set

delete

Done

---

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

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

  -
    name: "custom field shouldn't be part of dgraph schema"
    input: |
      type User {
        id: ID!
        name: String!
        bio: String! @lambda
        friends: [User] @custom(http: {
          url: "http://mock:8888/users",
          method: "GET"
        })
      }
    output: |
      type User {
        User.name
      }
      User.name: string .

seems this test got removed in merge

Done

[pawanrawal]

Reviewed 2 of 65 files at r1, 64 of 64 files at r2.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @arijitAD and @MichaelJCompton)

---

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

func rewritePolygon(val map[string]interface{}) []interface{} {
	var resPoly []interface{}
	for _, pointList := range val[schema.Coordinates].([]interface{}) {

I suppose this typecasting to []interface{} is safe to do and val[Coordinates] would never be nil?

Also maybe we can initialize these slices with the correct length in the first place to optimize for memory.

---

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

	for _, pointList := range val[schema.Coordinates].([]interface{}) {
		var resPointList []interface{}
		for _, point := range pointList.(map[string]interface{})[schema.Points].([]interface{}) {

again, is this all safe because of the strict GraphQL schema?

---

graphql/resolve/query_rewriter.go, line 1162 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

the other case for this where both point and polygon are null in this would be taken care of after my PR for unions is merged with @oneOf directive. Which will ensure that always one of the fields should be there.

Add comments about the current behavior and what should we do in the future to tackle this properly and also create a JIRA issue about this.

---

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

    				polygon: {
    					coordinates: [{
    						points: [{

tabs looks strange here

---

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

    			intersects: {
    				polygon: {
    					coordinates: [{

tabs should be fixed

---

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

              coordinates {
                points {
                  latitude

nice bunch of test cases here

---

graphql/schema/testdata/schemagen/input/geo-type.graphql, line 5 at r2 (raw file):

    name: String!
    location: Point @search
    area: Polygon @search

Can we add fields for geo types where @search is not there.

[abhimanyusinghgaur]

Reviewable status: 61 of 66 files reviewed, 6 unresolved discussions (waiting on @abhimanyusinghgaur, @arijitAD, @MichaelJCompton, and @pawanrawal)

---

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

Previously, pawanrawal (Pawan Rawal) wrote…

I suppose this typecasting to []interface{} is safe to do and val[Coordinates] would never be nil?

Also maybe we can initialize these slices with the correct length in the first place to optimize for memory.

Yeah! Done.

---

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

Previously, pawanrawal (Pawan Rawal) wrote…

again, is this all safe because of the strict GraphQL schema?

Yeah

---

graphql/resolve/query_rewriter.go, line 1162 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Add comments about the current behavior and what should we do in the future to tackle this properly and also create a JIRA issue about this.

Done

---

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

Previously, pawanrawal (Pawan Rawal) wrote…

tabs looks strange here

Done

---

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

Previously, pawanrawal (Pawan Rawal) wrote…

tabs should be fixed

Done

---

graphql/schema/testdata/schemagen/input/geo-type.graphql, line 5 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Can we add fields for geo types where @search is not there.

Done