Error on config results in `Error: [object Object]`

[erikreedstrom]

Issue

When attempting to return unauthorized from a config macro, the socket impl throws an error of the stringified errors map.

Although onSubscriptionResponse maps errors, if an error map is returned on join, the error is not handled correctly.

Actual

{:error, :unauthorized}
[debug] -- Absinthe Phoenix Reply --
{:error, %{errors: [%{locations: [%{column: 0, line: 2}], message: :unauthorized}]}}

Results in the following for GraphiQL

Error: [object Object]
    at ...

Expected

Error maps are handled properly using gqlErrorsToString

[erikreedstrom]

The issue is here: https://github.com/absinthe-graphql/absinthe-socket/blob/master/packages/socket/src/pushRequest.js#L43

It seems this should check to see if the response is a map or a string, and call gqlErrorsToString in case of the former.

Possibly related to #20

[southerneer]

This is definitely related to #20 (submitted by my colleague @aksonov). We were using his fork created for that PR and after updating to v0.2.0 we're seeing the same behavior.

[southerneer]

I'm trying to create PR that addresses the problem, but I'm new to rollup, prepack, etc. Any chance we could get some contribution guidelines for this repo?

[mgtitimoli]

Below is the comment I wrote in #39:

Message must be string by spec.

We could consider adding support to extensions.

[pietro909]

Would you consider a simple solution like wrapping the failing code in a try {} catch as a valid PR right now? At least it would avoid the runtime error.

[heydtn]

I just ran into this error as well, after quite a bit of digging and debugging, I don't think it looks like this should be returning just an Error string. The errorMessage variable doesn't actually contain a message, it contains a response body, and the name responseData or something might be better. It looks like the logic here affects socket-apollo-link and causes an incompatibility with the API that the apollo-link libraries use.

I discovered this when I attempted to manage errors within my application by using the pretty standard { errors: [{...}] } object that gets returned from a query, and it turns out that's not possible. Similarly, I attempted to use apollo-link-error, and my onError method is actually being passed a big string with the error message casted to [object Object] within it instead of a result object as expected.

This should likely be returning a payload of { errors: [{...}] } instead of a stringified version of it in the case of a graphQL error, to allow consumers of the socket data to properly digest the error messages.

In the case of apollo, other apollo-link libraries take in data like this. apollo-link-error handles their return values here. The graphQLErrors field in apollo-link-error relies on the error returned having a value of: { result: { errors: [{...}] } }

Similarly, this logic takes this object and uses it to populate the response field, which passes down and allows for the errors to be returned to the user application for further processing.

I made a quick adjustment just for the purposes of local testing to pushRequestUsing.js line 42.

if (errorMessage && typeof errorMessage === 'object') {
   return abortNotifier(absintheSocket, notifier, { result: errorMessage });
 } else {
   return abortNotifier(absintheSocket, notifier, createRequestError(errorMessage));
}

This works as expected with apollo, successfully returning the data to the application while being compatible with apollo-link. I feel like other non-apollo clients would also be expecting the socket to return a standard { errors: [{...}] } response. Perhaps this can return the { errors: [{...}] }, and then socket-apollo-link could wrap it in a { result: { errors: [{...}] } } structure? Would this cause any issues that I haven't identified?

[heydtn]

Just as a side note/update to my previous comment, it looks like apollo-link-http-comon returns an error object with a message and all the relevant data added onto it https://github.com/apollographql/apollo-link/blob/d56fce6f4115dd1d5b304a3593cb69770c971d2b/packages/apollo-link-http-common/src/index.ts#L113. Perhaps a similar pattern could be used here, which would allow for a backwards-compatible message to be provided, while also providing the necessary extra data for consumers of the error?

[arjan]

Any update on this ticket? It would be really helpful if this gets fixed one way or another. Willing to help!

[dorian-marchal]

Below is the comment I wrote in #39:

Message must be string by spec. We could consider adding support to extensions.

The spec says:

The errors entry in the response is a non‐empty list of errors, where each error is a map.

If no errors were encountered during the requested operation, the errors entry should not be present in the result.

[...]

Every error must contain an entry with the key message with a string description of the error intended for the developer as a guide to understand and correct the error.

If an error can be associated to a particular point in the requested GraphQL document, it should contain an entry with the key locations with a list of locations, where each location is a map with the keys line and column, both positive numbers starting from 1 which describe the beginning of an associated syntax element.

Which is what the parent sends (assuming atoms are serialized into strings):

{:error, %{errors: [%{locations: [%{column: 0, line: 2}], message: :unauthorized}]}}

So this looks like a bug in Absinthe Socket. This prevents proper error handling as there is no way to access the original error data later in the pipeline.

I cannot think of a workaround to avoid that as createAbsintheSocketLink returns a terminating link so there is no way to change the error format on front side before it is parsed by Absinthe Socket.

Also, Apollo Client's Http Link handles these errors properly so it's surprising for users adding support for subscriptions to their app: GraphQL errors over HTTP are handled properly but the get an [object Object] over websocket.

[alexandrchebotar]

It feels like @absinthe/socket shouldn't handle response with errors and should pass it to link as is.

For example, here the errors received by websocket:

Due to apollo docs I expect to see these two errors errors in graphQLErrors array.

If the query produces one or more errors, this object contains either an array of graphQLErrors or a single networkError. Otherwise, this value is undefined.

But I got networkError with no payload instead:

I guess for correct errors handling apollo needs result with errors. The other links, for example, apollo-logger-link also expects errors array in result. But they never receive it because @absinthe/socket converts array of graphQLErrors into a single networkError with ugly message.

[matrinox]

Is there any update on this? I made a PR that aims to at least provide useful error messages. But I agree with @alexandrchebotar that the preferred method is probably for @absinthe/socket to not create a new error object so it can be compatible with other links

I also noticed someone made a helpful commit to @absinthe/socket to add the original error object to the new error object in the object key. That commit was merged 2 years ago but there's been no new nom release since. Will there be a new release soon?

[arjan]

I am waiting for a new release for ages now, it seems this repo has become unmaintained..

[matrinox]

@mgtitimoli I'm happy to contribute to this project and create new releases. Is that something your team would be open to?

[arjan]

:+1: I guess we have to ping the maintainers? @mgtitimoli @tlvenn @bruce

[mgtitimoli]

In the coming days, I will be sending an update that automates the release process, so we can start moving the engines again.

[mgtitimoli]

However regarding to this specific topic, the spec says the message must be string, and there is an extensions field to provide additional context about the error.

[dorian-marchal]

However regarding to this specific topic, the spec says the message must be string, and there is an extensions field to provide additional context about the error.

The spec does say that message should be a string, but errors should be a list of maps:

The errors entry in the response is a non‐empty list of errors, where each error is a map.

This is actually what Absinthe Socket should be returning, something like:

{errors: [{locations: [{column: 0, line: 2}], message: "unauthorized"}]}

I don't think we need extensions for that because the received error is already properly formatted by Absinthe. Absinthe Socket just has to forward the errors map as-is, without converting it to a string. This is how queries and mutations already work, for example.

Currently, the error is lost and there is no way for applications to get it (e.g. to log it or send it to an error tracking platform like Sentry).

WDYT?