Allow subscription config errors to return spec compliant errors

[bryanjos]

Currently subscription config functions expect either {:ok, config} or {:error, msg} to be returned. In the error case, this is what's returned ultimately:

%{errors: [%{message: msg}]}

We would like to send spec compliant errors with extra information from the config errors.

Right now if we return the following from config

{:error, %{extensions: %{code: "FAILED"}, message: "failed"}}

We get

%{errors: [%{message: %{extensions: %{code: "FAILED"}, message: "failed"}}]}

Which breaks some tools that expect spec compliant errors.

There are a couple of ways we've thought about fixing it and could use some feedback before making a PR.

The first way would be to update Absinthe.Phase.Document.Result to look for a map in the message property of the Absinthe.Phase.Error struct created and return what's there as the error.

So returning {:error, %{extensions: %{code: "FAILED"}, message: "failed"} from the config function would give

%{errors: [%{extensions: %{code: "FAILED"}, message: "failed"}]}

The second way we were thinking is updating Absinthe.Phase.Subscription.SubscribeSelf to let config return a 3-tuple, {:error, msg, extra} and put the extra there onto the extra property of the Absinthe.Phase.Error struct created.

With that, and with spec_compliant_errors set to true, if {:error, "failed", %{code: "FAILED"} is returned, you would get the above as well.

Or some other way we aren't thinking of.

[michaelcaterisano]

Thanks @bryanjos! We've already discussed, but I'll add my thoughts here as well.

I encountered unexpected behavior with config/2 when I returned {:error, map}, assuming its api would be the same as a resolver. Spoiler, it is not! The second element of the tuple is wrapped in a map, placed on a message key.

My take is that the config/2 function's API should match the resolver API. That is, it should support the same kind of error tuples. Currently, resolvers allow returning {:error, "message"} or {:error, map}, where map is %{message: "message", extensions: %{}}. https://github.com/absinthe-graphql/absinthe/commit/32db847a137834e618d6f66751b1fa3bcdc4a5fe introduced a spec_compliant_errors option, which, when set to true, creates a map like the one above, with "extra errors" being added to the extensions key. I think the idea is for Absinthe to help ensure that errors are spec compliant, but as the PR mentions, the change needs to be opt-in because it's breaking. I also wasn't able to find this option in the docs, but maybe I missed it.

IMO, while this option is convenient, it's a bit surprising that Absinthe reformats errors in this way. A simpler approach would be to make the config/2 function's api match a resolver's api, so that there's one way to return errors to clients. This means developers are responsible for returning spec-compliant errors, but that seems reasonable to me. It would also be a non-breaking change.

If folks prefer the spec_compliant_errors option route, we should make sure it's well documented.

@maartenvanvliet I know it's been a while since this PR, but curious if you have opinions here.

[bryanjos]

Knowing the history now, I think I'd back what @michaelcaterisano suggests.

[bryanjos]

Made a PR #1341