Response errors should result in a Left of error#578
Response errors should result in a Left of error#578nalchevanidze merged 1 commit intomorpheusgraphql:masterfrom
Conversation
| processResponse JSONResponse {responseData = Just x} = Right x | ||
| processResponse JSONResponse {responseData = Just x, responseErrors = Nothing} = Right x | ||
| processResponse JSONResponse {responseData = Just x, responseErrors = Just []} = Right x | ||
| processResponse invalidResponse = Left (show invalidResponse) |
There was a problem hiding this comment.
what do you think, maybe we can use JSON "decode" instead of regular show?
There was a problem hiding this comment.
will provide easier understandable error messages.
There was a problem hiding this comment.
So you might do something like:
processResponse JSONResponse {responseData = Just x, responseErrors = Nothing} = Right x
processResponse JSONResponse {responseData = Just x, responseErrors = Just []} = Right x
processResponse JSONResponse {responseErrors = err} = Left $ decodeUtf8 $ encode errbut you can't do
processResponse invalidResponse = Left $ decodeUtf8 $ encode invalidResponseAs JSONResponse has an a and based on the Fetch typeclass we don't know that we have ToJSON a constraint. So you would need to either add the ToJSON a constraint, which maybe the user doesn't want to provide. Or exclude the responseData from the error message.
Based on this issue I think the current implementation is fine. Also, I think unless you prettify the json, a json error message is not too different to a show error message. What do you think?
There was a problem hiding this comment.
If you wanted to improve this more, I think the right solution would be to have fetch return a m (Either FetchFailure a) where (roughly sketched)
data FetchFailure =
FetchConnectionIssue String
| ResponseDecodeFailure String
| ServerGQLErrors (NonEmpty GQLError)
deriving (Show, ToJSON)This gives the user structured data they can work with if they want, or they can just show / render to JSON. I'd suggest this be done as a separate issue/PR though.
|
@AlistairB thanks, i leaved some comments. they are proposals, but if you think same way, we could implement them. |
|
@nalchevanidze I've done 2/3 and left some comments on the other suggestion. Let know what you think. Thanks. |
3bdea10 to
fa72731
Compare
|
@nalchevanidze I had a bit of time so I thought I'd implement something like #577 (comment) The missing piece is even though the {
"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'."
}
]
}I'd like to be able to get the I think to do this I'd like to update data FetchGQLError = FetchGQLError {
fetchError :: GQLError,
rawError :: HashMap Text Value
}When you aren't busy with your thesis, I'm curious what you think of what I have implemented and this addition. Thanks (I'll probably have a crack at this addition in a week or so) |
90fe2a2 to
7086bf5
Compare
|
@nalchevanidze I have updated the PR based on #577 (comment) . The only difference is I added another case to Oh I also updated the stackage to use the latest LTS, I can revert if you want. EDIT: This PR does not solve the problem of exposing additional custom fields in the errors, but I think that should be addressed with a separate PR. |
|
@AlistairB sorry for long delay . there is some merge conflicts when you resolve them feel free to merge it (is approved!). yeah. would be great if you could also update change-log :) |
57efdfd to
5325359
Compare
There was a problem hiding this comment.
Thanks for the approval @nalchevanidze . I have rebased and updated this PR.
There was one area I was uncertain about which I have left a comment. Also the tests for morpheus-graphql-app are failing in master, but otherwise I think everything is passing. (And the Deploy Docs build is failing, apparently unrelated to the changes).
5325359 to
117fd6c
Compare
morpheus-graphql-core/src/Data/Morpheus/Types/Internal/AST/Error.hs
Outdated
Show resolved
Hide resolved
Even when the response `a` can still be successfully parsed. Add a new `FetchError` type to cover all the possible fetch failure scenarios.
117fd6c to
5dd8ea4
Compare
Even when the response
acan still be successfully parsed.Closes #577