Graphql-shield: Wrong exception handling

[jahudka]

Bug report

• [x] I have checked other issues to make sure this is not a duplicate.

Describe the bug

Errors that you wish to pass on should be _thrown_, not _returned_.

If you set allowExternalErrors to true, graphql-shield will _return_ any errors it catches inside rules or resolvers instead of throwing them. This is wrong. I cannot overstate this: RETURNING ERRORS IS WRONG. Middlewares usually work with the assumption that if a resolver _returns_ a value, then all is well, the current middleware should just do its own thing and pass back whatever result it arrives at; it usually doesn't check the returned value in case it was an Error object, because errors should be thrown. That's why we have try { .. } catch { .. }.

The only way to obtain the _correct_ behaviour is to set the debug option to true. The only effect of this setting is that errors caught inside graphql-shield are thrown. Essentially what allowExternalErrors should do.

To Reproduce

• configure graphql-shield with allowExternalErrors: true
• configure another middleware _above_ graphql-shield which expects try { .. } catch { .. } to be a thing
• throw an error from within a resolver
• weep

Expected behavior

I would expect healthy and sane programming patterns to be promoted.

Actual behaviour

I weep.

Additional context

Sorry if I'm being too much of a dick, but this really pissed me off because it's such a counterintuitive thing to look for and broke my app down bad and probably cost my boss money.. so I guess I got worked up a little.

[maticzav]

Make a PR. I’ll review it, maybe even merge. Please read the docs and find related issues.

Please learn to behave in the open source community - I wasn’t paid to create GraphQL Shield, and I am not paid to read your “dicky” comments. I and many other people from the community have spent considerable time pondering on how to handle errors. Please thank your boss for sponsoring the project.

[jahudka]

@maticzav, I'm sorry about being a dick. Really. I got carried away. I'm the author and maintainer of multiple open source projects and I know how much it sucks to read stuff like that. Please forgive me for taking out my accumulated frustration on you. I respect and value the time you and others have invested in making GraphQL Shield, even if I don't agree with some of the design decisions.

I tried forking the repo and making a PR to add the option to throw errors instead of returning them, but I abandoned the attempt when I realised there are probably consequences I can't foresee with my limited insight into the design of the library. If you ever feel like adding it (or something like it) yourself, I'll be much obliged.

If you want to better understand the mindset in which I wrote the initial issue, I think this article sums it up rather nicely (I'm not the author, but I think it's spot-on). Read it, you'll laugh, you'll weep and you'll be quoting it to friends years from now.

[maticzav]

Thank you for the article.