Feat(GraphQL): This PR allows to return errors from custom REST endpoint.

[JatinDev543]

Fixes GRAPHQL-359

Returned errors are parsed in type GqlErrorList []*GqlError where GqlError is

type GqlError struct {
        Message     string                   `json:"message"`
	Locations   []Location               `json:"locations,omitempty"`
	Path        []interface{}            `json:"path,omitempty"`
	Extensions  map[string]interface{}   `json:"extensions,omitempty"`
}

Example:
{"errors":[{"message": "Rest API returns Error for myFavoriteMovies query","locations": [ { "line": 5, "column": 4 } ],"path": ["Movies","name"]}]}
`

---

This change is 

Docs Preview:

[abhimanyusinghgaur]

Thinking about it, I feel a REST API when returning correct results, can give one of four kinds of data:

1. Scalar
2. Object
3. List of scalar
4. List of Object

What we are doing at present is, for every case we are expecting error in the following form from the API:

{ "errors": [{"message": "some error message"}]}

This seems best fit for case 2. For case 4, people may be inclined to give errors for every object in the list, so should we expect something like this for case 4?

[{ "errors": [{"message": "obj1 error"}]}, { "errors": [{"message": "obj2 error"}]}]

For case 1 and 3, I don't have anything in mind, other than reusing these formats.

Should we do the above, or should we just continue with the current behaviour?

Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @jatindevdg, @MichaelJCompton, and @pawanrawal)

---

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

type RESTErr struct{
			Errors x.GqlErrorList `json:"errors,omitempty"`
		}

We should declare this type outside of the function, if we are going to use it.

[JatinDev543]

Thinking about it, I feel a REST API when returning correct results, can give one of four kinds of data:

1. Scalar
2. Object
3. List of scalar
4. List of Object

What we are doing at present is, for every case we are expecting error in the following form from the API:

{ "errors": [{"message": "some error message"}]}

This seems best fit for case 2. For case 4, people may be inclined to give errors for every object in the list, so should we expect something like this for case 4?

[{ "errors": [{"message": "obj1 error"}]}, { "errors": [{"message": "obj2 error"}]}]

For case 1 and 3, I don't have anything in mind, other than reusing these formats.

Should we do the above, or should we just continue with the current behaviour?

Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @jatindevdg, @MichaelJCompton, and @pawanrawal)

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

type RESTErr struct{
			Errors x.GqlErrorList `json:"errors,omitempty"`
		}

We should declare this type outside of the function, if we are going to use it.

We are returning an error list for custom mutation/query or field. For the custom field if one field return error then other fields still return the response.

If we want to return multiple errors then we can put them in our error lists as follows
[{ "errors": [{"message": "obj1 error"},{"message": "obj2 error"}]}]

If there are multiple custom fields returning error then it can be in below format
{[{ "errors": [{"message": "obj1 error field 1"},{"message": "obj2 error field 1"}]}],[{ "errors": [{"message": "obj1 error field2"}]}]}

[pawanrawal]

I think we should continue with the present behaviour. We have to just decide a format and say that we expect the errors in that format in the docs.

In GraphQL terms, the errors contain paths which are the path of a field. So for us, while resolving a custom field, we should just add the path of the field for which the error is encountered ideally.

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

---

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

		var errs error
		var result []interface{}
		type RESTErr struct{

restErr because no need for it to be exported

---

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

Previously, JatinDevDG (Jatin Dev) wrote…

We are returning an error list for custom mutation/query or field. For the custom field if one field return error then other fields still return the response.

If we want to return multiple errors then we can put them in our error lists as follows [{ "errors": [{"message": "obj1 error"},{"message": "obj2 error"}]}]

If there are multiple custom fields returning error then it can be in below format {[{ "errors": [{"message": "obj1 error field 1"},{"message": "obj2 error field 1"}]}],[{ "errors": [{"message": "obj1 error field2"}]}]}

All the errors should be returned inside an errors key` and ideally they should the path as well but skip the path for now. See https://github.com/graphql/graphql-spec/blob/master/spec/Section%207%20--%20Response.md

---

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

			}
		} else if err := json.Unmarshal(b, &customErr); err == nil && len(customErr.Errors)!=0 {
			errCh<-customErr.Errors

no gofmt?

---

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

					return
				}
			} else if err := json.Unmarshal(b, &customErr); err == nil && len(customErr.Errors)!=0 {

Actually club this and the condition below into the same else block

if graphql {
} else {
// REST case

// Now unmarshal based on status code
// If response.StatusCode >= 200 && response.StatusCode < 300
// unmarshal into result
// else into errors

}

[JatinDev543]

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

---

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

Previously, pawanrawal (Pawan Rawal) wrote…

restErr because no need for it to be exported

done.

---

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

Previously, pawanrawal (Pawan Rawal) wrote…

All the errors should be returned inside an errors key` and ideally they should the path as well but skip the path for now. See https://github.com/graphql/graphql-spec/blob/master/spec/Section%207%20--%20Response.md

ok.

---

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

Previously, pawanrawal (Pawan Rawal) wrote…

no gofmt?

done.

---

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

Previously, pawanrawal (Pawan Rawal) wrote…

Actually club this and the condition below into the same else block

if graphql {
} else {
// REST case

// Now unmarshal based on status code
// If response.StatusCode >= 200 && response.StatusCode < 300
// unmarshal into result
// else into errors

}

changed.

[JatinDev543]

Path is already available in returned errors because it's of type gqlErrorList. Added it in one of the test.

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

[pawanrawal]

Looking mostly good, just needs some small formatting changes.

Reviewed 1 of 3 files at r2.
Reviewable status: 1 of 3 files reviewed, 9 unresolved discussions (waiting on @abhimanyusinghgaur, @jatindevdg, and @MichaelJCompton)

---

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

Previously, JatinDevDG (Jatin Dev) wrote…

changed.

similar comment as above

---

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

		var errs error
		var result []interface{}
		var customErr restErr

var rerr restErr

---

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

				return
			}
		} else if status >= 200 && status < 300 {

} else {
  // This must be a REST call

  if status >=200 && status < 300 {
  } else {
  }
}

Format it like this to make it clear that the status based handling is only for REST.

---

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

		} else {
			if err = json.Unmarshal(b, &customErr); err != nil {
				err = errors.Errorf("unexpected Error with status code: %v", status)

unexpected error with

---

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

			var result interface{}
			var customErr restErr

var rerr restErr

---

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

			} else {
				if err = json.Unmarshal(b, &customErr); err != nil {
					err = errors.Errorf("unexpected Error with status code: %v", status)

unexpected error

---

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

	if hrc.RemoteGqlQueryName == "" {
		var result interface{}
		var customErr restErr

var rerr restErr

---

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

			}
		} else if err := json.Unmarshal(b, &customErr); err != nil {
			err = errors.Errorf("unexpected Error with status code: %v", status)

unexpected error

[pawanrawal]

Reviewed 1 of 3 files at r2, 2 of 2 files at r3.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @abhimanyusinghgaur, @jatindevdg, and @MichaelJCompton)

[JatinDev543]

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

---

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

Previously, pawanrawal (Pawan Rawal) wrote…

similar comment as above

done.

---

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

Previously, pawanrawal (Pawan Rawal) wrote…

var rerr restErr

done.

---

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

Previously, pawanrawal (Pawan Rawal) wrote…

} else {
  // This must be a REST call

  if status >=200 && status < 300 {
  } else {
  }
}

Format it like this to make it clear that the status based handling is only for REST.

done.

---

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

Previously, pawanrawal (Pawan Rawal) wrote…

unexpected error with

done.

---

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

Previously, pawanrawal (Pawan Rawal) wrote…

var rerr restErr

done.

---

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

Previously, pawanrawal (Pawan Rawal) wrote…

unexpected error

done.

---

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

Previously, pawanrawal (Pawan Rawal) wrote…

var rerr restErr

done.

---

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

Previously, pawanrawal (Pawan Rawal) wrote…

unexpected error

done.