[client] response errors sometimes not made available

[AlistairB]

Hi,

When using the updateCheckRun mutation on github graphql api in one case I get the following response.

{
    "errors": [
      {
        "locations": [
          {
            "line": 3,
            "column": 7
          }
        ],
        "path": [
          "updateCheckRun"
        ],
        "message": "Could not resolve to a node with the global id of 'MDg6Q2hlY2tSdW4xNzk2OTI3MjMx'.",
        "type": "NOT_FOUND"
      }
    ],
    "data": {
      "updateCheckRun": null
    }
  }

The relevant graphql for this is:

type Mutation {
    updateCheckRun(input: UpdateCheckRunInput!): UpdateCheckRunPayload
}

As per this definition "updateCheckRun": null is a perfectly valid response and will json decode correctly. As such, morpheus assumes that everything is fine, however there are in fact errors. I believe the reason UpdateCheckRunPayload is nullable is because in the error case it won't be returned.

So I guess the assumption that a successful decode of the response = no errors is incorrect. ie. https://github.com/morpheusgraphql/morpheus-graphql/blob/master/morpheus-graphql-client/src/Data/Morpheus/Client/Fetch.hs#L63 perhaps this success case should also pattern match on an empty error list.

Or you might want 3 responses of, success, success with errors, parse failure.

I'd happy to attempt a PR if you know what the ideal solution is.

[nalchevanidze]

hi @AlistairB, thank you i would be happy if you open a PR.

So I guess the assumption that a successful decode of the response = no errors is incorrect. ie. https://github.com/morpheusgraphql/morpheus-graphql/blob/master/morpheus-graphql-client/src/Data/Morpheus/Client/Fetch.hs#L63 perhaps this success case should also pattern match on an empty error list.

i think official interface should stay same. However, decode must check errors and fail if there are any (like you suggested).

  fetch :: (Monad m, FromJSON a) => (ByteString -> m ByteString) -> Args a -> m (Either String a)

Or you might want 3 responses of, success, success with errors, parse failure.

For this case we could add an additional operation fetchPartial . however I am not necessarily convinced of this.

[AlistairB]

Please ignore for now David I know you are busy! I just spent some time trying to integrate my PR into my project and had some more thoughts which I wanted to record.

There are 3 cases with the github graphql API

1. Successful fetch with data
2. Missing result.

{
  "data": {
    "repository": null
  },
  "errors": [
    {
      "type": "NOT_FOUND",
      "path": [
        "repository"
      ],
      "locations": [
        {
          "line": 7,
          "column": 3
        }
      ],
      "message": "Could not resolve to a Repository with the name 'facebook/reacttt'."
    }
  ]
}

3. Exception / Bad Request

{
  "errors": [
    {
      "path": [
        "query",
        "repository",
        "namee"
      ],
      "extensions": {
        "code": "undefinedField",
        "typeName": "Repository",
        "fieldName": "namee"
      },
      "locations": [
        {
          "line": 8,
          "column": 4
        }
      ],
      "message": "Field 'namee' doesn't exist on type 'Repository'"
    }
  ]
}

I assume exception is similar but not sure how to reproduce. Actually, looking at https://graphql.org/learn/serving-over-http/#response it seems that in the exception case you may get the data field, so it could be more case 2.

---

I think in some ways the current functionality is nice for the not found case. The issue is there may be other errors as well when you get a null result. Additionally, in some cases not found is very normal and expected (fetch random repository), but in other cases (update the 'CheckRun' I created) it is very unexpected and exceptional.

I would suggest the following for fetch.

fetch :: (Monad m, FromJSON a) => (ByteString -> m ByteString) -> Args a -> m (Either FetchError ([GQLError], a))

data FetchError = 
  ResultParseFailure String
  | NoResultWithErrors (NonEmpty GQLError)

As such in the 3 above cases you get (vaguely sketched):

1. Success - Right ([], aWithJustInside)
2. Not Found - Right ([GQLError not found], aWithNothing)
3. Error - Left (NoResultWithErrors (GQLErrors))

Then you also have the parse failure result which will also be a Left (ResultParseFailure "Unexpected blah")

In the first 1 my code will sanity check there are no errors and happily use the a.

In the second case:

• if a not found error is normal, I will check the GQLError is NOT_FOUND and just ignore it.
• if a not found error is unexpected, I will log the GQLError and go into exception handling mode

In the third / left cases:

• I will log the error and go into exception handling mode.

---

I guess as part of this you would need to add type to GQLError, although this appears to be non-standard. There might be another way to expose it / the full error.

[nalchevanidze]

HI @AlistairB. i think that Right must be only for Succesfull cases.
therefore the type mus be Either ([GQLError],Maybe a) a:

1. Success - Right a
2. Partial Result with Errors - Left ([some error], Just a)
3. Error - Left ([some error], Nothing)

what do you think?

[AlistairB]

@nalchevanidze Yeah I think it is ok. It's fairly unclear to me the right modelling, partly because I am not that familiar with graphql.

For example, the 404/Not Found result is usually not considered exceptional with restful apis, so it makes me think it should be a Right. Although, you don't really know if the error is exceptional or not.

Anyway, I think your modelling is fine, but I would tweak it slightly.

1. looks good.
2. If Left always has at least one error, I would use NonEmpty ie. Left (NonEmpty errors, Just a)
3. Same here

There is also another case of parse failure, which I modelled with a sum type.

morpheus-graphql/morpheus-graphql-client/src/Data/Morpheus/Client/Internal/Types.hs

Lines 49 to 59 in b760851

	data FetchError
	= FetchParseFailure String
	| FetchNoResult GQLErrors
	deriving (Eq, Show)
	data FetchResult a
	= FetchResult {
	fetchResult :: a,
	fetchErrors :: [GQLError]
	}
	deriving (Eq, Show)

morpheus-graphql/morpheus-graphql-client/src/Data/Morpheus/Client/Fetch.hs

Line 62 in b760851

m (Either FetchError (FetchResult a))

is how I did it, which is slightly different in that you get a Right if the a is parsed. Happy to go with your style though.

[AlistairB]

@nalchevanidze so are you happy with the following?

fetch :: (Monad m, FromJSON a) => (ByteString -> m ByteString) -> Args a -> m (Either (FetchError a) a)

data FetchError a
  = FetchParseFailure String 
  | FetchProducedErrors (NonEmpty GQLError) (Maybe a)

[nalchevanidze]

@AlistairB yes. let try it