Error masking and internals leaking in error handling
ods opened this issue · 3 comments
There are several places (execute_graphql(), complete_value(), complete_value_catching_error(), resolve_or_error(), execute() ExecutionContext.report_error(), may be there is more) where the library indiscriminately catches all exceptions and reports them to client. It's correct behaviour for parsing/usage errors. But for programming and runtime errors there are problems:
- original error is lost and traceback is not reported, so it becomes hard to debug;
- potentially sensitive information is leaked to client via error message.
I believe the the right behaviour would be to catch and report to client specific exceptions only (GraphQLError and subclasses?) while propagating the rest.
Related issues:
This has been bugging me a lot, since I want to raise and catch my own Exception subclasses for some occasions in my app. This library currently makes this impossible, without changing the code in the library itself too.
Using except Exception as e has pretty much never been a good idea :/ But looking through the code, I see it pop up in a lot of places, so I wasn't sure where to start. @ods I saw that you changed a lot (maybe all) of it in your linked commit f81ca08. Did this fix it completely for you?
Did this fix it completely for you?
Yes, it did so far. There are actually two commits in my fork: second is to silence annoying noise in logs. I'm going to issue a pull request after some time of using it in our project.