HTTP interface doesn't handle errors correctly

[xtian]

Graphqelm.Http decodes data and error responses using Json.Decode.oneOf with the data field handled first. This swallows errors in a response like this where someField is nullable:

{
  "data": {"someField": null},
  "errors": [{"path": ["someField"], "message": "Not Found"}]
}

Maybe an interface like this would work?

send : (Result HttpError (List GraphQlError, a) -> msg) -> Request (List GraphQlError, a) -> Cmd msg

toTask : Request (List GraphQlError, decodesTo) -> Task HttpError (List GraphQlError, decodesTo)

The other use-case to consider here is an operation with multiple queries or mutations where some succeed and some produce errors.

[dillonkearns]

Hey @xtian thanks for opening the issue. I thought about this when I wrote that code, but somehow it didn't seem like the obvious choice to ignore the data whenever an error occurs unconditionally, or to return both errors and data. Now that I think about it more, I think that maybe having multiple functions to handle these different strategies would be a good idea.

Apollo Client has a similar approach. They allow you to configure Apollo's error handling strategy so that when there is both success data and > 0 errors in the response, you can choose to have it:

1. Ignore the error and act exactly like a regular successful response (this is the current Graphqelm behavior)
2. Ignore the data and act exactly like a server error (perhaps this is a sensible default? It's hard to say, I'm not used to thinking about having both errors and a result in this kind of context so it's hard to know what the semantics should be)
3. Give you both the data and the errors

For example, the Graphqelm.Http module could provide the following send functions (and similar for toTask:

-- Function for option 2
-- This would be the default that most people would use and it would behave as you might expect
send : (Result Error decodesTo -> msg) -> Request decodesTo -> Cmd msg

-- Function for option 1
sendIgnoreErrorIfHasData : (Result Error decodesTo -> msg) -> Request decodesTo -> Cmd msg

-- Function for option 3
sendIncludeErrorsIfHasData :
    (Result Error ( decodesTo, List GraphqlError ) -> msg)
    -> Request decodesTo
    -> Cmd msg

Maybe these names could be more concise or expressive, but something like that. Another approach would be to have a union type to say which kind of request you want to make explicitly, but I think I like the idea of encouraging the best practice (and obvious behavior) without confusing configuration options, and then exposing the less common options if you need them so I think I prefer having separate functions as above.

I also like that this multiple functions approach doesn't clutter up the return types by having this error data that you can forget to check if you want it to just fail if there were errors.

What do you think of the multiple functions approach?

[xtian]

What do you think of the multiple functions approach?

I think that's a great idea! Would definitely work for our use-case

[dillonkearns]

Great! That feels like the right direction to me as well.

I wonder if it should be a major version bump? Because in a way this could be considered a breaking change.

[dillonkearns]

@xtian I'm thinking about merging these strategies into one. I've implemented this on master and I think this gives us the benefits of all of them pretty well. In a nutshell,

• If there are any errors in the response, the whole thing is treated as an error
• When there are errors in the response, the contents of data is always available as part of the data type (see the data type here)
• If the decoder for data succeeds, the Error will contain ParsedData decodesTo. Otherwise it will contain UnparsedData Json.Decode.Value

I like this strategy because it always allows you to use the data when there is an error if you'd like to (or see it for debugging), but it also makes it clear when there's been an error instead of blurring that line. And the standard use that works for most people is easy to find and to use.

What do you think of this approach?

[xtian]

That seems simpler to me. Sounds good!

[dillonkearns]

@xtian this has been published in Graphqelm Elm package version 11.0.0 (more details in the changelog). Let me know how this works for you. Thanks for reporting the issue!

[dillonkearns]

Hey @xtian I think this is resolved so I'll close this out, but please let me know if you have any issues or feedback. Thanks!

[xtian]

@dillonkearns Just got around to upgrading our app to 11.1.0. These changes work great. Thanks so much!

[dillonkearns]

Fantastic, glad to hear it!