strawberry-graphql / strawberry

A GraphQL library for Python that leverages type annotations 🍓
https://strawberry.rocks
MIT License
3.92k stars 516 forks source link

Add `process_errors` hook to extensions #1155

Open patrick91 opened 3 years ago

patrick91 commented 3 years ago

It could enable us to ship plugins like this: https://github.com/dotansimha/envelop/blob/94db02d9ecabcf2b77f7694ad1495db644b77965/packages/core/src/plugins/use-masked-errors.ts

See also the conversation on discord: https://discord.com/channels/689806334337482765/750088532479180840/877569440114430002

This might be better than doing https://github.com/strawberry-graphql/strawberry/issues/1154

Upvote & Fund

Fund with Polar

jkimbo commented 2 years ago

So I'm not sure that adding a process_errors hook is necessary because you can just do it in the on_request_end hook we currently have. That also gives you an easy way to modify or hide errors that you don't want to show end users. I created a MaskErrors extension that does exactly that in issue #1221 and it's pretty straightforward: https://github.com/strawberry-graphql/strawberry/issues/1221#issuecomment-917473167

I was going to try and create a PR to add that extension to the library however I came across an issue: the extension would run before the process_errors function on the schema so the masked messages would be logged instead of the real ones. I think I see 3 different options to deal with this and I'd appreciate any opinions on which one to go with:

  1. Follow the proposal in this issue and add an on_errors or process_errors hook to extensions that will run after the process_errors function on the schema
  2. Remove the logging in the default process_errors function on the schema and log all errors before the end of the on_request_end hook. Encourage that errors be dealt with through extensions only and eventually remove the process_errors function. To hide errors in logs you would have to silence the logger.
  3. Remove the process_errors function on the schema and add a LogErrors extension. Nothing would happen to errors by default, you would have to add the LogErrors extension to get the default behaviour back.

I’m leaning towards option 2 at the moment because I think it's cleaner. Generally I’m thinking that we should deal with schema customisation through extensions only and not by subclassing the schema.

Any thoughts? I’ve probably missed some options here so would be interested in any other suggestions.

jkimbo commented 2 years ago

Actually there is a 4th option which is to just run the process_errors function before the end of the on_request extension hook. It's not the nicest solution code wise but its the least disruptive one. Probably should just do that.